[PATCH 00/11] qdev: Add JSON -device and fix QMP device_add
It's still a long way until we'll have QAPIfied devices, but there are some improvements that we can already make now to make the future switch easier. One important part of this is having code paths without QemuOpts, which we want to get rid of and replace with the keyval parser in the long run. This series adds support for JSON syntax to -device, which bypasses QemuOpts. While we're not using QAPI yet, devices are based on QOM, so we already do have type checks and an implied schema. JSON syntax supported now can be supported by QAPI later and regarding command line compatibility, actually switching to it becomes an implementation detail this way (of course, it will still add valuable user-visible features like introspection and documentation). Apart from making things more future proof, this also immediately adds a way to do non-scalar properties on the command line. nvme could have used list support recently, and the lack of it in -device led to some rather unnatural solution in the first version (doing the relationship between a device and objects backwards) and loss of features in the following. With this series, using a list as a device property should be possible without any weird tricks. Unfortunately, even QMP device_add goes through QemuOpts before this series, which destroys any type safety QOM provides and also can't support non-scalar properties. This is a bug that is fixed during this series, but that is technically an incompatible change. A similar change was made for netdev_add in commit db2a380c84. libvirt needs to make sure that it actually always passes the right type, because passing a wrong type will start to fail after this series. I hope that libvirt already does things correctly, so this won't cause any trouble, but if it does, there is the option of using the QemuOpts-less code path only for JSON -device and going through a deprecation period for QMP device_add. Kevin Wolf (11): qom: Reduce use of error_propagate() iotests/245: Fix type for iothread property iotests/051: Fix typo qdev: Avoid using string visitor for properties qdev: Make DeviceState.id independent of QemuOpts qdev: Add Error parameter to qdev_set_id() qemu-option: Allow deleting opts during qemu_opts_foreach() qdev: Base object creation on QDict rather than QemuOpts qdev: Avoid QemuOpts in QMP device_add vl: Enable JSON syntax for -device Deprecate stable non-JSON -device and -object qapi/qdev.json | 15 +++-- docs/about/deprecated.rst | 11 ++++ include/hw/qdev-core.h | 10 ++-- include/monitor/qdev.h | 2 +- hw/arm/virt.c | 2 +- hw/core/qdev.c | 6 +- hw/net/virtio-net.c | 4 +- hw/pci-bridge/pci_expander_bridge.c | 2 +- hw/ppc/e500.c | 2 +- hw/vfio/pci.c | 4 +- hw/xen/xen-legacy-backend.c | 3 +- qom/object.c | 7 +-- qom/object_interfaces.c | 17 ++---- softmmu/qdev-monitor.c | 90 +++++++++++++++++------------ softmmu/vl.c | 58 ++++++++++++++++--- util/qemu-option.c | 4 +- tests/qemu-iotests/051 | 2 +- tests/qemu-iotests/051.pc.out | 4 +- tests/qemu-iotests/245 | 4 +- 19 files changed, 161 insertions(+), 86 deletions(-) -- 2.31.1
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@redhat.com> --- 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
24.09.2021 12:04, Kevin Wolf wrote:
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@redhat.com> --- 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; }
This is OK: prop->set doesn't return value, so we have to analyze errp to make our own return value.
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,
ERRP_GUARD() + dereferencing errp is good when we use functions which don't return any value. So we want to check is it fail or not and we have to analyze errp. But that is not the case: all called functions follow modern recommendations of returning some value indicating failure together with setting errp. I think, it's better not use ERRP_GUARD where it is not needed: in ideal world, all functions that may fail do return value, and we don't need neither error propagation, nor dereferencing errp. And don't need ERRP_GUARD. ERRP_GUARD will be still needed to support error_prepend()/error_append() together with error_fatal, but again that's not the case. Here I suggest something like this: static void object_set_properties_from_qdict(Object *obj, const QDict *qdict, Visitor *v, Error **errp) { const QDictEntry *e; 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, errp)) { goto out; } } visit_check_struct(v, errp); out: visit_end_struct(v, NULL); } -- Best regards, Vladimir
Kevin Wolf <kwolf@redhat.com> writes:
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@redhat.com>
Yes. The code you patch uses error_propagate() to work around functions not returning distinct error values. error.h's big comment recommends such return values, but recommendations don't update code, patches do. Until then, ERRP_GUARD() is clearly a better crutch than error_propagate(). Reviewed-by: Markus Armbruster <armbru@redhat.com>
On Fri, Sep 24, 2021 at 11:04:17AM +0200, Kevin Wolf wrote:
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@redhat.com> --- qom/object.c | 7 +++---- qom/object_interfaces.c | 17 ++++++----------- 2 files changed, 9 insertions(+), 15 deletions(-)
Reviewed-by: Eric Blake <eblake@redhat.com> -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
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@redhat.com> --- 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
24.09.2021 12:04, Kevin Wolf wrote:
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@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> -- Best regards, Vladimir
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@redhat.com> --- 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
24.09.2021 12:04, Kevin Wolf wrote:
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@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> -- Best regards, Vladimir
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@redhat.com> --- 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
On Fri, Sep 24, 2021 at 11:04:20AM +0200, Kevin Wolf wrote:
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@redhat.com> --- softmmu/qdev-monitor.c | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-)
Reviewed-by: Eric Blake <eblake@redhat.com> -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
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@redhat.com> --- 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
24.09.2021 12:04, Kevin Wolf wrote:
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@redhat.com>
Interesting that in hw/xen/xen-legacy-backend.c g_strdup_printf-allocated id is passed to qdev_set_id prior this patch. So, the patch seems to fix that small leak. Worth to mention?
--- 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)) {
In hw/pci/pci_bridge.c we do br->bus_name = dev->qdev.id It seems bad, as we now stole dynamic pointer, that may be freed later. -- Best regards, Vladimir
Am 24.09.2021 um 16:02 hat Vladimir Sementsov-Ogievskiy geschrieben:
24.09.2021 12:04, Kevin Wolf wrote:
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@redhat.com>
Interesting that in hw/xen/xen-legacy-backend.c g_strdup_printf-allocated id is passed to qdev_set_id prior this patch. So, the patch seems to fix that small leak. Worth to mention?
Ok, I can mention it explicitly.
--- 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)) {
In hw/pci/pci_bridge.c
we do
br->bus_name = dev->qdev.id
It seems bad, as we now stole dynamic pointer, that may be freed later.
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
24.09.2021 18:10, Kevin Wolf wrote:
Am 24.09.2021 um 16:02 hat Vladimir Sementsov-Ogievskiy geschrieben:
24.09.2021 12:04, Kevin Wolf wrote:
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@redhat.com>
Interesting that in hw/xen/xen-legacy-backend.c g_strdup_printf-allocated id is passed to qdev_set_id prior this patch. So, the patch seems to fix that small leak. Worth to mention?
Ok, I can mention it explicitly.
--- 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)) {
In hw/pci/pci_bridge.c
we do
br->bus_name = dev->qdev.id
It seems bad, as we now stole dynamic pointer, that may be freed later.
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.
Ah, right, OK.
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.
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> -- Best regards, Vladimir
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@redhat.com> --- 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
24.09.2021 12:04, Kevin Wolf wrote:
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@redhat.com> --- 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)
According to recommendations in error.h, worth adding also return value (for example true=success false=failure), so [..]
{ if (id) { dev->id = id; }
Unrelated but.. What's the strange logic? Is it intended that with passed id=NULL we don't update dev->id variable but try to do following logic with old dev->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; + }
[..] here we'll have if (!qdev_set_id(...)) { goto err_del_dev; } and no need for ERRP_GUARD.
/* set properties */ if (qemu_opt_foreach(opts, set_property, dev, errp)) {
-- Best regards, Vladimir
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:
24.09.2021 12:04, Kevin Wolf wrote:
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@redhat.com> --- 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)
According to recommendations in error.h, worth adding also return value (for example true=success false=failure), so [..]
{ if (id) { dev->id = id; }
Unrelated but.. What's the strange logic?
Is it intended that with passed id=NULL we don't update dev->id variable but try to do following logic with old dev->id?
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.
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; + }
[..] here we'll have
if (!qdev_set_id(...)) { goto err_del_dev; }
and no need for ERRP_GUARD.
/* set properties */ if (qemu_opt_foreach(opts, set_property, dev, errp)) {
Am 27.09.2021 um 12:33 hat Damien Hedde geschrieben:
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.
Thanks, I'm replacing my patch with yours for v2. Kevin
Use QTAILQ_FOREACH_SAFE() so that the current QemuOpts can be deleted while iterating through the whole list. Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- 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
24.09.2021 12:04, Kevin Wolf wrote:
Use QTAILQ_FOREACH_SAFE() so that the current QemuOpts can be deleted while iterating through the whole list.
Signed-off-by: Kevin Wolf<kwolf@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> -- Best regards, Vladimir
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@redhat.com> --- 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
On Fri, Sep 24, 2021 at 11:04:24AM +0200, Kevin Wolf wrote:
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@redhat.com> --- 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(-)
Reviewed-by: Eric Blake <eblake@redhat.com> -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
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@redhat.com> --- 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 Fri, Sep 24, 2021 at 11:04:25AM +0200, Kevin Wolf wrote:
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@redhat.com> --- softmmu/qdev-monitor.c | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-)
Reviewed-by: Eric Blake <eblake@redhat.com> -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
On 9/24/21 11:04, Kevin Wolf wrote:
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@redhat.com> --- 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);
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:06 hat Damien Hedde geschrieben:
On 9/24/21 11:04, Kevin Wolf wrote:
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@redhat.com> --- 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);
Hi Kevin,
I'm wandering if deleting the opts (which remove it from the "device" opts list) is really a no-op ?
It's not exactly a no-op. Previously, the QemuOpts would only be freed when the device is destroying, now we delete it immediately after creating the device. This could matter in some cases. The one case I was aware of is that QemuOpts used to be responsible for checking for duplicate IDs. Obviously, it can't do this job any more when we call qemu_opts_del() right after creating the device. This is the reason for patch 6.
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
Yes, you are right. These callers probably need to be changed. Going through the command line options rather than looking at the actual device objects that exist doesn't feel entirely clean anyway. Kevin
Am 27.09.2021 um 13:39 hat Kevin Wolf geschrieben:
Am 27.09.2021 um 13:06 hat Damien Hedde geschrieben:
On 9/24/21 11:04, Kevin Wolf wrote:
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@redhat.com> --- 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);
Hi Kevin,
I'm wandering if deleting the opts (which remove it from the "device" opts list) is really a no-op ?
It's not exactly a no-op. Previously, the QemuOpts would only be freed when the device is destroying, now we delete it immediately after creating the device. This could matter in some cases.
The one case I was aware of is that QemuOpts used to be responsible for checking for duplicate IDs. Obviously, it can't do this job any more when we call qemu_opts_del() right after creating the device. This is the reason for patch 6.
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
Yes, you are right. These callers probably need to be changed. Going through the command line options rather than looking at the actual device objects that exist doesn't feel entirely clean anyway.
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
On 10/5/21 16:37, Kevin Wolf wrote:
Am 27.09.2021 um 13:39 hat Kevin Wolf geschrieben:
Am 27.09.2021 um 13:06 hat Damien Hedde geschrieben:
On 9/24/21 11:04, Kevin Wolf wrote:
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@redhat.com> --- 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);
Hi Kevin,
I'm wandering if deleting the opts (which remove it from the "device" opts list) is really a no-op ?
It's not exactly a no-op. Previously, the QemuOpts would only be freed when the device is destroying, now we delete it immediately after creating the device. This could matter in some cases.
The one case I was aware of is that QemuOpts used to be responsible for checking for duplicate IDs. Obviously, it can't do this job any more when we call qemu_opts_del() right after creating the device. This is the reason for patch 6.
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
Yes, you are right. These callers probably need to be changed. Going through the command line options rather than looking at the actual device objects that exist doesn't feel entirely clean anyway.
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. >
Just read the following from docs/system/virtio-net-failover.rst
Usage -----
The primary device can be hotplugged or be part of the startup configuration
-device virtio-net-pci,netdev=hostnet1,id=net1, mac=52:54:00:6f:55:cc,bus=root2,failover=on
With the parameter failover=on the VIRTIO_NET_F_STANDBY feature will be enabled.
-device vfio-pci,host=5e:00.2,id=hostdev0,bus=root1, failover_pair_id=net1
failover_pair_id references the id of the virtio-net standby device. This is only for pairing the devices within QEMU. The guest kernel module net_failover will match devices with identical MAC addresses.
Hotplug -------
Both primary and standby device can be hotplugged via the QEMU monitor. Note that if the virtio-net device is plugged first a warning will be issued that it couldn't find the primary device.
So maybe this whole primary device lookup can happen during the -device CLI option creation loop. And we can indeed have un-created devices still in the list ? Damien
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:
On 10/5/21 16:37, Kevin Wolf wrote:
Am 27.09.2021 um 13:39 hat Kevin Wolf geschrieben:
Am 27.09.2021 um 13:06 hat Damien Hedde geschrieben:
On 9/24/21 11:04, Kevin Wolf wrote:
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@redhat.com> --- 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);
Hi Kevin,
I'm wandering if deleting the opts (which remove it from the "device" opts list) is really a no-op ?
It's not exactly a no-op. Previously, the QemuOpts would only be freed when the device is destroying, now we delete it immediately after creating the device. This could matter in some cases.
The one case I was aware of is that QemuOpts used to be responsible for checking for duplicate IDs. Obviously, it can't do this job any more when we call qemu_opts_del() right after creating the device. This is the reason for patch 6.
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
Yes, you are right. These callers probably need to be changed. Going through the command line options rather than looking at the actual device objects that exist doesn't feel entirely clean anyway.
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. >
Just read the following from docs/system/virtio-net-failover.rst
Usage -----
The primary device can be hotplugged or be part of the startup configuration
-device virtio-net-pci,netdev=hostnet1,id=net1, mac=52:54:00:6f:55:cc,bus=root2,failover=on
With the parameter failover=on the VIRTIO_NET_F_STANDBY feature will be enabled.
-device vfio-pci,host=5e:00.2,id=hostdev0,bus=root1, failover_pair_id=net1
failover_pair_id references the id of the virtio-net standby device. This is only for pairing the devices within QEMU. The guest kernel module net_failover will match devices with identical MAC addresses.
Hotplug -------
Both primary and standby device can be hotplugged via the QEMU monitor. Note that if the virtio-net device is plugged first a warning will be issued that it couldn't find the primary device.
So maybe this whole primary device lookup can happen during the -device CLI option creation loop. And we can indeed have un-created devices still in the list ?
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
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
Kevin Wolf <kwolf@redhat.com> wrote:
Am 05.10.2021 um 17:52 hat Damien Hedde geschrieben:
Hi
Usage -----
The primary device can be hotplugged or be part of the startup configuration
-device virtio-net-pci,netdev=hostnet1,id=net1, mac=52:54:00:6f:55:cc,bus=root2,failover=on
With the parameter failover=on the VIRTIO_NET_F_STANDBY feature will be enabled.
-device vfio-pci,host=5e:00.2,id=hostdev0,bus=root1, failover_pair_id=net1
failover_pair_id references the id of the virtio-net standby device. This is only for pairing the devices within QEMU. The guest kernel module net_failover will match devices with identical MAC addresses.
Hotplug -------
Both primary and standby device can be hotplugged via the QEMU monitor. Note that if the virtio-net device is plugged first a warning will be issued that it couldn't find the primary device.
So maybe this whole primary device lookup can happen during the -device CLI option creation loop. And we can indeed have un-created devices still in the list ?
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.
Failover is ... interesting. You have two devices: primary and seconday. seconday is virtio-net, primary can be vfio and some other emulated devices. In the command line, devices can appear on any order, primary then secondary, secondary then primary, or only one of them. You can add (any of them) later in the toplevel. And now, what all this mess is about. We only enable the primary if the guest knows about failover. Otherwise we use only the virtio device (*). The important bit here is that we need to wait until the guest is booted, and the virtio-net driver is loaded, and then it tells us if it understands failover (or not). At that point we decide if we want to "really" create the primary. I know that it abuses device_add() as much as it can be, but I can't see any better way to handle it. We need to be able to "create" a device without showing it to the guest. And later, when we create a different device, and depending of driver support on the guest, we "finish" the creation of the primary device. Any good idea? Later, Juan. *: This changed recently and we can only have the "primary" and not the virtio one, but it doesn't matter on this discussion.
On 06/10/2021 10:21, Juan Quintela wrote:
Kevin Wolf <kwolf@redhat.com> wrote:
Am 05.10.2021 um 17:52 hat Damien Hedde geschrieben:
Hi
Usage -----
The primary device can be hotplugged or be part of the startup configuration
-device virtio-net-pci,netdev=hostnet1,id=net1, mac=52:54:00:6f:55:cc,bus=root2,failover=on
With the parameter failover=on the VIRTIO_NET_F_STANDBY feature will be enabled.
-device vfio-pci,host=5e:00.2,id=hostdev0,bus=root1, failover_pair_id=net1
failover_pair_id references the id of the virtio-net standby device. This is only for pairing the devices within QEMU. The guest kernel module net_failover will match devices with identical MAC addresses.
Hotplug -------
Both primary and standby device can be hotplugged via the QEMU monitor. Note that if the virtio-net device is plugged first a warning will be issued that it couldn't find the primary device.
So maybe this whole primary device lookup can happen during the -device CLI option creation loop. And we can indeed have un-created devices still in the list ?
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.
Failover is ... interesting.
You have two devices: primary and seconday. seconday is virtio-net, primary can be vfio and some other emulated devices.
In the command line, devices can appear on any order, primary then secondary, secondary then primary, or only one of them. You can add (any of them) later in the toplevel.
And now, what all this mess is about. We only enable the primary if the guest knows about failover. Otherwise we use only the virtio device (*). The important bit here is that we need to wait until the guest is booted, and the virtio-net driver is loaded, and then it tells us if it understands failover (or not). At that point we decide if we want to "really" create the primary.
I know that it abuses device_add() as much as it can be, but I can't see any better way to handle it. We need to be able to "create" a device without showing it to the guest. And later, when we create a different device, and depending of driver support on the guest, we "finish" the creation of the primary device.
Any good idea?
I don't know if it can help the discussion, but I'm reformatting the failover code to move all the PCI stuff to pci files. And there is a lot of inconsistencies regarding the device_add and --device option so I've been in the end to add a list of of hidden devices rather than relying on the command line. See PATCH 8 of series "[RFC PATCH v2 0/8] virtio-net failover cleanup and new features" https://patchew.org/QEMU/20210820142002.152994-1-lvivier@redhat.com/ Thanks, Laurent
Am 06.10.2021 um 11:20 hat Laurent Vivier geschrieben:
On 06/10/2021 10:21, Juan Quintela wrote:
Kevin Wolf <kwolf@redhat.com> wrote:
Am 05.10.2021 um 17:52 hat Damien Hedde geschrieben:
Hi
Usage -----
The primary device can be hotplugged or be part of the startup configuration
-device virtio-net-pci,netdev=hostnet1,id=net1, mac=52:54:00:6f:55:cc,bus=root2,failover=on
With the parameter failover=on the VIRTIO_NET_F_STANDBY feature will be enabled.
-device vfio-pci,host=5e:00.2,id=hostdev0,bus=root1, failover_pair_id=net1
failover_pair_id references the id of the virtio-net standby device. This is only for pairing the devices within QEMU. The guest kernel module net_failover will match devices with identical MAC addresses.
Hotplug -------
Both primary and standby device can be hotplugged via the QEMU monitor. Note that if the virtio-net device is plugged first a warning will be issued that it couldn't find the primary device.
So maybe this whole primary device lookup can happen during the -device CLI option creation loop. And we can indeed have un-created devices still in the list ?
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.
Failover is ... interesting.
You have two devices: primary and seconday. seconday is virtio-net, primary can be vfio and some other emulated devices.
In the command line, devices can appear on any order, primary then secondary, secondary then primary, or only one of them. You can add (any of them) later in the toplevel.
And now, what all this mess is about. We only enable the primary if the guest knows about failover. Otherwise we use only the virtio device (*). The important bit here is that we need to wait until the guest is booted, and the virtio-net driver is loaded, and then it tells us if it understands failover (or not). At that point we decide if we want to "really" create the primary.
I know that it abuses device_add() as much as it can be, but I can't see any better way to handle it. We need to be able to "create" a device without showing it to the guest. And later, when we create a different device, and depending of driver support on the guest, we "finish" the creation of the primary device.
Any good idea?
Hm, the naive idea would be creating the device without attaching it to any bus. But I suppose qdev doesn't let you do that. Anyway, the part that I missed yesterday is that qdev_device_add() already skips creating the device if qdev_should_hide_device(), which explains how the inconsistency is created. (As an aside, it then returns NULL without setting an error to indicate success, which is an awkward interface, and sure enough, qmp_device_add() gets it wrong and deletes the QemuOpts again. So hotplugging the virtio-net standby device doesn't even seem to work?) Could we just save the configuration in the .hide_device callback (i.e. failover_hide_primary_device() in virtio-net) to a new field in VirtIONet and then use that when actually creating the device instead of accessing the command line state in the QemuOptsList? It seems that we can currently add two primary devices that are then both hidden. failover_add_primary() adds only one of them, leaving the other one hidden. Is this a bug and we should reject such a configuration or do we need to support keeping configurations for multiple primary devices in a single standby device? This would still be ugly because the configuration is only really validated when the primary device is actually added instead of immediately on -device/device_add, but at least it would keep the ugliness more local and wouldn't block the move away from QemuOpts (the config would just be stored as a QDict after my patches).
I don't know if it can help the discussion, but I'm reformatting the failover code to move all the PCI stuff to pci files.
And there is a lot of inconsistencies regarding the device_add and --device option so I've been in the end to add a list of of hidden devices rather than relying on the command line.
See PATCH 8 of series "[RFC PATCH v2 0/8] virtio-net failover cleanup and new features"
https://patchew.org/QEMU/20210820142002.152994-1-lvivier@redhat.com/
While it's certainly an improvement over the current state, we really should move away from QemuOpts and I think using global state for this is wrong anyway. So it feels like it's not the change we need here, but more a step sideways. But thanks for mentioning this series here, we might get some merge conflicts there. I'll try to remember to CC you for v2 of this series. Kevin
On 06/10/2021 12:53, Kevin Wolf wrote:
Am 06.10.2021 um 11:20 hat Laurent Vivier geschrieben:
On 06/10/2021 10:21, Juan Quintela wrote:
Kevin Wolf <kwolf@redhat.com> wrote:
Am 05.10.2021 um 17:52 hat Damien Hedde geschrieben:
Hi
Usage -----
The primary device can be hotplugged or be part of the startup configuration
-device virtio-net-pci,netdev=hostnet1,id=net1, mac=52:54:00:6f:55:cc,bus=root2,failover=on
With the parameter failover=on the VIRTIO_NET_F_STANDBY feature will be enabled.
-device vfio-pci,host=5e:00.2,id=hostdev0,bus=root1, failover_pair_id=net1
failover_pair_id references the id of the virtio-net standby device. This is only for pairing the devices within QEMU. The guest kernel module net_failover will match devices with identical MAC addresses.
Hotplug -------
Both primary and standby device can be hotplugged via the QEMU monitor. Note that if the virtio-net device is plugged first a warning will be issued that it couldn't find the primary device.
So maybe this whole primary device lookup can happen during the -device CLI option creation loop. And we can indeed have un-created devices still in the list ?
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.
Failover is ... interesting.
You have two devices: primary and seconday. seconday is virtio-net, primary can be vfio and some other emulated devices.
In the command line, devices can appear on any order, primary then secondary, secondary then primary, or only one of them. You can add (any of them) later in the toplevel.
And now, what all this mess is about. We only enable the primary if the guest knows about failover. Otherwise we use only the virtio device (*). The important bit here is that we need to wait until the guest is booted, and the virtio-net driver is loaded, and then it tells us if it understands failover (or not). At that point we decide if we want to "really" create the primary.
I know that it abuses device_add() as much as it can be, but I can't see any better way to handle it. We need to be able to "create" a device without showing it to the guest. And later, when we create a different device, and depending of driver support on the guest, we "finish" the creation of the primary device.
Any good idea?
Hm, the naive idea would be creating the device without attaching it to any bus. But I suppose qdev doesn't let you do that.
Anyway, the part that I missed yesterday is that qdev_device_add() already skips creating the device if qdev_should_hide_device(), which explains how the inconsistency is created.
(As an aside, it then returns NULL without setting an error to indicate success, which is an awkward interface, and sure enough, qmp_device_add() gets it wrong and deletes the QemuOpts again. So hotplugging the virtio-net standby device doesn't even seem to work?)
Could we just save the configuration in the .hide_device callback (i.e. failover_hide_primary_device() in virtio-net) to a new field in VirtIONet and then use that when actually creating the device instead of accessing the command line state in the QemuOptsList?
It seems that we can currently add two primary devices that are then both hidden. failover_add_primary() adds only one of them, leaving the other one hidden. Is this a bug and we should reject such a configuration or do we need to support keeping configurations for multiple primary devices in a single standby device?
This would still be ugly because the configuration is only really validated when the primary device is actually added instead of immediately on -device/device_add, but at least it would keep the ugliness more local and wouldn't block the move away from QemuOpts (the config would just be stored as a QDict after my patches).
I don't know if it can help the discussion, but I'm reformatting the failover code to move all the PCI stuff to pci files.
And there is a lot of inconsistencies regarding the device_add and --device option so I've been in the end to add a list of of hidden devices rather than relying on the command line.
See PATCH 8 of series "[RFC PATCH v2 0/8] virtio-net failover cleanup and new features"
https://patchew.org/QEMU/20210820142002.152994-1-lvivier@redhat.com/
While it's certainly an improvement over the current state, we really should move away from QemuOpts and I think using global state for this
I totally agree with that.
is wrong anyway. So it feels like it's not the change we need here, but more a step sideways.
Yes, I wanted to fix the problem without modifying to much the existing code.
But thanks for mentioning this series here, we might get some merge conflicts there. I'll try to remember to CC you for v2 of this series.
Thank you. I'll try to find a better solution based on your series. Laurent
On Fri, Sep 24, 2021 at 11:04:25 +0200, Kevin Wolf wrote:
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.
Sorry for chiming in a bit late, but preferrably this commit should be postponed to at least the next release so that we decrease the amount of libvirt users broken by this. Granted users are supposed to use new libvirt with new qemu but that's not always the case. Anyways, libvirt is currently mangling all the types to strings with device_add. I'm currently working on fixing it and it will hopefully be done until next-month's release, but preferrably we increase the window of working combinations by postponing this until the next release.
On 10/1/21 16:42, Peter Krempa wrote:
On Fri, Sep 24, 2021 at 11:04:25 +0200, Kevin Wolf wrote:
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.
Sorry for chiming in a bit late, but preferrably this commit should be postponed to at least the next release so that we decrease the amount of libvirt users broken by this.
Granted users are supposed to use new libvirt with new qemu but that's not always the case.
Anyways, libvirt is currently mangling all the types to strings with device_add. I'm currently working on fixing it and it will hopefully be done until next-month's release, but preferrably we increase the window of working combinations by postponing this until the next release.
Switching to qdict is really an improvement I think. If we choose to keep legacy behavior working for now, I think we should find a way to still do this switch. Maybe we can temporarily keep the str_to_int and str_to_bool conversion when converting the qdict to the qdev properties afterward ? Damien
Am 04.10.2021 um 14:18 hat Damien Hedde geschrieben:
On 10/1/21 16:42, Peter Krempa wrote:
On Fri, Sep 24, 2021 at 11:04:25 +0200, Kevin Wolf wrote:
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.
Sorry for chiming in a bit late, but preferrably this commit should be postponed to at least the next release so that we decrease the amount of libvirt users broken by this.
Granted users are supposed to use new libvirt with new qemu but that's not always the case.
Anyways, libvirt is currently mangling all the types to strings with device_add. I'm currently working on fixing it and it will hopefully be done until next-month's release, but preferrably we increase the window of working combinations by postponing this until the next release.
Switching to qdict is really an improvement I think.
If we choose to keep legacy behavior working for now, I think we should find a way to still do this switch. Maybe we can temporarily keep the str_to_int and str_to_bool conversion when converting the qdict to the qdev properties afterward?
I guess we can keep the detour through QemuOpts for QMP for now, and make sure that the command line code bypasses this bit and still requires correct types for JSON input. It's only this patch that breaks compatibility with libvirt, patch 8 should still be okay. 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@redhat.com> --- 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
On Fri, Sep 24, 2021 at 11:04:26AM +0200, Kevin Wolf wrote:
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@redhat.com> --- 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'] }
Eventually, we'll get rid of this 'gen':false, but this patch series is already an improvement towards that goal. Reviewed-by: Eric Blake <eblake@redhat.com> -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
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@redhat.com> --- 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
On Fri, Sep 24, 2021 at 11:04:27AM +0200, Kevin Wolf wrote:
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@redhat.com> --- docs/about/deprecated.rst | 11 +++++++++++ 1 file changed, 11 insertions(+)
Reviewed-by: Eric Blake <eblake@redhat.com>
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
-- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
On 24/09/21 11:04, Kevin Wolf wrote:
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.
Maybe we need a different section for unstable syntaxes, rather than overloading deprecated.rst? Paolo
On Mon, Sep 27, 2021 at 10:15:43AM +0200, Paolo Bonzini wrote:
On 24/09/21 11:04, Kevin Wolf wrote:
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.
Maybe we need a different section for unstable syntaxes, rather than overloading deprecated.rst?
This case feels like it hits two scenarios - we want to declare it unstable, which is something we should document in qemu-options.hx. We want to also to warn of specific breakage when the impl changes which is something suitable for deprecations. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Am 27.09.2021 um 10:21 hat Daniel P. Berrangé geschrieben:
On Mon, Sep 27, 2021 at 10:15:43AM +0200, Paolo Bonzini wrote:
On 24/09/21 11:04, Kevin Wolf wrote:
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.
Maybe we need a different section for unstable syntaxes, rather than overloading deprecated.rst?
This case feels like it hits two scenarios - we want to declare it unstable, which is something we should document in qemu-options.hx.
Actually, I think a section for unstable syntaxes or generally compatibility promises wouldn't hurt. When I checked, I couldn't find any documentation about the support status of HMP either. Basically, I imagine HMP and non-JSON -device/-object would be on the same level: We don't change things without a reason, but if we do want to change things, compatibility isn't an argument against making the change.
We want to also to warn of specific breakage when the impl changes which is something suitable for deprecations.
We don't do this for HMP either for individual changes. Basically this deprecation notice was meant to make people aware that we're lowering the support status from a long-term stable interface to HMP-like. Kevin
On Mon, Sep 27, 2021 at 12:17:03PM +0200, Kevin Wolf wrote:
Am 27.09.2021 um 10:21 hat Daniel P. Berrangé geschrieben:
On Mon, Sep 27, 2021 at 10:15:43AM +0200, Paolo Bonzini wrote:
On 24/09/21 11:04, Kevin Wolf wrote:
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.
Maybe we need a different section for unstable syntaxes, rather than overloading deprecated.rst?
This case feels like it hits two scenarios - we want to declare it unstable, which is something we should document in qemu-options.hx.
Actually, I think a section for unstable syntaxes or generally compatibility promises wouldn't hurt. When I checked, I couldn't find any documentation about the support status of HMP either.
Basically, I imagine HMP and non-JSON -device/-object would be on the same level: We don't change things without a reason, but if we do want to change things, compatibility isn't an argument against making the change.
We want to also to warn of specific breakage when the impl changes which is something suitable for deprecations.
We don't do this for HMP either for individual changes.
Well HMP as a whole is considered non-stable, so we don't need to call out individual things. We've got a simple story that QMP == stable, HMP == unstable. The comparison here would be if we declared the entire QEMU CLI to be unstable, except for JSON syntax args.
Basically this deprecation notice was meant to make people aware that we're lowering the support status from a long-term stable interface to HMP-like.
Bearing in mind our previous discussions it feels like our goal is that we're tending towards a world where we are only wanting to consider JSON based configuration to be stable, and everything else non-stable. I think that's a good long term plan, but if we're really doing that then I think we need to big picture explain it in our docs rather than mention it in passing against individual args. BTW I'm also not a fan of deprecating stuff when our documentation is still using the deprecated syntax and nothing shows the new preferred syntax. We've got alot of results for $ git grep -- ' -object' Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Fri, 24 Sept 2021 at 10:14, Kevin Wolf <kwolf@redhat.com> wrote:
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@redhat.com>
+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.
As it stands, this is basically saying "pretty much anybody using the command line, your stuff may break in future, instead use some other interface you've never heard of, which doesn't appear to be documented in the manual and which none of the documentation's examples use". Is there some more limited deprecation we can do rather than "the entire commandline for almost all users" ? thanks -- PMM
Am 27.09.2021 um 11:00 hat Peter Maydell geschrieben:
On Fri, 24 Sept 2021 at 10:14, Kevin Wolf <kwolf@redhat.com> wrote:
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@redhat.com>
+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.
As it stands, this is basically saying "pretty much anybody using the command line, your stuff may break in future, instead use some other interface you've never heard of, which doesn't appear to be documented in the manual and which none of the documentation's examples use".
The documentation is a valid criticism. We need to document the JSON interfaces properly (which will really mostly be a pointer to the existing QMP documentation at least for -object, but it's important to tell people where to look for the details).
Is there some more limited deprecation we can do rather than "the entire commandline for almost all users" ?
I don't think "almost all" users is true. I see three groups of users: 1. Using a management tool that is probably using libvirt. This is likely the vast majority of users. They won't notice a difference because libvirt abstracts it away. libvirt developers are actively asking us for JSON (and QAPI) based interfaces because using the same representation to describe configurations in QMP and on the CLI makes their life easier. 2. People starting QEMU on the command line manually. This is essentially the same situation as HMP: If something changes, you get an error message, you look up the new syntax, done. Small inconvenience, but that's it. This includes simple scripts that just start QEMU and are only used to store a long command line somewhere. 3. People writing more complex scripts or applications that invoke QEMU manually instead of using libvirt. They may actually be hurt by such changes. They should probably be using a proper machine interface, i.e. JSON mode, so the deprecation notice is for them to change their code. This is probably a small minority and not "almost all users". Yes, we could in theory do a more limited deprecation. The planned change from my side is just going from QemuOpts to the keyval parser, which doesn't change anything in the vast majority of cases. But we have the separation in the monitor between QMP and HMP for a good reason: Requirements for a good machine interface are different from a good human interface. The same is true for the command line. So it seems to make a lot of sense to me to have both a machine interface (JSON based) and a human interface (non-JSON) on the command line, too, and take the same liberties for evolving the human interface as we already do in HMP - which means that it's technically an unstable interface where compatibility doesn't prevent improvements, but not that it looks completely different in every QEMU version. Kevin
On Mon, 27 Sept 2021 at 12:27, Kevin Wolf <kwolf@redhat.com> wrote:
Am 27.09.2021 um 11:00 hat Peter Maydell geschrieben:
On Fri, 24 Sept 2021 at 10:14, Kevin Wolf <kwolf@redhat.com> wrote:
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@redhat.com>
+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.
As it stands, this is basically saying "pretty much anybody using the command line, your stuff may break in future, instead use some other interface you've never heard of, which doesn't appear to be documented in the manual and which none of the documentation's examples use".
The documentation is a valid criticism. We need to document the JSON interfaces properly (which will really mostly be a pointer to the existing QMP documentation at least for -object, but it's important to tell people where to look for the details).
Is there some more limited deprecation we can do rather than "the entire commandline for almost all users" ?
I don't think "almost all" users is true.
I see three groups of users
...all of whom "rely on a stable interface for -device and -object", and only two of whom it's reasonable to say "use the JSON version" to.
1. Using a management tool that is probably using libvirt. This is likely the vast majority of users. They won't notice a difference because libvirt abstracts it away. libvirt developers are actively asking us for JSON (and QAPI) based interfaces because using the same representation to describe configurations in QMP and on the CLI makes their life easier.
Yes, absolutely we should be recommending that libvirt and other management interfaces use the JSON.
2. People starting QEMU on the command line manually. This is essentially the same situation as HMP: If something changes, you get an error message, you look up the new syntax, done. Small inconvenience, but that's it. This includes simple scripts that just start QEMU and are only used to store a long command line somewhere.
It's a small inconvenience that we seem to be imposing on our users on a pretty frequent basis. Moreover, each one of these really needs to be its own deprecation, so that users actually can have some advance notice if they need it and look up the new syntax. We shouldn't hide them all under this umbrella "we might break anything at any time" entry, which I think will pretty much encourage breaking compatibility more often because you don't have to think about "oh, we should deprecate this and maybe print warnings about use of deprecated syntax and then drop it later", you can just break things and point to this "we said -device wasn't going to be stable" entry. As a concrete example, the commit message for this patch vaguely mentions some issues "around list handling", which gives me no idea at all about what syntax is going to break in future or whether it is likely to affect scripts I've written.
3. People writing more complex scripts or applications that invoke QEMU manually instead of using libvirt. They may actually be hurt by such changes. They should probably be using a proper machine interface, i.e. JSON mode, so the deprecation notice is for them to change their code. This is probably a small minority and not "almost all users".
Yeah, this group is kind of similar to group 1 (well, at one end it shades into group 1 and at the other into group 2). More generally, I think I'd rather see the deprecation of the old approach appear after some period when the JSON command line version has been available to users and adopted by major consumers like libvirt, not as a patch in the same series which is introducing the JSON syntax in the first plaec. -- PMM
Am 27.09.2021 um 14:52 hat Peter Maydell geschrieben:
On Mon, 27 Sept 2021 at 12:27, Kevin Wolf <kwolf@redhat.com> wrote:
Am 27.09.2021 um 11:00 hat Peter Maydell geschrieben:
On Fri, 24 Sept 2021 at 10:14, Kevin Wolf <kwolf@redhat.com> wrote:
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@redhat.com>
+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.
As it stands, this is basically saying "pretty much anybody using the command line, your stuff may break in future, instead use some other interface you've never heard of, which doesn't appear to be documented in the manual and which none of the documentation's examples use".
The documentation is a valid criticism. We need to document the JSON interfaces properly (which will really mostly be a pointer to the existing QMP documentation at least for -object, but it's important to tell people where to look for the details).
Is there some more limited deprecation we can do rather than "the entire commandline for almost all users" ?
I don't think "almost all" users is true.
I see three groups of users
...all of whom "rely on a stable interface for -device and -object", and only two of whom it's reasonable to say "use the JSON version" to.
I'm not sure that the human interactive case requires unchanged syntax as strictly as the other cases do. After each distro upgrade (or even a browser upgrade within the same distro), some UI changes somewhere and I have to adapt. I don't think anyone ever makes promises like "this button is going to stay in the exact same place forever". And our policy is already that we're not making such promises for HMP either.
1. Using a management tool that is probably using libvirt. This is likely the vast majority of users. They won't notice a difference because libvirt abstracts it away. libvirt developers are actively asking us for JSON (and QAPI) based interfaces because using the same representation to describe configurations in QMP and on the CLI makes their life easier.
Yes, absolutely we should be recommending that libvirt and other management interfaces use the JSON.
2. People starting QEMU on the command line manually. This is essentially the same situation as HMP: If something changes, you get an error message, you look up the new syntax, done. Small inconvenience, but that's it. This includes simple scripts that just start QEMU and are only used to store a long command line somewhere.
It's a small inconvenience that we seem to be imposing on our users on a pretty frequent basis. Moreover, each one of these really needs to be its own deprecation, so that users actually can have some advance notice if they need it and look up the new syntax. We shouldn't hide them all under this umbrella "we might break anything at any time" entry, which I think will pretty much encourage breaking compatibility more often because you don't have to think about "oh, we should deprecate this and maybe print warnings about use of deprecated syntax and then drop it later", you can just break things and point to this "we said -device wasn't going to be stable" entry.
Are you suggesting bringing back stricter compatibility rules for HMP then? The problem with the deprecation period is that you need to have a time where both options work. But when you have two different parsers, you can't just magically have the union of their accepted syntaxes. Unless you invest a lot of time and effort, you have a flag day where the syntax changes. And I think this is perfectly reasonable for interactive use. Deprecations are the wrong tool for human interfaces. You don't need to know months in advance that something will change in the UI (you'll forget it again anyway until the change happens and the information becomes relevant), but this is a case for the changelog. If you do need to know months in advance, JSON is probably better suited for your use case.
As a concrete example, the commit message for this patch vaguely mentions some issues "around list handling", which gives me no idea at all about what syntax is going to break in future or whether it is likely to affect scripts I've written.
The one known incompatible case for -object is the 'host-nodes' property of memory-backend, which is a list. QemuOpts doesn't support lists at all, but the string visitor allows using a comma separated list of integer ranges. (By the way, the existing syntax of it seems to be undocumented, so not sure how many users even know about how to use this list support.) For -device, who knows. I'm positive that the string visitor list support isn't used there and a patch in this series removes the string visitor from -device therefore. What other incompatibilities and hacks there are in some devices, we'll find out when we actually try to describe devices in QAPI. I'm not even entirely sure yet that there will be incompatibilities. But if we do find a case, I don't want to have to stop and delay work for another year to have a deprecation period for interactive human interfaces. Another similar case is -drive: We only still have it in its current form because even figuring out what the exact cases are that would be incompatible is hard enough to stop us from even trying to deprecate them individually. But actually it would seem reasonable enough to me to just change -drive because that makes sense as a convenient option for interactive use, while -blockdev is more verbose and covers the stable API part.
3. People writing more complex scripts or applications that invoke QEMU manually instead of using libvirt. They may actually be hurt by such changes. They should probably be using a proper machine interface, i.e. JSON mode, so the deprecation notice is for them to change their code. This is probably a small minority and not "almost all users".
Yeah, this group is kind of similar to group 1 (well, at one end it shades into group 1 and at the other into group 2).
More generally, I think I'd rather see the deprecation of the old approach appear after some period when the JSON command line version has been available to users and adopted by major consumers like libvirt, not as a patch in the same series which is introducing the JSON syntax in the first plaec.
Okay, that can be done. So I can post this final patch separately and only for -object for now, and we'll do -device separately once libvirt uses JSON for it. Kevin
participants (11)
-
Damien Hedde -
Daniel P. Berrangé -
Eric Blake -
Juan Quintela -
Kevin Wolf -
Laurent Vivier -
Markus Armbruster -
Paolo Bonzini -
Peter Krempa -
Peter Maydell -
Vladimir Sementsov-Ogievskiy