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

[Daniel P. Berrange]

On Wed, Oct 12, 2016 at 11:18:21AM +0200, Markus Armbruster wrote:
> "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 ;)

Well, I call it "preferred" in the sense that that option syntax
directly maps to the QAPI syntax in an unambigous manner. ie
given the arg value alone "foo.0=hello,foo.1=world" you can clearly
determine that "foo" is a list. With the compat syntax you cannot
distinguish list from scalar, without knowing the QAPI schema.

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

The default behaviour would be that only the last key is present in
the dict, eg foo=world, and then if you tried to visit a list, the
visitor would complain that its got a QString instead of QList for
the key 'foo'.

This is related to patch 14

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

Essentially we'd be permanently allowing 2 distinct syntaxes for
dealing with lists, for all options. I felt it desirable that we
have only a single syntax and only allow this alt syntax in the
backcompat cases.




> > 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?

Previously the stack stored a borrowed reference, since it didn't
ever need responsibility for free'ing the object when popping the
stack. This is no longer the case if you look a few lines later....


> 
> > @@ -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);

... here we are storing the 'qmplist' in the stack, and so when
popping the stack, we must free that object. We thus need
the stack to always hold its own reference, so when popping
it can decref and (potentially) release the last reference.

> > +    } 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?

I'm not sure I understand what you mean here ?

> 
>        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...]

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://entangle-photo.org       -o-    http://search.cpan.org/~danberr/ :|