Stefan Hajnoczi <stefa...@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.
>
> Move the drain_call_rcu() invocation into qdev_device_add_from_qdict()
> so all callers benefit from it automatically. This avoids code
> duplication.
>
> 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 <arm...@redhat.com>
> Cc: Daniel P. Berrangé <berra...@redhat.com>
> Signed-off-by: Stefan Hajnoczi <stefa...@redhat.com>
> ---
> system/qdev-monitor.c | 56 ++++++++++++++++++++++---------------------
> 1 file changed, 29 insertions(+), 27 deletions(-)
>
> diff --git a/system/qdev-monitor.c b/system/qdev-monitor.c
> index 6af6ef7d66..8a756b1a91 100644
> --- a/system/qdev-monitor.c
> +++ b/system/qdev-monitor.c
> @@ -725,6 +725,17 @@ err_del_dev:
> if (dev) {
> object_unparent(OBJECT(dev));
> object_unref(OBJECT(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();
> }
> return NULL;
> }
Moving this from qmp_device_add() adds RCU draining to call chains not
going through qmp_device_add().
Can adding it hurt? I guess it can't.
Can it fix bugs? I don't know.
Let's review the callers of qdev_device_add_from_qdict():
* qdev_device_add()
- called from qmp_device_add()
No change.
- called from device_init_func() called from qemu_create_cli_devices()
See below.
- called from usbback_portid_add() called from usbback_process_port()
called from usbback_backend_changed()
· called from usbback_init()
· called as XenDevOps method backend_changed()
This is Xen. We now drain pending RCU callbacks. Impact? Beats
me.
* qemu_create_cli_devices() called from qmp_x_exit_preconfig()
- as QMP command with -preconfig, phase must be
PHASE_MACHINE_INITIALIZED
- called from qemu_init() without -preconfig
We now drain pending RCU callbacks. Can any be pending at this
early point? If not, the change is a no-op.
* failover_add_primary() called from virtio_net_set_features() called as
VirtioDeviceClass method set_features()
This is virtio-net failover. We now drain pending RCU callbacks.
Impact? Beats me.
My gut feeling is "improvement, possibly even a bug fix". It deserves
its own commit, doesn't it?
> @@ -849,34 +860,10 @@ 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);
> - 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);
> - return;
> - }
> - object_unref(OBJECT(dev));
> + dev = qdev_device_add_from_qdict(qdict, true, errp);
> + object_unref(dev);
> }
>
> static DeviceState *find_device_state(const char *id, Error **errp)
> @@ -967,8 +954,23 @@ 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) {
> + qemu_opts_del(opts);
> + }
> + object_unref(dev);
> +out:
> hmp_handle_error(mon, err);
> }
Remainder looks good to me.
