Re: [Qemu-devel] [PATCH 7/7] qapi: support nested structs in OptsVisitor

[Eric Blake <eblake@redhat.com>]

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

On 09/07/2015 06:14 AM, Kővágó, Zoltán wrote:
> The current OptsVisitor flattens the whole structure, if there are same
> named fields under different paths (like `in' and `out' in `Audiodev'),
> the current visitor can't cope with them (for example setting
> `frequency=44100' will set the in's frequency to 44100 and leave out's
> frequency unspecified).
> 
> This patch fixes it, by always requiring a complete path in case of
> nested structs.  Fields in the path are separated by dots, similar to C
> structs (without pointers), like `in.frequency' or `out.frequency'.
> 
> You must provide a full path even in non-ambiguous cases.  The qapi
> flattening commits hopefully ensures that this change doesn't create
> backward compatibility problems.

It's the "hopefully" that worries me.

If I understand correctly, prior to this series, we have several
instances of qapi simple unions, where a member is a substruct when
converting to QObject.  Conceptually:

{ 'struct':'Sub', 'data':{ 'c':'int' } }
{ 'union':'Main', 'data':{ 'X':'Sub' } }

would accept the following QDict:

{ "type":"X", "data":{ "c":1 } }

and directly translating that to QemuOpts would be spelled:

-opt type=X,data.c=1

but we wanted shorthand:

-opt type=X,c=1

So, the "flattening" that opts-visitor does is what allows "c" instead
of "data.c" to refer to a nested member of a union.  It worked as long
as 'c' appeared only once per visit of the overall qapi type (multiple
branches of the union may have 'c', but for a given union branch, 'c'
appears only once no matter what depth of substructs it is reached through).

Question: do we actually ALLOW the user to specify data.c, or do we ONLY
accept the shorthand?


This series then proceeds to teach opts-visitor how to handle flat
unions.  Converting the above example to a flat union is done by:

{ 'enum':'Foo', 'data':['X'] }
{ 'struct':'Base', 'data':{ 'type':'Foo' } }
{ 'struct':'Sub', 'data':{ 'c':'int' } }
{ 'union':'Main', 'base':'Base', 'discriminator':'type',
  'data':{ 'X':'Sub' } }

which now accepts the following QDict:

{ "type":"X", "c":1" }

(note the loss of the nested 'data' struct), and directly translating
that to QemuOpts would be spelled:

-opt type=X,c=1

which exactly matches the shorthand that we previously accepted.  It
completely gets rid of the data.c longhand, but I don't know if we
accepted that in the first place.

So, the next question is whether we still need flattening.  If we no
longer have any simple unions parsed by QemuOpts, then the flattening no
longer helps us.  Furthermore, you have a case in audio where flattening
hurts; the QDict:

{ "in":{ "frequency":4400 }, "out":{ "frequency":4400 } }

has unambiguous paths 'in.frequency' and 'out.frequency', but the
shorthand 'frequency' could now match two separate places in the struct.

Now, let's look at what your testsuite changes say about this patch:

> @@ -271,6 +299,12 @@ main(int argc, char **argv)
>      add_test("/visitor/opts/i64/range/2big/full", &expect_fail,
>               "i64=-0x8000000000000000-0x7fffffffffffffff");
>  
> +    /* Test nested structs support */
> +    add_test("/visitor/opts/nested/unqualified", &expect_fail, "nint=13");

You are saying that an unqualified member no longer resolves (that is,
when nesting is in place, we MUST spell it by the long name);

> +    add_test("/visitor/opts/nested/both",        &expect_both,
> +             "sub0.nint=13,sub1.nint=17");
> +    add_test("/visitor/opts/nested/sub0",        &expect_sub0, "sub0.nint=13");
> +    add_test("/visitor/opts/nested/sub1",        &expect_sub1, "sub1.nint=13");

and that using qualified names gets at the nested struct members as
desired.  Furthermore, if we truly have converted ALL qapi unions to
flat unions, and if qapi unions were the ONLY reason that we had
flattening in the first place, and if we did NOT accept longhand
'data.c=1' for the flattened shorthand of 'c=1' within a simple union
branch, then it looks like you are 100% backwards-compatible.  But
that's quite a few things to verify.  Also, I'm not sure if your visitor
approaches the change of no longer flattening things in the most
efficient manner (I still haven't studied the rest of the patch closely).

I'd like to defer reviewing this until the qapi patch queue is not quite
so long, but the premise behind it is appealing.  However, I strongly
recommend that the commit message be improved to cover some of the
background that I spelled out here, as well as offering more proof than
just "hopefully", and an analysis of whether we are breaking any
longhand data.c=1 option spellings.

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