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

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);
>  }

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

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