ERRP_GUARD() makes debugging easier by making sure that &error_abort still fails at the real origin of the error instead of error_propagate(). Signed-off-by: Kevin Wolf <kwolf(a)redhat.com&gt; --- qom/object.c | 7 +++---- qom/object_interfaces.c | 17 ++++++----------- 2 files changed, 9 insertions(+), 15 deletions(-) diff --git a/qom/object.c b/qom/object.c index e86cb05b84..6be710bc40 100644 --- a/qom/object.c +++ b/qom/object.c @@ -1389,7 +1389,7 @@ bool object_property_get(Object *obj, const char *name, Visitor *v, bool object_property_set(Object *obj, const char *name, Visitor *v, Error **errp) { - Error *err = NULL; + ERRP_GUARD(); ObjectProperty *prop = object_property_find_err(obj, name, errp); if (prop == NULL) { @@ -1400,9 +1400,8 @@ bool object_property_set(Object *obj, const char *name, Visitor *v, error_setg(errp, QERR_PERMISSION_DENIED); return false; } - prop->set(obj, v, name, prop->opaque, &err); - error_propagate(errp, err); - return !err; + prop->set(obj, v, name, prop->opaque, errp); + return !*errp; } bool object_property_set_str(Object *obj, const char *name, diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c index ad9b56b59a..80691e88cd 100644 --- a/qom/object_interfaces.c +++ b/qom/object_interfaces.c @@ -45,26 +45,21 @@ bool user_creatable_can_be_deleted(UserCreatable *uc) static void object_set_properties_from_qdict(Object *obj, const QDict *qdict, Visitor *v, Error **errp) { + ERRP_GUARD(); const QDictEntry *e; - Error *local_err = NULL; - if (!visit_start_struct(v, NULL, NULL, 0, &local_err)) { - goto out; + if (!visit_start_struct(v, NULL, NULL, 0, errp)) { + return; } for (e = qdict_first(qdict); e; e = qdict_next(qdict, e)) { - if (!object_property_set(obj, e->key, v, &local_err)) { + if (!object_property_set(obj, e->key, v, errp)) { break; } } - if (!local_err) { - visit_check_struct(v, &local_err); + if (!*errp) { + visit_check_struct(v, errp); } visit_end_struct(v, NULL); - -out: - if (local_err) { - error_propagate(errp, local_err); - } } void object_set_properties_from_keyval(Object *obj, const QDict *qdict, -- 2.31.1

iothread is a string property, so None (= JSON null) is not a valid value for it. Pass the empty string instead to get the default iothread. Signed-off-by: Kevin Wolf <kwolf(a)redhat.com&gt; --- tests/qemu-iotests/245 | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/qemu-iotests/245 b/tests/qemu-iotests/245 index bf8261eec0..9b12b42eed 100755 --- a/tests/qemu-iotests/245 +++ b/tests/qemu-iotests/245 @@ -1189,10 +1189,10 @@ class TestBlockdevReopen(iotests.QMPTestCase): self.run_test_iothreads('iothread0', 'iothread0') def test_iothreads_switch_backing(self): - self.run_test_iothreads('iothread0', None) + self.run_test_iothreads('iothread0', '') def test_iothreads_switch_overlay(self): - self.run_test_iothreads(None, 'iothread0') + self.run_test_iothreads('', 'iothread0') if __name__ == '__main__': iotests.activate_logging() -- 2.31.1

The iothread isn't called 'iothread0', but 'thread0'. Depending on the order that properties are parsed, the error message may change from the expected one to another one saying that the iothread doesn't exist. Signed-off-by: Kevin Wolf <kwolf(a)redhat.com&gt; --- tests/qemu-iotests/051 | 2 +- tests/qemu-iotests/051.pc.out | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/qemu-iotests/051 b/tests/qemu-iotests/051 index 7bf29343d7..1d2fa93a11 100755 --- a/tests/qemu-iotests/051 +++ b/tests/qemu-iotests/051 @@ -199,7 +199,7 @@ case "$QEMU_DEFAULT_MACHINE" in # virtio-blk enables the iothread only when the driver initialises the # device, so a second virtio-blk device can't be added even with the # same iothread. virtio-scsi allows this. - run_qemu $iothread -device virtio-blk-pci,drive=disk,iothread=iothread0,share-rw=on + run_qemu $iothread -device virtio-blk-pci,drive=disk,iothread=thread0,share-rw=on run_qemu $iothread -device virtio-scsi,id=virtio-scsi1,iothread=thread0 -device scsi-hd,bus=virtio-scsi1.0,drive=disk,share-rw=on ;; *) diff --git a/tests/qemu-iotests/051.pc.out b/tests/qemu-iotests/051.pc.out index afe7632964..063e4fc584 100644 --- a/tests/qemu-iotests/051.pc.out +++ b/tests/qemu-iotests/051.pc.out @@ -183,9 +183,9 @@ Testing: -drive file=TEST_DIR/t.qcow2,if=none,node-name=disk -object iothread,id QEMU X.Y.Z monitor - type 'help' for more information (qemu) QEMU_PROG: -device scsi-hd,bus=virtio-scsi1.0,drive=disk,share-rw=on: Cannot change iothread of active block backend -Testing: -drive file=TEST_DIR/t.qcow2,if=none,node-name=disk -object iothread,id=thread0 -device virtio-scsi,iothread=thread0,id=virtio-scsi0 -device scsi-hd,bus=virtio-scsi0.0,drive=disk,share-rw=on -device virtio-blk-pci,drive=disk,iothread=iothread0,share-rw=on +Testing: -drive file=TEST_DIR/t.qcow2,if=none,node-name=disk -object iothread,id=thread0 -device virtio-scsi,iothread=thread0,id=virtio-scsi0 -device scsi-hd,bus=virtio-scsi0.0,drive=disk,share-rw=on -device virtio-blk-pci,drive=disk,iothread=thread0,share-rw=on QEMU X.Y.Z monitor - type 'help' for more information -(qemu) QEMU_PROG: -device virtio-blk-pci,drive=disk,iothread=iothread0,share-rw=on: Cannot change iothread of active block backend +(qemu) QEMU_PROG: -device virtio-blk-pci,drive=disk,iothread=thread0,share-rw=on: Cannot change iothread of active block backend Testing: -drive file=TEST_DIR/t.qcow2,if=none,node-name=disk -object iothread,id=thread0 -device virtio-scsi,iothread=thread0,id=virtio-scsi0 -device scsi-hd,bus=virtio-scsi0.0,drive=disk,share-rw=on -device virtio-scsi,id=virtio-scsi1,iothread=thread0 -device scsi-hd,bus=virtio-scsi1.0,drive=disk,share-rw=on QEMU X.Y.Z monitor - type 'help' for more information -- 2.31.1

The only thing the string visitor adds compared to a keyval visitor is list support. git grep for 'visit_start_list' and 'visit.*List' shows that devices don't make use of this. In a world with a QAPIfied command line interface, the keyval visitor is used to parse the command line. In order to make sure that no devices start using this feature that would make backwards compatibility harder, just switch away from object_property_parse(), which internally uses the string visitor, to a keyval visitor and object_property_set(). Signed-off-by: Kevin Wolf <kwolf(a)redhat.com&gt; --- softmmu/qdev-monitor.c | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-) diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c index 0705f00846..034b999401 100644 --- a/softmmu/qdev-monitor.c +++ b/softmmu/qdev-monitor.c @@ -28,6 +28,8 @@ #include "qapi/qmp/dispatch.h" #include "qapi/qmp/qdict.h" #include "qapi/qmp/qerror.h" +#include "qapi/qmp/qstring.h" +#include "qapi/qobject-input-visitor.h" #include "qemu/config-file.h" #include "qemu/error-report.h" #include "qemu/help_option.h" @@ -198,16 +200,28 @@ static int set_property(void *opaque, const char *name, const char *value, Error **errp) { Object *obj = opaque; + QString *val; + Visitor *v; + int ret; if (strcmp(name, "driver") == 0) return 0; if (strcmp(name, "bus") == 0) return 0; - if (!object_property_parse(obj, name, value, errp)) { - return -1; + val = qstring_from_str(value); + v = qobject_input_visitor_new_keyval(QOBJECT(val)); + + if (!object_property_set(obj, name, v, errp)) { + ret = -1; + goto out; } - return 0; + + ret = 0; +out: + visit_free(v); + qobject_unref(val); + return ret; } static const char *find_typename_by_alias(const char *alias) -- 2.31.1

DeviceState.id is a pointer to a string that is stored in the QemuOpts object DeviceState.opts and freed together with it. We want to create devices without going through QemuOpts in the future, so make this a separately allocated string. Signed-off-by: Kevin Wolf <kwolf(a)redhat.com&gt; --- include/hw/qdev-core.h | 2 +- include/monitor/qdev.h | 2 +- hw/arm/virt.c | 2 +- hw/core/qdev.c | 1 + hw/pci-bridge/pci_expander_bridge.c | 2 +- hw/ppc/e500.c | 2 +- softmmu/qdev-monitor.c | 5 +++-- 7 files changed, 9 insertions(+), 7 deletions(-) diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h index 34c8a7506a..1857d9698e 100644 --- a/include/hw/qdev-core.h +++ b/include/hw/qdev-core.h @@ -176,7 +176,7 @@ struct DeviceState { Object parent_obj; /*< public >*/ - const char *id; + char *id; char *canonical_path; bool realized; bool pending_deleted_event; diff --git a/include/monitor/qdev.h b/include/monitor/qdev.h index eaa947d73a..389287eb44 100644 --- a/include/monitor/qdev.h +++ b/include/monitor/qdev.h @@ -9,6 +9,6 @@ void qmp_device_add(QDict *qdict, QObject **ret_data, Error **errp); int qdev_device_help(QemuOpts *opts); DeviceState *qdev_device_add(QemuOpts *opts, Error **errp); -void qdev_set_id(DeviceState *dev, const char *id); +void qdev_set_id(DeviceState *dev, char *id); #endif diff --git a/hw/arm/virt.c b/hw/arm/virt.c index 1d59f0e59f..f933d48d3b 100644 --- a/hw/arm/virt.c +++ b/hw/arm/virt.c @@ -1459,7 +1459,7 @@ static void create_platform_bus(VirtMachineState *vms) MemoryRegion *sysmem = get_system_memory(); dev = qdev_new(TYPE_PLATFORM_BUS_DEVICE); - dev->id = TYPE_PLATFORM_BUS_DEVICE; + dev->id = g_strdup(TYPE_PLATFORM_BUS_DEVICE); qdev_prop_set_uint32(dev, "num_irqs", PLATFORM_BUS_NUM_IRQS); qdev_prop_set_uint32(dev, "mmio_size", vms->memmap[VIRT_PLATFORM_BUS].size); sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal); diff --git a/hw/core/qdev.c b/hw/core/qdev.c index cefc5eaa0a..d918b50a1d 100644 --- a/hw/core/qdev.c +++ b/hw/core/qdev.c @@ -956,6 +956,7 @@ static void device_finalize(Object *obj) } qemu_opts_del(dev->opts); + g_free(dev->id); } static void device_class_base_init(ObjectClass *class, void *data) diff --git a/hw/pci-bridge/pci_expander_bridge.c b/hw/pci-bridge/pci_expander_bridge.c index 7112dc3062..10e6e7c2ab 100644 --- a/hw/pci-bridge/pci_expander_bridge.c +++ b/hw/pci-bridge/pci_expander_bridge.c @@ -245,7 +245,7 @@ static void pxb_dev_realize_common(PCIDevice *dev, bool pcie, Error **errp) } else { bus = pci_root_bus_new(ds, "pxb-internal", NULL, NULL, 0, TYPE_PXB_BUS); bds = qdev_new("pci-bridge"); - bds->id = dev_name; + bds->id = g_strdup(dev_name); qdev_prop_set_uint8(bds, PCI_BRIDGE_DEV_PROP_CHASSIS_NR, pxb->bus_nr); qdev_prop_set_bit(bds, PCI_BRIDGE_DEV_PROP_SHPC, false); } diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c index 95451414dd..960e7efcd3 100644 --- a/hw/ppc/e500.c +++ b/hw/ppc/e500.c @@ -1006,7 +1006,7 @@ void ppce500_init(MachineState *machine) /* Platform Bus Device */ if (pmc->has_platform_bus) { dev = qdev_new(TYPE_PLATFORM_BUS_DEVICE); - dev->id = TYPE_PLATFORM_BUS_DEVICE; + dev->id = g_strdup(TYPE_PLATFORM_BUS_DEVICE); qdev_prop_set_uint32(dev, "num_irqs", pmc->platform_bus_num_irqs); qdev_prop_set_uint32(dev, "mmio_size", pmc->platform_bus_size); sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal); diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c index 034b999401..1207e57a46 100644 --- a/softmmu/qdev-monitor.c +++ b/softmmu/qdev-monitor.c @@ -592,7 +592,8 @@ static BusState *qbus_find(const char *path, Error **errp) return bus; } -void qdev_set_id(DeviceState *dev, const char *id) +/* Takes ownership of @id, will be freed when deleting the device */ +void qdev_set_id(DeviceState *dev, char *id) { if (id) { dev->id = id; @@ -690,7 +691,7 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error **errp) } } - qdev_set_id(dev, qemu_opts_id(opts)); + qdev_set_id(dev, g_strdup(qemu_opts_id(opts))); /* set properties */ if (qemu_opt_foreach(opts, set_property, dev, errp)) { -- 2.31.1

Am 24.09.2021 um 16:02 hat Vladimir Sementsov-Ogievskiy geschrieben: Ok, I can mention it explicitly. If it's bad now, it was as bad before because previously it was just a pointer stolen from the QemuOpts that is freed in the same function as dev->id after this patch. I think the code is correct (it's just another field of the same device, the real bus name is just a copy of it, and the bus can't outlive the device that owns it anyway), but it might be bad style. Kevin

object_property_add_child() fails (with &error_abort) if an object with the same name already exists. As long as QemuOpts is in use for -device and device_add, it catches duplicate IDs before qdev_set_id() is even called. However, for enabling non-QemuOpts code paths, we need to make sure that the condition doesn't cause a crash, but fails gracefully. Signed-off-by: Kevin Wolf <kwolf(a)redhat.com&gt; --- include/monitor/qdev.h | 2 +- hw/xen/xen-legacy-backend.c | 3 ++- softmmu/qdev-monitor.c | 16 ++++++++++------ 3 files changed, 13 insertions(+), 8 deletions(-) diff --git a/include/monitor/qdev.h b/include/monitor/qdev.h index 389287eb44..7961308c75 100644 --- a/include/monitor/qdev.h +++ b/include/monitor/qdev.h @@ -9,6 +9,6 @@ void qmp_device_add(QDict *qdict, QObject **ret_data, Error **errp); int qdev_device_help(QemuOpts *opts); DeviceState *qdev_device_add(QemuOpts *opts, Error **errp); -void qdev_set_id(DeviceState *dev, char *id); +void qdev_set_id(DeviceState *dev, char *id, Error **errp); #endif diff --git a/hw/xen/xen-legacy-backend.c b/hw/xen/xen-legacy-backend.c index dd8ae1452d..17aca85ddc 100644 --- a/hw/xen/xen-legacy-backend.c +++ b/hw/xen/xen-legacy-backend.c @@ -276,7 +276,8 @@ static struct XenLegacyDevice *xen_be_get_xendev(const char *type, int dom, xendev = g_malloc0(ops->size); object_initialize(&xendev->qdev, ops->size, TYPE_XENBACKEND); OBJECT(xendev)->free = g_free; - qdev_set_id(DEVICE(xendev), g_strdup_printf("xen-%s-%d", type, dev)); + qdev_set_id(DEVICE(xendev), g_strdup_printf("xen-%s-%d", type, dev), + &error_abort); qdev_realize(DEVICE(xendev), xen_sysbus, &error_fatal); object_unref(OBJECT(xendev)); diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c index 1207e57a46..c2af906df0 100644 --- a/softmmu/qdev-monitor.c +++ b/softmmu/qdev-monitor.c @@ -593,26 +593,27 @@ static BusState *qbus_find(const char *path, Error **errp) } /* Takes ownership of @id, will be freed when deleting the device */ -void qdev_set_id(DeviceState *dev, char *id) +void qdev_set_id(DeviceState *dev, char *id, Error **errp) { if (id) { dev->id = id; } if (dev->id) { - object_property_add_child(qdev_get_peripheral(), dev->id, - OBJECT(dev)); + object_property_try_add_child(qdev_get_peripheral(), dev->id, + OBJECT(dev), errp); } else { static int anon_count; gchar *name = g_strdup_printf("device[%d]", anon_count++); - object_property_add_child(qdev_get_peripheral_anon(), name, - OBJECT(dev)); + object_property_try_add_child(qdev_get_peripheral_anon(), name, + OBJECT(dev), errp); g_free(name); } } DeviceState *qdev_device_add(QemuOpts *opts, Error **errp) { + ERRP_GUARD(); DeviceClass *dc; const char *driver, *path; DeviceState *dev = NULL; @@ -691,7 +692,10 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error **errp) } } - qdev_set_id(dev, g_strdup(qemu_opts_id(opts))); + qdev_set_id(dev, g_strdup(qemu_opts_id(opts)), errp); + if (*errp) { + goto err_del_dev; + } /* set properties */ if (qemu_opt_foreach(opts, set_property, dev, errp)) { -- 2.31.1

Hi Kevin, I proposed a very similar patch in our rfc series because we needed some of the cleaning you do here. https://lists.gnu.org/archive/html/qemu-devel/2021-09/msg05679.html I've added a bit of doc for the function, feel free to take it if you want. On 9/24/21 16:09, Vladimir Sementsov-Ogievskiy wrote: dev->id is expected to be NULL. The object is created just before calling this function so it is always the case. We could probably assert this.

Use QTAILQ_FOREACH_SAFE() so that the current QemuOpts can be deleted while iterating through the whole list. Signed-off-by: Kevin Wolf <kwolf(a)redhat.com&gt; --- util/qemu-option.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/util/qemu-option.c b/util/qemu-option.c index 61cb4a97bd..eedd08929b 100644 --- a/util/qemu-option.c +++ b/util/qemu-option.c @@ -1126,11 +1126,11 @@ int qemu_opts_foreach(QemuOptsList *list, qemu_opts_loopfunc func, void *opaque, Error **errp) { Location loc; - QemuOpts *opts; + QemuOpts *opts, *next; int rc = 0; loc_push_none(&loc); - QTAILQ_FOREACH(opts, &list->head, next) { + QTAILQ_FOREACH_SAFE(opts, &list->head, next, next) { loc_restore(&opts->loc); rc = func(opaque, opts, errp); if (rc) { -- 2.31.1

QDicts are both what QMP natively uses and what the keyval parser produces. Going through QemuOpts isn't useful for either one, so switch the main device creation function to QDicts. By sharing more code with the -object/object-add code path, we can even reduce the code size a bit. This commit doesn't remove the detour through QemuOpts from any code path yet, but it allows the following commits to do so. Signed-off-by: Kevin Wolf <kwolf(a)redhat.com&gt; --- include/hw/qdev-core.h | 8 ++--- hw/core/qdev.c | 5 ++-- hw/net/virtio-net.c | 4 +-- hw/vfio/pci.c | 4 +-- softmmu/qdev-monitor.c | 67 +++++++++++++++++++----------------------- 5 files changed, 41 insertions(+), 47 deletions(-) diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h index 1857d9698e..5b3d4704a5 100644 --- a/include/hw/qdev-core.h +++ b/include/hw/qdev-core.h @@ -180,7 +180,7 @@ struct DeviceState { char *canonical_path; bool realized; bool pending_deleted_event; - QemuOpts *opts; + QDict *opts; int hotplugged; bool allow_unplug_during_migration; BusState *parent_bus; @@ -202,7 +202,7 @@ struct DeviceListener { * hide a failover device depending for example on the device * opts. */ - bool (*hide_device)(DeviceListener *listener, QemuOpts *device_opts); + bool (*hide_device)(DeviceListener *listener, const QDict *device_opts); QTAILQ_ENTRY(DeviceListener) link; }; @@ -831,13 +831,13 @@ void device_listener_unregister(DeviceListener *listener); /** * @qdev_should_hide_device: - * @opts: QemuOpts as passed on cmdline. + * @opts: options QDict * * Check if a device should be added. * When a device is added via qdev_device_add() this will be called, * and return if the device should be added now or not. */ -bool qdev_should_hide_device(QemuOpts *opts); +bool qdev_should_hide_device(const QDict *opts); typedef enum MachineInitPhase { /* current_machine is NULL. */ diff --git a/hw/core/qdev.c b/hw/core/qdev.c index d918b50a1d..5b889866c5 100644 --- a/hw/core/qdev.c +++ b/hw/core/qdev.c @@ -28,6 +28,7 @@ #include "qemu/osdep.h" #include "qapi/error.h" #include "qapi/qapi-events-qdev.h" +#include "qapi/qmp/qdict.h" #include "qapi/qmp/qerror.h" #include "qapi/visitor.h" #include "qemu/error-report.h" @@ -211,7 +212,7 @@ void device_listener_unregister(DeviceListener *listener) QTAILQ_REMOVE(&device_listeners, listener, link); } -bool qdev_should_hide_device(QemuOpts *opts) +bool qdev_should_hide_device(const QDict *opts) { DeviceListener *listener; @@ -955,7 +956,7 @@ static void device_finalize(Object *obj) dev->canonical_path = NULL; } - qemu_opts_del(dev->opts); + qobject_unref(dev->opts); g_free(dev->id); } diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c index f205331dcf..5684c2b2b7 100644 --- a/hw/net/virtio-net.c +++ b/hw/net/virtio-net.c @@ -3304,7 +3304,7 @@ static void virtio_net_migration_state_notifier(Notifier *notifier, void *data) } static bool failover_hide_primary_device(DeviceListener *listener, - QemuOpts *device_opts) + const QDict *device_opts) { VirtIONet *n = container_of(listener, VirtIONet, primary_listener); const char *standby_id; @@ -3312,7 +3312,7 @@ static bool failover_hide_primary_device(DeviceListener *listener, if (!device_opts) { return false; } - standby_id = qemu_opt_get(device_opts, "failover_pair_id"); + standby_id = qdict_get_try_str(device_opts, "failover_pair_id"); if (g_strcmp0(standby_id, n->netclient_name) != 0) { return false; } diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c index 4feaa1cb68..5cdf1d4298 100644 --- a/hw/vfio/pci.c +++ b/hw/vfio/pci.c @@ -29,10 +29,10 @@ #include "hw/qdev-properties.h" #include "hw/qdev-properties-system.h" #include "migration/vmstate.h" +#include "qapi/qmp/qdict.h" #include "qemu/error-report.h" #include "qemu/main-loop.h" #include "qemu/module.h" -#include "qemu/option.h" #include "qemu/range.h" #include "qemu/units.h" #include "sysemu/kvm.h" @@ -941,7 +941,7 @@ static void vfio_pci_size_rom(VFIOPCIDevice *vdev) } if (vfio_opt_rom_in_denylist(vdev)) { - if (dev->opts && qemu_opt_get(dev->opts, "rombar")) { + if (dev->opts && qdict_haskey(dev->opts, "rombar")) { warn_report("Device at %s is known to cause system instability" " issues during option rom execution", vdev->vbasedev.name); diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c index c2af906df0..c09b7430eb 100644 --- a/softmmu/qdev-monitor.c +++ b/softmmu/qdev-monitor.c @@ -196,34 +196,6 @@ static void qdev_print_devinfos(bool show_no_user) g_slist_free(list); } -static int set_property(void *opaque, const char *name, const char *value, - Error **errp) -{ - Object *obj = opaque; - QString *val; - Visitor *v; - int ret; - - if (strcmp(name, "driver") == 0) - return 0; - if (strcmp(name, "bus") == 0) - return 0; - - val = qstring_from_str(value); - v = qobject_input_visitor_new_keyval(QOBJECT(val)); - - if (!object_property_set(obj, name, v, errp)) { - ret = -1; - goto out; - } - - ret = 0; -out: - visit_free(v); - qobject_unref(val); - return ret; -} - static const char *find_typename_by_alias(const char *alias) { int i; @@ -611,15 +583,17 @@ void qdev_set_id(DeviceState *dev, char *id, Error **errp) } } -DeviceState *qdev_device_add(QemuOpts *opts, Error **errp) +static DeviceState *qdev_device_add_from_qdict(const QDict *opts, + bool from_json, Error **errp) { ERRP_GUARD(); DeviceClass *dc; const char *driver, *path; + char *id; DeviceState *dev = NULL; BusState *bus = NULL; - driver = qemu_opt_get(opts, "driver"); + driver = qdict_get_try_str(opts, "driver"); if (!driver) { error_setg(errp, QERR_MISSING_PARAMETER, "driver"); return NULL; @@ -632,7 +606,7 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error **errp) } /* find bus */ - path = qemu_opt_get(opts, "bus"); + path = qdict_get_try_str(opts, "bus"); if (path != NULL) { bus = qbus_find(path, errp); if (!bus) { @@ -652,8 +626,8 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error **errp) } } - if (qemu_opt_get(opts, "failover_pair_id")) { - if (!opts->id) { + if (qdict_haskey(opts, "failover_pair_id")) { + if (!qdict_haskey(opts, "id")) { error_setg(errp, "Device with failover_pair_id don't have id"); return NULL; } @@ -692,19 +666,25 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error **errp) } } - qdev_set_id(dev, g_strdup(qemu_opts_id(opts)), errp); + id = g_strdup(qdict_get_try_str(opts, "id")); + qdev_set_id(dev, id, errp); if (*errp) { goto err_del_dev; } /* set properties */ - if (qemu_opt_foreach(opts, set_property, dev, errp)) { + dev->opts = qdict_clone_shallow(opts); + qdict_del(dev->opts, "driver"); + qdict_del(dev->opts, "bus"); + qdict_del(dev->opts, "id"); + + object_set_properties_from_keyval(&dev->parent_obj, dev->opts, from_json, + errp); + if (*errp) { goto err_del_dev; } - dev->opts = opts; if (!qdev_realize(DEVICE(dev), bus, errp)) { - dev->opts = NULL; goto err_del_dev; } return dev; @@ -717,6 +697,19 @@ err_del_dev: return NULL; } +/* Takes ownership of @opts on success */ +DeviceState *qdev_device_add(QemuOpts *opts, Error **errp) +{ + QDict *qdict = qemu_opts_to_qdict(opts, NULL); + DeviceState *ret; + + ret = qdev_device_add_from_qdict(qdict, false, errp); + if (ret) { + qemu_opts_del(opts); + } + qobject_unref(qdict); + return ret; +} #define qdev_printf(fmt, ...) monitor_printf(mon, "%*s" fmt, indent, "", ## __VA_ARGS__) static void qbus_print(Monitor *mon, BusState *bus, int indent); -- 2.31.1

Directly call qdev_device_add_from_qdict() for QMP device_add instead of first going through QemuOpts and converting back to QDict. Note that this changes the behaviour of device_add, though in ways that should be considered bug fixes: QemuOpts ignores differences between data types, so you could successfully pass a string "123" for an integer property, or a string "on" for a boolean property (and vice versa). After this change, the correct data type for the property must be used in the JSON input. qemu_opts_from_qdict() also silently ignores any options whose value is a QDict, QList or QNull. To illustrate, the following QMP command was accepted before and is now rejected for both reasons: { "execute": "device_add", "arguments": { "driver": "scsi-cd", "drive": { "completely": "invalid" }, "physical_block_size": "4096" } } Signed-off-by: Kevin Wolf <kwolf(a)redhat.com&gt; --- softmmu/qdev-monitor.c | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c index c09b7430eb..8622ccade6 100644 --- a/softmmu/qdev-monitor.c +++ b/softmmu/qdev-monitor.c @@ -812,7 +812,8 @@ void hmp_info_qdm(Monitor *mon, const QDict *qdict) qdev_print_devinfos(true); } -void qmp_device_add(QDict *qdict, QObject **ret_data, Error **errp) +static void monitor_device_add(QDict *qdict, QObject **ret_data, + bool from_json, Error **errp) { QemuOpts *opts; DeviceState *dev; @@ -825,7 +826,9 @@ void qmp_device_add(QDict *qdict, QObject **ret_data, Error **errp) qemu_opts_del(opts); return; } - dev = qdev_device_add(opts, errp); + qemu_opts_del(opts); + + dev = qdev_device_add_from_qdict(qdict, from_json, errp); /* * Drain all pending RCU callbacks. This is done because @@ -838,13 +841,14 @@ void qmp_device_add(QDict *qdict, QObject **ret_data, Error **errp) */ drain_call_rcu(); - if (!dev) { - qemu_opts_del(opts); - return; - } object_unref(OBJECT(dev)); } +void qmp_device_add(QDict *qdict, QObject **ret_data, Error **errp) +{ + monitor_device_add(qdict, ret_data, true, errp); +} + static DeviceState *find_device_state(const char *id, Error **errp) { Object *obj; @@ -936,7 +940,7 @@ void hmp_device_add(Monitor *mon, const QDict *qdict) { Error *err = NULL; - qmp_device_add((QDict *)qdict, NULL, &err); + monitor_device_add((QDict *)qdict, NULL, false, &err); hmp_handle_error(mon, err); } -- 2.31.1

On 9/24/21 11:04, Kevin Wolf wrote: Hi Kevin, I'm wandering if deleting the opts (which remove it from the "device" opts list) is really a no-op ? The opts list is, eg, traversed in hw/net/virtio-net.c in the function failover_find_primary_device_id() which may be called during the virtio_net_set_features() (a TYPE_VIRTIO_NET method). I do not have the knowledge to tell when this method is called. But If this is after we create the devices. Then the list will be empty at this point now. It seems, there are 2 other calling sites of "qemu_opts_foreach(qemu_find_opts("device"), [...]" in net/vhost-user.c and net/vhost-vdpa.c -- Damien

Am 27.09.2021 um 13:39 hat Kevin Wolf geschrieben: So I tried to have a look at the virtio-net case, and ended up very confused. Obviously looking at command line options (even of a differrent device) from within a device is very unclean. With a non-broken, i.e. type safe, device-add (as well as with the JSON CLI option introduced by this series), we can't have a QemuOpts any more that is by definition unsafe. So this code needs a replacement. My naive idea was that we just need to look at runtime state instead. Don't search the options for a device with a matching 'failover_pair_id' (which, by the way, would fail as soon as any other device introduces a property with the same name), but search for actual PCIDevices in qdev that have pci_dev->failover_pair_id set accordingly. However, the logic in failover_add_primary() suggests that we can have a state where QemuOpts for a device exist, but the device doesn't, and then it hotplugs the device from the command line options. How would we ever get into such an inconsistent state where QemuOpts contains a device that doesn't exist? Normally devices get their QemuOpts when they are created and device_finalize() deletes the QemuOpts again. Any suggestions how to get rid of the QemuOpts abuse in the failover code? If this is a device that we previously managed to rip out without deleting its QemuOpts, can we store its dev->opts (which is a type safe QDict after this series) somewhere locally instead of looking at global state? Preferably I would even like to get rid of dev->opts because we really should look at live state rather than command line options after device creation, but I guess one step at a time. (Actually, I'm half tempted to just break it because no test cases seem to exist, so apparently nobody is really interested in it.) Kevin

Am 05.10.2021 um 17:52 hat Damien Hedde geschrieben: Yes, that's the only case for which I could imagine for an inconsistency between the qdev tree and QemuOpts, but failover_add_primary() is only called after feature negotiation with the guest driver, so we can be sure that the -device loop has completed long ago. And even if it hadn't completed yet, the paragraph also says that even hotplugging the device later is supported, so creating devices in the wrong order should still succeed. I hope that some of the people I added to CC have some more hints. Kevin

Like we already do for -object, introduce support for JSON syntax in -device, which can be kept stable in the long term and guarantees that a single code path with identical behaviour is used for both QMP and the command line. Compared to the QemuOpts based code, the parser contains less surprises and has support for non-scalar options (lists and structs). Switching management tools to JSON means that we can more easily change the "human" CLI syntax from QemuOpts to the keyval parser later. In the QAPI schema, a feature flag is added to the device-add command to allow management tools to detect support for this. Signed-off-by: Kevin Wolf <kwolf(a)redhat.com&gt; --- qapi/qdev.json | 15 +++++++++---- softmmu/vl.c | 58 ++++++++++++++++++++++++++++++++++++++++++++------ 2 files changed, 62 insertions(+), 11 deletions(-) diff --git a/qapi/qdev.json b/qapi/qdev.json index b83178220b..cdc8f911b5 100644 --- a/qapi/qdev.json +++ b/qapi/qdev.json @@ -32,17 +32,23 @@ ## # @device_add: # +# Add a device. +# # @driver: the name of the new device's driver # # @bus: the device's parent bus (device tree path) # # @id: the device's ID, must be unique # -# Additional arguments depend on the type. -# -# Add a device. +# Features: +# @json-cli: If present, the "-device" command line option supports JSON +# syntax with a structure identical to the arguments of this +# command. # # Notes: +# +# Additional arguments depend on the type. +# # 1. For detailed information about this command, please refer to the # 'docs/qdev-device-use.txt' file. # @@ -67,7 +73,8 @@ ## { 'command': 'device_add', 'data': {'driver': 'str', '*bus': 'str', '*id': 'str'}, - 'gen': false } # so we can get the additional arguments + 'gen': false, # so we can get the additional arguments + 'features': ['json-cli'] } ## # @device_del: diff --git a/softmmu/vl.c b/softmmu/vl.c index 55ab70eb97..7596d9da06 100644 --- a/softmmu/vl.c +++ b/softmmu/vl.c @@ -144,6 +144,12 @@ typedef struct ObjectOption { QTAILQ_ENTRY(ObjectOption) next; } ObjectOption; +typedef struct DeviceOption { + QDict *opts; + Location loc; + QTAILQ_ENTRY(DeviceOption) next; +} DeviceOption; + static const char *cpu_option; static const char *mem_path; static const char *incoming; @@ -151,6 +157,7 @@ static const char *loadvm; static const char *accelerators; static QDict *machine_opts_dict; static QTAILQ_HEAD(, ObjectOption) object_opts = QTAILQ_HEAD_INITIALIZER(object_opts); +static QTAILQ_HEAD(, DeviceOption) device_opts = QTAILQ_HEAD_INITIALIZER(device_opts); static ram_addr_t maxram_size; static uint64_t ram_slots; static int display_remote; @@ -494,21 +501,39 @@ const char *qemu_get_vm_name(void) return qemu_name; } -static int default_driver_check(void *opaque, QemuOpts *opts, Error **errp) +static void default_driver_disable(const char *driver) { - const char *driver = qemu_opt_get(opts, "driver"); int i; - if (!driver) - return 0; + if (!driver) { + return; + } + for (i = 0; i < ARRAY_SIZE(default_list); i++) { if (strcmp(default_list[i].driver, driver) != 0) continue; *(default_list[i].flag) = 0; } +} + +static int default_driver_check(void *opaque, QemuOpts *opts, Error **errp) +{ + const char *driver = qemu_opt_get(opts, "driver"); + + default_driver_disable(driver); return 0; } +static void default_driver_check_json(void) +{ + DeviceOption *opt; + + QTAILQ_FOREACH(opt, &device_opts, next) { + const char *driver = qdict_get_try_str(opt->opts, "driver"); + default_driver_disable(driver); + } +} + static int parse_name(void *opaque, QemuOpts *opts, Error **errp) { const char *proc_name; @@ -1311,6 +1336,7 @@ static void qemu_disable_default_devices(void) { MachineClass *machine_class = MACHINE_GET_CLASS(current_machine); + default_driver_check_json(); qemu_opts_foreach(qemu_find_opts("device"), default_driver_check, NULL, NULL); qemu_opts_foreach(qemu_find_opts("global"), @@ -2637,6 +2663,8 @@ static void qemu_init_board(void) static void qemu_create_cli_devices(void) { + DeviceOption *opt; + soundhw_init(); qemu_opts_foreach(qemu_find_opts("fw_cfg"), @@ -2652,6 +2680,13 @@ static void qemu_create_cli_devices(void) rom_set_order_override(FW_CFG_ORDER_OVERRIDE_DEVICE); qemu_opts_foreach(qemu_find_opts("device"), device_init_func, NULL, &error_fatal); + QTAILQ_FOREACH(opt, &device_opts, next) { + QObject *ret_data; + + loc_push_restore(&opt->loc); + qmp_device_add(opt->opts, &ret_data, &error_fatal); + loc_pop(&opt->loc); + } rom_reset_order_override(); } @@ -3352,9 +3387,18 @@ void qemu_init(int argc, char **argv, char **envp) add_device_config(DEV_USB, optarg); break; case QEMU_OPTION_device: - if (!qemu_opts_parse_noisily(qemu_find_opts("device"), - optarg, true)) { - exit(1); + if (optarg[0] == '{') { + QObject *obj = qobject_from_json(optarg, &error_fatal); + DeviceOption *opt = g_new0(DeviceOption, 1); + opt->opts = qobject_to(QDict, obj); + loc_save(&opt->loc); + assert(opt->opts != NULL); + QTAILQ_INSERT_TAIL(&device_opts, opt, next); + } else { + if (!qemu_opts_parse_noisily(qemu_find_opts("device"), + optarg, true)) { + exit(1); + } } break; case QEMU_OPTION_smp: -- 2.31.1

We want to switch both from QemuOpts to the keyval parser in the future, which results in some incompatibilities, mainly around list handling. Mark the non-JSON version of both as unstable syntax so that management tools switch to JSON and we can later make the change without breaking things. Signed-off-by: Kevin Wolf <kwolf(a)redhat.com&gt; --- docs/about/deprecated.rst | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst index 3c2be84d80..42f6a478fb 100644 --- a/docs/about/deprecated.rst +++ b/docs/about/deprecated.rst @@ -160,6 +160,17 @@ Use ``-display sdl`` instead. Use ``-display curses`` instead. +Stable non-JSON ``-device`` and ``-object`` syntax (since 6.2) +'''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''' + +If you rely on a stable interface for ``-device`` and ``-object`` that doesn't +change incompatibly between QEMU versions (e.g. because you are using the QEMU +command line as a machine interface in scripts rather than interactively), use +JSON syntax for these options instead. + +There is no intention to remove support for non-JSON syntax entirely, but +future versions may change the way to spell some options. + Plugin argument passing through ``arg=<string>`` (since 6.1) '''''''''''''''''''''''''''''''''''''''''''''''''''''''''''' -- 2.31.1