[PATCH v2 0/2] qdev-monitor: avoid QemuOpts in QMP device_add

[PATCH v2 0/2] qdev-monitor: avoid QemuOpts in QMP device_add

[Stefan Hajnoczi]

v2:
- Rename Patch 1 to indicate that we're avoiding QemuOpts rather than doing a
  full conversion to QAPI. Also mention that 'gen': false is still being used.
  [Markus]
- Add Patch 2 to address a TODO comment suggesting that
  qemu_create_cli_devices() should call qmp_device_add(). [Markus]
- Move drain_call_rcu() into qdev_device_add_from_qdict() to avoid code
  duplication. [Markus]

This series enables non-scalar parameter parsing in device_add (e.g.
virtio-blk-pci,iothread-vq-mapping=). Stop converting from QDict to QemuOpts
and back again as this loses type information and cannot represent non-scalars.

Stefan Hajnoczi (2):
  qdev-monitor: avoid QemuOpts in QMP device_add
  vl: use qmp_device_add() in qemu_create_cli_devices()

 system/qdev-monitor.c | 56 ++++++++++++++++++++++---------------------
 system/vl.c           | 14 ++++-------
 2 files changed, 33 insertions(+), 37 deletions(-)

-- 
2.45.2

[PATCH v2 1/2] qdev-monitor: avoid QemuOpts in QMP device_add

[Stefan Hajnoczi]

The QMP device_add monitor command converts the QDict arguments to
QemuOpts and then back again to QDict. This process only supports scalar
types. Device properties like virtio-blk-pci's iothread-vq-mapping (an
array of objects) are silently dropped by qemu_opts_from_qdict() during
the QemuOpts conversion even though QAPI is capable of validating them.
As a result, hotplugging virtio-blk-pci devices with the
iothread-vq-mapping property does not work as expected (the property is
ignored).

Get rid of the QemuOpts conversion in qmp_device_add() and call
qdev_device_add_from_qdict() with from_json=true. Using the QMP
command's QDict arguments directly allows non-scalar properties.

The HMP is also adjusted since qmp_device_add()'s now expects properly
typed JSON arguments and cannot be used from HMP anymore. Move the code
that was previously in qmp_device_add() (with QemuOpts conversion and
from_json=false) into hmp_device_add() so that its behavior is
unchanged.

This patch changes the behavior of QMP device_add but not HMP
device_add. QMP clients that sent incorrectly typed device_add QMP
commands no longer work. This is a breaking change but clients should be
using the correct types already. See the netdev_add QAPIfication in
commit db2a380c8457 for similar reasoning and object-add in commit
9151e59a8b6e. Unlike those commits, we continue to rely on 'gen': false
for the time being.

Move the drain_call_rcu() invocation into qdev_device_add_from_qdict()
so all callers benefit from it automatically. This avoids code
duplication.

Markus helped me figure this out and even provided a draft patch. The
code ended up very close to what he suggested.

Suggested-by: Markus Armbruster <armbru@redhat.com>
Cc: Daniel P. Berrangé <berrange@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 system/qdev-monitor.c | 56 ++++++++++++++++++++++---------------------
 1 file changed, 29 insertions(+), 27 deletions(-)

diff --git a/system/qdev-monitor.c b/system/qdev-monitor.c
index 6af6ef7d66..8a756b1a91 100644
--- a/system/qdev-monitor.c
+++ b/system/qdev-monitor.c
@@ -725,6 +725,17 @@ err_del_dev:
     if (dev) {
         object_unparent(OBJECT(dev));
         object_unref(OBJECT(dev));
+
+        /*
+         * Drain all pending RCU callbacks. This is done because
+         * some bus related operations can delay a device removal
+         * (in this case this can happen if device is added and then
+         * removed due to a configuration error)
+         * to a RCU callback, but user might expect that this interface
+         * will finish its job completely once qmp command returns result
+         * to the user
+         */
+        drain_call_rcu();
     }
     return NULL;
 }
@@ -849,34 +860,10 @@ void hmp_info_qdm(Monitor *mon, const QDict *qdict)
 
 void qmp_device_add(QDict *qdict, QObject **ret_data, Error **errp)
 {
-    QemuOpts *opts;
     DeviceState *dev;
 
-    opts = qemu_opts_from_qdict(qemu_find_opts("device"), qdict, errp);
-    if (!opts) {
-        return;
-    }
-    if (!monitor_cur_is_qmp() && qdev_device_help(opts)) {
-        qemu_opts_del(opts);
-        return;
-    }
-    dev = qdev_device_add(opts, errp);
-    if (!dev) {
-        /*
-         * Drain all pending RCU callbacks. This is done because
-         * some bus related operations can delay a device removal
-         * (in this case this can happen if device is added and then
-         * removed due to a configuration error)
-         * to a RCU callback, but user might expect that this interface
-         * will finish its job completely once qmp command returns result
-         * to the user
-         */
-        drain_call_rcu();
-
-        qemu_opts_del(opts);
-        return;
-    }
-    object_unref(OBJECT(dev));
+    dev = qdev_device_add_from_qdict(qdict, true, errp);
+    object_unref(dev);
 }
 
 static DeviceState *find_device_state(const char *id, Error **errp)
@@ -967,8 +954,23 @@ void qmp_device_del(const char *id, Error **errp)
 void hmp_device_add(Monitor *mon, const QDict *qdict)
 {
     Error *err = NULL;
+    QemuOpts *opts;
+    DeviceState *dev;
 
-    qmp_device_add((QDict *)qdict, NULL, &err);
+    opts = qemu_opts_from_qdict(qemu_find_opts("device"), qdict, &err);
+    if (!opts) {
+        goto out;
+    }
+    if (qdev_device_help(opts)) {
+        qemu_opts_del(opts);
+        return;
+    }
+    dev = qdev_device_add(opts, &err);
+    if (!dev) {
+        qemu_opts_del(opts);
+    }
+    object_unref(dev);
+out:
     hmp_handle_error(mon, err);
 }
 
-- 
2.45.2

Re: [PATCH v2 1/2] qdev-monitor: avoid QemuOpts in QMP device_add

[Markus Armbruster]

Stefan Hajnoczi <stefanha@redhat.com> writes:

> The QMP device_add monitor command converts the QDict arguments to
> QemuOpts and then back again to QDict. This process only supports scalar
> types. Device properties like virtio-blk-pci's iothread-vq-mapping (an
> array of objects) are silently dropped by qemu_opts_from_qdict() during
> the QemuOpts conversion even though QAPI is capable of validating them.
> As a result, hotplugging virtio-blk-pci devices with the
> iothread-vq-mapping property does not work as expected (the property is
> ignored).
>
> Get rid of the QemuOpts conversion in qmp_device_add() and call
> qdev_device_add_from_qdict() with from_json=true. Using the QMP
> command's QDict arguments directly allows non-scalar properties.
>
> The HMP is also adjusted since qmp_device_add()'s now expects properly
> typed JSON arguments and cannot be used from HMP anymore. Move the code
> that was previously in qmp_device_add() (with QemuOpts conversion and
> from_json=false) into hmp_device_add() so that its behavior is
> unchanged.
>
> This patch changes the behavior of QMP device_add but not HMP
> device_add. QMP clients that sent incorrectly typed device_add QMP
> commands no longer work. This is a breaking change but clients should be
> using the correct types already. See the netdev_add QAPIfication in
> commit db2a380c8457 for similar reasoning and object-add in commit
> 9151e59a8b6e. Unlike those commits, we continue to rely on 'gen': false
> for the time being.
>
> Move the drain_call_rcu() invocation into qdev_device_add_from_qdict()
> so all callers benefit from it automatically. This avoids code
> duplication.
>
> Markus helped me figure this out and even provided a draft patch. The
> code ended up very close to what he suggested.
>
> Suggested-by: Markus Armbruster <armbru@redhat.com>
> Cc: Daniel P. Berrangé <berrange@redhat.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  system/qdev-monitor.c | 56 ++++++++++++++++++++++---------------------
>  1 file changed, 29 insertions(+), 27 deletions(-)
>
> diff --git a/system/qdev-monitor.c b/system/qdev-monitor.c
> index 6af6ef7d66..8a756b1a91 100644
> --- a/system/qdev-monitor.c
> +++ b/system/qdev-monitor.c
> @@ -725,6 +725,17 @@ err_del_dev:
>      if (dev) {
>          object_unparent(OBJECT(dev));
>          object_unref(OBJECT(dev));
> +
> +        /*
> +         * Drain all pending RCU callbacks. This is done because
> +         * some bus related operations can delay a device removal
> +         * (in this case this can happen if device is added and then
> +         * removed due to a configuration error)
> +         * to a RCU callback, but user might expect that this interface
> +         * will finish its job completely once qmp command returns result
> +         * to the user
> +         */
> +        drain_call_rcu();
>      }
>      return NULL;
>  }

Moving this from qmp_device_add() adds RCU draining to call chains not
going through qmp_device_add().

Can adding it hurt?  I guess it can't.

Can it fix bugs?  I don't know.

Let's review the callers of qdev_device_add_from_qdict():

* qdev_device_add()

  - called from qmp_device_add()

    No change.

  - called from device_init_func() called from qemu_create_cli_devices()

    See below.

  - called from usbback_portid_add() called from usbback_process_port()
    called from usbback_backend_changed()

    · called from usbback_init()

    · called as XenDevOps method backend_changed()

    This is Xen.  We now drain pending RCU callbacks.  Impact?  Beats
    me.

* qemu_create_cli_devices() called from qmp_x_exit_preconfig()

  - as QMP command with -preconfig, phase must be
    PHASE_MACHINE_INITIALIZED

  - called from qemu_init() without -preconfig

  We now drain pending RCU callbacks.  Can any be pending at this
  early point?  If not, the change is a no-op.

* failover_add_primary() called from virtio_net_set_features() called as
  VirtioDeviceClass method set_features()

  This is virtio-net failover.  We now drain pending RCU callbacks.
  Impact?  Beats me.

My gut feeling is "improvement, possibly even a bug fix".  It deserves
its own commit, doesn't it?

> @@ -849,34 +860,10 @@ void hmp_info_qdm(Monitor *mon, const QDict *qdict)
>  
>  void qmp_device_add(QDict *qdict, QObject **ret_data, Error **errp)
>  {
> -    QemuOpts *opts;
>      DeviceState *dev;
>  
> -    opts = qemu_opts_from_qdict(qemu_find_opts("device"), qdict, errp);
> -    if (!opts) {
> -        return;
> -    }
> -    if (!monitor_cur_is_qmp() && qdev_device_help(opts)) {
> -        qemu_opts_del(opts);
> -        return;
> -    }
> -    dev = qdev_device_add(opts, errp);
> -    if (!dev) {
> -        /*
> -         * Drain all pending RCU callbacks. This is done because
> -         * some bus related operations can delay a device removal
> -         * (in this case this can happen if device is added and then
> -         * removed due to a configuration error)
> -         * to a RCU callback, but user might expect that this interface
> -         * will finish its job completely once qmp command returns result
> -         * to the user
> -         */
> -        drain_call_rcu();
> -
> -        qemu_opts_del(opts);
> -        return;
> -    }
> -    object_unref(OBJECT(dev));
> +    dev = qdev_device_add_from_qdict(qdict, true, errp);
> +    object_unref(dev);
>  }
>  
>  static DeviceState *find_device_state(const char *id, Error **errp)
> @@ -967,8 +954,23 @@ void qmp_device_del(const char *id, Error **errp)
>  void hmp_device_add(Monitor *mon, const QDict *qdict)
>  {
>      Error *err = NULL;
> +    QemuOpts *opts;
> +    DeviceState *dev;
>  
> -    qmp_device_add((QDict *)qdict, NULL, &err);
> +    opts = qemu_opts_from_qdict(qemu_find_opts("device"), qdict, &err);
> +    if (!opts) {
> +        goto out;
> +    }
> +    if (qdev_device_help(opts)) {
> +        qemu_opts_del(opts);
> +        return;
> +    }
> +    dev = qdev_device_add(opts, &err);
> +    if (!dev) {
> +        qemu_opts_del(opts);
> +    }
> +    object_unref(dev);
> +out:
>      hmp_handle_error(mon, err);
>  }

Remainder looks good to me.

Re: [PATCH v2 1/2] qdev-monitor: avoid QemuOpts in QMP device_add

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 5882 bytes --]

On Fri, Aug 02, 2024 at 10:01:20AM +0200, Markus Armbruster wrote:
> Stefan Hajnoczi <stefanha@redhat.com> writes:
> 
> > The QMP device_add monitor command converts the QDict arguments to
> > QemuOpts and then back again to QDict. This process only supports scalar
> > types. Device properties like virtio-blk-pci's iothread-vq-mapping (an
> > array of objects) are silently dropped by qemu_opts_from_qdict() during
> > the QemuOpts conversion even though QAPI is capable of validating them.
> > As a result, hotplugging virtio-blk-pci devices with the
> > iothread-vq-mapping property does not work as expected (the property is
> > ignored).
> >
> > Get rid of the QemuOpts conversion in qmp_device_add() and call
> > qdev_device_add_from_qdict() with from_json=true. Using the QMP
> > command's QDict arguments directly allows non-scalar properties.
> >
> > The HMP is also adjusted since qmp_device_add()'s now expects properly
> > typed JSON arguments and cannot be used from HMP anymore. Move the code
> > that was previously in qmp_device_add() (with QemuOpts conversion and
> > from_json=false) into hmp_device_add() so that its behavior is
> > unchanged.
> >
> > This patch changes the behavior of QMP device_add but not HMP
> > device_add. QMP clients that sent incorrectly typed device_add QMP
> > commands no longer work. This is a breaking change but clients should be
> > using the correct types already. See the netdev_add QAPIfication in
> > commit db2a380c8457 for similar reasoning and object-add in commit
> > 9151e59a8b6e. Unlike those commits, we continue to rely on 'gen': false
> > for the time being.
> >
> > Move the drain_call_rcu() invocation into qdev_device_add_from_qdict()
> > so all callers benefit from it automatically. This avoids code
> > duplication.
> >
> > Markus helped me figure this out and even provided a draft patch. The
> > code ended up very close to what he suggested.
> >
> > Suggested-by: Markus Armbruster <armbru@redhat.com>
> > Cc: Daniel P. Berrangé <berrange@redhat.com>
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > ---
> >  system/qdev-monitor.c | 56 ++++++++++++++++++++++---------------------
> >  1 file changed, 29 insertions(+), 27 deletions(-)
> >
> > diff --git a/system/qdev-monitor.c b/system/qdev-monitor.c
> > index 6af6ef7d66..8a756b1a91 100644
> > --- a/system/qdev-monitor.c
> > +++ b/system/qdev-monitor.c
> > @@ -725,6 +725,17 @@ err_del_dev:
> >      if (dev) {
> >          object_unparent(OBJECT(dev));
> >          object_unref(OBJECT(dev));
> > +
> > +        /*
> > +         * Drain all pending RCU callbacks. This is done because
> > +         * some bus related operations can delay a device removal
> > +         * (in this case this can happen if device is added and then
> > +         * removed due to a configuration error)
> > +         * to a RCU callback, but user might expect that this interface
> > +         * will finish its job completely once qmp command returns result
> > +         * to the user
> > +         */
> > +        drain_call_rcu();
> >      }
> >      return NULL;
> >  }
> 
> Moving this from qmp_device_add() adds RCU draining to call chains not
> going through qmp_device_add().
> 
> Can adding it hurt?  I guess it can't.
> 
> Can it fix bugs?  I don't know.
> 
> Let's review the callers of qdev_device_add_from_qdict():
> 
> * qdev_device_add()
> 
>   - called from qmp_device_add()
> 
>     No change.
> 
>   - called from device_init_func() called from qemu_create_cli_devices()
> 
>     See below.
> 
>   - called from usbback_portid_add() called from usbback_process_port()
>     called from usbback_backend_changed()
> 
>     · called from usbback_init()
> 
>     · called as XenDevOps method backend_changed()
> 
>     This is Xen.  We now drain pending RCU callbacks.  Impact?  Beats
>     me.
> 
> * qemu_create_cli_devices() called from qmp_x_exit_preconfig()
> 
>   - as QMP command with -preconfig, phase must be
>     PHASE_MACHINE_INITIALIZED
> 
>   - called from qemu_init() without -preconfig
> 
>   We now drain pending RCU callbacks.  Can any be pending at this
>   early point?  If not, the change is a no-op.
> 
> * failover_add_primary() called from virtio_net_set_features() called as
>   VirtioDeviceClass method set_features()
> 
>   This is virtio-net failover.  We now drain pending RCU callbacks.
>   Impact?  Beats me.
> 
> My gut feeling is "improvement, possibly even a bug fix".  It deserves
> its own commit, doesn't it?

I thought I'd checked it, but now that I have reviewed the functions you
looked at, I no longer think it's safe to move drain_call_rcu() into
qdev_device_add_from_qdict(). drain_call_rcu() drops the BQL and must
not be called from non-reentrant multi-threaded code paths.

For example, if drain_call_rcu() is called from device emulation code
then another vCPU thread could execute that same device's emulation code
while the BQL is dropped. That's usually unsafe because most device
emulation code only expects to run in one thread at any given time
thanks to the BQL.

Here are the cases:
- qemu_create_cli_devices: safe because this happens before device
  emulation runs in vCPU threads
- Xen: safe because there are no vCPU threads when QEMU just provides
  device emulation in a normal Xen system. But I'm not sure if usbback
  can also be used in a QEMU's Xen HVM guest mode where KVM handles
  vCPUs, it might be a problem there.
- virtio_net_set_features() -> failover_add_primary() is unsafe. Another
  vCPU thread could enter virtio-net emulation code while the thread
  running virtio_net_set_features() drops the lock.

I think it's better to duplicate drain_call_rcu() into hmp_device_add()
than to risk breaking the other code paths.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 1/2] qdev-monitor: avoid QemuOpts in QMP device_add

[Markus Armbruster]

Stefan Hajnoczi <stefanha@redhat.com> writes:

> On Fri, Aug 02, 2024 at 10:01:20AM +0200, Markus Armbruster wrote:
>> Stefan Hajnoczi <stefanha@redhat.com> writes:
>> 
>> > The QMP device_add monitor command converts the QDict arguments to
>> > QemuOpts and then back again to QDict. This process only supports scalar
>> > types. Device properties like virtio-blk-pci's iothread-vq-mapping (an
>> > array of objects) are silently dropped by qemu_opts_from_qdict() during
>> > the QemuOpts conversion even though QAPI is capable of validating them.
>> > As a result, hotplugging virtio-blk-pci devices with the
>> > iothread-vq-mapping property does not work as expected (the property is
>> > ignored).
>> >
>> > Get rid of the QemuOpts conversion in qmp_device_add() and call
>> > qdev_device_add_from_qdict() with from_json=true. Using the QMP
>> > command's QDict arguments directly allows non-scalar properties.
>> >
>> > The HMP is also adjusted since qmp_device_add()'s now expects properly
>> > typed JSON arguments and cannot be used from HMP anymore. Move the code
>> > that was previously in qmp_device_add() (with QemuOpts conversion and
>> > from_json=false) into hmp_device_add() so that its behavior is
>> > unchanged.
>> >
>> > This patch changes the behavior of QMP device_add but not HMP
>> > device_add. QMP clients that sent incorrectly typed device_add QMP
>> > commands no longer work. This is a breaking change but clients should be
>> > using the correct types already. See the netdev_add QAPIfication in
>> > commit db2a380c8457 for similar reasoning and object-add in commit
>> > 9151e59a8b6e. Unlike those commits, we continue to rely on 'gen': false
>> > for the time being.
>> >
>> > Move the drain_call_rcu() invocation into qdev_device_add_from_qdict()
>> > so all callers benefit from it automatically. This avoids code
>> > duplication.
>> >
>> > Markus helped me figure this out and even provided a draft patch. The
>> > code ended up very close to what he suggested.
>> >
>> > Suggested-by: Markus Armbruster <armbru@redhat.com>
>> > Cc: Daniel P. Berrangé <berrange@redhat.com>
>> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>> > ---
>> >  system/qdev-monitor.c | 56 ++++++++++++++++++++++---------------------
>> >  1 file changed, 29 insertions(+), 27 deletions(-)
>> >
>> > diff --git a/system/qdev-monitor.c b/system/qdev-monitor.c
>> > index 6af6ef7d66..8a756b1a91 100644
>> > --- a/system/qdev-monitor.c
>> > +++ b/system/qdev-monitor.c
>> > @@ -725,6 +725,17 @@ err_del_dev:
>> >      if (dev) {
>> >          object_unparent(OBJECT(dev));
>> >          object_unref(OBJECT(dev));
>> > +
>> > +        /*
>> > +         * Drain all pending RCU callbacks. This is done because
>> > +         * some bus related operations can delay a device removal
>> > +         * (in this case this can happen if device is added and then
>> > +         * removed due to a configuration error)
>> > +         * to a RCU callback, but user might expect that this interface
>> > +         * will finish its job completely once qmp command returns result
>> > +         * to the user
>> > +         */
>> > +        drain_call_rcu();
>> >      }
>> >      return NULL;
>> >  }
>> 
>> Moving this from qmp_device_add() adds RCU draining to call chains not
>> going through qmp_device_add().
>> 
>> Can adding it hurt?  I guess it can't.
>> 
>> Can it fix bugs?  I don't know.
>> 
>> Let's review the callers of qdev_device_add_from_qdict():
>> 
>> * qdev_device_add()
>> 
>>   - called from qmp_device_add()
>> 
>>     No change.
>> 
>>   - called from device_init_func() called from qemu_create_cli_devices()
>> 
>>     See below.
>> 
>>   - called from usbback_portid_add() called from usbback_process_port()
>>     called from usbback_backend_changed()
>> 
>>     · called from usbback_init()
>> 
>>     · called as XenDevOps method backend_changed()
>> 
>>     This is Xen.  We now drain pending RCU callbacks.  Impact?  Beats
>>     me.
>> 
>> * qemu_create_cli_devices() called from qmp_x_exit_preconfig()
>> 
>>   - as QMP command with -preconfig, phase must be
>>     PHASE_MACHINE_INITIALIZED
>> 
>>   - called from qemu_init() without -preconfig
>> 
>>   We now drain pending RCU callbacks.  Can any be pending at this
>>   early point?  If not, the change is a no-op.
>> 
>> * failover_add_primary() called from virtio_net_set_features() called as
>>   VirtioDeviceClass method set_features()
>> 
>>   This is virtio-net failover.  We now drain pending RCU callbacks.
>>   Impact?  Beats me.
>> 
>> My gut feeling is "improvement, possibly even a bug fix".  It deserves
>> its own commit, doesn't it?
>
> I thought I'd checked it, but now that I have reviewed the functions you
> looked at, I no longer think it's safe to move drain_call_rcu() into
> qdev_device_add_from_qdict(). drain_call_rcu() drops the BQL and must
> not be called from non-reentrant multi-threaded code paths.
>
> For example, if drain_call_rcu() is called from device emulation code
> then another vCPU thread could execute that same device's emulation code
> while the BQL is dropped. That's usually unsafe because most device
> emulation code only expects to run in one thread at any given time
> thanks to the BQL.
>
> Here are the cases:
> - qemu_create_cli_devices: safe because this happens before device
>   emulation runs in vCPU threads
> - Xen: safe because there are no vCPU threads when QEMU just provides
>   device emulation in a normal Xen system. But I'm not sure if usbback
>   can also be used in a QEMU's Xen HVM guest mode where KVM handles
>   vCPUs, it might be a problem there.
> - virtio_net_set_features() -> failover_add_primary() is unsafe. Another
>   vCPU thread could enter virtio-net emulation code while the thread
>   running virtio_net_set_features() drops the lock.

I see.

Can you make an argument why the reason for RCU draining does not apply
in failover_add_primary()?

If it doesn't apply, capturing the argument in a comment would be nice.

If it does apply, a FIXME comment seems in order.

Also, RCU draining / BQL dropping vs. non-draining/dropping behavior of
non-static functions should be documented in function comments.

All this can be done on top.

> I think it's better to duplicate drain_call_rcu() into hmp_device_add()
> than to risk breaking the other code paths.

Makes sense.

[PATCH v2 2/2] vl: use qmp_device_add() in qemu_create_cli_devices()

[Stefan Hajnoczi]

qemu_create_cli_devices() should use qmp_device_add() to match the
behavior of the QMP monitor. A comment explained that libvirt changes
implementing strict CLI syntax were needed.

Peter Krempa <pkrempa@redhat.com> has confirmed that modern libvirt uses
the same JSON for -device (CLI) and device_add (QMP). Go ahead and use
qmp_device_add().

Cc: Peter Krempa <pkrempa@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 system/vl.c | 14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/system/vl.c b/system/vl.c
index 9e8f16f155..0beb8bfb57 100644
--- a/system/vl.c
+++ b/system/vl.c
@@ -2651,17 +2651,11 @@ static void qemu_create_cli_devices(void)
     qemu_opts_foreach(qemu_find_opts("device"),
                       device_init_func, NULL, &error_fatal);
     QTAILQ_FOREACH(opt, &device_opts, next) {
-        DeviceState *dev;
+        QObject *ret_data = NULL;
+
         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.
-         */
-        dev = qdev_device_add_from_qdict(opt->opts, true, &error_fatal);
-        object_unref(OBJECT(dev));
+        qmp_device_add(opt->opts, &ret_data, &error_fatal);
+        assert(ret_data == NULL); /* error_fatal aborts */
         loc_pop(&opt->loc);
     }
     rom_reset_order_override();
-- 
2.45.2

Re: [PATCH v2 2/2] vl: use qmp_device_add() in qemu_create_cli_devices()

[Markus Armbruster]

Stefan Hajnoczi <stefanha@redhat.com> writes:

> qemu_create_cli_devices() should use qmp_device_add() to match the
> behavior of the QMP monitor. A comment explained that libvirt changes
> implementing strict CLI syntax were needed.
>
> Peter Krempa <pkrempa@redhat.com> has confirmed that modern libvirt uses
> the same JSON for -device (CLI) and device_add (QMP). Go ahead and use
> qmp_device_add().
>
> Cc: Peter Krempa <pkrempa@redhat.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  system/vl.c | 14 ++++----------
>  1 file changed, 4 insertions(+), 10 deletions(-)
>
> diff --git a/system/vl.c b/system/vl.c
> index 9e8f16f155..0beb8bfb57 100644
> --- a/system/vl.c
> +++ b/system/vl.c
> @@ -2651,17 +2651,11 @@ static void qemu_create_cli_devices(void)
>      qemu_opts_foreach(qemu_find_opts("device"),
>                        device_init_func, NULL, &error_fatal);
>      QTAILQ_FOREACH(opt, &device_opts, next) {
> -        DeviceState *dev;
> +        QObject *ret_data = NULL;
> +
>          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.
> -         */
> -        dev = qdev_device_add_from_qdict(opt->opts, true, &error_fatal);
> -        object_unref(OBJECT(dev));
> +        qmp_device_add(opt->opts, &ret_data, &error_fatal);
> +        assert(ret_data == NULL); /* error_fatal aborts */
>          loc_pop(&opt->loc);
>      }
>      rom_reset_order_override();

Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [PATCH v2 0/2] qdev-monitor: avoid QemuOpts in QMP device_add

[Markus Armbruster]

Can we additionally cut out the QemuOpts middleman in
usbback_portid_add()?

    qdict = qdict_new();
    qdict_put_str(qdict, "driver", "usb-host");
    tmp = g_strdup_printf("%s.0", usbif->xendev.qdev.id);
    qdict_put_str(qdict, "bus", tmp);
    g_free(tmp);
    tmp = g_strdup_printf("%s-%u", usbif->xendev.qdev.id, port);
    qdict_put_str(qdict, "id", tmp);
    g_free(tmp);
    qdict_put_int(qdict, "port", port);
    qdict_put_int(qdict, "hostbus", atoi(busid));
    qdict_put_str(qdict, "hostport", portname);
    opts = qemu_opts_from_qdict(qemu_find_opts("device"), qdict,
                                &error_abort);
    usbif->ports[port - 1].dev = USB_DEVICE(qdev_device_add(opts, &local_err));

Trying this is up to you!

Re: [PATCH v2 0/2] qdev-monitor: avoid QemuOpts in QMP device_add

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1081 bytes --]

On Fri, Aug 02, 2024 at 10:10:43AM +0200, Markus Armbruster wrote:
> Can we additionally cut out the QemuOpts middleman in
> usbback_portid_add()?
> 
>     qdict = qdict_new();
>     qdict_put_str(qdict, "driver", "usb-host");
>     tmp = g_strdup_printf("%s.0", usbif->xendev.qdev.id);
>     qdict_put_str(qdict, "bus", tmp);
>     g_free(tmp);
>     tmp = g_strdup_printf("%s-%u", usbif->xendev.qdev.id, port);
>     qdict_put_str(qdict, "id", tmp);
>     g_free(tmp);
>     qdict_put_int(qdict, "port", port);
>     qdict_put_int(qdict, "hostbus", atoi(busid));
>     qdict_put_str(qdict, "hostport", portname);
>     opts = qemu_opts_from_qdict(qemu_find_opts("device"), qdict,
>                                 &error_abort);
>     usbif->ports[port - 1].dev = USB_DEVICE(qdev_device_add(opts, &local_err));
> 
> Trying this is up to you!

Paul or Anthony: Do you know how to run usbback_portid_add() for
testing? I would like to make sure that suggested the code change works
and don't have experience running the Xen code in QEMU.

Thanks,
Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 0/2] qdev-monitor: avoid QemuOpts in QMP device_add

[Paul Durrant]

On 12/08/2024 19:15, Stefan Hajnoczi wrote:
> On Fri, Aug 02, 2024 at 10:10:43AM +0200, Markus Armbruster wrote:
>> Can we additionally cut out the QemuOpts middleman in
>> usbback_portid_add()?
>>
>>      qdict = qdict_new();
>>      qdict_put_str(qdict, "driver", "usb-host");
>>      tmp = g_strdup_printf("%s.0", usbif->xendev.qdev.id);
>>      qdict_put_str(qdict, "bus", tmp);
>>      g_free(tmp);
>>      tmp = g_strdup_printf("%s-%u", usbif->xendev.qdev.id, port);
>>      qdict_put_str(qdict, "id", tmp);
>>      g_free(tmp);
>>      qdict_put_int(qdict, "port", port);
>>      qdict_put_int(qdict, "hostbus", atoi(busid));
>>      qdict_put_str(qdict, "hostport", portname);
>>      opts = qemu_opts_from_qdict(qemu_find_opts("device"), qdict,
>>                                  &error_abort);
>>      usbif->ports[port - 1].dev = USB_DEVICE(qdev_device_add(opts, &local_err));
>>
>> Trying this is up to you!
> 
> Paul or Anthony: Do you know how to run usbback_portid_add() for
> testing? I would like to make sure that suggested the code change works
> and don't have experience running the Xen code in QEMU.

Sorry, PV USB is not something I'm familiar with. 
https://wiki.xenproject.org/wiki/Xen_USB_Passthrough suggests that `xl 
usbdev-attach` might be the way to test... but you'd need a system with 
Xen installed and suitably configured guest, so not trivial to set up.

Re: [PATCH v2 0/2] qdev-monitor: avoid QemuOpts in QMP device_add

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1695 bytes --]

On Tue, Aug 13, 2024 at 09:18:46AM +0100, Paul Durrant wrote:
> On 12/08/2024 19:15, Stefan Hajnoczi wrote:
> > On Fri, Aug 02, 2024 at 10:10:43AM +0200, Markus Armbruster wrote:
> > > Can we additionally cut out the QemuOpts middleman in
> > > usbback_portid_add()?
> > > 
> > >      qdict = qdict_new();
> > >      qdict_put_str(qdict, "driver", "usb-host");
> > >      tmp = g_strdup_printf("%s.0", usbif->xendev.qdev.id);
> > >      qdict_put_str(qdict, "bus", tmp);
> > >      g_free(tmp);
> > >      tmp = g_strdup_printf("%s-%u", usbif->xendev.qdev.id, port);
> > >      qdict_put_str(qdict, "id", tmp);
> > >      g_free(tmp);
> > >      qdict_put_int(qdict, "port", port);
> > >      qdict_put_int(qdict, "hostbus", atoi(busid));
> > >      qdict_put_str(qdict, "hostport", portname);
> > >      opts = qemu_opts_from_qdict(qemu_find_opts("device"), qdict,
> > >                                  &error_abort);
> > >      usbif->ports[port - 1].dev = USB_DEVICE(qdev_device_add(opts, &local_err));
> > > 
> > > Trying this is up to you!
> > 
> > Paul or Anthony: Do you know how to run usbback_portid_add() for
> > testing? I would like to make sure that suggested the code change works
> > and don't have experience running the Xen code in QEMU.
> 
> Sorry, PV USB is not something I'm familiar with.
> https://wiki.xenproject.org/wiki/Xen_USB_Passthrough suggests that `xl
> usbdev-attach` might be the way to test... but you'd need a system with Xen
> installed and suitably configured guest, so not trivial to set up.

Thanks for the pointer! I will leave the usbback_portid_add()
refactoring because I don't have a setup for testing it.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]