diff mbox series

[3/4] docs: document use of automatic cleanup functions in glib

Message ID 20190823163931.7442-4-berrange@redhat.com (mailing list archive)
State New, archived
Headers show
Series docs: add docs about use of automatic cleanup functions | expand

Commit Message

Daniel P. Berrangé Aug. 23, 2019, 4:39 p.m. UTC 
Document the use of g_autofree and g_autoptr in glib for automatic
freeing of memory, or other resource cleanup (eg mutex unlocking).

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 CODING_STYLE.md | 101 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 101 insertions(+)

Comments

Eric Blake Aug. 23, 2019, 7:53 p.m. UTC | #1 
On 8/23/19 11:39 AM, Daniel P. Berrangé wrote:
> Document the use of g_autofree and g_autoptr in glib for automatic
> freeing of memory, or other resource cleanup (eg mutex unlocking).
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  CODING_STYLE.md | 101 ++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 101 insertions(+)


> +The cleanup functions are not restricted to simply free'ing memory. The
> +GMutexLocker class is a variant of GMutex that has automatic locking and
> +unlocking at start and end of the enclosing scope
> +
> +In the following example, the `lock` in `MyObj` will be held for the
> +precise duration of the `somefunc` function
> +
> +    typedef struct {
> +        GMutex lock;
> +    } MyObj;
> +
> +    char *somefunc(MyObj *obj) {
> +        g_autofree GMutexLocker *locker = g_mutex_locker_new(&obj->lock)

Wrong example (you don't want to call g_free, and you missed ';'). This
should be

g_autoptr (GMutexLocker) locker = g_mutex_locker_new(&obj->lock);

With that fixed,

Reviewed-by: Eric Blake <eblake@redhat.com>
Stefan Hajnoczi Aug. 28, 2019, 9 a.m. UTC | #2 
On Fri, Aug 23, 2019 at 05:39:30PM +0100, Daniel P. Berrangé wrote:
> Document the use of g_autofree and g_autoptr in glib for automatic
> freeing of memory, or other resource cleanup (eg mutex unlocking).
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  CODING_STYLE.md | 101 ++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 101 insertions(+)

With Eric's suggestion:

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Alex Bennée Aug. 28, 2019, 3:14 p.m. UTC | #3 
Daniel P. Berrangé <berrange@redhat.com> writes:

> Document the use of g_autofree and g_autoptr in glib for automatic
> freeing of memory, or other resource cleanup (eg mutex unlocking).
>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  CODING_STYLE.md | 101 ++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 101 insertions(+)
>
> diff --git a/CODING_STYLE.md b/CODING_STYLE.md
> index 9f4fc9dc77..f37b6c2d01 100644
> --- a/CODING_STYLE.md
> +++ b/CODING_STYLE.md
> @@ -479,3 +479,104 @@ terminate QEMU.
>
>  Note that &error_fatal is just another way to exit(1), and &error_abort
>  is just another way to abort().
> +
> +
> +## Automatic memory deallocation
> +
> +QEMU has a mandatory dependency either the GCC or CLang compiler. As
> +such it has the freedom to make use of a C language extension for
> +automatically running a cleanup function when a stack variable goes
> +out of scope. This can be used to simplify function cleanup paths,
> +often allowing many goto jumps to be eliminated, through automatic
> +free'ing of memory.
> +
> +The GLib2 library provides a number of functions/macros for enabling
> +automatic cleanup:
> +
> + https://developer.gnome.org/glib/stable/glib-Miscellaneous-Macros.html
> +
> +Most notably:
> +
> + - g_autofree - will invoke g_free() on the variable going out of scope
> +
> + - g_autoptr - for structs / objects, will invoke the cleanup func created
> +               by a previous use of G_DEFINE_AUTOPTR_CLEANUP_FUNC. This is
> +               supported for most GLib data types and GObjects
> +
> +For example, instead of
> +
> +    int somefunc(void) {
> +        int ret = -1;
> +        char *foo = g_strdup_printf("foo%", "wibble");
> +        GList *bar = .....
> +
> +        if (eek) {
> +           goto cleanup;
> +        }
> +
> +        ret = 0;
> +
> +      cleanup:
> +        g_free(foo);
> +        g_list_free(bar);
> +        return ret;
> +    }
> +
> +Using g_autofree/g_autoptr enables the code to be written as:
> +
> +    int somefunc(void) {
> +        g_autofree char *foo = g_strdup_printf("foo%", "wibble");
> +        g_autoptr (GList) bar = .....
> +
> +        if (eek) {
> +           return -1;
> +        }
> +
> +        return 0;
> +    }
> +
> +While this generally results in simpler, less leak-prone code, there
> +are still some caveats to beware of
> +
> + * Variables declared with g_auto* MUST always be initialized,
> +   otherwise the cleanup function will use uninitialized stack memory
> +
> + * If a variable declared with g_auto* holds a value which must
> +   live beyond the life of the function, that value must be saved
> +   and the original variable NULL'd out. This can be simpler using
> +   g_steal_pointer
> +
> +
> +    char *somefunc(void) {
> +        g_autofree char *foo = g_strdup_printf("foo%", "wibble");
> +        g_autoptr (GList) bar = .....
> +
> +        if (eek) {
> +           return NULL;
> +        }
> +
> +        return g_steal_pointer(&foo);
> +    }

All good so far.

> +
> +The cleanup functions are not restricted to simply free'ing memory. The
> +GMutexLocker class is a variant of GMutex that has automatic locking and
> +unlocking at start and end of the enclosing scope
> +
> +In the following example, the `lock` in `MyObj` will be held for the
> +precise duration of the `somefunc` function
> +
> +    typedef struct {
> +        GMutex lock;
> +    } MyObj;
> +
> +    char *somefunc(MyObj *obj) {
> +        g_autofree GMutexLocker *locker = g_mutex_locker_new(&obj->lock)
> +        g_autofree char *foo = g_strdup_printf("foo%", "wibble");
> +        g_autoptr (GList) bar = .....
> +
> +        if (eek) {
> +           return NULL;
> +        }
> +
> +        return g_steal_pointer(&foo);
> +    }

I would personally prefer we get some RFC patches for auto-unlocking under our
belt before we codify it's usage in our developer docs. Locking is a
fickle beast at the best of times and I'd like to see where it benefits
us before there is a rush to covert to the new style.

For one thing the only uses I see of g_mutex_lock is in our tests, the
main code base uses qemu_mutex_lock. How would we go about registering
the clean-up functions for those in the code base?

But apart from the lock stuff:

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

--
Alex Bennée
Daniel P. Berrangé Aug. 28, 2019, 3:20 p.m. UTC | #4 
On Wed, Aug 28, 2019 at 04:14:00PM +0100, Alex Bennée wrote:
> > +The cleanup functions are not restricted to simply free'ing memory. The
> > +GMutexLocker class is a variant of GMutex that has automatic locking and
> > +unlocking at start and end of the enclosing scope
> > +
> > +In the following example, the `lock` in `MyObj` will be held for the
> > +precise duration of the `somefunc` function
> > +
> > +    typedef struct {
> > +        GMutex lock;
> > +    } MyObj;
> > +
> > +    char *somefunc(MyObj *obj) {
> > +        g_autofree GMutexLocker *locker = g_mutex_locker_new(&obj->lock)
> > +        g_autofree char *foo = g_strdup_printf("foo%", "wibble");
> > +        g_autoptr (GList) bar = .....
> > +
> > +        if (eek) {
> > +           return NULL;
> > +        }
> > +
> > +        return g_steal_pointer(&foo);
> > +    }
> 
> I would personally prefer we get some RFC patches for auto-unlocking under our
> belt before we codify it's usage in our developer docs. Locking is a
> fickle beast at the best of times and I'd like to see where it benefits
> us before there is a rush to covert to the new style.
> 
> For one thing the only uses I see of g_mutex_lock is in our tests, the
> main code base uses qemu_mutex_lock. How would we go about registering
> the clean-up functions for those in the code base?

Ideally we could just relpace qemu_mutex with g_mutex, but if that's
not possible we would have to create a clone of GMutexLocker as
QemuMutexLocker doing exactly the same thing. It is a shame to reinvent
the wheel with our threading code though.

/me tries to remember what it was that we can do with QEMU's threads
that we can't do with GLib's threads.

Regards,
Daniel
Alex Bennée Aug. 28, 2019, 4:04 p.m. UTC | #5 
Daniel P. Berrangé <berrange@redhat.com> writes:

> On Wed, Aug 28, 2019 at 04:14:00PM +0100, Alex Bennée wrote:
>> > +The cleanup functions are not restricted to simply free'ing memory. The
>> > +GMutexLocker class is a variant of GMutex that has automatic locking and
>> > +unlocking at start and end of the enclosing scope
>> > +
>> > +In the following example, the `lock` in `MyObj` will be held for the
>> > +precise duration of the `somefunc` function
>> > +
>> > +    typedef struct {
>> > +        GMutex lock;
>> > +    } MyObj;
>> > +
>> > +    char *somefunc(MyObj *obj) {
>> > +        g_autofree GMutexLocker *locker = g_mutex_locker_new(&obj->lock)
>> > +        g_autofree char *foo = g_strdup_printf("foo%", "wibble");
>> > +        g_autoptr (GList) bar = .....
>> > +
>> > +        if (eek) {
>> > +           return NULL;
>> > +        }
>> > +
>> > +        return g_steal_pointer(&foo);
>> > +    }
>>
>> I would personally prefer we get some RFC patches for auto-unlocking under our
>> belt before we codify it's usage in our developer docs. Locking is a
>> fickle beast at the best of times and I'd like to see where it benefits
>> us before there is a rush to covert to the new style.
>>
>> For one thing the only uses I see of g_mutex_lock is in our tests, the
>> main code base uses qemu_mutex_lock. How would we go about registering
>> the clean-up functions for those in the code base?
>
> Ideally we could just relpace qemu_mutex with g_mutex, but if that's
> not possible we would have to create a clone of GMutexLocker as
> QemuMutexLocker doing exactly the same thing. It is a shame to reinvent
> the wheel with our threading code though.
>
> /me tries to remember what it was that we can do with QEMU's threads
> that we can't do with GLib's threads.

Apart from having separate POSIX and Win32 implementations we have also
extended the mutex handling to add trace points and also support
profiling of lock latency.

>
> Regards,
> Daniel


--
Alex Bennée
diff mbox series

Patch

diff --git a/CODING_STYLE.md b/CODING_STYLE.md
index 9f4fc9dc77..f37b6c2d01 100644
--- a/CODING_STYLE.md
+++ b/CODING_STYLE.md
@@ -479,3 +479,104 @@  terminate QEMU.
 
 Note that &error_fatal is just another way to exit(1), and &error_abort
 is just another way to abort().
+
+
+## Automatic memory deallocation
+
+QEMU has a mandatory dependency either the GCC or CLang compiler. As
+such it has the freedom to make use of a C language extension for
+automatically running a cleanup function when a stack variable goes
+out of scope. This can be used to simplify function cleanup paths,
+often allowing many goto jumps to be eliminated, through automatic
+free'ing of memory.
+
+The GLib2 library provides a number of functions/macros for enabling
+automatic cleanup:
+
+ https://developer.gnome.org/glib/stable/glib-Miscellaneous-Macros.html
+
+Most notably:
+
+ - g_autofree - will invoke g_free() on the variable going out of scope
+
+ - g_autoptr - for structs / objects, will invoke the cleanup func created
+               by a previous use of G_DEFINE_AUTOPTR_CLEANUP_FUNC. This is
+               supported for most GLib data types and GObjects
+
+For example, instead of
+
+    int somefunc(void) {
+        int ret = -1;
+        char *foo = g_strdup_printf("foo%", "wibble");
+        GList *bar = .....
+
+        if (eek) {
+           goto cleanup;
+        }
+
+        ret = 0;
+
+      cleanup:
+        g_free(foo);
+        g_list_free(bar);
+        return ret;
+    }
+
+Using g_autofree/g_autoptr enables the code to be written as:
+
+    int somefunc(void) {
+        g_autofree char *foo = g_strdup_printf("foo%", "wibble");
+        g_autoptr (GList) bar = .....
+
+        if (eek) {
+           return -1;
+        }
+
+        return 0;
+    }
+
+While this generally results in simpler, less leak-prone code, there
+are still some caveats to beware of
+
+ * Variables declared with g_auto* MUST always be initialized,
+   otherwise the cleanup function will use uninitialized stack memory
+
+ * If a variable declared with g_auto* holds a value which must
+   live beyond the life of the function, that value must be saved
+   and the original variable NULL'd out. This can be simpler using
+   g_steal_pointer
+
+
+    char *somefunc(void) {
+        g_autofree char *foo = g_strdup_printf("foo%", "wibble");
+        g_autoptr (GList) bar = .....
+
+        if (eek) {
+           return NULL;
+        }
+
+        return g_steal_pointer(&foo);
+    }
+
+The cleanup functions are not restricted to simply free'ing memory. The
+GMutexLocker class is a variant of GMutex that has automatic locking and
+unlocking at start and end of the enclosing scope
+
+In the following example, the `lock` in `MyObj` will be held for the
+precise duration of the `somefunc` function
+
+    typedef struct {
+        GMutex lock;
+    } MyObj;
+
+    char *somefunc(MyObj *obj) {
+        g_autofree GMutexLocker *locker = g_mutex_locker_new(&obj->lock)
+        g_autofree char *foo = g_strdup_printf("foo%", "wibble");
+        g_autoptr (GList) bar = .....
+
+        if (eek) {
+           return NULL;
+        }
+
+        return g_steal_pointer(&foo);
+    }