Re: [Qemu-devel] [PATCH V7 07/11] qapi script: support pre-defined enum type as discriminator in union

[Wenchao Xia <xiawenc@linux.vnet.ibm.com>]

于 2014/2/21 0:38, Markus Armbruster 写道:
> Wenchao Xia <xiawenc@linux.vnet.ibm.com> writes:
> 
>> By default, any union will automatically generate a enum type as
>> "[UnionName]Kind" in C code, and it is duplicated when the discriminator
>> is specified as a pre-defined enum type in schema. After this patch,
>> the pre-defined enum type will be really used as the switch case
>> condition in generated C code, if discriminator is an enum field.
>>
>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>> ---
>>   docs/qapi-code-gen.txt |    8 ++++++--
>>   scripts/qapi-types.py  |   20 ++++++++++++++++----
>>   scripts/qapi-visit.py  |   27 ++++++++++++++++++++-------
>>   scripts/qapi.py        |   13 ++++++++++++-
>>   4 files changed, 54 insertions(+), 14 deletions(-)
>>
>> diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt
>> index 0728f36..a2e7921 100644
>> --- a/docs/qapi-code-gen.txt
>> +++ b/docs/qapi-code-gen.txt
>> @@ -123,11 +123,15 @@ And it looks like this on the wire:
>>   
>>   Flat union types avoid the nesting on the wire. They are used whenever a
>>   specific field of the base type is declared as the discriminator ('type' is
>> -then no longer generated). The discriminator must always be a string field.
>> +then no longer generated). The discriminator can be a string field or a
>> +predefined enum field. If it is a string field, a hidden enum type will be
>> +generated as "[UNION_NAME]Kind". If it is an enum field, a compile time check
>> +will be done to verify the correctness. It is recommended to use an enum field.
>>   The above example can then be modified as follows:
>>   
>> + { 'enum': 'BlockdevDriver', 'data': [ 'raw', 'qcow2' ] }
>>    { 'type': 'BlockdevCommonOptions',
>> -   'data': { 'driver': 'str', 'readonly': 'bool' } }
>> +   'data': { 'driver': 'BlockdevDriver', 'readonly': 'bool' } }
>>    { 'union': 'BlockdevOptions',
>>      'base': 'BlockdevCommonOptions',
>>      'discriminator': 'driver',
>> diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
>> index 656a9a0..4098c60 100644
>> --- a/scripts/qapi-types.py
>> +++ b/scripts/qapi-types.py
>> @@ -201,14 +201,22 @@ def generate_union(expr):
>>       base = expr.get('base')
>>       discriminator = expr.get('discriminator')
>>   
>> +    expr_elem = {'expr': expr}
>> +    enum_define = discriminator_find_enum_define(expr_elem)
> 
> expr_elem has no fp, line.  What if discriminator_find_enum_define
> throws a QAPIExprError?
> 
  It shouldn't happen, since all error check happens in parse_schema().

> More of the same below.
> 
>> +    if enum_define:
>> +        discriminator_type_name = enum_define['enum_name']
>> +    else:
>> +        discriminator_type_name = '%sKind' % (name)
>> +
>>       ret = mcgen('''
>>   struct %(name)s
>>   {
>> -    %(name)sKind kind;
>> +    %(discriminator_type_name)s kind;
>>       union {
>>           void *data;
>>   ''',
>> -                name=name)
>> +                name=name,
>> +                discriminator_type_name=discriminator_type_name)
>>   
>>       for key in typeinfo:
>>           ret += mcgen('''
>> @@ -389,8 +397,12 @@ for expr in exprs:
>>           fdef.write(generate_enum_lookup(expr['enum'], expr['data']))
>>       elif expr.has_key('union'):
>>           ret += generate_fwd_struct(expr['union'], expr['data']) + "\n"
>> -        ret += generate_enum('%sKind' % expr['union'], expr['data'].keys())
>> -        fdef.write(generate_enum_lookup('%sKind' % expr['union'], expr['data'].keys()))
>> +        expr_elem = {'expr': expr}
>> +        enum_define = discriminator_find_enum_define(expr_elem)
>> +        if not enum_define:
>> +            ret += generate_enum('%sKind' % expr['union'], expr['data'].keys())
>> +            fdef.write(generate_enum_lookup('%sKind' % expr['union'],
>> +                                            expr['data'].keys()))
> 
> Generate the implicit enum only when we don't have an explicit enum
> discriminator.  Good.
> 
>>           if expr.get('discriminator') == {}:
>>               fdef.write(generate_anon_union_qtypes(expr))
>>       else:
>> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
>> index 87e6df7..08685a7 100644
>> --- a/scripts/qapi-visit.py
>> +++ b/scripts/qapi-visit.py
>> @@ -260,10 +260,17 @@ def generate_visit_union(expr):
>>           assert not base
>>           return generate_visit_anon_union(name, members)
>>   
>> -    # There will always be a discriminator in the C switch code, by default it
>> -    # is an enum type generated silently as "'%sKind' % (name)"
>> -    ret = generate_visit_enum('%sKind' % name, members.keys())
>> -    discriminator_type_name = '%sKind' % (name)
>> +    expr_elem = {'expr': expr}
>> +    enum_define = discriminator_find_enum_define(expr_elem)
>> +    if enum_define:
>> +        # Use the predefined enum type as discriminator
>> +        ret = ""
>> +        discriminator_type_name = enum_define['enum_name']
>> +    else:
>> +        # There will always be a discriminator in the C switch code, by default it
>> +        # is an enum type generated silently as "'%sKind' % (name)"
>> +        ret = generate_visit_enum('%sKind' % name, members.keys())
>> +        discriminator_type_name = '%sKind' % (name)
> 
> Generate the visit of the discriminator only when we don't have an
> explicit enum discriminator (which gets visited elsewhere already).
> Good.
> 
>>   
>>       if base:
>>           base_fields = find_struct(base)['data']
>> @@ -303,11 +310,12 @@ void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error **
>>       else:
>>           desc_type = discriminator
>>       ret += mcgen('''
>> -        visit_type_%(name)sKind(m, &(*obj)->kind, "%(type)s", &err);
>> +        visit_type_%(discriminator_type_name)s(m, &(*obj)->kind, "%(type)s", &err);
> 
> Another long line due to very long identifier.
> 
  Will improve.

>>           if (!err) {
>>               switch ((*obj)->kind) {
>>   ''',
>> -                 name=name, type=desc_type)
>> +                 discriminator_type_name=discriminator_type_name,
>> +                 type=desc_type)
>>   
>>       for key in members:
>>           if not discriminator:
>> @@ -519,7 +527,12 @@ for expr in exprs:
>>           ret += generate_visit_list(expr['union'], expr['data'])
>>           fdef.write(ret)
>>   
>> -        ret = generate_decl_enum('%sKind' % expr['union'], expr['data'].keys())
>> +        expr_elem = {'expr': expr}
>> +        enum_define = discriminator_find_enum_define(expr_elem)
>> +        ret = ""
>> +        if not enum_define:
>> +            ret = generate_decl_enum('%sKind' % expr['union'],
>> +                                     expr['data'].keys())
> 
> Generate the visitor for the implicit enum only when we don't have an
> explicit enum discriminator (which has its own visitor already).  Good.
> 
>>           ret += generate_declaration(expr['union'], expr['data'])
>>           fdecl.write(ret)
>>       elif expr.has_key('enum'):
>> diff --git a/scripts/qapi.py b/scripts/qapi.py
>> index 130dced..2a5eb59 100644
>> --- a/scripts/qapi.py
>> +++ b/scripts/qapi.py
>> @@ -250,11 +250,22 @@ def parse_schema(fp):
>>               add_enum(expr['enum'], expr['data'])
>>           elif expr.has_key('union'):
>>               add_union(expr)
>> -            add_enum('%sKind' % expr['union'])
>>           elif expr.has_key('type'):
>>               add_struct(expr)
>>           exprs.append(expr)
>>   
>> +    # Try again for hidden UnionKind enum
>> +    for expr_elem in schema.exprs:
>> +        expr = expr_elem['expr']
>> +        if expr.has_key('union'):
>> +            try:
>> +                enum_define = discriminator_find_enum_define(expr_elem)
>> +            except QAPIExprError, e:
>> +                print >>sys.stderr, e
>> +                exit(1)
>> +            if not enum_define:
>> +                add_enum('%sKind' % expr['union'])
>> +
>>       try:
>>           check_exprs(schema)
>>       except QAPIExprError, e:
> 
> I guess you move this into its own loop because when base types are used
> before they're defined, or an enum type is used for a discriminator
> before it's defined, then discriminator_find_enum_define() complains.
> Correct?
> 
  Exactly, which allow enum define after usage in schema.