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

[Kevin Wolf]

Am 14.09.2023 um 12:24 hat Peter Maydell geschrieben:
> On Fri, 8 Sept 2023 at 15:37, Kevin Wolf <kwolf@redhat.com> wrote:
> >
> > 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>
> 
> I'm hoping that somebody who understands the visitor APIs
> better than me will have a look at this patch, but in the
> meantime, here's my review, which I suspect has a lot of
> comments that mostly reflect that I don't really understand
> the visitor stuff...

I discussed the visitor aspects with Markus before sending the series,
so I think he agrees with the approach. But I wouldn't mind an explicit
Reviewed-by, of course.

> > ---
> >  include/hw/qdev-properties.h     |  23 ++--
> >  hw/core/qdev-properties-system.c |   2 +-
> >  hw/core/qdev-properties.c        | 204 +++++++++++++++++++------------
> >  3 files changed, 133 insertions(+), 96 deletions(-)
> >
> > diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
> > index 7fa2fdb7c9..9370b36b72 100644
> > --- a/include/hw/qdev-properties.h
> > +++ b/include/hw/qdev-properties.h
> > @@ -61,7 +61,7 @@ extern const PropertyInfo qdev_prop_size;
> >  extern const PropertyInfo qdev_prop_string;
> >  extern const PropertyInfo qdev_prop_on_off_auto;
> >  extern const PropertyInfo qdev_prop_size32;
> > -extern const PropertyInfo qdev_prop_arraylen;
> > +extern const PropertyInfo qdev_prop_array;
> >  extern const PropertyInfo qdev_prop_link;
> >
> >  #define DEFINE_PROP(_name, _state, _field, _prop, _type, ...) {  \
> > @@ -115,8 +115,6 @@ extern const PropertyInfo qdev_prop_link;
> >                  .bitmask    = (_bitmask),                     \
> >                  .set_default = false)
> >
> > -#define PROP_ARRAY_LEN_PREFIX "len-"
> > -
> >  /**
> >   * DEFINE_PROP_ARRAY:
> >   * @_name: name of the array
> > @@ -127,24 +125,21 @@ extern const PropertyInfo qdev_prop_link;
> >   * @_arrayprop: PropertyInfo defining what property the array elements have
> >   * @_arraytype: C type of the array elements
> >   *
> > - * Define device properties for a variable-length array _name.  A
> > - * static property "len-arrayname" is defined. When the device creator
> > - * sets this property to the desired length of array, further dynamic
> > - * properties "arrayname[0]", "arrayname[1]", ...  are defined so the
> > - * device creator can set the array element values. Setting the
> > - * "len-arrayname" property more than once is an error.
> > + * Define device properties for a variable-length array _name.  The array 
> > is
> > + * represented as a list in the visitor interface.
> > + *
> > + * @_arraytype is required to be movable with memcpy().
> >   *
> > - * When the array length is set, the @_field member of the device
> > + * When the array property is set, the @_field member of the device
> >   * struct is set to the array length, and @_arrayfield is set to point
> > - * to (zero-initialised) memory allocated for the array.  For a zero
> > - * length array, @_field will be set to 0 and @_arrayfield to NULL.
> > + * to the memory allocated for the array.
> > + *
> >   * It is the responsibility of the device deinit code to free the
> >   * @_arrayfield memory.
> >   */
> >  #define DEFINE_PROP_ARRAY(_name, _state, _field,               \
> >                            _arrayfield, _arrayprop, _arraytype) \
> > -    DEFINE_PROP((PROP_ARRAY_LEN_PREFIX _name),                 \
> > -                _state, _field, qdev_prop_arraylen, uint32_t,  \
> > +    DEFINE_PROP(_name, _state, _field, qdev_prop_array, uint32_t,     \
> >                  .set_default = true,                           \
> >                  .defval.u = 0,                                 \
> >                  .arrayinfo = &(_arrayprop),                    \
> > diff --git a/hw/core/qdev-properties-system.c 
> > b/hw/core/qdev-properties-system.c
> > index 6d5d43eda2..f557ee886e 100644
> > --- a/hw/core/qdev-properties-system.c
> > +++ b/hw/core/qdev-properties-system.c
> > @@ -450,7 +450,7 @@ static void set_netdev(Object *obj, Visitor *v, const 
> > char *name,
> >      peers_ptr->queues = queues;
> >
> >  out:
> > -    error_set_from_qdev_prop_error(errp, err, obj, name, str);
> > +    error_set_from_qdev_prop_error(errp, err, obj, prop->name, str);
> >      g_free(str);
> >  }
> 
> Is this change intentional? It's not clear to me why the netdev
> property setter needs to change.

It is, but you're also right that it's not obvious. I should mention
this hunk in the commit message or even move it to a separate patch.

The problem is that @name is primarily used for the visitor interface,
and for list elements it has to be NULL (because each element doesn't
have a separate name; passing a non-NULL value runs into assertion
failures in the visitor code).

But of course, in error messages "(null)" isn't very helpful to identify
what QEMU is complaining about. prop->name contains the name of the
array property, so that's what I changed it to. Outside of lists, I
think name and prop->name are always the same.

> > diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
> > index 950ef48e01..b2303a6fbc 100644
> > --- a/hw/core/qdev-properties.c
> > +++ b/hw/core/qdev-properties.c
> > @@ -546,98 +546,152 @@ const PropertyInfo qdev_prop_size32 = {
> >
> >  /* --- support for array properties --- */
> >
> > -/* Used as an opaque for the object properties we add for each
> > - * array element. Note that the struct Property must be first
> > - * in the struct so that a pointer to this works as the opaque
> > - * for the underlying element's property hooks as well as for
> > - * our own release callback.
> > - */
> > -typedef struct {
> > -    struct Property prop;
> > -    char *propname;
> > -    ObjectPropertyRelease *release;
> > -} ArrayElementProperty;
> > -
> > -/* object property release callback for array element properties:
> > - * we call the underlying element's property release hook, and
> > - * then free the memory we allocated when we added the property.
> > +static Property array_elem_prop(Object *obj, Property *parent_prop,
> > +                                const char *name, char *elem)
> > +{
> > +    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,
> 
> Stray space after ')'.
> 
> > +    };
> > +}
> > +
> > +/*
> > + * 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.
> >   */
> > -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;
> > +    char *elem = *arrayptr;
> > +    int i;
> > +
> > +    for (i = 0; i < *alenptr; i++) {
> 
> Is there something somewhere that enforces that a list can't
> have more than INT_MAX elements? Otherwise this will go wrong,
> I think, since we're iterating with an 'int'.

I expect you might run out of memory or get another problem before that,
but you're right, there is no explicit check. Maybe set_prop_array()
should check that *alenptr doesn't become larger than some limit. It
could be INT_MAX or maybe even something lower.

> > +        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)
> 
> Is there something somewhere in this that guards against the
> caller trying to set an array property with a list that doesn't
> have elements that are all the same type?

Kind of, we always call the same prop->arrayinfo->set() callback for
every element, which is what visits the element. So unless that is doing
crazy things, it should always parse the same structure.

If we ever get to QAPIfying device_add/-device, QAPI would actually
enforce it in a way that individual property implementations can't work
around. For now, we just rely on property implementations being
sensible.

> >  {
> > -    /* 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();
> > +
> >      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;
> > +    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 */
> 
> Why do we need the temporary list? Shouldn't we already know
> at this point the length of the list and be able to allocate
> the memory and write directly into that?

No, unfortunately we don't know the length yet. I discussed adding a
visitor interface to return the length with Markus, but even ignoring
the complexity of defining the finer details of that interface and
implementing it in all visitor types, in the end it would essentially
mean that the visitor parses the input twice, which isn't any better.

> > +    elem = list;
> > +    while (elem) {
> > +        Property elem_prop = array_elem_prop(obj, prop, name, 
> > elem->padding);
> > +        prop->arrayinfo->set(obj, v, NULL, &elem_prop, errp);
> 
> Why do we call a set() function if we're getting the value
> of the array item ?

This is set_prop_array(), i.e. the setter for the whole property array.
This line sets individual elements that will be in the array (on the QOM
level), so we need the setter for the element property - which of course
does internally get values from the visitor, but it's the set method of
the QOM property, not of the visitor.

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

Kevin