qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Markus Armbruster
Subject: Re: [Qemu-devel] [PATCH V8 04/10] qapi script: check correctness of union
Date: Tue, 04 Mar 2014 13:47:07 +0100
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/24.2 (gnu/linux)

Wenchao Xia <address@hidden> writes:

> From: Wenchao Xia <address@hidden>

Commit message lost detail since v7.  Intentional?

> Signed-off-by: Wenchao Xia <address@hidden>
> Signed-off-by: Wenchao Xia <address@hidden>
> ---
>  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.

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

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

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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]