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

[Luiz Capitulino <lcapitulino@redhat.com>]

On Mon, 17 Feb 2014 09:50:10 +0800
Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:

> 于 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?

Yes, but I have to handle current pull request right now. I'll let you
know when I apply it.

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