Re: [Qemu-devel] [PATCH V8 04/10] qapi script: check correctness of union

[Wenchao Xia <wenchaoqemu@gmail.com>]

于 2014/3/4 20:47, Markus Armbruster 写道:
> Wenchao Xia <wenchaoqemu@gmail.com> writes:
>
>> From: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> Commit message lost detail since v7.  Intentional?
>
I thought you don't want the message in V7, maybe I misunderstood it.


>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>> Signed-off-by: Wenchao Xia <wenchaoqemu@gmail.com>
>> ---
>>  scripts/qapi.py                                    |  106 +++++++++++++++++++-
>>  tests/Makefile                                     |    4 +-
>>  .../qapi-schema/flat-union-invalid-branch-key.err  |    1 +
>>  .../qapi-schema/flat-union-invalid-branch-key.exit |    1 +
>>  .../qapi-schema/flat-union-invalid-branch-key.json |   17 +++
>>  .../flat-union-invalid-discriminator.err           |    1 +
>>  .../flat-union-invalid-discriminator.exit          |    1 +
>>  .../flat-union-invalid-discriminator.json          |   17 +++
>>  tests/qapi-schema/flat-union-no-base.err           |    1 +
>>  tests/qapi-schema/flat-union-no-base.exit          |    1 +
>>  tests/qapi-schema/flat-union-no-base.json          |   16 +++
>>  tests/qapi-schema/union-invalid-base.err           |    1 +
>>  tests/qapi-schema/union-invalid-base.exit          |    1 +
>>  tests/qapi-schema/union-invalid-base.json          |   10 ++
>>  14 files changed, 175 insertions(+), 3 deletions(-)
>>  create mode 100644 tests/qapi-schema/flat-union-invalid-branch-key.err
>>  create mode 100644 tests/qapi-schema/flat-union-invalid-branch-key.exit
>>  create mode 100644 tests/qapi-schema/flat-union-invalid-branch-key.json
>>  create mode 100644 tests/qapi-schema/flat-union-invalid-branch-key.out
>>  create mode 100644 tests/qapi-schema/flat-union-invalid-discriminator.err
>>  create mode 100644 tests/qapi-schema/flat-union-invalid-discriminator.exit
>>  create mode 100644 tests/qapi-schema/flat-union-invalid-discriminator.json
>>  create mode 100644 tests/qapi-schema/flat-union-invalid-discriminator.out
>>  create mode 100644 tests/qapi-schema/flat-union-no-base.err
>>  create mode 100644 tests/qapi-schema/flat-union-no-base.exit
>>  create mode 100644 tests/qapi-schema/flat-union-no-base.json
>>  create mode 100644 tests/qapi-schema/flat-union-no-base.out
>>  create mode 100644 tests/qapi-schema/union-invalid-base.err
>>  create mode 100644 tests/qapi-schema/union-invalid-base.exit
>>  create mode 100644 tests/qapi-schema/union-invalid-base.json
>>  create mode 100644 tests/qapi-schema/union-invalid-base.out
>>
>> diff --git a/scripts/qapi.py b/scripts/qapi.py
>> index 1954292..cea346f 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_info, msg):
>> +        self.fp = expr_info['fp']
>> +        self.line = expr_info['line']
>> +        self.msg = msg
>> +
>> +    def __str__(self):
>> +        return "%s:%s: %s" % (self.fp.name, self.line, self.msg)
>> +
>>  class QAPISchema:
>>  
>>      def __init__(self, fp):
>> @@ -64,7 +73,10 @@ class QAPISchema:
>>          self.accept()
>>  
>>          while self.tok != None:
>> -            self.exprs.append(self.get_expr(False))
>> +            expr_info = {'fp': fp, 'line': self.line}
>> +            expr_elem = {'expr': self.get_expr(False),
>> +                         'info': expr_info}
>> +            self.exprs.append(expr_elem)
>>  
>>      def accept(self):
>>          while True:
>> @@ -162,6 +174,89 @@ class QAPISchema:
>>              raise QAPISchemaError(self, 'Expected "{", "[" or string')
>>          return expr
>>  
>> +def find_base_fields(base):
>> +    base_struct_define = find_struct(base)
>> +    if not base_struct_define:
>> +        return None
>> +    return base_struct_define['data']
>> +
>> +# Return the discriminator enum define if discrminator is specified as an
>> +# enum type, otherwise return None.
>> +def discriminator_find_enum_define(expr):
>> +    base = expr.get('base')
>> +    discriminator = expr.get('discriminator')
> Why not expr['base'] and expr['discriminator']?
>
> More of the same below.  No need to respin just for that; we can clean
> it up on top.
>
It is possible 'base' not present in some caller, so use .get to
avoid python error.

>> +
>> +    if not (discriminator and base):
>> +        return None
>> +
>> +    base_fields = find_base_fields(base)
>> +    if not base_fields:
>> +        return None
>> +
>> +    discriminator_type = base_fields.get(discriminator)
>> +    if not discriminator_type:
>> +        return None
>> +
>> +    return find_enum(discriminator_type)
>> +
>> +def check_union(expr, expr_info):
>> +    name = expr['union']
>> +    base = expr.get('base')
>> +    discriminator = expr.get('discriminator')
>> +    members = expr['data']
>> +
>> +    # If the object has a member 'base', its value must name a complex type.
>> +    if base:
>> +        base_fields = find_base_fields(base)
>> +        if not base_fields:
>> +            raise QAPIExprError(expr_info,
>> +                                "Base '%s' is not a valid type"
>> +                                % base)
>> +
>> +    # If the union object has no member 'discriminator', it's an
>> +    # ordinary union.
>> +    if not discriminator:
>> +        enum_define = None
>> +
>> +    # Else if the value of member 'discriminator' is {}, it's an
>> +    # anonymous union.
>> +    elif discriminator == {}:
>> +        enum_define = None
>> +
>> +    # Else, it's a flat union.
>> +    else:
>> +        # The object must have a member 'base'.
>> +        if not base:
>> +            raise QAPIExprError(expr_info,
>> +                                "Flat union '%s' must have a base field"
>> +                                % name)
>> +        # The value of member 'discriminator' must name a member of the
>> +        # base type.
>> +        if not base_fields.get(discriminator):
>> +            raise QAPIExprError(expr_info,
>> +                                "Discriminator '%s' is not a member of base "
>> +                                "type '%s'"
>> +                                % (discriminator, base))
>> +        enum_define = discriminator_find_enum_define(expr)
> Could this be simplified?  Like this:
>
>            # The value of member 'discriminator' must name a member of the
>            # base type.
>            discriminator_type = base_fields.get(discriminator):
>            if not discriminator_type
>                raise QAPIExprError(expr_info,
>                                    "Discriminator '%s' is not a member of base "
>                                    "type '%s'"
>                                    % (discriminator, base))
>            enum_define = find_enum(discriminator_type)
>
> It's the only use of discriminator_find_enum_define()...
>
> Hmm, later patches add more uses, so my simplification is probably not
> worthwhile.  Anyway, we can simplify on top.
>
>> +
>> +    # Check every branch
>> +    for (key, value) in members.items():
>> +        # If this named member's value names an enum type, then all members
>> +        # of 'data' must also be members of the enum type.
>> +        if enum_define and not key in enum_define['enum_values']:
>> +            raise QAPIExprError(expr_info,
>> +                                "Discriminator value '%s' is not found in "
>> +                                "enum '%s'" %
>> +                                (key, enum_define["enum_name"]))
>> +        # Todo: put more check such as whether each value is valid, but it may
>> +        # need more functions to handle array case, so leave it now.
> I'm not sure I get your comment.
>
Above only checks key, and I found it is possible to check every
branch's value,
so leaves a comments here.

>> +
>> +def check_exprs(schema):
>> +    for expr_elem in schema.exprs:
>> +        expr = expr_elem['expr']
>> +        if expr.has_key('union'):
>> +            check_union(expr, expr_elem['info'])
>> +
>>  def parse_schema(fp):
>>      try:
>>          schema = QAPISchema(fp)
>> @@ -171,7 +266,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 +277,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):
>> diff --git a/tests/Makefile b/tests/Makefile
>> index dfe06eb..6ac9889 100644
>> --- a/tests/Makefile
>> +++ b/tests/Makefile
>> @@ -143,7 +143,9 @@ check-qapi-schema-y := $(addprefix tests/qapi-schema/, \
>>          qapi-schema-test.json quoted-structural-chars.json \
>>          trailing-comma-list.json trailing-comma-object.json \
>>          unclosed-list.json unclosed-object.json unclosed-string.json \
>> -        duplicate-key.json)
>> +        duplicate-key.json union-invalid-base.json flat-union-no-base.json \
>> +        flat-union-invalid-discriminator.json \
>> +        flat-union-invalid-branch-key.json)
>>  
>>  GENERATED_HEADERS += tests/test-qapi-types.h tests/test-qapi-visit.h tests/test-qmp-commands.h
>>  
>> diff --git a/tests/qapi-schema/flat-union-invalid-branch-key.err b/tests/qapi-schema/flat-union-invalid-branch-key.err
>> new file mode 100644
>> index 0000000..1125caf
>> --- /dev/null
>> +++ b/tests/qapi-schema/flat-union-invalid-branch-key.err
>> @@ -0,0 +1 @@
>> +<stdin>:13: Discriminator value 'value_wrong' is not found in enum 'TestEnum'
>> diff --git a/tests/qapi-schema/flat-union-invalid-branch-key.exit b/tests/qapi-schema/flat-union-invalid-branch-key.exit
>> new file mode 100644
>> index 0000000..d00491f
>> --- /dev/null
>> +++ b/tests/qapi-schema/flat-union-invalid-branch-key.exit
>> @@ -0,0 +1 @@
>> +1
>> diff --git a/tests/qapi-schema/flat-union-invalid-branch-key.json b/tests/qapi-schema/flat-union-invalid-branch-key.json
>> new file mode 100644
>> index 0000000..a624282
>> --- /dev/null
>> +++ b/tests/qapi-schema/flat-union-invalid-branch-key.json
>> @@ -0,0 +1,17 @@
>> +{ 'enum': 'TestEnum',
>> +  'data': [ 'value1', 'value2' ] }
>> +
>> +{ 'type': 'TestBase',
>> +  'data': { 'enum1': 'TestEnum' } }
>> +
>> +{ 'type': 'TestTypeA',
>> +  'data': { 'string': 'str' } }
>> +
>> +{ 'type': 'TestTypeB',
>> +  'data': { 'integer': 'int' } }
>> +
>> +{ 'union': 'TestUnion',
>> +  'base': 'TestBase',
>> +  'discriminator': 'enum1',
>> +  'data': { 'value_wrong': 'TestTypeA',
>> +            'value2': 'TestTypeB' } }
>> diff --git a/tests/qapi-schema/flat-union-invalid-branch-key.out b/tests/qapi-schema/flat-union-invalid-branch-key.out
>> new file mode 100644
>> index 0000000..e69de29
>> diff --git a/tests/qapi-schema/flat-union-invalid-discriminator.err b/tests/qapi-schema/flat-union-invalid-discriminator.err
>> new file mode 100644
>> index 0000000..cad9dbf
>> --- /dev/null
>> +++ b/tests/qapi-schema/flat-union-invalid-discriminator.err
>> @@ -0,0 +1 @@
>> +<stdin>:13: Discriminator 'enum_wrong' is not a member of base type 'TestBase'
>> diff --git a/tests/qapi-schema/flat-union-invalid-discriminator.exit b/tests/qapi-schema/flat-union-invalid-discriminator.exit
>> new file mode 100644
>> index 0000000..d00491f
>> --- /dev/null
>> +++ b/tests/qapi-schema/flat-union-invalid-discriminator.exit
>> @@ -0,0 +1 @@
>> +1
>> diff --git a/tests/qapi-schema/flat-union-invalid-discriminator.json b/tests/qapi-schema/flat-union-invalid-discriminator.json
>> new file mode 100644
>> index 0000000..887157e
>> --- /dev/null
>> +++ b/tests/qapi-schema/flat-union-invalid-discriminator.json
>> @@ -0,0 +1,17 @@
>> +{ 'enum': 'TestEnum',
>> +  'data': [ 'value1', 'value2' ] }
>> +
>> +{ 'type': 'TestBase',
>> +  'data': { 'enum1': 'TestEnum' } }
>> +
>> +{ 'type': 'TestTypeA',
>> +  'data': { 'string': 'str' } }
>> +
>> +{ 'type': 'TestTypeB',
>> +  'data': { 'integer': 'int' } }
>> +
>> +{ 'union': 'TestUnion',
>> +  'base': 'TestBase',
>> +  'discriminator': 'enum_wrong',
>> +  'data': { 'value1': 'TestTypeA',
>> +            'value2': 'TestTypeB' } }
>> diff --git a/tests/qapi-schema/flat-union-invalid-discriminator.out b/tests/qapi-schema/flat-union-invalid-discriminator.out
>> new file mode 100644
>> index 0000000..e69de29
>> diff --git a/tests/qapi-schema/flat-union-no-base.err b/tests/qapi-schema/flat-union-no-base.err
>> new file mode 100644
>> index 0000000..e695315
>> --- /dev/null
>> +++ b/tests/qapi-schema/flat-union-no-base.err
>> @@ -0,0 +1 @@
>> +<stdin>:13: Flat union 'TestUnion' must have a base field
>> diff --git a/tests/qapi-schema/flat-union-no-base.exit b/tests/qapi-schema/flat-union-no-base.exit
>> new file mode 100644
>> index 0000000..d00491f
>> --- /dev/null
>> +++ b/tests/qapi-schema/flat-union-no-base.exit
>> @@ -0,0 +1 @@
>> +1
>> diff --git a/tests/qapi-schema/flat-union-no-base.json b/tests/qapi-schema/flat-union-no-base.json
>> new file mode 100644
>> index 0000000..e0900d4
>> --- /dev/null
>> +++ b/tests/qapi-schema/flat-union-no-base.json
>> @@ -0,0 +1,16 @@
>> +{ 'enum': 'TestEnum',
>> +  'data': [ 'value1', 'value2' ] }
>> +
>> +{ 'type': 'TestBase',
>> +  'data': { 'enum1': 'TestEnum' } }
>> +
> TestEnum and TestBase aren't used.
>
will remove.

>> +{ 'type': 'TestTypeA',
>> +  'data': { 'string': 'str' } }
>> +
>> +{ 'type': 'TestTypeB',
>> +  'data': { 'integer': 'int' } }
>> +
>> +{ 'union': 'TestUnion',
>> +  'discriminator': 'enum1',
>> +  'data': { 'value1': 'TestTypeA',
>> +            'value2': 'TestTypeB' } }
>> diff --git a/tests/qapi-schema/flat-union-no-base.out b/tests/qapi-schema/flat-union-no-base.out
>> new file mode 100644
>> index 0000000..e69de29
>> diff --git a/tests/qapi-schema/union-invalid-base.err b/tests/qapi-schema/union-invalid-base.err
>> new file mode 100644
>> index 0000000..dd8e3d1
>> --- /dev/null
>> +++ b/tests/qapi-schema/union-invalid-base.err
>> @@ -0,0 +1 @@
>> +<stdin>:7: Base 'TestBaseWrong' is not a valid type
>> diff --git a/tests/qapi-schema/union-invalid-base.exit b/tests/qapi-schema/union-invalid-base.exit
>> new file mode 100644
>> index 0000000..d00491f
>> --- /dev/null
>> +++ b/tests/qapi-schema/union-invalid-base.exit
>> @@ -0,0 +1 @@
>> +1
>> diff --git a/tests/qapi-schema/union-invalid-base.json b/tests/qapi-schema/union-invalid-base.json
>> new file mode 100644
>> index 0000000..1fa4930
>> --- /dev/null
>> +++ b/tests/qapi-schema/union-invalid-base.json
>> @@ -0,0 +1,10 @@
>> +{ 'type': 'TestTypeA',
>> +  'data': { 'string': 'str' } }
>> +
>> +{ 'type': 'TestTypeB',
>> +  'data': { 'integer': 'int' } }
>> +
>> +{ 'union': 'TestUnion',
>> +  'base': 'TestBaseWrong',
>> +  'data': { 'value1': 'TestTypeA',
>> +            'value2': 'TestTypeB' } }
>> diff --git a/tests/qapi-schema/union-invalid-base.out b/tests/qapi-schema/union-invalid-base.out
>> new file mode 100644
>> index 0000000..e69de29
> Tests cover all the new errors.  Good.