[PATCH v3 0/2] qdev-monitor: avoid QemuOpts in QMP device_add()

[PATCH v3 0/2] qdev-monitor: avoid QemuOpts in QMP device_add()

[Stefan Hajnoczi]

v3:
- Duplicate drain_call_rcu() into hmp_device_add() because moving it into
  qdev_device_add_from_qdict turned out to be unsafe.
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 | 44 ++++++++++++++++++++++++++++---------------
 system/vl.c           | 14 ++++----------
 2 files changed, 33 insertions(+), 25 deletions(-)

-- 
2.46.0

[PATCH v3 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.

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 | 44 ++++++++++++++++++++++++++++---------------
 1 file changed, 29 insertions(+), 15 deletions(-)

diff --git a/system/qdev-monitor.c b/system/qdev-monitor.c
index 6af6ef7d66..26404f314d 100644
--- a/system/qdev-monitor.c
+++ b/system/qdev-monitor.c
@@ -849,18 +849,9 @@ 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);
+    dev = qdev_device_add_from_qdict(qdict, true, errp);
     if (!dev) {
         /*
          * Drain all pending RCU callbacks. This is done because
@@ -872,11 +863,8 @@ void qmp_device_add(QDict *qdict, QObject **ret_data, Error **errp)
          * to the user
          */
         drain_call_rcu();
-
-        qemu_opts_del(opts);
-        return;
     }
-    object_unref(OBJECT(dev));
+    object_unref(dev);
 }
 
 static DeviceState *find_device_state(const char *id, Error **errp)
@@ -967,8 +955,34 @@ 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) {
+        /*
+         * 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);
+    }
+    object_unref(dev);
+out:
     hmp_handle_error(mon, err);
 }
 
-- 
2.46.0

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

[Daniel P. Berrangé]

On Tue, Aug 27, 2024 at 03:27:50PM -0400, Stefan Hajnoczi wrote:
> 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.
> 
> 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 | 44 ++++++++++++++++++++++++++++---------------
>  1 file changed, 29 insertions(+), 15 deletions(-)


Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


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: [PATCH v3 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.
>
> Markus helped me figure this out and even provided a draft patch. The
> code ended up very close to what he suggested.

Should we discuss the RCU draining issue here?

> 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 | 44 ++++++++++++++++++++++++++++---------------
>  1 file changed, 29 insertions(+), 15 deletions(-)
>
> diff --git a/system/qdev-monitor.c b/system/qdev-monitor.c
> index 6af6ef7d66..26404f314d 100644
> --- a/system/qdev-monitor.c
> +++ b/system/qdev-monitor.c
> @@ -849,18 +849,9 @@ 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);
> +    dev = qdev_device_add_from_qdict(qdict, true, errp);
>      if (!dev) {
>          /*
>           * Drain all pending RCU callbacks. This is done because
> @@ -872,11 +863,8 @@ void qmp_device_add(QDict *qdict, QObject **ret_data, Error **errp)
>           * to the user
>           */
>          drain_call_rcu();
> -
> -        qemu_opts_del(opts);
> -        return;

The removal of return gave me pause.  It's actually okay, because the
code we now execute in addition is a no-op: object_unref(NULL).

Matter of taste.

>      }
> -    object_unref(OBJECT(dev));
> +    object_unref(dev);

TIL that object_unref() takes a void *.

commit c5a61e5a3c68144a421117916aef04f2c0fab84b
Author: Daniel P. Berrangé <berrange@redhat.com>
Date:   Mon Aug 31 17:07:23 2020 -0400

    qom: make object_ref/unref use a void * instead of Object *.
    
    The object_ref/unref methods are intended for use with any subclass of
    the base Object. Using "Object *" in the signature is not adding any
    meaningful level of type safety, since callers simply use "OBJECT(ptr)"
    and this expands to an unchecked cast "(Object *)".
    
    By using "void *" we enable the object_unref() method to be used to
    provide support for g_autoptr() with any subclass.
    
    Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
    Message-Id: <20200723181410.3145233-2-berrange@redhat.com>
    Message-Id: <20200831210740.126168-2-ehabkost@redhat.com>
    Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>

About 2 out of 3 callers still pass an OBJECT(...) argument.  Similar
for object_ref().

If we believe dropping the OBJECT() is an improvement, we should do it
globally.

Suggest not to touch it in this patch.

>  }
>  
>  static DeviceState *find_device_state(const char *id, Error **errp)
> @@ -967,8 +955,34 @@ 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) {
> +        /*
> +         * 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);
> +    }
> +    object_unref(dev);
> +out:
>      hmp_handle_error(mon, err);
>  }

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

[Kevin Wolf]

Am 30.08.2024 um 09:29 hat Markus Armbruster geschrieben:
> 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).

What is the status of this?

This is a bug we certainly want to have fixed for 9.2. It's probably
also something for stable.

> > 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.
> >
> > Markus helped me figure this out and even provided a draft patch. The
> > code ended up very close to what he suggested.
> 
> Should we discuss the RCU draining issue here?

What RCU draining issue is this?

If I'm reading the patch correctly, it doesn't change anything related
to the RCU logic, but just inlines an existing call for HMP.

> > 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 | 44 ++++++++++++++++++++++++++++---------------
> >  1 file changed, 29 insertions(+), 15 deletions(-)
> >
> > diff --git a/system/qdev-monitor.c b/system/qdev-monitor.c
> > index 6af6ef7d66..26404f314d 100644
> > --- a/system/qdev-monitor.c
> > +++ b/system/qdev-monitor.c
> > @@ -849,18 +849,9 @@ 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);
> > +    dev = qdev_device_add_from_qdict(qdict, true, errp);
> >      if (!dev) {
> >          /*
> >           * Drain all pending RCU callbacks. This is done because
> > @@ -872,11 +863,8 @@ void qmp_device_add(QDict *qdict, QObject **ret_data, Error **errp)
> >           * to the user
> >           */
> >          drain_call_rcu();
> > -
> > -        qemu_opts_del(opts);
> > -        return;
> 
> The removal of return gave me pause.  It's actually okay, because the
> code we now execute in addition is a no-op: object_unref(NULL).
> 
> Matter of taste.
> 
> >      }
> > -    object_unref(OBJECT(dev));
> > +    object_unref(dev);
> 
> TIL that object_unref() takes a void *.
> 
> commit c5a61e5a3c68144a421117916aef04f2c0fab84b
> Author: Daniel P. Berrangé <berrange@redhat.com>
> Date:   Mon Aug 31 17:07:23 2020 -0400
> 
>     qom: make object_ref/unref use a void * instead of Object *.
>     
>     The object_ref/unref methods are intended for use with any subclass of
>     the base Object. Using "Object *" in the signature is not adding any
>     meaningful level of type safety, since callers simply use "OBJECT(ptr)"
>     and this expands to an unchecked cast "(Object *)".
>     
>     By using "void *" we enable the object_unref() method to be used to
>     provide support for g_autoptr() with any subclass.
>     
>     Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
>     Message-Id: <20200723181410.3145233-2-berrange@redhat.com>
>     Message-Id: <20200831210740.126168-2-ehabkost@redhat.com>
>     Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> 
> About 2 out of 3 callers still pass an OBJECT(...) argument.  Similar
> for object_ref().
> 
> If we believe dropping the OBJECT() is an improvement, we should do it
> globally.
> 
> Suggest not to touch it in this patch.

Is this the only change you want to see before this is merged?

If so, I can fix it up and take it through my tree.

Kevin

[PATCH v3 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>
Reviewed-by: Markus Armbruster <armbru@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 01b8b8e77a..3db63af764 100644
--- a/system/vl.c
+++ b/system/vl.c
@@ -2652,17 +2652,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.46.0

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

[Daniel P. Berrangé]

On Tue, Aug 27, 2024 at 03:27:51PM -0400, Stefan Hajnoczi wrote:
> 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>
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  system/vl.c | 14 ++++----------
>  1 file changed, 4 insertions(+), 10 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


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: [PATCH v3 0/2] qdev-monitor: avoid QemuOpts in QMP device_add()

[Kevin Wolf]

Am 27.08.2024 um 21:27 hat Stefan Hajnoczi geschrieben:
> v3:
> - Duplicate drain_call_rcu() into hmp_device_add() because moving it into
>   qdev_device_add_from_qdict turned out to be unsafe.
> 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.

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

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

[Kevin Wolf]

Am 27.08.2024 um 21:27 hat Stefan Hajnoczi geschrieben:
> v3:
> - Duplicate drain_call_rcu() into hmp_device_add() because moving it into
>   qdev_device_add_from_qdict turned out to be unsafe.
> 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.

Thanks, applied to the block branch.

Kevin