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

[Christian Schoenebeck]

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:
[...]
> > > If using QArray, it still has to keep passing around the
> > > 'size_t naddrs' value because QArray hides the length
> > > field from the code.
> > 
> > Well no, you don't need to pass around anything, as the array length is
> > always accessible; it is always just (compile time) constant -wordsize
> > (-padding) offset away from the C-array pointer. Maybe the phrasing
> > "private" was a bit misleading in the QArray.h comments.
> > 
> > It is correct that my 9p use case so far did not need the array length
> > info by means of accessing an API, for that reason I really just ommitted
> > (yet) to add a separate patch for that. All it would take was extending
> > QArray.h in a way like (roughly):
> > 
> > typedef struct _QArrayGeneric {
> > 
> >     size_t len;
> >     char first[];
> > 
> > } _QArrayGeneric;
> > 
> > /**
> > 
> >  * Returns the amount of scalar elements in the passed array.
> >  *
> >  * @param first - start of array
> >  */
> > 
> > size_t qarray_len(void* first)
> > {
> > 
> >     if (!first) {
> >     
> >         return 0;
> >     
> >     }
> >     _QArrayGeneric *arr = (_QArrayGeneric *) (
> >     
> >         ((char *)first) - offsetof(_QArrayGeneric, first)
> >     
> >     );
> >     return arr->len;
> > 
> > }
> > 
> > #define QARRAY_LEN(arr) qarray_len(arr)
> > 
> > And as this is generic code for all array scalar types, it would probably
> > be partly placed in a separate qarray.c file.
> > 
> > After that change your user example would become:
> >   for (i = 0; i < QARRAY_LEN(addrs); i++) {
> >   
> >       ...try to connect to addrs[i]...
> >   
> >   }
> > 
> > If you want I can post a v3 with a formal patch (or two) to handle that
> > array length API.
> 
> I still find this all overkill compared to just exposing the
> array struct explicitly.

Yes, you made that clear. :)

> > > If it instead just exposed the array struct explicitly, it can
> > > use the normal g_autoptr() declarator, and can also now just
> > > return the array directly since it is a single pointer
> > > 
> > >   int open_conn(const char *hostname) {
> > >   
> > >     g_autoptr(SocketAddressArray) addrs = NULL;
> > >     int ret = -1;
> > >     size_t i;
> > >     
> > >     if (!(addrs = resolve_hostname(hostname)))
> > >     
> > >         return -1;
> > >     
> > >     for (i = 0; i < addrs.len; i++) {
> > >     
> > >         ...try to connect to addrs.data[i]...
> > >     
> > >     }
> > >     
> > >     ret = 0
> > >    
> > >    cleanup:
> > >     return ret;
> > >   
> > >   }
> > > 
> > > In terms of the first example, it adds an indirection to access
> > > the array data, but on the plus side IMHO the code is clearer
> > > because it uses 'g_autoptr' which is what is more in line with
> > > what is expected for variables that are automatically freed.
> > > QArrayRef() as a name doesn't make it clear that the value will
> > > be freed.
> > > 
> > >    void doSomething(int n) {
> > >    
> > >        g_autoptr(FooArray) foos = NULL;
> > >        QARRAY_CREATE(Foo, foos, n);
> > >        for (size_t i = 0; i < foos.len; ++i) {
> > >        
> > >            foos.data[i].i = i;
> > >            foos.data[i].s = calloc(4096, 1);
> > >            snprintf(foos.data[i].s, 4096, "foo %d", i);
> > >        
> > >        }
> > >    
> > >    }
> > 
> > Well, that would destroy the intended major feature "as little refactoring
> > as possible". The amount of locations where you define a reference
> > variable is usually much smaller than the amount of code locations where
> > you actually access arrays.
> 
> If there's a large amount of existing code refactoring to be avoided
> an intermediate variable can be declared to point to the struct field
> to avoid the field references.

That would be one additional (unguarded) raw pointer variable per array & 
function, that multiplied by the amount of arrays and functions ...

... the suggested shared utility code is 34 lines LOC net.

> > Personally I would not mix in this case macros of foreign libraries (glib)
> > with macros of a local framework (QArray), because if for some reason one
> > of the two deviate in future in a certain way, you would need to refactor
> > a whole bunch of user code. By just separating those definitions from day
> > one, you can avoid such future refactoring work right from the start.
> 
> 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.

But AFAICS this G_DEFINE_AUTOPTR_CLEANUP_FUNC() & g_autoptr concept does not 
have any notion of "size" or "amount", right?

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?

> > As far as the terminology is concerned: probably a matter of taste. For me
> > a "reference" implies (either unique or shared) ownership, a "pointer"
> > IMO doesn't. And the usage of QArray alone makes it clear that an array
> > without any references gets automatically freed.
> 
> It is more important than a matter of taste - it is about having a
> consistent approach throughout QEMU. That means automatic free'ing of
> variables should involve g_autoptr, not something custom to a specific type
> with different terminology.

The barriers to add few lines of utility code are really high. :)

> > > I would also suggest that QARRAY_CREATE doesn't need to
> > > exist as a macro - callers could just use the allocator
> > > function directly for clearer code, if it was changed to
> > > 
> > > return the ptr rather than use an out parameter:
> > >    void doSomething(int n) {
> > >    
> > >        g_autoptr(FooArray) foos = foo_array_new(n);
> > >        for (size_t i = 0; i < foos.len; ++i) {
> > >        
> > >            foos.data[i].i = i;
> > >            foos.data[i].s = calloc(4096, 1);
> > >            snprintf(foos.data[i].s, 4096, "foo %d", i);
> > >        
> > >        }
> > >    
> > >    }
> > > 
> > > For this it needs to pass 2 args into the DECLARE_QARRAY_TYPE
> > > macro - the struct name and desired method name - basically
> > > the method name is the struct name in lowercase with underscores.
> > 
> > As you can see with patch 2, one of the movations of making this a macro
> > was> 
> > the intention to increase strictness of type safety, e.g to make things 
like:
> >     void *p;
> >     ...
> >     QARRAY_CREATE(FooType, p, n);
> > 
> > to raise a compiler error immediately, but that's not all ...
> > 
> > > Overall I think the goal of having an convenient sized array for
> > > types is good, but I think we should make it look a bit less
> > > magic. I think we only need the DECLARE_QARRAY_TYPE and
> > > DEFINE_QARRAY_TYPE macros.
> > 
> > ... actually making it appear anything like magic was not my intention.
> > The
> > actual main reason for wrapping these things into macros is because that's
> > actually the only way to write generic code in C. Especially in larger
> > projects like this one I favour clear separation of API ("how to use it")
> > from its actual implementation ("how does it do it exactly").
> > 
> > So if you use macros for all those things from the beginning, it is far
> > less likely that you will need to refactor a huge amount of user code
> > with future changes of this array framework.
> 
> I can't see the array framework being complex enough that it will be
> changed in a way that invalidates existing usage.

Well, there are some things that would come to my mind (e.g. strong vs. weak 
refs) , but I think for now my upper question is more important ATM, i.e. 
whether there is already something that does the job (right).
 
> > > Incidentally, I'd suggest naming to be QARRAY_DECLARE_TYPE
> > > and QARRAY_DEFINE_TYPE.
> > 
> > Also a matter of taste I guess. The suggested naming DECLARE_QARRAY_TYPE()
> > and DEFINE_QARRAY_TYPE() reflect more natural language IMO.
> 
> I consider the QEMU normal practice for namespacing types/macros/functions
> is to have the typename as the first component.
> 
> Regards,
> Daniel