[PATCH 00/11] qdev: Add JSON -device and fix QMP device_add

[PATCH 00/11] qdev: Add JSON -device and fix QMP device_add

[Kevin Wolf]

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

[PATCH 01/11] qom: Reduce use of error_propagate()

[Kevin Wolf]

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

[PATCH 02/11] iotests/245: Fix type for iothread property

[Kevin Wolf]

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

[PATCH 03/11] iotests/051: Fix typo

[Kevin Wolf]

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

[PATCH 04/11] qdev: Avoid using string visitor for properties

[Kevin Wolf]

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

[PATCH 05/11] qdev: Make DeviceState.id independent of QemuOpts

[Kevin Wolf]

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

[PATCH 06/11] qdev: Add Error parameter to qdev_set_id()

[Kevin Wolf]

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

[PATCH 07/11] qemu-option: Allow deleting opts during qemu_opts_foreach()

[Kevin Wolf]

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

[PATCH 08/11] qdev: Base object creation on QDict rather than QemuOpts

[Kevin Wolf]

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

[PATCH 09/11] qdev: Avoid QemuOpts in QMP device_add

[Kevin Wolf]

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

[PATCH 10/11] vl: Enable JSON syntax for -device

[Kevin Wolf]

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

[PATCH 11/11] Deprecate stable non-JSON -device and -object

[Kevin Wolf]

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

Re: [PATCH 01/11] qom: Reduce use of error_propagate()

[Vladimir Sementsov-Ogievskiy]

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

Re: [PATCH 02/11] iotests/245: Fix type for iothread property

[Vladimir Sementsov-Ogievskiy]

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

Re: [PATCH 03/11] iotests/051: Fix typo

[Vladimir Sementsov-Ogievskiy]

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

Re: [PATCH 05/11] qdev: Make DeviceState.id independent of QemuOpts

[Vladimir Sementsov-Ogievskiy]

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

Re: [PATCH 01/11] qom: Reduce use of error_propagate()

[Markus Armbruster]

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>

Re: [PATCH 06/11] qdev: Add Error parameter to qdev_set_id()

[Vladimir Sementsov-Ogievskiy]

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

Re: [PATCH 07/11] qemu-option: Allow deleting opts during qemu_opts_foreach()

[Vladimir Sementsov-Ogievskiy]

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

Re: [PATCH 05/11] qdev: Make DeviceState.id independent of QemuOpts

[Kevin Wolf]

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

Re: [PATCH 05/11] qdev: Make DeviceState.id independent of QemuOpts

[Vladimir Sementsov-Ogievskiy]

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

Re: [PATCH 01/11] qom: Reduce use of error_propagate()

[Eric Blake]

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

Re: [PATCH 04/11] qdev: Avoid using string visitor for properties

[Eric Blake]

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

Re: [PATCH 08/11] qdev: Base object creation on QDict rather than QemuOpts

[Eric Blake]

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

Re: [PATCH 09/11] qdev: Avoid QemuOpts in QMP device_add

[Eric Blake]

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

Re: [PATCH 10/11] vl: Enable JSON syntax for -device

[Eric Blake]

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

Re: [PATCH 11/11] Deprecate stable non-JSON -device and -object

[Eric Blake]

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

Re: [PATCH 11/11] Deprecate stable non-JSON -device and -object

[Paolo Bonzini]

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

Re: [PATCH 11/11] Deprecate stable non-JSON -device and -object

[Daniel P. Berrangé]

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 :|

Re: [PATCH 11/11] Deprecate stable non-JSON -device and -object

[Peter Maydell]

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

Re: [PATCH 11/11] Deprecate stable non-JSON -device and -object

[Kevin Wolf]

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

Re: [PATCH 06/11] qdev: Add Error parameter to qdev_set_id()

[Damien Hedde]

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)) {
>>
> 
> 

Re: [PATCH 11/11] Deprecate stable non-JSON -device and -object

[Daniel P. Berrangé]

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 :|

Re: [PATCH 09/11] qdev: Avoid QemuOpts in QMP device_add

[Damien Hedde]

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

Re: [PATCH 11/11] Deprecate stable non-JSON -device and -object

[Kevin Wolf]

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

Re: [PATCH 09/11] qdev: Avoid QemuOpts in QMP device_add

[Kevin Wolf]

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

Re: [PATCH 11/11] Deprecate stable non-JSON -device and -object

[Peter Maydell]

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

Re: [PATCH 11/11] Deprecate stable non-JSON -device and -object

[Kevin Wolf]

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

Re: [PATCH 09/11] qdev: Avoid QemuOpts in QMP device_add

[Peter Krempa]

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.

Re: [PATCH 09/11] qdev: Avoid QemuOpts in QMP device_add

[Damien Hedde]

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

Re: [PATCH 09/11] qdev: Avoid QemuOpts in QMP device_add

[Kevin Wolf]

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

Re: [PATCH 06/11] qdev: Add Error parameter to qdev_set_id()

[Kevin Wolf]

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

Re: [PATCH 09/11] qdev: Avoid QemuOpts in QMP device_add

[Kevin Wolf]

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

Re: [PATCH 09/11] qdev: Avoid QemuOpts in QMP device_add

[Damien Hedde]

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
> 

Re: [PATCH 09/11] qdev: Avoid QemuOpts in QMP device_add

[Kevin Wolf]

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
> > 
> 

Re: [PATCH 09/11] qdev: Avoid QemuOpts in QMP device_add

[Juan Quintela]

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.

Re: [PATCH 09/11] qdev: Avoid QemuOpts in QMP device_add

[Laurent Vivier]

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

Re: [PATCH 09/11] qdev: Avoid QemuOpts in QMP device_add

[Kevin Wolf]

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

Re: [PATCH 09/11] qdev: Avoid QemuOpts in QMP device_add

[Laurent Vivier]

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