Re: [PATCH v3 1/2] qdev-monitor: avoid QemuOpts in 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).
>
> 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);
>  }