| Message ID | 20240827192751.948633-2-stefanha@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | qdev-monitor: avoid QemuOpts in QMP device_add() | expand |
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
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); > }
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); }
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(-)