Re: [Qemu-devel] [PATCH V7 04/11] qapi script: check correctness of discriminator values in union

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> On 02/20/2014 07:43 AM, Markus Armbruster wrote:
>> Wenchao Xia <address@hidden> writes:
>> 
>>> It will check whether base is set, whether discriminator is found
>>> in base, whether the values specified are written correctly, and
>>> whether all enum values are covered, when discriminator is a
>
>> 
>> And every member of the discriminator enum type must also occur as key
>> of the union's member 'data'.  Why?
>> 
>> Consider:
>> 
>>     { 'enum': 'FooEnum', 'data': [ 'plain', 'bells', 'whistles' ] }
>> 
>>     { 'type': 'CommonFooOptions',
>>       'data': { 'type: 'FooType', 'readonly': 'bool' } }
>>     { 'union': 'FooOptions',
>>       'base': 'CommonFooOptions',
>>       'discriminator': 'type',
>>       'data': { 'bells': 'BellsOptions',
>>                 'whistles': 'WhistlesOptions' } }
>> 
>> Type 'plain' doesn't have options beyond CommonFooOptions.
>
> I'd still rather make it explicit that we KNOW that this branch of the
> union has no additional options:
>
> { 'union': 'FooOptions',
>   'base': 'CommonFooOptions',
>   'discriminator': 'type',
>   'data': { 'plain': {},
>             'bells': 'BellsOptions',
>             'whistles': 'WhistlesOptions' } }
>
> to show that we explicitly thought about all the cases.  We don't
> currently have any such unions with an empty branch, but it would be
> worth documenting in the qapi text and explicitly testing that it works
> if we intend to support this.

Fair point.  However, it requires 'plain': {} to work, and it doesn't in
my testing.  When I add "'c': {}" to qapi-schema-test.json's
UserDefFlatUnion like this:

    { 'union': 'UserDefFlatUnion',
      'base': 'UserDefOne',
      'discriminator': 'string',
      'data': { 'a' : 'UserDefA', 'b' : 'UserDefB', 'c': {} } }

the generator gives me

    struct UserDefFlatUnion
    {
        UserDefFlatUnionKind kind;
        union {
            void *data;
            UserDefA * a;
            UserDefB * b;
--->        void c;
        };
        bool has_enum1;
        EnumOne enum1;
    };

which doesn't compile.  This is only the first compile error, there are
more.

We should extend the generator to permit {} before we insist on unions
covering all discriminator values explicitly.  Because if we don't,
people will be compelled to add dummy fields.