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

[PATCH 00/11] 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 11 for details.

Kevin Wolf (11):
  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: Rework array properties based on list visitor

 include/hw/qdev-properties.h     |  26 ++--
 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        | 197 ++++++++++++++++++++-----------
 hw/i386/pc.c                     |   8 +-
 hw/rx/rx62n.c                    |  19 +--
 qom/object.c                     |   6 +
 13 files changed, 227 insertions(+), 129 deletions(-)

-- 
2.41.0

[PATCH 01/11] 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>
---
 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..950ef48e01 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);
+    uint32_t i = 0;
+
+    object_property_set_int(OBJECT(dev), prop_len, qlist_size(values),
+                            &error_abort);
+
+    QLIST_FOREACH_ENTRY(values, entry) {
+        g_autofree char *prop_idx = g_strdup_printf("%s[%u]", name, i);
+        object_property_set_qobject(OBJECT(dev), prop_idx, entry->value,
+                                    &error_abort);
+        i++;
+    }
+
+    qobject_unref(values);
+}
+
 static GPtrArray *global_props(void)
 {
     static GPtrArray *gp;
-- 
2.41.0

[PATCH 02/11] 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>
---
 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 54838c0c41..0e84e454cb 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -81,6 +81,7 @@
 #include "qapi/error.h"
 #include "qapi/qapi-visit-common.h"
 #include "qapi/qapi-visit-machine.h"
+#include "qapi/qmp/qlist.h"
 #include "qapi/visitor.h"
 #include "hw/core/cpu.h"
 #include "hw/usb.h"
@@ -1508,9 +1509,10 @@ static void pc_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev,
         char *resv_prop_str = g_strdup_printf("0xfee00000:0xfeefffff:%d",
                                               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);
+        QList *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 03/11] 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>
---
 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 04/11] 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>
---
 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 05/11] 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>
---
 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 bc89eb4806..354c1a037d 100644
--- a/hw/arm/sbsa-ref.c
+++ b/hw/arm/sbsa-ref.c
@@ -46,6 +46,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
@@ -441,6 +442,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();
@@ -459,8 +461,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 06/11] 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>
---
 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 56abadd9b8..7c7af0a9cb 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"
 
 #define VEXPRESS_BOARD_ID 0x8e0
@@ -551,6 +552,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);
@@ -591,20 +593,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 07/11] 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>
---
 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 a13c658bbf..be04f68b48 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"
@@ -746,14 +747,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) {
@@ -762,14 +772,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",
@@ -2743,6 +2745,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) {
@@ -2769,9 +2772,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 08/11] 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>
---
 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 60bf5fe657..fe2a810f9f 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 09/11] 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>
---
 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 3e887a0fc7..6990096642 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(sysctl, "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(sysctl, "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 10/11] 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>
---
 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 e25f1e96db..c328591dba 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"
@@ -1574,6 +1575,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 11/11] 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.

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

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

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

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

diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
index 7fa2fdb7c9..9370b36b72 100644
--- a/include/hw/qdev-properties.h
+++ b/include/hw/qdev-properties.h
@@ -61,7 +61,7 @@ extern const PropertyInfo qdev_prop_size;
 extern const PropertyInfo qdev_prop_string;
 extern const PropertyInfo qdev_prop_on_off_auto;
 extern const PropertyInfo qdev_prop_size32;
-extern const PropertyInfo qdev_prop_arraylen;
+extern const PropertyInfo qdev_prop_array;
 extern const PropertyInfo qdev_prop_link;
 
 #define DEFINE_PROP(_name, _state, _field, _prop, _type, ...) {  \
@@ -115,8 +115,6 @@ extern const PropertyInfo qdev_prop_link;
                 .bitmask    = (_bitmask),                     \
                 .set_default = false)
 
-#define PROP_ARRAY_LEN_PREFIX "len-"
-
 /**
  * DEFINE_PROP_ARRAY:
  * @_name: name of the array
@@ -127,24 +125,21 @@ extern const PropertyInfo qdev_prop_link;
  * @_arrayprop: PropertyInfo defining what property the array elements have
  * @_arraytype: C type of the array elements
  *
- * Define device properties for a variable-length array _name.  A
- * static property "len-arrayname" is defined. When the device creator
- * sets this property to the desired length of array, further dynamic
- * properties "arrayname[0]", "arrayname[1]", ...  are defined so the
- * device creator can set the array element values. Setting the
- * "len-arrayname" property more than once is an error.
+ * Define device properties for a variable-length array _name.  The array is
+ * represented as a list in the visitor interface.
+ *
+ * @_arraytype is required to be movable with memcpy().
  *
- * When the array length is set, the @_field member of the device
+ * When the array property is set, the @_field member of the device
  * struct is set to the array length, and @_arrayfield is set to point
- * to (zero-initialised) memory allocated for the array.  For a zero
- * length array, @_field will be set to 0 and @_arrayfield to NULL.
+ * to the memory allocated for the array.
+ *
  * It is the responsibility of the device deinit code to free the
  * @_arrayfield memory.
  */
 #define DEFINE_PROP_ARRAY(_name, _state, _field,               \
                           _arrayfield, _arrayprop, _arraytype) \
-    DEFINE_PROP((PROP_ARRAY_LEN_PREFIX _name),                 \
-                _state, _field, qdev_prop_arraylen, uint32_t,  \
+    DEFINE_PROP(_name, _state, _field, qdev_prop_array, uint32_t,     \
                 .set_default = true,                           \
                 .defval.u = 0,                                 \
                 .arrayinfo = &(_arrayprop),                    \
diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c
index 6d5d43eda2..f557ee886e 100644
--- a/hw/core/qdev-properties-system.c
+++ b/hw/core/qdev-properties-system.c
@@ -450,7 +450,7 @@ static void set_netdev(Object *obj, Visitor *v, const char *name,
     peers_ptr->queues = queues;
 
 out:
-    error_set_from_qdev_prop_error(errp, err, obj, name, str);
+    error_set_from_qdev_prop_error(errp, err, obj, prop->name, str);
     g_free(str);
 }
 
diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
index 950ef48e01..b2303a6fbc 100644
--- a/hw/core/qdev-properties.c
+++ b/hw/core/qdev-properties.c
@@ -546,98 +546,152 @@ const PropertyInfo qdev_prop_size32 = {
 
 /* --- support for array properties --- */
 
-/* Used as an opaque for the object properties we add for each
- * array element. Note that the struct Property must be first
- * in the struct so that a pointer to this works as the opaque
- * for the underlying element's property hooks as well as for
- * our own release callback.
- */
-typedef struct {
-    struct Property prop;
-    char *propname;
-    ObjectPropertyRelease *release;
-} ArrayElementProperty;
-
-/* object property release callback for array element properties:
- * we call the underlying element's property release hook, and
- * then free the memory we allocated when we added the property.
+static Property array_elem_prop(Object *obj, Property *parent_prop,
+                                const char *name, char *elem)
+{
+    return (Property) {
+        .info = parent_prop->arrayinfo,
+        .name = name,
+        /*
+         * This ugly piece of pointer arithmetic sets up the offset so
+         * that when the underlying release hook calls qdev_get_prop_ptr
+         * they get the right answer despite the array element not actually
+         * being inside the device struct.
+         */
+        .offset = elem - (char *) obj,
+    };
+}
+
+/*
+ * Object property release callback for array properties: We call the underlying
+ * element's property release hook for each element.
+ *
+ * Note that it is the responsibility of the individual device's deinit to free
+ * the array proper.
  */
-static void array_element_release(Object *obj, const char *name, void *opaque)
+static void release_prop_array(Object *obj, const char *name, void *opaque)
 {
-    ArrayElementProperty *p = opaque;
-    if (p->release) {
-        p->release(obj, name, opaque);
+    Property *prop = opaque;
+    uint32_t *alenptr = object_field_prop_ptr(obj, prop);
+    void **arrayptr = (void *)obj + prop->arrayoffset;
+    char *elem = *arrayptr;
+    int i;
+
+    for (i = 0; i < *alenptr; i++) {
+        Property elem_prop = array_elem_prop(obj, prop, name, elem);
+        prop->arrayinfo->release(obj, NULL, &elem_prop);
+        elem += prop->arrayfieldsize;
     }
-    g_free(p->propname);
-    g_free(p);
 }
 
-static void set_prop_arraylen(Object *obj, Visitor *v, const char *name,
-                              void *opaque, Error **errp)
+/*
+ * Setter for an array property. This sets both the array length (which is
+ * technically the property field in the object) and the array itself (a pointer
+ * to which is stored in the additional field described by prop->arrayoffset).
+ */
+static void set_prop_array(Object *obj, Visitor *v, const char *name,
+                           void *opaque, Error **errp)
 {
-    /* Setter for the property which defines the length of a
-     * variable-sized property array. As well as actually setting the
-     * array-length field in the device struct, we have to create the
-     * array itself and dynamically add the corresponding properties.
-     */
+    ERRP_GUARD();
+
     Property *prop = opaque;
     uint32_t *alenptr = object_field_prop_ptr(obj, prop);
     void **arrayptr = (void *)obj + prop->arrayoffset;
-    void *eltptr;
-    const char *arrayname;
-    int i;
+    GenericList *list, *elem, *next;
+    const size_t list_elem_size = sizeof(*list) + prop->arrayfieldsize;
+    char *elemptr;
+    bool ok = true;
 
     if (*alenptr) {
         error_setg(errp, "array size property %s may not be set more than once",
                    name);
         return;
     }
-    if (!visit_type_uint32(v, name, alenptr, errp)) {
+
+    if (!visit_start_list(v, name, &list, list_elem_size, errp)) {
         return;
     }
-    if (!*alenptr) {
+
+    /* Read the whole input into a temporary list */
+    elem = list;
+    while (elem) {
+        Property elem_prop = array_elem_prop(obj, prop, name, elem->padding);
+        prop->arrayinfo->set(obj, v, NULL, &elem_prop, errp);
+        if (*errp) {
+            ok = false;
+            goto out_obj;
+        }
+        (*alenptr)++;
+        elem = visit_next_list(v, elem, list_elem_size);
+    }
+
+    ok = visit_check_list(v, errp);
+out_obj:
+    visit_end_list(v, (void**) &list);
+
+    if (!ok) {
+        for (elem = list; elem; elem = next) {
+            next = elem->next;
+            g_free(elem);
+        }
         return;
     }
 
-    /* DEFINE_PROP_ARRAY guarantees that name should start with this prefix;
-     * strip it off so we can get the name of the array itself.
+    /*
+     * Now that we know how big the array has to be, move the data over to a
+     * linear array and free the temporary list.
      */
-    assert(strncmp(name, PROP_ARRAY_LEN_PREFIX,
-                   strlen(PROP_ARRAY_LEN_PREFIX)) == 0);
-    arrayname = name + strlen(PROP_ARRAY_LEN_PREFIX);
+    *arrayptr = g_malloc_n(*alenptr, prop->arrayfieldsize);
+    elemptr = *arrayptr;
+    for (elem = list; elem; elem = next) {
+        memcpy(elemptr, elem->padding, prop->arrayfieldsize);
+        elemptr += prop->arrayfieldsize;
+        next = elem->next;
+        g_free(elem);
+    }
+}
 
-    /* Note that it is the responsibility of the individual device's deinit
-     * to free the array proper.
-     */
-    *arrayptr = eltptr = g_malloc0(*alenptr * prop->arrayfieldsize);
-    for (i = 0; i < *alenptr; i++, eltptr += prop->arrayfieldsize) {
-        char *propname = g_strdup_printf("%s[%d]", arrayname, i);
-        ArrayElementProperty *arrayprop = g_new0(ArrayElementProperty, 1);
-        arrayprop->release = prop->arrayinfo->release;
-        arrayprop->propname = propname;
-        arrayprop->prop.info = prop->arrayinfo;
-        arrayprop->prop.name = propname;
-        /* This ugly piece of pointer arithmetic sets up the offset so
-         * that when the underlying get/set hooks call qdev_get_prop_ptr
-         * they get the right answer despite the array element not actually
-         * being inside the device struct.
-         */
-        arrayprop->prop.offset = eltptr - (void *)obj;
-        assert(object_field_prop_ptr(obj, &arrayprop->prop) == eltptr);
-        object_property_add(obj, propname,
-                            arrayprop->prop.info->name,
-                            field_prop_getter(arrayprop->prop.info),
-                            field_prop_setter(arrayprop->prop.info),
-                            array_element_release,
-                            arrayprop);
+static void get_prop_array(Object *obj, Visitor *v, const char *name,
+                           void *opaque, Error **errp)
+{
+    ERRP_GUARD();
+
+    Property *prop = opaque;
+    uint32_t *alenptr = object_field_prop_ptr(obj, prop);
+    void **arrayptr = (void *)obj + prop->arrayoffset;
+    char *elem = *arrayptr;
+    GenericList *list;
+    const size_t list_elem_size = sizeof(*list) + prop->arrayfieldsize;
+    int i;
+
+    if (!visit_start_list(v, name, &list, list_elem_size, errp)) {
+        return;
     }
+
+    for (i = 0; i < *alenptr; i++) {
+        Property elem_prop = array_elem_prop(obj, prop, name, elem);
+        prop->arrayinfo->get(obj, v, NULL, &elem_prop, errp);
+        if (*errp) {
+            goto out_obj;
+        }
+        elem += prop->arrayfieldsize;
+    }
+
+out_obj:
+    visit_end_list(v, (void**) &list);
 }
 
-const PropertyInfo qdev_prop_arraylen = {
-    .name = "uint32",
-    .get = get_uint32,
-    .set = set_prop_arraylen,
-    .set_default_value = qdev_propinfo_set_default_value_uint,
+static void default_prop_array(ObjectProperty *op, const Property *prop)
+{
+    object_property_set_default_list(op);
+}
+
+const PropertyInfo qdev_prop_array = {
+    .name = "list",
+    .get = get_prop_array,
+    .set = set_prop_array,
+    .release = release_prop_array,
+    .set_default_value = default_prop_array,
 };
 
 /* --- public helpers --- */
@@ -743,20 +797,8 @@ void qdev_prop_set_enum(DeviceState *dev, const char *name, int value)
 
 void qdev_prop_set_array(DeviceState *dev, const char *name, QList *values)
 {
-    const QListEntry *entry;
-    g_autofree char *prop_len = g_strdup_printf("len-%s", name);
-    uint32_t i = 0;
-
-    object_property_set_int(OBJECT(dev), prop_len, qlist_size(values),
-                            &error_abort);
-
-    QLIST_FOREACH_ENTRY(values, entry) {
-        g_autofree char *prop_idx = g_strdup_printf("%s[%u]", name, i);
-        object_property_set_qobject(OBJECT(dev), prop_idx, entry->value,
-                                    &error_abort);
-        i++;
-    }
-
+    object_property_set_qobject(OBJECT(dev), name, QOBJECT(values),
+                                &error_abort);
     qobject_unref(values);
 }
 
-- 
2.41.0

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

[Peter Maydell]

On Fri, 8 Sept 2023 at 15:37, Kevin Wolf <kwolf@redhat.com> wrote:
>
> Until now, array properties are actually implemented with a hack that
> uses multiple properties on the QOM level: a static "foo-len" property
> and after it is set, dynamically created "foo[i]" properties.
>
> In external interfaces (-device on the command line and device_add in
> QMP), this interface was broken by commit f3558b1b ('qdev: Base object
> creation on QDict rather than QemuOpts') because QDicts are unordered
> and therefore it could happen that QEMU tried to set the indexed
> properties before setting the length, which fails and effectively makes
> array properties inaccessible. In particular, this affects the 'ports'
> property of the 'rocker' device.
>
> This patch reworks the external interface so that instead of using a
> separate top-level property for the length and for each element, we use
> a single true array property that accepts a list value. In the external
> interfaces, this is naturally expressed as a JSON list and makes array
> properties accessible again.
>
> Creating an array property on the command line without using JSON format
> is currently not possible. This could be fixed by switching from
> QemuOpts to a keyval parser, which however requires consideration of the
> compatibility implications.

Could we have a specific example in the commit message of:

The old (currently broken) syntax for setting the ports
property on the rocker device is:
 -device rocker,len-ports=2,ports[0]=dev0,ports[1]=dev1
The new syntax that works as of this commit is:
 [whatever]

?

I would expect most users have no idea what the JSON list
syntax is.

thanks
-- PMM

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

[Kevin Wolf]

Am 08.09.2023 um 17:18 hat Peter Maydell geschrieben:
> On Fri, 8 Sept 2023 at 15:37, Kevin Wolf <kwolf@redhat.com> wrote:
> >
> > Until now, array properties are actually implemented with a hack that
> > uses multiple properties on the QOM level: a static "foo-len" property
> > and after it is set, dynamically created "foo[i]" properties.
> >
> > In external interfaces (-device on the command line and device_add in
> > QMP), this interface was broken by commit f3558b1b ('qdev: Base object
> > creation on QDict rather than QemuOpts') because QDicts are unordered
> > and therefore it could happen that QEMU tried to set the indexed
> > properties before setting the length, which fails and effectively makes
> > array properties inaccessible. In particular, this affects the 'ports'
> > property of the 'rocker' device.
> >
> > This patch reworks the external interface so that instead of using a
> > separate top-level property for the length and for each element, we use
> > a single true array property that accepts a list value. In the external
> > interfaces, this is naturally expressed as a JSON list and makes array
> > properties accessible again.
> >
> > Creating an array property on the command line without using JSON format
> > is currently not possible. This could be fixed by switching from
> > QemuOpts to a keyval parser, which however requires consideration of the
> > compatibility implications.
> 
> Could we have a specific example in the commit message of:
> 
> The old (currently broken) syntax for setting the ports
> property on the rocker device is:
>  -device rocker,len-ports=2,ports[0]=dev0,ports[1]=dev1
> The new syntax that works as of this commit is:
>  [whatever]
> 
> ?

Sure, that's a good idea.

> I would expect most users have no idea what the JSON list
> syntax is.

To fill in your "[whatever]", it's something like this:

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

If we can eventually get -device converted to the keyval parser instead
of QemuOpts, the non-JSON syntax will look like this:

-device rocker,ports.0=dev0,ports.1=dev1

But I assume we'll have to solve some compatibility problems with other
devices before this can be done.

Kevin

Re: [PATCH 01/11] qdev: Add qdev_prop_set_array()

[Peter Maydell]

On Fri, 8 Sept 2023 at 15:37, Kevin Wolf <kwolf@redhat.com> wrote:
>
> 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>

thanks
-- PMM

Re: [PATCH 02/11] hw/i386/pc: Use qdev_prop_set_array()

[Peter Maydell]

On Fri, 8 Sept 2023 at 15:37, Kevin Wolf <kwolf@redhat.com> wrote:
>
> 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>
> ---
>  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 54838c0c41..0e84e454cb 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -81,6 +81,7 @@
>  #include "qapi/error.h"
>  #include "qapi/qapi-visit-common.h"
>  #include "qapi/qapi-visit-machine.h"
> +#include "qapi/qmp/qlist.h"
>  #include "qapi/visitor.h"
>  #include "hw/core/cpu.h"
>  #include "hw/usb.h"
> @@ -1508,9 +1509,10 @@ static void pc_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev,
>          char *resv_prop_str = g_strdup_printf("0xfee00000:0xfeefffff:%d",
>                                                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);
> +        QList *reserved_regions = qlist_new();
> +        qlist_append_str(reserved_regions, resv_prop_str);
> +        qdev_prop_set_array(dev, "reserved-regions", reserved_regions);
> +

The variable declaration should be at the top of the block;
otherwise
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [PATCH 03/11] hw/arm/mps2-tz: Use qdev_prop_set_array()

[Peter Maydell]

On Fri, 8 Sept 2023 at 15:37, Kevin Wolf <kwolf@redhat.com> wrote:
>
> 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>

thanks
-- PMM

Re: [PATCH 04/11] hw/arm/mps2: Use qdev_prop_set_array()

[Peter Maydell]

On Fri, 8 Sept 2023 at 15:37, Kevin Wolf <kwolf@redhat.com> wrote:
>
> 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>
> ---
>  hw/arm/mps2.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [PATCH 05/11] hw/arm/sbsa-ref: Use qdev_prop_set_array()

[Peter Maydell]

On Fri, 8 Sept 2023 at 15:37, Kevin Wolf <kwolf@redhat.com> wrote:
>
> 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>

thanks
-- PMM

Re: [PATCH 06/11] hw/arm/vexpress: Use qdev_prop_set_array()

[Peter Maydell]

On Fri, 8 Sept 2023 at 15:37, Kevin Wolf <kwolf@redhat.com> wrote:
>
> 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>

thanks
-- PMM

Re: [PATCH 07/11] hw/arm/virt: Use qdev_prop_set_array()

[Peter Maydell]

On Fri, 8 Sept 2023 at 15:39, Kevin Wolf <kwolf@redhat.com> wrote:
>
> 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>

thanks
-- PMM

Re: [PATCH 08/11] hw/arm/xlnx-versal: Use qdev_prop_set_array()

[Peter Maydell]

On Fri, 8 Sept 2023 at 15:37, Kevin Wolf <kwolf@redhat.com> wrote:
>
> 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>

thanks
-- PMM

Re: [PATCH 09/11] hw/rx/rx62n: Use qdev_prop_set_array()

[Peter Maydell]

On Fri, 8 Sept 2023 at 15:37, Kevin Wolf <kwolf@redhat.com> wrote:
>
> 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>
> ---
>  hw/rx/rx62n.c | 19 ++++++++++---------

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [PATCH 10/11] qom: Add object_property_set_default_list()

[Peter Maydell]

On Fri, 8 Sept 2023 at 15:37, Kevin Wolf <kwolf@redhat.com> wrote:
>
> 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>

thanks
-- PMM

Re: [PATCH 02/11] hw/i386/pc: Use qdev_prop_set_array()

[Kevin Wolf]

Am 11.09.2023 um 17:42 hat Peter Maydell geschrieben:
> On Fri, 8 Sept 2023 at 15:37, Kevin Wolf <kwolf@redhat.com> wrote:
> >
> > 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>
> > ---
> >  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 54838c0c41..0e84e454cb 100644
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -81,6 +81,7 @@
> >  #include "qapi/error.h"
> >  #include "qapi/qapi-visit-common.h"
> >  #include "qapi/qapi-visit-machine.h"
> > +#include "qapi/qmp/qlist.h"
> >  #include "qapi/visitor.h"
> >  #include "hw/core/cpu.h"
> >  #include "hw/usb.h"
> > @@ -1508,9 +1509,10 @@ static void pc_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev,
> >          char *resv_prop_str = g_strdup_printf("0xfee00000:0xfeefffff:%d",
> >                                                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);
> > +        QList *reserved_regions = qlist_new();
> > +        qlist_append_str(reserved_regions, resv_prop_str);
> > +        qdev_prop_set_array(dev, "reserved-regions", reserved_regions);
> > +
> 
> The variable declaration should be at the top of the block;

It is at the top of the block, the only thing before it is another
variable declaration. Would you prefer to have the empty line removed or
after the declaration to make this visually clearer?

Kevin

Re: [PATCH 09/11] hw/rx/rx62n: Use qdev_prop_set_array()

[Philippe Mathieu-Daudé]

On 8/9/23 16:37, Kevin Wolf wrote:
> 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>
> ---
>   hw/rx/rx62n.c | 19 ++++++++++---------
>   1 file changed, 10 insertions(+), 9 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 01/11] qdev: Add qdev_prop_set_array()

[Philippe Mathieu-Daudé]

On 8/9/23 16:36, Kevin Wolf wrote:
> 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>
> ---
>   include/hw/qdev-properties.h |  3 +++
>   hw/core/qdev-properties.c    | 21 +++++++++++++++++++++
>   2 files changed, 24 insertions(+)


> +void qdev_prop_set_array(DeviceState *dev, const char *name, QList *values)
> +{
> +    const QListEntry *entry;
> +    g_autofree char *prop_len = g_strdup_printf("len-%s", name);
> +    uint32_t i = 0;

"unsigned"? Anyway,

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

> +
> +    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);
> +}

Re: [PATCH 02/11] hw/i386/pc: Use qdev_prop_set_array()

[Peter Maydell]

On Mon, 11 Sept 2023 at 17:42, Kevin Wolf <kwolf@redhat.com> wrote:
>
> Am 11.09.2023 um 17:42 hat Peter Maydell geschrieben:
> > On Fri, 8 Sept 2023 at 15:37, Kevin Wolf <kwolf@redhat.com> wrote:
> > >
> > > 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>
> > > ---
> > >  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 54838c0c41..0e84e454cb 100644
> > > --- a/hw/i386/pc.c
> > > +++ b/hw/i386/pc.c
> > > @@ -81,6 +81,7 @@
> > >  #include "qapi/error.h"
> > >  #include "qapi/qapi-visit-common.h"
> > >  #include "qapi/qapi-visit-machine.h"
> > > +#include "qapi/qmp/qlist.h"
> > >  #include "qapi/visitor.h"
> > >  #include "hw/core/cpu.h"
> > >  #include "hw/usb.h"
> > > @@ -1508,9 +1509,10 @@ static void pc_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev,
> > >          char *resv_prop_str = g_strdup_printf("0xfee00000:0xfeefffff:%d",
> > >                                                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);
> > > +        QList *reserved_regions = qlist_new();
> > > +        qlist_append_str(reserved_regions, resv_prop_str);
> > > +        qdev_prop_set_array(dev, "reserved-regions", reserved_regions);
> > > +
> >
> > The variable declaration should be at the top of the block;
>
> It is at the top of the block, the only thing before it is another
> variable declaration. Would you prefer to have the empty line removed or
> after the declaration to make this visually clearer?

Sorry, I think I just misread the diff somehow. I guess that having
the blank line after the variable declarations would be more usual,
but it doesn't really matter.

-- PMM

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

[Peter Maydell]

On Fri, 8 Sept 2023 at 15:37, Kevin Wolf <kwolf@redhat.com> wrote:
>
> Until now, array properties are actually implemented with a hack that
> uses multiple properties on the QOM level: a static "foo-len" property
> and after it is set, dynamically created "foo[i]" properties.
>
> In external interfaces (-device on the command line and device_add in
> QMP), this interface was broken by commit f3558b1b ('qdev: Base object
> creation on QDict rather than QemuOpts') because QDicts are unordered
> and therefore it could happen that QEMU tried to set the indexed
> properties before setting the length, which fails and effectively makes
> array properties inaccessible. In particular, this affects the 'ports'
> property of the 'rocker' device.
>
> This patch reworks the external interface so that instead of using a
> separate top-level property for the length and for each element, we use
> a single true array property that accepts a list value. In the external
> interfaces, this is naturally expressed as a JSON list and makes array
> properties accessible again.
>
> Creating an array property on the command line without using JSON format
> is currently not possible. This could be fixed by switching from
> QemuOpts to a keyval parser, which however requires consideration of the
> compatibility implications.
>
> All internal users of devices with array properties go through
> qdev_prop_set_array() at this point, so updating it takes care of all of
> them.
>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1090
> Fixes: f3558b1b763683bb877f7dd5b282469cdadc65c3
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>

I'm hoping that somebody who understands the visitor APIs
better than me will have a look at this patch, but in the
meantime, here's my review, which I suspect has a lot of
comments that mostly reflect that I don't really understand
the visitor stuff...

> ---
>  include/hw/qdev-properties.h     |  23 ++--
>  hw/core/qdev-properties-system.c |   2 +-
>  hw/core/qdev-properties.c        | 204 +++++++++++++++++++------------
>  3 files changed, 133 insertions(+), 96 deletions(-)
>
> diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
> index 7fa2fdb7c9..9370b36b72 100644
> --- a/include/hw/qdev-properties.h
> +++ b/include/hw/qdev-properties.h
> @@ -61,7 +61,7 @@ extern const PropertyInfo qdev_prop_size;
>  extern const PropertyInfo qdev_prop_string;
>  extern const PropertyInfo qdev_prop_on_off_auto;
>  extern const PropertyInfo qdev_prop_size32;
> -extern const PropertyInfo qdev_prop_arraylen;
> +extern const PropertyInfo qdev_prop_array;
>  extern const PropertyInfo qdev_prop_link;
>
>  #define DEFINE_PROP(_name, _state, _field, _prop, _type, ...) {  \
> @@ -115,8 +115,6 @@ extern const PropertyInfo qdev_prop_link;
>                  .bitmask    = (_bitmask),                     \
>                  .set_default = false)
>
> -#define PROP_ARRAY_LEN_PREFIX "len-"
> -
>  /**
>   * DEFINE_PROP_ARRAY:
>   * @_name: name of the array
> @@ -127,24 +125,21 @@ extern const PropertyInfo qdev_prop_link;
>   * @_arrayprop: PropertyInfo defining what property the array elements have
>   * @_arraytype: C type of the array elements
>   *
> - * Define device properties for a variable-length array _name.  A
> - * static property "len-arrayname" is defined. When the device creator
> - * sets this property to the desired length of array, further dynamic
> - * properties "arrayname[0]", "arrayname[1]", ...  are defined so the
> - * device creator can set the array element values. Setting the
> - * "len-arrayname" property more than once is an error.
> + * Define device properties for a variable-length array _name.  The array is
> + * represented as a list in the visitor interface.
> + *
> + * @_arraytype is required to be movable with memcpy().
>   *
> - * When the array length is set, the @_field member of the device
> + * When the array property is set, the @_field member of the device
>   * struct is set to the array length, and @_arrayfield is set to point
> - * to (zero-initialised) memory allocated for the array.  For a zero
> - * length array, @_field will be set to 0 and @_arrayfield to NULL.
> + * to the memory allocated for the array.
> + *
>   * It is the responsibility of the device deinit code to free the
>   * @_arrayfield memory.
>   */
>  #define DEFINE_PROP_ARRAY(_name, _state, _field,               \
>                            _arrayfield, _arrayprop, _arraytype) \
> -    DEFINE_PROP((PROP_ARRAY_LEN_PREFIX _name),                 \
> -                _state, _field, qdev_prop_arraylen, uint32_t,  \
> +    DEFINE_PROP(_name, _state, _field, qdev_prop_array, uint32_t,     \
>                  .set_default = true,                           \
>                  .defval.u = 0,                                 \
>                  .arrayinfo = &(_arrayprop),                    \
> diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c
> index 6d5d43eda2..f557ee886e 100644
> --- a/hw/core/qdev-properties-system.c
> +++ b/hw/core/qdev-properties-system.c
> @@ -450,7 +450,7 @@ static void set_netdev(Object *obj, Visitor *v, const char *name,
>      peers_ptr->queues = queues;
>
>  out:
> -    error_set_from_qdev_prop_error(errp, err, obj, name, str);
> +    error_set_from_qdev_prop_error(errp, err, obj, prop->name, str);
>      g_free(str);
>  }

Is this change intentional? It's not clear to me why the netdev
property setter needs to change.

> diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
> index 950ef48e01..b2303a6fbc 100644
> --- a/hw/core/qdev-properties.c
> +++ b/hw/core/qdev-properties.c
> @@ -546,98 +546,152 @@ const PropertyInfo qdev_prop_size32 = {
>
>  /* --- support for array properties --- */
>
> -/* Used as an opaque for the object properties we add for each
> - * array element. Note that the struct Property must be first
> - * in the struct so that a pointer to this works as the opaque
> - * for the underlying element's property hooks as well as for
> - * our own release callback.
> - */
> -typedef struct {
> -    struct Property prop;
> -    char *propname;
> -    ObjectPropertyRelease *release;
> -} ArrayElementProperty;
> -
> -/* object property release callback for array element properties:
> - * we call the underlying element's property release hook, and
> - * then free the memory we allocated when we added the property.
> +static Property array_elem_prop(Object *obj, Property *parent_prop,
> +                                const char *name, char *elem)
> +{
> +    return (Property) {
> +        .info = parent_prop->arrayinfo,
> +        .name = name,
> +        /*
> +         * This ugly piece of pointer arithmetic sets up the offset so
> +         * that when the underlying release hook calls qdev_get_prop_ptr
> +         * they get the right answer despite the array element not actually
> +         * being inside the device struct.
> +         */
> +        .offset = elem - (char *) obj,

Stray space after ')'.

> +    };
> +}
> +
> +/*
> + * Object property release callback for array properties: We call the underlying
> + * element's property release hook for each element.
> + *
> + * Note that it is the responsibility of the individual device's deinit to free
> + * the array proper.
>   */
> -static void array_element_release(Object *obj, const char *name, void *opaque)
> +static void release_prop_array(Object *obj, const char *name, void *opaque)
>  {
> -    ArrayElementProperty *p = opaque;
> -    if (p->release) {
> -        p->release(obj, name, opaque);
> +    Property *prop = opaque;
> +    uint32_t *alenptr = object_field_prop_ptr(obj, prop);
> +    void **arrayptr = (void *)obj + prop->arrayoffset;
> +    char *elem = *arrayptr;
> +    int i;
> +
> +    for (i = 0; i < *alenptr; i++) {

Is there something somewhere that enforces that a list can't
have more than INT_MAX elements? Otherwise this will go wrong,
I think, since we're iterating with an 'int'.

> +        Property elem_prop = array_elem_prop(obj, prop, name, elem);
> +        prop->arrayinfo->release(obj, NULL, &elem_prop);
> +        elem += prop->arrayfieldsize;
>      }
> -    g_free(p->propname);
> -    g_free(p);
>  }
>
> -static void set_prop_arraylen(Object *obj, Visitor *v, const char *name,
> -                              void *opaque, Error **errp)
> +/*
> + * Setter for an array property. This sets both the array length (which is
> + * technically the property field in the object) and the array itself (a pointer
> + * to which is stored in the additional field described by prop->arrayoffset).
> + */
> +static void set_prop_array(Object *obj, Visitor *v, const char *name,
> +                           void *opaque, Error **errp)

Is there something somewhere in this that guards against the
caller trying to set an array property with a list that doesn't
have elements that are all the same type?

>  {
> -    /* Setter for the property which defines the length of a
> -     * variable-sized property array. As well as actually setting the
> -     * array-length field in the device struct, we have to create the
> -     * array itself and dynamically add the corresponding properties.
> -     */
> +    ERRP_GUARD();
> +
>      Property *prop = opaque;
>      uint32_t *alenptr = object_field_prop_ptr(obj, prop);
>      void **arrayptr = (void *)obj + prop->arrayoffset;
> -    void *eltptr;
> -    const char *arrayname;
> -    int i;
> +    GenericList *list, *elem, *next;
> +    const size_t list_elem_size = sizeof(*list) + prop->arrayfieldsize;
> +    char *elemptr;
> +    bool ok = true;
>
>      if (*alenptr) {
>          error_setg(errp, "array size property %s may not be set more than once",
>                     name);
>          return;
>      }
> -    if (!visit_type_uint32(v, name, alenptr, errp)) {
> +
> +    if (!visit_start_list(v, name, &list, list_elem_size, errp)) {
>          return;
>      }
> -    if (!*alenptr) {
> +
> +    /* Read the whole input into a temporary list */

Why do we need the temporary list? Shouldn't we already know
at this point the length of the list and be able to allocate
the memory and write directly into that?

> +    elem = list;
> +    while (elem) {
> +        Property elem_prop = array_elem_prop(obj, prop, name, elem->padding);
> +        prop->arrayinfo->set(obj, v, NULL, &elem_prop, errp);

Why do we call a set() function if we're getting the value
of the array item ?

> +        if (*errp) {
> +            ok = false;
> +            goto out_obj;
> +        }
> +        (*alenptr)++;
> +        elem = visit_next_list(v, elem, list_elem_size);
> +    }
> +
> +    ok = visit_check_list(v, errp);
> +out_obj:
> +    visit_end_list(v, (void**) &list);
> +
> +    if (!ok) {
> +        for (elem = list; elem; elem = next) {
> +            next = elem->next;
> +            g_free(elem);
> +        }
>          return;
>      }
>
> -    /* DEFINE_PROP_ARRAY guarantees that name should start with this prefix;
> -     * strip it off so we can get the name of the array itself.
> +    /*
> +     * Now that we know how big the array has to be, move the data over to a
> +     * linear array and free the temporary list.
>       */
> -    assert(strncmp(name, PROP_ARRAY_LEN_PREFIX,
> -                   strlen(PROP_ARRAY_LEN_PREFIX)) == 0);
> -    arrayname = name + strlen(PROP_ARRAY_LEN_PREFIX);
> +    *arrayptr = g_malloc_n(*alenptr, prop->arrayfieldsize);
> +    elemptr = *arrayptr;
> +    for (elem = list; elem; elem = next) {
> +        memcpy(elemptr, elem->padding, prop->arrayfieldsize);
> +        elemptr += prop->arrayfieldsize;
> +        next = elem->next;
> +        g_free(elem);
> +    }
> +}
>
> -    /* Note that it is the responsibility of the individual device's deinit
> -     * to free the array proper.
> -     */
> -    *arrayptr = eltptr = g_malloc0(*alenptr * prop->arrayfieldsize);
> -    for (i = 0; i < *alenptr; i++, eltptr += prop->arrayfieldsize) {
> -        char *propname = g_strdup_printf("%s[%d]", arrayname, i);
> -        ArrayElementProperty *arrayprop = g_new0(ArrayElementProperty, 1);
> -        arrayprop->release = prop->arrayinfo->release;
> -        arrayprop->propname = propname;
> -        arrayprop->prop.info = prop->arrayinfo;
> -        arrayprop->prop.name = propname;
> -        /* This ugly piece of pointer arithmetic sets up the offset so
> -         * that when the underlying get/set hooks call qdev_get_prop_ptr
> -         * they get the right answer despite the array element not actually
> -         * being inside the device struct.
> -         */
> -        arrayprop->prop.offset = eltptr - (void *)obj;
> -        assert(object_field_prop_ptr(obj, &arrayprop->prop) == eltptr);
> -        object_property_add(obj, propname,
> -                            arrayprop->prop.info->name,
> -                            field_prop_getter(arrayprop->prop.info),
> -                            field_prop_setter(arrayprop->prop.info),
> -                            array_element_release,
> -                            arrayprop);
> +static void get_prop_array(Object *obj, Visitor *v, const char *name,
> +                           void *opaque, Error **errp)
> +{
> +    ERRP_GUARD();
> +
> +    Property *prop = opaque;
> +    uint32_t *alenptr = object_field_prop_ptr(obj, prop);
> +    void **arrayptr = (void *)obj + prop->arrayoffset;
> +    char *elem = *arrayptr;
> +    GenericList *list;
> +    const size_t list_elem_size = sizeof(*list) + prop->arrayfieldsize;
> +    int i;
> +
> +    if (!visit_start_list(v, name, &list, list_elem_size, errp)) {
> +        return;
>      }
> +
> +    for (i = 0; i < *alenptr; i++) {
> +        Property elem_prop = array_elem_prop(obj, prop, name, elem);
> +        prop->arrayinfo->get(obj, v, NULL, &elem_prop, errp);
> +        if (*errp) {
> +            goto out_obj;
> +        }
> +        elem += prop->arrayfieldsize;
> +    }
> +
> +out_obj:
> +    visit_end_list(v, (void**) &list);
>  }
>
> -const PropertyInfo qdev_prop_arraylen = {
> -    .name = "uint32",
> -    .get = get_uint32,
> -    .set = set_prop_arraylen,
> -    .set_default_value = qdev_propinfo_set_default_value_uint,
> +static void default_prop_array(ObjectProperty *op, const Property *prop)
> +{
> +    object_property_set_default_list(op);
> +}
> +
> +const PropertyInfo qdev_prop_array = {
> +    .name = "list",
> +    .get = get_prop_array,
> +    .set = set_prop_array,
> +    .release = release_prop_array,
> +    .set_default_value = default_prop_array,
>  };
>
>  /* --- public helpers --- */
> @@ -743,20 +797,8 @@ void qdev_prop_set_enum(DeviceState *dev, const char *name, int value)
>
>  void qdev_prop_set_array(DeviceState *dev, const char *name, QList *values)
>  {
> -    const QListEntry *entry;
> -    g_autofree char *prop_len = g_strdup_printf("len-%s", name);
> -    uint32_t i = 0;
> -
> -    object_property_set_int(OBJECT(dev), prop_len, qlist_size(values),
> -                            &error_abort);
> -
> -    QLIST_FOREACH_ENTRY(values, entry) {
> -        g_autofree char *prop_idx = g_strdup_printf("%s[%u]", name, i);
> -        object_property_set_qobject(OBJECT(dev), prop_idx, entry->value,
> -                                    &error_abort);
> -        i++;
> -    }
> -
> +    object_property_set_qobject(OBJECT(dev), name, QOBJECT(values),
> +                                &error_abort);
>      qobject_unref(values);
>  }
>
> --
> 2.41.0

thanks
-- PMM

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

[Kevin Wolf]

Am 14.09.2023 um 12:24 hat Peter Maydell geschrieben:
> On Fri, 8 Sept 2023 at 15:37, Kevin Wolf <kwolf@redhat.com> wrote:
> >
> > Until now, array properties are actually implemented with a hack that
> > uses multiple properties on the QOM level: a static "foo-len" property
> > and after it is set, dynamically created "foo[i]" properties.
> >
> > In external interfaces (-device on the command line and device_add in
> > QMP), this interface was broken by commit f3558b1b ('qdev: Base object
> > creation on QDict rather than QemuOpts') because QDicts are unordered
> > and therefore it could happen that QEMU tried to set the indexed
> > properties before setting the length, which fails and effectively makes
> > array properties inaccessible. In particular, this affects the 'ports'
> > property of the 'rocker' device.
> >
> > This patch reworks the external interface so that instead of using a
> > separate top-level property for the length and for each element, we use
> > a single true array property that accepts a list value. In the external
> > interfaces, this is naturally expressed as a JSON list and makes array
> > properties accessible again.
> >
> > Creating an array property on the command line without using JSON format
> > is currently not possible. This could be fixed by switching from
> > QemuOpts to a keyval parser, which however requires consideration of the
> > compatibility implications.
> >
> > All internal users of devices with array properties go through
> > qdev_prop_set_array() at this point, so updating it takes care of all of
> > them.
> >
> > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1090
> > Fixes: f3558b1b763683bb877f7dd5b282469cdadc65c3
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> 
> I'm hoping that somebody who understands the visitor APIs
> better than me will have a look at this patch, but in the
> meantime, here's my review, which I suspect has a lot of
> comments that mostly reflect that I don't really understand
> the visitor stuff...

I discussed the visitor aspects with Markus before sending the series,
so I think he agrees with the approach. But I wouldn't mind an explicit
Reviewed-by, of course.

> > ---
> >  include/hw/qdev-properties.h     |  23 ++--
> >  hw/core/qdev-properties-system.c |   2 +-
> >  hw/core/qdev-properties.c        | 204 +++++++++++++++++++------------
> >  3 files changed, 133 insertions(+), 96 deletions(-)
> >
> > diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
> > index 7fa2fdb7c9..9370b36b72 100644
> > --- a/include/hw/qdev-properties.h
> > +++ b/include/hw/qdev-properties.h
> > @@ -61,7 +61,7 @@ extern const PropertyInfo qdev_prop_size;
> >  extern const PropertyInfo qdev_prop_string;
> >  extern const PropertyInfo qdev_prop_on_off_auto;
> >  extern const PropertyInfo qdev_prop_size32;
> > -extern const PropertyInfo qdev_prop_arraylen;
> > +extern const PropertyInfo qdev_prop_array;
> >  extern const PropertyInfo qdev_prop_link;
> >
> >  #define DEFINE_PROP(_name, _state, _field, _prop, _type, ...) {  \
> > @@ -115,8 +115,6 @@ extern const PropertyInfo qdev_prop_link;
> >                  .bitmask    = (_bitmask),                     \
> >                  .set_default = false)
> >
> > -#define PROP_ARRAY_LEN_PREFIX "len-"
> > -
> >  /**
> >   * DEFINE_PROP_ARRAY:
> >   * @_name: name of the array
> > @@ -127,24 +125,21 @@ extern const PropertyInfo qdev_prop_link;
> >   * @_arrayprop: PropertyInfo defining what property the array elements have
> >   * @_arraytype: C type of the array elements
> >   *
> > - * Define device properties for a variable-length array _name.  A
> > - * static property "len-arrayname" is defined. When the device creator
> > - * sets this property to the desired length of array, further dynamic
> > - * properties "arrayname[0]", "arrayname[1]", ...  are defined so the
> > - * device creator can set the array element values. Setting the
> > - * "len-arrayname" property more than once is an error.
> > + * Define device properties for a variable-length array _name.  The array is
> > + * represented as a list in the visitor interface.
> > + *
> > + * @_arraytype is required to be movable with memcpy().
> >   *
> > - * When the array length is set, the @_field member of the device
> > + * When the array property is set, the @_field member of the device
> >   * struct is set to the array length, and @_arrayfield is set to point
> > - * to (zero-initialised) memory allocated for the array.  For a zero
> > - * length array, @_field will be set to 0 and @_arrayfield to NULL.
> > + * to the memory allocated for the array.
> > + *
> >   * It is the responsibility of the device deinit code to free the
> >   * @_arrayfield memory.
> >   */
> >  #define DEFINE_PROP_ARRAY(_name, _state, _field,               \
> >                            _arrayfield, _arrayprop, _arraytype) \
> > -    DEFINE_PROP((PROP_ARRAY_LEN_PREFIX _name),                 \
> > -                _state, _field, qdev_prop_arraylen, uint32_t,  \
> > +    DEFINE_PROP(_name, _state, _field, qdev_prop_array, uint32_t,     \
> >                  .set_default = true,                           \
> >                  .defval.u = 0,                                 \
> >                  .arrayinfo = &(_arrayprop),                    \
> > diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c
> > index 6d5d43eda2..f557ee886e 100644
> > --- a/hw/core/qdev-properties-system.c
> > +++ b/hw/core/qdev-properties-system.c
> > @@ -450,7 +450,7 @@ static void set_netdev(Object *obj, Visitor *v, const char *name,
> >      peers_ptr->queues = queues;
> >
> >  out:
> > -    error_set_from_qdev_prop_error(errp, err, obj, name, str);
> > +    error_set_from_qdev_prop_error(errp, err, obj, prop->name, str);
> >      g_free(str);
> >  }
> 
> Is this change intentional? It's not clear to me why the netdev
> property setter needs to change.

It is, but you're also right that it's not obvious. I should mention
this hunk in the commit message or even move it to a separate patch.

The problem is that @name is primarily used for the visitor interface,
and for list elements it has to be NULL (because each element doesn't
have a separate name; passing a non-NULL value runs into assertion
failures in the visitor code).

But of course, in error messages "(null)" isn't very helpful to identify
what QEMU is complaining about. prop->name contains the name of the
array property, so that's what I changed it to. Outside of lists, I
think name and prop->name are always the same.

> > diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
> > index 950ef48e01..b2303a6fbc 100644
> > --- a/hw/core/qdev-properties.c
> > +++ b/hw/core/qdev-properties.c
> > @@ -546,98 +546,152 @@ const PropertyInfo qdev_prop_size32 = {
> >
> >  /* --- support for array properties --- */
> >
> > -/* Used as an opaque for the object properties we add for each
> > - * array element. Note that the struct Property must be first
> > - * in the struct so that a pointer to this works as the opaque
> > - * for the underlying element's property hooks as well as for
> > - * our own release callback.
> > - */
> > -typedef struct {
> > -    struct Property prop;
> > -    char *propname;
> > -    ObjectPropertyRelease *release;
> > -} ArrayElementProperty;
> > -
> > -/* object property release callback for array element properties:
> > - * we call the underlying element's property release hook, and
> > - * then free the memory we allocated when we added the property.
> > +static Property array_elem_prop(Object *obj, Property *parent_prop,
> > +                                const char *name, char *elem)
> > +{
> > +    return (Property) {
> > +        .info = parent_prop->arrayinfo,
> > +        .name = name,
> > +        /*
> > +         * This ugly piece of pointer arithmetic sets up the offset so
> > +         * that when the underlying release hook calls qdev_get_prop_ptr
> > +         * they get the right answer despite the array element not actually
> > +         * being inside the device struct.
> > +         */
> > +        .offset = elem - (char *) obj,
> 
> Stray space after ')'.
> 
> > +    };
> > +}
> > +
> > +/*
> > + * Object property release callback for array properties: We call the underlying
> > + * element's property release hook for each element.
> > + *
> > + * Note that it is the responsibility of the individual device's deinit to free
> > + * the array proper.
> >   */
> > -static void array_element_release(Object *obj, const char *name, void *opaque)
> > +static void release_prop_array(Object *obj, const char *name, void *opaque)
> >  {
> > -    ArrayElementProperty *p = opaque;
> > -    if (p->release) {
> > -        p->release(obj, name, opaque);
> > +    Property *prop = opaque;
> > +    uint32_t *alenptr = object_field_prop_ptr(obj, prop);
> > +    void **arrayptr = (void *)obj + prop->arrayoffset;
> > +    char *elem = *arrayptr;
> > +    int i;
> > +
> > +    for (i = 0; i < *alenptr; i++) {
> 
> Is there something somewhere that enforces that a list can't
> have more than INT_MAX elements? Otherwise this will go wrong,
> I think, since we're iterating with an 'int'.

I expect you might run out of memory or get another problem before that,
but you're right, there is no explicit check. Maybe set_prop_array()
should check that *alenptr doesn't become larger than some limit. It
could be INT_MAX or maybe even something lower.

> > +        Property elem_prop = array_elem_prop(obj, prop, name, elem);
> > +        prop->arrayinfo->release(obj, NULL, &elem_prop);
> > +        elem += prop->arrayfieldsize;
> >      }
> > -    g_free(p->propname);
> > -    g_free(p);
> >  }
> >
> > -static void set_prop_arraylen(Object *obj, Visitor *v, const char *name,
> > -                              void *opaque, Error **errp)
> > +/*
> > + * Setter for an array property. This sets both the array length (which is
> > + * technically the property field in the object) and the array itself (a pointer
> > + * to which is stored in the additional field described by prop->arrayoffset).
> > + */
> > +static void set_prop_array(Object *obj, Visitor *v, const char *name,
> > +                           void *opaque, Error **errp)
> 
> Is there something somewhere in this that guards against the
> caller trying to set an array property with a list that doesn't
> have elements that are all the same type?

Kind of, we always call the same prop->arrayinfo->set() callback for
every element, which is what visits the element. So unless that is doing
crazy things, it should always parse the same structure.

If we ever get to QAPIfying device_add/-device, QAPI would actually
enforce it in a way that individual property implementations can't work
around. For now, we just rely on property implementations being
sensible.

> >  {
> > -    /* Setter for the property which defines the length of a
> > -     * variable-sized property array. As well as actually setting the
> > -     * array-length field in the device struct, we have to create the
> > -     * array itself and dynamically add the corresponding properties.
> > -     */
> > +    ERRP_GUARD();
> > +
> >      Property *prop = opaque;
> >      uint32_t *alenptr = object_field_prop_ptr(obj, prop);
> >      void **arrayptr = (void *)obj + prop->arrayoffset;
> > -    void *eltptr;
> > -    const char *arrayname;
> > -    int i;
> > +    GenericList *list, *elem, *next;
> > +    const size_t list_elem_size = sizeof(*list) + prop->arrayfieldsize;
> > +    char *elemptr;
> > +    bool ok = true;
> >
> >      if (*alenptr) {
> >          error_setg(errp, "array size property %s may not be set more than once",
> >                     name);
> >          return;
> >      }
> > -    if (!visit_type_uint32(v, name, alenptr, errp)) {
> > +
> > +    if (!visit_start_list(v, name, &list, list_elem_size, errp)) {
> >          return;
> >      }
> > -    if (!*alenptr) {
> > +
> > +    /* Read the whole input into a temporary list */
> 
> Why do we need the temporary list? Shouldn't we already know
> at this point the length of the list and be able to allocate
> the memory and write directly into that?

No, unfortunately we don't know the length yet. I discussed adding a
visitor interface to return the length with Markus, but even ignoring
the complexity of defining the finer details of that interface and
implementing it in all visitor types, in the end it would essentially
mean that the visitor parses the input twice, which isn't any better.

> > +    elem = list;
> > +    while (elem) {
> > +        Property elem_prop = array_elem_prop(obj, prop, name, elem->padding);
> > +        prop->arrayinfo->set(obj, v, NULL, &elem_prop, errp);
> 
> Why do we call a set() function if we're getting the value
> of the array item ?

This is set_prop_array(), i.e. the setter for the whole property array.
This line sets individual elements that will be in the array (on the QOM
level), so we need the setter for the element property - which of course
does internally get values from the visitor, but it's the set method of
the QOM property, not of the visitor.

> > +        if (*errp) {
> > +            ok = false;
> > +            goto out_obj;
> > +        }
> > +        (*alenptr)++;
> > +        elem = visit_next_list(v, elem, list_elem_size);
> > +    }
> > +
> > +    ok = visit_check_list(v, errp);
> > +out_obj:
> > +    visit_end_list(v, (void**) &list);
> > +
> > +    if (!ok) {
> > +        for (elem = list; elem; elem = next) {
> > +            next = elem->next;
> > +            g_free(elem);
> > +        }
> >          return;
> >      }
> >
> > -    /* DEFINE_PROP_ARRAY guarantees that name should start with this prefix;
> > -     * strip it off so we can get the name of the array itself.
> > +    /*
> > +     * Now that we know how big the array has to be, move the data over to a
> > +     * linear array and free the temporary list.
> >       */
> > -    assert(strncmp(name, PROP_ARRAY_LEN_PREFIX,
> > -                   strlen(PROP_ARRAY_LEN_PREFIX)) == 0);
> > -    arrayname = name + strlen(PROP_ARRAY_LEN_PREFIX);
> > +    *arrayptr = g_malloc_n(*alenptr, prop->arrayfieldsize);
> > +    elemptr = *arrayptr;
> > +    for (elem = list; elem; elem = next) {
> > +        memcpy(elemptr, elem->padding, prop->arrayfieldsize);
> > +        elemptr += prop->arrayfieldsize;
> > +        next = elem->next;
> > +        g_free(elem);
> > +    }
> > +}

Kevin

Re: [PATCH 02/11] hw/i386/pc: Use qdev_prop_set_array()

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> On Mon, 11 Sept 2023 at 17:42, Kevin Wolf <kwolf@redhat.com> wrote:
>>
>> Am 11.09.2023 um 17:42 hat Peter Maydell geschrieben:
>> > On Fri, 8 Sept 2023 at 15:37, Kevin Wolf <kwolf@redhat.com> wrote:
>> > >
>> > > 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>
>> > > ---
>> > >  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 54838c0c41..0e84e454cb 100644
>> > > --- a/hw/i386/pc.c
>> > > +++ b/hw/i386/pc.c
>> > > @@ -81,6 +81,7 @@
>> > >  #include "qapi/error.h"
>> > >  #include "qapi/qapi-visit-common.h"
>> > >  #include "qapi/qapi-visit-machine.h"
>> > > +#include "qapi/qmp/qlist.h"
>> > >  #include "qapi/visitor.h"
>> > >  #include "hw/core/cpu.h"
>> > >  #include "hw/usb.h"
>> > > @@ -1508,9 +1509,10 @@ static void pc_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev,
>> > >          char *resv_prop_str = g_strdup_printf("0xfee00000:0xfeefffff:%d",
>> > >                                                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);
>> > > +        QList *reserved_regions = qlist_new();
>> > > +        qlist_append_str(reserved_regions, resv_prop_str);
>> > > +        qdev_prop_set_array(dev, "reserved-regions", reserved_regions);
>> > > +
>> >
>> > The variable declaration should be at the top of the block;
>>
>> It is at the top of the block, the only thing before it is another
>> variable declaration. Would you prefer to have the empty line removed or
>> after the declaration to make this visually clearer?
>
> Sorry, I think I just misread the diff somehow. I guess that having
> the blank line after the variable declarations would be more usual,
> but it doesn't really matter.

I really like to have a blank line between declarations and statements,
because declaration vs. statement can be difficult to decide at a
glance, and the blank line convention helps.

Re: [PATCH 09/11] hw/rx/rx62n: Use qdev_prop_set_array()

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> 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>
> ---
>  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 3e887a0fc7..6990096642 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(sysctl, "ipr-map", ipr_map);

../hw/rx/rx62n.c:143:25: error: ‘sysctl’ undeclared (first use in this function); did you mean ‘syscall’?

Should be DEVICE(icu), I guess.

> +
> +    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(sysctl, "trigger-level", trigger_level);

Again.

>  
>      for (i = 0; i < NR_IRQS; i++) {
>          s->irq[i] = qdev_get_gpio_in(DEVICE(icu), i);

Re: [PATCH 01/11] qdev: Add qdev_prop_set_array()

[Markus Armbruster]

Philippe Mathieu-Daudé <philmd@linaro.org> writes:

> On 8/9/23 16:36, Kevin Wolf wrote:
>> 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>
>> ---
>>   include/hw/qdev-properties.h |  3 +++
>>   hw/core/qdev-properties.c    | 21 +++++++++++++++++++++
>>   2 files changed, 24 insertions(+)
>
>
>> +void qdev_prop_set_array(DeviceState *dev, const char *name, QList *values)
>> +{
>> +    const QListEntry *entry;
>> +    g_autofree char *prop_len = g_strdup_printf("len-%s", name);
>> +    uint32_t i = 0;
>
> "unsigned"? Anyway,

Yes, or even plain int.  It all gets replaced in the last patch, though.

> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>
>> +
>> +    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);
>> +}

Re: [PATCH 11/11] 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.
>
> This patch reworks the external interface so that instead of using a
> separate top-level property for the length and for each element, we use
> a single true array property that accepts a list value. In the external
> interfaces, this is naturally expressed as a JSON list and makes array
> properties accessible again.
>
> Creating an array property on the command line without using JSON format
> is currently not possible. This could be fixed by switching from
> QemuOpts to a keyval parser, which however requires consideration of the
> compatibility implications.
>
> All internal users of devices with array properties go through
> qdev_prop_set_array() at this point, so updating it takes care of all of
> them.
>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1090
> Fixes: f3558b1b763683bb877f7dd5b282469cdadc65c3
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  include/hw/qdev-properties.h     |  23 ++--
>  hw/core/qdev-properties-system.c |   2 +-
>  hw/core/qdev-properties.c        | 204 +++++++++++++++++++------------
>  3 files changed, 133 insertions(+), 96 deletions(-)
>
> diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
> index 7fa2fdb7c9..9370b36b72 100644
> --- a/include/hw/qdev-properties.h
> +++ b/include/hw/qdev-properties.h
> @@ -61,7 +61,7 @@ extern const PropertyInfo qdev_prop_size;
>  extern const PropertyInfo qdev_prop_string;
>  extern const PropertyInfo qdev_prop_on_off_auto;
>  extern const PropertyInfo qdev_prop_size32;
> -extern const PropertyInfo qdev_prop_arraylen;
> +extern const PropertyInfo qdev_prop_array;
>  extern const PropertyInfo qdev_prop_link;
>  
>  #define DEFINE_PROP(_name, _state, _field, _prop, _type, ...) {  \
> @@ -115,8 +115,6 @@ extern const PropertyInfo qdev_prop_link;
>                  .bitmask    = (_bitmask),                     \
>                  .set_default = false)
>  
> -#define PROP_ARRAY_LEN_PREFIX "len-"
> -
>  /**
>   * DEFINE_PROP_ARRAY:
>   * @_name: name of the array
> @@ -127,24 +125,21 @@ extern const PropertyInfo qdev_prop_link;
>   * @_arrayprop: PropertyInfo defining what property the array elements have
>   * @_arraytype: C type of the array elements
>   *
> - * Define device properties for a variable-length array _name.  A
> - * static property "len-arrayname" is defined. When the device creator
> - * sets this property to the desired length of array, further dynamic
> - * properties "arrayname[0]", "arrayname[1]", ...  are defined so the
> - * device creator can set the array element values. Setting the
> - * "len-arrayname" property more than once is an error.
> + * Define device properties for a variable-length array _name.  The array is

Please wrap comments around column 70.  More of the same below, not
noted again.

> + * represented as a list in the visitor interface.
> + *
> + * @_arraytype is required to be movable with memcpy().
>   *
> - * When the array length is set, the @_field member of the device
> + * When the array property is set, the @_field member of the device
>   * struct is set to the array length, and @_arrayfield is set to point
> - * to (zero-initialised) memory allocated for the array.  For a zero
> - * length array, @_field will be set to 0 and @_arrayfield to NULL.
> + * to the memory allocated for the array.

Worth mentioning that the @field member must be uint32_t?

> + *
>   * It is the responsibility of the device deinit code to free the
>   * @_arrayfield memory.
>   */
>  #define DEFINE_PROP_ARRAY(_name, _state, _field,               \
>                            _arrayfield, _arrayprop, _arraytype) \
> -    DEFINE_PROP((PROP_ARRAY_LEN_PREFIX _name),                 \
> -                _state, _field, qdev_prop_arraylen, uint32_t,  \
> +    DEFINE_PROP(_name, _state, _field, qdev_prop_array, uint32_t,     \
>                  .set_default = true,                           \
>                  .defval.u = 0,                                 \
>                  .arrayinfo = &(_arrayprop),                    \

The backslashes are no longer aligned.

> diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c
> index 6d5d43eda2..f557ee886e 100644
> --- a/hw/core/qdev-properties-system.c
> +++ b/hw/core/qdev-properties-system.c
> @@ -450,7 +450,7 @@ static void set_netdev(Object *obj, Visitor *v, const char *name,
>      peers_ptr->queues = queues;
>  
>  out:
> -    error_set_from_qdev_prop_error(errp, err, obj, name, str);
> +    error_set_from_qdev_prop_error(errp, err, obj, prop->name, str);
>      g_free(str);
>  }
>  

Intentional?

> diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
> index 950ef48e01..b2303a6fbc 100644
> --- a/hw/core/qdev-properties.c
> +++ b/hw/core/qdev-properties.c
> @@ -546,98 +546,152 @@ const PropertyInfo qdev_prop_size32 = {
>  
>  /* --- support for array properties --- */
>  
> -/* Used as an opaque for the object properties we add for each
> - * array element. Note that the struct Property must be first
> - * in the struct so that a pointer to this works as the opaque
> - * for the underlying element's property hooks as well as for
> - * our own release callback.
> - */
> -typedef struct {
> -    struct Property prop;
> -    char *propname;
> -    ObjectPropertyRelease *release;
> -} ArrayElementProperty;
> -
> -/* object property release callback for array element properties:
> - * we call the underlying element's property release hook, and
> - * then free the memory we allocated when we added the property.
> +static Property array_elem_prop(Object *obj, Property *parent_prop,
> +                                const char *name, char *elem)

@parent_prop is an array property.  It's backed by an uint32_t length
and an element array.  @elem points into the element array.  Correct?

> +{
> +    return (Property) {
> +        .info = parent_prop->arrayinfo,
> +        .name = name,
> +        /*
> +         * This ugly piece of pointer arithmetic sets up the offset so
> +         * that when the underlying release hook calls qdev_get_prop_ptr
> +         * they get the right answer despite the array element not actually
> +         * being inside the device struct.
> +         */
> +        .offset = elem - (char *) obj,

Isn't this is undefined behavior?

Delete the space between (char *) and obj.

> +    };
> +}
> +
> +/*
> + * Object property release callback for array properties: We call the underlying
> + * element's property release hook for each element.
> + *
> + * Note that it is the responsibility of the individual device's deinit to free
> + * the array proper.

What is a device's "deinit"?  Is it the unrealize() method?  The
instance_finalize() method?

>   */
> -static void array_element_release(Object *obj, const char *name, void *opaque)
> +static void release_prop_array(Object *obj, const char *name, void *opaque)
>  {
> -    ArrayElementProperty *p = opaque;
> -    if (p->release) {
> -        p->release(obj, name, opaque);
> +    Property *prop = opaque;
> +    uint32_t *alenptr = object_field_prop_ptr(obj, prop);
> +    void **arrayptr = (void *)obj + prop->arrayoffset;

I'd call these @plen and @pelts, but that's clearly a matter of taste.

> +    char *elem = *arrayptr;
> +    int i;
> +
> +    for (i = 0; i < *alenptr; i++) {
> +        Property elem_prop = array_elem_prop(obj, prop, name, elem);
> +        prop->arrayinfo->release(obj, NULL, &elem_prop);
> +        elem += prop->arrayfieldsize;
>      }
> -    g_free(p->propname);
> -    g_free(p);
>  }
>  
> -static void set_prop_arraylen(Object *obj, Visitor *v, const char *name,
> -                              void *opaque, Error **errp)
> +/*
> + * Setter for an array property. This sets both the array length (which is
> + * technically the property field in the object) and the array itself (a pointer
> + * to which is stored in the additional field described by prop->arrayoffset).
> + */
> +static void set_prop_array(Object *obj, Visitor *v, const char *name,
> +                           void *opaque, Error **errp)
>  {
> -    /* Setter for the property which defines the length of a
> -     * variable-sized property array. As well as actually setting the
> -     * array-length field in the device struct, we have to create the
> -     * array itself and dynamically add the corresponding properties.
> -     */
> +    ERRP_GUARD();
> +

Drop the blank line.

>      Property *prop = opaque;
>      uint32_t *alenptr = object_field_prop_ptr(obj, prop);
>      void **arrayptr = (void *)obj + prop->arrayoffset;
> -    void *eltptr;
> -    const char *arrayname;
> -    int i;
> +    GenericList *list, *elem, *next;
> +    const size_t list_elem_size = sizeof(*list) + prop->arrayfieldsize;

This can be smaller than the size of the QAPI-generated list type, since
the compiler may add padding.  Does it matter?

> +    char *elemptr;
> +    bool ok = true;
>  
>      if (*alenptr) {
>          error_setg(errp, "array size property %s may not be set more than once",
>                     name);
>          return;
>      }
> -    if (!visit_type_uint32(v, name, alenptr, errp)) {
> +
> +    if (!visit_start_list(v, name, &list, list_elem_size, errp)) {
>          return;
>      }
> -    if (!*alenptr) {
> +
> +    /* Read the whole input into a temporary list */
> +    elem = list;
> +    while (elem) {
> +        Property elem_prop = array_elem_prop(obj, prop, name, elem->padding);
> +        prop->arrayinfo->set(obj, v, NULL, &elem_prop, errp);
> +        if (*errp) {
> +            ok = false;
> +            goto out_obj;
> +        }
> +        (*alenptr)++;
> +        elem = visit_next_list(v, elem, list_elem_size);
> +    }
> +
> +    ok = visit_check_list(v, errp);
> +out_obj:
> +    visit_end_list(v, (void**) &list);
> +
> +    if (!ok) {
> +        for (elem = list; elem; elem = next) {
> +            next = elem->next;
> +            g_free(elem);
> +        }

We consume the list even on error.  It's too late in my day for me to
see why that's proper.

>          return;
>      }
>  
> -    /* DEFINE_PROP_ARRAY guarantees that name should start with this prefix;
> -     * strip it off so we can get the name of the array itself.
> +    /*
> +     * Now that we know how big the array has to be, move the data over to a
> +     * linear array and free the temporary list.
>       */
> -    assert(strncmp(name, PROP_ARRAY_LEN_PREFIX,
> -                   strlen(PROP_ARRAY_LEN_PREFIX)) == 0);
> -    arrayname = name + strlen(PROP_ARRAY_LEN_PREFIX);
> +    *arrayptr = g_malloc_n(*alenptr, prop->arrayfieldsize);
> +    elemptr = *arrayptr;
> +    for (elem = list; elem; elem = next) {
> +        memcpy(elemptr, elem->padding, prop->arrayfieldsize);
> +        elemptr += prop->arrayfieldsize;
> +        next = elem->next;
> +        g_free(elem);
> +    }
> +}
>  
> -    /* Note that it is the responsibility of the individual device's deinit
> -     * to free the array proper.
> -     */
> -    *arrayptr = eltptr = g_malloc0(*alenptr * prop->arrayfieldsize);
> -    for (i = 0; i < *alenptr; i++, eltptr += prop->arrayfieldsize) {
> -        char *propname = g_strdup_printf("%s[%d]", arrayname, i);
> -        ArrayElementProperty *arrayprop = g_new0(ArrayElementProperty, 1);
> -        arrayprop->release = prop->arrayinfo->release;
> -        arrayprop->propname = propname;
> -        arrayprop->prop.info = prop->arrayinfo;
> -        arrayprop->prop.name = propname;
> -        /* This ugly piece of pointer arithmetic sets up the offset so
> -         * that when the underlying get/set hooks call qdev_get_prop_ptr
> -         * they get the right answer despite the array element not actually
> -         * being inside the device struct.
> -         */
> -        arrayprop->prop.offset = eltptr - (void *)obj;
> -        assert(object_field_prop_ptr(obj, &arrayprop->prop) == eltptr);
> -        object_property_add(obj, propname,
> -                            arrayprop->prop.info->name,
> -                            field_prop_getter(arrayprop->prop.info),
> -                            field_prop_setter(arrayprop->prop.info),
> -                            array_element_release,
> -                            arrayprop);
> +static void get_prop_array(Object *obj, Visitor *v, const char *name,
> +                           void *opaque, Error **errp)
> +{
> +    ERRP_GUARD();
> +

Drop the blank line.

> +    Property *prop = opaque;
> +    uint32_t *alenptr = object_field_prop_ptr(obj, prop);
> +    void **arrayptr = (void *)obj + prop->arrayoffset;
> +    char *elem = *arrayptr;
> +    GenericList *list;
> +    const size_t list_elem_size = sizeof(*list) + prop->arrayfieldsize;
> +    int i;
> +
> +    if (!visit_start_list(v, name, &list, list_elem_size, errp)) {
> +        return;
>      }
> +
> +    for (i = 0; i < *alenptr; i++) {
> +        Property elem_prop = array_elem_prop(obj, prop, name, elem);
> +        prop->arrayinfo->get(obj, v, NULL, &elem_prop, errp);
> +        if (*errp) {
> +            goto out_obj;
> +        }
> +        elem += prop->arrayfieldsize;
> +    }
> +

You neglect to call visit_check_list().

> +out_obj:
> +    visit_end_list(v, (void**) &list);
>  }
>  
> -const PropertyInfo qdev_prop_arraylen = {
> -    .name = "uint32",
> -    .get = get_uint32,
> -    .set = set_prop_arraylen,
> -    .set_default_value = qdev_propinfo_set_default_value_uint,
> +static void default_prop_array(ObjectProperty *op, const Property *prop)
> +{
> +    object_property_set_default_list(op);
> +}
> +
> +const PropertyInfo qdev_prop_array = {
> +    .name = "list",
> +    .get = get_prop_array,
> +    .set = set_prop_array,
> +    .release = release_prop_array,
> +    .set_default_value = default_prop_array,
>  };
>  
>  /* --- public helpers --- */
> @@ -743,20 +797,8 @@ void qdev_prop_set_enum(DeviceState *dev, const char *name, int value)
>  
>  void qdev_prop_set_array(DeviceState *dev, const char *name, QList *values)
>  {
> -    const QListEntry *entry;
> -    g_autofree char *prop_len = g_strdup_printf("len-%s", name);
> -    uint32_t i = 0;
> -
> -    object_property_set_int(OBJECT(dev), prop_len, qlist_size(values),
> -                            &error_abort);
> -
> -    QLIST_FOREACH_ENTRY(values, entry) {
> -        g_autofree char *prop_idx = g_strdup_printf("%s[%u]", name, i);
> -        object_property_set_qobject(OBJECT(dev), prop_idx, entry->value,
> -                                    &error_abort);
> -        i++;
> -    }
> -
> +    object_property_set_qobject(OBJECT(dev), name, QOBJECT(values),
> +                                &error_abort);
>      qobject_unref(values);
>  }

I like this very much.

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

[Kevin Wolf]

Am 22.09.2023 um 17:05 hat Markus Armbruster geschrieben:
> Kevin Wolf <kwolf@redhat.com> writes:
> 
> > Until now, array properties are actually implemented with a hack that
> > uses multiple properties on the QOM level: a static "foo-len" property
> > and after it is set, dynamically created "foo[i]" properties.
> >
> > In external interfaces (-device on the command line and device_add in
> > QMP), this interface was broken by commit f3558b1b ('qdev: Base object
> > creation on QDict rather than QemuOpts') because QDicts are unordered
> > and therefore it could happen that QEMU tried to set the indexed
> > properties before setting the length, which fails and effectively makes
> > array properties inaccessible. In particular, this affects the 'ports'
> > property of the 'rocker' device.
> >
> > This patch reworks the external interface so that instead of using a
> > separate top-level property for the length and for each element, we use
> > a single true array property that accepts a list value. In the external
> > interfaces, this is naturally expressed as a JSON list and makes array
> > properties accessible again.
> >
> > Creating an array property on the command line without using JSON format
> > is currently not possible. This could be fixed by switching from
> > QemuOpts to a keyval parser, which however requires consideration of the
> > compatibility implications.
> >
> > All internal users of devices with array properties go through
> > qdev_prop_set_array() at this point, so updating it takes care of all of
> > them.
> >
> > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1090
> > Fixes: f3558b1b763683bb877f7dd5b282469cdadc65c3
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  include/hw/qdev-properties.h     |  23 ++--
> >  hw/core/qdev-properties-system.c |   2 +-
> >  hw/core/qdev-properties.c        | 204 +++++++++++++++++++------------
> >  3 files changed, 133 insertions(+), 96 deletions(-)
> >
> > diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
> > index 7fa2fdb7c9..9370b36b72 100644
> > --- a/include/hw/qdev-properties.h
> > +++ b/include/hw/qdev-properties.h
> > @@ -61,7 +61,7 @@ extern const PropertyInfo qdev_prop_size;
> >  extern const PropertyInfo qdev_prop_string;
> >  extern const PropertyInfo qdev_prop_on_off_auto;
> >  extern const PropertyInfo qdev_prop_size32;
> > -extern const PropertyInfo qdev_prop_arraylen;
> > +extern const PropertyInfo qdev_prop_array;
> >  extern const PropertyInfo qdev_prop_link;
> >  
> >  #define DEFINE_PROP(_name, _state, _field, _prop, _type, ...) {  \
> > @@ -115,8 +115,6 @@ extern const PropertyInfo qdev_prop_link;
> >                  .bitmask    = (_bitmask),                     \
> >                  .set_default = false)
> >  
> > -#define PROP_ARRAY_LEN_PREFIX "len-"
> > -
> >  /**
> >   * DEFINE_PROP_ARRAY:
> >   * @_name: name of the array
> > @@ -127,24 +125,21 @@ extern const PropertyInfo qdev_prop_link;
> >   * @_arrayprop: PropertyInfo defining what property the array elements have
> >   * @_arraytype: C type of the array elements
> >   *
> > - * Define device properties for a variable-length array _name.  A
> > - * static property "len-arrayname" is defined. When the device creator
> > - * sets this property to the desired length of array, further dynamic
> > - * properties "arrayname[0]", "arrayname[1]", ...  are defined so the
> > - * device creator can set the array element values. Setting the
> > - * "len-arrayname" property more than once is an error.
> > + * Define device properties for a variable-length array _name.  The array is
> 
> Please wrap comments around column 70.  More of the same below, not
> noted again.

My comments are consistent with the existing style of this file.

> > + * represented as a list in the visitor interface.
> > + *
> > + * @_arraytype is required to be movable with memcpy().
> >   *
> > - * When the array length is set, the @_field member of the device
> > + * When the array property is set, the @_field member of the device
> >   * struct is set to the array length, and @_arrayfield is set to point
> > - * to (zero-initialised) memory allocated for the array.  For a zero
> > - * length array, @_field will be set to 0 and @_arrayfield to NULL.
> > + * to the memory allocated for the array.
> 
> Worth mentioning that the @field member must be uint32_t?

It's actually already mentioned above in the description of the fields.

> > + *
> >   * It is the responsibility of the device deinit code to free the
> >   * @_arrayfield memory.
> >   */
> >  #define DEFINE_PROP_ARRAY(_name, _state, _field,               \
> >                            _arrayfield, _arrayprop, _arraytype) \
> > -    DEFINE_PROP((PROP_ARRAY_LEN_PREFIX _name),                 \
> > -                _state, _field, qdev_prop_arraylen, uint32_t,  \
> > +    DEFINE_PROP(_name, _state, _field, qdev_prop_array, uint32_t,     \
> >                  .set_default = true,                           \
> >                  .defval.u = 0,                                 \
> >                  .arrayinfo = &(_arrayprop),                    \
> 
> The backslashes are no longer aligned.
>
> > diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c
> > index 6d5d43eda2..f557ee886e 100644
> > --- a/hw/core/qdev-properties-system.c
> > +++ b/hw/core/qdev-properties-system.c
> > @@ -450,7 +450,7 @@ static void set_netdev(Object *obj, Visitor *v, const char *name,
> >      peers_ptr->queues = queues;
> >  
> >  out:
> > -    error_set_from_qdev_prop_error(errp, err, obj, name, str);
> > +    error_set_from_qdev_prop_error(errp, err, obj, prop->name, str);
> >      g_free(str);
> >  }
> >  
> 
> Intentional?

Yes, as I had explained to Peter. I'll make this a separate patch in v2
and explain in the commit message.

> > diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
> > index 950ef48e01..b2303a6fbc 100644
> > --- a/hw/core/qdev-properties.c
> > +++ b/hw/core/qdev-properties.c
> > @@ -546,98 +546,152 @@ const PropertyInfo qdev_prop_size32 = {
> >  
> >  /* --- support for array properties --- */
> >  
> > -/* Used as an opaque for the object properties we add for each
> > - * array element. Note that the struct Property must be first
> > - * in the struct so that a pointer to this works as the opaque
> > - * for the underlying element's property hooks as well as for
> > - * our own release callback.
> > - */
> > -typedef struct {
> > -    struct Property prop;
> > -    char *propname;
> > -    ObjectPropertyRelease *release;
> > -} ArrayElementProperty;
> > -
> > -/* object property release callback for array element properties:
> > - * we call the underlying element's property release hook, and
> > - * then free the memory we allocated when we added the property.
> > +static Property array_elem_prop(Object *obj, Property *parent_prop,
> > +                                const char *name, char *elem)
> 
> @parent_prop is an array property.  It's backed by an uint32_t length
> and an element array.  @elem points into the element array.  Correct?

Correct.

> > +{
> > +    return (Property) {
> > +        .info = parent_prop->arrayinfo,
> > +        .name = name,
> > +        /*
> > +         * This ugly piece of pointer arithmetic sets up the offset so
> > +         * that when the underlying release hook calls qdev_get_prop_ptr
> > +         * they get the right answer despite the array element not actually
> > +         * being inside the device struct.
> > +         */
> > +        .offset = elem - (char *) obj,
> 
> Isn't this is undefined behavior?

It should be at least less illegal than the old version of it, which did
the calculation on void * and still worked in practice...

But yes, strictly speaking, it's probably undefined behaviour. I can
calculate on uintptr_t instead, and then it should be defined here.

The QOM counterpart object_field_prop_ptr() is probably still undefined
because it calculates on a pointer and I think the spec allows casting
back to a pointer only after we've applied the offset so that we stay in
the same object with pointer arithmetics.

> Delete the space between (char *) and obj.
> 
> > +    };
> > +}
> > +
> > +/*
> > + * Object property release callback for array properties: We call the underlying
> > + * element's property release hook for each element.
> > + *
> > + * Note that it is the responsibility of the individual device's deinit to free
> > + * the array proper.
> 
> What is a device's "deinit"?  Is it the unrealize() method?  The
> instance_finalize() method?

Who knows? I only moved this comment.

My guess is that it doesn't really matter as long as _something_ frees
the array when unplugging the device.

> >   */
> > -static void array_element_release(Object *obj, const char *name, void *opaque)
> > +static void release_prop_array(Object *obj, const char *name, void *opaque)
> >  {
> > -    ArrayElementProperty *p = opaque;
> > -    if (p->release) {
> > -        p->release(obj, name, opaque);
> > +    Property *prop = opaque;
> > +    uint32_t *alenptr = object_field_prop_ptr(obj, prop);
> > +    void **arrayptr = (void *)obj + prop->arrayoffset;
> 
> I'd call these @plen and @pelts, but that's clearly a matter of taste.

I just kept the old names in set_prop_array() and used the same names in
new functions to stay consistent. But to be honest, @plen and @pelts
would be equally confusing to me.

My own choice would probably have been something like array_len and
array_data (if you want to know that it's a pointer, look at its type).

> > +    char *elem = *arrayptr;
> > +    int i;
> > +
> > +    for (i = 0; i < *alenptr; i++) {
> > +        Property elem_prop = array_elem_prop(obj, prop, name, elem);
> > +        prop->arrayinfo->release(obj, NULL, &elem_prop);
> > +        elem += prop->arrayfieldsize;
> >      }
> > -    g_free(p->propname);
> > -    g_free(p);
> >  }
> >  
> > -static void set_prop_arraylen(Object *obj, Visitor *v, const char *name,
> > -                              void *opaque, Error **errp)
> > +/*
> > + * Setter for an array property. This sets both the array length (which is
> > + * technically the property field in the object) and the array itself (a pointer
> > + * to which is stored in the additional field described by prop->arrayoffset).
> > + */
> > +static void set_prop_array(Object *obj, Visitor *v, const char *name,
> > +                           void *opaque, Error **errp)
> >  {
> > -    /* Setter for the property which defines the length of a
> > -     * variable-sized property array. As well as actually setting the
> > -     * array-length field in the device struct, we have to create the
> > -     * array itself and dynamically add the corresponding properties.
> > -     */
> > +    ERRP_GUARD();
> > +
> 
> Drop the blank line.
> 
> >      Property *prop = opaque;
> >      uint32_t *alenptr = object_field_prop_ptr(obj, prop);
> >      void **arrayptr = (void *)obj + prop->arrayoffset;
> > -    void *eltptr;
> > -    const char *arrayname;
> > -    int i;
> > +    GenericList *list, *elem, *next;
> > +    const size_t list_elem_size = sizeof(*list) + prop->arrayfieldsize;
> 
> This can be smaller than the size of the QAPI-generated list type, since
> the compiler may add padding.  Does it matter?

If it happens in practice, it does matter. Do we have any cleaner way to
get the element size without knowing the content of the list?

I expect that because GenericList only contains a single pointer, the
rest should have natural alignment and therefore the compiler shouldn't
have any reason to insert padding.

If you think this is not enough and there is no other way to get the
size of the list elements, we might have to generate packed structs for
the QAPI list types (which are really only two pointers, so not much to
lose when we do that).

> > +    char *elemptr;
> > +    bool ok = true;
> >  
> >      if (*alenptr) {
> >          error_setg(errp, "array size property %s may not be set more than once",
> >                     name);
> >          return;
> >      }
> > -    if (!visit_type_uint32(v, name, alenptr, errp)) {
> > +
> > +    if (!visit_start_list(v, name, &list, list_elem_size, errp)) {
> >          return;
> >      }
> > -    if (!*alenptr) {
> > +
> > +    /* Read the whole input into a temporary list */
> > +    elem = list;
> > +    while (elem) {
> > +        Property elem_prop = array_elem_prop(obj, prop, name, elem->padding);
> > +        prop->arrayinfo->set(obj, v, NULL, &elem_prop, errp);
> > +        if (*errp) {
> > +            ok = false;
> > +            goto out_obj;
> > +        }
> > +        (*alenptr)++;
> > +        elem = visit_next_list(v, elem, list_elem_size);
> > +    }
> > +
> > +    ok = visit_check_list(v, errp);
> > +out_obj:
> > +    visit_end_list(v, (void**) &list);
> > +
> > +    if (!ok) {
> > +        for (elem = list; elem; elem = next) {
> > +            next = elem->next;
> > +            g_free(elem);
> > +        }
> 
> We consume the list even on error.  It's too late in my day for me to
> see why that's proper.

Who else would free it otherwise?

This is pretty much the same as the generated list visitors do.

> >          return;
> >      }
> >  
> > -    /* DEFINE_PROP_ARRAY guarantees that name should start with this prefix;
> > -     * strip it off so we can get the name of the array itself.
> > +    /*
> > +     * Now that we know how big the array has to be, move the data over to a
> > +     * linear array and free the temporary list.
> >       */
> > -    assert(strncmp(name, PROP_ARRAY_LEN_PREFIX,
> > -                   strlen(PROP_ARRAY_LEN_PREFIX)) == 0);
> > -    arrayname = name + strlen(PROP_ARRAY_LEN_PREFIX);
> > +    *arrayptr = g_malloc_n(*alenptr, prop->arrayfieldsize);
> > +    elemptr = *arrayptr;
> > +    for (elem = list; elem; elem = next) {
> > +        memcpy(elemptr, elem->padding, prop->arrayfieldsize);
> > +        elemptr += prop->arrayfieldsize;
> > +        next = elem->next;
> > +        g_free(elem);
> > +    }
> > +}
> >  
> > -    /* Note that it is the responsibility of the individual device's deinit
> > -     * to free the array proper.
> > -     */
> > -    *arrayptr = eltptr = g_malloc0(*alenptr * prop->arrayfieldsize);
> > -    for (i = 0; i < *alenptr; i++, eltptr += prop->arrayfieldsize) {
> > -        char *propname = g_strdup_printf("%s[%d]", arrayname, i);
> > -        ArrayElementProperty *arrayprop = g_new0(ArrayElementProperty, 1);
> > -        arrayprop->release = prop->arrayinfo->release;
> > -        arrayprop->propname = propname;
> > -        arrayprop->prop.info = prop->arrayinfo;
> > -        arrayprop->prop.name = propname;
> > -        /* This ugly piece of pointer arithmetic sets up the offset so
> > -         * that when the underlying get/set hooks call qdev_get_prop_ptr
> > -         * they get the right answer despite the array element not actually
> > -         * being inside the device struct.
> > -         */
> > -        arrayprop->prop.offset = eltptr - (void *)obj;
> > -        assert(object_field_prop_ptr(obj, &arrayprop->prop) == eltptr);
> > -        object_property_add(obj, propname,
> > -                            arrayprop->prop.info->name,
> > -                            field_prop_getter(arrayprop->prop.info),
> > -                            field_prop_setter(arrayprop->prop.info),
> > -                            array_element_release,
> > -                            arrayprop);
> > +static void get_prop_array(Object *obj, Visitor *v, const char *name,
> > +                           void *opaque, Error **errp)
> > +{
> > +    ERRP_GUARD();
> > +
> 
> Drop the blank line.
> 
> > +    Property *prop = opaque;
> > +    uint32_t *alenptr = object_field_prop_ptr(obj, prop);
> > +    void **arrayptr = (void *)obj + prop->arrayoffset;
> > +    char *elem = *arrayptr;
> > +    GenericList *list;
> > +    const size_t list_elem_size = sizeof(*list) + prop->arrayfieldsize;
> > +    int i;
> > +
> > +    if (!visit_start_list(v, name, &list, list_elem_size, errp)) {
> > +        return;
> >      }
> > +
> > +    for (i = 0; i < *alenptr; i++) {
> > +        Property elem_prop = array_elem_prop(obj, prop, name, elem);
> > +        prop->arrayinfo->get(obj, v, NULL, &elem_prop, errp);
> > +        if (*errp) {
> > +            goto out_obj;
> > +        }
> > +        elem += prop->arrayfieldsize;
> > +    }
> > +
> 
> You neglect to call visit_check_list().

It is documented to be intended for input visitors only. Do we need it
with an output visitor?

> > +out_obj:
> > +    visit_end_list(v, (void**) &list);
> >  }
> >  
> > -const PropertyInfo qdev_prop_arraylen = {
> > -    .name = "uint32",
> > -    .get = get_uint32,
> > -    .set = set_prop_arraylen,
> > -    .set_default_value = qdev_propinfo_set_default_value_uint,
> > +static void default_prop_array(ObjectProperty *op, const Property *prop)
> > +{
> > +    object_property_set_default_list(op);
> > +}
> > +
> > +const PropertyInfo qdev_prop_array = {
> > +    .name = "list",
> > +    .get = get_prop_array,
> > +    .set = set_prop_array,
> > +    .release = release_prop_array,
> > +    .set_default_value = default_prop_array,
> >  };
> >  
> >  /* --- public helpers --- */
> > @@ -743,20 +797,8 @@ void qdev_prop_set_enum(DeviceState *dev, const char *name, int value)
> >  
> >  void qdev_prop_set_array(DeviceState *dev, const char *name, QList *values)
> >  {
> > -    const QListEntry *entry;
> > -    g_autofree char *prop_len = g_strdup_printf("len-%s", name);
> > -    uint32_t i = 0;
> > -
> > -    object_property_set_int(OBJECT(dev), prop_len, qlist_size(values),
> > -                            &error_abort);
> > -
> > -    QLIST_FOREACH_ENTRY(values, entry) {
> > -        g_autofree char *prop_idx = g_strdup_printf("%s[%u]", name, i);
> > -        object_property_set_qobject(OBJECT(dev), prop_idx, entry->value,
> > -                                    &error_abort);
> > -        i++;
> > -    }
> > -
> > +    object_property_set_qobject(OBJECT(dev), name, QOBJECT(values),
> > +                                &error_abort);
> >      qobject_unref(values);
> >  }
> 
> I like this very much.

Thanks for the review.

Kevin

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

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 22.09.2023 um 17:05 hat Markus Armbruster geschrieben:
>> Kevin Wolf <kwolf@redhat.com> writes:
>> 
>> > Until now, array properties are actually implemented with a hack that
>> > uses multiple properties on the QOM level: a static "foo-len" property
>> > and after it is set, dynamically created "foo[i]" properties.
>> >
>> > In external interfaces (-device on the command line and device_add in
>> > QMP), this interface was broken by commit f3558b1b ('qdev: Base object
>> > creation on QDict rather than QemuOpts') because QDicts are unordered
>> > and therefore it could happen that QEMU tried to set the indexed
>> > properties before setting the length, which fails and effectively makes
>> > array properties inaccessible. In particular, this affects the 'ports'
>> > property of the 'rocker' device.
>> >
>> > This patch reworks the external interface so that instead of using a
>> > separate top-level property for the length and for each element, we use
>> > a single true array property that accepts a list value. In the external
>> > interfaces, this is naturally expressed as a JSON list and makes array
>> > properties accessible again.
>> >
>> > Creating an array property on the command line without using JSON format
>> > is currently not possible. This could be fixed by switching from
>> > QemuOpts to a keyval parser, which however requires consideration of the
>> > compatibility implications.
>> >
>> > All internal users of devices with array properties go through
>> > qdev_prop_set_array() at this point, so updating it takes care of all of
>> > them.
>> >
>> > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1090
>> > Fixes: f3558b1b763683bb877f7dd5b282469cdadc65c3
>> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>

[...]

>> > diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
>> > index 950ef48e01..b2303a6fbc 100644
>> > --- a/hw/core/qdev-properties.c
>> > +++ b/hw/core/qdev-properties.c
>> > @@ -546,98 +546,152 @@ const PropertyInfo qdev_prop_size32 = {
>> >  
>> >  /* --- support for array properties --- */
>> >  
>> > -/* Used as an opaque for the object properties we add for each
>> > - * array element. Note that the struct Property must be first
>> > - * in the struct so that a pointer to this works as the opaque
>> > - * for the underlying element's property hooks as well as for
>> > - * our own release callback.
>> > - */
>> > -typedef struct {
>> > -    struct Property prop;
>> > -    char *propname;
>> > -    ObjectPropertyRelease *release;
>> > -} ArrayElementProperty;
>> > -
>> > -/* object property release callback for array element properties:
>> > - * we call the underlying element's property release hook, and
>> > - * then free the memory we allocated when we added the property.
>> > +static Property array_elem_prop(Object *obj, Property *parent_prop,
>> > +                                const char *name, char *elem)
>> 
>> @parent_prop is an array property.  It's backed by an uint32_t length
>> and an element array.  @elem points into the element array.  Correct?
>
> Correct.

Worth explaining in a comment?

>> > +{
>> > +    return (Property) {
>> > +        .info = parent_prop->arrayinfo,
>> > +        .name = name,
>> > +        /*
>> > +         * This ugly piece of pointer arithmetic sets up the offset so
>> > +         * that when the underlying release hook calls qdev_get_prop_ptr
>> > +         * they get the right answer despite the array element not actually
>> > +         * being inside the device struct.
>> > +         */
>> > +        .offset = elem - (char *) obj,
>> 
>> Isn't this is undefined behavior?
>
> It should be at least less illegal than the old version of it, which did
> the calculation on void * and still worked in practice...
>
> But yes, strictly speaking, it's probably undefined behaviour. I can
> calculate on uintptr_t instead, and then it should be defined here.
>
> The QOM counterpart object_field_prop_ptr() is probably still undefined
> because it calculates on a pointer and I think the spec allows casting
> back to a pointer only after we've applied the offset so that we stay in
> the same object with pointer arithmetics.

We should not have to waste time on worrying about compilers using UB
fine print against us, but sadly we do.

I'm not objecting to your code, I'm merely pointing out a potential time
bomb.  In a programming environment that has embraced time bombing with
gusto.

>> Delete the space between (char *) and obj.
>> 
>> > +    };
>> > +}
>> > +
>> > +/*
>> > + * Object property release callback for array properties: We call the underlying
>> > + * element's property release hook for each element.
>> > + *
>> > + * Note that it is the responsibility of the individual device's deinit to free
>> > + * the array proper.
>> 
>> What is a device's "deinit"?  Is it the unrealize() method?  The
>> instance_finalize() method?
>
> Who knows? I only moved this comment.

Opportunity to improve it.  Not a demand.

> My guess is that it doesn't really matter as long as _something_ frees
> the array when unplugging the device.
>
>> >   */
>> > -static void array_element_release(Object *obj, const char *name, void *opaque)
>> > +static void release_prop_array(Object *obj, const char *name, void *opaque)
>> >  {
>> > -    ArrayElementProperty *p = opaque;
>> > -    if (p->release) {
>> > -        p->release(obj, name, opaque);
>> > +    Property *prop = opaque;
>> > +    uint32_t *alenptr = object_field_prop_ptr(obj, prop);
>> > +    void **arrayptr = (void *)obj + prop->arrayoffset;
>> 
>> I'd call these @plen and @pelts, but that's clearly a matter of taste.
>
> I just kept the old names in set_prop_array() and used the same names in
> new functions to stay consistent. But to be honest, @plen and @pelts
> would be equally confusing to me.
>
> My own choice would probably have been something like array_len and
> array_data (if you want to know that it's a pointer, look at its type).
>
>> > +    char *elem = *arrayptr;
>> > +    int i;
>> > +
>> > +    for (i = 0; i < *alenptr; i++) {
>> > +        Property elem_prop = array_elem_prop(obj, prop, name, elem);
>> > +        prop->arrayinfo->release(obj, NULL, &elem_prop);
>> > +        elem += prop->arrayfieldsize;
>> >      }
>> > -    g_free(p->propname);
>> > -    g_free(p);
>> >  }
>> >  
>> > -static void set_prop_arraylen(Object *obj, Visitor *v, const char *name,
>> > -                              void *opaque, Error **errp)
>> > +/*
>> > + * Setter for an array property. This sets both the array length (which is
>> > + * technically the property field in the object) and the array itself (a pointer
>> > + * to which is stored in the additional field described by prop->arrayoffset).
>> > + */
>> > +static void set_prop_array(Object *obj, Visitor *v, const char *name,
>> > +                           void *opaque, Error **errp)
>> >  {
>> > -    /* Setter for the property which defines the length of a
>> > -     * variable-sized property array. As well as actually setting the
>> > -     * array-length field in the device struct, we have to create the
>> > -     * array itself and dynamically add the corresponding properties.
>> > -     */
>> > +    ERRP_GUARD();
>> > +
>> 
>> Drop the blank line.
>> 
>> >      Property *prop = opaque;
>> >      uint32_t *alenptr = object_field_prop_ptr(obj, prop);
>> >      void **arrayptr = (void *)obj + prop->arrayoffset;
>> > -    void *eltptr;
>> > -    const char *arrayname;
>> > -    int i;
>> > +    GenericList *list, *elem, *next;
>> > +    const size_t list_elem_size = sizeof(*list) + prop->arrayfieldsize;
>> 
>> This can be smaller than the size of the QAPI-generated list type, since
>> the compiler may add padding.  Does it matter?
>
> If it happens in practice, it does matter. Do we have any cleaner way to
> get the element size without knowing the content of the list?
>
> I expect that because GenericList only contains a single pointer, the
> rest should have natural alignment

Yes, GenericList's size and alignment should match a pointer's:

    typedef struct GenericList {
        struct GenericList *next;
        char padding[];
    } GenericList;

>                                    and therefore the compiler shouldn't
> have any reason to insert padding.

The actual list will look like

    struct FOOList {
        FOOList *next;
        FOOTYPE value;
    }

where FOOTYPE is some QAPI-generated type.  No padding as long as
FOOTYPE's alignment divides the pointer size.  I figure that's true for
our current targets and generated QAPI types (currently pointers,
double, bool, or integers up to 64 bits).

> If you think this is not enough and there is no other way to get the
> size of the list elements, we might have to generate packed structs for
> the QAPI list types (which are really only two pointers, so not much to
> lose when we do that).

Could we assert the element type's alignment divides GenericList's size?
Not here, obviously, but in DEFINE_PROP_ARRAY(), where we can use
__alignof__(_arraytype).

We could also play with attribute aligned to ensure GenericList's size is
safe, but I doubt that's worthwhile.

>> > +    char *elemptr;
>> > +    bool ok = true;
>> >  
>> >      if (*alenptr) {
>> >          error_setg(errp, "array size property %s may not be set more than once",
>> >                     name);
>> >          return;
>> >      }
>> > -    if (!visit_type_uint32(v, name, alenptr, errp)) {
>> > +
>> > +    if (!visit_start_list(v, name, &list, list_elem_size, errp)) {
>> >          return;
>> >      }
>> > -    if (!*alenptr) {
>> > +
>> > +    /* Read the whole input into a temporary list */
>> > +    elem = list;
>> > +    while (elem) {
>> > +        Property elem_prop = array_elem_prop(obj, prop, name, elem->padding);
>> > +        prop->arrayinfo->set(obj, v, NULL, &elem_prop, errp);
>> > +        if (*errp) {
>> > +            ok = false;
>> > +            goto out_obj;
>> > +        }
>> > +        (*alenptr)++;
>> > +        elem = visit_next_list(v, elem, list_elem_size);
>> > +    }
>> > +
>> > +    ok = visit_check_list(v, errp);
>> > +out_obj:
>> > +    visit_end_list(v, (void**) &list);
>> > +
>> > +    if (!ok) {
>> > +        for (elem = list; elem; elem = next) {
>> > +            next = elem->next;
>> > +            g_free(elem);
>> > +        }
>> 
>> We consume the list even on error.  It's too late in my day for me to
>> see why that's proper.
>
> Who else would free it otherwise?
>
> This is pretty much the same as the generated list visitors do.

Help me out: point me to the precedence you have in mind.

>> >          return;
>> >      }
>> >  
>> > -    /* DEFINE_PROP_ARRAY guarantees that name should start with this prefix;
>> > -     * strip it off so we can get the name of the array itself.
>> > +    /*
>> > +     * Now that we know how big the array has to be, move the data over to a
>> > +     * linear array and free the temporary list.
>> >       */
>> > -    assert(strncmp(name, PROP_ARRAY_LEN_PREFIX,
>> > -                   strlen(PROP_ARRAY_LEN_PREFIX)) == 0);
>> > -    arrayname = name + strlen(PROP_ARRAY_LEN_PREFIX);
>> > +    *arrayptr = g_malloc_n(*alenptr, prop->arrayfieldsize);
>> > +    elemptr = *arrayptr;
>> > +    for (elem = list; elem; elem = next) {
>> > +        memcpy(elemptr, elem->padding, prop->arrayfieldsize);
>> > +        elemptr += prop->arrayfieldsize;
>> > +        next = elem->next;
>> > +        g_free(elem);
>> > +    }
>> > +}
>> >  
>> > -    /* Note that it is the responsibility of the individual device's deinit
>> > -     * to free the array proper.
>> > -     */
>> > -    *arrayptr = eltptr = g_malloc0(*alenptr * prop->arrayfieldsize);
>> > -    for (i = 0; i < *alenptr; i++, eltptr += prop->arrayfieldsize) {
>> > -        char *propname = g_strdup_printf("%s[%d]", arrayname, i);
>> > -        ArrayElementProperty *arrayprop = g_new0(ArrayElementProperty, 1);
>> > -        arrayprop->release = prop->arrayinfo->release;
>> > -        arrayprop->propname = propname;
>> > -        arrayprop->prop.info = prop->arrayinfo;
>> > -        arrayprop->prop.name = propname;
>> > -        /* This ugly piece of pointer arithmetic sets up the offset so
>> > -         * that when the underlying get/set hooks call qdev_get_prop_ptr
>> > -         * they get the right answer despite the array element not actually
>> > -         * being inside the device struct.
>> > -         */
>> > -        arrayprop->prop.offset = eltptr - (void *)obj;
>> > -        assert(object_field_prop_ptr(obj, &arrayprop->prop) == eltptr);
>> > -        object_property_add(obj, propname,
>> > -                            arrayprop->prop.info->name,
>> > -                            field_prop_getter(arrayprop->prop.info),
>> > -                            field_prop_setter(arrayprop->prop.info),
>> > -                            array_element_release,
>> > -                            arrayprop);
>> > +static void get_prop_array(Object *obj, Visitor *v, const char *name,
>> > +                           void *opaque, Error **errp)
>> > +{
>> > +    ERRP_GUARD();
>> > +
>> 
>> Drop the blank line.
>> 
>> > +    Property *prop = opaque;
>> > +    uint32_t *alenptr = object_field_prop_ptr(obj, prop);
>> > +    void **arrayptr = (void *)obj + prop->arrayoffset;
>> > +    char *elem = *arrayptr;
>> > +    GenericList *list;
>> > +    const size_t list_elem_size = sizeof(*list) + prop->arrayfieldsize;
>> > +    int i;
>> > +
>> > +    if (!visit_start_list(v, name, &list, list_elem_size, errp)) {
>> > +        return;
>> >      }
>> > +
>> > +    for (i = 0; i < *alenptr; i++) {
>> > +        Property elem_prop = array_elem_prop(obj, prop, name, elem);
>> > +        prop->arrayinfo->get(obj, v, NULL, &elem_prop, errp);
>> > +        if (*errp) {
>> > +            goto out_obj;
>> > +        }
>> > +        elem += prop->arrayfieldsize;
>> > +    }
>> > +
>> 
>> You neglect to call visit_check_list().
>
> It is documented to be intended for input visitors only. Do we need it
> with an output visitor?

Help me out: where is that documented?

>> > +out_obj:
>> > +    visit_end_list(v, (void**) &list);
>> >  }
>> >  
>> > -const PropertyInfo qdev_prop_arraylen = {
>> > -    .name = "uint32",
>> > -    .get = get_uint32,
>> > -    .set = set_prop_arraylen,
>> > -    .set_default_value = qdev_propinfo_set_default_value_uint,
>> > +static void default_prop_array(ObjectProperty *op, const Property *prop)
>> > +{
>> > +    object_property_set_default_list(op);
>> > +}
>> > +
>> > +const PropertyInfo qdev_prop_array = {
>> > +    .name = "list",
>> > +    .get = get_prop_array,
>> > +    .set = set_prop_array,
>> > +    .release = release_prop_array,
>> > +    .set_default_value = default_prop_array,
>> >  };
>> >  
>> >  /* --- public helpers --- */
>> > @@ -743,20 +797,8 @@ void qdev_prop_set_enum(DeviceState *dev, const char *name, int value)
>> >  
>> >  void qdev_prop_set_array(DeviceState *dev, const char *name, QList *values)
>> >  {
>> > -    const QListEntry *entry;
>> > -    g_autofree char *prop_len = g_strdup_printf("len-%s", name);
>> > -    uint32_t i = 0;
>> > -
>> > -    object_property_set_int(OBJECT(dev), prop_len, qlist_size(values),
>> > -                            &error_abort);
>> > -
>> > -    QLIST_FOREACH_ENTRY(values, entry) {
>> > -        g_autofree char *prop_idx = g_strdup_printf("%s[%u]", name, i);
>> > -        object_property_set_qobject(OBJECT(dev), prop_idx, entry->value,
>> > -                                    &error_abort);
>> > -        i++;
>> > -    }
>> > -
>> > +    object_property_set_qobject(OBJECT(dev), name, QOBJECT(values),
>> > +                                &error_abort);
>> >      qobject_unref(values);
>> >  }
>> 
>> I like this very much.
>
> Thanks for the review.

You're welcome.

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

[Kevin Wolf]

Am 14.10.2023 um 08:36 hat Markus Armbruster geschrieben:
> Kevin Wolf <kwolf@redhat.com> writes:
> 
> > Am 22.09.2023 um 17:05 hat Markus Armbruster geschrieben:
> >> Kevin Wolf <kwolf@redhat.com> writes:
> >> 
> >> > Until now, array properties are actually implemented with a hack that
> >> > uses multiple properties on the QOM level: a static "foo-len" property
> >> > and after it is set, dynamically created "foo[i]" properties.
> >> >
> >> > In external interfaces (-device on the command line and device_add in
> >> > QMP), this interface was broken by commit f3558b1b ('qdev: Base object
> >> > creation on QDict rather than QemuOpts') because QDicts are unordered
> >> > and therefore it could happen that QEMU tried to set the indexed
> >> > properties before setting the length, which fails and effectively makes
> >> > array properties inaccessible. In particular, this affects the 'ports'
> >> > property of the 'rocker' device.
> >> >
> >> > This patch reworks the external interface so that instead of using a
> >> > separate top-level property for the length and for each element, we use
> >> > a single true array property that accepts a list value. In the external
> >> > interfaces, this is naturally expressed as a JSON list and makes array
> >> > properties accessible again.
> >> >
> >> > Creating an array property on the command line without using JSON format
> >> > is currently not possible. This could be fixed by switching from
> >> > QemuOpts to a keyval parser, which however requires consideration of the
> >> > compatibility implications.
> >> >
> >> > All internal users of devices with array properties go through
> >> > qdev_prop_set_array() at this point, so updating it takes care of all of
> >> > them.
> >> >
> >> > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1090
> >> > Fixes: f3558b1b763683bb877f7dd5b282469cdadc65c3
> >> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>

> >> > +{
> >> > +    return (Property) {
> >> > +        .info = parent_prop->arrayinfo,
> >> > +        .name = name,
> >> > +        /*
> >> > +         * This ugly piece of pointer arithmetic sets up the offset so
> >> > +         * that when the underlying release hook calls qdev_get_prop_ptr
> >> > +         * they get the right answer despite the array element not actually
> >> > +         * being inside the device struct.
> >> > +         */
> >> > +        .offset = elem - (char *) obj,
> >> 
> >> Isn't this is undefined behavior?
> >
> > It should be at least less illegal than the old version of it, which did
> > the calculation on void * and still worked in practice...
> >
> > But yes, strictly speaking, it's probably undefined behaviour. I can
> > calculate on uintptr_t instead, and then it should be defined here.
> >
> > The QOM counterpart object_field_prop_ptr() is probably still undefined
> > because it calculates on a pointer and I think the spec allows casting
> > back to a pointer only after we've applied the offset so that we stay in
> > the same object with pointer arithmetics.
> 
> We should not have to waste time on worrying about compilers using UB
> fine print against us, but sadly we do.
> 
> I'm not objecting to your code, I'm merely pointing out a potential time
> bomb.  In a programming environment that has embraced time bombing with
> gusto.

While I'm touching this part, I'll change it to uintptr_t. I'll leave
the counterpart for later.

> >> Delete the space between (char *) and obj.
> >> 
> >> > +    };
> >> > +}
> >> > +
> >> > +/*
> >> > + * Object property release callback for array properties: We call the underlying
> >> > + * element's property release hook for each element.
> >> > + *
> >> > + * Note that it is the responsibility of the individual device's deinit to free
> >> > + * the array proper.
> >> 
> >> What is a device's "deinit"?  Is it the unrealize() method?  The
> >> instance_finalize() method?
> >
> > Who knows? I only moved this comment.
> 
> Opportunity to improve it.  Not a demand.

If you have a better version of it? My guess is only a guess, so I'd
avoid putting it in the code without understanding if there are reasons
why it has to be a specific place, or why a specific place doesn't work.

Leaving it vague is probably better than being specific, but potentially
wrong.

> > My guess is that it doesn't really matter as long as _something_ frees
> > the array when unplugging the device.
> >
> >> >   */
> >> > -static void array_element_release(Object *obj, const char *name, void *opaque)
> >> > +static void release_prop_array(Object *obj, const char *name, void *opaque)
> >> >  {
> >> > -    ArrayElementProperty *p = opaque;
> >> > -    if (p->release) {
> >> > -        p->release(obj, name, opaque);
> >> > +    Property *prop = opaque;
> >> > +    uint32_t *alenptr = object_field_prop_ptr(obj, prop);
> >> > +    void **arrayptr = (void *)obj + prop->arrayoffset;
> >> 
> >> I'd call these @plen and @pelts, but that's clearly a matter of taste.
> >
> > I just kept the old names in set_prop_array() and used the same names in
> > new functions to stay consistent. But to be honest, @plen and @pelts
> > would be equally confusing to me.
> >
> > My own choice would probably have been something like array_len and
> > array_data (if you want to know that it's a pointer, look at its type).
> >
> >> > +    char *elem = *arrayptr;
> >> > +    int i;
> >> > +
> >> > +    for (i = 0; i < *alenptr; i++) {
> >> > +        Property elem_prop = array_elem_prop(obj, prop, name, elem);
> >> > +        prop->arrayinfo->release(obj, NULL, &elem_prop);
> >> > +        elem += prop->arrayfieldsize;
> >> >      }
> >> > -    g_free(p->propname);
> >> > -    g_free(p);
> >> >  }
> >> >  
> >> > -static void set_prop_arraylen(Object *obj, Visitor *v, const char *name,
> >> > -                              void *opaque, Error **errp)
> >> > +/*
> >> > + * Setter for an array property. This sets both the array length (which is
> >> > + * technically the property field in the object) and the array itself (a pointer
> >> > + * to which is stored in the additional field described by prop->arrayoffset).
> >> > + */
> >> > +static void set_prop_array(Object *obj, Visitor *v, const char *name,
> >> > +                           void *opaque, Error **errp)
> >> >  {
> >> > -    /* Setter for the property which defines the length of a
> >> > -     * variable-sized property array. As well as actually setting the
> >> > -     * array-length field in the device struct, we have to create the
> >> > -     * array itself and dynamically add the corresponding properties.
> >> > -     */
> >> > +    ERRP_GUARD();
> >> > +
> >> 
> >> Drop the blank line.
> >> 
> >> >      Property *prop = opaque;
> >> >      uint32_t *alenptr = object_field_prop_ptr(obj, prop);
> >> >      void **arrayptr = (void *)obj + prop->arrayoffset;
> >> > -    void *eltptr;
> >> > -    const char *arrayname;
> >> > -    int i;
> >> > +    GenericList *list, *elem, *next;
> >> > +    const size_t list_elem_size = sizeof(*list) + prop->arrayfieldsize;
> >> 
> >> This can be smaller than the size of the QAPI-generated list type, since
> >> the compiler may add padding.  Does it matter?
> >
> > If it happens in practice, it does matter. Do we have any cleaner way to
> > get the element size without knowing the content of the list?
> >
> > I expect that because GenericList only contains a single pointer, the
> > rest should have natural alignment
> 
> Yes, GenericList's size and alignment should match a pointer's:
> 
>     typedef struct GenericList {
>         struct GenericList *next;
>         char padding[];
>     } GenericList;
> 
> >                                    and therefore the compiler shouldn't
> > have any reason to insert padding.
> 
> The actual list will look like
> 
>     struct FOOList {
>         FOOList *next;
>         FOOTYPE value;
>     }
> 
> where FOOTYPE is some QAPI-generated type.  No padding as long as
> FOOTYPE's alignment divides the pointer size.  I figure that's true for
> our current targets and generated QAPI types (currently pointers,
> double, bool, or integers up to 64 bits).

I'm quite confused about the whole alignment stuff at the moment, but
after looking some more at the code, I think I just realised one
important thing: We're actually not dealing with QAPI-generated types
here, but with custom visitors in QOM property setters. The netdev one
specifically (which is used in the array property of the 'rocker'
device) uses NICPeers (not a pointer to NICPeers!), which is much
larger.

But now I'm wondering, FOOList doesn't even exist as a type. We're
calculating list_elem_size only to pass it to visit_start_list(), which
doesn't have FOOList either. What does the visitor do with it?

It seems it doesn't do anything with it except using it as the size to
malloc() elements. And then we (the array property code in this patch)
use &elem->padding as the element pointer, directly from GenericList,
not any specific FOOList. So I think this just means that we never
insert any padding and what the compiler would do with a hypothetical
FOOList, which doesn't even have to exist as a type, doesn't matter.

Am I missing something or does this mean that the code is actually fine?

> > If you think this is not enough and there is no other way to get the
> > size of the list elements, we might have to generate packed structs for
> > the QAPI list types (which are really only two pointers, so not much to
> > lose when we do that).
> 
> Could we assert the element type's alignment divides GenericList's size?
> Not here, obviously, but in DEFINE_PROP_ARRAY(), where we can use
> __alignof__(_arraytype).

A bit tricky because we're in the middle of a literal, but I suppose
with the GCC extension we could change one random element of the struct
to something like:

    ({ QEMU_BUILD_BUG_ON(...); real_value; })

Of course, if my thoughts above were right, this might not actually be
needed.

> We could also play with attribute aligned to ensure GenericList's size is
> safe, but I doubt that's worthwhile.
> 
> >> > +    char *elemptr;
> >> > +    bool ok = true;
> >> >  
> >> >      if (*alenptr) {
> >> >          error_setg(errp, "array size property %s may not be set more than once",
> >> >                     name);
> >> >          return;
> >> >      }
> >> > -    if (!visit_type_uint32(v, name, alenptr, errp)) {
> >> > +
> >> > +    if (!visit_start_list(v, name, &list, list_elem_size, errp)) {
> >> >          return;
> >> >      }
> >> > -    if (!*alenptr) {
> >> > +
> >> > +    /* Read the whole input into a temporary list */
> >> > +    elem = list;
> >> > +    while (elem) {
> >> > +        Property elem_prop = array_elem_prop(obj, prop, name, elem->padding);
> >> > +        prop->arrayinfo->set(obj, v, NULL, &elem_prop, errp);
> >> > +        if (*errp) {
> >> > +            ok = false;
> >> > +            goto out_obj;
> >> > +        }
> >> > +        (*alenptr)++;
> >> > +        elem = visit_next_list(v, elem, list_elem_size);
> >> > +    }
> >> > +
> >> > +    ok = visit_check_list(v, errp);
> >> > +out_obj:
> >> > +    visit_end_list(v, (void**) &list);
> >> > +
> >> > +    if (!ok) {
> >> > +        for (elem = list; elem; elem = next) {
> >> > +            next = elem->next;
> >> > +            g_free(elem);
> >> > +        }
> >> 
> >> We consume the list even on error.  It's too late in my day for me to
> >> see why that's proper.
> >
> > Who else would free it otherwise?
> >
> > This is pretty much the same as the generated list visitors do.
> 
> Help me out: point me to the precedence you have in mind.

Any QAPI generated list visiting function should do. As an example, I'm
looking at visit_type_Qcow2BitmapInfoList():

    ...
    ok = visit_check_list(v, errp);
out_obj:
    visit_end_list(v, (void **)obj);
    if (!ok && visit_is_input(v)) {
        qapi_free_Qcow2BitmapInfoList(*obj);
        *obj = NULL;
    }
    return ok;
}

On failure, it calls qapi_free_Qcow2BitmapInfoList().

What's probably wrong in my code is that to be a full equivalent, it
also needs to free things behind pointers that are owned by elem, i.e.
call prop->arrayinfo->release().

In practice, it doesn't currently make a difference because none of the
types used with array properties actually have a release callback.
Should still be fixed, of course.

> >> >          return;
> >> >      }
> >> >  
> >> > -    /* DEFINE_PROP_ARRAY guarantees that name should start with this prefix;
> >> > -     * strip it off so we can get the name of the array itself.
> >> > +    /*
> >> > +     * Now that we know how big the array has to be, move the data over to a
> >> > +     * linear array and free the temporary list.
> >> >       */
> >> > -    assert(strncmp(name, PROP_ARRAY_LEN_PREFIX,
> >> > -                   strlen(PROP_ARRAY_LEN_PREFIX)) == 0);
> >> > -    arrayname = name + strlen(PROP_ARRAY_LEN_PREFIX);
> >> > +    *arrayptr = g_malloc_n(*alenptr, prop->arrayfieldsize);
> >> > +    elemptr = *arrayptr;
> >> > +    for (elem = list; elem; elem = next) {
> >> > +        memcpy(elemptr, elem->padding, prop->arrayfieldsize);
> >> > +        elemptr += prop->arrayfieldsize;
> >> > +        next = elem->next;
> >> > +        g_free(elem);
> >> > +    }
> >> > +}
> >> >  
> >> > -    /* Note that it is the responsibility of the individual device's deinit
> >> > -     * to free the array proper.
> >> > -     */
> >> > -    *arrayptr = eltptr = g_malloc0(*alenptr * prop->arrayfieldsize);
> >> > -    for (i = 0; i < *alenptr; i++, eltptr += prop->arrayfieldsize) {
> >> > -        char *propname = g_strdup_printf("%s[%d]", arrayname, i);
> >> > -        ArrayElementProperty *arrayprop = g_new0(ArrayElementProperty, 1);
> >> > -        arrayprop->release = prop->arrayinfo->release;
> >> > -        arrayprop->propname = propname;
> >> > -        arrayprop->prop.info = prop->arrayinfo;
> >> > -        arrayprop->prop.name = propname;
> >> > -        /* This ugly piece of pointer arithmetic sets up the offset so
> >> > -         * that when the underlying get/set hooks call qdev_get_prop_ptr
> >> > -         * they get the right answer despite the array element not actually
> >> > -         * being inside the device struct.
> >> > -         */
> >> > -        arrayprop->prop.offset = eltptr - (void *)obj;
> >> > -        assert(object_field_prop_ptr(obj, &arrayprop->prop) == eltptr);
> >> > -        object_property_add(obj, propname,
> >> > -                            arrayprop->prop.info->name,
> >> > -                            field_prop_getter(arrayprop->prop.info),
> >> > -                            field_prop_setter(arrayprop->prop.info),
> >> > -                            array_element_release,
> >> > -                            arrayprop);
> >> > +static void get_prop_array(Object *obj, Visitor *v, const char *name,
> >> > +                           void *opaque, Error **errp)
> >> > +{
> >> > +    ERRP_GUARD();
> >> > +
> >> 
> >> Drop the blank line.
> >> 
> >> > +    Property *prop = opaque;
> >> > +    uint32_t *alenptr = object_field_prop_ptr(obj, prop);
> >> > +    void **arrayptr = (void *)obj + prop->arrayoffset;
> >> > +    char *elem = *arrayptr;
> >> > +    GenericList *list;
> >> > +    const size_t list_elem_size = sizeof(*list) + prop->arrayfieldsize;
> >> > +    int i;
> >> > +
> >> > +    if (!visit_start_list(v, name, &list, list_elem_size, errp)) {
> >> > +        return;
> >> >      }
> >> > +
> >> > +    for (i = 0; i < *alenptr; i++) {
> >> > +        Property elem_prop = array_elem_prop(obj, prop, name, elem);
> >> > +        prop->arrayinfo->get(obj, v, NULL, &elem_prop, errp);
> >> > +        if (*errp) {
> >> > +            goto out_obj;
> >> > +        }
> >> > +        elem += prop->arrayfieldsize;
> >> > +    }
> >> > +
> >> 
> >> You neglect to call visit_check_list().
> >
> > It is documented to be intended for input visitors only. Do we need it
> > with an output visitor?
> 
> Help me out: where is that documented?

In struct Visitor:

    /* Optional; intended for input visitors */
    bool (*check_list)(Visitor *v, Error **errp);

And indeed, the existing output vistors don't implement it.

Admittedly, the public visit_check_list() is less clear:

    /*
     * Prepare for completing a list visit.
     *
     * On failure, store an error through @errp.  Can happen only when @v
     * is an input visitor.
     *
     * Return true on success, false on failure.
     *
     * Should be called prior to visit_end_list() if all other
     * intermediate visit steps were successful, to allow the visitor one
     * last chance to report errors.  May be skipped on a cleanup path,
     * where there is no need to check for further errors.
     */
    bool visit_check_list(Visitor *v, Error **errp);

If you prefer, I can add it here just to be sure. Instead of having real
error handling, assert(ok) should be enough.

> >> > +out_obj:
> >> > +    visit_end_list(v, (void**) &list);
> >> >  }

Kevin

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

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 14.10.2023 um 08:36 hat Markus Armbruster geschrieben:
>> Kevin Wolf <kwolf@redhat.com> writes:
>> 
>> > Am 22.09.2023 um 17:05 hat Markus Armbruster geschrieben:
>> >> Kevin Wolf <kwolf@redhat.com> writes:
>> >> 
>> >> > Until now, array properties are actually implemented with a hack that
>> >> > uses multiple properties on the QOM level: a static "foo-len" property
>> >> > and after it is set, dynamically created "foo[i]" properties.
>> >> >
>> >> > In external interfaces (-device on the command line and device_add in
>> >> > QMP), this interface was broken by commit f3558b1b ('qdev: Base object
>> >> > creation on QDict rather than QemuOpts') because QDicts are unordered
>> >> > and therefore it could happen that QEMU tried to set the indexed
>> >> > properties before setting the length, which fails and effectively makes
>> >> > array properties inaccessible. In particular, this affects the 'ports'
>> >> > property of the 'rocker' device.
>> >> >
>> >> > This patch reworks the external interface so that instead of using a
>> >> > separate top-level property for the length and for each element, we use
>> >> > a single true array property that accepts a list value. In the external
>> >> > interfaces, this is naturally expressed as a JSON list and makes array
>> >> > properties accessible again.
>> >> >
>> >> > Creating an array property on the command line without using JSON format
>> >> > is currently not possible. This could be fixed by switching from
>> >> > QemuOpts to a keyval parser, which however requires consideration of the
>> >> > compatibility implications.
>> >> >
>> >> > All internal users of devices with array properties go through
>> >> > qdev_prop_set_array() at this point, so updating it takes care of all of
>> >> > them.
>> >> >
>> >> > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1090
>> >> > Fixes: f3558b1b763683bb877f7dd5b282469cdadc65c3
>> >> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>
>> >> > +{
>> >> > +    return (Property) {
>> >> > +        .info = parent_prop->arrayinfo,
>> >> > +        .name = name,
>> >> > +        /*
>> >> > +         * This ugly piece of pointer arithmetic sets up the offset so
>> >> > +         * that when the underlying release hook calls qdev_get_prop_ptr
>> >> > +         * they get the right answer despite the array element not actually
>> >> > +         * being inside the device struct.
>> >> > +         */
>> >> > +        .offset = elem - (char *) obj,
>> >> 
>> >> Isn't this is undefined behavior?
>> >
>> > It should be at least less illegal than the old version of it, which did
>> > the calculation on void * and still worked in practice...
>> >
>> > But yes, strictly speaking, it's probably undefined behaviour. I can
>> > calculate on uintptr_t instead, and then it should be defined here.
>> >
>> > The QOM counterpart object_field_prop_ptr() is probably still undefined
>> > because it calculates on a pointer and I think the spec allows casting
>> > back to a pointer only after we've applied the offset so that we stay in
>> > the same object with pointer arithmetics.
>> 
>> We should not have to waste time on worrying about compilers using UB
>> fine print against us, but sadly we do.
>> 
>> I'm not objecting to your code, I'm merely pointing out a potential time
>> bomb.  In a programming environment that has embraced time bombing with
>> gusto.
>
> While I'm touching this part, I'll change it to uintptr_t. I'll leave
> the counterpart for later.
>
>> >> Delete the space between (char *) and obj.
>> >> 
>> >> > +    };
>> >> > +}
>> >> > +
>> >> > +/*
>> >> > + * Object property release callback for array properties: We call the underlying
>> >> > + * element's property release hook for each element.
>> >> > + *
>> >> > + * Note that it is the responsibility of the individual device's deinit to free
>> >> > + * the array proper.
>> >> 
>> >> What is a device's "deinit"?  Is it the unrealize() method?  The
>> >> instance_finalize() method?
>> >
>> > Who knows? I only moved this comment.
>> 
>> Opportunity to improve it.  Not a demand.
>
> If you have a better version of it? My guess is only a guess, so I'd
> avoid putting it in the code without understanding if there are reasons
> why it has to be a specific place, or why a specific place doesn't work.
>
> Leaving it vague is probably better than being specific, but potentially
> wrong.

Fair.

>> > My guess is that it doesn't really matter as long as _something_ frees
>> > the array when unplugging the device.
>> >
>> >> >   */
>> >> > -static void array_element_release(Object *obj, const char *name, void *opaque)
>> >> > +static void release_prop_array(Object *obj, const char *name, void *opaque)
>> >> >  {
>> >> > -    ArrayElementProperty *p = opaque;
>> >> > -    if (p->release) {
>> >> > -        p->release(obj, name, opaque);
>> >> > +    Property *prop = opaque;
>> >> > +    uint32_t *alenptr = object_field_prop_ptr(obj, prop);
>> >> > +    void **arrayptr = (void *)obj + prop->arrayoffset;
>> >> 
>> >> I'd call these @plen and @pelts, but that's clearly a matter of taste.
>> >
>> > I just kept the old names in set_prop_array() and used the same names in
>> > new functions to stay consistent. But to be honest, @plen and @pelts
>> > would be equally confusing to me.
>> >
>> > My own choice would probably have been something like array_len and
>> > array_data (if you want to know that it's a pointer, look at its type).
>> >
>> >> > +    char *elem = *arrayptr;
>> >> > +    int i;
>> >> > +
>> >> > +    for (i = 0; i < *alenptr; i++) {
>> >> > +        Property elem_prop = array_elem_prop(obj, prop, name, elem);
>> >> > +        prop->arrayinfo->release(obj, NULL, &elem_prop);
>> >> > +        elem += prop->arrayfieldsize;
>> >> >      }
>> >> > -    g_free(p->propname);
>> >> > -    g_free(p);
>> >> >  }
>> >> >  
>> >> > -static void set_prop_arraylen(Object *obj, Visitor *v, const char *name,
>> >> > -                              void *opaque, Error **errp)
>> >> > +/*
>> >> > + * Setter for an array property. This sets both the array length (which is
>> >> > + * technically the property field in the object) and the array itself (a pointer
>> >> > + * to which is stored in the additional field described by prop->arrayoffset).
>> >> > + */
>> >> > +static void set_prop_array(Object *obj, Visitor *v, const char *name,
>> >> > +                           void *opaque, Error **errp)
>> >> >  {
>> >> > -    /* Setter for the property which defines the length of a
>> >> > -     * variable-sized property array. As well as actually setting the
>> >> > -     * array-length field in the device struct, we have to create the
>> >> > -     * array itself and dynamically add the corresponding properties.
>> >> > -     */
>> >> > +    ERRP_GUARD();
>> >> > +
>> >> 
>> >> Drop the blank line.
>> >> 
>> >> >      Property *prop = opaque;
>> >> >      uint32_t *alenptr = object_field_prop_ptr(obj, prop);
>> >> >      void **arrayptr = (void *)obj + prop->arrayoffset;
>> >> > -    void *eltptr;
>> >> > -    const char *arrayname;
>> >> > -    int i;
>> >> > +    GenericList *list, *elem, *next;
>> >> > +    const size_t list_elem_size = sizeof(*list) + prop->arrayfieldsize;
>> >> 
>> >> This can be smaller than the size of the QAPI-generated list type, since
>> >> the compiler may add padding.  Does it matter?
>> >
>> > If it happens in practice, it does matter. Do we have any cleaner way to
>> > get the element size without knowing the content of the list?
>> >
>> > I expect that because GenericList only contains a single pointer, the
>> > rest should have natural alignment
>> 
>> Yes, GenericList's size and alignment should match a pointer's:
>> 
>>     typedef struct GenericList {
>>         struct GenericList *next;
>>         char padding[];
>>     } GenericList;
>> 
>> >                                    and therefore the compiler shouldn't
>> > have any reason to insert padding.
>> 
>> The actual list will look like
>> 
>>     struct FOOList {
>>         FOOList *next;
>>         FOOTYPE value;
>>     }
>> 
>> where FOOTYPE is some QAPI-generated type.  No padding as long as
>> FOOTYPE's alignment divides the pointer size.  I figure that's true for
>> our current targets and generated QAPI types (currently pointers,
>> double, bool, or integers up to 64 bits).
>
> I'm quite confused about the whole alignment stuff at the moment, but
> after looking some more at the code, I think I just realised one
> important thing: We're actually not dealing with QAPI-generated types
> here, but with custom visitors in QOM property setters.

Hmm.

The patch is about qdev array properties, which are special QOM
properties.

A QOM property encapsulates arbitray C data within a QOM object.  Its
get() method enables getting this data with an output visitor, and its
set() method enables setting it with an input visitor.

When the C data is a QAPI type T, set() and get() simply call
visit_type_T().  Example: property_get_uint8_ptr(),
property_set_uint8_ptr().

When it isn't, set() and get() do what qapi/visitor.h calls a "virtual
walk".  Example: the array property we're discussing here.

However, there's a catch: set_prop_array() doesn't visit the property's
C data (a heap-allocated array of ELEMENT-TYPE) directly.  Instead, it
visits a heap-allocated GenericList of ELEMENT-TYPE, which it later
copies to the array.

Here's the visit:

    /* Read the whole input into a temporary list */
    elem = list;
    while (elem) {
        Property elem_prop = array_elem_prop(obj, prop, name, elem->padding);
--->    prop->arrayinfo->set(obj, v, NULL, &elem_prop, errp);
        if (*errp) {
            ok = false;
            goto out_obj;
        }
        (*alenptr)++;
        elem = visit_next_list(v, elem, list_elem_size);
    }

The call marked ---> visits an element at address &elem->padding.

This address is pointer-aligned.  What if the element type requires more
alignment?  Unusual, but certainly not impossible.  For instance, SSE
and AVX vectors require 16 or even 32 byte alignment.

Or am I confused?

>                                                         The netdev one
> specifically (which is used in the array property of the 'rocker'
> device) uses NICPeers (not a pointer to NICPeers!), which is much
> larger.
>
> But now I'm wondering, FOOList doesn't even exist as a type. We're
> calculating list_elem_size only to pass it to visit_start_list(), which
> doesn't have FOOList either. What does the visitor do with it?
>
> It seems it doesn't do anything with it except using it as the size to
> malloc() elements. And then we (the array property code in this patch)
> use &elem->padding as the element pointer, directly from GenericList,
> not any specific FOOList. So I think this just means that we never
> insert any padding and what the compiler would do with a hypothetical
> FOOList, which doesn't even have to exist as a type, doesn't matter.
>
> Am I missing something or does this mean that the code is actually fine?
>
>> > If you think this is not enough and there is no other way to get the
>> > size of the list elements, we might have to generate packed structs for
>> > the QAPI list types (which are really only two pointers, so not much to
>> > lose when we do that).
>> 
>> Could we assert the element type's alignment divides GenericList's size?
>> Not here, obviously, but in DEFINE_PROP_ARRAY(), where we can use
>> __alignof__(_arraytype).
>
> A bit tricky because we're in the middle of a literal, but I suppose
> with the GCC extension we could change one random element of the struct
> to something like:
>
>     ({ QEMU_BUILD_BUG_ON(...); real_value; })
>
> Of course, if my thoughts above were right, this might not actually be
> needed.
>
>> We could also play with attribute aligned to ensure GenericList's size is
>> safe, but I doubt that's worthwhile.
>> 
>> >> > +    char *elemptr;
>> >> > +    bool ok = true;
>> >> >  
>> >> >      if (*alenptr) {
>> >> >          error_setg(errp, "array size property %s may not be set more than once",
>> >> >                     name);
>> >> >          return;
>> >> >      }
>> >> > -    if (!visit_type_uint32(v, name, alenptr, errp)) {
>> >> > +
>> >> > +    if (!visit_start_list(v, name, &list, list_elem_size, errp)) {
>> >> >          return;
>> >> >      }
>> >> > -    if (!*alenptr) {
>> >> > +
>> >> > +    /* Read the whole input into a temporary list */
>> >> > +    elem = list;
>> >> > +    while (elem) {
>> >> > +        Property elem_prop = array_elem_prop(obj, prop, name, elem->padding);
>> >> > +        prop->arrayinfo->set(obj, v, NULL, &elem_prop, errp);
>> >> > +        if (*errp) {
>> >> > +            ok = false;
>> >> > +            goto out_obj;
>> >> > +        }
>> >> > +        (*alenptr)++;
>> >> > +        elem = visit_next_list(v, elem, list_elem_size);
>> >> > +    }
>> >> > +
>> >> > +    ok = visit_check_list(v, errp);
>> >> > +out_obj:
>> >> > +    visit_end_list(v, (void**) &list);
>> >> > +
>> >> > +    if (!ok) {
>> >> > +        for (elem = list; elem; elem = next) {
>> >> > +            next = elem->next;
>> >> > +            g_free(elem);
>> >> > +        }
>> >> 
>> >> We consume the list even on error.  It's too late in my day for me to
>> >> see why that's proper.
>> >
>> > Who else would free it otherwise?
>> >
>> > This is pretty much the same as the generated list visitors do.
>> 
>> Help me out: point me to the precedence you have in mind.
>
> Any QAPI generated list visiting function should do. As an example, I'm
> looking at visit_type_Qcow2BitmapInfoList():
>
>     ...
>     ok = visit_check_list(v, errp);
> out_obj:
>     visit_end_list(v, (void **)obj);
>     if (!ok && visit_is_input(v)) {
>         qapi_free_Qcow2BitmapInfoList(*obj);
>         *obj = NULL;
>     }
>     return ok;
> }
>
> On failure, it calls qapi_free_Qcow2BitmapInfoList().
>
> What's probably wrong in my code is that to be a full equivalent, it
> also needs to free things behind pointers that are owned by elem, i.e.
> call prop->arrayinfo->release().
>
> In practice, it doesn't currently make a difference because none of the
> types used with array properties actually have a release callback.
> Should still be fixed, of course.

Glad I got sufficiently confused to ask ;)

>> >> >          return;
>> >> >      }
>> >> >  
>> >> > -    /* DEFINE_PROP_ARRAY guarantees that name should start with this prefix;
>> >> > -     * strip it off so we can get the name of the array itself.
>> >> > +    /*
>> >> > +     * Now that we know how big the array has to be, move the data over to a
>> >> > +     * linear array and free the temporary list.
>> >> >       */
>> >> > -    assert(strncmp(name, PROP_ARRAY_LEN_PREFIX,
>> >> > -                   strlen(PROP_ARRAY_LEN_PREFIX)) == 0);
>> >> > -    arrayname = name + strlen(PROP_ARRAY_LEN_PREFIX);
>> >> > +    *arrayptr = g_malloc_n(*alenptr, prop->arrayfieldsize);
>> >> > +    elemptr = *arrayptr;
>> >> > +    for (elem = list; elem; elem = next) {
>> >> > +        memcpy(elemptr, elem->padding, prop->arrayfieldsize);
>> >> > +        elemptr += prop->arrayfieldsize;
>> >> > +        next = elem->next;
>> >> > +        g_free(elem);
>> >> > +    }
>> >> > +}
>> >> >  
>> >> > -    /* Note that it is the responsibility of the individual device's deinit
>> >> > -     * to free the array proper.
>> >> > -     */
>> >> > -    *arrayptr = eltptr = g_malloc0(*alenptr * prop->arrayfieldsize);
>> >> > -    for (i = 0; i < *alenptr; i++, eltptr += prop->arrayfieldsize) {
>> >> > -        char *propname = g_strdup_printf("%s[%d]", arrayname, i);
>> >> > -        ArrayElementProperty *arrayprop = g_new0(ArrayElementProperty, 1);
>> >> > -        arrayprop->release = prop->arrayinfo->release;
>> >> > -        arrayprop->propname = propname;
>> >> > -        arrayprop->prop.info = prop->arrayinfo;
>> >> > -        arrayprop->prop.name = propname;
>> >> > -        /* This ugly piece of pointer arithmetic sets up the offset so
>> >> > -         * that when the underlying get/set hooks call qdev_get_prop_ptr
>> >> > -         * they get the right answer despite the array element not actually
>> >> > -         * being inside the device struct.
>> >> > -         */
>> >> > -        arrayprop->prop.offset = eltptr - (void *)obj;
>> >> > -        assert(object_field_prop_ptr(obj, &arrayprop->prop) == eltptr);
>> >> > -        object_property_add(obj, propname,
>> >> > -                            arrayprop->prop.info->name,
>> >> > -                            field_prop_getter(arrayprop->prop.info),
>> >> > -                            field_prop_setter(arrayprop->prop.info),
>> >> > -                            array_element_release,
>> >> > -                            arrayprop);
>> >> > +static void get_prop_array(Object *obj, Visitor *v, const char *name,
>> >> > +                           void *opaque, Error **errp)
>> >> > +{
>> >> > +    ERRP_GUARD();
>> >> > +
>> >> 
>> >> Drop the blank line.
>> >> 
>> >> > +    Property *prop = opaque;
>> >> > +    uint32_t *alenptr = object_field_prop_ptr(obj, prop);
>> >> > +    void **arrayptr = (void *)obj + prop->arrayoffset;
>> >> > +    char *elem = *arrayptr;
>> >> > +    GenericList *list;
>> >> > +    const size_t list_elem_size = sizeof(*list) + prop->arrayfieldsize;
>> >> > +    int i;
>> >> > +
>> >> > +    if (!visit_start_list(v, name, &list, list_elem_size, errp)) {
>> >> > +        return;
>> >> >      }
>> >> > +
>> >> > +    for (i = 0; i < *alenptr; i++) {
>> >> > +        Property elem_prop = array_elem_prop(obj, prop, name, elem);
>> >> > +        prop->arrayinfo->get(obj, v, NULL, &elem_prop, errp);
>> >> > +        if (*errp) {
>> >> > +            goto out_obj;
>> >> > +        }
>> >> > +        elem += prop->arrayfieldsize;
>> >> > +    }
>> >> > +
>> >> 
>> >> You neglect to call visit_check_list().
>> >
>> > It is documented to be intended for input visitors only. Do we need it
>> > with an output visitor?
>> 
>> Help me out: where is that documented?
>
> In struct Visitor:
>
>     /* Optional; intended for input visitors */
>     bool (*check_list)(Visitor *v, Error **errp);

I think this comment tries to say "this is intended to be used by input
visitors", which doesn't quite imply "only input visitors use this".

> And indeed, the existing output vistors don't implement it.

It's implemented by all the input visitors.

Fine print: the forward visitor is the same kind of visitor as the one
it wraps, either input or output.  It implements check_list()
unconditionally.

> Admittedly, the public visit_check_list() is less clear:
>
>     /*
>      * Prepare for completing a list visit.
>      *
>      * On failure, store an error through @errp.  Can happen only when @v
>      * is an input visitor.
>      *
>      * Return true on success, false on failure.
>      *
>      * Should be called prior to visit_end_list() if all other
>      * intermediate visit steps were successful, to allow the visitor one
>      * last chance to report errors.  May be skipped on a cleanup path,
>      * where there is no need to check for further errors.
>      */
>     bool visit_check_list(Visitor *v, Error **errp);
>
> If you prefer, I can add it here just to be sure. Instead of having real
> error handling, assert(ok) should be enough.

We either tighten the public visitor contract to make calling
visit_check_list() clearly optional when you know you're not using an
input visitor.

Or we call it here, too.  Probably simpler.

>> >> > +out_obj:
>> >> > +    visit_end_list(v, (void**) &list);
>> >> >  }
>
> Kevin

Re: [PATCH 01/11] qdev: Add qdev_prop_set_array()

[Peter Maydell]

On Fri, 8 Sept 2023 at 15:37, Kevin Wolf <kwolf@redhat.com> wrote:
>
> 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>
> ---
>  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);

Are we happy enough with this interface that I can take this single
patch in a series that I'm doing (v2 of
https://patchew.org/QEMU/20231017122302.1692902-1-peter.maydell@linaro.org/
"arm/stellaris: convert gamepad input device to qdev"), or should
I stick to the old style "set length and element properties by hand"
code until this whole series has passed patch review (thus giving
you another item to add to the conversion list) ? I went for "include
this patch" in v1, but this series has spent longer in code review
than I was anticipating at that point.

thanks
-- PMM

Re: [PATCH 01/11] qdev: Add qdev_prop_set_array()

[Kevin Wolf]

Am 27.10.2023 um 20:06 hat Peter Maydell geschrieben:
> On Fri, 8 Sept 2023 at 15:37, Kevin Wolf <kwolf@redhat.com> wrote:
> >
> > 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>
> > ---
> >  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);
> 
> Are we happy enough with this interface that I can take this single
> patch in a series that I'm doing (v2 of
> https://patchew.org/QEMU/20231017122302.1692902-1-peter.maydell@linaro.org/
> "arm/stellaris: convert gamepad input device to qdev"), or should
> I stick to the old style "set length and element properties by hand"
> code until this whole series has passed patch review (thus giving
> you another item to add to the conversion list) ? I went for "include
> this patch" in v1, but this series has spent longer in code review
> than I was anticipating at that point.

As far as I am concerned, feel free to include it in your series.

I'm also planning to send v2 of this series soon, I think I'm only
missing the build time assertion to check the correct alignment that
Markus wants to see.

Kevin