Re: [PULL 13/29] qapi: Flatten object-add

[Paolo Bonzini]

On 06/03/20 18:14, Kevin Wolf wrote:
> Mapping object-add to the command line as is doesn't result in nice
> syntax because of the nesting introduced with 'props'. This becomes
> nicer and more consistent with device_add and netdev_add when we accept
> properties for the object on the top level instead.
> 
> 'props' is still accepted after this patch, but marked as deprecated.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> Message-Id: <20200224143008.13362-8-kwolf@redhat.com>
> Acked-by: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>

Hi Kevin and Markus,

I just noticed this patch.  In my opinion "nice syntax" is not a good
argument for having a "gen: false" command that is even less
introspectable than the "props" argument.

As an aside, it would have been nice to run this through Markus and me,
though in all fairness I'm not sure I would have been responsive back in
February.

I would like to un-deprecate this for 5.1, and revert it in either 5.1
or 5.2.  (Also I will be away next week, so the decision would have to
be taken quickly).

Paolo


> ---
>  qapi/qom.json                   | 12 +++++++---
>  docs/system/deprecated.rst      |  5 ++++
>  include/qom/object_interfaces.h |  7 ++++++
>  hw/block/xen-block.c            | 11 ++++++++-
>  monitor/misc.c                  |  2 ++
>  qom/qom-qmp-cmds.c              | 42 +++++++++++++++++++++++++++------
>  6 files changed, 68 insertions(+), 11 deletions(-)
> 
> diff --git a/qapi/qom.json b/qapi/qom.json
> index ecc60c4401..8abe998962 100644
> --- a/qapi/qom.json
> +++ b/qapi/qom.json
> @@ -210,7 +210,12 @@
>  #
>  # @id: the name of the new object
>  #
> -# @props: a dictionary of properties to be passed to the backend
> +# @props: a dictionary of properties to be passed to the backend. Deprecated
> +#         since 5.0, specify the properties on the top level instead. It is 
> an
> +#         error to specify the same option both on the top level and in 
> @props.
> +#
> +# Additional arguments depend on qom-type and are passed to the backend
> +# unchanged.
>  #
>  # Returns: Nothing on success
>  #          Error if @qom-type is not a valid class name
> @@ -221,12 +226,13 @@
>  #
>  # -> { "execute": "object-add",
>  #      "arguments": { "qom-type": "rng-random", "id": "rng1",
> -#                     "props": { "filename": "/dev/hwrng" } } }
> +#                     "filename": "/dev/hwrng" } }
>  # <- { "return": {} }
>  #
>  ##
>  { 'command': 'object-add',
> -  'data': {'qom-type': 'str', 'id': 'str', '*props': 'any'} }
> +  'data': {'qom-type': 'str', 'id': 'str', '*props': 'any'},
> +  'gen': false } # so we can get the additional arguments
>  
>  ##
>  # @object-del:
> diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst
> index 1eaa559079..6c1d9034d9 100644
> --- a/docs/system/deprecated.rst
> +++ b/docs/system/deprecated.rst
> @@ -190,6 +190,11 @@ Use ``migrate-set-parameters`` instead.
>  
>  Use ``migrate-set-parameters`` and ``query-migrate-parameters`` instead.
>  
> +``object-add`` option ``props`` (since 5.0)
> +'''''''''''''''''''''''''''''''''''''''''''
> +
> +Specify the properties for the object as top-level arguments instead.
> +
>  ``query-block`` result field ``dirty-bitmaps[i].status`` (since 4.0)
>  ''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''
>  
> diff --git a/include/qom/object_interfaces.h b/include/qom/object_interfaces.h
> index 3e4e1d928b..6f92f3cebb 100644
> --- a/include/qom/object_interfaces.h
> +++ b/include/qom/object_interfaces.h
> @@ -162,4 +162,11 @@ void user_creatable_del(const char *id, Error **errp);
>   */
>  void user_creatable_cleanup(void);
>  
> +/**
> + * qmp_object_add:
> + *
> + * QMP command handler for object-add. See the QAPI schema for documentation.
> + */
> +void qmp_object_add(QDict *qdict, QObject **ret_data, Error **errp);
> +
>  #endif
> diff --git a/hw/block/xen-block.c b/hw/block/xen-block.c
> index 686bbc3f0d..3885464513 100644
> --- a/hw/block/xen-block.c
> +++ b/hw/block/xen-block.c
> @@ -18,6 +18,7 @@
>  #include "qapi/visitor.h"
>  #include "qapi/qmp/qdict.h"
>  #include "qapi/qmp/qstring.h"
> +#include "qom/object_interfaces.h"
>  #include "hw/xen/xen_common.h"
>  #include "hw/block/xen_blkif.h"
>  #include "hw/qdev-properties.h"
> @@ -858,10 +859,18 @@ static XenBlockIOThread 
> *xen_block_iothread_create(const char *id,
>  {
>      XenBlockIOThread *iothread = g_new(XenBlockIOThread, 1);
>      Error *local_err = NULL;
> +    QDict *opts;
> +    QObject *ret_data;
>  
>      iothread->id = g_strdup(id);
>  
> -    qmp_object_add(TYPE_IOTHREAD, id, false, NULL, &local_err);
> +    opts = qdict_new();
> +    qdict_put_str(opts, "qom-type", TYPE_IOTHREAD);
> +    qdict_put_str(opts, "id", id);
> +    qmp_object_add(opts, &ret_data, &local_err);
> +    qobject_unref(opts);
> +    qobject_unref(ret_data);
> +
>      if (local_err) {
>          error_propagate(errp, local_err);
>  
> diff --git a/monitor/misc.c b/monitor/misc.c
> index 6c41293102..1748ab3911 100644
> --- a/monitor/misc.c
> +++ b/monitor/misc.c
> @@ -248,6 +248,8 @@ static void monitor_init_qmp_commands(void)
>                           QCO_NO_OPTIONS);
>      qmp_register_command(&qmp_commands, "netdev_add", qmp_netdev_add,
>                           QCO_NO_OPTIONS);
> +    qmp_register_command(&qmp_commands, "object-add", qmp_object_add,
> +                         QCO_NO_OPTIONS);
>  
>      QTAILQ_INIT(&qmp_cap_negotiation_commands);
>      qmp_register_command(&qmp_cap_negotiation_commands, "qmp_capabilities",
> diff --git a/qom/qom-qmp-cmds.c b/qom/qom-qmp-cmds.c
> index 6136efec16..49db926fcc 100644
> --- a/qom/qom-qmp-cmds.c
> +++ b/qom/qom-qmp-cmds.c
> @@ -14,6 +14,7 @@
>   */
>  
>  #include "qemu/osdep.h"
> +#include "block/qdict.h"
>  #include "hw/qdev-core.h"
>  #include "qapi/error.h"
>  #include "qapi/qapi-commands-qdev.h"
> @@ -240,13 +241,34 @@ ObjectPropertyInfoList *qmp_qom_list_properties(const 
> char *typename,
>      return prop_list;
>  }
>  
> -void qmp_object_add(const char *type, const char *id,
> -                    bool has_props, QObject *props, Error **errp)
> +void qmp_object_add(QDict *qdict, QObject **ret_data, Error **errp)
>  {
> +    QObject *props;
>      QDict *pdict;
>      Visitor *v;
>      Object *obj;
> +    const char *type;
> +    const char *id;
>  
> +    type = qdict_get_try_str(qdict, "qom-type");
> +    if (!type) {
> +        error_setg(errp, QERR_MISSING_PARAMETER, "qom-type");
> +        return;
> +    } else {
> +        type = g_strdup(type);
> +        qdict_del(qdict, "qom-type");
> +    }
> +
> +    id = qdict_get_try_str(qdict, "id");
> +    if (!id) {
> +        error_setg(errp, QERR_MISSING_PARAMETER, "id");
> +        return;
> +    } else {
> +        id = g_strdup(id);
> +        qdict_del(qdict, "id");
> +    }
> +
> +    props = qdict_get(qdict, "props");
>      if (props) {
>          pdict = qobject_to(QDict, props);
>          if (!pdict) {
> @@ -254,17 +276,23 @@ void qmp_object_add(const char *type, const char *id,
>              return;
>          }
>          qobject_ref(pdict);
> -    } else {
> -        pdict = qdict_new();
> +        qdict_del(qdict, "props");
> +        qdict_join(qdict, pdict, false);
> +        if (qdict_size(pdict) != 0) {
> +            error_setg(errp, "Option in 'props' conflicts with top level");
> +            qobject_unref(pdict);
> +            return;
> +        }
> +        qobject_unref(pdict);
>      }
>  
> -    v = qobject_input_visitor_new(QOBJECT(pdict));
> -    obj = user_creatable_add_type(type, id, pdict, v, errp);
> +    v = qobject_input_visitor_new(QOBJECT(qdict));
> +    obj = user_creatable_add_type(type, id, qdict, v, errp);
>      visit_free(v);
>      if (obj) {
>          object_unref(obj);
>      }
> -    qobject_unref(pdict);
> +    *ret_data = QOBJECT(qdict_new());
>  }
>  
>  void qmp_object_del(const char *id, Error **errp)
>