Re: [Qemu-devel] [PATCH v2 0/3] require newer glib2 to enable autofree'ing of stack variables exiting scope

[Marc-André Lureau]

On Thu, Jul 25, 2019 at 12:44 PM Daniel P. Berrangé <address@hidden> wrote:
>
> Both GCC and CLang support a C extension attribute((cleanup)) which
> allows you to define a function that is invoked when a stack variable
> exits scope. This typically used to free the memory allocated to it,
> though you're not restricted to this. For example it could be used to
> unlock a mutex.
>
> We could use that functionality now, but the syntax is a bit ugly in
> plain C. Since version 2.44 of GLib, there have been a few macros to
> make it more friendly to use - g_autofree, g_autoptr and
> G_DEFINE_AUTOPTR_CLEANUP_FUNC.
>
>   https://developer.gnome.org/glib/stable/glib-Miscellaneous-Macros.html
>
>   https://blogs.gnome.org/desrt/2015/01/30/g_autoptr/
>
> The key selling point is that it simplifies the cleanup code paths,
> often eliminating the need to goto cleanup labels. This improves
> the readability of the code and makes it less likely that you'll
> leak memory accidentally.
>
> Inspired by seeing it added to glib, and used in systemd, Libvirt
> finally got around to adopting this in Feb 2019. Overall our
> experience with it has been favourable/positive, with the code
> simplification being very nice.
>
> The main caveats with it are
>
>  - Only works with GCC or CLang. We don't care as those are
>    the only two compilers we declare support for.
>
>  - You must always initialize the variables when declared
>    to ensure predictable behaviour when they leave scope.
>    Chances are most methods with goto jumps for cleanup
>    are doing this already
>
>  - You must not directly return the value that's assigned
>    to a auto-cleaned variable. You must steal the pointer
>    in some way. ie
>
>     BAD:
>         g_autofree char *wibble = g_strdup("wibble")
>         ....
>         return wibble;
>
>     GOOD:
>         g_autofree char *wibble = g_strdup("wibble")
>         ...
>         return g_steal_pointer(wibble);
>
>     g_steal_pointer is an inline function which simply copies
>     the pointer to a new variable, and sets the original variable
>     to NULL, thus avoiding cleanup.
>
> I've illustrated the usage by converting a bunch of the crypto code in
> QEMU to use auto cleanup.
>
> Changed on v2:
>
>  - Actually commit the rest of the changes to patch 3 so that what's
>    posted works :-) Sigh.
>
> Daniel P. Berrangé (3):
>   glib: bump min required glib library version to 2.48
>   crypto: define cleanup functions for use with g_autoptr
>   crypto: use auto cleanup for many stack variables

Series:
Reviewed-by: Marc-André Lureau <address@hidden>

>
>  configure                   |  2 +-
>  crypto/afsplit.c            | 28 ++++----------
>  crypto/block-luks.c         | 74 +++++++++++--------------------------
>  crypto/block.c              | 15 +++-----
>  crypto/hmac-glib.c          |  5 ---
>  crypto/pbkdf.c              |  5 +--
>  crypto/secret.c             | 38 ++++++++-----------
>  crypto/tlscredsanon.c       | 16 +++-----
>  crypto/tlscredspsk.c        |  5 +--
>  crypto/tlscredsx509.c       | 16 +++-----
>  include/crypto/block.h      |  2 +
>  include/crypto/cipher.h     |  2 +
>  include/crypto/hmac.h       |  2 +
>  include/crypto/ivgen.h      |  2 +
>  include/crypto/tlssession.h |  2 +
>  include/glib-compat.h       | 42 +--------------------
>  16 files changed, 78 insertions(+), 178 deletions(-)
>
> --
> 2.21.0
>
>


-- 
Marc-André Lureau