Re: [Qemu-devel] [PATCH V7 04/11] qapi script: check correctness of discriminator values in union

[Markus Armbruster]

Wenchao Xia <address@hidden> writes:

> It will check whether base is set, whether discriminator is found
> in base, whether the values specified are written correctly, and
> whether all enum values are covered, when discriminator is a

Please delete the comma in case you respin anyway.

> pre-defined enum type. Exprs now have addtional info inside qapi.py
> to form better error message.

This sounds like you're improving error messages.  Not true; you're only
adding them.  The improvement is compared to v6.  I think you could drop
this sentence if you respin anyway.

Here's what we need to check:

    If the object has no member 'union', it's not a union, and checking
    it is somebody else's problem.

    Else:

        Member 'data' is an object.  The members are discriminator
        values, and their valus are types.

        If the object has a member 'base', its value must name a complex
        type.  Call it "the base type".

        If the union object has no member 'discriminator', it's an
        ordinary union.

        Else if the value of member 'discriminator' is {}, it's an
        anonymous union.

        Else, it's a flat union.  The object must have a member 'base'.
        The value of member 'discriminator' must name a member of the
        base type.  If this named member's value names an enum type,
        then all members of 'data' must also be members of the enum
        type.

I'd add test cases for all these errors first, and then fix qapi.py to
properly diagnose them.

The current qapi.py is lousy at diagnosing semantic errors :(

> Signed-off-by: Wenchao Xia <address@hidden>
> ---
>  scripts/qapi.py |   84 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 files changed, 82 insertions(+), 2 deletions(-)
>
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index c504eb4..8af8cfd 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -50,6 +50,15 @@ class QAPISchemaError(Exception):
>      def __str__(self):
>          return "%s:%s:%s: %s" % (self.fp.name, self.line, self.col, self.msg)
>  
> +class QAPIExprError(Exception):
> +    def __init__(self, expr_elem, msg):
> +        self.fp = expr_elem['fp']
> +        self.line = expr_elem['line']
> +        self.msg = msg

Entirely separate exception type.  I would've tried to reuse
QAPISchemaError.  Matter of taste; no need to change anything.

No column information.  Can be added later if we care for it.

> +
> +    def __str__(self):
> +        return "%s:%s: %s" % (self.fp.name, self.line, self.msg)
> +
>  class QAPISchema:
>  
>      def __init__(self, fp):
> @@ -64,7 +73,11 @@ class QAPISchema:
>          self.accept()
>  
>          while self.tok != None:
> -            self.exprs.append(self.get_expr(False))
> +            line = self.line
> +            expr_elem = {'expr': self.get_expr(False),
> +                         'fp': fp,
> +                         'line': line}
> +            self.exprs.append(expr_elem)

Here, you save the location of top-level expressions.  More on that
below.

>  
>      def accept(self):
>          while True:
> @@ -162,6 +175,66 @@ class QAPISchema:
>              raise QAPISchemaError(self, 'Expected "{", "[" or string')
>          return expr
>  
> +# 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')

Why not base_struct_define['data'] ?

> +
> +# Return the discriminator enum define, if discriminator is specified in
> +# @expr_elem["expr"] and it is a pre-defined enum type

Actually, this function checks the discriminator against the base.  Its
caller checks the flat union case

> +def discriminator_find_enum_define(expr_elem):
> +    expr = expr_elem['expr']
> +    discriminator = expr.get('discriminator')
> +    base = expr.get('base')

Why not expr['base'] ?

> +
> +    # Only support discriminator when base present

"Support"?  Do you mean "recognize"?

> +    if not (discriminator and base):
> +        return None
> +
> +    base_fields = find_base_fields(base)
> +
> +    if not base_fields:
> +        raise QAPIExprError(expr_elem,
> +                            "Base '%s' is not a valid type"
> +                            % base)

The test case for this error is in 11/11.  If you need to respin anyway,
consider adding error tests in the same patch as the error they test.

The error is reported with the union's location instead of its base's
location.  Best you can do as long as you save positions only for
top-level expressions.  Can be improved later if we care.

> +
> +    discriminator_type = base_fields.get(discriminator)
> +
> +    if not discriminator_type:
> +        raise QAPIExprError(expr_elem,
> +                            "Discriminator '%s' not found in schema"
> +                            % discriminator)

Suggest something like "Discriminator '%s' is not a member of base type '%s'"

> +
> +    return find_enum(discriminator_type)
> +
> +def check_union(expr_elem):
> +    # If discriminator is specified and it is a pre-defined enum in schema,
> +    # check its correctness
> +    enum_define = discriminator_find_enum_define(expr_elem)
> +    name = expr_elem['expr']['union']
> +    members = expr_elem['expr']['data']
> +    if enum_define:

If the discriminator is of enum type:

> +        for key in members:
> +            if not key in enum_define['enum_values']:
> +                raise QAPIExprError(expr_elem,
> +                                    "Discriminator value '%s' is not found 
> in "
> +                                    "enum '%s'" %
> +                                    (key, enum_define["enum_name"]))

Then all keys of the union's member 'data' need to be members of the
discriminator enum type.  Good.

> +        for key in enum_define['enum_values']:
> +            if not key in members:
> +                raise QAPIExprError(expr_elem,
> +                                    "Enum value '%s' is not covered by a "
> +                                    "branch of union '%s'" %
> +                                    (key, name))

And every member of the discriminator enum type must also occur as key
of the union's member 'data'.  Why?

Consider:

    { 'enum': 'FooEnum', 'data': [ 'plain', 'bells', 'whistles' ] }

    { 'type': 'CommonFooOptions',
      'data': { 'type: 'FooType', 'readonly': 'bool' } }
    { 'union': 'FooOptions',
      'base': 'CommonFooOptions',
      'discriminator': 'type',
      'data': { 'bells': 'BellsOptions',
                'whistles': 'WhistlesOptions' } }

Type 'plain' doesn't have options beyond CommonFooOptions.

> +
> +def check_exprs(schema):
> +    for expr_elem in schema.exprs:
> +        expr = expr_elem['expr']
> +        if expr.has_key('union'):
> +            check_union(expr_elem)
> +
>  def parse_schema(fp):
>      try:
>          schema = QAPISchema(fp)
> @@ -171,7 +244,8 @@ def parse_schema(fp):
>  
>      exprs = []
>  
> -    for expr in schema.exprs:
> +    for expr_elem in schema.exprs:
> +        expr = expr_elem['expr']
>          if expr.has_key('enum'):
>              add_enum(expr['enum'], expr['data'])
>          elif expr.has_key('union'):
> @@ -181,6 +255,12 @@ def parse_schema(fp):
>              add_struct(expr)
>          exprs.append(expr)
>  
> +    try:
> +        check_exprs(schema)
> +    except QAPIExprError, e:
> +        print >>sys.stderr, e
> +        exit(1)
> +
>      return exprs
>  
>  def parse_args(typeinfo):

I don't think your semantic checks are complete (compare against my
"Here's what we need to check"), but it's an improvement.  We can do
"complete" on top.