qdev: Rework array properties based on list visitor

Until now, array properties are actually implemented with a hack that
uses multiple properties on the QOM level: a static "foo-len" property
and after it is set, dynamically created "foo[i]" properties.

In external interfaces (-device on the command line and device_add in
QMP), this interface was broken by commit f3558b1b ('qdev: Base object
creation on QDict rather than QemuOpts') because QDicts are unordered
and therefore it could happen that QEMU tried to set the indexed
properties before setting the length, which fails and effectively makes
array properties inaccessible. In particular, this affects the 'ports'
property of the 'rocker' device, which used to be configured like this:

-device rocker,len-ports=2,ports[0]=dev0,ports[1]=dev1

This patch reworks the external interface so that instead of using a
separate top-level property for the length and for each element, we use
a single true array property that accepts a list value. In the external
interfaces, this is naturally expressed as a JSON list and makes array
properties accessible again. The new syntax looks like this:

-device '{"driver":"rocker","ports":["dev0","dev1"]}'

Creating an array property on the command line without using JSON format
is currently not possible. This could be fixed by switching from
QemuOpts to a keyval parser, which however requires consideration of the
compatibility implications.

All internal users of devices with array properties go through
qdev_prop_set_array() at this point, so updating it takes care of all of
them.

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1090
Fixes: f3558b1b763683bb877f7dd5b282469cdadc65c3
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Message-ID: <20231109174240.72376-12-kwolf@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>

1 files changed, 157 insertions, 80 deletions

diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
index 950ef48e01..91632f7be9 100644
--- a/hw/core/qdev-properties.c
+++ b/hw/core/qdev-properties.c
@@ -546,98 +546,187 @@ 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 ArrayElementList ArrayElementList;
+
+struct ArrayElementList {
+    ArrayElementList *next;
+    void *value;
+};
+
+/*
+ * Given an array property @parent_prop in @obj, return a Property for a
+ * specific element of the array. Arrays are backed by an uint32_t length field
+ * and an element array. @elem points at an element in this element array.
  */
-typedef struct {
-    struct Property prop;
-    char *propname;
-    ObjectPropertyRelease *release;
-} ArrayElementProperty;
-
-/* object property release callback for array element properties:
- * we call the underlying element's property release hook, and
- * then free the memory we allocated when we added the property.
+static Property array_elem_prop(Object *obj, Property *parent_prop,
+                                const char *name, char *elem)
+{
+    return (Property) {
+        .info = parent_prop->arrayinfo,
+        .name = name,
+        /*
+         * This ugly piece of pointer arithmetic sets up the offset so
+         * that when the underlying release hook calls qdev_get_prop_ptr
+         * they get the right answer despite the array element not actually
+         * being inside the device struct.
+         */
+        .offset = (uintptr_t)elem - (uintptr_t)obj,
+    };
+}
+
+/*
+ * Object property release callback for array properties: We call the
+ * underlying element's property release hook for each element.
+ *
+ * Note that it is the responsibility of the individual device's deinit
+ * to free the array proper.
  */
-static void array_element_release(Object *obj, const char *name, void *opaque)
+static void release_prop_array(Object *obj, const char *name, void *opaque)
 {
-    ArrayElementProperty *p = opaque;
-    if (p->release) {
-        p->release(obj, name, opaque);
+    Property *prop = opaque;
+    uint32_t *alenptr = object_field_prop_ptr(obj, prop);
+    void **arrayptr = (void *)obj + prop->arrayoffset;
+    char *elem = *arrayptr;
+    int i;
+
+    if (!prop->arrayinfo->release) {
+        return;
+    }
+
+    for (i = 0; i < *alenptr; i++) {
+        Property elem_prop = array_elem_prop(obj, prop, name, elem);
+        prop->arrayinfo->release(obj, NULL, &elem_prop);
+        elem += prop->arrayfieldsize;
     }
-    g_free(p->propname);
-    g_free(p);
 }
 
-static void set_prop_arraylen(Object *obj, Visitor *v, const char *name,
-                              void *opaque, Error **errp)
+/*
+ * Setter for an array property. This sets both the array length (which
+ * is technically the property field in the object) and the array itself
+ * (a pointer to which is stored in the additional field described by
+ * prop->arrayoffset).
+ */
+static void set_prop_array(Object *obj, Visitor *v, const char *name,
+                           void *opaque, Error **errp)
 {
-    /* Setter for the property which defines the length of a
-     * variable-sized property array. As well as actually setting the
-     * array-length field in the device struct, we have to create the
-     * array itself and dynamically add the corresponding properties.
-     */
+    ERRP_GUARD();
     Property *prop = opaque;
     uint32_t *alenptr = object_field_prop_ptr(obj, prop);
     void **arrayptr = (void *)obj + prop->arrayoffset;
-    void *eltptr;
-    const char *arrayname;
-    int i;
+    ArrayElementList *list, *elem, *next;
+    const size_t size = sizeof(*list);
+    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, (GenericList **) &list, size, errp)) {
         return;
     }
-    if (!*alenptr) {
+
+    /* Read the whole input into a temporary list */
+    elem = list;
+    while (elem) {
+        Property elem_prop;
+
+        elem->value = g_malloc0(prop->arrayfieldsize);
+        elem_prop = array_elem_prop(obj, prop, name, elem->value);
+        prop->arrayinfo->set(obj, v, NULL, &elem_prop, errp);
+        if (*errp) {
+            ok = false;
+            goto out_obj;
+        }
+        if (*alenptr == INT_MAX) {
+            error_setg(errp, "array is too big");
+            return;
+        }
+        (*alenptr)++;
+        elem = (ArrayElementList *) visit_next_list(v, (GenericList*) elem,
+                                                    size);
+    }
+
+    ok = visit_check_list(v, errp);
+out_obj:
+    visit_end_list(v, (void**) &list);
+
+    if (!ok) {
+        for (elem = list; elem; elem = next) {
+            Property elem_prop = array_elem_prop(obj, prop, name,
+                                                 elem->value);
+            if (prop->arrayinfo->release) {
+                prop->arrayinfo->release(obj, NULL, &elem_prop);
+            }
+            next = elem->next;
+            g_free(elem->value);
+            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->value, prop->arrayfieldsize);
+        elemptr += prop->arrayfieldsize;
+        next = elem->next;
+        g_free(elem->value);
+        g_free(elem);
+    }
+}
 
-    /* Note that it is the responsibility of the individual device's deinit
-     * to free the array proper.
-     */
-    *arrayptr = eltptr = g_malloc0(*alenptr * prop->arrayfieldsize);
-    for (i = 0; i < *alenptr; i++, eltptr += prop->arrayfieldsize) {
-        char *propname = g_strdup_printf("%s[%d]", arrayname, i);
-        ArrayElementProperty *arrayprop = g_new0(ArrayElementProperty, 1);
-        arrayprop->release = prop->arrayinfo->release;
-        arrayprop->propname = propname;
-        arrayprop->prop.info = prop->arrayinfo;
-        arrayprop->prop.name = propname;
-        /* This ugly piece of pointer arithmetic sets up the offset so
-         * that when the underlying get/set hooks call qdev_get_prop_ptr
-         * they get the right answer despite the array element not actually
-         * being inside the device struct.
-         */
-        arrayprop->prop.offset = eltptr - (void *)obj;
-        assert(object_field_prop_ptr(obj, &arrayprop->prop) == eltptr);
-        object_property_add(obj, propname,
-                            arrayprop->prop.info->name,
-                            field_prop_getter(arrayprop->prop.info),
-                            field_prop_setter(arrayprop->prop.info),
-                            array_element_release,
-                            arrayprop);
+static void get_prop_array(Object *obj, Visitor *v, const char *name,
+                           void *opaque, Error **errp)
+{
+    ERRP_GUARD();
+    Property *prop = opaque;
+    uint32_t *alenptr = object_field_prop_ptr(obj, prop);
+    void **arrayptr = (void *)obj + prop->arrayoffset;
+    char *elem = *arrayptr;
+    GenericList *list;
+    const size_t list_elem_size = sizeof(*list) + prop->arrayfieldsize;
+    int i;
+    bool ok;
+
+    if (!visit_start_list(v, name, &list, list_elem_size, errp)) {
+        return;
     }
+
+    for (i = 0; i < *alenptr; i++) {
+        Property elem_prop = array_elem_prop(obj, prop, name, elem);
+        prop->arrayinfo->get(obj, v, NULL, &elem_prop, errp);
+        if (*errp) {
+            goto out_obj;
+        }
+        elem += prop->arrayfieldsize;
+    }
+
+    /* visit_check_list() can only fail for input visitors */
+    ok = visit_check_list(v, errp);
+    assert(ok);
+
+out_obj:
+    visit_end_list(v, (void**) &list);
 }
 
-const PropertyInfo qdev_prop_arraylen = {
-    .name = "uint32",
-    .get = get_uint32,
-    .set = set_prop_arraylen,
-    .set_default_value = qdev_propinfo_set_default_value_uint,
+static void default_prop_array(ObjectProperty *op, const Property *prop)
+{
+    object_property_set_default_list(op);
+}
+
+const PropertyInfo qdev_prop_array = {
+    .name = "list",
+    .get = get_prop_array,
+    .set = set_prop_array,
+    .release = release_prop_array,
+    .set_default_value = default_prop_array,
 };
 
 /* --- public helpers --- */
@@ -743,20 +832,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);
 }