[PATCH v2 00/12] qdev: Make array properties user accessible again

[PATCH v2 00/12] qdev: Make array properties user accessible again

[Kevin Wolf]

Array properties have been inaccessible since commit f3558b1b both on
the command line and in QMP. This series reworks them so that they are
made accessible again in these external interfaces, this time as JSON
lists. See patch 12 for details.

v2:
- Patch 1: Use unsigned instead of uint32_t
- Patch 9: Fixed build error
- Patch 11: Split into a separate patch to clarify the intention
- Patch 12:
  * Improved the commit message
  * Document and statically assert alignment requirements for array
    elements (the static assertion turned out to be much uglier than I
    had hoped, but it is what it is)
  * Replace UB in pointer arithmetics with uintptr_t calculations
  * Fix properties without a .release callback
  * Check array size for integer overflow
  * Call visit_check_list() even for output visitors
- Coding style changes

Kevin Wolf (12):
  qdev: Add qdev_prop_set_array()
  hw/i386/pc: Use qdev_prop_set_array()
  hw/arm/mps2-tz: Use qdev_prop_set_array()
  hw/arm/mps2: Use qdev_prop_set_array()
  hw/arm/sbsa-ref: Use qdev_prop_set_array()
  hw/arm/vexpress: Use qdev_prop_set_array()
  hw/arm/virt: Use qdev_prop_set_array()
  hw/arm/xlnx-versal: Use qdev_prop_set_array()
  hw/rx/rx62n: Use qdev_prop_set_array()
  qom: Add object_property_set_default_list()
  qdev: Make netdev properties work as list elements
  qdev: Rework array properties based on list visitor

 include/hw/qdev-properties.h     |  62 ++++++---
 include/qom/object.h             |   8 ++
 hw/arm/mps2-tz.c                 |  10 +-
 hw/arm/mps2.c                    |  12 +-
 hw/arm/sbsa-ref.c                |   7 +-
 hw/arm/vexpress.c                |  21 +--
 hw/arm/virt.c                    |  31 +++--
 hw/arm/xlnx-versal.c             |   9 +-
 hw/core/qdev-properties-system.c |   2 +-
 hw/core/qdev-properties.c        | 217 +++++++++++++++++++++----------
 hw/i386/pc.c                     |   8 +-
 hw/rx/rx62n.c                    |  19 +--
 qom/object.c                     |   6 +
 13 files changed, 278 insertions(+), 134 deletions(-)

-- 
2.41.0

[PATCH v2 01/12] qdev: Add qdev_prop_set_array()

[Kevin Wolf]

Instead of exposing the ugly hack of how we represent arrays in qdev (a
static "foo-len" property and after it is set, dynamically created
"foo[i]" properties) to boards, add an interface that allows setting the
whole array at once.

Once all internal users of devices with array properties have been
converted to use this function, we can change the implementation to move
away from this hack.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/hw/qdev-properties.h |  3 +++
 hw/core/qdev-properties.c    | 21 +++++++++++++++++++++
 2 files changed, 24 insertions(+)

diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
index e1df08876c..7fa2fdb7c9 100644
--- a/include/hw/qdev-properties.h
+++ b/include/hw/qdev-properties.h
@@ -206,6 +206,9 @@ void qdev_prop_set_macaddr(DeviceState *dev, const char *name,
                            const uint8_t *value);
 void qdev_prop_set_enum(DeviceState *dev, const char *name, int value);
 
+/* Takes ownership of @values */
+void qdev_prop_set_array(DeviceState *dev, const char *name, QList *values);
+
 void *object_field_prop_ptr(Object *obj, Property *prop);
 
 void qdev_prop_register_global(GlobalProperty *prop);
diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
index 357b8761b5..fb4daba799 100644
--- a/hw/core/qdev-properties.c
+++ b/hw/core/qdev-properties.c
@@ -3,12 +3,14 @@
 #include "qapi/error.h"
 #include "qapi/qapi-types-misc.h"
 #include "qapi/qmp/qerror.h"
+#include "qapi/qmp/qlist.h"
 #include "qemu/ctype.h"
 #include "qemu/error-report.h"
 #include "qapi/visitor.h"
 #include "qemu/units.h"
 #include "qemu/cutils.h"
 #include "qdev-prop-internal.h"
+#include "qom/qom-qobject.h"
 
 void qdev_prop_set_after_realize(DeviceState *dev, const char *name,
                                   Error **errp)
@@ -739,6 +741,25 @@ void qdev_prop_set_enum(DeviceState *dev, const char *name, int value)
                             &error_abort);
 }
 
+void qdev_prop_set_array(DeviceState *dev, const char *name, QList *values)
+{
+    const QListEntry *entry;
+    g_autofree char *prop_len = g_strdup_printf("len-%s", name);
+    unsigned i = 0;
+
+    object_property_set_int(OBJECT(dev), prop_len, qlist_size(values),
+                            &error_abort);
+
+    QLIST_FOREACH_ENTRY(values, entry) {
+        g_autofree char *prop_idx = g_strdup_printf("%s[%u]", name, i);
+        object_property_set_qobject(OBJECT(dev), prop_idx, entry->value,
+                                    &error_abort);
+        i++;
+    }
+
+    qobject_unref(values);
+}
+
 static GPtrArray *global_props(void)
 {
     static GPtrArray *gp;
-- 
2.41.0

[PATCH v2 02/12] hw/i386/pc: Use qdev_prop_set_array()

[Kevin Wolf]

Instead of manually setting "foo-len" and "foo[i]" properties, build a
QList and use the new qdev_prop_set_array() helper to set the whole
array property with a single call.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/i386/pc.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 6031234a73..5be6a78491 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -43,6 +43,7 @@
 #include "sysemu/reset.h"
 #include "kvm/kvm_i386.h"
 #include "hw/xen/xen.h"
+#include "qapi/qmp/qlist.h"
 #include "qemu/error-report.h"
 #include "hw/acpi/cpu_hotplug.h"
 #include "acpi-build.h"
@@ -1435,10 +1436,11 @@ static void pc_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev,
         /* Declare the APIC range as the reserved MSI region */
         char *resv_prop_str = g_strdup_printf("0xfee00000:0xfeefffff:%d",
                                               VIRTIO_IOMMU_RESV_MEM_T_MSI);
+        QList *reserved_regions = qlist_new();
+
+        qlist_append_str(reserved_regions, resv_prop_str);
+        qdev_prop_set_array(dev, "reserved-regions", reserved_regions);
 
-        object_property_set_uint(OBJECT(dev), "len-reserved-regions", 1, errp);
-        object_property_set_str(OBJECT(dev), "reserved-regions[0]",
-                                resv_prop_str, errp);
         g_free(resv_prop_str);
     }
 
-- 
2.41.0

[PATCH v2 03/12] hw/arm/mps2-tz: Use qdev_prop_set_array()

[Kevin Wolf]

Instead of manually setting "foo-len" and "foo[i]" properties, build a
QList and use the new qdev_prop_set_array() helper to set the whole
array property with a single call.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm/mps2-tz.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/hw/arm/mps2-tz.c b/hw/arm/mps2-tz.c
index eae3639da2..668db5ed61 100644
--- a/hw/arm/mps2-tz.c
+++ b/hw/arm/mps2-tz.c
@@ -48,6 +48,7 @@
 #include "qemu/units.h"
 #include "qemu/cutils.h"
 #include "qapi/error.h"
+#include "qapi/qmp/qlist.h"
 #include "qemu/error-report.h"
 #include "hw/arm/boot.h"
 #include "hw/arm/armv7m.h"
@@ -461,6 +462,7 @@ static MemoryRegion *make_scc(MPS2TZMachineState *mms, void *opaque,
     MPS2SCC *scc = opaque;
     DeviceState *sccdev;
     MPS2TZMachineClass *mmc = MPS2TZ_MACHINE_GET_CLASS(mms);
+    QList *oscclk;
     uint32_t i;
 
     object_initialize_child(OBJECT(mms), "scc", scc, TYPE_MPS2_SCC);
@@ -469,11 +471,13 @@ static MemoryRegion *make_scc(MPS2TZMachineState *mms, void *opaque,
     qdev_prop_set_uint32(sccdev, "scc-cfg4", 0x2);
     qdev_prop_set_uint32(sccdev, "scc-aid", 0x00200008);
     qdev_prop_set_uint32(sccdev, "scc-id", mmc->scc_id);
-    qdev_prop_set_uint32(sccdev, "len-oscclk", mmc->len_oscclk);
+
+    oscclk = qlist_new();
     for (i = 0; i < mmc->len_oscclk; i++) {
-        g_autofree char *propname = g_strdup_printf("oscclk[%u]", i);
-        qdev_prop_set_uint32(sccdev, propname, mmc->oscclk[i]);
+        qlist_append_int(oscclk, mmc->oscclk[i]);
     }
+    qdev_prop_set_array(sccdev, "oscclk", oscclk);
+
     sysbus_realize(SYS_BUS_DEVICE(scc), &error_fatal);
     return sysbus_mmio_get_region(SYS_BUS_DEVICE(sccdev), 0);
 }
-- 
2.41.0

[PATCH v2 04/12] hw/arm/mps2: Use qdev_prop_set_array()

[Kevin Wolf]

Instead of manually setting "foo-len" and "foo[i]" properties, build a
QList and use the new qdev_prop_set_array() helper to set the whole
array property with a single call.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm/mps2.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/hw/arm/mps2.c b/hw/arm/mps2.c
index d92fd60684..292a180ad2 100644
--- a/hw/arm/mps2.c
+++ b/hw/arm/mps2.c
@@ -48,6 +48,7 @@
 #include "net/net.h"
 #include "hw/watchdog/cmsdk-apb-watchdog.h"
 #include "hw/qdev-clock.h"
+#include "qapi/qmp/qlist.h"
 #include "qom/object.h"
 
 typedef enum MPS2FPGAType {
@@ -138,6 +139,7 @@ static void mps2_common_init(MachineState *machine)
     MemoryRegion *system_memory = get_system_memory();
     MachineClass *mc = MACHINE_GET_CLASS(machine);
     DeviceState *armv7m, *sccdev;
+    QList *oscclk;
     int i;
 
     if (strcmp(machine->cpu_type, mc->default_cpu_type) != 0) {
@@ -402,10 +404,12 @@ static void mps2_common_init(MachineState *machine)
     qdev_prop_set_uint32(sccdev, "scc-aid", 0x00200008);
     qdev_prop_set_uint32(sccdev, "scc-id", mmc->scc_id);
     /* All these FPGA images have the same OSCCLK configuration */
-    qdev_prop_set_uint32(sccdev, "len-oscclk", 3);
-    qdev_prop_set_uint32(sccdev, "oscclk[0]", 50000000);
-    qdev_prop_set_uint32(sccdev, "oscclk[1]", 24576000);
-    qdev_prop_set_uint32(sccdev, "oscclk[2]", 25000000);
+    oscclk = qlist_new();
+    qlist_append_int(oscclk, 50000000);
+    qlist_append_int(oscclk, 24576000);
+    qlist_append_int(oscclk, 25000000);
+    qdev_prop_set_array(sccdev, "oscclk", oscclk);
+
     sysbus_realize(SYS_BUS_DEVICE(&mms->scc), &error_fatal);
     sysbus_mmio_map(SYS_BUS_DEVICE(sccdev), 0, 0x4002f000);
     object_initialize_child(OBJECT(mms), "fpgaio",
-- 
2.41.0

[PATCH v2 05/12] hw/arm/sbsa-ref: Use qdev_prop_set_array()

[Kevin Wolf]

Instead of manually setting "foo-len" and "foo[i]" properties, build a
QList and use the new qdev_prop_set_array() helper to set the whole
array property with a single call.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm/sbsa-ref.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/hw/arm/sbsa-ref.c b/hw/arm/sbsa-ref.c
index e8a82618f0..6cf913300d 100644
--- a/hw/arm/sbsa-ref.c
+++ b/hw/arm/sbsa-ref.c
@@ -48,6 +48,7 @@
 #include "hw/char/pl011.h"
 #include "hw/watchdog/sbsa_gwdt.h"
 #include "net/net.h"
+#include "qapi/qmp/qlist.h"
 #include "qom/object.h"
 
 #define RAMLIMIT_GB 8192
@@ -436,6 +437,7 @@ static void create_gic(SBSAMachineState *sms, MemoryRegion *mem)
     SysBusDevice *gicbusdev;
     const char *gictype;
     uint32_t redist0_capacity, redist0_count;
+    QList *redist_region_count;
     int i;
 
     gictype = gicv3_class_name();
@@ -454,8 +456,9 @@ static void create_gic(SBSAMachineState *sms, MemoryRegion *mem)
                 sbsa_ref_memmap[SBSA_GIC_REDIST].size / GICV3_REDIST_SIZE;
     redist0_count = MIN(smp_cpus, redist0_capacity);
 
-    qdev_prop_set_uint32(sms->gic, "len-redist-region-count", 1);
-    qdev_prop_set_uint32(sms->gic, "redist-region-count[0]", redist0_count);
+    redist_region_count = qlist_new();
+    qlist_append_int(redist_region_count, redist0_count);
+    qdev_prop_set_array(sms->gic, "redist-region-count", redist_region_count);
 
     object_property_set_link(OBJECT(sms->gic), "sysmem",
                              OBJECT(mem), &error_fatal);
-- 
2.41.0

[PATCH v2 06/12] hw/arm/vexpress: Use qdev_prop_set_array()

[Kevin Wolf]

Instead of manually setting "foo-len" and "foo[i]" properties, build a
QList and use the new qdev_prop_set_array() helper to set the whole
array property with a single call.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm/vexpress.c | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/hw/arm/vexpress.c b/hw/arm/vexpress.c
index 8ff37f52ca..eb720f6de7 100644
--- a/hw/arm/vexpress.c
+++ b/hw/arm/vexpress.c
@@ -43,6 +43,7 @@
 #include "hw/cpu/a15mpcore.h"
 #include "hw/i2c/arm_sbcon_i2c.h"
 #include "hw/sd/sd.h"
+#include "qapi/qmp/qlist.h"
 #include "qom/object.h"
 #include "audio/audio.h"
 
@@ -552,6 +553,7 @@ static void vexpress_common_init(MachineState *machine)
     ram_addr_t vram_size, sram_size;
     MemoryRegion *sysmem = get_system_memory();
     const hwaddr *map = daughterboard->motherboard_map;
+    QList *db_voltage, *db_clock;
     int i;
 
     daughterboard->init(vms, machine->ram_size, machine->cpu_type, pic);
@@ -592,20 +594,19 @@ static void vexpress_common_init(MachineState *machine)
     sysctl = qdev_new("realview_sysctl");
     qdev_prop_set_uint32(sysctl, "sys_id", sys_id);
     qdev_prop_set_uint32(sysctl, "proc_id", daughterboard->proc_id);
-    qdev_prop_set_uint32(sysctl, "len-db-voltage",
-                         daughterboard->num_voltage_sensors);
+
+    db_voltage = qlist_new();
     for (i = 0; i < daughterboard->num_voltage_sensors; i++) {
-        char *propname = g_strdup_printf("db-voltage[%d]", i);
-        qdev_prop_set_uint32(sysctl, propname, daughterboard->voltages[i]);
-        g_free(propname);
+        qlist_append_int(db_voltage, daughterboard->voltages[i]);
     }
-    qdev_prop_set_uint32(sysctl, "len-db-clock",
-                         daughterboard->num_clocks);
+    qdev_prop_set_array(sysctl, "db-voltage", db_voltage);
+
+    db_clock = qlist_new();
     for (i = 0; i < daughterboard->num_clocks; i++) {
-        char *propname = g_strdup_printf("db-clock[%d]", i);
-        qdev_prop_set_uint32(sysctl, propname, daughterboard->clocks[i]);
-        g_free(propname);
+        qlist_append_int(db_clock, daughterboard->clocks[i]);
     }
+    qdev_prop_set_array(sysctl, "db-clock", db_clock);
+
     sysbus_realize_and_unref(SYS_BUS_DEVICE(sysctl), &error_fatal);
     sysbus_mmio_map(SYS_BUS_DEVICE(sysctl), 0, map[VE_SYSREGS]);
 
-- 
2.41.0

[PATCH v2 07/12] hw/arm/virt: Use qdev_prop_set_array()

[Kevin Wolf]

Instead of manually setting "foo-len" and "foo[i]" properties, build a
QList and use the new qdev_prop_set_array() helper to set the whole
array property with a single call.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm/virt.c | 31 +++++++++++++++++--------------
 1 file changed, 17 insertions(+), 14 deletions(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 529f1c089c..421ffc0b9c 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -69,6 +69,7 @@
 #include "hw/firmware/smbios.h"
 #include "qapi/visitor.h"
 #include "qapi/qapi-visit-common.h"
+#include "qapi/qmp/qlist.h"
 #include "standard-headers/linux/input.h"
 #include "hw/arm/smmuv3.h"
 #include "hw/acpi/acpi.h"
@@ -750,14 +751,23 @@ static void create_gic(VirtMachineState *vms, MemoryRegion *mem)
     }
 
     if (vms->gic_version != VIRT_GIC_VERSION_2) {
+        QList *redist_region_count;
         uint32_t redist0_capacity = virt_redist_capacity(vms, VIRT_GIC_REDIST);
         uint32_t redist0_count = MIN(smp_cpus, redist0_capacity);
 
         nb_redist_regions = virt_gicv3_redist_region_count(vms);
 
-        qdev_prop_set_uint32(vms->gic, "len-redist-region-count",
-                             nb_redist_regions);
-        qdev_prop_set_uint32(vms->gic, "redist-region-count[0]", redist0_count);
+        redist_region_count = qlist_new();
+        qlist_append_int(redist_region_count, redist0_count);
+        if (nb_redist_regions == 2) {
+            uint32_t redist1_capacity =
+                virt_redist_capacity(vms, VIRT_HIGH_GIC_REDIST2);
+
+            qlist_append_int(redist_region_count,
+                MIN(smp_cpus - redist0_count, redist1_capacity));
+        }
+        qdev_prop_set_array(vms->gic, "redist-region-count",
+                            redist_region_count);
 
         if (!kvm_irqchip_in_kernel()) {
             if (vms->tcg_its) {
@@ -766,14 +776,6 @@ static void create_gic(VirtMachineState *vms, MemoryRegion *mem)
                 qdev_prop_set_bit(vms->gic, "has-lpi", true);
             }
         }
-
-        if (nb_redist_regions == 2) {
-            uint32_t redist1_capacity =
-                virt_redist_capacity(vms, VIRT_HIGH_GIC_REDIST2);
-
-            qdev_prop_set_uint32(vms->gic, "redist-region-count[1]",
-                MIN(smp_cpus - redist0_count, redist1_capacity));
-        }
     } else {
         if (!kvm_irqchip_in_kernel()) {
             qdev_prop_set_bit(vms->gic, "has-virtualization-extensions",
@@ -2746,6 +2748,7 @@ static void virt_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev,
         virtio_md_pci_pre_plug(VIRTIO_MD_PCI(dev), MACHINE(hotplug_dev), errp);
     } else if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_IOMMU_PCI)) {
         hwaddr db_start = 0, db_end = 0;
+        QList *reserved_regions;
         char *resv_prop_str;
 
         if (vms->iommu != VIRT_IOMMU_NONE) {
@@ -2772,9 +2775,9 @@ static void virt_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev,
                                         db_start, db_end,
                                         VIRTIO_IOMMU_RESV_MEM_T_MSI);
 
-        object_property_set_uint(OBJECT(dev), "len-reserved-regions", 1, errp);
-        object_property_set_str(OBJECT(dev), "reserved-regions[0]",
-                                resv_prop_str, errp);
+        reserved_regions = qlist_new();
+        qlist_append_str(reserved_regions, resv_prop_str);
+        qdev_prop_set_array(dev, "reserved-regions", reserved_regions);
         g_free(resv_prop_str);
     }
 }
-- 
2.41.0

[PATCH v2 08/12] hw/arm/xlnx-versal: Use qdev_prop_set_array()

[Kevin Wolf]

Instead of manually setting "foo-len" and "foo[i]" properties, build a
QList and use the new qdev_prop_set_array() helper to set the whole
array property with a single call.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm/xlnx-versal.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/hw/arm/xlnx-versal.c b/hw/arm/xlnx-versal.c
index fa556d8764..d0f6235d4f 100644
--- a/hw/arm/xlnx-versal.c
+++ b/hw/arm/xlnx-versal.c
@@ -12,6 +12,7 @@
 #include "qemu/osdep.h"
 #include "qemu/units.h"
 #include "qapi/error.h"
+#include "qapi/qmp/qlist.h"
 #include "qemu/module.h"
 #include "hw/sysbus.h"
 #include "net/net.h"
@@ -69,6 +70,7 @@ static void versal_create_apu_gic(Versal *s, qemu_irq *pic)
     };
     SysBusDevice *gicbusdev;
     DeviceState *gicdev;
+    QList *redist_region_count;
     int nr_apu_cpus = ARRAY_SIZE(s->fpd.apu.cpu);
     int i;
 
@@ -79,8 +81,11 @@ static void versal_create_apu_gic(Versal *s, qemu_irq *pic)
     qdev_prop_set_uint32(gicdev, "revision", 3);
     qdev_prop_set_uint32(gicdev, "num-cpu", nr_apu_cpus);
     qdev_prop_set_uint32(gicdev, "num-irq", XLNX_VERSAL_NR_IRQS + 32);
-    qdev_prop_set_uint32(gicdev, "len-redist-region-count", 1);
-    qdev_prop_set_uint32(gicdev, "redist-region-count[0]", nr_apu_cpus);
+
+    redist_region_count = qlist_new();
+    qlist_append_int(redist_region_count, nr_apu_cpus);
+    qdev_prop_set_array(gicdev, "redist-region-count", redist_region_count);
+
     qdev_prop_set_bit(gicdev, "has-security-extensions", true);
 
     sysbus_realize(SYS_BUS_DEVICE(&s->fpd.apu.gic), &error_fatal);
-- 
2.41.0

[PATCH v2 09/12] hw/rx/rx62n: Use qdev_prop_set_array()

[Kevin Wolf]

Instead of manually setting "foo-len" and "foo[i]" properties, build a
QList and use the new qdev_prop_set_array() helper to set the whole
array property with a single call.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/rx/rx62n.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/hw/rx/rx62n.c b/hw/rx/rx62n.c
index d00fcb0ef0..4dc44afd9d 100644
--- a/hw/rx/rx62n.c
+++ b/hw/rx/rx62n.c
@@ -28,6 +28,7 @@
 #include "hw/sysbus.h"
 #include "hw/qdev-properties.h"
 #include "sysemu/sysemu.h"
+#include "qapi/qmp/qlist.h"
 #include "qom/object.h"
 
 /*
@@ -130,22 +131,22 @@ static void register_icu(RX62NState *s)
 {
     int i;
     SysBusDevice *icu;
+    QList *ipr_map, *trigger_level;
 
     object_initialize_child(OBJECT(s), "icu", &s->icu, TYPE_RX_ICU);
     icu = SYS_BUS_DEVICE(&s->icu);
-    qdev_prop_set_uint32(DEVICE(icu), "len-ipr-map", NR_IRQS);
+
+    ipr_map = qlist_new();
     for (i = 0; i < NR_IRQS; i++) {
-        char propname[32];
-        snprintf(propname, sizeof(propname), "ipr-map[%d]", i);
-        qdev_prop_set_uint32(DEVICE(icu), propname, ipr_table[i]);
+        qlist_append_int(ipr_map, ipr_table[i]);
     }
-    qdev_prop_set_uint32(DEVICE(icu), "len-trigger-level",
-                         ARRAY_SIZE(levelirq));
+    qdev_prop_set_array(DEVICE(icu), "ipr-map", ipr_map);
+
+    trigger_level = qlist_new();
     for (i = 0; i < ARRAY_SIZE(levelirq); i++) {
-        char propname[32];
-        snprintf(propname, sizeof(propname), "trigger-level[%d]", i);
-        qdev_prop_set_uint32(DEVICE(icu), propname, levelirq[i]);
+        qlist_append_int(trigger_level, levelirq[i]);
     }
+    qdev_prop_set_array(DEVICE(icu), "trigger-level", trigger_level);
 
     for (i = 0; i < NR_IRQS; i++) {
         s->irq[i] = qdev_get_gpio_in(DEVICE(icu), i);
-- 
2.41.0

[PATCH v2 10/12] qom: Add object_property_set_default_list()

[Kevin Wolf]

This function provides a default for properties that are accessed using
the list visitor interface. The default is always an empty list.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/qom/object.h | 8 ++++++++
 qom/object.c         | 6 ++++++
 2 files changed, 14 insertions(+)

diff --git a/include/qom/object.h b/include/qom/object.h
index ef7258a5e1..afccd24ca7 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -1093,6 +1093,14 @@ void object_property_set_default_bool(ObjectProperty *prop, bool value);
  */
 void object_property_set_default_str(ObjectProperty *prop, const char *value);
 
+/**
+ * object_property_set_default_list:
+ * @prop: the property to set
+ *
+ * Set the property default value to be an empty list.
+ */
+void object_property_set_default_list(ObjectProperty *prop);
+
 /**
  * object_property_set_default_int:
  * @prop: the property to set
diff --git a/qom/object.c b/qom/object.c
index 8557fe8e4e..95c0dc8285 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -31,6 +31,7 @@
  * of the QOM core on QObject?  */
 #include "qom/qom-qobject.h"
 #include "qapi/qmp/qbool.h"
+#include "qapi/qmp/qlist.h"
 #include "qapi/qmp/qnum.h"
 #include "qapi/qmp/qstring.h"
 #include "qemu/error-report.h"
@@ -1588,6 +1589,11 @@ void object_property_set_default_str(ObjectProperty *prop, const char *value)
     object_property_set_default(prop, QOBJECT(qstring_from_str(value)));
 }
 
+void object_property_set_default_list(ObjectProperty *prop)
+{
+    object_property_set_default(prop, QOBJECT(qlist_new()));
+}
+
 void object_property_set_default_int(ObjectProperty *prop, int64_t value)
 {
     object_property_set_default(prop, QOBJECT(qnum_from_int(value)));
-- 
2.41.0

[PATCH v2 11/12] qdev: Make netdev properties work as list elements

[Kevin Wolf]

The 'name' parameter of QOM setters is primarily used to specify the name
of the currently parsed input element in the visitor interface. For
top-level qdev properties, this is always set and matches 'prop->name'.

However, for list elements it is NULL, because each element of a list
doesn't have a separate name. Passing a non-NULL value runs into
assertion failures in the visitor code.

Therefore, using 'name' in error messages is not right for property
types that are used in lists, because "(null)" isn't very helpful to
identify what QEMU is complaining about.

Change netdev properties to use 'prop->name' instead, which will contain
the name of the array property after switching array properties to lists
in the external interface.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 hw/core/qdev-properties-system.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c
index 7c6dfab128..bf243d72d6 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);
 }
 
-- 
2.41.0

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

[Kevin Wolf]

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

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

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

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

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

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

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

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1090
Fixes: f3558b1b763683bb877f7dd5b282469cdadc65c3
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 include/hw/qdev-properties.h |  59 +++++----
 hw/core/qdev-properties.c    | 224 ++++++++++++++++++++++-------------
 2 files changed, 183 insertions(+), 100 deletions(-)

diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
index 7fa2fdb7c9..cac752bade 100644
--- a/include/hw/qdev-properties.h
+++ b/include/hw/qdev-properties.h
@@ -1,7 +1,10 @@
 #ifndef QEMU_QDEV_PROPERTIES_H
 #define QEMU_QDEV_PROPERTIES_H
 
+#include <stdalign.h>
+
 #include "hw/qdev-core.h"
+#include "qapi/visitor.h"
 
 /**
  * Property:
@@ -61,7 +64,7 @@ extern const PropertyInfo qdev_prop_size;
 extern const PropertyInfo qdev_prop_string;
 extern const PropertyInfo qdev_prop_on_off_auto;
 extern const PropertyInfo qdev_prop_size32;
-extern const PropertyInfo qdev_prop_arraylen;
+extern const PropertyInfo qdev_prop_array;
 extern const PropertyInfo qdev_prop_link;
 
 #define DEFINE_PROP(_name, _state, _field, _prop, _type, ...) {  \
@@ -115,8 +118,6 @@ extern const PropertyInfo qdev_prop_link;
                 .bitmask    = (_bitmask),                     \
                 .set_default = false)
 
-#define PROP_ARRAY_LEN_PREFIX "len-"
-
 /**
  * DEFINE_PROP_ARRAY:
  * @_name: name of the array
@@ -127,28 +128,46 @@ extern const PropertyInfo qdev_prop_link;
  * @_arrayprop: PropertyInfo defining what property the array elements have
  * @_arraytype: C type of the array elements
  *
- * Define device properties for a variable-length array _name.  A
- * static property "len-arrayname" is defined. When the device creator
- * sets this property to the desired length of array, further dynamic
- * properties "arrayname[0]", "arrayname[1]", ...  are defined so the
- * device creator can set the array element values. Setting the
- * "len-arrayname" property more than once is an error.
+ * Define device properties for a variable-length array _name.  The array is
+ * represented as a list in the visitor interface.
  *
- * When the array length is set, the @_field member of the device
+ * @_arraytype is required to be movable with memcpy() and to have an alignment
+ * such that it can be stored at GenericList.padding.
+ *
+ * When the array property is set, the @_field member of the device
  * struct is set to the array length, and @_arrayfield is set to point
- * to (zero-initialised) memory allocated for the array.  For a zero
- * length array, @_field will be set to 0 and @_arrayfield to NULL.
+ * to the memory allocated for the array.
+ *
  * It is the responsibility of the device deinit code to free the
  * @_arrayfield memory.
  */
-#define DEFINE_PROP_ARRAY(_name, _state, _field,               \
-                          _arrayfield, _arrayprop, _arraytype) \
-    DEFINE_PROP((PROP_ARRAY_LEN_PREFIX _name),                 \
-                _state, _field, qdev_prop_arraylen, uint32_t,  \
-                .set_default = true,                           \
-                .defval.u = 0,                                 \
-                .arrayinfo = &(_arrayprop),                    \
-                .arrayfieldsize = sizeof(_arraytype),          \
+#define DEFINE_PROP_ARRAY(_name, _state, _field,                        \
+                          _arrayfield, _arrayprop, _arraytype)          \
+    DEFINE_PROP(_name, _state, _field, qdev_prop_array, uint32_t,       \
+                .set_default = true,                                    \
+                .defval.u = 0,                                          \
+                .arrayinfo = &(_arrayprop),                             \
+                /*                                                      \
+                 * set_prop_array() temporarily stores elements at      \
+                 * GenericList.padding. Make sure that this has the     \
+                 * right alignment for @_arraytype.                     \
+                 *                                                      \
+                 * Hack: In this place, neither static assertions work  \
+                 * nor is a statement expression allowed. This          \
+                 * abomination of an expression works because inside    \
+                 * the declaration of a dummy struct, static assertions \
+                 * are possible. Using the comma operator causes        \
+                 * warnings about an unused value and casting to void   \
+                 * makes the expression not constant in gcc, so instead \
+                 * of ignoring the first part, make it evaluate to 0    \
+                 * and add it to the actual result.                     \
+                 */                                                     \
+                .arrayfieldsize = (!sizeof(struct {                     \
+                    QEMU_BUILD_BUG_ON(                                  \
+                        !QEMU_IS_ALIGNED(sizeof(GenericList),           \
+                                         alignof(_arraytype)));         \
+                    int dummy;                                          \
+                }) + sizeof(_arraytype)),                               \
                 .arrayoffset = offsetof(_state, _arrayfield))
 
 #define DEFINE_PROP_LINK(_name, _state, _field, _type, _ptr_type)     \
diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
index fb4daba799..4f3e1152be 100644
--- a/hw/core/qdev-properties.c
+++ b/hw/core/qdev-properties.c
@@ -546,98 +546,174 @@ const PropertyInfo qdev_prop_size32 = {
 
 /* --- support for array properties --- */
 
-/* Used as an opaque for the object properties we add for each
- * array element. Note that the struct Property must be first
- * in the struct so that a pointer to this works as the opaque
- * for the underlying element's property hooks as well as for
- * our own release callback.
+/*
+ * Given an array property @parent_prop in @obj, return a Property for a
+ * specific element of the array. Arrays are backed by an uint32_t length field
+ * and an element array. @elem points at an element in this element array.
  */
-typedef struct {
-    struct Property prop;
-    char *propname;
-    ObjectPropertyRelease *release;
-} ArrayElementProperty;
-
-/* object property release callback for array element properties:
- * we call the underlying element's property release hook, and
- * then free the memory we allocated when we added the property.
+static Property array_elem_prop(Object *obj, Property *parent_prop,
+                                const char *name, char *elem)
+{
+    return (Property) {
+        .info = parent_prop->arrayinfo,
+        .name = name,
+        /*
+         * This ugly piece of pointer arithmetic sets up the offset so
+         * that when the underlying release hook calls qdev_get_prop_ptr
+         * they get the right answer despite the array element not actually
+         * being inside the device struct.
+         */
+        .offset = (uintptr_t)elem - (uintptr_t)obj,
+    };
+}
+
+/*
+ * Object property release callback for array properties: We call the
+ * underlying element's property release hook for each element.
+ *
+ * Note that it is the responsibility of the individual device's deinit
+ * to free the array proper.
  */
-static void array_element_release(Object *obj, const char *name, void *opaque)
+static void release_prop_array(Object *obj, const char *name, void *opaque)
 {
-    ArrayElementProperty *p = opaque;
-    if (p->release) {
-        p->release(obj, name, opaque);
+    Property *prop = opaque;
+    uint32_t *alenptr = object_field_prop_ptr(obj, prop);
+    void **arrayptr = (void *)obj + prop->arrayoffset;
+    char *elem = *arrayptr;
+    int i;
+
+    if (!prop->arrayinfo->release) {
+        return;
+    }
+
+    for (i = 0; i < *alenptr; i++) {
+        Property elem_prop = array_elem_prop(obj, prop, name, elem);
+        prop->arrayinfo->release(obj, NULL, &elem_prop);
+        elem += prop->arrayfieldsize;
     }
-    g_free(p->propname);
-    g_free(p);
 }
 
-static void set_prop_arraylen(Object *obj, Visitor *v, const char *name,
-                              void *opaque, Error **errp)
+/*
+ * Setter for an array property. This sets both the array length (which
+ * is technically the property field in the object) and the array itself
+ * (a pointer to which is stored in the additional field described by
+ * prop->arrayoffset).
+ */
+static void set_prop_array(Object *obj, Visitor *v, const char *name,
+                           void *opaque, Error **errp)
 {
-    /* Setter for the property which defines the length of a
-     * variable-sized property array. As well as actually setting the
-     * array-length field in the device struct, we have to create the
-     * array itself and dynamically add the corresponding properties.
-     */
+    ERRP_GUARD();
     Property *prop = opaque;
     uint32_t *alenptr = object_field_prop_ptr(obj, prop);
     void **arrayptr = (void *)obj + prop->arrayoffset;
-    void *eltptr;
-    const char *arrayname;
-    int i;
+    GenericList *list, *elem, *next;
+    const size_t list_elem_size = sizeof(*list) + prop->arrayfieldsize;
+    char *elemptr;
+    bool ok = true;
 
     if (*alenptr) {
         error_setg(errp, "array size property %s may not be set more than once",
                    name);
         return;
     }
-    if (!visit_type_uint32(v, name, alenptr, errp)) {
+
+    if (!visit_start_list(v, name, &list, list_elem_size, errp)) {
         return;
     }
-    if (!*alenptr) {
+
+    /* Read the whole input into a temporary list */
+    elem = list;
+    while (elem) {
+        Property elem_prop = array_elem_prop(obj, prop, name, elem->padding);
+        prop->arrayinfo->set(obj, v, NULL, &elem_prop, errp);
+        if (*errp) {
+            ok = false;
+            goto out_obj;
+        }
+        if (*alenptr == INT_MAX) {
+            error_setg(errp, "array is too big");
+            return;
+        }
+        (*alenptr)++;
+        elem = visit_next_list(v, elem, list_elem_size);
+    }
+
+    ok = visit_check_list(v, errp);
+out_obj:
+    visit_end_list(v, (void**) &list);
+
+    if (!ok) {
+        for (elem = list; elem; elem = next) {
+            Property elem_prop = array_elem_prop(obj, prop, name,
+                                                 elem->padding);
+            if (prop->arrayinfo->release) {
+                prop->arrayinfo->release(obj, NULL, &elem_prop);
+            }
+            next = elem->next;
+            g_free(elem);
+        }
         return;
     }
 
-    /* DEFINE_PROP_ARRAY guarantees that name should start with this prefix;
-     * strip it off so we can get the name of the array itself.
+    /*
+     * Now that we know how big the array has to be, move the data over to a
+     * linear array and free the temporary list.
      */
-    assert(strncmp(name, PROP_ARRAY_LEN_PREFIX,
-                   strlen(PROP_ARRAY_LEN_PREFIX)) == 0);
-    arrayname = name + strlen(PROP_ARRAY_LEN_PREFIX);
+    *arrayptr = g_malloc_n(*alenptr, prop->arrayfieldsize);
+    elemptr = *arrayptr;
+    for (elem = list; elem; elem = next) {
+        memcpy(elemptr, elem->padding, prop->arrayfieldsize);
+        elemptr += prop->arrayfieldsize;
+        next = elem->next;
+        g_free(elem);
+    }
+}
 
-    /* Note that it is the responsibility of the individual device's deinit
-     * to free the array proper.
-     */
-    *arrayptr = eltptr = g_malloc0(*alenptr * prop->arrayfieldsize);
-    for (i = 0; i < *alenptr; i++, eltptr += prop->arrayfieldsize) {
-        char *propname = g_strdup_printf("%s[%d]", arrayname, i);
-        ArrayElementProperty *arrayprop = g_new0(ArrayElementProperty, 1);
-        arrayprop->release = prop->arrayinfo->release;
-        arrayprop->propname = propname;
-        arrayprop->prop.info = prop->arrayinfo;
-        arrayprop->prop.name = propname;
-        /* This ugly piece of pointer arithmetic sets up the offset so
-         * that when the underlying get/set hooks call qdev_get_prop_ptr
-         * they get the right answer despite the array element not actually
-         * being inside the device struct.
-         */
-        arrayprop->prop.offset = eltptr - (void *)obj;
-        assert(object_field_prop_ptr(obj, &arrayprop->prop) == eltptr);
-        object_property_add(obj, propname,
-                            arrayprop->prop.info->name,
-                            field_prop_getter(arrayprop->prop.info),
-                            field_prop_setter(arrayprop->prop.info),
-                            array_element_release,
-                            arrayprop);
+static void get_prop_array(Object *obj, Visitor *v, const char *name,
+                           void *opaque, Error **errp)
+{
+    ERRP_GUARD();
+    Property *prop = opaque;
+    uint32_t *alenptr = object_field_prop_ptr(obj, prop);
+    void **arrayptr = (void *)obj + prop->arrayoffset;
+    char *elem = *arrayptr;
+    GenericList *list;
+    const size_t list_elem_size = sizeof(*list) + prop->arrayfieldsize;
+    int i;
+    bool ok;
+
+    if (!visit_start_list(v, name, &list, list_elem_size, errp)) {
+        return;
     }
+
+    for (i = 0; i < *alenptr; i++) {
+        Property elem_prop = array_elem_prop(obj, prop, name, elem);
+        prop->arrayinfo->get(obj, v, NULL, &elem_prop, errp);
+        if (*errp) {
+            goto out_obj;
+        }
+        elem += prop->arrayfieldsize;
+    }
+
+    /* visit_check_list() can only fail for input visitors */
+    ok = visit_check_list(v, errp);
+    assert(ok);
+
+out_obj:
+    visit_end_list(v, (void**) &list);
 }
 
-const PropertyInfo qdev_prop_arraylen = {
-    .name = "uint32",
-    .get = get_uint32,
-    .set = set_prop_arraylen,
-    .set_default_value = qdev_propinfo_set_default_value_uint,
+static void default_prop_array(ObjectProperty *op, const Property *prop)
+{
+    object_property_set_default_list(op);
+}
+
+const PropertyInfo qdev_prop_array = {
+    .name = "list",
+    .get = get_prop_array,
+    .set = set_prop_array,
+    .release = release_prop_array,
+    .set_default_value = default_prop_array,
 };
 
 /* --- public helpers --- */
@@ -743,20 +819,8 @@ void qdev_prop_set_enum(DeviceState *dev, const char *name, int value)
 
 void qdev_prop_set_array(DeviceState *dev, const char *name, QList *values)
 {
-    const QListEntry *entry;
-    g_autofree char *prop_len = g_strdup_printf("len-%s", name);
-    unsigned i = 0;
-
-    object_property_set_int(OBJECT(dev), prop_len, qlist_size(values),
-                            &error_abort);
-
-    QLIST_FOREACH_ENTRY(values, entry) {
-        g_autofree char *prop_idx = g_strdup_printf("%s[%u]", name, i);
-        object_property_set_qobject(OBJECT(dev), prop_idx, entry->value,
-                                    &error_abort);
-        i++;
-    }
-
+    object_property_set_qobject(OBJECT(dev), name, QOBJECT(values),
+                                &error_abort);
     qobject_unref(values);
 }
 
-- 
2.41.0

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

[Mark Cave-Ayland]

On 30/10/2023 14:26, Kevin Wolf 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, 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.

Is it possible to find a suitable place in the documentation to show how the new 
array syntax is used with -device on the command line? The description above is 
really useful, but I can see this being quite hard for users to find if it is only 
documented in the commit message.

> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1090
> Fixes: f3558b1b763683bb877f7dd5b282469cdadc65c3
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>   include/hw/qdev-properties.h |  59 +++++----
>   hw/core/qdev-properties.c    | 224 ++++++++++++++++++++++-------------
>   2 files changed, 183 insertions(+), 100 deletions(-)
> 
> diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
> index 7fa2fdb7c9..cac752bade 100644
> --- a/include/hw/qdev-properties.h
> +++ b/include/hw/qdev-properties.h
> @@ -1,7 +1,10 @@
>   #ifndef QEMU_QDEV_PROPERTIES_H
>   #define QEMU_QDEV_PROPERTIES_H
>   
> +#include <stdalign.h>
> +
>   #include "hw/qdev-core.h"
> +#include "qapi/visitor.h"
>   
>   /**
>    * Property:
> @@ -61,7 +64,7 @@ extern const PropertyInfo qdev_prop_size;
>   extern const PropertyInfo qdev_prop_string;
>   extern const PropertyInfo qdev_prop_on_off_auto;
>   extern const PropertyInfo qdev_prop_size32;
> -extern const PropertyInfo qdev_prop_arraylen;
> +extern const PropertyInfo qdev_prop_array;
>   extern const PropertyInfo qdev_prop_link;
>   
>   #define DEFINE_PROP(_name, _state, _field, _prop, _type, ...) {  \
> @@ -115,8 +118,6 @@ extern const PropertyInfo qdev_prop_link;
>                   .bitmask    = (_bitmask),                     \
>                   .set_default = false)
>   
> -#define PROP_ARRAY_LEN_PREFIX "len-"
> -
>   /**
>    * DEFINE_PROP_ARRAY:
>    * @_name: name of the array
> @@ -127,28 +128,46 @@ extern const PropertyInfo qdev_prop_link;
>    * @_arrayprop: PropertyInfo defining what property the array elements have
>    * @_arraytype: C type of the array elements
>    *
> - * Define device properties for a variable-length array _name.  A
> - * static property "len-arrayname" is defined. When the device creator
> - * sets this property to the desired length of array, further dynamic
> - * properties "arrayname[0]", "arrayname[1]", ...  are defined so the
> - * device creator can set the array element values. Setting the
> - * "len-arrayname" property more than once is an error.
> + * Define device properties for a variable-length array _name.  The array is
> + * represented as a list in the visitor interface.
>    *
> - * When the array length is set, the @_field member of the device
> + * @_arraytype is required to be movable with memcpy() and to have an alignment
> + * such that it can be stored at GenericList.padding.
> + *
> + * When the array property is set, the @_field member of the device
>    * struct is set to the array length, and @_arrayfield is set to point
> - * to (zero-initialised) memory allocated for the array.  For a zero
> - * length array, @_field will be set to 0 and @_arrayfield to NULL.
> + * to the memory allocated for the array.
> + *
>    * It is the responsibility of the device deinit code to free the
>    * @_arrayfield memory.
>    */
> -#define DEFINE_PROP_ARRAY(_name, _state, _field,               \
> -                          _arrayfield, _arrayprop, _arraytype) \
> -    DEFINE_PROP((PROP_ARRAY_LEN_PREFIX _name),                 \
> -                _state, _field, qdev_prop_arraylen, uint32_t,  \
> -                .set_default = true,                           \
> -                .defval.u = 0,                                 \
> -                .arrayinfo = &(_arrayprop),                    \
> -                .arrayfieldsize = sizeof(_arraytype),          \
> +#define DEFINE_PROP_ARRAY(_name, _state, _field,                        \
> +                          _arrayfield, _arrayprop, _arraytype)          \
> +    DEFINE_PROP(_name, _state, _field, qdev_prop_array, uint32_t,       \
> +                .set_default = true,                                    \
> +                .defval.u = 0,                                          \
> +                .arrayinfo = &(_arrayprop),                             \
> +                /*                                                      \
> +                 * set_prop_array() temporarily stores elements at      \
> +                 * GenericList.padding. Make sure that this has the     \
> +                 * right alignment for @_arraytype.                     \
> +                 *                                                      \
> +                 * Hack: In this place, neither static assertions work  \
> +                 * nor is a statement expression allowed. This          \
> +                 * abomination of an expression works because inside    \
> +                 * the declaration of a dummy struct, static assertions \
> +                 * are possible. Using the comma operator causes        \
> +                 * warnings about an unused value and casting to void   \
> +                 * makes the expression not constant in gcc, so instead \
> +                 * of ignoring the first part, make it evaluate to 0    \
> +                 * and add it to the actual result.                     \
> +                 */                                                     \
> +                .arrayfieldsize = (!sizeof(struct {                     \
> +                    QEMU_BUILD_BUG_ON(                                  \
> +                        !QEMU_IS_ALIGNED(sizeof(GenericList),           \
> +                                         alignof(_arraytype)));         \
> +                    int dummy;                                          \
> +                }) + sizeof(_arraytype)),                               \
>                   .arrayoffset = offsetof(_state, _arrayfield))
>   
>   #define DEFINE_PROP_LINK(_name, _state, _field, _type, _ptr_type)     \
> diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
> index fb4daba799..4f3e1152be 100644
> --- a/hw/core/qdev-properties.c
> +++ b/hw/core/qdev-properties.c
> @@ -546,98 +546,174 @@ const PropertyInfo qdev_prop_size32 = {
>   
>   /* --- support for array properties --- */
>   
> -/* Used as an opaque for the object properties we add for each
> - * array element. Note that the struct Property must be first
> - * in the struct so that a pointer to this works as the opaque
> - * for the underlying element's property hooks as well as for
> - * our own release callback.
> +/*
> + * Given an array property @parent_prop in @obj, return a Property for a
> + * specific element of the array. Arrays are backed by an uint32_t length field
> + * and an element array. @elem points at an element in this element array.
>    */
> -typedef struct {
> -    struct Property prop;
> -    char *propname;
> -    ObjectPropertyRelease *release;
> -} ArrayElementProperty;
> -
> -/* object property release callback for array element properties:
> - * we call the underlying element's property release hook, and
> - * then free the memory we allocated when we added the property.
> +static Property array_elem_prop(Object *obj, Property *parent_prop,
> +                                const char *name, char *elem)
> +{
> +    return (Property) {
> +        .info = parent_prop->arrayinfo,
> +        .name = name,
> +        /*
> +         * This ugly piece of pointer arithmetic sets up the offset so
> +         * that when the underlying release hook calls qdev_get_prop_ptr
> +         * they get the right answer despite the array element not actually
> +         * being inside the device struct.
> +         */
> +        .offset = (uintptr_t)elem - (uintptr_t)obj,
> +    };
> +}
> +
> +/*
> + * Object property release callback for array properties: We call the
> + * underlying element's property release hook for each element.
> + *
> + * Note that it is the responsibility of the individual device's deinit
> + * to free the array proper.
>    */
> -static void array_element_release(Object *obj, const char *name, void *opaque)
> +static void release_prop_array(Object *obj, const char *name, void *opaque)
>   {
> -    ArrayElementProperty *p = opaque;
> -    if (p->release) {
> -        p->release(obj, name, opaque);
> +    Property *prop = opaque;
> +    uint32_t *alenptr = object_field_prop_ptr(obj, prop);
> +    void **arrayptr = (void *)obj + prop->arrayoffset;
> +    char *elem = *arrayptr;
> +    int i;
> +
> +    if (!prop->arrayinfo->release) {
> +        return;
> +    }
> +
> +    for (i = 0; i < *alenptr; i++) {
> +        Property elem_prop = array_elem_prop(obj, prop, name, elem);
> +        prop->arrayinfo->release(obj, NULL, &elem_prop);
> +        elem += prop->arrayfieldsize;
>       }
> -    g_free(p->propname);
> -    g_free(p);
>   }
>   
> -static void set_prop_arraylen(Object *obj, Visitor *v, const char *name,
> -                              void *opaque, Error **errp)
> +/*
> + * Setter for an array property. This sets both the array length (which
> + * is technically the property field in the object) and the array itself
> + * (a pointer to which is stored in the additional field described by
> + * prop->arrayoffset).
> + */
> +static void set_prop_array(Object *obj, Visitor *v, const char *name,
> +                           void *opaque, Error **errp)
>   {
> -    /* Setter for the property which defines the length of a
> -     * variable-sized property array. As well as actually setting the
> -     * array-length field in the device struct, we have to create the
> -     * array itself and dynamically add the corresponding properties.
> -     */
> +    ERRP_GUARD();
>       Property *prop = opaque;
>       uint32_t *alenptr = object_field_prop_ptr(obj, prop);
>       void **arrayptr = (void *)obj + prop->arrayoffset;
> -    void *eltptr;
> -    const char *arrayname;
> -    int i;
> +    GenericList *list, *elem, *next;
> +    const size_t list_elem_size = sizeof(*list) + prop->arrayfieldsize;
> +    char *elemptr;
> +    bool ok = true;
>   
>       if (*alenptr) {
>           error_setg(errp, "array size property %s may not be set more than once",
>                      name);
>           return;
>       }
> -    if (!visit_type_uint32(v, name, alenptr, errp)) {
> +
> +    if (!visit_start_list(v, name, &list, list_elem_size, errp)) {
>           return;
>       }
> -    if (!*alenptr) {
> +
> +    /* Read the whole input into a temporary list */
> +    elem = list;
> +    while (elem) {
> +        Property elem_prop = array_elem_prop(obj, prop, name, elem->padding);
> +        prop->arrayinfo->set(obj, v, NULL, &elem_prop, errp);
> +        if (*errp) {
> +            ok = false;
> +            goto out_obj;
> +        }
> +        if (*alenptr == INT_MAX) {
> +            error_setg(errp, "array is too big");
> +            return;
> +        }
> +        (*alenptr)++;
> +        elem = visit_next_list(v, elem, list_elem_size);
> +    }
> +
> +    ok = visit_check_list(v, errp);
> +out_obj:
> +    visit_end_list(v, (void**) &list);
> +
> +    if (!ok) {
> +        for (elem = list; elem; elem = next) {
> +            Property elem_prop = array_elem_prop(obj, prop, name,
> +                                                 elem->padding);
> +            if (prop->arrayinfo->release) {
> +                prop->arrayinfo->release(obj, NULL, &elem_prop);
> +            }
> +            next = elem->next;
> +            g_free(elem);
> +        }
>           return;
>       }
>   
> -    /* DEFINE_PROP_ARRAY guarantees that name should start with this prefix;
> -     * strip it off so we can get the name of the array itself.
> +    /*
> +     * Now that we know how big the array has to be, move the data over to a
> +     * linear array and free the temporary list.
>        */
> -    assert(strncmp(name, PROP_ARRAY_LEN_PREFIX,
> -                   strlen(PROP_ARRAY_LEN_PREFIX)) == 0);
> -    arrayname = name + strlen(PROP_ARRAY_LEN_PREFIX);
> +    *arrayptr = g_malloc_n(*alenptr, prop->arrayfieldsize);
> +    elemptr = *arrayptr;
> +    for (elem = list; elem; elem = next) {
> +        memcpy(elemptr, elem->padding, prop->arrayfieldsize);
> +        elemptr += prop->arrayfieldsize;
> +        next = elem->next;
> +        g_free(elem);
> +    }
> +}
>   
> -    /* Note that it is the responsibility of the individual device's deinit
> -     * to free the array proper.
> -     */
> -    *arrayptr = eltptr = g_malloc0(*alenptr * prop->arrayfieldsize);
> -    for (i = 0; i < *alenptr; i++, eltptr += prop->arrayfieldsize) {
> -        char *propname = g_strdup_printf("%s[%d]", arrayname, i);
> -        ArrayElementProperty *arrayprop = g_new0(ArrayElementProperty, 1);
> -        arrayprop->release = prop->arrayinfo->release;
> -        arrayprop->propname = propname;
> -        arrayprop->prop.info = prop->arrayinfo;
> -        arrayprop->prop.name = propname;
> -        /* This ugly piece of pointer arithmetic sets up the offset so
> -         * that when the underlying get/set hooks call qdev_get_prop_ptr
> -         * they get the right answer despite the array element not actually
> -         * being inside the device struct.
> -         */
> -        arrayprop->prop.offset = eltptr - (void *)obj;
> -        assert(object_field_prop_ptr(obj, &arrayprop->prop) == eltptr);
> -        object_property_add(obj, propname,
> -                            arrayprop->prop.info->name,
> -                            field_prop_getter(arrayprop->prop.info),
> -                            field_prop_setter(arrayprop->prop.info),
> -                            array_element_release,
> -                            arrayprop);
> +static void get_prop_array(Object *obj, Visitor *v, const char *name,
> +                           void *opaque, Error **errp)
> +{
> +    ERRP_GUARD();
> +    Property *prop = opaque;
> +    uint32_t *alenptr = object_field_prop_ptr(obj, prop);
> +    void **arrayptr = (void *)obj + prop->arrayoffset;
> +    char *elem = *arrayptr;
> +    GenericList *list;
> +    const size_t list_elem_size = sizeof(*list) + prop->arrayfieldsize;
> +    int i;
> +    bool ok;
> +
> +    if (!visit_start_list(v, name, &list, list_elem_size, errp)) {
> +        return;
>       }
> +
> +    for (i = 0; i < *alenptr; i++) {
> +        Property elem_prop = array_elem_prop(obj, prop, name, elem);
> +        prop->arrayinfo->get(obj, v, NULL, &elem_prop, errp);
> +        if (*errp) {
> +            goto out_obj;
> +        }
> +        elem += prop->arrayfieldsize;
> +    }
> +
> +    /* visit_check_list() can only fail for input visitors */
> +    ok = visit_check_list(v, errp);
> +    assert(ok);
> +
> +out_obj:
> +    visit_end_list(v, (void**) &list);
>   }
>   
> -const PropertyInfo qdev_prop_arraylen = {
> -    .name = "uint32",
> -    .get = get_uint32,
> -    .set = set_prop_arraylen,
> -    .set_default_value = qdev_propinfo_set_default_value_uint,
> +static void default_prop_array(ObjectProperty *op, const Property *prop)
> +{
> +    object_property_set_default_list(op);
> +}
> +
> +const PropertyInfo qdev_prop_array = {
> +    .name = "list",
> +    .get = get_prop_array,
> +    .set = set_prop_array,
> +    .release = release_prop_array,
> +    .set_default_value = default_prop_array,
>   };
>   
>   /* --- public helpers --- */
> @@ -743,20 +819,8 @@ void qdev_prop_set_enum(DeviceState *dev, const char *name, int value)
>   
>   void qdev_prop_set_array(DeviceState *dev, const char *name, QList *values)
>   {
> -    const QListEntry *entry;
> -    g_autofree char *prop_len = g_strdup_printf("len-%s", name);
> -    unsigned i = 0;
> -
> -    object_property_set_int(OBJECT(dev), prop_len, qlist_size(values),
> -                            &error_abort);
> -
> -    QLIST_FOREACH_ENTRY(values, entry) {
> -        g_autofree char *prop_idx = g_strdup_printf("%s[%u]", name, i);
> -        object_property_set_qobject(OBJECT(dev), prop_idx, entry->value,
> -                                    &error_abort);
> -        i++;
> -    }
> -
> +    object_property_set_qobject(OBJECT(dev), name, QOBJECT(values),
> +                                &error_abort);
>       qobject_unref(values);
>   }


ATB,

Mark.

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

[Kevin Wolf]

Am 30.10.2023 um 21:48 hat Mark Cave-Ayland geschrieben:
> On 30/10/2023 14:26, Kevin Wolf 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, 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.
> 
> Is it possible to find a suitable place in the documentation to show
> how the new array syntax is used with -device on the command line? The
> description above is really useful, but I can see this being quite
> hard for users to find if it is only documented in the commit message.

Actually, it seems that the documentation doesn't explicitly mention
JSON syntax for command line options anywhere. Support for it is quite
widespread meanwhile. I can see it for at least:

-audiodev
-blockdev
-compat
-device
-display
-netdev
-object

In qemu-storage-daemon, it's additionally supported for:

--export
--monitor
--nbd-server

I think the manpage should at least mention for each of these options
that they support JSON.

Ideally, we'd then have a generic section that describes the mapping
between JSON and human syntax, but unfortunately, the human oriented
parsers differ quite a lot between these options, so there is nothing we
can describe and that is valid for all options. So maybe things like
"Specifying values for list properties is only possible with JSON
syntax" must be specified for each option where it applies.

Kevin

Re: [PATCH v2 11/12] qdev: Make netdev properties work as list elements

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> The 'name' parameter of QOM setters is primarily used to specify the name
> of the currently parsed input element in the visitor interface. For
> top-level qdev properties, this is always set and matches 'prop->name'.
>
> However, for list elements it is NULL, because each element of a list
> doesn't have a separate name. Passing a non-NULL value runs into
> assertion failures in the visitor code.

Yes.  visitor.h's big comment:

 * The @name parameter of visit_type_FOO() describes the relation
 * between this QAPI value and its parent container.  When visiting
 * the root of a tree, @name is ignored; when visiting a member of an
 * object, @name is the key associated with the value; when visiting a
 * member of a list, @name is NULL; and when visiting the member of an
 * alternate, @name should equal the name used for visiting the
 * alternate.

> Therefore, using 'name' in error messages is not right for property
> types that are used in lists, because "(null)" isn't very helpful to
> identify what QEMU is complaining about.

We get "(null)" on some hosts, and SEGV on others.

Same problem in all property setters and getters (qdev and QOM), I
presume.  The @name parameter looks like a death trap.  Thoughts?

Any reproducer known before the next patch?

> Change netdev properties to use 'prop->name' instead, which will contain
> the name of the array property after switching array properties to lists
> in the external interface.

Points at the entire array property without telling the user which of
the elements is at fault.  Sure better than NULL.  I'm not asking you to
improve the error message further.  Mention the error message's lack of
precision in the commit message?

> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  hw/core/qdev-properties-system.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c
> index 7c6dfab128..bf243d72d6 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);
>  }

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

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Until now, array properties are actually implemented with a hack that
> uses multiple properties on the QOM level: a static "foo-len" property
> and after it is set, dynamically created "foo[i]" properties.
>
> In external interfaces (-device on the command line and device_add in
> QMP), this interface was broken by commit f3558b1b ('qdev: Base object
> creation on QDict rather than QemuOpts') because QDicts are unordered
> and therefore it could happen that QEMU tried to set the indexed
> properties before setting the length, which fails and effectively makes
> array properties inaccessible. In particular, this affects the 'ports'
> property of the 'rocker' device, which used to be configured like this:
>
> -device rocker,len-ports=2,ports[0]=dev0,ports[1]=dev1
>
> This patch reworks the external interface so that instead of using a
> separate top-level property for the length and for each element, we use
> a single true array property that accepts a list value. In the external
> interfaces, this is naturally expressed as a JSON list and makes array
> properties accessible again. The new syntax looks like this:
>
> -device '{"driver":"rocker","ports":["dev0","dev1"]}'
>
> Creating an array property on the command line without using JSON format
> is currently not possible. This could be fixed by switching from
> QemuOpts to a keyval parser, which however requires consideration of the
> compatibility implications.
>
> All internal users of devices with array properties go through
> qdev_prop_set_array() at this point, so updating it takes care of all of
> them.
>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1090
> Fixes: f3558b1b763683bb877f7dd5b282469cdadc65c3
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  include/hw/qdev-properties.h |  59 +++++----
>  hw/core/qdev-properties.c    | 224 ++++++++++++++++++++++-------------
>  2 files changed, 183 insertions(+), 100 deletions(-)
>
> diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
> index 7fa2fdb7c9..cac752bade 100644
> --- a/include/hw/qdev-properties.h
> +++ b/include/hw/qdev-properties.h
> @@ -1,7 +1,10 @@
>  #ifndef QEMU_QDEV_PROPERTIES_H
>  #define QEMU_QDEV_PROPERTIES_H
>  
> +#include <stdalign.h>
> +

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

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

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

>  
>  /**
>   * Property:
> @@ -61,7 +64,7 @@ extern const PropertyInfo qdev_prop_size;
>  extern const PropertyInfo qdev_prop_string;
>  extern const PropertyInfo qdev_prop_on_off_auto;
>  extern const PropertyInfo qdev_prop_size32;
> -extern const PropertyInfo qdev_prop_arraylen;
> +extern const PropertyInfo qdev_prop_array;
>  extern const PropertyInfo qdev_prop_link;
>  
>  #define DEFINE_PROP(_name, _state, _field, _prop, _type, ...) {  \
> @@ -115,8 +118,6 @@ extern const PropertyInfo qdev_prop_link;
>                  .bitmask    = (_bitmask),                     \
>                  .set_default = false)
>  
> -#define PROP_ARRAY_LEN_PREFIX "len-"
> -
>  /**
>   * DEFINE_PROP_ARRAY:
>   * @_name: name of the array
> @@ -127,28 +128,46 @@ extern const PropertyInfo qdev_prop_link;
>   * @_arrayprop: PropertyInfo defining what property the array elements have
>   * @_arraytype: C type of the array elements
>   *
> - * Define device properties for a variable-length array _name.  A
> - * static property "len-arrayname" is defined. When the device creator
> - * sets this property to the desired length of array, further dynamic
> - * properties "arrayname[0]", "arrayname[1]", ...  are defined so the
> - * device creator can set the array element values. Setting the
> - * "len-arrayname" property more than once is an error.
> + * Define device properties for a variable-length array _name.  The array is
> + * represented as a list in the visitor interface.
>   *
> - * When the array length is set, the @_field member of the device
> + * @_arraytype is required to be movable with memcpy() and to have an alignment
> + * such that it can be stored at GenericList.padding.


Please wrap like this for readability:

    * Define device properties for a variable-length array _name.  The
    * array is represented as a list in the visitor interface.
    *
    * @_arraytype is required to be movable with memcpy() and to have an
    * alignment such that it can be stored at GenericList.padding.

> + *
> + * When the array property is set, the @_field member of the device
>   * struct is set to the array length, and @_arrayfield is set to point
> - * to (zero-initialised) memory allocated for the array.  For a zero
> - * length array, @_field will be set to 0 and @_arrayfield to NULL.
> + * to the memory allocated for the array.
> + *
>   * It is the responsibility of the device deinit code to free the
>   * @_arrayfield memory.
>   */
> -#define DEFINE_PROP_ARRAY(_name, _state, _field,               \
> -                          _arrayfield, _arrayprop, _arraytype) \
> -    DEFINE_PROP((PROP_ARRAY_LEN_PREFIX _name),                 \
> -                _state, _field, qdev_prop_arraylen, uint32_t,  \
> -                .set_default = true,                           \
> -                .defval.u = 0,                                 \
> -                .arrayinfo = &(_arrayprop),                    \
> -                .arrayfieldsize = sizeof(_arraytype),          \
> +#define DEFINE_PROP_ARRAY(_name, _state, _field,                        \
> +                          _arrayfield, _arrayprop, _arraytype)          \
> +    DEFINE_PROP(_name, _state, _field, qdev_prop_array, uint32_t,       \
> +                .set_default = true,                                    \
> +                .defval.u = 0,                                          \
> +                .arrayinfo = &(_arrayprop),                             \
> +                /*                                                      \
> +                 * set_prop_array() temporarily stores elements at      \
> +                 * GenericList.padding. Make sure that this has the     \
> +                 * right alignment for @_arraytype.                     \
> +                 *                                                      \
> +                 * Hack: In this place, neither static assertions work  \
> +                 * nor is a statement expression allowed. This          \
> +                 * abomination of an expression works because inside    \
> +                 * the declaration of a dummy struct, static assertions \
> +                 * are possible. Using the comma operator causes        \
> +                 * warnings about an unused value and casting to void   \
> +                 * makes the expression not constant in gcc, so instead \
> +                 * of ignoring the first part, make it evaluate to 0    \
> +                 * and add it to the actual result.                     \
> +                 */                                                     \
> +                .arrayfieldsize = (!sizeof(struct {                     \
> +                    QEMU_BUILD_BUG_ON(                                  \
> +                        !QEMU_IS_ALIGNED(sizeof(GenericList),           \
> +                                         alignof(_arraytype)));         \
> +                    int dummy;                                          \
> +                }) + sizeof(_arraytype)),                               \
>                  .arrayoffset = offsetof(_state, _arrayfield))
>  
>  #define DEFINE_PROP_LINK(_name, _state, _field, _type, _ptr_type)     \
> diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
> index fb4daba799..4f3e1152be 100644
> --- a/hw/core/qdev-properties.c
> +++ b/hw/core/qdev-properties.c
> @@ -546,98 +546,174 @@ const PropertyInfo qdev_prop_size32 = {
>  
>  /* --- support for array properties --- */
>  
> -/* Used as an opaque for the object properties we add for each
> - * array element. Note that the struct Property must be first
> - * in the struct so that a pointer to this works as the opaque
> - * for the underlying element's property hooks as well as for
> - * our own release callback.
> +/*
> + * Given an array property @parent_prop in @obj, return a Property for a
> + * specific element of the array. Arrays are backed by an uint32_t length field
> + * and an element array. @elem points at an element in this element array.
>   */

Please wrap like this for readability:

   /*
    * Given an array property @parent_prop in @obj, return a Property for
    * a specific element of the array.  Arrays are backed by an uint32_t
    * length field and an element array.  @elem points at an element in
    * this element array.
    */
> -typedef struct {
> -    struct Property prop;
> -    char *propname;
> -    ObjectPropertyRelease *release;
> -} ArrayElementProperty;
> -
> -/* object property release callback for array element properties:
> - * we call the underlying element's property release hook, and
> - * then free the memory we allocated when we added the property.
> +static Property array_elem_prop(Object *obj, Property *parent_prop,
> +                                const char *name, char *elem)
> +{
> +    return (Property) {
> +        .info = parent_prop->arrayinfo,
> +        .name = name,
> +        /*
> +         * This ugly piece of pointer arithmetic sets up the offset so
> +         * that when the underlying release hook calls qdev_get_prop_ptr
> +         * they get the right answer despite the array element not actually
> +         * being inside the device struct.
> +         */
> +        .offset = (uintptr_t)elem - (uintptr_t)obj,
> +    };
> +}
> +
> +/*
> + * Object property release callback for array properties: We call the
> + * underlying element's property release hook for each element.
> + *
> + * Note that it is the responsibility of the individual device's deinit
> + * to free the array proper.
>   */
> -static void array_element_release(Object *obj, const char *name, void *opaque)
> +static void release_prop_array(Object *obj, const char *name, void *opaque)
>  {
> -    ArrayElementProperty *p = opaque;
> -    if (p->release) {
> -        p->release(obj, name, opaque);
> +    Property *prop = opaque;
> +    uint32_t *alenptr = object_field_prop_ptr(obj, prop);
> +    void **arrayptr = (void *)obj + prop->arrayoffset;
> +    char *elem = *arrayptr;
> +    int i;
> +
> +    if (!prop->arrayinfo->release) {
> +        return;
> +    }
> +
> +    for (i = 0; i < *alenptr; i++) {
> +        Property elem_prop = array_elem_prop(obj, prop, name, elem);
> +        prop->arrayinfo->release(obj, NULL, &elem_prop);
> +        elem += prop->arrayfieldsize;
>      }
> -    g_free(p->propname);
> -    g_free(p);
>  }
>  
> -static void set_prop_arraylen(Object *obj, Visitor *v, const char *name,
> -                              void *opaque, Error **errp)
> +/*
> + * Setter for an array property. This sets both the array length (which
> + * is technically the property field in the object) and the array itself
> + * (a pointer to which is stored in the additional field described by
> + * prop->arrayoffset).
> + */
> +static void set_prop_array(Object *obj, Visitor *v, const char *name,
> +                           void *opaque, Error **errp)
>  {
> -    /* Setter for the property which defines the length of a
> -     * variable-sized property array. As well as actually setting the
> -     * array-length field in the device struct, we have to create the
> -     * array itself and dynamically add the corresponding properties.
> -     */
> +    ERRP_GUARD();
>      Property *prop = opaque;
>      uint32_t *alenptr = object_field_prop_ptr(obj, prop);
>      void **arrayptr = (void *)obj + prop->arrayoffset;
> -    void *eltptr;
> -    const char *arrayname;
> -    int i;
> +    GenericList *list, *elem, *next;
> +    const size_t list_elem_size = sizeof(*list) + prop->arrayfieldsize;
> +    char *elemptr;
> +    bool ok = true;
>  
>      if (*alenptr) {
>          error_setg(errp, "array size property %s may not be set more than once",
>                     name);
>          return;
>      }
> -    if (!visit_type_uint32(v, name, alenptr, errp)) {
> +
> +    if (!visit_start_list(v, name, &list, list_elem_size, errp)) {
>          return;
>      }
> -    if (!*alenptr) {
> +
> +    /* Read the whole input into a temporary list */
> +    elem = list;
> +    while (elem) {
> +        Property elem_prop = array_elem_prop(obj, prop, name, elem->padding);
> +        prop->arrayinfo->set(obj, v, NULL, &elem_prop, errp);
> +        if (*errp) {
> +            ok = false;
> +            goto out_obj;
> +        }
> +        if (*alenptr == INT_MAX) {
> +            error_setg(errp, "array is too big");
> +            return;
> +        }
> +        (*alenptr)++;
> +        elem = visit_next_list(v, elem, list_elem_size);
> +    }
> +
> +    ok = visit_check_list(v, errp);
> +out_obj:
> +    visit_end_list(v, (void**) &list);
> +
> +    if (!ok) {
> +        for (elem = list; elem; elem = next) {
> +            Property elem_prop = array_elem_prop(obj, prop, name,
> +                                                 elem->padding);
> +            if (prop->arrayinfo->release) {
> +                prop->arrayinfo->release(obj, NULL, &elem_prop);
> +            }
> +            next = elem->next;
> +            g_free(elem);
> +        }
>          return;
>      }
>  
> -    /* DEFINE_PROP_ARRAY guarantees that name should start with this prefix;
> -     * strip it off so we can get the name of the array itself.
> +    /*
> +     * Now that we know how big the array has to be, move the data over to a
> +     * linear array and free the temporary list.
>       */
> -    assert(strncmp(name, PROP_ARRAY_LEN_PREFIX,
> -                   strlen(PROP_ARRAY_LEN_PREFIX)) == 0);
> -    arrayname = name + strlen(PROP_ARRAY_LEN_PREFIX);
> +    *arrayptr = g_malloc_n(*alenptr, prop->arrayfieldsize);
> +    elemptr = *arrayptr;
> +    for (elem = list; elem; elem = next) {
> +        memcpy(elemptr, elem->padding, prop->arrayfieldsize);
> +        elemptr += prop->arrayfieldsize;
> +        next = elem->next;
> +        g_free(elem);
> +    }
> +}
>  
> -    /* Note that it is the responsibility of the individual device's deinit
> -     * to free the array proper.
> -     */
> -    *arrayptr = eltptr = g_malloc0(*alenptr * prop->arrayfieldsize);
> -    for (i = 0; i < *alenptr; i++, eltptr += prop->arrayfieldsize) {
> -        char *propname = g_strdup_printf("%s[%d]", arrayname, i);
> -        ArrayElementProperty *arrayprop = g_new0(ArrayElementProperty, 1);
> -        arrayprop->release = prop->arrayinfo->release;
> -        arrayprop->propname = propname;
> -        arrayprop->prop.info = prop->arrayinfo;
> -        arrayprop->prop.name = propname;
> -        /* This ugly piece of pointer arithmetic sets up the offset so
> -         * that when the underlying get/set hooks call qdev_get_prop_ptr
> -         * they get the right answer despite the array element not actually
> -         * being inside the device struct.
> -         */
> -        arrayprop->prop.offset = eltptr - (void *)obj;
> -        assert(object_field_prop_ptr(obj, &arrayprop->prop) == eltptr);
> -        object_property_add(obj, propname,
> -                            arrayprop->prop.info->name,
> -                            field_prop_getter(arrayprop->prop.info),
> -                            field_prop_setter(arrayprop->prop.info),
> -                            array_element_release,
> -                            arrayprop);
> +static void get_prop_array(Object *obj, Visitor *v, const char *name,
> +                           void *opaque, Error **errp)
> +{
> +    ERRP_GUARD();
> +    Property *prop = opaque;
> +    uint32_t *alenptr = object_field_prop_ptr(obj, prop);
> +    void **arrayptr = (void *)obj + prop->arrayoffset;
> +    char *elem = *arrayptr;
> +    GenericList *list;
> +    const size_t list_elem_size = sizeof(*list) + prop->arrayfieldsize;
> +    int i;
> +    bool ok;
> +
> +    if (!visit_start_list(v, name, &list, list_elem_size, errp)) {
> +        return;
>      }
> +
> +    for (i = 0; i < *alenptr; i++) {
> +        Property elem_prop = array_elem_prop(obj, prop, name, elem);
> +        prop->arrayinfo->get(obj, v, NULL, &elem_prop, errp);
> +        if (*errp) {
> +            goto out_obj;
> +        }
> +        elem += prop->arrayfieldsize;
> +    }
> +
> +    /* visit_check_list() can only fail for input visitors */
> +    ok = visit_check_list(v, errp);
> +    assert(ok);
> +
> +out_obj:
> +    visit_end_list(v, (void**) &list);
>  }
>  
> -const PropertyInfo qdev_prop_arraylen = {
> -    .name = "uint32",
> -    .get = get_uint32,
> -    .set = set_prop_arraylen,
> -    .set_default_value = qdev_propinfo_set_default_value_uint,
> +static void default_prop_array(ObjectProperty *op, const Property *prop)
> +{
> +    object_property_set_default_list(op);
> +}
> +
> +const PropertyInfo qdev_prop_array = {
> +    .name = "list",
> +    .get = get_prop_array,
> +    .set = set_prop_array,
> +    .release = release_prop_array,
> +    .set_default_value = default_prop_array,
>  };
>  
>  /* --- public helpers --- */
> @@ -743,20 +819,8 @@ void qdev_prop_set_enum(DeviceState *dev, const char *name, int value)
>  
>  void qdev_prop_set_array(DeviceState *dev, const char *name, QList *values)
>  {
> -    const QListEntry *entry;
> -    g_autofree char *prop_len = g_strdup_printf("len-%s", name);
> -    unsigned i = 0;
> -
> -    object_property_set_int(OBJECT(dev), prop_len, qlist_size(values),
> -                            &error_abort);
> -
> -    QLIST_FOREACH_ENTRY(values, entry) {
> -        g_autofree char *prop_idx = g_strdup_printf("%s[%u]", name, i);
> -        object_property_set_qobject(OBJECT(dev), prop_idx, entry->value,
> -                                    &error_abort);
> -        i++;
> -    }
> -
> +    object_property_set_qobject(OBJECT(dev), name, QOBJECT(values),
> +                                &error_abort);
>      qobject_unref(values);
>  }

Appreciate the comments explaining the hairy parts.

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

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

[Eric Blake]

On Mon, Oct 30, 2023 at 03:26:58PM +0100, Kevin Wolf 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, which used to be configured like this:
> 
> -device rocker,len-ports=2,ports[0]=dev0,ports[1]=dev1

If you tweak the commit message, you might also want to mention that
this form is a shell-glob deathtrap (if you are unlucky enough to have
a file named rocker,len-ports=2,ports0=dev0,ports1=dev1 in the current
directory), and therefore should have used shell quoting...

> 
> 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"]}'

...at which point, the fact that you HAVE to use shell quoting to get
JSON through the command line is less onerous than if the older syntax
had been safe without quotes.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization:  qemu.org | libguestfs.org

Re: [PATCH v2 11/12] qdev: Make netdev properties work as list elements

[Kevin Wolf]

Am 02.11.2023 um 13:55 hat Markus Armbruster geschrieben:
> Kevin Wolf <kwolf@redhat.com> writes:
> 
> > The 'name' parameter of QOM setters is primarily used to specify the name
> > of the currently parsed input element in the visitor interface. For
> > top-level qdev properties, this is always set and matches 'prop->name'.
> >
> > However, for list elements it is NULL, because each element of a list
> > doesn't have a separate name. Passing a non-NULL value runs into
> > assertion failures in the visitor code.
> 
> Yes.  visitor.h's big comment:
> 
>  * The @name parameter of visit_type_FOO() describes the relation
>  * between this QAPI value and its parent container.  When visiting
>  * the root of a tree, @name is ignored; when visiting a member of an
>  * object, @name is the key associated with the value; when visiting a
>  * member of a list, @name is NULL; and when visiting the member of an
>  * alternate, @name should equal the name used for visiting the
>  * alternate.
> 
> > Therefore, using 'name' in error messages is not right for property
> > types that are used in lists, because "(null)" isn't very helpful to
> > identify what QEMU is complaining about.
> 
> We get "(null)" on some hosts, and SEGV on others.

Fair, I can add "(or even a segfault)" to the commit message.

> Same problem in all property setters and getters (qdev and QOM), I
> presume.  The @name parameter looks like a death trap.  Thoughts?

All property setters and getters that abuse @name instead of just
passing it to the visitor. I don't know how many do, but let's see:

* qdev_prop_size32 uses @name in an error message

* Array properties themselves use @name in an error message, too
  (preexisting)

* qdev-properties-system.c has more complicated properties, which have
  the same problem: drive, chardev, macaddr, netdev (before this patch),
  reserved_region, pci_devfn (one error path explicitly considers name
  == NULL, but another doesn't), uuid

* Outside of hw/core/ we may have some problematic properties, too.
  I found qdev_prop_tpm, and am now too lazy to go through all devices.
  These are probably specialised enough that they are less likely to be
  used in arrays.

We can fix these on top of this series, though we should probably first
figure out what the best way to replace it is to avoid touching
everything twice.

> Any reproducer known before the next patch?

Not to me.

> > Change netdev properties to use 'prop->name' instead, which will contain
> > the name of the array property after switching array properties to lists
> > in the external interface.
> 
> Points at the entire array property without telling the user which of
> the elements is at fault.  Sure better than NULL.  I'm not asking you to
> improve the error message further.  Mention the error message's lack of
> precision in the commit message?

Can do.

At least the input visitor has better ways internally to identify the
currently visited object with full_name(). If other visitor types can do
similar things, maybe extending the public visitor interface to access
the name would be possible?

Kevin

Re: [PATCH v2 11/12] qdev: Make netdev properties work as list elements

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 02.11.2023 um 13:55 hat Markus Armbruster geschrieben:
>> Kevin Wolf <kwolf@redhat.com> writes:
>> 
>> > The 'name' parameter of QOM setters is primarily used to specify the name
>> > of the currently parsed input element in the visitor interface. For
>> > top-level qdev properties, this is always set and matches 'prop->name'.
>> >
>> > However, for list elements it is NULL, because each element of a list
>> > doesn't have a separate name. Passing a non-NULL value runs into
>> > assertion failures in the visitor code.
>> 
>> Yes.  visitor.h's big comment:
>> 
>>  * The @name parameter of visit_type_FOO() describes the relation
>>  * between this QAPI value and its parent container.  When visiting
>>  * the root of a tree, @name is ignored; when visiting a member of an
>>  * object, @name is the key associated with the value; when visiting a
>>  * member of a list, @name is NULL; and when visiting the member of an
>>  * alternate, @name should equal the name used for visiting the
>>  * alternate.
>> 
>> > Therefore, using 'name' in error messages is not right for property
>> > types that are used in lists, because "(null)" isn't very helpful to
>> > identify what QEMU is complaining about.
>> 
>> We get "(null)" on some hosts, and SEGV on others.
>
> Fair, I can add "(or even a segfault)" to the commit message.
>
>> Same problem in all property setters and getters (qdev and QOM), I
>> presume.  The @name parameter looks like a death trap.  Thoughts?
>
> All property setters and getters that abuse @name instead of just
> passing it to the visitor. I don't know how many do, but let's see:
>
> * qdev_prop_size32 uses @name in an error message
>
> * Array properties themselves use @name in an error message, too
>   (preexisting)
>
> * qdev-properties-system.c has more complicated properties, which have
>   the same problem: drive, chardev, macaddr, netdev (before this patch),
>   reserved_region, pci_devfn (one error path explicitly considers name
>   == NULL, but another doesn't), uuid
>
> * Outside of hw/core/ we may have some problematic properties, too.
>   I found qdev_prop_tpm, and am now too lazy to go through all devices.
>   These are probably specialised enough that they are less likely to be
>   used in arrays.
>
> We can fix these on top of this series, though we should probably first
> figure out what the best way to replace it is to avoid touching
> everything twice.

Makes sense.

>> Any reproducer known before the next patch?
>
> Not to me.
>
>> > Change netdev properties to use 'prop->name' instead, which will contain
>> > the name of the array property after switching array properties to lists
>> > in the external interface.
>> 
>> Points at the entire array property without telling the user which of
>> the elements is at fault.  Sure better than NULL.  I'm not asking you to
>> improve the error message further.  Mention the error message's lack of
>> precision in the commit message?
>
> Can do.
>
> At least the input visitor has better ways internally to identify the
> currently visited object with full_name(). If other visitor types can do
> similar things, maybe extending the public visitor interface to access
> the name would be possible?

Yes, there's a need for better error reporting around visitors, ideally
without doing it in every visitor like the QObject input visitor does.
I've thought about it, but only briefly, and without a conclusion.

I figure your idea is to provide a visitor method to get a malloced
string describing the thing being visited.  Might work, but to know for
sure, we have to try.

This could also help with getting rid of the dangerous @name use we
discussed above.

Thanks!