[PULL 00/15] qdev: Add JSON -device

[PULL 00/15] qdev: Add JSON -device

[Kevin Wolf]

The following changes since commit 4d1a525dfafe995a98bb486e702da09e31b68b9c:

  Merge remote-tracking branch 'remotes/bonzini/tags/for-upstream' into staging (2021-10-14 10:49:38 -0700)

are available in the Git repository at:

  git://repo.or.cz/qemu/kevin.git tags/for-upstream

for you to fetch changes up to 5dacda5167560b3af8eadbce5814f60ba44b467e:

  vl: Enable JSON syntax for -device (2021-10-15 16:11:22 +0200)

----------------------------------------------------------------
qdev: Add JSON -device

- Add a JSON mode to the -device command line option
- net/vhost-{user,vdpa}: Fix device compatibility check
- Minor iotests fixes

----------------------------------------------------------------
Damien Hedde (1):
      softmmu/qdev-monitor: add error handling in qdev_set_id

Kevin Wolf (14):
      net: Introduce NetClientInfo.check_peer_type()
      net/vhost-user: Fix device compatibility check
      net/vhost-vdpa: Fix device compatibility check
      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
      qemu-option: Allow deleting opts during qemu_opts_foreach()
      qdev: Add Error parameter to hide_device() callbacks
      virtio-net: Store failover primary opts pointer locally
      virtio-net: Avoid QemuOpts in failover_find_primary_device()
      qdev: Base object creation on QDict rather than QemuOpts
      vl: Enable JSON syntax for -device

 qapi/qdev.json                      |  15 ++++--
 include/hw/qdev-core.h              |  16 ++++--
 include/hw/virtio/virtio-net.h      |   2 +
 include/monitor/qdev.h              |  27 +++++++++-
 include/net/net.h                   |   2 +
 hw/arm/virt.c                       |   2 +-
 hw/core/qdev-properties-system.c    |   6 +++
 hw/core/qdev.c                      |  11 ++--
 hw/net/virtio-net.c                 |  85 +++++++++++++++---------------
 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 +-
 net/vhost-user.c                    |  41 +++++----------
 net/vhost-vdpa.c                    |  37 +++++--------
 qom/object.c                        |   7 ++-
 qom/object_interfaces.c             |  19 +++----
 softmmu/qdev-monitor.c              | 100 +++++++++++++++++++++++-------------
 softmmu/vl.c                        |  63 ++++++++++++++++++++---
 util/qemu-option.c                  |   4 +-
 tests/qemu-iotests/051              |   2 +-
 tests/qemu-iotests/051.pc.out       |   4 +-
 tests/qemu-iotests/245              |   4 +-
 23 files changed, 280 insertions(+), 178 deletions(-)

[PULL 01/15] net: Introduce NetClientInfo.check_peer_type()

[Kevin Wolf]

Some network backends (vhost-user and vhost-vdpa) work only with
specific devices. At startup, they second guess what the command line
option handling will do and error out if they think a non-virtio device
will attach to them.

This second guessing is not only ugly, it can lead to wrong error
messages ('-device floppy,netdev=foo' should complain about an unknown
property, not about the wrong kind of network device being attached) and
completely ignores hotplugging.

Add a callback where backends can check compatibility with a device when
it actually tries to attach, even on hotplug.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Message-Id: <20211008133442.141332-2-kwolf@redhat.com>
Reviewed-by: Damien Hedde <damien.hedde@greensocs.com>
Acked-by: Jason Wang <jasowang@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Tested-by: Peter Krempa <pkrempa@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 include/net/net.h                | 2 ++
 hw/core/qdev-properties-system.c | 6 ++++++
 2 files changed, 8 insertions(+)

diff --git a/include/net/net.h b/include/net/net.h
index 5d1508081f..986288eb07 100644
--- a/include/net/net.h
+++ b/include/net/net.h
@@ -62,6 +62,7 @@ typedef struct SocketReadState SocketReadState;
 typedef void (SocketReadStateFinalize)(SocketReadState *rs);
 typedef void (NetAnnounce)(NetClientState *);
 typedef bool (SetSteeringEBPF)(NetClientState *, int);
+typedef bool (NetCheckPeerType)(NetClientState *, ObjectClass *, Error **);
 
 typedef struct NetClientInfo {
     NetClientDriver type;
@@ -84,6 +85,7 @@ typedef struct NetClientInfo {
     SetVnetBE *set_vnet_be;
     NetAnnounce *announce;
     SetSteeringEBPF *set_steering_ebpf;
+    NetCheckPeerType *check_peer_type;
 } NetClientInfo;
 
 struct NetClientState {
diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c
index e71f5d64d1..a91f60567a 100644
--- a/hw/core/qdev-properties-system.c
+++ b/hw/core/qdev-properties-system.c
@@ -431,6 +431,12 @@ static void set_netdev(Object *obj, Visitor *v, const char *name,
             goto out;
         }
 
+        if (peers[i]->info->check_peer_type) {
+            if (!peers[i]->info->check_peer_type(peers[i], obj->class, errp)) {
+                goto out;
+            }
+        }
+
         ncs[i] = peers[i];
         ncs[i]->queue_index = i;
     }
-- 
2.31.1

[PULL 02/15] net/vhost-user: Fix device compatibility check

[Kevin Wolf]

vhost-user works only with specific devices. At startup, it second
guesses what the command line option handling will do and error out if
it thinks a non-virtio device will attach to them.

This second guessing is not only ugly, it can lead to wrong error
messages ('-device floppy,netdev=foo' should complain about an unknown
property, not about the wrong kind of network device being attached) and
completely ignores hotplugging.

Drop the old checks and implement .check_peer_type() instead to fix
this. As a nice side effect, it also removes one more dependency on the
legacy QemuOpts infrastructure and even reduces the code size.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Message-Id: <20211008133442.141332-3-kwolf@redhat.com>
Reviewed-by: Damien Hedde <damien.hedde@greensocs.com>
Acked-by: Jason Wang <jasowang@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Tested-by: Peter Krempa <pkrempa@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 net/vhost-user.c | 41 ++++++++++++++---------------------------
 1 file changed, 14 insertions(+), 27 deletions(-)

diff --git a/net/vhost-user.c b/net/vhost-user.c
index 4a939124d2..b1a0247b59 100644
--- a/net/vhost-user.c
+++ b/net/vhost-user.c
@@ -198,6 +198,19 @@ static bool vhost_user_has_ufo(NetClientState *nc)
     return true;
 }
 
+static bool vhost_user_check_peer_type(NetClientState *nc, ObjectClass *oc,
+                                       Error **errp)
+{
+    const char *driver = object_class_get_name(oc);
+
+    if (!g_str_has_prefix(driver, "virtio-net-")) {
+        error_setg(errp, "vhost-user requires frontend driver virtio-net-*");
+        return false;
+    }
+
+    return true;
+}
+
 static NetClientInfo net_vhost_user_info = {
         .type = NET_CLIENT_DRIVER_VHOST_USER,
         .size = sizeof(NetVhostUserState),
@@ -207,6 +220,7 @@ static NetClientInfo net_vhost_user_info = {
         .has_ufo = vhost_user_has_ufo,
         .set_vnet_be = vhost_user_set_vnet_endianness,
         .set_vnet_le = vhost_user_set_vnet_endianness,
+        .check_peer_type = vhost_user_check_peer_type,
 };
 
 static gboolean net_vhost_user_watch(void *do_not_use, GIOCondition cond,
@@ -397,27 +411,6 @@ static Chardev *net_vhost_claim_chardev(
     return chr;
 }
 
-static int net_vhost_check_net(void *opaque, QemuOpts *opts, Error **errp)
-{
-    const char *name = opaque;
-    const char *driver, *netdev;
-
-    driver = qemu_opt_get(opts, "driver");
-    netdev = qemu_opt_get(opts, "netdev");
-
-    if (!driver || !netdev) {
-        return 0;
-    }
-
-    if (strcmp(netdev, name) == 0 &&
-        !g_str_has_prefix(driver, "virtio-net-")) {
-        error_setg(errp, "vhost-user requires frontend driver virtio-net-*");
-        return -1;
-    }
-
-    return 0;
-}
-
 int net_init_vhost_user(const Netdev *netdev, const char *name,
                         NetClientState *peer, Error **errp)
 {
@@ -433,12 +426,6 @@ int net_init_vhost_user(const Netdev *netdev, const char *name,
         return -1;
     }
 
-    /* verify net frontend */
-    if (qemu_opts_foreach(qemu_find_opts("device"), net_vhost_check_net,
-                          (char *)name, errp)) {
-        return -1;
-    }
-
     queues = vhost_user_opts->has_queues ? vhost_user_opts->queues : 1;
     if (queues < 1 || queues > MAX_QUEUE_NUM) {
         error_setg(errp,
-- 
2.31.1

[PULL 03/15] net/vhost-vdpa: Fix device compatibility check

[Kevin Wolf]

vhost-vdpa works only with specific devices. At startup, it second
guesses what the command line option handling will do and error out if
it thinks a non-virtio device will attach to them.

This second guessing is not only ugly, it can lead to wrong error
messages ('-device floppy,netdev=foo' should complain about an unknown
property, not about the wrong kind of network device being attached) and
completely ignores hotplugging.

Drop the old checks and implement .check_peer_type() instead to fix
this. As a nice side effect, it also removes one more dependency on the
legacy QemuOpts infrastructure and even reduces the code size.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Message-Id: <20211008133442.141332-4-kwolf@redhat.com>
Reviewed-by: Damien Hedde <damien.hedde@greensocs.com>
Acked-by: Jason Wang <jasowang@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Tested-by: Peter Krempa <pkrempa@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 net/vhost-vdpa.c | 37 ++++++++++++++-----------------------
 1 file changed, 14 insertions(+), 23 deletions(-)

diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
index 912686457c..6dc68d8677 100644
--- a/net/vhost-vdpa.c
+++ b/net/vhost-vdpa.c
@@ -147,12 +147,26 @@ static bool vhost_vdpa_has_ufo(NetClientState *nc)
 
 }
 
+static bool vhost_vdpa_check_peer_type(NetClientState *nc, ObjectClass *oc,
+                                       Error **errp)
+{
+    const char *driver = object_class_get_name(oc);
+
+    if (!g_str_has_prefix(driver, "virtio-net-")) {
+        error_setg(errp, "vhost-vdpa requires frontend driver virtio-net-*");
+        return false;
+    }
+
+    return true;
+}
+
 static NetClientInfo net_vhost_vdpa_info = {
         .type = NET_CLIENT_DRIVER_VHOST_VDPA,
         .size = sizeof(VhostVDPAState),
         .cleanup = vhost_vdpa_cleanup,
         .has_vnet_hdr = vhost_vdpa_has_vnet_hdr,
         .has_ufo = vhost_vdpa_has_ufo,
+        .check_peer_type = vhost_vdpa_check_peer_type,
 };
 
 static int net_vhost_vdpa_init(NetClientState *peer, const char *device,
@@ -179,24 +193,6 @@ static int net_vhost_vdpa_init(NetClientState *peer, const char *device,
     return ret;
 }
 
-static int net_vhost_check_net(void *opaque, QemuOpts *opts, Error **errp)
-{
-    const char *name = opaque;
-    const char *driver, *netdev;
-
-    driver = qemu_opt_get(opts, "driver");
-    netdev = qemu_opt_get(opts, "netdev");
-    if (!driver || !netdev) {
-        return 0;
-    }
-    if (strcmp(netdev, name) == 0 &&
-        !g_str_has_prefix(driver, "virtio-net-")) {
-        error_setg(errp, "vhost-vdpa requires frontend driver virtio-net-*");
-        return -1;
-    }
-    return 0;
-}
-
 int net_init_vhost_vdpa(const Netdev *netdev, const char *name,
                         NetClientState *peer, Error **errp)
 {
@@ -204,10 +200,5 @@ int net_init_vhost_vdpa(const Netdev *netdev, const char *name,
 
     assert(netdev->type == NET_CLIENT_DRIVER_VHOST_VDPA);
     opts = &netdev->u.vhost_vdpa;
-    /* verify net frontend */
-    if (qemu_opts_foreach(qemu_find_opts("device"), net_vhost_check_net,
-                          (char *)name, errp)) {
-        return -1;
-    }
     return net_vhost_vdpa_init(peer, TYPE_VHOST_VDPA, name, opts->vhostdev);
 }
-- 
2.31.1

[PULL 04/15] 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>
Message-Id: <20211008133442.141332-5-kwolf@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Tested-by: Peter Krempa <pkrempa@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 qom/object.c            |  7 +++----
 qom/object_interfaces.c | 19 ++++++-------------
 2 files changed, 9 insertions(+), 17 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..3b61c195c5 100644
--- a/qom/object_interfaces.c
+++ b/qom/object_interfaces.c
@@ -46,25 +46,18 @@ static void object_set_properties_from_qdict(Object *obj, const QDict *qdict,
                                              Visitor *v, Error **errp)
 {
     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)) {
-            break;
+        if (!object_property_set(obj, e->key, v, errp)) {
+            goto out;
         }
     }
-    if (!local_err) {
-        visit_check_struct(v, &local_err);
-    }
-    visit_end_struct(v, NULL);
-
+    visit_check_struct(v, errp);
 out:
-    if (local_err) {
-        error_propagate(errp, local_err);
-    }
+    visit_end_struct(v, NULL);
 }
 
 void object_set_properties_from_keyval(Object *obj, const QDict *qdict,
-- 
2.31.1

[PULL 05/15] 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>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Message-Id: <20211008133442.141332-6-kwolf@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Tested-by: Peter Krempa <pkrempa@redhat.com>
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

[PULL 06/15] 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>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Message-Id: <20211008133442.141332-7-kwolf@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Tested-by: Peter Krempa <pkrempa@redhat.com>
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

[PULL 07/15] 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>
Reviewed-by: Eric Blake <eblake@redhat.com>
Message-Id: <20211008133442.141332-8-kwolf@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Tested-by: Peter Krempa <pkrempa@redhat.com>
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 3df99ce9fc..672f87ed4f 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

[PULL 08/15] 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>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Message-Id: <20211008133442.141332-9-kwolf@redhat.com>
Reviewed-by: Damien Hedde <damien.hedde@greensocs.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Tested-by: Peter Krempa <pkrempa@redhat.com>
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 4ff19c714b..5a073fc368 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 7170aaacd5..4160d49688 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 672f87ed4f..b7c2d69207 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

[PULL 09/15] softmmu/qdev-monitor: add error handling in qdev_set_id

[Kevin Wolf]

From: Damien Hedde <damien.hedde@greensocs.com>

qdev_set_id() is mostly used when the user adds a device (using
-device cli option or device_add qmp command). This commit adds
an error parameter to handle the case where the given id is
already taken.

Also document the function and add a return value in order to
be able to capture success/failure: the function now returns the
id in case of success, or NULL in case of failure.

The commit modifies the 2 calling places (qdev-monitor and
xen-legacy-backend) to add the error object parameter.

Note that the id is, right now, guaranteed to be unique because
all ids came from the "device" QemuOptsList where the id is used
as key. This addition is a preparation for a future commit which
will relax the uniqueness.

Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Message-Id: <20211008133442.141332-10-kwolf@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Tested-by: Peter Krempa <pkrempa@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 include/monitor/qdev.h      | 25 +++++++++++++++++++++++-
 hw/xen/xen-legacy-backend.c |  3 ++-
 softmmu/qdev-monitor.c      | 39 +++++++++++++++++++++++++++----------
 3 files changed, 55 insertions(+), 12 deletions(-)

diff --git a/include/monitor/qdev.h b/include/monitor/qdev.h
index 389287eb44..74e6c55a2b 100644
--- a/include/monitor/qdev.h
+++ b/include/monitor/qdev.h
@@ -9,6 +9,29 @@ 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);
+
+/**
+ * qdev_set_id: parent the device and set its id if provided.
+ * @dev: device to handle
+ * @id: id to be given to the device, or NULL.
+ *
+ * Returns: the id of the device in case of success; otherwise NULL.
+ *
+ * @dev must be unrealized, unparented and must not have an id.
+ *
+ * If @id is non-NULL, this function tries to setup @dev qom path as
+ * "/peripheral/id". If @id is already taken, it fails. If it succeeds,
+ * the id field of @dev is set to @id (@dev now owns the given @id
+ * parameter).
+ *
+ * If @id is NULL, this function generates a unique name and setups @dev
+ * qom path as "/peripheral-anon/name". This name is not set as the id
+ * of @dev.
+ *
+ * Upon success, it returns the id/name (generated or provided). The
+ * returned string is owned by the corresponding child property and must
+ * not be freed by the caller.
+ */
+const char *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 be3cf4a195..085fd31ef7 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_fatal);
     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 b7c2d69207..0b6833cc57 100644
--- a/softmmu/qdev-monitor.c
+++ b/softmmu/qdev-monitor.c
@@ -593,22 +593,35 @@ 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)
+const char *qdev_set_id(DeviceState *dev, char *id, Error **errp)
 {
-    if (id) {
-        dev->id = id;
-    }
+    ObjectProperty *prop;
 
-    if (dev->id) {
-        object_property_add_child(qdev_get_peripheral(), dev->id,
-                                  OBJECT(dev));
+    assert(!dev->id && !dev->realized);
+
+    /*
+     * object_property_[try_]add_child() below will assert the device
+     * has no parent
+     */
+    if (id) {
+        prop = object_property_try_add_child(qdev_get_peripheral(), id,
+                                             OBJECT(dev), NULL);
+        if (prop) {
+            dev->id = id;
+        } else {
+            g_free(id);
+            error_setg(errp, "Duplicate device ID '%s'", id);
+            return NULL;
+        }
     } 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));
+        prop = object_property_add_child(qdev_get_peripheral_anon(), name,
+                                         OBJECT(dev));
         g_free(name);
     }
+
+    return prop->name;
 }
 
 DeviceState *qdev_device_add(QemuOpts *opts, Error **errp)
@@ -691,7 +704,13 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error **errp)
         }
     }
 
-    qdev_set_id(dev, g_strdup(qemu_opts_id(opts)));
+    /*
+     * set dev's parent and register its id.
+     * If it fails it means the id is already taken.
+     */
+    if (!qdev_set_id(dev, g_strdup(qemu_opts_id(opts)), errp)) {
+        goto err_del_dev;
+    }
 
     /* set properties */
     if (qemu_opt_foreach(opts, set_property, dev, errp)) {
-- 
2.31.1

[PULL 10/15] 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>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Message-Id: <20211008133442.141332-11-kwolf@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Tested-by: Peter Krempa <pkrempa@redhat.com>
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

[PULL 11/15] qdev: Add Error parameter to hide_device() callbacks

[Kevin Wolf]

hide_device() is used for virtio-net failover, where the standby virtio
device delays creation of the primary device. It only makes sense to
have a single primary device for each standby device. Adding a second
one should result in an error instead of hiding it and never using it
afterwards.

Prepare for this by adding an Error parameter to the hide_device()
callback where virtio-net is informed about adding a primary device.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Message-Id: <20211008133442.141332-12-kwolf@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Tested-by: Peter Krempa <pkrempa@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 include/hw/qdev-core.h | 8 ++++++--
 hw/core/qdev.c         | 7 +++++--
 hw/net/virtio-net.c    | 2 +-
 softmmu/qdev-monitor.c | 5 ++++-
 4 files changed, 16 insertions(+), 6 deletions(-)

diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 5a073fc368..74d8b614a7 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -201,8 +201,12 @@ struct DeviceListener {
      * informs qdev if a device should be visible or hidden.  We can
      * hide a failover device depending for example on the device
      * opts.
+     *
+     * On errors, it returns false and errp is set. Device creation
+     * should fail in this case.
      */
-    bool (*hide_device)(DeviceListener *listener, QemuOpts *device_opts);
+    bool (*hide_device)(DeviceListener *listener, QemuOpts *device_opts,
+                        Error **errp);
     QTAILQ_ENTRY(DeviceListener) link;
 };
 
@@ -837,7 +841,7 @@ void device_listener_unregister(DeviceListener *listener);
  * 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(QemuOpts *opts, Error **errp);
 
 typedef enum MachineInitPhase {
     /* current_machine is NULL.  */
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index d918b50a1d..c3a021c444 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -211,14 +211,17 @@ void device_listener_unregister(DeviceListener *listener)
     QTAILQ_REMOVE(&device_listeners, listener, link);
 }
 
-bool qdev_should_hide_device(QemuOpts *opts)
+bool qdev_should_hide_device(QemuOpts *opts, Error **errp)
 {
+    ERRP_GUARD();
     DeviceListener *listener;
 
     QTAILQ_FOREACH(listener, &device_listeners, link) {
         if (listener->hide_device) {
-            if (listener->hide_device(listener, opts)) {
+            if (listener->hide_device(listener, opts, errp)) {
                 return true;
+            } else if (*errp) {
+                return false;
             }
         }
     }
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index f205331dcf..a17d5739fc 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)
+                                         QemuOpts *device_opts, Error **errp)
 {
     VirtIONet *n = container_of(listener, VirtIONet, primary_listener);
     const char *standby_id;
diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
index 0b6833cc57..ea737db028 100644
--- a/softmmu/qdev-monitor.c
+++ b/softmmu/qdev-monitor.c
@@ -626,6 +626,7 @@ const char *qdev_set_id(DeviceState *dev, char *id, Error **errp)
 
 DeviceState *qdev_device_add(QemuOpts *opts, Error **errp)
 {
+    ERRP_GUARD();
     DeviceClass *dc;
     const char *driver, *path;
     DeviceState *dev = NULL;
@@ -669,11 +670,13 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error **errp)
             error_setg(errp, "Device with failover_pair_id don't have id");
             return NULL;
         }
-        if (qdev_should_hide_device(opts)) {
+        if (qdev_should_hide_device(opts, errp)) {
             if (bus && !qbus_is_hotpluggable(bus)) {
                 error_setg(errp, QERR_BUS_NO_HOTPLUG, bus->name);
             }
             return NULL;
+        } else if (*errp) {
+            return NULL;
         }
     }
 
-- 
2.31.1

[PULL 12/15] virtio-net: Store failover primary opts pointer locally

[Kevin Wolf]

Instead of accessing the global QemuOptsList, which really belong to the
command line parser and shouldn't be accessed from devices, store a
pointer to the QemuOpts in a new VirtIONet field.

This is not the final state, but just an intermediate step to get rid of
QemuOpts in devices. It will later be replaced with an options QDict.

Before this patch, two "primary" devices could be hidden for the same
standby device, but only one of them would actually be enabled and the
other one would be kept hidden forever, so this doesn't make sense.
After this patch, configuring a second primary device is an error.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Message-Id: <20211008133442.141332-13-kwolf@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Tested-by: Peter Krempa <pkrempa@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 include/hw/virtio/virtio-net.h |  1 +
 hw/net/virtio-net.c            | 26 ++++++++++++++++++--------
 2 files changed, 19 insertions(+), 8 deletions(-)

diff --git a/include/hw/virtio/virtio-net.h b/include/hw/virtio/virtio-net.h
index 824a69c23f..d118c95f69 100644
--- a/include/hw/virtio/virtio-net.h
+++ b/include/hw/virtio/virtio-net.h
@@ -209,6 +209,7 @@ struct VirtIONet {
     bool failover_primary_hidden;
     bool failover;
     DeviceListener primary_listener;
+    QemuOpts *primary_opts;
     Notifier migration_state;
     VirtioNetRssData rss_data;
     struct NetRxPkt *rx_pkt;
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index a17d5739fc..ed9a9012e9 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -858,27 +858,24 @@ static DeviceState *failover_find_primary_device(VirtIONet *n)
 static void failover_add_primary(VirtIONet *n, Error **errp)
 {
     Error *err = NULL;
-    QemuOpts *opts;
-    char *id;
     DeviceState *dev = failover_find_primary_device(n);
 
     if (dev) {
         return;
     }
 
-    id = failover_find_primary_device_id(n);
-    if (!id) {
+    if (!n->primary_opts) {
         error_setg(errp, "Primary device not found");
         error_append_hint(errp, "Virtio-net failover will not work. Make "
                           "sure primary device has parameter"
                           " failover_pair_id=%s\n", n->netclient_name);
         return;
     }
-    opts = qemu_opts_find(qemu_find_opts("device"), id);
-    g_assert(opts); /* cannot be NULL because id was found using opts list */
-    dev = qdev_device_add(opts, &err);
+
+    dev = qdev_device_add(n->primary_opts, &err);
     if (err) {
-        qemu_opts_del(opts);
+        qemu_opts_del(n->primary_opts);
+        n->primary_opts = NULL;
     } else {
         object_unref(OBJECT(dev));
     }
@@ -3317,6 +3314,19 @@ static bool failover_hide_primary_device(DeviceListener *listener,
         return false;
     }
 
+    if (n->primary_opts) {
+        error_setg(errp, "Cannot attach more than one primary device to '%s'",
+                   n->netclient_name);
+        return false;
+    }
+
+    /*
+     * Having a weak reference here should be okay because a device can't be
+     * deleted while it's hidden. This will be replaced soon with a QDict that
+     * has a clearer ownership model.
+     */
+    n->primary_opts = device_opts;
+
     /* failover_primary_hidden is set during feature negotiation */
     return qatomic_read(&n->failover_primary_hidden);
 }
-- 
2.31.1

[PULL 13/15] virtio-net: Avoid QemuOpts in failover_find_primary_device()

[Kevin Wolf]

Don't go through the global QemuOptsList, it is state of the legacy
command line parser and we will create devices that are not contained
in it. It is also just the command line configuration and not
necessarily the current runtime state.

Instead, look at the qdev device tree which has the current state of all
existing devices.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Message-Id: <20211008133442.141332-14-kwolf@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Tested-by: Peter Krempa <pkrempa@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 hw/net/virtio-net.c | 52 +++++++++++++++++----------------------------
 1 file changed, 19 insertions(+), 33 deletions(-)

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index ed9a9012e9..f503f28c00 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -796,48 +796,34 @@ static inline uint64_t virtio_net_supported_guest_offloads(VirtIONet *n)
 
 typedef struct {
     VirtIONet *n;
-    char *id;
-} FailoverId;
+    DeviceState *dev;
+} FailoverDevice;
 
 /**
- * Set the id of the failover primary device
+ * Set the failover primary device
  *
  * @opaque: FailoverId to setup
  * @opts: opts for device we are handling
  * @errp: returns an error if this function fails
  */
-static int failover_set_primary(void *opaque, QemuOpts *opts, Error **errp)
+static int failover_set_primary(DeviceState *dev, void *opaque)
 {
-    FailoverId *fid = opaque;
-    const char *standby_id = qemu_opt_get(opts, "failover_pair_id");
+    FailoverDevice *fdev = opaque;
+    PCIDevice *pci_dev = (PCIDevice *)
+        object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE);
 
-    if (g_strcmp0(standby_id, fid->n->netclient_name) == 0) {
-        fid->id = g_strdup(opts->id);
+    if (!pci_dev) {
+        return 0;
+    }
+
+    if (!g_strcmp0(pci_dev->failover_pair_id, fdev->n->netclient_name)) {
+        fdev->dev = dev;
         return 1;
     }
 
     return 0;
 }
 
-/**
- * Find the primary device id for this failover virtio-net
- *
- * @n: VirtIONet device
- * @errp: returns an error if this function fails
- */
-static char *failover_find_primary_device_id(VirtIONet *n)
-{
-    Error *err = NULL;
-    FailoverId fid;
-
-    fid.n = n;
-    if (!qemu_opts_foreach(qemu_find_opts("device"),
-                           failover_set_primary, &fid, &err)) {
-        return NULL;
-    }
-    return fid.id;
-}
-
 /**
  * Find the primary device for this failover virtio-net
  *
@@ -846,13 +832,13 @@ static char *failover_find_primary_device_id(VirtIONet *n)
  */
 static DeviceState *failover_find_primary_device(VirtIONet *n)
 {
-    char *id = failover_find_primary_device_id(n);
-
-    if (!id) {
-        return NULL;
-    }
+    FailoverDevice fdev = {
+        .n = n,
+    };
 
-    return qdev_find_recursive(sysbus_get_default(), id);
+    qbus_walk_children(sysbus_get_default(), failover_set_primary, NULL,
+                       NULL, NULL, &fdev);
+    return fdev.dev;
 }
 
 static void failover_add_primary(VirtIONet *n, Error **errp)
-- 
2.31.1

[PULL 14/15] 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>
Message-Id: <20211008133442.141332-15-kwolf@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Tested-by: Peter Krempa <pkrempa@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 include/hw/qdev-core.h         | 12 +++---
 include/hw/virtio/virtio-net.h |  3 +-
 include/monitor/qdev.h         |  2 +
 hw/core/qdev.c                 |  7 ++--
 hw/net/virtio-net.c            | 23 +++++++-----
 hw/vfio/pci.c                  |  4 +-
 softmmu/qdev-monitor.c         | 69 +++++++++++++++-------------------
 7 files changed, 61 insertions(+), 59 deletions(-)

diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 74d8b614a7..1bad07002d 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;
@@ -205,8 +205,8 @@ struct DeviceListener {
      * On errors, it returns false and errp is set. Device creation
      * should fail in this case.
      */
-    bool (*hide_device)(DeviceListener *listener, QemuOpts *device_opts,
-                        Error **errp);
+    bool (*hide_device)(DeviceListener *listener, const QDict *device_opts,
+                        bool from_json, Error **errp);
     QTAILQ_ENTRY(DeviceListener) link;
 };
 
@@ -835,13 +835,15 @@ void device_listener_unregister(DeviceListener *listener);
 
 /**
  * @qdev_should_hide_device:
- * @opts: QemuOpts as passed on cmdline.
+ * @opts: options QDict
+ * @from_json: true if @opts entries are typed, false for all strings
+ * @errp: pointer to error object
  *
  * 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, Error **errp);
+bool qdev_should_hide_device(const QDict *opts, bool from_json, Error **errp);
 
 typedef enum MachineInitPhase {
     /* current_machine is NULL.  */
diff --git a/include/hw/virtio/virtio-net.h b/include/hw/virtio/virtio-net.h
index d118c95f69..74a10ebe85 100644
--- a/include/hw/virtio/virtio-net.h
+++ b/include/hw/virtio/virtio-net.h
@@ -209,7 +209,8 @@ struct VirtIONet {
     bool failover_primary_hidden;
     bool failover;
     DeviceListener primary_listener;
-    QemuOpts *primary_opts;
+    QDict *primary_opts;
+    bool primary_opts_from_json;
     Notifier migration_state;
     VirtioNetRssData rss_data;
     struct NetRxPkt *rx_pkt;
diff --git a/include/monitor/qdev.h b/include/monitor/qdev.h
index 74e6c55a2b..1d57bf6577 100644
--- a/include/monitor/qdev.h
+++ b/include/monitor/qdev.h
@@ -9,6 +9,8 @@ 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);
+DeviceState *qdev_device_add_from_qdict(const QDict *opts,
+                                        bool from_json, Error **errp);
 
 /**
  * qdev_set_id: parent the device and set its id if provided.
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index c3a021c444..7f06403752 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,14 +212,14 @@ void device_listener_unregister(DeviceListener *listener)
     QTAILQ_REMOVE(&device_listeners, listener, link);
 }
 
-bool qdev_should_hide_device(QemuOpts *opts, Error **errp)
+bool qdev_should_hide_device(const QDict *opts, bool from_json, Error **errp)
 {
     ERRP_GUARD();
     DeviceListener *listener;
 
     QTAILQ_FOREACH(listener, &device_listeners, link) {
         if (listener->hide_device) {
-            if (listener->hide_device(listener, opts, errp)) {
+            if (listener->hide_device(listener, opts, from_json, errp)) {
                 return true;
             } else if (*errp) {
                 return false;
@@ -958,7 +959,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 f503f28c00..09e173a558 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -858,9 +858,11 @@ static void failover_add_primary(VirtIONet *n, Error **errp)
         return;
     }
 
-    dev = qdev_device_add(n->primary_opts, &err);
+    dev = qdev_device_add_from_qdict(n->primary_opts,
+                                     n->primary_opts_from_json,
+                                     &err);
     if (err) {
-        qemu_opts_del(n->primary_opts);
+        qobject_unref(n->primary_opts);
         n->primary_opts = NULL;
     } else {
         object_unref(OBJECT(dev));
@@ -3287,7 +3289,9 @@ static void virtio_net_migration_state_notifier(Notifier *notifier, void *data)
 }
 
 static bool failover_hide_primary_device(DeviceListener *listener,
-                                         QemuOpts *device_opts, Error **errp)
+                                         const QDict *device_opts,
+                                         bool from_json,
+                                         Error **errp)
 {
     VirtIONet *n = container_of(listener, VirtIONet, primary_listener);
     const char *standby_id;
@@ -3295,7 +3299,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;
     }
@@ -3306,12 +3310,8 @@ static bool failover_hide_primary_device(DeviceListener *listener,
         return false;
     }
 
-    /*
-     * Having a weak reference here should be okay because a device can't be
-     * deleted while it's hidden. This will be replaced soon with a QDict that
-     * has a clearer ownership model.
-     */
-    n->primary_opts = device_opts;
+    n->primary_opts = qdict_clone_shallow(device_opts);
+    n->primary_opts_from_json = from_json;
 
     /* failover_primary_hidden is set during feature negotiation */
     return qatomic_read(&n->failover_primary_hidden);
@@ -3502,8 +3502,11 @@ static void virtio_net_device_unrealize(DeviceState *dev)
     g_free(n->vlans);
 
     if (n->failover) {
+        qobject_unref(n->primary_opts);
         device_listener_unregister(&n->primary_listener);
         remove_migration_state_change_notifier(&n->migration_state);
+    } else {
+        assert(n->primary_opts == NULL);
     }
 
     max_queues = n->multiqueue ? n->max_queues : 1;
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 ea737db028..89c473cb22 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;
@@ -624,15 +596,17 @@ const char *qdev_set_id(DeviceState *dev, char *id, Error **errp)
     return prop->name;
 }
 
-DeviceState *qdev_device_add(QemuOpts *opts, Error **errp)
+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;
@@ -645,7 +619,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) {
@@ -665,12 +639,12 @@ 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;
         }
-        if (qdev_should_hide_device(opts, errp)) {
+        if (qdev_should_hide_device(opts, from_json, errp)) {
             if (bus && !qbus_is_hotpluggable(bus)) {
                 error_setg(errp, QERR_BUS_NO_HOTPLUG, bus->name);
             }
@@ -711,18 +685,24 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error **errp)
      * set dev's parent and register its id.
      * If it fails it means the id is already taken.
      */
-    if (!qdev_set_id(dev, g_strdup(qemu_opts_id(opts)), errp)) {
+    id = g_strdup(qdict_get_try_str(opts, "id"));
+    if (!qdev_set_id(dev, id, 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;
@@ -735,6 +715,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

Re: [PULL 14/15] qdev: Base object creation on QDict rather than QemuOpts

[Peter Maydell]

On Fri, 15 Oct 2021 at 16:01, Kevin Wolf <kwolf@redhat.com> 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>
> Message-Id: <20211008133442.141332-15-kwolf@redhat.com>
> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> Tested-by: Peter Krempa <pkrempa@redhat.com>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>

Hi; we discovered via a report on IRC this this commit broke
handling of "array properties", of which one example is:
qemu-system-x86_64 -netdev user,id=a -device rocker,len-ports=1,ports[0]=a

This used to work, and now fails with
 qemu-system-x86_64: -device rocker,len-ports=1,ports[0]=a: Property
'rocker.ports[0]' not found

I think this happens because array properties have the
requirement that the len-foo property is set first before
any of the foo[n] properties can be set. In the old code
I guess we used to set properties from the command line
in the order they were specified, whereas in the new code
we end up in object_set_properties_from_qdict() which
tries to set them in whatever order the qdict hash table
provides them, which turns out to be the wrong one :-(

Any suggestions for how to address this ?

thanks
-- PMM

Re: [PULL 14/15] qdev: Base object creation on QDict rather than QemuOpts

[Markus Armbruster]

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

> On Fri, 15 Oct 2021 at 16:01, Kevin Wolf <kwolf@redhat.com> 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>
>> Message-Id: <20211008133442.141332-15-kwolf@redhat.com>
>> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
>> Tested-by: Peter Krempa <pkrempa@redhat.com>
>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>
> Hi; we discovered via a report on IRC this this commit broke
> handling of "array properties", of which one example is:
> qemu-system-x86_64 -netdev user,id=a -device rocker,len-ports=1,ports[0]=a
>
> This used to work, and now fails with
>  qemu-system-x86_64: -device rocker,len-ports=1,ports[0]=a: Property
> 'rocker.ports[0]' not found
>
> I think this happens because array properties have the
> requirement that the len-foo property is set first before
> any of the foo[n] properties can be set. In the old code
> I guess we used to set properties from the command line
> in the order they were specified, whereas in the new code
> we end up in object_set_properties_from_qdict() which
> tries to set them in whatever order the qdict hash table
> provides them, which turns out to be the wrong one :-(
>
> Any suggestions for how to address this ?

My initial (knee-jerk) reaction to breaking array properties: Faster,
Pussycat! Kill! Kill!

Back to serious: replace the implementation of QDict so it iterates in
order?

Re: [PULL 14/15] qdev: Base object creation on QDict rather than QemuOpts

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> Peter Maydell <peter.maydell@linaro.org> writes:
>
>> On Fri, 15 Oct 2021 at 16:01, Kevin Wolf <kwolf@redhat.com> 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>
>>> Message-Id: <20211008133442.141332-15-kwolf@redhat.com>
>>> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
>>> Tested-by: Peter Krempa <pkrempa@redhat.com>
>>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>>
>> Hi; we discovered via a report on IRC this this commit broke
>> handling of "array properties", of which one example is:
>> qemu-system-x86_64 -netdev user,id=a -device rocker,len-ports=1,ports[0]=a
>>
>> This used to work, and now fails with
>>  qemu-system-x86_64: -device rocker,len-ports=1,ports[0]=a: Property
>> 'rocker.ports[0]' not found
>>
>> I think this happens because array properties have the
>> requirement that the len-foo property is set first before
>> any of the foo[n] properties can be set. In the old code
>> I guess we used to set properties from the command line
>> in the order they were specified, whereas in the new code
>> we end up in object_set_properties_from_qdict() which
>> tries to set them in whatever order the qdict hash table
>> provides them, which turns out to be the wrong one :-(
>>
>> Any suggestions for how to address this ?
>
> My initial (knee-jerk) reaction to breaking array properties: Faster,
> Pussycat! Kill! Kill!
>
> Back to serious: replace the implementation of QDict so it iterates in
> order?

I just sent

    [RFC PATCH] qobject: Rewrite implementation of QDict for in-order traversal
    Message-Id: <20220705095421.2455041-1-armbru@redhat.com>

Please test whether this fixes the regressions you observed.

Re: [PULL 14/15] qdev: Base object creation on QDict rather than QemuOpts

[Peter Maydell]

On Mon, 4 Jul 2022 at 05:50, Markus Armbruster <armbru@redhat.com> wrote:
> My initial (knee-jerk) reaction to breaking array properties: Faster,
> Pussycat! Kill! Kill!

In an ideal world, what would you replace them with?

thanks
-- PMM

The case for array properties (was: [PULL 14/15] qdev: Base object creation on QDict rather than QemuOpts)

[Markus Armbruster]

Cc'ing QOM maintainers.

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

> On Mon, 4 Jul 2022 at 05:50, Markus Armbruster <armbru@redhat.com> wrote:
>> My initial (knee-jerk) reaction to breaking array properties: Faster,
>> Pussycat! Kill! Kill!
>
> In an ideal world, what would you replace them with?

Let's first recapitulate their intended purpose.

commit 339659041f87a76f8b71ad3d12cadfc5f89b4bb3q
Author: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
Date:   Tue Aug 19 23:55:52 2014 -0700

    qom: Add automatic arrayification to object_property_add()
    
    If "[*]" is given as the last part of a QOM property name, treat that
    as an array property. The added property is given the first available
    name, replacing the * with a decimal number counting from 0.
    
    First add with name "foo[*]" will be "foo[0]". Second "foo[1]" and so
    on.
    
    Callers may inspect the ObjectProperty * return value to see what
    number the added property was given.
    
    Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
    Signed-off-by: Andreas Färber <afaerber@suse.de>

This describes how they work, but sadly not why we want them.  For such
arcane lore, we need to consult a guru.  Possibly via the mailing list
archive.

Digression: when you add a feature, please, please, *please* explain why
you need it right in the commit message.  Such rationale is useful
information, tends to age well, and can be quite laborious to
reconstruct later.

Even though I'm sure we discussed the intended purpose(s) of array
properties before, a quick grep of my list archive comes up mostly
empty, so I'm falling back to (foggy) memory.  Please correct mistakes
and fill in omissions.

We occasionally have a need for an array of properties where the length
of the array is not fixed at compile time.  Say in code common to
several related devices, where some have two frobs, some four, and a
future one may have some other number.

We could define properties frob0, frob1, ... frobN for some fixed N.
Users have to set them like frob0=...,frob1=... and so forth.  We need
code to reject frobI=... for I exeeding the actual limit.

Array properties spare developers picking a fixed N, and users adding an
index to the property name.  Whether the latter is a good idea is
unclear.  We need code to reject usage exceeding the actual limit.

A secondary use is (was?) avoiding memory region name clashes in code we
don't want to touch.  Discussed in the review of my attempt to strangle
array properties in 2014:

    Message-ID: <87sihn9nji.fsf@blackfin.pond.sub.org>
    https://lists.gnu.org/archive/html/qemu-devel/2014-11/msg02103.html

Re: The case for array properties (was: [PULL 14/15] qdev: Base object creation on QDict rather than QemuOpts)

[Daniel P. Berrangé]

On Fri, Jul 08, 2022 at 01:40:43PM +0200, Markus Armbruster wrote:
> Cc'ing QOM maintainers.
> 
> Peter Maydell <peter.maydell@linaro.org> writes:
> 
> > On Mon, 4 Jul 2022 at 05:50, Markus Armbruster <armbru@redhat.com> wrote:
> >> My initial (knee-jerk) reaction to breaking array properties: Faster,
> >> Pussycat! Kill! Kill!
> >
> > In an ideal world, what would you replace them with?
> 
> Let's first recapitulate their intended purpose.
> 
> commit 339659041f87a76f8b71ad3d12cadfc5f89b4bb3q
> Author: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> Date:   Tue Aug 19 23:55:52 2014 -0700
> 
>     qom: Add automatic arrayification to object_property_add()
>     
>     If "[*]" is given as the last part of a QOM property name, treat that
>     as an array property. The added property is given the first available
>     name, replacing the * with a decimal number counting from 0.
>     
>     First add with name "foo[*]" will be "foo[0]". Second "foo[1]" and so
>     on.
>     
>     Callers may inspect the ObjectProperty * return value to see what
>     number the added property was given.
>     
>     Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>     Signed-off-by: Andreas Färber <afaerber@suse.de>
> 
> This describes how they work, but sadly not why we want them.  For such
> arcane lore, we need to consult a guru.  Possibly via the mailing list
> archive.

Also doesn't describe why we need to explicitly set the array length
upfront, rather than inferring it from the set of elements that are
specified, auto-extending the array bounds as we set each property.

> Digression: when you add a feature, please, please, *please* explain why
> you need it right in the commit message.  Such rationale is useful
> information, tends to age well, and can be quite laborious to
> reconstruct later.
> 
> Even though I'm sure we discussed the intended purpose(s) of array
> properties before, a quick grep of my list archive comes up mostly
> empty, so I'm falling back to (foggy) memory.  Please correct mistakes
> and fill in omissions.
> 
> We occasionally have a need for an array of properties where the length
> of the array is not fixed at compile time.  Say in code common to
> several related devices, where some have two frobs, some four, and a
> future one may have some other number.
> 
> We could define properties frob0, frob1, ... frobN for some fixed N.
> Users have to set them like frob0=...,frob1=... and so forth.  We need
> code to reject frobI=... for I exeeding the actual limit.
> 
> Array properties spare developers picking a fixed N, and users adding an
> index to the property name.  Whether the latter is a good idea is
> unclear.  We need code to reject usage exceeding the actual limit.

If we consider that our canonical representation is aiming to be QAPI,
and QAPI has unbounded arrays, then by implication if we want a mapping
to a flat CLI syntax, then we need some mechanism for unbounded arrays.

It would be valid to argue that we shouldn'be be trying to map the full
expressiveness of QAPI into a flat CLI syntax though, and should just
strive for full JSON everywhere.

Indeed every time we have these discussions, I wish we were already at
the "full JSON everywhere" point, so we can stop consuming our time
debating how to flatten JSON structure into CLI options. But since
these array props already exist, we need to find a way out of the
problem...

With 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: The case for array properties

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Fri, Jul 08, 2022 at 01:40:43PM +0200, Markus Armbruster wrote:
>> Cc'ing QOM maintainers.
>> 
>> Peter Maydell <peter.maydell@linaro.org> writes:
>> 
>> > On Mon, 4 Jul 2022 at 05:50, Markus Armbruster <armbru@redhat.com> wrote:
>> >> My initial (knee-jerk) reaction to breaking array properties: Faster,
>> >> Pussycat! Kill! Kill!
>> >
>> > In an ideal world, what would you replace them with?
>> 
>> Let's first recapitulate their intended purpose.
>> 
>> commit 339659041f87a76f8b71ad3d12cadfc5f89b4bb3q
>> Author: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>> Date:   Tue Aug 19 23:55:52 2014 -0700
>> 
>>     qom: Add automatic arrayification to object_property_add()
>>     
>>     If "[*]" is given as the last part of a QOM property name, treat that
>>     as an array property. The added property is given the first available
>>     name, replacing the * with a decimal number counting from 0.
>>     
>>     First add with name "foo[*]" will be "foo[0]". Second "foo[1]" and so
>>     on.
>>     
>>     Callers may inspect the ObjectProperty * return value to see what
>>     number the added property was given.
>>     
>>     Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>>     Signed-off-by: Andreas Färber <afaerber@suse.de>
>> 
>> This describes how they work, but sadly not why we want them.  For such
>> arcane lore, we need to consult a guru.  Possibly via the mailing list
>> archive.
>
> Also doesn't describe why we need to explicitly set the array length
> upfront, rather than inferring it from the set of elements that are
> specified, auto-extending the array bounds as we set each property.
>
>> Digression: when you add a feature, please, please, *please* explain why
>> you need it right in the commit message.  Such rationale is useful
>> information, tends to age well, and can be quite laborious to
>> reconstruct later.
>> 
>> Even though I'm sure we discussed the intended purpose(s) of array
>> properties before, a quick grep of my list archive comes up mostly
>> empty, so I'm falling back to (foggy) memory.  Please correct mistakes
>> and fill in omissions.
>> 
>> We occasionally have a need for an array of properties where the length
>> of the array is not fixed at compile time.  Say in code common to
>> several related devices, where some have two frobs, some four, and a
>> future one may have some other number.
>> 
>> We could define properties frob0, frob1, ... frobN for some fixed N.
>> Users have to set them like frob0=...,frob1=... and so forth.  We need
>> code to reject frobI=... for I exeeding the actual limit.
>> 
>> Array properties spare developers picking a fixed N, and users adding an
>> index to the property name.  Whether the latter is a good idea is
>> unclear.  We need code to reject usage exceeding the actual limit.
>
> If we consider that our canonical representation is aiming to be QAPI,
> and QAPI has unbounded arrays, then by implication if we want a mapping
> to a flat CLI syntax, then we need some mechanism for unbounded arrays.
>
> It would be valid to argue that we shouldn'be be trying to map the full
> expressiveness of QAPI into a flat CLI syntax though, and should just
> strive for full JSON everywhere.
>
> Indeed every time we have these discussions, I wish we were already at
> the "full JSON everywhere" point, so we can stop consuming our time
> debating how to flatten JSON structure into CLI options. But since
> these array props already exist, we need to find a way out of the
> problem...

This isn't just a CLI problem, it's worse: we have property-setting code
that relies on "automatic arrayification".

Re: The case for array properties (was: [PULL 14/15] qdev: Base object creation on QDict rather than QemuOpts)

[Peter Maydell]

On Fri, 8 Jul 2022 at 12:40, Markus Armbruster <armbru@redhat.com> wrote:
>
> Cc'ing QOM maintainers.
>
> Peter Maydell <peter.maydell@linaro.org> writes:
>
> > On Mon, 4 Jul 2022 at 05:50, Markus Armbruster <armbru@redhat.com> wrote:
> >> My initial (knee-jerk) reaction to breaking array properties: Faster,
> >> Pussycat! Kill! Kill!
> >
> > In an ideal world, what would you replace them with?
>
> Let's first recapitulate their intended purpose.
>
> commit 339659041f87a76f8b71ad3d12cadfc5f89b4bb3q
> Author: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> Date:   Tue Aug 19 23:55:52 2014 -0700
>
>     qom: Add automatic arrayification to object_property_add()
>
>     If "[*]" is given as the last part of a QOM property name, treat that
>     as an array property. The added property is given the first available
>     name, replacing the * with a decimal number counting from 0.
>
>     First add with name "foo[*]" will be "foo[0]". Second "foo[1]" and so
>     on.
>
>     Callers may inspect the ObjectProperty * return value to see what
>     number the added property was given.
>
>     Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>     Signed-off-by: Andreas Färber <afaerber@suse.de>
>
> This describes how they work, but sadly not why we want them.  For such
> arcane lore, we need to consult a guru.  Possibly via the mailing list
> archive.

This is not the initial array property feature addition. It's
adding a convenience extra subfeature (ability to use "[*]"
to mean "next one in the array"). The inital array property
feature commit is:

commit 0be6bfac6262900425c10847de513ee2490078d3
Author: Peter Maydell <peter.maydell@linaro.org>
Date:   Fri Mar 15 16:41:57 2013 +0000

    qdev: Implement (variable length) array properties

    Add support for declaring array properties for qdev devices.
    These work by defining an initial static property 'len-arrayname'
    which the user of the device should set to the desired size
    of the array. When this property is set, memory is allocated
    for the array elements, and dynamic properties "arrayname[0]",
    "arrayname[1]"... are created so the user of the device can
    then set the values of the individual array elements.

Admittedly this doesn't give the justification either :-(
The reason is for the benefit of the following commit (and
similar use cases):

commit 8bd4824a6122292060a318741595927e0d05ff5e
Author: Peter Maydell <peter.maydell@linaro.org>
Date:   Fri Mar 15 16:41:57 2013 +0000

    hw/arm_sysctl: Implement SYS_CFG_VOLT

    Implement the SYS_CFG_VOLT registers which return the voltage
    of various supplies on motherboard and daughterboard. Since
    QEMU implements a perfectly stable power supply these registers
    always return a constant value. The number and value of the
    daughterboard voltages is dependent on the specific daughterboard,
    so we use a property array to allow the board to configure them
    appropriately.

> We occasionally have a need for an array of properties where the length
> of the array is not fixed at compile time.  Say in code common to
> several related devices, where some have two frobs, some four, and a
> future one may have some other number.
>
> We could define properties frob0, frob1, ... frobN for some fixed N.
> Users have to set them like frob0=...,frob1=... and so forth.  We need
> code to reject frobI=... for I exeeding the actual limit.
>
> Array properties spare developers picking a fixed N, and users adding an
> index to the property name.  Whether the latter is a good idea is
> unclear.  We need code to reject usage exceeding the actual limit.

Note that the initial intention of array properties was largely
for within-QEMU-code QOM properties. The usage of array properties
for user-creatable devices is a later development.

The basic rationale for array properties is:
Sometimes the thing we want to be able to configure on a device
is naturally expressed as "we have an indexed set of things".
When writing a device implementation, it's nice to be able to
straightforwardly say "this is an indexed set of things" and have
the property system set the device struct fields accordingly
(ie setting the num_foo struct field to the number of things
and the foo field to an allocated array of foos), and for the
way that device users to set the values of that indexed set
of things not to look too awful either.

The specific implementation we have today is just a cute hack
that avoided having to mess about too much with the core QOM
property implementation. (And it's only the implementation that
imposes the "set the length first" constraint, that's not
inherent to the idea of array properties.)

I don't have a strong feeling about the "[*]" automatic-indexing
subfeature -- none of the use of array properties that I've
added uses that.

thanks
-- PMM

Re: [PULL 14/15] qdev: Base object creation on QDict rather than QemuOpts

[Kevin Wolf]

Am 07.07.2022 um 22:24 hat Peter Maydell geschrieben:
> On Mon, 4 Jul 2022 at 05:50, Markus Armbruster <armbru@redhat.com> wrote:
> > My initial (knee-jerk) reaction to breaking array properties: Faster,
> > Pussycat! Kill! Kill!
> 
> In an ideal world, what would you replace them with?

The next (and final) patch in this pull request added JSON syntax for
-device, so we can actually have proper list properties now that are
accessed with the normal list visitors. I suppose some integration with
the qdev property system is still missing, but on the QOM level it could
be used.

In the ideal world, we would also be able to replace the human CLI
syntax so that JSON isn't required for this, but doing this in reality
would probably cause new compatibility problems - though libvirt has
been using JSON for a while, so I guess it wouldn't mind an incompatible
change of human syntax.

Kevin

[PULL 15/15] 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>
Reviewed-by: Eric Blake <eblake@redhat.com>
Message-Id: <20211008133442.141332-16-kwolf@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Tested-by: Peter Krempa <pkrempa@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 qapi/qdev.json | 15 ++++++++----
 softmmu/vl.c   | 63 ++++++++++++++++++++++++++++++++++++++++++++------
 2 files changed, 67 insertions(+), 11 deletions(-)

diff --git a/qapi/qdev.json b/qapi/qdev.json
index d75e68908b..69656b14df 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..af0c4cbd99 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,18 @@ 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) {
+        loc_push_restore(&opt->loc);
+        /*
+         * TODO Eventually we should call qmp_device_add() here to make sure it
+         * behaves the same, but QMP still has to accept incorrectly typed
+         * options until libvirt is fixed and we want to be strict on the CLI
+         * from the start, so call qdev_device_add_from_qdict() directly for
+         * now.
+         */
+        qdev_device_add_from_qdict(opt->opts, true, &error_fatal);
+        loc_pop(&opt->loc);
+    }
     rom_reset_order_override();
 }
 
@@ -3352,9 +3392,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

Re: [PULL 00/15] qdev: Add JSON -device

[Richard Henderson]

On 10/15/21 7:46 AM, Kevin Wolf wrote:
> The following changes since commit 4d1a525dfafe995a98bb486e702da09e31b68b9c:
> 
>    Merge remote-tracking branch 'remotes/bonzini/tags/for-upstream' into staging (2021-10-14 10:49:38 -0700)
> 
> are available in the Git repository at:
> 
>    git://repo.or.cz/qemu/kevin.git tags/for-upstream
> 
> for you to fetch changes up to 5dacda5167560b3af8eadbce5814f60ba44b467e:
> 
>    vl: Enable JSON syntax for -device (2021-10-15 16:11:22 +0200)
> 
> ----------------------------------------------------------------
> qdev: Add JSON -device
> 
> - Add a JSON mode to the -device command line option
> - net/vhost-{user,vdpa}: Fix device compatibility check
> - Minor iotests fixes
> 
> ----------------------------------------------------------------
> Damien Hedde (1):
>        softmmu/qdev-monitor: add error handling in qdev_set_id
> 
> Kevin Wolf (14):
>        net: Introduce NetClientInfo.check_peer_type()
>        net/vhost-user: Fix device compatibility check
>        net/vhost-vdpa: Fix device compatibility check
>        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
>        qemu-option: Allow deleting opts during qemu_opts_foreach()
>        qdev: Add Error parameter to hide_device() callbacks
>        virtio-net: Store failover primary opts pointer locally
>        virtio-net: Avoid QemuOpts in failover_find_primary_device()
>        qdev: Base object creation on QDict rather than QemuOpts
>        vl: Enable JSON syntax for -device
> 
>   qapi/qdev.json                      |  15 ++++--
>   include/hw/qdev-core.h              |  16 ++++--
>   include/hw/virtio/virtio-net.h      |   2 +
>   include/monitor/qdev.h              |  27 +++++++++-
>   include/net/net.h                   |   2 +
>   hw/arm/virt.c                       |   2 +-
>   hw/core/qdev-properties-system.c    |   6 +++
>   hw/core/qdev.c                      |  11 ++--
>   hw/net/virtio-net.c                 |  85 +++++++++++++++---------------
>   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 +-
>   net/vhost-user.c                    |  41 +++++----------
>   net/vhost-vdpa.c                    |  37 +++++--------
>   qom/object.c                        |   7 ++-
>   qom/object_interfaces.c             |  19 +++----
>   softmmu/qdev-monitor.c              | 100 +++++++++++++++++++++++-------------
>   softmmu/vl.c                        |  63 ++++++++++++++++++++---
>   util/qemu-option.c                  |   4 +-
>   tests/qemu-iotests/051              |   2 +-
>   tests/qemu-iotests/051.pc.out       |   4 +-
>   tests/qemu-iotests/245              |   4 +-
>   23 files changed, 280 insertions(+), 178 deletions(-)

Applied, thanks.

r~