| Message ID | 20240708143027.480821-1-stefanha@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | qdev-monitor: QAPIfy QMP device_add | expand |
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!
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
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.
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
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); }
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(-)