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

[Markus Armbruster <armbru@redhat.com>]

"Daniel P. Berrange" <berrange@redhat.com> writes:

> On Wed, Oct 12, 2016 at 11:18:21AM +0200, Markus Armbruster wrote:
>> "Daniel P. Berrange" <berrange@redhat.com> 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.

Fair point.

The bolted-on extensions (options visitor's integer list syntax, block
layer's dotted key convention) have only ever worked in the places that
choose to use them.  Even the integrated support for repeated keys is
only used for lists in the places that choose to use it that way.

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

Aha.

Manual reference counting is a PITA.

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

I'm suggesting a structure like this:

    1. Map special case to normal case
    2. Deal with normal case

Keeping the normal case and the special case separate like that can make
it easier to understand either.

Here's my attempt to do it:

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

The new code is the mapping.  The normal case code below remains
unchanged.  It's just a sketch; I didn't spend a single brainwave on the
reference counting.

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