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

[Markus Armbruster]

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, which used to be configured like this:
>
> -device rocker,len-ports=2,ports[0]=dev0,ports[1]=dev1
>
> 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. The new syntax looks like this:
>
> -device '{"driver":"rocker","ports":["dev0","dev1"]}'
>
> 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>
> ---
>  include/hw/qdev-properties.h |  59 +++++----
>  hw/core/qdev-properties.c    | 224 ++++++++++++++++++++++-------------
>  2 files changed, 183 insertions(+), 100 deletions(-)
>
> diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
> index 7fa2fdb7c9..cac752bade 100644
> --- a/include/hw/qdev-properties.h
> +++ b/include/hw/qdev-properties.h
> @@ -1,7 +1,10 @@
>  #ifndef QEMU_QDEV_PROPERTIES_H
>  #define QEMU_QDEV_PROPERTIES_H
>  
> +#include <stdalign.h>
> +

First use of this header in QEMU.  You need it for macro alignof().  We
use GCC extension __alignof__ elsewhere.  Recommend to stick to that.

>  #include "hw/qdev-core.h"
> +#include "qapi/visitor.h"

This one you need just for GenericList.  Might want to move GenericList
into its own header.

>  
>  /**
>   * Property:
> @@ -61,7 +64,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 +118,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,28 +128,46 @@ 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.
>   *
> - * When the array length is set, the @_field member of the device
> + * @_arraytype is required to be movable with memcpy() and to have an 
> alignment
> + * such that it can be stored at GenericList.padding.


Please wrap like this for readability:

    * 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() and to have an
    * alignment such that it can be stored at GenericList.padding.

> + *
> + * 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,  \
> -                .set_default = true,                           \
> -                .defval.u = 0,                                 \
> -                .arrayinfo = &(_arrayprop),                    \
> -                .arrayfieldsize = sizeof(_arraytype),          \
> +#define DEFINE_PROP_ARRAY(_name, _state, _field,                        \
> +                          _arrayfield, _arrayprop, _arraytype)          \
> +    DEFINE_PROP(_name, _state, _field, qdev_prop_array, uint32_t,       \
> +                .set_default = true,                                    \
> +                .defval.u = 0,                                          \
> +                .arrayinfo = &(_arrayprop),                             \
> +                /*                                                      \
> +                 * set_prop_array() temporarily stores elements at      \
> +                 * GenericList.padding. Make sure that this has the     \
> +                 * right alignment for @_arraytype.                     \
> +                 *                                                      \
> +                 * Hack: In this place, neither static assertions work  \
> +                 * nor is a statement expression allowed. This          \
> +                 * abomination of an expression works because inside    \
> +                 * the declaration of a dummy struct, static assertions \
> +                 * are possible. Using the comma operator causes        \
> +                 * warnings about an unused value and casting to void   \
> +                 * makes the expression not constant in gcc, so instead \
> +                 * of ignoring the first part, make it evaluate to 0    \
> +                 * and add it to the actual result.                     \
> +                 */                                                     \
> +                .arrayfieldsize = (!sizeof(struct {                     \
> +                    QEMU_BUILD_BUG_ON(                                  \
> +                        !QEMU_IS_ALIGNED(sizeof(GenericList),           \
> +                                         alignof(_arraytype)));         \
> +                    int dummy;                                          \
> +                }) + sizeof(_arraytype)),                               \
>                  .arrayoffset = offsetof(_state, _arrayfield))
>  
>  #define DEFINE_PROP_LINK(_name, _state, _field, _type, _ptr_type)     \
> diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
> index fb4daba799..4f3e1152be 100644
> --- a/hw/core/qdev-properties.c
> +++ b/hw/core/qdev-properties.c
> @@ -546,98 +546,174 @@ 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.
> +/*
> + * Given an array property @parent_prop in @obj, return a Property for a
> + * specific element of the array. Arrays are backed by an uint32_t length 
> field
> + * and an element array. @elem points at an element in this element array.
>   */

Please wrap like this for readability:

   /*
    * Given an array property @parent_prop in @obj, return a Property for
    * a specific element of the array.  Arrays are backed by an uint32_t
    * length field and an element array.  @elem points at an element in
    * this element array.
    */
> -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 = (uintptr_t)elem - (uintptr_t)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.
>   */
> -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;
> +
> +    if (!prop->arrayinfo->release) {
> +        return;
> +    }
> +
> +    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();
>      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 */
> +    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;
> +        }
> +        if (*alenptr == INT_MAX) {
> +            error_setg(errp, "array is too big");
> +            return;
> +        }
> +        (*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) {
> +            Property elem_prop = array_elem_prop(obj, prop, name,
> +                                                 elem->padding);
> +            if (prop->arrayinfo->release) {
> +                prop->arrayinfo->release(obj, NULL, &elem_prop);
> +            }
> +            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);
> +    }
> +}
>  
> -    /* 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();
> +    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;
> +    bool ok;
> +
> +    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;
> +    }
> +
> +    /* visit_check_list() can only fail for input visitors */
> +    ok = visit_check_list(v, errp);
> +    assert(ok);
> +
> +out_obj:
> +    visit_end_list(v, (void**) &list);
>  }
>  
> -const PropertyInfo qdev_prop_arraylen = {
> -    .name = "uint32",
> -    .get = get_uint32,
> -    .set = set_prop_arraylen,
> -    .set_default_value = qdev_propinfo_set_default_value_uint,
> +static void default_prop_array(ObjectProperty *op, const Property *prop)
> +{
> +    object_property_set_default_list(op);
> +}
> +
> +const PropertyInfo qdev_prop_array = {
> +    .name = "list",
> +    .get = get_prop_array,
> +    .set = set_prop_array,
> +    .release = release_prop_array,
> +    .set_default_value = default_prop_array,
>  };
>  
>  /* --- public helpers --- */
> @@ -743,20 +819,8 @@ void qdev_prop_set_enum(DeviceState *dev, const char 
> *name, int value)
>  
>  void qdev_prop_set_array(DeviceState *dev, const char *name, QList *values)
>  {
> -    const QListEntry *entry;
> -    g_autofree char *prop_len = g_strdup_printf("len-%s", name);
> -    unsigned i = 0;
> -
> -    object_property_set_int(OBJECT(dev), prop_len, qlist_size(values),
> -                            &error_abort);
> -
> -    QLIST_FOREACH_ENTRY(values, entry) {
> -        g_autofree char *prop_idx = g_strdup_printf("%s[%u]", name, i);
> -        object_property_set_qobject(OBJECT(dev), prop_idx, entry->value,
> -                                    &error_abort);
> -        i++;
> -    }
> -
> +    object_property_set_qobject(OBJECT(dev), name, QOBJECT(values),
> +                                &error_abort);
>      qobject_unref(values);
>  }

Appreciate the comments explaining the hairy parts.

Reviewed-by: Markus Armbruster <armbru@redhat.com>