Re: [Qemu-block] [Qemu-devel] [PATCH v14 15/21] qom: support non-scalar properties with -object

[Markus Armbruster]

"Daniel P. Berrange" <address@hidden> writes:

> The current -object command line syntax only allows for
> creation of objects with scalar properties, or a list
> with a fixed scalar element type. Objects which have
> properties that are represented as structs in the QAPI
> schema cannot be created using -object.
>
> This is a design limitation of the way the OptsVisitor
> is written. It simply iterates over the QemuOpts values
> as a flat list. The support for lists is enabled by
> allowing the same key to be repeated in the opts string.
>
> The QObjectInputVisitor now has functionality that allows
> it to replace OptsVisitor, maintaining full backwards
> compatibility for previous CLI syntax, while also allowing
> use of new syntax for structs.
>
> Thus -object can now support non-scalar properties,
> for example the QMP object:
>
>   {
>     "execute": "object-add",
>     "arguments": {
>       "qom-type": "demo",
>       "id": "demo0",
>       "parameters": {
>         "foo": [
>           { "bar": "one", "wizz": "1" },
>           { "bar": "two", "wizz": "2" }
>         ]
>       }
>     }
>   }
>
> Would be creatable via the CLI now using
>
>     $QEMU \
>       -object demo,id=demo0,\
>               foo.0.bar=one,foo.0.wizz=1,\
>               foo.1.bar=two,foo.1.wizz=2
>
> Notice that this syntax is intentionally compatible
> with that currently used by block drivers. NB the
> indentation seen here after line continuations should
> not be used in reality, it is just for clarity in this
> commit message.
>
> Signed-off-by: Daniel P. Berrange <address@hidden>
> ---
>  qapi/qobject-input-visitor.c |   2 +-
>  qom/object_interfaces.c      |  37 ++++-
>  tests/check-qom-proplist.c   | 367 
> ++++++++++++++++++++++++++++++++++++++++++-
>  3 files changed, 397 insertions(+), 9 deletions(-)
>
> diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c
> index 5a3872c..f1030d5 100644
> --- a/qapi/qobject-input-visitor.c
> +++ b/qapi/qobject-input-visitor.c
> @@ -204,7 +204,7 @@ static void qobject_input_start_struct(Visitor *v, const 
> char *name, void **obj,
>          *obj = NULL;
>      }
>  
> -    if (!qobj && (qiv->struct_level < qiv->autocreate_struct_levels)) {
> +    if (!qobj && (qiv->struct_level <= qiv->autocreate_struct_levels)) {
>          /* Create a new dict that contains all the currently
>           * unvisited items */
>          if (tos) {

Uh, can you explain this hunk?  The < comes from PATCH 10.

> diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
> index fdc406b..88a1e88 100644
> --- a/qom/object_interfaces.c
> +++ b/qom/object_interfaces.c
> @@ -4,7 +4,8 @@
>  #include "qemu/module.h"
>  #include "qapi-visit.h"
>  #include "qapi/qobject-output-visitor.h"
> -#include "qapi/opts-visitor.h"
> +#include "qapi/qobject-input-visitor.h"
> +#include "qemu/option.h"
>  
>  void user_creatable_complete(Object *obj, Error **errp)
>  {
> @@ -63,12 +64,16 @@ Object *user_creatable_add(const QDict *qdict,
>      if (local_err) {
>          goto out_visit;
>      }
> -    visit_check_struct(v, &local_err);
> +
> +    obj = user_creatable_add_type(type, id, pdict, v, &local_err);
>      if (local_err) {
>          goto out_visit;
>      }
>  
> -    obj = user_creatable_add_type(type, id, pdict, v, &local_err);
> +    visit_check_struct(v, &local_err);
> +    if (local_err) {
> +        goto out_visit;
> +    }
>  
>  out_visit:
>      visit_end_struct(v, NULL);

Can you explain why you swap the order of visit_check_struct() and
user_creatable_add_type()?  It smells like a bug fix to me...

Odd: opts_check_struct() does nothing unless depth == 0.  But depth is
at least 1 between opts_start_struct() and opts_end_struct().  Bug?

> @@ -114,7 +119,7 @@ Object *user_creatable_add_type(const char *type, const 
> char *id,
>  
>      assert(qdict);
>      obj = object_new(type);
> -    visit_start_struct(v, NULL, NULL, 0, &local_err);
> +    visit_start_struct(v, "props", NULL, 0, &local_err);
>      if (local_err) {
>          goto out;
>      }

Why this hunk?

> @@ -158,14 +163,32 @@ Object *user_creatable_add_opts(QemuOpts *opts, Error 
> **errp)
>  {
>      Visitor *v;
>      QDict *pdict;
> +    QObject *pobj;
>      Object *obj = NULL;
>  
> -    v = opts_visitor_new(opts);
> -    pdict = qemu_opts_to_qdict(opts, NULL, QEMU_OPTS_REPEAT_POLICY_LAST,
> +    pdict = qemu_opts_to_qdict(opts, NULL,
> +                               QEMU_OPTS_REPEAT_POLICY_ALL,
>                                 &error_abort);
>  
> -    obj = user_creatable_add(pdict, v, errp);
> +    pobj = qdict_crumple(pdict, true, errp);
> +    if (!pobj) {
> +        goto cleanup;
> +    }
> +    /*
> +     * We need autocreate_list=true + permit_int_ranges=true
> +     * in order to maintain compat with OptsVisitor creation
> +     * of the 'numa' object which had an int16List property.

I can't find a "numa" object in the object-add sense, only a NumaOptions
QAPI object created by -numa.  Is that what you mean?

> +     *
> +     * We need autocreate_structs=1, because at the CLI level
> +     * we have the object type + properties in the same flat
> +     * struct, even though at the QMP level they are nested.

We screwed up QMP object-add.  device_add, netdev_add and object_add all
treat additional arguments as properties *except* for QMP object-add,
which has them wrapped in a JSON object instead.

Aside: if someone foolishly adds a property named "qom-type" or "id",
it'll work in QMP but not in HMP or -object.

The inconsistency complicates the code even before this patch:

* Core: user_creatable_add_type() takes properties in *two* forms: as a
  visitor and as a qdict.  It visits a struct with the members given by
  the qdict's keys.  That's its only use of the qdict.

* QMP: qmp_object_add() gets properties as a separate QDict.  It creates
  a QObject visitor for it, and passes it to user_creatable_add_type()
  along with the QDict.

* -object: user_creatable_add_opts() gets the option argument as a
  QemuOpts.  It creates an options visitor for it *and* converts it to a
  QDict, then passes both to user_creatable_add().

* user_creatable_add() visits a struct.  It first visits the
  non-property arguments "qom-type" and "id".  It passes the visitor and
  a shallow copy of the QDict with the non-property entries deleted to
  user_creatable_add_type() to visit the remaining struct members.

* HMP: hmp_object_add() gets its arguments as a QDict.  It converts it
  to QemuOpts and creates an options visitor for it *boggle*.  It passes
  the visitor along with original QDict to user_creatable_add().

I still can't quite see what's requiring autocreate_structs.  But I hope
we can get rid of it by cleaning up this mess a bit.  Here's how I'd
try:

* Move visit_start_struct() and visit_end_struct() from
  user_creatable_add_type() to its two callers qmp_object_add() and
  user_creatable_add().

* Make user_creatable_add_type() ignore QDict keys "qom-type" and "id".

* QMP: any attempt to specify property "qom-type" or "id" now fails,
  because visit_check_struct() flags them as unexpected.

* user_creatable_add(): ditch the silly QDict copying.

* hmp_object_add(): ditch the silly conversion to QemuOpts, and create a
  QObject visitor instead.

> +     */
> +    v = qobject_input_visitor_new_autocast(pobj, true, 1, true);
> +
> +    obj = user_creatable_add(qobject_to_qdict(pobj), v, errp);
>      visit_free(v);
> +    qobject_decref(pobj);
> + cleanup:
>      QDECREF(pdict);
>      return obj;
>  }
[Skipping test updates for now...]