Re: [Qemu-devel] [PATCH V6 03/10] qapi script: check correctness of discriminator values in union

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

于 2014/2/13 23:14, Markus Armbruster 写道:
> Wenchao Xia <xiawenc@linux.vnet.ibm.com> writes:
> 
>> It will check whether the values specified are written correctly,
>> and whether all enum values are covered, when discriminator is a
>> pre-defined enum type
>>
>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>> ---
>>   scripts/qapi-visit.py |   17 +++++++++++++++++
>>   scripts/qapi.py       |   31 +++++++++++++++++++++++++++++++
>>   2 files changed, 48 insertions(+), 0 deletions(-)
>>
>> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
>> index 65f1a54..c0efb5f 100644
>> --- a/scripts/qapi-visit.py
>> +++ b/scripts/qapi-visit.py
>> @@ -255,6 +255,23 @@ def generate_visit_union(expr):
>>           assert not base
>>           return generate_visit_anon_union(name, members)
>>   
>> +    # If discriminator is specified and it is a pre-defined enum in schema,
>> +    # check its correctness
>> +    enum_define = discriminator_find_enum_define(expr)
>> +    if enum_define:
>> +        for key in members:
>> +            if not key in enum_define["enum_values"]:
>> +                sys.stderr.write("Discriminator value '%s' is not found in "
>> +                                 "enum '%s'\n" %
>> +                                 (key, enum_define["enum_name"]))
>> +                sys.exit(1)
> 
> Can this happen?  If yes, why isn't it diagnosed in qapi.py, like all
> the other semantic errors?
> 
  I think the parse procedure contains two part:
1 read qapi-schema.json and parse it into exprs.
2 translate exprs into final output.
  Looking at qapi.py, qapi-visit.py, qapi-types.py, it seems qapi.py is
in charge of step 1 handling literal error, and other two script are in
charge of step 2. The above error can be only detected in step 2 after
all enum defines are remembered in step 1, so I didn't add those things
into qapi.py.

  I guess you want to place the check inside parse_schema() to let
test case detect it easier, one way to go is, let qapi.py do checks
for step 2:

def parse_schema(fp):
    try:
        schema = QAPISchema(fp)
    except QAPISchemaError, e:
        print >>sys.stderr, e
        exit(1)

    exprs = []

    for expr in schema.exprs:
        if expr.has_key('enum'):
            add_enum(expr['enum'])
        elif expr.has_key('union'):
            add_union(expr)
            add_enum('%sKind' % expr['union'])
        elif expr.has_key('type'):
            add_struct(expr)
        exprs.append(expr)

+    for expr in schema.exprs:
+        if expr.has_key('union'):
+            #check code

    return exprs

  This way qapi.py can detect such errors. Disadvantage is that,
qapi.py is invloved for step 2 things, so some code in qapi.py
and qapi-visit.py may be dupicated, here the "if .... union...
discriminator" code may appear in both qapi.py and qapi-visit.py.

>> +        for key in enum_define["enum_values"]:
>> +            if not key in members:
>> +                sys.stderr.write("Enum value '%s' is not covered by a branch "
>> +                                 "of union '%s'\n" %
>> +                                 (key, name))
>> +                sys.exit(1)
>> +
> 
> Likewise.
> 
>>       ret = generate_visit_enum('%sKind' % name, members.keys())
>>   
>>       if base:
>> diff --git a/scripts/qapi.py b/scripts/qapi.py
>> index cf34768..0a3ab80 100644
>> --- a/scripts/qapi.py
>> +++ b/scripts/qapi.py
>> @@ -385,3 +385,34 @@ def guardend(name):
>>   
>>   ''',
>>                    name=guardname(name))
>> +

  The funtions below are likely helper funtions, I planed to put them
into qapi_helper.py, but they are not much so kepted for easy.

>> +# This function can be used to check whether "base" is valid
>> +def find_base_fields(base):
>> +    base_struct_define = find_struct(base)
>> +    if not base_struct_define:
>> +        return None
>> +    return base_struct_define.get('data')
>> +
>> +# Return the discriminator enum define, if discriminator is specified in
>> +# @expr and it is a pre-defined enum type
>> +def discriminator_find_enum_define(expr):
>> +    discriminator = expr.get('discriminator')
>> +    base = expr.get('base')
>> +
>> +    # Only support discriminator when base present
>> +    if not (discriminator and base):
>> +        return None
>> +
>> +    base_fields = find_base_fields(base)
>> +
>> +    if not base_fields:
>> +        raise StandardError("Base '%s' is not a valid type\n"
>> +                            % base)
> 
> Why not QAPISchemaError, like for other semantic errors?
> 

  I think QAPISchemaError is a literal error of step 1, here
it can't be used unless we record the text/line number belong to
each expr.

>> +
>> +    discriminator_type = base_fields.get(discriminator)
>> +
>> +    if not discriminator_type:
>> +        raise StandardError("Discriminator '%s' not found in schema\n"
>> +                            % discriminator)
> 
> Likewise.
> 
>> +
>> +    return find_enum(discriminator_type)
> 
> All errors should have a test in tests/qapi-schema/.  I can try to add
> tests for you when I rebase your 09/10.
>