[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.
[...]
- [Qemu-devel] [PATCH v4 05/19] qapi: Add some enum tests, (continued)
- [Qemu-devel] [PATCH v4 04/19] qapi: Document type-safety considerations, Eric Blake, 2014/09/19
- [Qemu-devel] [PATCH v4 08/19] qapi: Better error messages for bad expressions, Eric Blake, 2014/09/19
- Re: [Qemu-devel] [PATCH v4 08/19] qapi: Better error messages for bad expressions,
Markus Armbruster <=
- Re: [Qemu-devel] [PATCH v4 08/19] qapi: Better error messages for bad expressions, Eric Blake, 2014/09/23
- Re: [Qemu-devel] [PATCH v4 08/19] qapi: Better error messages for bad expressions, Markus Armbruster, 2014/09/24
- Re: [Qemu-devel] [PATCH v4 08/19] qapi: Better error messages for bad expressions, Kevin Wolf, 2014/09/24
- Re: [Qemu-devel] [PATCH v4 08/19] qapi: Better error messages for bad expressions, Markus Armbruster, 2014/09/24
- Re: [Qemu-devel] [PATCH v4 08/19] qapi: Better error messages for bad expressions, Markus Armbruster, 2014/09/26
- Re: [Qemu-devel] [PATCH v4 08/19] qapi: Better error messages for bad expressions, Kevin Wolf, 2014/09/26
- Re: [Qemu-devel] [PATCH v4 08/19] qapi: Better error messages for bad expressions, Markus Armbruster, 2014/09/26
[Qemu-devel] [PATCH v4 09/19] qapi: Add tests of redefined expressions, Eric Blake, 2014/09/19
[Qemu-devel] [PATCH v4 11/19] qapi: Add tests of type bypass, Eric Blake, 2014/09/19