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

[Markus Armbruster]

Wenchao Xia <address@hidden> writes:

> 于 2014/2/14 17:23, Markus Armbruster 写道:
>> Wenchao Xia <address@hidden> writes:
>>
>>> 于 2014/2/13 23:14, Markus Armbruster 写道:
>>>> Wenchao Xia <address@hidden> 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 <address@hidden>
>>>>> Reviewed-by: Eric Blake <address@hidden>
>>>>> ---
>>>>>    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.
>>
>> The distribution of work between the qapi*py isn't spelled out anywhere,
>> but my working hypothesis is qapi.py is the frontend, and the
>> qapi-{commands,types,visit}.py are backends.
>>
>> The frontend's job is lexical, syntax and semantic analysis.
>>
>> The backends' job is source code generation.
>>
>> This isn't the only possible split, but it's the orthodox way to split
>> compilers.
>>
>>>    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.
>>
>> How much code would be duplicated?
>>
>   Not many now, my concern is it may becomes more complex
> when more check introduced in future.
>   However, your distribution of qapi*.py as complier make
> sense, so I am OK to respin this series.

I'll gladly review your respin.  If you need help rebasing, don't
hesitate to ask me.

>   Luiz, could you apply or push Markus's series, so I
> can pull it as my working base?

I pushed my series trivially rebased to current master to
git://repo.or.cz/qemu/armbru.git branch qapi-scripts.  It applies fine
to Luiz's qmp-unstable branch.