Kevin Wolf <kw...@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 <kw...@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 <arm...@redhat.com>
