Re: [Qemu-devel] [PATCH v14 09/21] qapi: permit auto-creating single element lists

[Markus Armbruster]

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

> When converting QemuOpts to a QObject, there is no information
> about compound types available,

Yes, that's a drawback of splitting the conversion into a QemuOpts ->
QObject part that is oblivious of types, and a QObject -> QAPI object
part that knows the types.

>                                 so when visiting a list, the
> corresponding QObject is not guaranteed to be a QList. We
> therefore need to be able to auto-create a single element QList
> from whatever type we find.
>
> This mode should only be enabled if you have compatibility
> requirements for
>
>    -arg foo=hello,foo=world
>
> to be treated as equivalent to the preferred syntax:
>
>    -arg foo.0=hello,foo.1=world

Not sure this is "preferred".  "More powerfully warty" is probably
closer to the truth ;)

How is "-arg foo=hello,foo=world" treated if this mode isn't enabled?

What would be the drawbacks of doing this always instead of only when we
"have compatibility requirements"?

> Signed-off-by: Daniel P. Berrange <address@hidden>
> ---
>  include/qapi/qobject-input-visitor.h | 20 +++++++-
>  qapi/qobject-input-visitor.c         | 27 +++++++++--
>  tests/test-qobject-input-visitor.c   | 88 
> +++++++++++++++++++++++++++++++-----
>  3 files changed, 117 insertions(+), 18 deletions(-)
>
> diff --git a/include/qapi/qobject-input-visitor.h 
> b/include/qapi/qobject-input-visitor.h
> index f134d90..1809f48 100644
> --- a/include/qapi/qobject-input-visitor.h
> +++ b/include/qapi/qobject-input-visitor.h
> @@ -42,6 +42,19 @@ Visitor *qobject_input_visitor_new(QObject *obj, bool 
> strict);
>   * represented as strings. i.e. if visiting a boolean, the value should
>   * be a QString whose contents represent a valid boolean.
>   *
> + * If @autocreate_list is true, then as an alternative to a normal QList,
> + * list values can be stored as a QString or QDict instead, which will
> + * be interpreted as representing single element lists. This should only
> + * by used if compatibility is required with the OptsVisitor which allowed
> + * repeated keys, without list indexes, to represent lists. e.g. set this
> + * to true if you have compatibility requirements for
> + *
> + *   -arg foo=hello,foo=world
> + *
> + * to be treated as equivalent to the preferred syntax:
> + *
> + *   -arg foo.0=hello,foo.1=world
> + *
>   * The visitor always operates in strict mode, requiring all dict keys
>   * to be consumed during visitation. An error will be reported if this
>   * does not happen.
> @@ -49,7 +62,8 @@ Visitor *qobject_input_visitor_new(QObject *obj, bool 
> strict);
>   * The returned input visitor should be released by calling
>   * visit_free() when no longer required.
>   */
> -Visitor *qobject_input_visitor_new_autocast(QObject *obj);
> +Visitor *qobject_input_visitor_new_autocast(QObject *obj,
> +                                            bool autocreate_list);
>  
>  
>  /**
> @@ -64,6 +78,8 @@ Visitor *qobject_input_visitor_new_autocast(QObject *obj);
>   * The returned input visitor should be released by calling
>   * visit_free() when no longer required.
>   */
> -Visitor *qobject_input_visitor_new_opts(const QemuOpts *opts, Error **errp);
> +Visitor *qobject_input_visitor_new_opts(const QemuOpts *opts,
> +                                        bool autocreate_list,
> +                                        Error **errp);
>  
>  #endif
> diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c
> index d9269c9..d88e9f9 100644
> --- a/qapi/qobject-input-visitor.c
> +++ b/qapi/qobject-input-visitor.c
> @@ -48,6 +48,10 @@ struct QObjectInputVisitor
>  
>      /* True to reject parse in visit_end_struct() if unvisited keys remain. 
> */
>      bool strict;
> +
> +    /* Whether we can auto-create single element lists when
> +     * encountering a non-QList type */
> +    bool autocreate_list;
>  };
>  
>  static QObjectInputVisitor *to_qiv(Visitor *v)
> @@ -108,6 +112,7 @@ static const QListEntry 
> *qobject_input_push(QObjectInputVisitor *qiv,
>      assert(obj);
>      tos->obj = obj;
>      tos->qapi = qapi;
> +    qobject_incref(obj);
>  
>      if (qiv->strict && qobject_type(obj) == QTYPE_QDICT) {
>          h = g_hash_table_new(g_str_hash, g_str_equal);
> @@ -147,6 +152,7 @@ static void qobject_input_stack_object_free(StackObject 
> *tos)
>      if (tos->h) {
>          g_hash_table_unref(tos->h);
>      }
> +    qobject_decref(tos->obj);
>  
>      g_free(tos);
>  }

Can you explain the reference counting change?

> @@ -197,7 +203,7 @@ static void qobject_input_start_list(Visitor *v, const 
> char *name,
>      QObject *qobj = qobject_input_get_object(qiv, name, true);
>      const QListEntry *entry;
>  
> -    if (!qobj || qobject_type(qobj) != QTYPE_QLIST) {
> +    if (!qobj || (!qiv->autocreate_list && qobject_type(qobj) != 
> QTYPE_QLIST)) {

Long line, but I believe it'll go away when you rebase for commit
1382d4a.

>          if (list) {
>              *list = NULL;
>          }
> @@ -206,7 +212,16 @@ static void qobject_input_start_list(Visitor *v, const 
> char *name,
>          return;
>      }
>  
> -    entry = qobject_input_push(qiv, qobj, list, errp);
> +    if (qobject_type(qobj) != QTYPE_QLIST) {
> +        QList *tmplist = qlist_new();
> +        qlist_append_obj(tmplist, qobj);
> +        qobject_incref(qobj);
> +        entry = qobject_input_push(qiv, QOBJECT(tmplist), list, errp);
> +        QDECREF(tmplist);
> +    } else {
> +        entry = qobject_input_push(qiv, qobj, list, errp);
> +    }
> +
>      if (list) {
>          if (entry) {
>              *list = g_malloc0(size);

Buries autolist behavior in the middle of things.  What about doing it
first, so it's more separate?

       QObjectInputVisitor *qiv = to_qiv(v);
       QObject *qobj = qobject_input_get_object_(qiv, name, true, errp);
       const QListEntry *entry;

       if (!qobj) {
           return;
       }
   
  +    if (qiv->autocreate_list && qobject_type(qobj) != QTYPE_QLIST) {
  +        QList *auto_list = qlist_new();
  +        qlist_append_obj(auto_list, qobj);
  +        qobj = auto_list;
  +    }
  +
       if (qobject_type(qobj) != QTYPE_QLIST) {

I ignored reference counting here, because I don't yet understand how
and why you're changing it.

> @@ -514,7 +529,8 @@ Visitor *qobject_input_visitor_new(QObject *obj, bool 
> strict)
>      return &v->visitor;
>  }
>  
> -Visitor *qobject_input_visitor_new_autocast(QObject *obj)
> +Visitor *qobject_input_visitor_new_autocast(QObject *obj,
> +                                            bool autocreate_list)
>  {
>      QObjectInputVisitor *v;
>  
> @@ -539,6 +555,7 @@ Visitor *qobject_input_visitor_new_autocast(QObject *obj)
>      v->visitor.optional = qobject_input_optional;
>      v->visitor.free = qobject_input_free;
>      v->strict = true;
> +    v->autocreate_list = autocreate_list;
>  
>      v->root = obj;
>      qobject_incref(obj);
> @@ -548,6 +565,7 @@ Visitor *qobject_input_visitor_new_autocast(QObject *obj)
>  
>  
>  Visitor *qobject_input_visitor_new_opts(const QemuOpts *opts,
> +                                        bool autocreate_list,
>                                          Error **errp)
>  {
>      QDict *pdict;
> @@ -564,7 +582,8 @@ Visitor *qobject_input_visitor_new_opts(const QemuOpts 
> *opts,
>          goto cleanup;
>      }
>  
> -    v = qobject_input_visitor_new_autocast(pobj);
> +    v = qobject_input_visitor_new_autocast(pobj,
> +                                           autocreate_list);
>   cleanup:
>      qobject_decref(pobj);
>      QDECREF(pdict);
[Skipping test updates for now...]