Re: [PATCH v2 1/5] qemu/qarray.h: introduce QArray

[Daniel P . Berrangé]

On Thu, Sep 30, 2021 at 03:20:19PM +0200, Christian Schoenebeck wrote:
> On Mittwoch, 29. September 2021 19:48:38 CEST Daniel P. Berrangé wrote:
> > On Wed, Sep 29, 2021 at 07:32:39PM +0200, Christian Schoenebeck wrote:
> > > On Dienstag, 28. September 2021 18:41:17 CEST Daniel P. Berrangé wrote:
> > > > On Tue, Sep 28, 2021 at 06:23:23PM +0200, Christian Schoenebeck wrote:
> > > > > On Dienstag, 28. September 2021 15:04:36 CEST Daniel P. Berrangé 
> wrote:
> > > > > > On Sun, Aug 22, 2021 at 03:16:46PM +0200, Christian Schoenebeck 
> wrote:
> > > [...]
> > > > The GLib automatic memory support is explicitly designed to be extendd
> > > > with support for application specific types. We already do exactly that
> > > > all over QEMU with many calls to G_DEFINE_AUTOPTR_CLEANUP_FUNC(..) to
> > > > register functions for free'ing specific types, such that you can
> > > > use 'g_autoptr' with them.
> > > 
> > > Ok, just to make sure that I am not missing something here, because really
> > > if there is already something that does the job that I simply haven't
> > > seen, then I happily drop this QArray code.
> > 
> > I don't believe there is anything that currently addresses this well.
> > 
> > > But AFAICS this G_DEFINE_AUTOPTR_CLEANUP_FUNC() & g_autoptr concept does
> > > not have any notion of "size" or "amount", right?
> > 
> > Correct, all it knows is that there's a data type and an associated
> > free function.
> 
> Ok, thanks for the clarification.
> 
> > > So let's say you already have the following type and cleanup function in
> > > your existing code:
> > > 
> > > typedef struct MyScalar {
> > > 
> > >     int a;
> > >     char *b;
> > > 
> > > } MyScalar;
> > > 
> > > void myscalar_free(MayScalar *s) {
> > > 
> > >     g_free(s->b);
> > > 
> > > }
> > > 
> > > Then if you want to use G_DEFINE_AUTOPTR_CLEANUP_FUNC() for an array on
> > > that scalar type, then you still would need to *manually* write
> > > additionally a separate type and cleanup function like:
> > > 
> > > typedef struct MyArray {
> > > 
> > >     MyScalar *s;
> > >     int n;
> > > 
> > > };
> > > 
> > > void myarray_free(MyArray *a) {
> > > 
> > >     for (int i = 0; i < a->n; ++i) {
> > >     
> > >         myscalar_free(a->s[i]);
> > >     
> > >     }
> > >     g_free(a);
> > > 
> > > }
> > > 
> > > Plus you have to manually populate that field 'n' after allocation.
> > > 
> > > Am I wrong?
> > 
> > Yes and no.  You can of course manually write all these stuff
> > as you describe, but since we expect the array wrappers to be
> > needed for more than one type it makes more sense to have
> > that all done via macros.
> > 
> > Your patch contains a DECLARE_QARRAY_TYPE and DEFINE_QARRAY_TYPE
> > that provide all this reqiured boilerplate code.  The essential
> > difference that I'm suggesting is that the array struct type emitted
> > by the macro is explicitly visible as a concept to calling code such
> > that it is used directly used with g_autoptr.
> 
> I got that, but your preferred user pattern was this:
> 
>     DECLARE_QARRAY_TYPE(Foo);
>        ...
>     g_autoptr(FooArray) foos = foo_array_new(n);
> 
> I don't see a portable way to do upper-case to lower-case conversion with the 
> C preprocessor. So you would end up like this instead:
> 
>     g_autoptr(FooArray) foos = Foo_array_new(n);
> 
> Which does not really fit into common QEMU naming conventions either, does it?

Right, it would need to be a two arg macro:

  DECLARE_QARRAY_TYPE(Foo, foo);

similar to what we do with macros for declaring QOM types becuase of
the same case conversion needs.

> And I can help it, I don't see what's wrong in exposing a regular C-array to
> user code. I mean in the Linux kernel for instance it is absolutely normal to
> convert from a compound structure to its parent structure. I don't find
> anything magical about that and it is simply less code and better readable.

QEMU code is not Linux code. We're following the GLib practices for
automatic memory deallocation, and QOM is also modelled on GLib. The
proposal looks magical from the POV of QEMU's code patterns, as it is
not making use of GLib's g_auto* code.

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 :|