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

Implements deep auto free of arrays while retaining common C-style
squared bracket access.

Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
---
 include/qemu/qarray.h | 150 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 150 insertions(+)
 create mode 100644 include/qemu/qarray.h

Christian Schoenebeck <qemu_oss@crudebyte.com> writes:

> Implements deep auto free of arrays while retaining common C-style
> squared bracket access.
>
> Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>

You provide some motivation for this, but only in your cover letter:

    Patches 1 and 2 introduce include/qemu/qarray.h which implements a deep auto
    free mechanism for arrays. Unlike GArray it does not require special macros,
    function calls or member dereferencing to access the individual array
    elements. So existing C-style array code can be retained with only very
    little changes.

    In this initial version QArray only supports the concept of unique pointers,
    i.e. it does not support reference counting. The array (and all dynamically
    allocated memory of individual array elements) is auto freed once execution
    leaves the scope of the reference variable (unique pointer) associated with
    the array.

Please put it into the commit message, so it gets committed.

An example to illustrate how QArray is better than GArray (for some
value of "better") would help make your case that QArray is worth its
maintenance cost.

> ---
>  include/qemu/qarray.h | 150 ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 150 insertions(+)
>  create mode 100644 include/qemu/qarray.h
>
> diff --git a/include/qemu/qarray.h b/include/qemu/qarray.h
> new file mode 100644
> index 0000000000..9885e5e9ed
> --- /dev/null
> +++ b/include/qemu/qarray.h
> @@ -0,0 +1,150 @@
> +/*
> + * QArray - deep auto free C-array
> + *
> + * Copyright (c) 2021 Crudebyte
> + *
> + * Authors:
> + *   Christian Schoenebeck <qemu_oss@crudebyte.com>
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a copy
> + * of this software and associated documentation files (the "Software"), to deal
> + * in the Software without restriction, including without limitation the rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + */

Please use GPLv2+ unless you have a compelling reason not to.

[...]

On Dienstag, 24. August 2021 10:22:52 CEST Markus Armbruster wrote:
> Christian Schoenebeck <qemu_oss@crudebyte.com> writes:
> > Implements deep auto free of arrays while retaining common C-style
> > squared bracket access.
> > 
> > Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> 
> You provide some motivation for this, but only in your cover letter:
> 
>     Patches 1 and 2 introduce include/qemu/qarray.h which implements a deep
> auto free mechanism for arrays. Unlike GArray it does not require special
> macros, function calls or member dereferencing to access the individual
> array elements. So existing C-style array code can be retained with only
> very little changes.
> 
>     In this initial version QArray only supports the concept of unique
> pointers, i.e. it does not support reference counting. The array (and all
> dynamically allocated memory of individual array elements) is auto freed
> once execution leaves the scope of the reference variable (unique pointer)
> associated with the array.
> 
> Please put it into the commit message, so it gets committed.

Sure, np.

> An example to illustrate how QArray is better than GArray (for some
> value of "better") would help make your case that QArray is worth its
> maintenance cost.

Ok, I'll put that into the commit message as well, instead of into the API 
comments.

> > ---
> > 
> >  include/qemu/qarray.h | 150 ++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 150 insertions(+)
> >  create mode 100644 include/qemu/qarray.h
> > 
> > diff --git a/include/qemu/qarray.h b/include/qemu/qarray.h
> > new file mode 100644
> > index 0000000000..9885e5e9ed
> > --- /dev/null
> > +++ b/include/qemu/qarray.h
> > @@ -0,0 +1,150 @@
> > +/*
> > + * QArray - deep auto free C-array
> > + *
> > + * Copyright (c) 2021 Crudebyte
> > + *
> > + * Authors:
> > + *   Christian Schoenebeck <qemu_oss@crudebyte.com>
> > + *
> > + * Permission is hereby granted, free of charge, to any person obtaining
> > a copy + * of this software and associated documentation files (the
> > "Software"), to deal + * in the Software without restriction, including
> > without limitation the rights + * to use, copy, modify, merge, publish,
> > distribute, sublicense, and/or sell + * copies of the Software, and to
> > permit persons to whom the Software is + * furnished to do so, subject to
> > the following conditions:
> > + *
> > + * The above copyright notice and this permission notice shall be
> > included in + * all copies or substantial portions of the Software.
> > + *
> > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> > EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> > MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND
> > NONINFRINGEMENT. IN NO EVENT SHALL + * THE AUTHORS OR COPYRIGHT HOLDERS
> > BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN
> > ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN
> > CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN + * THE
> > SOFTWARE.
> > + */
> 
> Please use GPLv2+ unless you have a compelling reason not to.
> 
> [...]

Is that a requirement?

It is just my personal license preference. AFAICS there are numerous sources 
in QEMU released under MIT license as well.

Best regards,
Christian Schoenebeck

On Sonntag, 22. August 2021 15:16:46 CEST Christian Schoenebeck wrote:
> Implements deep auto free of arrays while retaining common C-style
> squared bracket access.
> 
> Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> ---
>  include/qemu/qarray.h | 150 ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 150 insertions(+)
>  create mode 100644 include/qemu/qarray.h
> 
> diff --git a/include/qemu/qarray.h b/include/qemu/qarray.h
> new file mode 100644
> index 0000000000..9885e5e9ed
> --- /dev/null
> +++ b/include/qemu/qarray.h
> @@ -0,0 +1,150 @@
> +/*
> + * QArray - deep auto free C-array
> + *
> + * Copyright (c) 2021 Crudebyte
> + *
> + * Authors:
> + *   Christian Schoenebeck <qemu_oss@crudebyte.com>
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> copy + * of this software and associated documentation files (the
> "Software"), to deal + * in the Software without restriction, including
> without limitation the rights + * to use, copy, modify, merge, publish,
> distribute, sublicense, and/or sell + * copies of the Software, and to
> permit persons to whom the Software is + * furnished to do so, subject to
> the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included
> in + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
> OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.
> IN NO EVENT SHALL + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY
> CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT,
> TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE
> SOFTWARE OR THE USE OR OTHER DEALINGS IN + * THE SOFTWARE.
> + */
> +#ifndef QEMU_QARRAY_H
> +#define QEMU_QARRAY_H
> +
> +/**
> + * QArray provides a mechanism to access arrays in common C-style (e.g. by
> + * square bracket [] operator) in conjunction with reference variables that
> + * perform deep auto free of the array when leaving the scope of the auto
> + * reference variable. That means not only is the array itself
> automatically + * freed, but also memory dynamically allocated by the
> individual array + * elements.
> + *
> + * Example:
> + *
> + * Consider the following user struct @c Foo which shall be used as scalar
> + * (element) type of an array:
> + * @code
> + * typedef struct Foo {
> + *     int i;
> + *     char *s;
> + * } Foo;
> + * @endcode
> + * and assume it has the following function to free memory allocated by @c
> Foo + * instances:
> + * @code
> + * void free_foo(Foo *foo) {
> + *     free(foo->s);
> + * }
> + * @endcode
> + * Add the following to a shared header file:
> + * @code
> + * DECLARE_QARRAY_TYPE(Foo);
> + * @endcode
> + * and the following to a C unit file:
> + * @code
> + * DEFINE_QARRAY_TYPE(Foo, free_foo);
> + * @endcode
> + * Finally the array may then be used like this:
> + * @code
> + * void doSomething(int n) {
> + *     QArrayRef(Foo) foos = NULL;
> + *     QARRAY_CREATE(Foo, foos, n);
> + *     for (size_t i = 0; i < n; ++i) {
> + *         foos[i].i = i;
> + *         foos[i].s = calloc(4096, 1);
> + *         snprintf(foos[i].s, 4096, "foo %d", i);
> + *     }
> + * }
> + * @endcode

Or should that probably be changed to upper case QArrayRef() -> QARRAY_REF(), 
because ...

> + */
> +
> +/**
> + * Declares an array type for the passed @a scalar_type.
> + *
> + * This is typically used from a shared header file.
> + *
> + * @param scalar_type - type of the individual array elements
> + */
> +#define DECLARE_QARRAY_TYPE(scalar_type) \
> +    typedef struct QArray##scalar_type { \
> +        size_t len; \
> +        scalar_type first[]; \
> +    } QArray##scalar_type; \
> +    \
> +    void qarray_create_##scalar_type(scalar_type **auto_var, size_t len); \
> +    void qarray_auto_free_##scalar_type(scalar_type **auto_var); \ +
> +/**
> + * Defines an array type for the passed @a scalar_type and appropriate
> + * @a scalar_cleanup_func.
> + *
> + * This is typically used from a C unit file.
> + *
> + * @param scalar_type - type of the individual array elements
> + * @param scalar_cleanup_func - appropriate function to free memory
> dynamically + *                              allocated by individual array
> elements before + */
> +#define DEFINE_QARRAY_TYPE(scalar_type, scalar_cleanup_func) \
> +    void qarray_create_##scalar_type(scalar_type **auto_var, size_t len) \
> +    { \
> +        qarray_auto_free_##scalar_type(auto_var); \
> +        QArray##scalar_type *arr = g_malloc0(sizeof(QArray##scalar_type) +
> \ +            len * sizeof(scalar_type)); \
> +        arr->len = len; \
> +        *auto_var = &arr->first[0]; \
> +    } \
> +    \
> +    void qarray_auto_free_##scalar_type(scalar_type **auto_var) \
> +    { \
> +        scalar_type *first = (*auto_var); \
> +        if (!first) { \
> +            return; \
> +        } \
> +        QArray##scalar_type *arr = (QArray##scalar_type *) ( \
> +            ((char *)first) - offsetof(QArray##scalar_type, first) \
> +        ); \
> +        for (size_t i = 0; i < arr->len; ++i) { \
> +            scalar_cleanup_func(&arr->first[i]); \
> +        } \
> +        g_free(arr); \
> +    } \
> +
> +/**
> + * Used to declare a reference variable (unique pointer) for an array.
> After + * leaving the scope of the reference variable, the associated array
> is + * automatically freed.
> + *
> + * @param scalar_type - type of the individual array elements
> + */
> +#define QArrayRef(scalar_type) \
> +    __attribute((__cleanup__(qarray_auto_free_##scalar_type))) scalar_type*
> +

... it is actually a macro?

> +/**
> + * Allocates a new array of passed @a scalar_type with @a len number of
> array + * elements and assigns the created array to the reference variable
> + * @a auto_var.
> + *
> + * @param scalar_type - type of the individual array elements
> + * @param auto_var - destination reference variable
> + * @param len - amount of array elements to be allocated immediately
> + */
> +#define QARRAY_CREATE(scalar_type, auto_var, len) \
> +    qarray_create_##scalar_type((&auto_var), len)
> +
> +#endif /* QEMU_QARRAY_H */

Best regards,
Christian Schoenebeck

Christian Schoenebeck <qemu_oss@crudebyte.com> writes:

> On Sonntag, 22. August 2021 15:16:46 CEST Christian Schoenebeck wrote:
>> Implements deep auto free of arrays while retaining common C-style
>> squared bracket access.
>> 
>> Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
>> ---
>>  include/qemu/qarray.h | 150 ++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 150 insertions(+)
>>  create mode 100644 include/qemu/qarray.h
>> 
>> diff --git a/include/qemu/qarray.h b/include/qemu/qarray.h
>> new file mode 100644
>> index 0000000000..9885e5e9ed
>> --- /dev/null
>> +++ b/include/qemu/qarray.h
>> @@ -0,0 +1,150 @@

[...]

>> + * Consider the following user struct @c Foo which shall be used as scalar
>> + * (element) type of an array:
>> + * @code
>> + * typedef struct Foo {
>> + *     int i;
>> + *     char *s;
>> + * } Foo;
>> + * @endcode
>> + * and assume it has the following function to free memory allocated by @c Foo
>> + * instances:
>> + * @code
>> + * void free_foo(Foo *foo) {
>> + *     free(foo->s);
>> + * }
>> + * @endcode
>> + * Add the following to a shared header file:
>> + * @code
>> + * DECLARE_QARRAY_TYPE(Foo);
>> + * @endcode
>> + * and the following to a C unit file:
>> + * @code
>> + * DEFINE_QARRAY_TYPE(Foo, free_foo);
>> + * @endcode
>> + * Finally the array may then be used like this:
>> + * @code
>> + * void doSomething(int n) {
>> + *     QArrayRef(Foo) foos = NULL;
>> + *     QARRAY_CREATE(Foo, foos, n);
>> + *     for (size_t i = 0; i < n; ++i) {
>> + *         foos[i].i = i;
>> + *         foos[i].s = calloc(4096, 1);
>> + *         snprintf(foos[i].s, 4096, "foo %d", i);
>> + *     }
>> + * }
>> + * @endcode
>
> Or should that probably be changed to upper case QArrayRef() -> QARRAY_REF(), 
> because ...
>
>> + */

[...]

>> +/**
>> + * Used to declare a reference variable (unique pointer) for an array. After
>> + * leaving the scope of the reference variable, the associated array is
>> + * automatically freed.
>> + *
>> + * @param scalar_type - type of the individual array elements
>> + */
>> +#define QArrayRef(scalar_type) \
>> +    __attribute((__cleanup__(qarray_auto_free_##scalar_type))) scalar_type*
>> +
>
> ... it is actually a macro?

I'd change it.

[...]

Christian Schoenebeck <qemu_oss@crudebyte.com> writes:

> On Dienstag, 24. August 2021 10:22:52 CEST Markus Armbruster wrote:

[...]

>> Please use GPLv2+ unless you have a compelling reason not to.
>> 
>> [...]
>
> Is that a requirement?
>
> It is just my personal license preference. AFAICS there are numerous sources 
> in QEMU released under MIT license as well.

The licensing situation is a mess.

The only hard requirement is "compatible with GPLv2+".  We prefer GPLv2+
for new code, except as detailed in ./LICENSE.  We're stuck with a
sizable body of existing code that is GPLv2 (no +), but decided to put
limits to that madness.  Again, see ./LICENSE for details.

I'm asking you to help with limiting the madness by sticking to GPLv2+
whenever possible.

On Dienstag, 24. August 2021 16:45:12 CEST Markus Armbruster wrote:
> Christian Schoenebeck <qemu_oss@crudebyte.com> writes:
> > On Dienstag, 24. August 2021 10:22:52 CEST Markus Armbruster wrote:
> [...]
> 
> >> Please use GPLv2+ unless you have a compelling reason not to.
> >> 
> >> [...]
> > 
> > Is that a requirement?
> > 
> > It is just my personal license preference. AFAICS there are numerous
> > sources in QEMU released under MIT license as well.
> 
> The licensing situation is a mess.
> 
> The only hard requirement is "compatible with GPLv2+".  We prefer GPLv2+
> for new code, except as detailed in ./LICENSE.  We're stuck with a
> sizable body of existing code that is GPLv2 (no +), but decided to put
> limits to that madness.  Again, see ./LICENSE for details.
> 
> I'm asking you to help with limiting the madness by sticking to GPLv2+
> whenever possible.

Okay, I see that there is quite a homogenous license structure in Qemu. 
However the MIT license is a very permissive license, so I don't see any 
conflicts.

What if I release this file under public domain? That's not even copyleft at 
all. What that be OK for you?

My idea was that people might simply take this header file and use it in other 
C projects as well. Putting it under GPL might cause conflicts for other 
projects.

Best regards,
Christian Schoenebeck

On Dienstag, 24. August 2021 17:24:50 CEST Christian Schoenebeck wrote:
> On Dienstag, 24. August 2021 16:45:12 CEST Markus Armbruster wrote:
> > Christian Schoenebeck <qemu_oss@crudebyte.com> writes:
> > > On Dienstag, 24. August 2021 10:22:52 CEST Markus Armbruster wrote:
> > [...]
> > 
> > >> Please use GPLv2+ unless you have a compelling reason not to.
> > >> 
> > >> [...]
> > > 
> > > Is that a requirement?
> > > 
> > > It is just my personal license preference. AFAICS there are numerous
> > > sources in QEMU released under MIT license as well.
> > 
> > The licensing situation is a mess.
> > 
> > The only hard requirement is "compatible with GPLv2+".  We prefer GPLv2+
> > for new code, except as detailed in ./LICENSE.  We're stuck with a
> > sizable body of existing code that is GPLv2 (no +), but decided to put
> > limits to that madness.  Again, see ./LICENSE for details.
> > 
> > I'm asking you to help with limiting the madness by sticking to GPLv2+
> > whenever possible.
> 
> Okay, I see that there is quite a homogenous license structure in Qemu.
> However the MIT license is a very permissive license, so I don't see any
> conflicts.

s/homogenous/heterogeneous/

> What if I release this file under public domain? That's not even copyleft at
> all. What that be OK for you?

"Would" that be OK for you?

> My idea was that people might simply take this header file and use it in
> other C projects as well. Putting it under GPL might cause conflicts for
> other projects.

Best regards,
Christian Schoenebeck

Christian Schoenebeck <qemu_oss@crudebyte.com> writes:

> On Dienstag, 24. August 2021 17:24:50 CEST Christian Schoenebeck wrote:
>> On Dienstag, 24. August 2021 16:45:12 CEST Markus Armbruster wrote:
>> > Christian Schoenebeck <qemu_oss@crudebyte.com> writes:
>> > > On Dienstag, 24. August 2021 10:22:52 CEST Markus Armbruster wrote:
>> > [...]
>> > 
>> > >> Please use GPLv2+ unless you have a compelling reason not to.
>> > >> 
>> > >> [...]
>> > > 
>> > > Is that a requirement?
>> > > 
>> > > It is just my personal license preference. AFAICS there are numerous
>> > > sources in QEMU released under MIT license as well.
>> > 
>> > The licensing situation is a mess.
>> > 
>> > The only hard requirement is "compatible with GPLv2+".  We prefer GPLv2+
>> > for new code, except as detailed in ./LICENSE.  We're stuck with a
>> > sizable body of existing code that is GPLv2 (no +), but decided to put
>> > limits to that madness.  Again, see ./LICENSE for details.
>> > 
>> > I'm asking you to help with limiting the madness by sticking to GPLv2+
>> > whenever possible.
>> 
>> Okay, I see that there is quite a homogenous license structure in Qemu.

Self-inflicted wound.  We should have insisted on GPLv2+.

>> However the MIT license is a very permissive license, so I don't see any
>> conflicts.
>
> s/homogenous/heterogeneous/
>
>> What if I release this file under public domain? That's not even copyleft at
>> all. What that be OK for you?
>
> "Would" that be OK for you?

My preference: GPLv2+ > MIT > public domain.

If you go with anything but GPLv2+, please explain why in your commit
message.  One sentence should suffice, say "MIT license to minimize
license issues when "stealing" this code for other projects."

>> My idea was that people might simply take this header file and use it in
>> other C projects as well. Putting it under GPL might cause conflicts for
>> other projects.

On Sun, Aug 22, 2021 at 03:16:46PM +0200, Christian Schoenebeck wrote:
> Implements deep auto free of arrays while retaining common C-style
> squared bracket access.
> 
> Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> ---
>  include/qemu/qarray.h | 150 ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 150 insertions(+)
>  create mode 100644 include/qemu/qarray.h
> 
> diff --git a/include/qemu/qarray.h b/include/qemu/qarray.h
> new file mode 100644
> index 0000000000..9885e5e9ed
> --- /dev/null
> +++ b/include/qemu/qarray.h
> @@ -0,0 +1,150 @@

> +#ifndef QEMU_QARRAY_H
> +#define QEMU_QARRAY_H
> +
> +/**
> + * QArray provides a mechanism to access arrays in common C-style (e.g. by
> + * square bracket [] operator) in conjunction with reference variables that
> + * perform deep auto free of the array when leaving the scope of the auto
> + * reference variable. That means not only is the array itself automatically
> + * freed, but also memory dynamically allocated by the individual array
> + * elements.
> + *
> + * Example:
> + *
> + * Consider the following user struct @c Foo which shall be used as scalar
> + * (element) type of an array:
> + * @code
> + * typedef struct Foo {
> + *     int i;
> + *     char *s;
> + * } Foo;
> + * @endcode
> + * and assume it has the following function to free memory allocated by @c Foo
> + * instances:
> + * @code
> + * void free_foo(Foo *foo) {
> + *     free(foo->s);
> + * }
> + * @endcode
> + * Add the following to a shared header file:
> + * @code
> + * DECLARE_QARRAY_TYPE(Foo);
> + * @endcode
> + * and the following to a C unit file:
> + * @code
> + * DEFINE_QARRAY_TYPE(Foo, free_foo);
> + * @endcode
> + * Finally the array may then be used like this:
> + * @code
> + * void doSomething(int n) {
> + *     QArrayRef(Foo) foos = NULL;
> + *     QARRAY_CREATE(Foo, foos, n);
> + *     for (size_t i = 0; i < n; ++i) {
> + *         foos[i].i = i;
> + *         foos[i].s = calloc(4096, 1);
> + *         snprintf(foos[i].s, 4096, "foo %d", i);
> + *     }
> + * }

So essentially here the 'foos' variable is still a plain C array
from POV of the caller ie it is a  'Foo *foos'

By QARRAY_CREATE() is actually allocating a 'QArrayFoo' which
is a struct containing a 'size_t len' along with the 'Foo *foos'
entry.

This is a clever trick, and it works in the example above,
because the code already has the length available in a
convenient way via the 'int n' parameter.

IMHO this makes the code less useful than it otherwise would
be in the general case, because there are places where the code
needs to pass around the array and its assoicated length, and
so this with this magic hidden length, we're still left to pass
around the length separately.

Consider this example:

  int open_conn(const char *hostname) {
    SocketAddress *addrs;
    size_t naddrs;
    int ret = -1;
    size_t i;

    if (resolve_hostname(hostname, &addrs, &naddrs) < 0)
        return -1;

    for (i = 0; i < naddrs; i++) {
        ...try to connect to addrs[i]...
    }

    ret = 0
   cleanup:
    for (i = 0; i < naddrs; i++) {
       qapi_free_SocketAddress(addrs[i])
    }
    return ret;
  }

To simplify this it is deisrable to autofree the 'addrs'
array.

If using QArray, it still has to keep passing around the
'size_t naddrs' value because QArray hides the length
field from the code.


  int open_conn(const char *hostname) {
    QArrayRef(SocketAddress) addrs = NULL;
    size_t naddrs;
    int ret = -1;
    size_t i;

    if (resolve_hostname(hostname, &addrs, &naddrs) < 0)
        return -1;

    for (i = 0; i < naddrs; i++) {
        ...try to connect to addrs[i]...
    }

    ret = 0
   cleanup:
    return ret;
  }


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);
       }
   }

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.

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.

Incidentally, I'd suggest naming to be QARRAY_DECLARE_TYPE
and QARRAY_DEFINE_TYPE.



> + * @endcode
> + */
> +
> +/**
> + * Declares an array type for the passed @a scalar_type.
> + *
> + * This is typically used from a shared header file.
> + *
> + * @param scalar_type - type of the individual array elements
> + */
> +#define DECLARE_QARRAY_TYPE(scalar_type) \
> +    typedef struct QArray##scalar_type { \
> +        size_t len; \
> +        scalar_type first[]; \
> +    } QArray##scalar_type; \
> +    \
> +    void qarray_create_##scalar_type(scalar_type **auto_var, size_t len); \
> +    void qarray_auto_free_##scalar_type(scalar_type **auto_var); \
> +
> +/**
> + * Defines an array type for the passed @a scalar_type and appropriate
> + * @a scalar_cleanup_func.
> + *
> + * This is typically used from a C unit file.
> + *
> + * @param scalar_type - type of the individual array elements
> + * @param scalar_cleanup_func - appropriate function to free memory dynamically
> + *                              allocated by individual array elements before
> + */
> +#define DEFINE_QARRAY_TYPE(scalar_type, scalar_cleanup_func) \
> +    void qarray_create_##scalar_type(scalar_type **auto_var, size_t len) \
> +    { \
> +        qarray_auto_free_##scalar_type(auto_var); \
> +        QArray##scalar_type *arr = g_malloc0(sizeof(QArray##scalar_type) + \
> +            len * sizeof(scalar_type)); \
> +        arr->len = len; \
> +        *auto_var = &arr->first[0]; \
> +    } \
> +    \
> +    void qarray_auto_free_##scalar_type(scalar_type **auto_var) \
> +    { \
> +        scalar_type *first = (*auto_var); \
> +        if (!first) { \
> +            return; \
> +        } \
> +        QArray##scalar_type *arr = (QArray##scalar_type *) ( \
> +            ((char *)first) - offsetof(QArray##scalar_type, first) \
> +        ); \
> +        for (size_t i = 0; i < arr->len; ++i) { \
> +            scalar_cleanup_func(&arr->first[i]); \
> +        } \
> +        g_free(arr); \
> +    } \
> +
> +/**
> + * Used to declare a reference variable (unique pointer) for an array. After
> + * leaving the scope of the reference variable, the associated array is
> + * automatically freed.
> + *
> + * @param scalar_type - type of the individual array elements
> + */
> +#define QArrayRef(scalar_type) \
> +    __attribute((__cleanup__(qarray_auto_free_##scalar_type))) scalar_type*
> +
> +/**
> + * Allocates a new array of passed @a scalar_type with @a len number of array
> + * elements and assigns the created array to the reference variable
> + * @a auto_var.
> + *
> + * @param scalar_type - type of the individual array elements
> + * @param auto_var - destination reference variable
> + * @param len - amount of array elements to be allocated immediately
> + */
> +#define QARRAY_CREATE(scalar_type, auto_var, len) \
> +    qarray_create_##scalar_type((&auto_var), len)
> +
> +#endif /* QEMU_QARRAY_H */
> -- 
> 2.20.1
> 
> 

Regards,
Daniel

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:
> > Implements deep auto free of arrays while retaining common C-style
> > squared bracket access.
> > 
> > Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> > ---
> > 
> >  include/qemu/qarray.h | 150 ++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 150 insertions(+)
> >  create mode 100644 include/qemu/qarray.h
> > 
> > diff --git a/include/qemu/qarray.h b/include/qemu/qarray.h
> > new file mode 100644
> > index 0000000000..9885e5e9ed
> > --- /dev/null
> > +++ b/include/qemu/qarray.h
> > @@ -0,0 +1,150 @@
> > 
> > +#ifndef QEMU_QARRAY_H
> > +#define QEMU_QARRAY_H
> > +
> > +/**
> > + * QArray provides a mechanism to access arrays in common C-style (e.g.
> > by
> > + * square bracket [] operator) in conjunction with reference variables
> > that + * perform deep auto free of the array when leaving the scope of
> > the auto + * reference variable. That means not only is the array itself
> > automatically + * freed, but also memory dynamically allocated by the
> > individual array + * elements.
> > + *
> > + * Example:
> > + *
> > + * Consider the following user struct @c Foo which shall be used as
> > scalar
> > + * (element) type of an array:
> > + * @code
> > + * typedef struct Foo {
> > + *     int i;
> > + *     char *s;
> > + * } Foo;
> > + * @endcode
> > + * and assume it has the following function to free memory allocated by
> > @c Foo + * instances:
> > + * @code
> > + * void free_foo(Foo *foo) {
> > + *     free(foo->s);
> > + * }
> > + * @endcode
> > + * Add the following to a shared header file:
> > + * @code
> > + * DECLARE_QARRAY_TYPE(Foo);
> > + * @endcode
> > + * and the following to a C unit file:
> > + * @code
> > + * DEFINE_QARRAY_TYPE(Foo, free_foo);
> > + * @endcode
> > + * Finally the array may then be used like this:
> > + * @code
> > + * void doSomething(int n) {
> > + *     QArrayRef(Foo) foos = NULL;
> > + *     QARRAY_CREATE(Foo, foos, n);
> > + *     for (size_t i = 0; i < n; ++i) {
> > + *         foos[i].i = i;
> > + *         foos[i].s = calloc(4096, 1);
> > + *         snprintf(foos[i].s, 4096, "foo %d", i);
> > + *     }
> > + * }
> 
> So essentially here the 'foos' variable is still a plain C array
> from POV of the caller ie it is a  'Foo *foos'

Yes, that's the main feature probably: getting rid of any (n times) manually 
written deep-free code branches while at the same time keeping the burden low, 
i.e. requiring to refactor as little existing code as possible, as it is still 
a "normal" C-array from user code perspective, just with meta info prepended 
in front of the C-array that the user code however does not need to care about 
at all:

  Contiguous Memory Space ->
  ----------------------------------------------------------
  | Array-Meta-Info | Scalar 0 | Scalar 1 | ... | Scalar n |
  ----------------------------------------------------------

  ^- QArrayFoo*     ^- Foo*

So switching between the two is just +- const_offset math.

Plus this has the positive side effect that a C-style array code is better to 
the human eye.

For now that "Array-Meta-Info" is really just the array size, in future this 
might be extended to contain a reference count for instance. And as this 
concept is actually generic code (a.k.a template code), you could also extend 
this in a way where you just pull in features that you really need in user 
code. I.e. if you don't need an atomic reference count at one place (and avoid 
the sync overhead), don't enable it for your use case. If you need it 
somewhere else, then enable it there.

> By QARRAY_CREATE() is actually allocating a 'QArrayFoo' which
> is a struct containing a 'size_t len' along with the 'Foo *foos'
> entry.
> 
> This is a clever trick, and it works in the example above,
> because the code already has the length available in a
> convenient way via the 'int n' parameter.
> 
> IMHO this makes the code less useful than it otherwise would
> be in the general case, because there are places where the code
> needs to pass around the array and its assoicated length, and
> so this with this magic hidden length, we're still left to pass
> around the length separately.
> 
> Consider this example:
> 
>   int open_conn(const char *hostname) {
>     SocketAddress *addrs;
>     size_t naddrs;
>     int ret = -1;
>     size_t i;
> 
>     if (resolve_hostname(hostname, &addrs, &naddrs) < 0)
>         return -1;
> 
>     for (i = 0; i < naddrs; i++) {
>         ...try to connect to addrs[i]...
>     }
> 
>     ret = 0
>    cleanup:
>     for (i = 0; i < naddrs; i++) {
>        qapi_free_SocketAddress(addrs[i])
>     }
>     return ret;
>   }
> 
> To simplify this it is deisrable to autofree the 'addrs'
> array.
> 
> 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.

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

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.

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.

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

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

> > + * @endcode
> > + */
> > +
> > +/**
> > + * Declares an array type for the passed @a scalar_type.
> > + *
> > + * This is typically used from a shared header file.
> > + *
> > + * @param scalar_type - type of the individual array elements
> > + */
> > +#define DECLARE_QARRAY_TYPE(scalar_type) \
> > +    typedef struct QArray##scalar_type { \
> > +        size_t len; \
> > +        scalar_type first[]; \
> > +    } QArray##scalar_type; \
> > +    \
> > +    void qarray_create_##scalar_type(scalar_type **auto_var, size_t len);
> > \ +    void qarray_auto_free_##scalar_type(scalar_type **auto_var); \ +
> > +/**
> > + * Defines an array type for the passed @a scalar_type and appropriate
> > + * @a scalar_cleanup_func.
> > + *
> > + * This is typically used from a C unit file.
> > + *
> > + * @param scalar_type - type of the individual array elements
> > + * @param scalar_cleanup_func - appropriate function to free memory
> > dynamically + *                              allocated by individual
> > array elements before + */
> > +#define DEFINE_QARRAY_TYPE(scalar_type, scalar_cleanup_func) \
> > +    void qarray_create_##scalar_type(scalar_type **auto_var, size_t len)
> > \
> > +    { \
> > +        qarray_auto_free_##scalar_type(auto_var); \
> > +        QArray##scalar_type *arr = g_malloc0(sizeof(QArray##scalar_type)
> > + \ +            len * sizeof(scalar_type)); \
> > +        arr->len = len; \
> > +        *auto_var = &arr->first[0]; \
> > +    } \
> > +    \
> > +    void qarray_auto_free_##scalar_type(scalar_type **auto_var) \
> > +    { \
> > +        scalar_type *first = (*auto_var); \
> > +        if (!first) { \
> > +            return; \
> > +        } \
> > +        QArray##scalar_type *arr = (QArray##scalar_type *) ( \
> > +            ((char *)first) - offsetof(QArray##scalar_type, first) \
> > +        ); \
> > +        for (size_t i = 0; i < arr->len; ++i) { \
> > +            scalar_cleanup_func(&arr->first[i]); \
> > +        } \
> > +        g_free(arr); \
> > +    } \
> > +
> > +/**
> > + * Used to declare a reference variable (unique pointer) for an array.
> > After + * leaving the scope of the reference variable, the associated
> > array is + * automatically freed.
> > + *
> > + * @param scalar_type - type of the individual array elements
> > + */
> > +#define QArrayRef(scalar_type) \
> > +    __attribute((__cleanup__(qarray_auto_free_##scalar_type)))
> > scalar_type* +
> > +/**
> > + * Allocates a new array of passed @a scalar_type with @a len number of
> > array + * elements and assigns the created array to the reference
> > variable + * @a auto_var.
> > + *
> > + * @param scalar_type - type of the individual array elements
> > + * @param auto_var - destination reference variable
> > + * @param len - amount of array elements to be allocated immediately
> > + */
> > +#define QARRAY_CREATE(scalar_type, auto_var, len) \
> > +    qarray_create_##scalar_type((&auto_var), len)
> > +
> > +#endif /* QEMU_QARRAY_H */
> 
> Regards,
> Daniel

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:
> > > Implements deep auto free of arrays while retaining common C-style
> > > squared bracket access.
> > > 
> > > Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> > > ---
> > > 
> > >  include/qemu/qarray.h | 150 ++++++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 150 insertions(+)
> > >  create mode 100644 include/qemu/qarray.h
> > > 
> > > diff --git a/include/qemu/qarray.h b/include/qemu/qarray.h
> > > new file mode 100644
> > > index 0000000000..9885e5e9ed
> > > --- /dev/null
> > > +++ b/include/qemu/qarray.h
> > > @@ -0,0 +1,150 @@
> > > 
> > > +#ifndef QEMU_QARRAY_H
> > > +#define QEMU_QARRAY_H
> > > +
> > > +/**
> > > + * QArray provides a mechanism to access arrays in common C-style (e.g.
> > > by
> > > + * square bracket [] operator) in conjunction with reference variables
> > > that + * perform deep auto free of the array when leaving the scope of
> > > the auto + * reference variable. That means not only is the array itself
> > > automatically + * freed, but also memory dynamically allocated by the
> > > individual array + * elements.
> > > + *
> > > + * Example:
> > > + *
> > > + * Consider the following user struct @c Foo which shall be used as
> > > scalar
> > > + * (element) type of an array:
> > > + * @code
> > > + * typedef struct Foo {
> > > + *     int i;
> > > + *     char *s;
> > > + * } Foo;
> > > + * @endcode
> > > + * and assume it has the following function to free memory allocated by
> > > @c Foo + * instances:
> > > + * @code
> > > + * void free_foo(Foo *foo) {
> > > + *     free(foo->s);
> > > + * }
> > > + * @endcode
> > > + * Add the following to a shared header file:
> > > + * @code
> > > + * DECLARE_QARRAY_TYPE(Foo);
> > > + * @endcode
> > > + * and the following to a C unit file:
> > > + * @code
> > > + * DEFINE_QARRAY_TYPE(Foo, free_foo);
> > > + * @endcode
> > > + * Finally the array may then be used like this:
> > > + * @code
> > > + * void doSomething(int n) {
> > > + *     QArrayRef(Foo) foos = NULL;
> > > + *     QARRAY_CREATE(Foo, foos, n);
> > > + *     for (size_t i = 0; i < n; ++i) {
> > > + *         foos[i].i = i;
> > > + *         foos[i].s = calloc(4096, 1);
> > > + *         snprintf(foos[i].s, 4096, "foo %d", i);
> > > + *     }
> > > + * }
> > 
> > So essentially here the 'foos' variable is still a plain C array
> > from POV of the caller ie it is a  'Foo *foos'
> 
> Yes, that's the main feature probably: getting rid of any (n times) manually 
> written deep-free code branches while at the same time keeping the burden low, 
> i.e. requiring to refactor as little existing code as possible, as it is still 
> a "normal" C-array from user code perspective, just with meta info prepended 
> in front of the C-array that the user code however does not need to care about 
> at all:
> 
>   Contiguous Memory Space ->
>   ----------------------------------------------------------
>   | Array-Meta-Info | Scalar 0 | Scalar 1 | ... | Scalar n |
>   ----------------------------------------------------------
> 
>   ^- QArrayFoo*     ^- Foo*
> 
> So switching between the two is just +- const_offset math.
> 
> Plus this has the positive side effect that a C-style array code is better to 
> the human eye.
> 
> For now that "Array-Meta-Info" is really just the array size, in future this 
> might be extended to contain a reference count for instance. And as this 
> concept is actually generic code (a.k.a template code), you could also extend 
> this in a way where you just pull in features that you really need in user 
> code. I.e. if you don't need an atomic reference count at one place (and avoid 
> the sync overhead), don't enable it for your use case. If you need it 
> somewhere else, then enable it there.
> 
> > By QARRAY_CREATE() is actually allocating a 'QArrayFoo' which
> > is a struct containing a 'size_t len' along with the 'Foo *foos'
> > entry.
> > 
> > This is a clever trick, and it works in the example above,
> > because the code already has the length available in a
> > convenient way via the 'int n' parameter.
> > 
> > IMHO this makes the code less useful than it otherwise would
> > be in the general case, because there are places where the code
> > needs to pass around the array and its assoicated length, and
> > so this with this magic hidden length, we're still left to pass
> > around the length separately.
> > 
> > Consider this example:
> > 
> >   int open_conn(const char *hostname) {
> >     SocketAddress *addrs;
> >     size_t naddrs;
> >     int ret = -1;
> >     size_t i;
> > 
> >     if (resolve_hostname(hostname, &addrs, &naddrs) < 0)
> >         return -1;
> > 
> >     for (i = 0; i < naddrs; i++) {
> >         ...try to connect to addrs[i]...
> >     }
> > 
> >     ret = 0
> >    cleanup:
> >     for (i = 0; i < naddrs; i++) {
> >        qapi_free_SocketAddress(addrs[i])
> >     }
> >     return ret;
> >   }
> > 
> > To simplify this it is deisrable to autofree the 'addrs'
> > array.
> > 
> > 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.

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

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

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

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

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

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

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:
> [...]

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

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.

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

Regards,
Daniel

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?

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.

Best regards,
Christian Schoenebeck

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

On Donnerstag, 30. September 2021 15:31:10 CEST Daniel P. Berrangé wrote:
> 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.

Hmm, I start to think whether I should just make it some 9p local utility code 
for now instead, e.g. "P9Array" or something.

Best regards,
Christian Schoenebeck

On Thu, Sep 30, 2021 at 03:55:36PM +0200, Christian Schoenebeck wrote:
> On Donnerstag, 30. September 2021 15:31:10 CEST Daniel P. Berrangé wrote:
> > 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.
> 
> Hmm, I start to think whether I should just make it some 9p local utility code 
> for now instead, e.g. "P9Array" or something.

IMHO even if it was private to a subsystem it should still be using the
standard g_auto functionality for automatically deallocating memory,
because this is a QEMU wide standard.


Regards,
Daniel

On Donnerstag, 30. September 2021 16:01:38 CEST Daniel P. Berrangé wrote:
> On Thu, Sep 30, 2021 at 03:55:36PM +0200, Christian Schoenebeck wrote:
> > On Donnerstag, 30. September 2021 15:31:10 CEST Daniel P. Berrangé wrote:
> > > 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.
> > 
> > Hmm, I start to think whether I should just make it some 9p local utility
> > code for now instead, e.g. "P9Array" or something.
> 
> IMHO even if it was private to a subsystem it should still be using the
> standard g_auto functionality for automatically deallocating memory,
> because this is a QEMU wide standard.

There are already things like V9fsString, V9fsPath, ... not introduced by me 
BTW. There is a whole bunch of stuff that you could argue that it does not 
comply with common project patterns and nobody cared about.

I follow project standards wherever possible. But in this particular case, if 
it does not fit, it simply does not fit. So I will go on making at a local 
type for now and if there is really some need for project wide usage we can 
resume that issue at a later point.

Best regards,
Christian Schoenebeck