[PATCH] qapi: enable use of g_autoptr with QAPI types

[PATCH] qapi: enable use of g_autoptr with QAPI types

[Daniel P. Berrangé]

Currently QAPI generates a type and function for free'ing it:

  typedef struct QCryptoBlockCreateOptions QCryptoBlockCreateOptions;
  void qapi_free_QCryptoBlockCreateOptions(QCryptoBlockCreateOptions *obj);

This is used in the traditional manner:

  QCryptoBlockCreateOptions *opts = NULL;

  opts = g_new0(QCryptoBlockCreateOptions, 1);

  ....do stuff with opts...

  qapi_free_QCryptoBlockCreateOptions(opts);

Since bumping the min glib to 2.48, QEMU has incrementally adopted the
use of g_auto/g_autoptr. This allows the compiler to run a function to
free a variable when it goes out of scope, the benefit being the
compiler can guarantee it is freed in all possible code ptahs.

This benefit is applicable to QAPI types too, and given the seriously
long method names for some qapi_free_XXXX() functions, is much less
typing. This change thus makes the code generator emit:

 G_DEFINE_AUTOPTR_CLEANUP_FUNC(QCryptoBlockCreateOptions,
                              qapi_free_QCryptoBlockCreateOptions)

The above code example now becomes

  g_autoptr(QCryptoBlockCreateOptions) opts = NULL;

  opts = g_new0(QCryptoBlockCreateOptions, 1);

  ....do stuff with opts...

Note, if the local pointer needs to live beyond the scope holding the
variable, then g_steal_pointer can be used. This is useful to return the
pointer to the caller in the success codepath, while letting it be freed
in all error codepaths.

  return g_steal_pointer(&opts);

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 include/crypto/block.h | 2 --
 scripts/qapi/types.py  | 1 +
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/include/crypto/block.h b/include/crypto/block.h
index d274819791..7a65e8e402 100644
--- a/include/crypto/block.h
+++ b/include/crypto/block.h
@@ -311,7 +311,5 @@ uint64_t qcrypto_block_get_sector_size(QCryptoBlock *block);
 void qcrypto_block_free(QCryptoBlock *block);
 
 G_DEFINE_AUTOPTR_CLEANUP_FUNC(QCryptoBlock, qcrypto_block_free)
-G_DEFINE_AUTOPTR_CLEANUP_FUNC(QCryptoBlockCreateOptions,
-                              qapi_free_QCryptoBlockCreateOptions)
 
 #endif /* QCRYPTO_BLOCK_H */
diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py
index 3ad33af4ee..3640f17cd6 100644
--- a/scripts/qapi/types.py
+++ b/scripts/qapi/types.py
@@ -213,6 +213,7 @@ def gen_type_cleanup_decl(name):
     ret = mcgen('''
 
 void qapi_free_%(c_name)s(%(c_name)s *obj);
+G_DEFINE_AUTOPTR_CLEANUP_FUNC(%(c_name)s, qapi_free_%(c_name)s)
 ''',
                 c_name=c_name(name))
     return ret
-- 
2.26.2

Re: [PATCH] qapi: enable use of g_autoptr with QAPI types

[Eric Blake]

On 7/23/20 6:12 AM, Daniel P. Berrangé wrote:
> Currently QAPI generates a type and function for free'ing it:
> 
>    typedef struct QCryptoBlockCreateOptions QCryptoBlockCreateOptions;
>    void qapi_free_QCryptoBlockCreateOptions(QCryptoBlockCreateOptions *obj);
> 

> The above code example now becomes
> 
>    g_autoptr(QCryptoBlockCreateOptions) opts = NULL;
> 
>    opts = g_new0(QCryptoBlockCreateOptions, 1);
> 
>    ....do stuff with opts...
> 
> Note, if the local pointer needs to live beyond the scope holding the
> variable, then g_steal_pointer can be used. This is useful to return the
> pointer to the caller in the success codepath, while letting it be freed
> in all error codepaths.
> 
>    return g_steal_pointer(&opts);
> 

Yep, the idea makes sense!

> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>   include/crypto/block.h | 2 --
>   scripts/qapi/types.py  | 1 +
>   2 files changed, 1 insertion(+), 2 deletions(-)

Missing a counterpart change to docs/devel/qapi-code-gen.txt.  And it 
might be nice to make this a series with at least one followup patch 
using the new capability, or at least so 'make check' coverage.  But 
otherwise on the right track.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH] qapi: enable use of g_autoptr with QAPI types

[Daniel P. Berrangé]

On Thu, Jul 23, 2020 at 06:49:44AM -0500, Eric Blake wrote:
> On 7/23/20 6:12 AM, Daniel P. Berrangé wrote:
> > Currently QAPI generates a type and function for free'ing it:
> > 
> >    typedef struct QCryptoBlockCreateOptions QCryptoBlockCreateOptions;
> >    void qapi_free_QCryptoBlockCreateOptions(QCryptoBlockCreateOptions *obj);
> > 
> 
> > The above code example now becomes
> > 
> >    g_autoptr(QCryptoBlockCreateOptions) opts = NULL;
> > 
> >    opts = g_new0(QCryptoBlockCreateOptions, 1);
> > 
> >    ....do stuff with opts...
> > 
> > Note, if the local pointer needs to live beyond the scope holding the
> > variable, then g_steal_pointer can be used. This is useful to return the
> > pointer to the caller in the success codepath, while letting it be freed
> > in all error codepaths.
> > 
> >    return g_steal_pointer(&opts);
> > 
> 
> Yep, the idea makes sense!
> 
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> >   include/crypto/block.h | 2 --
> >   scripts/qapi/types.py  | 1 +
> >   2 files changed, 1 insertion(+), 2 deletions(-)
> 
> Missing a counterpart change to docs/devel/qapi-code-gen.txt.  And it might
> be nice to make this a series with at least one followup patch using the new
> capability, or at least so 'make check' coverage.  But otherwise on the
> right track.

The crypto/block.c already makes use of this capability, which is why
I had to remove the line from block.h to avoid declaring the same thing
twice !

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH] qapi: enable use of g_autoptr with QAPI types

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Thu, Jul 23, 2020 at 06:49:44AM -0500, Eric Blake wrote:
>> On 7/23/20 6:12 AM, Daniel P. Berrangé wrote:
>> > Currently QAPI generates a type and function for free'ing it:
>> > 
>> >    typedef struct QCryptoBlockCreateOptions QCryptoBlockCreateOptions;
>> >    void qapi_free_QCryptoBlockCreateOptions(QCryptoBlockCreateOptions *obj);
>> > 
>> 
>> > The above code example now becomes
>> > 
>> >    g_autoptr(QCryptoBlockCreateOptions) opts = NULL;
>> > 
>> >    opts = g_new0(QCryptoBlockCreateOptions, 1);
>> > 
>> >    ....do stuff with opts...
>> > 
>> > Note, if the local pointer needs to live beyond the scope holding the
>> > variable, then g_steal_pointer can be used. This is useful to return the
>> > pointer to the caller in the success codepath, while letting it be freed
>> > in all error codepaths.
>> > 
>> >    return g_steal_pointer(&opts);
>> > 
>> 
>> Yep, the idea makes sense!

Agree.

>> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
>> > ---
>> >   include/crypto/block.h | 2 --
>> >   scripts/qapi/types.py  | 1 +
>> >   2 files changed, 1 insertion(+), 2 deletions(-)
>> 
>> Missing a counterpart change to docs/devel/qapi-code-gen.txt.

Yes.  Can do that in my tree.

>>                                                                And it might
>> be nice to make this a series with at least one followup patch using the new
>> capability, or at least so 'make check' coverage.  But otherwise on the
>> right track.
>
> The crypto/block.c already makes use of this capability, which is why
> I had to remove the line from block.h to avoid declaring the same thing
> twice !

Could be mentioned in the commit message.

Still, using it somewhere in tests would be nice.
test-qobject-input-visitor.c's test_visitor_in_struct_nested() looks
trivial to convert.  Feel free to pick something else.

In case you prefer not to, with a qapi-code-gen.txt update:
Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [PATCH] qapi: enable use of g_autoptr with QAPI types

[Daniel P. Berrangé]

On Thu, Jul 23, 2020 at 02:50:51PM +0200, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
> > On Thu, Jul 23, 2020 at 06:49:44AM -0500, Eric Blake wrote:
> >> On 7/23/20 6:12 AM, Daniel P. Berrangé wrote:
> >> > Currently QAPI generates a type and function for free'ing it:
> >> > 
> >> >    typedef struct QCryptoBlockCreateOptions QCryptoBlockCreateOptions;
> >> >    void qapi_free_QCryptoBlockCreateOptions(QCryptoBlockCreateOptions *obj);
> >> > 
> >> 
> >> > The above code example now becomes
> >> > 
> >> >    g_autoptr(QCryptoBlockCreateOptions) opts = NULL;
> >> > 
> >> >    opts = g_new0(QCryptoBlockCreateOptions, 1);
> >> > 
> >> >    ....do stuff with opts...
> >> > 
> >> > Note, if the local pointer needs to live beyond the scope holding the
> >> > variable, then g_steal_pointer can be used. This is useful to return the
> >> > pointer to the caller in the success codepath, while letting it be freed
> >> > in all error codepaths.
> >> > 
> >> >    return g_steal_pointer(&opts);
> >> > 
> >> 
> >> Yep, the idea makes sense!
> 
> Agree.
> 
> >> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> >> > ---
> >> >   include/crypto/block.h | 2 --
> >> >   scripts/qapi/types.py  | 1 +
> >> >   2 files changed, 1 insertion(+), 2 deletions(-)
> >> 
> >> Missing a counterpart change to docs/devel/qapi-code-gen.txt.
> 
> Yes.  Can do that in my tree.
> 
> >>                                                                And it might
> >> be nice to make this a series with at least one followup patch using the new
> >> capability, or at least so 'make check' coverage.  But otherwise on the
> >> right track.
> >
> > The crypto/block.c already makes use of this capability, which is why
> > I had to remove the line from block.h to avoid declaring the same thing
> > twice !
> 
> Could be mentioned in the commit message.
> 
> Still, using it somewhere in tests would be nice.
> test-qobject-input-visitor.c's test_visitor_in_struct_nested() looks
> trivial to convert.  Feel free to pick something else.

Ok, I'll convert some.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|