Re: [PATCH 11/11] qdev: Rework array properties based on list visitor

[Markus Armbruster <armbru@redhat.com>]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 14.10.2023 um 08:36 hat Markus Armbruster geschrieben:
>> Kevin Wolf <kwolf@redhat.com> writes:
>> 
>> > Am 22.09.2023 um 17:05 hat Markus Armbruster geschrieben:
>> >> Kevin Wolf <kwolf@redhat.com> writes:
>> >> 
>> >> > Until now, array properties are actually implemented with a hack that
>> >> > uses multiple properties on the QOM level: a static "foo-len" property
>> >> > and after it is set, dynamically created "foo[i]" properties.
>> >> >
>> >> > In external interfaces (-device on the command line and device_add in
>> >> > QMP), this interface was broken by commit f3558b1b ('qdev: Base object
>> >> > creation on QDict rather than QemuOpts') because QDicts are unordered
>> >> > and therefore it could happen that QEMU tried to set the indexed
>> >> > properties before setting the length, which fails and effectively makes
>> >> > array properties inaccessible. In particular, this affects the 'ports'
>> >> > property of the 'rocker' device.
>> >> >
>> >> > This patch reworks the external interface so that instead of using a
>> >> > separate top-level property for the length and for each element, we use
>> >> > a single true array property that accepts a list value. In the external
>> >> > interfaces, this is naturally expressed as a JSON list and makes array
>> >> > properties accessible again.
>> >> >
>> >> > Creating an array property on the command line without using JSON format
>> >> > is currently not possible. This could be fixed by switching from
>> >> > QemuOpts to a keyval parser, which however requires consideration of the
>> >> > compatibility implications.
>> >> >
>> >> > All internal users of devices with array properties go through
>> >> > qdev_prop_set_array() at this point, so updating it takes care of all of
>> >> > them.
>> >> >
>> >> > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1090
>> >> > Fixes: f3558b1b763683bb877f7dd5b282469cdadc65c3
>> >> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>
>> >> > +{
>> >> > +    return (Property) {
>> >> > +        .info = parent_prop->arrayinfo,
>> >> > +        .name = name,
>> >> > +        /*
>> >> > +         * This ugly piece of pointer arithmetic sets up the offset so
>> >> > +         * that when the underlying release hook calls qdev_get_prop_ptr
>> >> > +         * they get the right answer despite the array element not actually
>> >> > +         * being inside the device struct.
>> >> > +         */
>> >> > +        .offset = elem - (char *) obj,
>> >> 
>> >> Isn't this is undefined behavior?
>> >
>> > It should be at least less illegal than the old version of it, which did
>> > the calculation on void * and still worked in practice...
>> >
>> > But yes, strictly speaking, it's probably undefined behaviour. I can
>> > calculate on uintptr_t instead, and then it should be defined here.
>> >
>> > The QOM counterpart object_field_prop_ptr() is probably still undefined
>> > because it calculates on a pointer and I think the spec allows casting
>> > back to a pointer only after we've applied the offset so that we stay in
>> > the same object with pointer arithmetics.
>> 
>> We should not have to waste time on worrying about compilers using UB
>> fine print against us, but sadly we do.
>> 
>> I'm not objecting to your code, I'm merely pointing out a potential time
>> bomb.  In a programming environment that has embraced time bombing with
>> gusto.
>
> While I'm touching this part, I'll change it to uintptr_t. I'll leave
> the counterpart for later.
>
>> >> Delete the space between (char *) and obj.
>> >> 
>> >> > +    };
>> >> > +}
>> >> > +
>> >> > +/*
>> >> > + * Object property release callback for array properties: We call the underlying
>> >> > + * element's property release hook for each element.
>> >> > + *
>> >> > + * Note that it is the responsibility of the individual device's deinit to free
>> >> > + * the array proper.
>> >> 
>> >> What is a device's "deinit"?  Is it the unrealize() method?  The
>> >> instance_finalize() method?
>> >
>> > Who knows? I only moved this comment.
>> 
>> Opportunity to improve it.  Not a demand.
>
> If you have a better version of it? My guess is only a guess, so I'd
> avoid putting it in the code without understanding if there are reasons
> why it has to be a specific place, or why a specific place doesn't work.
>
> Leaving it vague is probably better than being specific, but potentially
> wrong.

Fair.

>> > My guess is that it doesn't really matter as long as _something_ frees
>> > the array when unplugging the device.
>> >
>> >> >   */
>> >> > -static void array_element_release(Object *obj, const char *name, void *opaque)
>> >> > +static void release_prop_array(Object *obj, const char *name, void *opaque)
>> >> >  {
>> >> > -    ArrayElementProperty *p = opaque;
>> >> > -    if (p->release) {
>> >> > -        p->release(obj, name, opaque);
>> >> > +    Property *prop = opaque;
>> >> > +    uint32_t *alenptr = object_field_prop_ptr(obj, prop);
>> >> > +    void **arrayptr = (void *)obj + prop->arrayoffset;
>> >> 
>> >> I'd call these @plen and @pelts, but that's clearly a matter of taste.
>> >
>> > I just kept the old names in set_prop_array() and used the same names in
>> > new functions to stay consistent. But to be honest, @plen and @pelts
>> > would be equally confusing to me.
>> >
>> > My own choice would probably have been something like array_len and
>> > array_data (if you want to know that it's a pointer, look at its type).
>> >
>> >> > +    char *elem = *arrayptr;
>> >> > +    int i;
>> >> > +
>> >> > +    for (i = 0; i < *alenptr; i++) {
>> >> > +        Property elem_prop = array_elem_prop(obj, prop, name, elem);
>> >> > +        prop->arrayinfo->release(obj, NULL, &elem_prop);
>> >> > +        elem += prop->arrayfieldsize;
>> >> >      }
>> >> > -    g_free(p->propname);
>> >> > -    g_free(p);
>> >> >  }
>> >> >  
>> >> > -static void set_prop_arraylen(Object *obj, Visitor *v, const char *name,
>> >> > -                              void *opaque, Error **errp)
>> >> > +/*
>> >> > + * Setter for an array property. This sets both the array length (which is
>> >> > + * technically the property field in the object) and the array itself (a pointer
>> >> > + * to which is stored in the additional field described by prop->arrayoffset).
>> >> > + */
>> >> > +static void set_prop_array(Object *obj, Visitor *v, const char *name,
>> >> > +                           void *opaque, Error **errp)
>> >> >  {
>> >> > -    /* Setter for the property which defines the length of a
>> >> > -     * variable-sized property array. As well as actually setting the
>> >> > -     * array-length field in the device struct, we have to create the
>> >> > -     * array itself and dynamically add the corresponding properties.
>> >> > -     */
>> >> > +    ERRP_GUARD();
>> >> > +
>> >> 
>> >> Drop the blank line.
>> >> 
>> >> >      Property *prop = opaque;
>> >> >      uint32_t *alenptr = object_field_prop_ptr(obj, prop);
>> >> >      void **arrayptr = (void *)obj + prop->arrayoffset;
>> >> > -    void *eltptr;
>> >> > -    const char *arrayname;
>> >> > -    int i;
>> >> > +    GenericList *list, *elem, *next;
>> >> > +    const size_t list_elem_size = sizeof(*list) + prop->arrayfieldsize;
>> >> 
>> >> This can be smaller than the size of the QAPI-generated list type, since
>> >> the compiler may add padding.  Does it matter?
>> >
>> > If it happens in practice, it does matter. Do we have any cleaner way to
>> > get the element size without knowing the content of the list?
>> >
>> > I expect that because GenericList only contains a single pointer, the
>> > rest should have natural alignment
>> 
>> Yes, GenericList's size and alignment should match a pointer's:
>> 
>>     typedef struct GenericList {
>>         struct GenericList *next;
>>         char padding[];
>>     } GenericList;
>> 
>> >                                    and therefore the compiler shouldn't
>> > have any reason to insert padding.
>> 
>> The actual list will look like
>> 
>>     struct FOOList {
>>         FOOList *next;
>>         FOOTYPE value;
>>     }
>> 
>> where FOOTYPE is some QAPI-generated type.  No padding as long as
>> FOOTYPE's alignment divides the pointer size.  I figure that's true for
>> our current targets and generated QAPI types (currently pointers,
>> double, bool, or integers up to 64 bits).
>
> I'm quite confused about the whole alignment stuff at the moment, but
> after looking some more at the code, I think I just realised one
> important thing: We're actually not dealing with QAPI-generated types
> here, but with custom visitors in QOM property setters.

Hmm.

The patch is about qdev array properties, which are special QOM
properties.

A QOM property encapsulates arbitray C data within a QOM object.  Its
get() method enables getting this data with an output visitor, and its
set() method enables setting it with an input visitor.

When the C data is a QAPI type T, set() and get() simply call
visit_type_T().  Example: property_get_uint8_ptr(),
property_set_uint8_ptr().

When it isn't, set() and get() do what qapi/visitor.h calls a "virtual
walk".  Example: the array property we're discussing here.

However, there's a catch: set_prop_array() doesn't visit the property's
C data (a heap-allocated array of ELEMENT-TYPE) directly.  Instead, it
visits a heap-allocated GenericList of ELEMENT-TYPE, which it later
copies to the array.

Here's the visit:

    /* Read the whole input into a temporary list */
    elem = list;
    while (elem) {
        Property elem_prop = array_elem_prop(obj, prop, name, elem->padding);
--->    prop->arrayinfo->set(obj, v, NULL, &elem_prop, errp);
        if (*errp) {
            ok = false;
            goto out_obj;
        }
        (*alenptr)++;
        elem = visit_next_list(v, elem, list_elem_size);
    }

The call marked ---> visits an element at address &elem->padding.

This address is pointer-aligned.  What if the element type requires more
alignment?  Unusual, but certainly not impossible.  For instance, SSE
and AVX vectors require 16 or even 32 byte alignment.

Or am I confused?

>                                                         The netdev one
> specifically (which is used in the array property of the 'rocker'
> device) uses NICPeers (not a pointer to NICPeers!), which is much
> larger.
>
> But now I'm wondering, FOOList doesn't even exist as a type. We're
> calculating list_elem_size only to pass it to visit_start_list(), which
> doesn't have FOOList either. What does the visitor do with it?
>
> It seems it doesn't do anything with it except using it as the size to
> malloc() elements. And then we (the array property code in this patch)
> use &elem->padding as the element pointer, directly from GenericList,
> not any specific FOOList. So I think this just means that we never
> insert any padding and what the compiler would do with a hypothetical
> FOOList, which doesn't even have to exist as a type, doesn't matter.
>
> Am I missing something or does this mean that the code is actually fine?
>
>> > If you think this is not enough and there is no other way to get the
>> > size of the list elements, we might have to generate packed structs for
>> > the QAPI list types (which are really only two pointers, so not much to
>> > lose when we do that).
>> 
>> Could we assert the element type's alignment divides GenericList's size?
>> Not here, obviously, but in DEFINE_PROP_ARRAY(), where we can use
>> __alignof__(_arraytype).
>
> A bit tricky because we're in the middle of a literal, but I suppose
> with the GCC extension we could change one random element of the struct
> to something like:
>
>     ({ QEMU_BUILD_BUG_ON(...); real_value; })
>
> Of course, if my thoughts above were right, this might not actually be
> needed.
>
>> We could also play with attribute aligned to ensure GenericList's size is
>> safe, but I doubt that's worthwhile.
>> 
>> >> > +    char *elemptr;
>> >> > +    bool ok = true;
>> >> >  
>> >> >      if (*alenptr) {
>> >> >          error_setg(errp, "array size property %s may not be set more than once",
>> >> >                     name);
>> >> >          return;
>> >> >      }
>> >> > -    if (!visit_type_uint32(v, name, alenptr, errp)) {
>> >> > +
>> >> > +    if (!visit_start_list(v, name, &list, list_elem_size, errp)) {
>> >> >          return;
>> >> >      }
>> >> > -    if (!*alenptr) {
>> >> > +
>> >> > +    /* Read the whole input into a temporary list */
>> >> > +    elem = list;
>> >> > +    while (elem) {
>> >> > +        Property elem_prop = array_elem_prop(obj, prop, name, elem->padding);
>> >> > +        prop->arrayinfo->set(obj, v, NULL, &elem_prop, errp);
>> >> > +        if (*errp) {
>> >> > +            ok = false;
>> >> > +            goto out_obj;
>> >> > +        }
>> >> > +        (*alenptr)++;
>> >> > +        elem = visit_next_list(v, elem, list_elem_size);
>> >> > +    }
>> >> > +
>> >> > +    ok = visit_check_list(v, errp);
>> >> > +out_obj:
>> >> > +    visit_end_list(v, (void**) &list);
>> >> > +
>> >> > +    if (!ok) {
>> >> > +        for (elem = list; elem; elem = next) {
>> >> > +            next = elem->next;
>> >> > +            g_free(elem);
>> >> > +        }
>> >> 
>> >> We consume the list even on error.  It's too late in my day for me to
>> >> see why that's proper.
>> >
>> > Who else would free it otherwise?
>> >
>> > This is pretty much the same as the generated list visitors do.
>> 
>> Help me out: point me to the precedence you have in mind.
>
> Any QAPI generated list visiting function should do. As an example, I'm
> looking at visit_type_Qcow2BitmapInfoList():
>
>     ...
>     ok = visit_check_list(v, errp);
> out_obj:
>     visit_end_list(v, (void **)obj);
>     if (!ok && visit_is_input(v)) {
>         qapi_free_Qcow2BitmapInfoList(*obj);
>         *obj = NULL;
>     }
>     return ok;
> }
>
> On failure, it calls qapi_free_Qcow2BitmapInfoList().
>
> What's probably wrong in my code is that to be a full equivalent, it
> also needs to free things behind pointers that are owned by elem, i.e.
> call prop->arrayinfo->release().
>
> In practice, it doesn't currently make a difference because none of the
> types used with array properties actually have a release callback.
> Should still be fixed, of course.

Glad I got sufficiently confused to ask ;)

>> >> >          return;
>> >> >      }
>> >> >  
>> >> > -    /* DEFINE_PROP_ARRAY guarantees that name should start with this prefix;
>> >> > -     * strip it off so we can get the name of the array itself.
>> >> > +    /*
>> >> > +     * Now that we know how big the array has to be, move the data over to a
>> >> > +     * linear array and free the temporary list.
>> >> >       */
>> >> > -    assert(strncmp(name, PROP_ARRAY_LEN_PREFIX,
>> >> > -                   strlen(PROP_ARRAY_LEN_PREFIX)) == 0);
>> >> > -    arrayname = name + strlen(PROP_ARRAY_LEN_PREFIX);
>> >> > +    *arrayptr = g_malloc_n(*alenptr, prop->arrayfieldsize);
>> >> > +    elemptr = *arrayptr;
>> >> > +    for (elem = list; elem; elem = next) {
>> >> > +        memcpy(elemptr, elem->padding, prop->arrayfieldsize);
>> >> > +        elemptr += prop->arrayfieldsize;
>> >> > +        next = elem->next;
>> >> > +        g_free(elem);
>> >> > +    }
>> >> > +}
>> >> >  
>> >> > -    /* Note that it is the responsibility of the individual device's deinit
>> >> > -     * to free the array proper.
>> >> > -     */
>> >> > -    *arrayptr = eltptr = g_malloc0(*alenptr * prop->arrayfieldsize);
>> >> > -    for (i = 0; i < *alenptr; i++, eltptr += prop->arrayfieldsize) {
>> >> > -        char *propname = g_strdup_printf("%s[%d]", arrayname, i);
>> >> > -        ArrayElementProperty *arrayprop = g_new0(ArrayElementProperty, 1);
>> >> > -        arrayprop->release = prop->arrayinfo->release;
>> >> > -        arrayprop->propname = propname;
>> >> > -        arrayprop->prop.info = prop->arrayinfo;
>> >> > -        arrayprop->prop.name = propname;
>> >> > -        /* This ugly piece of pointer arithmetic sets up the offset so
>> >> > -         * that when the underlying get/set hooks call qdev_get_prop_ptr
>> >> > -         * they get the right answer despite the array element not actually
>> >> > -         * being inside the device struct.
>> >> > -         */
>> >> > -        arrayprop->prop.offset = eltptr - (void *)obj;
>> >> > -        assert(object_field_prop_ptr(obj, &arrayprop->prop) == eltptr);
>> >> > -        object_property_add(obj, propname,
>> >> > -                            arrayprop->prop.info->name,
>> >> > -                            field_prop_getter(arrayprop->prop.info),
>> >> > -                            field_prop_setter(arrayprop->prop.info),
>> >> > -                            array_element_release,
>> >> > -                            arrayprop);
>> >> > +static void get_prop_array(Object *obj, Visitor *v, const char *name,
>> >> > +                           void *opaque, Error **errp)
>> >> > +{
>> >> > +    ERRP_GUARD();
>> >> > +
>> >> 
>> >> Drop the blank line.
>> >> 
>> >> > +    Property *prop = opaque;
>> >> > +    uint32_t *alenptr = object_field_prop_ptr(obj, prop);
>> >> > +    void **arrayptr = (void *)obj + prop->arrayoffset;
>> >> > +    char *elem = *arrayptr;
>> >> > +    GenericList *list;
>> >> > +    const size_t list_elem_size = sizeof(*list) + prop->arrayfieldsize;
>> >> > +    int i;
>> >> > +
>> >> > +    if (!visit_start_list(v, name, &list, list_elem_size, errp)) {
>> >> > +        return;
>> >> >      }
>> >> > +
>> >> > +    for (i = 0; i < *alenptr; i++) {
>> >> > +        Property elem_prop = array_elem_prop(obj, prop, name, elem);
>> >> > +        prop->arrayinfo->get(obj, v, NULL, &elem_prop, errp);
>> >> > +        if (*errp) {
>> >> > +            goto out_obj;
>> >> > +        }
>> >> > +        elem += prop->arrayfieldsize;
>> >> > +    }
>> >> > +
>> >> 
>> >> You neglect to call visit_check_list().
>> >
>> > It is documented to be intended for input visitors only. Do we need it
>> > with an output visitor?
>> 
>> Help me out: where is that documented?
>
> In struct Visitor:
>
>     /* Optional; intended for input visitors */
>     bool (*check_list)(Visitor *v, Error **errp);

I think this comment tries to say "this is intended to be used by input
visitors", which doesn't quite imply "only input visitors use this".

> And indeed, the existing output vistors don't implement it.

It's implemented by all the input visitors.

Fine print: the forward visitor is the same kind of visitor as the one
it wraps, either input or output.  It implements check_list()
unconditionally.

> Admittedly, the public visit_check_list() is less clear:
>
>     /*
>      * Prepare for completing a list visit.
>      *
>      * On failure, store an error through @errp.  Can happen only when @v
>      * is an input visitor.
>      *
>      * Return true on success, false on failure.
>      *
>      * Should be called prior to visit_end_list() if all other
>      * intermediate visit steps were successful, to allow the visitor one
>      * last chance to report errors.  May be skipped on a cleanup path,
>      * where there is no need to check for further errors.
>      */
>     bool visit_check_list(Visitor *v, Error **errp);
>
> If you prefer, I can add it here just to be sure. Instead of having real
> error handling, assert(ok) should be enough.

We either tighten the public visitor contract to make calling
visit_check_list() clearly optional when you know you're not using an
input visitor.

Or we call it here, too.  Probably simpler.

>> >> > +out_obj:
>> >> > +    visit_end_list(v, (void**) &list);
>> >> >  }
>
> Kevin