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/14 17:23, Markus Armbruster 写道:
> Wenchao Xia <xiawenc@linux.vnet.ibm.com> writes:
>
>> 于 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.
>
> 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.
   Luiz, could you apply or push Markus's series, so I
can pull it as my working base?


>>>> +        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.
>
> That's fine with me.
>
>>>> +# 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.
>
> Reporting an error without a location is not nice!
>
> If decent error messages require recording locations, then we should
> record locations.
>
> A real compiler frontend records full location information, i.e. every
> node in the abstract syntax tree (or whatever else it produces) is
> decorated with a location.
>
> Unfortunately, this wasn't done in qapi.py, so we get to retrofit it
> now.
>
> Perhaps recording only locations of top-level expressions would suffice
> to improve your error messages to acceptable levels.  I'm not saying we
> should take this shortcut, just pointing out it exists.
>
> qapi.py represents locations as character offset in the contents of the
> schema file (QAPISchema.cursor), which it converts to line, column on
> demand, in QAPISchemaError.__init__.  If we keep things working that
> way, the location data to record is the offset, not line, column.
>
>>>> +
>>>> +    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.
>>>
>