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

["Kővágó Zoltán" <dirty.ice.hu@gmail.com>]

2015-09-17 23:24 keltezéssel, Eric Blake írta:
> 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?

The current implementation in qemu only allows the shorthand version, in 
fact it doesn't even care about the name of the parent struct/union. 
Neither Main nor Sub has a field names 'data.c', so the data.c=1 version 
is currently invalid.  But the problem is, as you noted, it only works 
if each field name is unique in the whole visited type (excluding 
non-visited union branches).

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

No, data.c was never valid.

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

This is why exactly the flattening needs to be moved from OptsVisitor to 
somewhere else.  If the qapi structs are flattened, old options (-net, 
-numa) continue to work as before, while at allows -audiodev to specify 
in.frequency and out.frequency.  Of course, this isn't the only possible 
approach, but my previous attempt where I modified OptsVisitor instead 
to allow both shorthand and full paths were also problematic:

http://lists.nongnu.org/archive/html/qemu-devel/2015-06/msg04189.html

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