qemu-devel
[Top][All Lists]
Advanced

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

Re: [Qemu-devel] [PATCH v4 08/19] qapi: Better error messages for bad ex


From: Markus Armbruster
Subject: Re: [Qemu-devel] [PATCH v4 08/19] qapi: Better error messages for bad expressions
Date: Tue, 23 Sep 2014 16:56:55 +0200
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux)

Eric Blake <address@hidden> writes:

> The previous commit demonstrated that the generator overlooked some
> fairly basic broken expressions:
> - missing metataype
> - metatype key has a non-string value
> - unknown key in relation to the metatype
> - conflicting metatype (this patch treats the second metatype as an
> unknown key of the first key visited, which is not necessarily the
> first key the user typed)

The whole thing is a Saturday afternoon hack, with extra hacks bolted on
left & right.

> Add check_keys to cover these situations, and update testcases to
> match.  A couple other tests (enum-missing-data, indented-expr) had
> to change since the validation added here occurs so early.
>
> While valid .json files won't trigger any of these cases, we might
> as well be nicer to developers that make a typo while trying to add
> new QAPI code.
>
> Signed-off-by: Eric Blake <address@hidden>

> ---
>  scripts/qapi.py                         | 76 
> +++++++++++++++++++++++++--------
>  tests/qapi-schema/bad-type-dict.err     |  1 +
>  tests/qapi-schema/bad-type-dict.exit    |  2 +-
>  tests/qapi-schema/bad-type-dict.json    |  2 +-
>  tests/qapi-schema/bad-type-dict.out     |  3 --
>  tests/qapi-schema/double-type.err       |  1 +
>  tests/qapi-schema/double-type.exit      |  2 +-
>  tests/qapi-schema/double-type.json      |  2 +-
>  tests/qapi-schema/double-type.out       |  3 --
>  tests/qapi-schema/enum-missing-data.err |  2 +-
>  tests/qapi-schema/indented-expr.json    |  4 +-
>  tests/qapi-schema/indented-expr.out     |  2 +-
>  tests/qapi-schema/missing-type.err      |  1 +
>  tests/qapi-schema/missing-type.exit     |  2 +-
>  tests/qapi-schema/missing-type.json     |  2 +-
>  tests/qapi-schema/missing-type.out      |  3 --
>  tests/qapi-schema/unknown-expr-key.err  |  1 +
>  tests/qapi-schema/unknown-expr-key.exit |  2 +-
>  tests/qapi-schema/unknown-expr-key.json |  2 +-
>  tests/qapi-schema/unknown-expr-key.out  |  3 --
>  20 files changed, 75 insertions(+), 41 deletions(-)
>
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index 85aa8bf..8fbc45f 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -348,7 +348,29 @@ def check_exprs(schema):
>          if expr.has_key('enum'):
>              check_enum(expr, info)
>
> +def check_keys(expr_elem, meta, required, optional=[]):
> +    expr = expr_elem['expr']
> +    info = expr_elem['info']
> +    name = expr[meta]

Caller ensures expr[meta] exists.  Okay.

> +    if not isinstance(name, basestring):

I'm a Python noob: why basestring and not str?

> +        raise QAPIExprError(info,
> +                            "%s key must have a string value" % meta)
> +    expr_elem['name'] = name

Where is this used?

> +    required.append(meta)

Ugly side effect.  To avoid, either make a new list here

    required = required + [ meta ]

or do nothing here and...

> +    for (key, value) in expr.items():
> +        if not key in required and not key in optional:

... add "and key != meta" to this condition.

> +            raise QAPIExprError(info,
> +                                "%s '%s' has unknown key '%s'"
> +                                % (meta, name, key))
> +    for key in required:
> +        if not expr.has_key(key):
> +            raise QAPIExprError(info,
> +                                "%s '%s' is missing key '%s'"
> +                                % (meta, name, key))
> +
> +
>  def parse_schema(input_file):
> +    # First pass: read entire file into memory
>      try:
>          schema = QAPISchema(open(input_file, "r"))
>      except (QAPISchemaError, QAPIExprError), e:
> @@ -357,24 +379,44 @@ def parse_schema(input_file):
>
>      exprs = []
>
> -    for expr_elem in schema.exprs:
> -        expr = expr_elem['expr']
> -        if expr.has_key('enum'):
> -            add_enum(expr['enum'], expr.get('data'))
> -        elif expr.has_key('union'):
> -            add_union(expr)
> -        elif expr.has_key('type'):
> -            add_struct(expr)
> -        exprs.append(expr)
> -
> -    # Try again for hidden UnionKind enum
> -    for expr_elem in schema.exprs:
> -        expr = expr_elem['expr']
> -        if expr.has_key('union'):
> -            if not discriminator_find_enum_define(expr):
> -                add_enum('%sKind' % expr['union'])
> -
>      try:
> +        # Next pass: learn the types and check for valid expression keys. At
> +        # this point, top-level 'include' has already been flattened.
> +        for expr_elem in schema.exprs:
> +            expr = expr_elem['expr']
> +            if expr.has_key('enum'):
> +                check_keys(expr_elem, 'enum', ['data'])
> +                add_enum(expr['enum'], expr['data'])
> +            elif expr.has_key('union'):
> +                # Two styles of union, based on discriminator
> +                discriminator = expr.get('discriminator')
> +                if discriminator == {}:
> +                    check_keys(expr_elem, 'union', ['data', 'discriminator'])
> +                else:
> +                    check_keys(expr_elem, 'union', ['data'],
> +                               ['base', 'discriminator'])
> +                add_union(expr)
> +            elif expr.has_key('type'):
> +                check_keys(expr_elem, 'type', ['data'], ['base'])
> +                add_struct(expr)
> +            elif expr.has_key('command'):
> +                check_keys(expr_elem, 'command', [],
> +                           ['data', 'returns', 'gen', 'success-response'])
> +            elif expr.has_key('event'):
> +                check_keys(expr_elem, 'event', [], ['data'])
> +            else:
> +                raise QAPIExprError(expr_elem['info'],
> +                                    "expression is missing metatype")
> +            exprs.append(expr)
> +
> +        # Try again for hidden UnionKind enum
> +        for expr_elem in schema.exprs:
> +            expr = expr_elem['expr']
> +            if expr.has_key('union'):
> +                if not discriminator_find_enum_define(expr):
> +                    add_enum('%sKind' % expr['union'])
> +
> +        # Final pass - validate that exprs make sense
>          check_exprs(schema)
>      except QAPIExprError, e:
>          print >>sys.stderr, e

This hunk is easier to review with whitespace ignored:

  @@ -356,13 +356,34 @@

       exprs = []
  -    for expr_elem in schema.exprs:
  +    try:
  +        # Next pass: learn the types and check for valid expression keys. At
  +        # this point, top-level 'include' has already been flattened.
  +        for expr_elem in schema.exprs:
           expr = expr_elem['expr']
           if expr.has_key('enum'):
  -            add_enum(expr['enum'], expr.get('data'))
  +                check_keys(expr_elem, 'enum', ['data'])
  +                add_enum(expr['enum'], expr['data'])
           elif expr.has_key('union'):
  +                # Two styles of union, based on discriminator
  +                discriminator = expr.get('discriminator')
  +                if discriminator == {}:
  +                    check_keys(expr_elem, 'union', ['data', 'discriminator'])
  +                else:
  +                    check_keys(expr_elem, 'union', ['data'],
  +                               ['base', 'discriminator'])
               add_union(expr)
           elif expr.has_key('type'):
  +                check_keys(expr_elem, 'type', ['data'], ['base'])
               add_struct(expr)
  +            elif expr.has_key('command'):
  +                check_keys(expr_elem, 'command', [],
  +                           ['data', 'returns', 'gen', 'success-response'])
  +            elif expr.has_key('event'):
  +                check_keys(expr_elem, 'event', [], ['data'])
  +            else:
  +                raise QAPIExprError(expr_elem['info'],
  +                                    "expression is missing metatype")
           exprs.append(expr)

       # Try again for hidden UnionKind enum
  @@ -372,7 +393,7 @@
               if not discriminator_find_enum_define(expr):
                   add_enum('%sKind' % expr['union'])

  -    try:
  +        # Final pass - validate that exprs make sense
           check_exprs(schema)
       except QAPIExprError, e:
           print >>sys.stderr, e

Looks good to me.  The tests, too.

[...]



reply via email to

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