Re: [Qemu-devel] [PATCH v14 12/21] option: allow qemu_opts_to_qdict to merge repeated options

[Daniel P. Berrange]

On Thu, Oct 13, 2016 at 10:31:37AM +0200, Markus Armbruster wrote:
> "Daniel P. Berrange" <address@hidden> writes:
> 
> > If given an option string such as
> >
> >   size=1024,nodes=10,nodes=4-5,nodes=1-2,policy=bind
> >
> > the qemu_opts_to_qdict() method will currently overwrite
> > the values for repeated option keys, so only the last
> > value is in the returned dict:
> >
> >     size=QString("1024")
> >     nodes=QString("1-2")
> >     policy=QString("bind")
> >
> > With this change the caller can optionally ask for all
> > the repeated values to be stored in a QList. In the
> > above example that would result in 'nodes' being a
> > QList, so the returned dict would contain
> >
> >     size=QString("1024")
> >     nodes=QList([QString("10"),
> >                  QString("4-5"),
> >                  QString("1-2")])
> >     policy=QString("bind")
> >
> > Note that the conversion has no way of knowing whether
> > any given key is expected to be a list upfront - it can
> > only figure that out when seeing the first duplicated
> > key. Thus the caller has to be prepared to deal with the
> > fact that if a key 'foo' is a list, then the returned
> > qdict may contain either a QString or a QList for the
> > key 'foo'.
> 
> Actually three cases, not two:
> 
> 0. qdict does not contain the key means empty list.
> 
> 1. qdict contains the key with a QString value means list of one
> element.
> 
> 2. qdict contains the key with a QList value means list of more than one
> element.
> 
> I consider this weird.  However, it's usefully weird with at least your
> QObject input visitor.
> 
> > In a third mode, it is possible to ask for repeated
> > options to be reported as an error, rather than silently
> > dropping all but the last one.
> 
> Got users for this policy in the pipeline?

I in fact used it in the QObjectInputVisitor, when the
autocreate_list is not set.

I guess strictly speaking this is not back-compatible
if someone is passing repeated keys, but I judged that
rather than silently ignoring this incorrect usage, it
was acceptable to report an error.




> >      QTAILQ_FOREACH(opt, &opts->head, next) {
> >          val = QOBJECT(qstring_from_str(opt->str));
> > -        qdict_put_obj(qdict, opt->name, val);
> > +
> > +        if (qdict_haskey(ret, opt->name)) {
> > +            switch (repeatPolicy) {
> > +            case QEMU_OPTS_REPEAT_POLICY_ERROR:
> > +                error_setg(errp, "Option '%s' appears more than once",
> > +                           opt->name);
> > +                qobject_decref(val);
> > +                if (!qdict) {
> > +                    QDECREF(ret);
> > +                }
> > +                return NULL;
> > +
> > +            case QEMU_OPTS_REPEAT_POLICY_ALL:
> > +                prevval = qdict_get(ret, opt->name);
> > +                if (qobject_type(prevval) == QTYPE_QLIST) {
> > +                    /* Already identified this key as a list */
> > +                    list = qobject_to_qlist(prevval);
> > +                } else {
> > +                    /* Replace current scalar with a list */
> > +                    list = qlist_new();
> > +                    qobject_incref(prevval);
> 
> Where is this reference decremented?

'prevval' is a borrowed reference from 'ret', against the
key opt->name.

qdict_put_obj decrements the reference we borrowed
from ret against the key opt->name. 

qlist_append_obj() takes ownership of the reference
it is passed, so we must qobject_incref() to avoid
qdict_put_obj free'ing prevval.

When we call qdict_put_obj() we're replacing the value
currently associted

> 
> > +                    qlist_append_obj(list, prevval);
> > +                    qdict_put_obj(ret, opt->name, QOBJECT(list));
> > +                }
> > +                qlist_append_obj(list, val);
> > +                break;
> > +
> > +            case QEMU_OPTS_REPEAT_POLICY_LAST:
> > +                /* Just discard previously set value */
> > +                qdict_put_obj(ret, opt->name, val);
> > +                break;
> > +            }
> > +        } else {
> > +            qdict_put_obj(ret, opt->name, val);
> > +        }
> 
> A possible alternative:
> 
>            val = QOBJECT(qstring_from_str(opt->str));
> 
>            if (qdict_haskey(ret, opt->name)) {
>                switch (repeatPolicy) {
>                case QEMU_OPTS_REPEAT_POLICY_ERROR:
>                    error_setg(errp, "Option '%s' appears more than once",
>                               opt->name);
>                    qobject_decref(val);
>                    if (!qdict) {
>                        QDECREF(ret);
>                    }
>                    return NULL;
> 
>                case QEMU_OPTS_REPEAT_POLICY_ALL:
>                    prevval = qdict_get(ret, opt->name);
>                    if (qobject_type(prevval) == QTYPE_QLIST) {
>                        /* Already identified this key as a list */
>                        list = qobject_to_qlist(prevval);
>                    } else {
>                        /* Replace current scalar with a list */
>                        list = qlist_new();
>                        qobject_incref(prevval);
>                        qlist_append_obj(list, prevval);
>                    }
>                    qlist_append_obj(list, val);
>                    val = QOBJECT(list);
>                    break;
> 
>                case QEMU_OPTS_REPEAT_POLICY_LAST:
>                    break;
>                }
>            }
>            qdict_put_obj(ret, opt->name, val);
> 
> This shows the common part of the behavior more clearly.  Matter of
> taste, you get to use your artistic license.


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