[PATCH] qdev-monitor: QAPIfy QMP device_add

[PATCH] qdev-monitor: QAPIfy 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). It's time to QAPIfy QMP device_add!

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.

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 | 41 ++++++++++++++++++++++++++++-------------
 1 file changed, 28 insertions(+), 13 deletions(-)

diff --git a/system/qdev-monitor.c b/system/qdev-monitor.c
index 6af6ef7d66..1427aa173c 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,8 +863,6 @@ 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));
@@ -967,8 +956,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(OBJECT(dev));
+out:
     hmp_handle_error(mon, err);
 }
 
-- 
2.45.2

Re: [PATCH] qdev-monitor: QAPIfy 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). It's time to QAPIfy QMP device_add!

This patch doesn't fully QAPIfy device_add: we still lack a schema
and use 'gen': false.  It gets us closer, though.

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

Another one is 9151e59a8b6e: it QAPIfied object-add.

Both commits eliminated the roundtrip through QemuOpts, and weaned the
command off 'gen': false.

This commit eliminates the roundtrip, but keeps 'gen': false.  Best we
can do now, but I'd like the commit message to make this clear.

> 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 | 41 ++++++++++++++++++++++++++++-------------
>  1 file changed, 28 insertions(+), 13 deletions(-)
>
> diff --git a/system/qdev-monitor.c b/system/qdev-monitor.c
> index 6af6ef7d66..1427aa173c 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,8 +863,6 @@ 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));
> @@ -967,8 +956,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;
> +    }

The part above is moved from qmp_device_add().  The part below is
copied.  The duplication is a bid sad.  Could we factor it out into
qdev_device_add_from_qdict()?

> +    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(OBJECT(dev));
> +out:
>      hmp_handle_error(mon, err);
>  }

Have a look at this TODO in vl.c:

    QTAILQ_FOREACH(opt, &device_opts, next) {
        DeviceState *dev;
        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));
        loc_pop(&opt->loc);
    }

Could we resolve it now?

Thanks for tackling this!

Re: [PATCH] qdev-monitor: QAPIfy QMP device_add

[Daniel P. Berrangé]

On Mon, Jul 08, 2024 at 04:30:27PM +0200, 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). It's time to QAPIfy QMP device_add!
> 
> 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.
> 
> 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 | 41 ++++++++++++++++++++++++++++-------------
>  1 file changed, 28 insertions(+), 13 deletions(-)

I think we're justified in saying that applications should have been
using the correct types already.

On the libvirt side we already switched to using JSON for -device,
which has forced us to ensure we're using the correct types. So
the risk of converting device_add is minimal from libvirt's POV.

Other non-libvirt mgmt apps might get tripped up. Fixing those
should not be too difficult and fixed code would remain compatible
with older QEMU versions too.

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


> 
> diff --git a/system/qdev-monitor.c b/system/qdev-monitor.c
> index 6af6ef7d66..1427aa173c 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,8 +863,6 @@ 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));
> @@ -967,8 +956,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(OBJECT(dev));
> +out:
>      hmp_handle_error(mon, err);
>  }
>  
> -- 
> 2.45.2
> 

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] qdev-monitor: QAPIfy QMP device_add

[Peter Krempa]

On Tue, Jul 09, 2024 at 16:27:22 +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). It's time to QAPIfy QMP device_add!
> 
> This patch doesn't fully QAPIfy device_add: we still lack a schema
> and use 'gen': false.  It gets us closer, though.
> 
> > 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.
> 
> Another one is 9151e59a8b6e: it QAPIfied object-add.
> 
> Both commits eliminated the roundtrip through QemuOpts, and weaned the
> command off 'gen': false.
> 
> This commit eliminates the roundtrip, but keeps 'gen': false.  Best we
> can do now, but I'd like the commit message to make this clear.
> 
> > 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 | 41 ++++++++++++++++++++++++++++-------------
> >  1 file changed, 28 insertions(+), 13 deletions(-)

[...]

> Have a look at this TODO in vl.c:
> 
>     QTAILQ_FOREACH(opt, &device_opts, next) {
>         DeviceState *dev;
>         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.
>          */

So at least this TODO should already be handled on libvirt's side.

With modern qemu libvirt is using JSON for -device and exactly the same
JSON for device_add. From what I remember from the time when I've
converted -device to use JSON, -device already required the correct
types, which would mean that also libvirt uses the correct types now.

Re: [PATCH] qdev-monitor: QAPIfy QMP device_add

[Stefan Hajnoczi]

On Wed, 31 Jul 2024 at 09:59, Peter Krempa <pkrempa@redhat.com> wrote:
>
> On Tue, Jul 09, 2024 at 16:27:22 +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). It's time to QAPIfy QMP device_add!
> >
> > This patch doesn't fully QAPIfy device_add: we still lack a schema
> > and use 'gen': false.  It gets us closer, though.
> >
> > > 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.
> >
> > Another one is 9151e59a8b6e: it QAPIfied object-add.
> >
> > Both commits eliminated the roundtrip through QemuOpts, and weaned the
> > command off 'gen': false.
> >
> > This commit eliminates the roundtrip, but keeps 'gen': false.  Best we
> > can do now, but I'd like the commit message to make this clear.
> >
> > > 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 | 41 ++++++++++++++++++++++++++++-------------
> > >  1 file changed, 28 insertions(+), 13 deletions(-)
>
> [...]
>
> > Have a look at this TODO in vl.c:
> >
> >     QTAILQ_FOREACH(opt, &device_opts, next) {
> >         DeviceState *dev;
> >         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.
> >          */
>
> So at least this TODO should already be handled on libvirt's side.
>
> With modern qemu libvirt is using JSON for -device and exactly the same
> JSON for device_add. From what I remember from the time when I've
> converted -device to use JSON, -device already required the correct
> types, which would mean that also libvirt uses the correct types now.

Great, thank you. I will update the code.

Stefan