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

[Markus Armbruster <armbru@redhat.com>]

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?

>>> +        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.
>>