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

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 4073 bytes --]

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 <berrange@redhat.com>

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.

But overall I like this patch.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]