Eric Blake <ebl...@redhat.com> writes:
> The previous commit demonstrated that the generator overlooked
> duplicate expressions:
> - a complex type reusing a built-in type name
> - redeclaration of a type name, whether by the same or different
> metatype
> - redeclaration of a command or event
> - lack of tracking of 'size' as a built-in type
>
> Add a global array to track all known types and their source,
> as well as enhancing check_exprs to also check for duplicate
> events and commands. 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 <ebl...@redhat.com>
> ---
> scripts/qapi.py | 71
> +++++++++++++++++++++++++-------
> tests/qapi-schema/redefined-builtin.err | 1 +
> tests/qapi-schema/redefined-builtin.exit | 2 +-
> tests/qapi-schema/redefined-builtin.json | 2 +-
> tests/qapi-schema/redefined-builtin.out | 3 --
> tests/qapi-schema/redefined-command.err | 1 +
> tests/qapi-schema/redefined-command.exit | 2 +-
> tests/qapi-schema/redefined-command.json | 2 +-
> tests/qapi-schema/redefined-command.out | 4 --
> tests/qapi-schema/redefined-event.err | 1 +
> tests/qapi-schema/redefined-event.exit | 2 +-
> tests/qapi-schema/redefined-event.json | 2 +-
> tests/qapi-schema/redefined-event.out | 4 --
> tests/qapi-schema/redefined-type.err | 1 +
> tests/qapi-schema/redefined-type.exit | 2 +-
> tests/qapi-schema/redefined-type.json | 2 +-
> tests/qapi-schema/redefined-type.out | 4 --
> 17 files changed, 69 insertions(+), 37 deletions(-)
>
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index 8fbc45f..bf243fa 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -19,8 +19,15 @@ import sys
> builtin_types = [
> 'str', 'int', 'number', 'bool',
> 'int8', 'int16', 'int32', 'int64',
> - 'uint8', 'uint16', 'uint32', 'uint64'
> + 'uint8', 'uint16', 'uint32', 'uint64',
> + 'size'
> ]
I figure we want a blank line here.
Adding 'size' should have the following effects:
* Type sizeList is generated into qapi-types.h
* Declaration of qapi_free_sizeList() is generated into qapi-types.h,
definition into qapi-types.c
* generate_visit_anon_union() no longer fails an assertion when it runs
into a member of type 'size'
* Declaration of visit_type_sizeList() is generated into qapi-visit.h,
definition into qapi-visit.c
Make sense.
How can we be sure we now got all built-in types covered? Documentation
says yes, but it cannot be trusted. I figure the best evidence we have
is c_type(). Looks good.
Aside: have a look at how it recognizes event names: "name ==
name.upper()". Ugh! I guess this patch lets us clean it up to "name in
events".
I think you should add 'size' to builtin_type_qtypes[], too.
> +enum_types = []
> +struct_types = []
> +union_types = []
Semi-related code motion. Okay.
> +all_types = {}
> +commands = []
> +events = ['MAX']
>
> builtin_type_qtypes = {
> 'str': 'QTYPE_QSTRING',
> @@ -248,8 +255,22 @@ def discriminator_find_enum_define(expr):
>
> return find_enum(discriminator_type)
>
> +def check_command(expr, expr_info):
> + global commands
> + name = expr['command']
> + if name in commands:
> + raise QAPIExprError(expr_info,
> + "command '%s' is already defined" % name)
> + commands.append(name)
> +
This rejects duplicate commands. Good.
> def check_event(expr, expr_info):
> + global events
> + name = expr['event']
> params = expr.get('data')
> + if name in events:
> + raise QAPIExprError(expr_info,
> + "event '%s' is already defined" % name)
> + events.append(name)
> if params:
> for argname, argentry, optional, structured in parse_args(params):
> if structured:
This rejects duplicate events. Good.
> @@ -347,6 +368,8 @@ def check_exprs(schema):
> check_event(expr, info)
> if expr.has_key('enum'):
> check_enum(expr, info)
> + if expr.has_key('command'):
> + check_command(expr, info)
>
> def check_keys(expr_elem, meta, required, optional=[]):
> expr = expr_elem['expr']
> @@ -370,6 +393,9 @@ def check_keys(expr_elem, meta, required, optional=[]):
>
>
> def parse_schema(input_file):
> + global all_types
> + exprs = []
> +
> # First pass: read entire file into memory
> try:
> schema = QAPISchema(open(input_file, "r"))
> @@ -377,16 +403,17 @@ def parse_schema(input_file):
> print >>sys.stderr, e
> exit(1)
>
> - exprs = []
> -
> try:
> # Next pass: learn the types and check for valid expression keys. At
> # this point, top-level 'include' has already been flattened.
> + for builtin in builtin_types:
> + all_types[builtin] = 'built-in'
> for expr_elem in schema.exprs:
> expr = expr_elem['expr']
> + info = expr_elem['info']
> if expr.has_key('enum'):
> check_keys(expr_elem, 'enum', ['data'])
> - add_enum(expr['enum'], expr['data'])
> + add_enum(expr['enum'], info, expr['data'])
> elif expr.has_key('union'):
> # Two styles of union, based on discriminator
> discriminator = expr.get('discriminator')
> @@ -395,10 +422,10 @@ def parse_schema(input_file):
> else:
> check_keys(expr_elem, 'union', ['data'],
> ['base', 'discriminator'])
> - add_union(expr)
> + add_union(expr, info)
> elif expr.has_key('type'):
> check_keys(expr_elem, 'type', ['data'], ['base'])
> - add_struct(expr)
> + add_struct(expr, info)
> elif expr.has_key('command'):
> check_keys(expr_elem, 'command', [],
> ['data', 'returns', 'gen', 'success-response'])
> @@ -414,7 +441,7 @@ def parse_schema(input_file):
> expr = expr_elem['expr']
> if expr.has_key('union'):
> if not discriminator_find_enum_define(expr):
> - add_enum('%sKind' % expr['union'])
> + add_enum('%sKind' % expr['union'], expr_elem['info'])
>
> # Final pass - validate that exprs make sense
> check_exprs(schema)
> @@ -508,12 +535,15 @@ def type_name(name):
> return c_list_type(name[0])
> return name
>
> -enum_types = []
> -struct_types = []
> -union_types = []
> -
> -def add_struct(definition):
> +def add_struct(definition, info):
> global struct_types
> + global all_types
> + name = definition['type']
> + if name in all_types:
> + raise QAPIExprError(info,
> + "%s '%s' is already defined"
> + %(all_types[name], name))
> + all_types[name] = 'struct'
> struct_types.append(definition)
>
> def find_struct(name):
> @@ -523,8 +553,15 @@ def find_struct(name):
> return struct
> return None
>
> -def add_union(definition):
> +def add_union(definition, info):
> global union_types
> + global all_types
> + name = definition['union']
> + if name in all_types:
> + raise QAPIExprError(info,
> + "%s '%s' is already defined"
> + %(all_types[name], name))
> + all_types[name] = 'union'
> union_types.append(definition)
>
> def find_union(name):
> @@ -534,8 +571,14 @@ def find_union(name):
> return union
> return None
>
> -def add_enum(name, enum_values = None):
> +def add_enum(name, info, enum_values = None):
> global enum_types
> + global all_types
> + if name in all_types:
> + raise QAPIExprError(info,
> + "%s '%s' is already defined"
> + %(all_types[name], name))
> + all_types[name] = 'enum'
> enum_types.append({"enum_name": name, "enum_values": enum_values})
>
> def find_enum(name):
These hunks reject duplicate types of any kind: enum, complex
(a.k.a. struct), union.
We have separate name spaces for events, commands and types. Works for
me. A single name space would work for me, too.
[Tests snipped, they look good...]
