| Message ID | 20230908143703.172758-12-kwolf@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | qdev: Make array properties user accessible again | expand |
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. Could we have a specific example in the commit message of: The old (currently broken) syntax for setting the ports property on the rocker device is: -device rocker,len-ports=2,ports[0]=dev0,ports[1]=dev1 The new syntax that works as of this commit is: [whatever] ? I would expect most users have no idea what the JSON list syntax is. thanks -- PMM
Am 08.09.2023 um 17:18 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. > > Could we have a specific example in the commit message of: > > The old (currently broken) syntax for setting the ports > property on the rocker device is: > -device rocker,len-ports=2,ports[0]=dev0,ports[1]=dev1 > The new syntax that works as of this commit is: > [whatever] > > ? Sure, that's a good idea. > I would expect most users have no idea what the JSON list > syntax is. To fill in your "[whatever]", it's something like this: -device '{"driver":"rocker","ports":["dev0","dev1"]}' If we can eventually get -device converted to the keyval parser instead of QemuOpts, the non-JSON syntax will look like this: -device rocker,ports.0=dev0,ports.1=dev1 But I assume we'll have to solve some compatibility problems with other devices before this can be done. Kevin
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... > --- > 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. > 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'. > + 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? > { > - /* 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? > + 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 ? > + 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); > + } > +} > > - /* 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; > + > + 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; > + } > + > +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 +797,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); > - uint32_t 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); > } > > -- > 2.41.0 thanks -- PMM
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
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> > --- > 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 Please wrap comments around column 70. More of the same below, not noted again. > + * 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. Worth mentioning that the @field member must be uint32_t? > + * > * 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), \ The backslashes are no longer aligned. > 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); > } > Intentional? > 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) @parent_prop is an array property. It's backed by an uint32_t length and an element array. @elem points into the element array. Correct? > +{ > + 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? 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? > */ > -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. > + 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? > + 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. > 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(). > +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 +797,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); > - uint32_t 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); > } I like this very much.
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> > > --- > > 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 > > Please wrap comments around column 70. More of the same below, not > noted again. My comments are consistent with the existing style of this file. > > + * 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. > > Worth mentioning that the @field member must be uint32_t? It's actually already mentioned above in the description of the fields. > > + * > > * 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), \ > > The backslashes are no longer aligned. > > > 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); > > } > > > > Intentional? Yes, as I had explained to Peter. I'll make this a separate patch in v2 and explain in the commit message. > > 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) > > @parent_prop is an array property. It's backed by an uint32_t length > and an element array. @elem points into the element array. Correct? Correct. > > +{ > > + 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. > 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. 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 and therefore the compiler shouldn't have any reason to insert padding. 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). > > + 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. > > 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? > > +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 +797,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); > > - uint32_t 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); > > } > > I like this very much. Thanks for the review. Kevin
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> [...] >> > 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) >> >> @parent_prop is an array property. It's backed by an uint32_t length >> and an element array. @elem points into the element array. Correct? > > Correct. Worth explaining in a comment? >> > +{ >> > + 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. >> 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. > 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). > 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). 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. >> > 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? >> > +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 +797,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); >> > - uint32_t 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); >> > } >> >> I like this very much. > > Thanks for the review. You're welcome.
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. > > 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. 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. > >> > 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); And indeed, the existing output vistors don't implement it. 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. > >> > +out_obj: > >> > + visit_end_list(v, (void**) &list); > >> > } Kevin
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
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); } 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, + }; +} + +/* + * 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++) { + 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; + } + (*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); + } +} - /* 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; + + 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; + } + +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 +797,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); - uint32_t 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); }
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> --- 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(-)