Re: [Qemu-devel] [PATCH v11 6/6] qom: support arbitrary non-scalar properties with -object

[Daniel P. Berrange]

On Mon, Sep 12, 2016 at 01:20:25PM -0500, Eric Blake wrote:
> On 09/05/2016 10:16 AM, Daniel P. Berrange wrote:
> > 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.
> > 
> 
> > The previously added qdict_crumple() method is able to
> > take a qdict containing a flat set of properties and
> > turn that into a arbitrarily nested set of dicts and
> > lists. By combining qemu_opts_to_qdict and qdict_crumple()
> > together, we can turn the opt string into a data structure
> > that is practically identical to that passed over QMP
> > when defining an object. The only difference is that all
> > the scalar values are represented as strings, rather than
> > strings, ints and bools. This is sufficient to let us
> > replace the OptsVisitor with the QMPInputVisitor for
> 
> QObjectInputVisitor
> 
> > use with -object.
> > 
> > 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.
> > 
> > This is also wired up to work for the 'object_add' command
> > in the HMP monitor with the same syntax.
> > 
> >   (hmp) object_add demo,id=demo0,\
> >                    foo.0.bar=one,foo.0.wizz=1,\
> >                    foo.1.bar=two,foo.1.wizz=2
> > 
> > NB indentation should not be used with HMP commands, this
> > is just for convenient formatting in this commit message.
> > 
> > Signed-off-by: Daniel P. Berrange <address@hidden>
> 
> I still haven't looked closely at the intermediate patches in this
> series, but I do like the end result, so I'll start with a review of
> this one.
> 
> > @@ -158,13 +167,21 @@ 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);
> >  
> > -    obj = user_creatable_add(pdict, v, errp);
> > +    pobj = qdict_crumple(pdict, true, errp);
> > +    if (!pobj) {
> > +        goto cleanup;
> > +    }
> > +    v = qobject_string_input_visitor_new(pobj);
> > +
> > +    obj = user_creatable_add((QDict *)pobj, v, errp);
> 
> This cast looks fishy; I think you want qobject_to_qdict(pobj) for
> absolute safety (if some future change causes QDict to not contain
> QObject at offset 0).  Besides, qdict_crumple() can return a QList
> instead of a QDict in some cases, so asserting that qobject_to_qdict()
> did not return NULL may be worthwhile to prove that this particular
> crumple never gives us something unexpected.
> 
> > +static void test_dummy_createopts(void)
> > +{
> > +    const char *optstr = 
> > "qemu-dummy,id=dummy0,bv=yes,av=alligator,sv=hiss,"
> > +        
> > "person.name=fred,person.age=52,sizes.0=12,sizes.1=65,sizes.2=8139,"
> > +        "addrs.0.ip=127.0.0.1,addrs.0.prefix=24,addrs.0.ipv6only=yes,"
> > +        "addrs.1.ip=0.0.0.0,addrs.1.prefix=16,addrs.1.ipv6only=no";
> > +    QemuOpts *opts;
> 
> Actually, what happens if I do an optstr of "qemu-dummy,1=foo,2=bar"?
> Is that rejected (because '1' and '2' are not valid key names) or does
> it try to create a 2-element list instead of the usual dict?  That is,
> if a list is something the user can trigger at the CLI, then we need
> better error handling than just an assertion that we actually get a
> QDict above.

With that example you give, qdict_crumple() will raise an error because
you have a mixture of list and non-list keys at the top level. IOW, the
crumple method is enforcing that you must only have a dict at the top.
I'll add a test for this to validate the behaviour too.

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