| Message ID | 1411165504-18198-11-git-send-email-eblake@redhat.com |
|---|---|
| State | New |
| Headers | show |
Eric Blake <eblake@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 <eblake@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...]
On 09/24/2014 05:58 AM, Markus Armbruster wrote: > > We have separate name spaces for events, commands and types. Works for > me. A single name space would work for me, too. I thought about that too. Our conventions are that commands are all-lower-case, events are ALL_UPPER, and user-defined types are CamelCase - so a single namespace would not have any collisions, except for one case: built-in types like 'int' are also all lower, which collides with commands. But I see no technical reason why we wouldn't be able to generate C code for a command named 'int'. I can go either way, but should probably add a test for a .json file that does {'command':'int'} to test which way we go. Preferences on whether that should be allowed or forbidden?
Eric Blake <eblake@redhat.com> writes: > On 09/24/2014 05:58 AM, Markus Armbruster wrote: > >> >> We have separate name spaces for events, commands and types. Works for >> me. A single name space would work for me, too. > > I thought about that too. Our conventions are that commands are > all-lower-case, events are ALL_UPPER, and user-defined types are > CamelCase - so a single namespace would not have any collisions, except > for one case: built-in types like 'int' are also all lower, which > collides with commands. But I see no technical reason why we wouldn't > be able to generate C code for a command named 'int'. I can go either > way, but should probably add a test for a .json file that does > {'command':'int'} to test which way we go. Preferences on whether that > should be allowed or forbidden? With a single name space, 'command': 'int' should collide with the built-in type. With separate name spaces, it should in theory just work. No big deal if it doesn't due to generator sloppiness or something. As I said, I'm fine both with keeping the current separate name space, and with switching to a single name space. Either way, documentation would be nice.
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' ] +enum_types = [] +struct_types = [] +union_types = [] +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) + 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: @@ -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): diff --git a/tests/qapi-schema/redefined-builtin.err b/tests/qapi-schema/redefined-builtin.err index e69de29..b275722 100644 --- a/tests/qapi-schema/redefined-builtin.err +++ b/tests/qapi-schema/redefined-builtin.err @@ -0,0 +1 @@ +tests/qapi-schema/redefined-builtin.json:2: built-in 'size' is already defined diff --git a/tests/qapi-schema/redefined-builtin.exit b/tests/qapi-schema/redefined-builtin.exit index 573541a..d00491f 100644 --- a/tests/qapi-schema/redefined-builtin.exit +++ b/tests/qapi-schema/redefined-builtin.exit @@ -1 +1 @@ -0 +1 diff --git a/tests/qapi-schema/redefined-builtin.json b/tests/qapi-schema/redefined-builtin.json index a10050d..df328cc 100644 --- a/tests/qapi-schema/redefined-builtin.json +++ b/tests/qapi-schema/redefined-builtin.json @@ -1,2 +1,2 @@ -# FIXME: we should reject types that duplicate builtin names +# we reject types that duplicate builtin names { 'type': 'size', 'data': { 'myint': 'size' } } diff --git a/tests/qapi-schema/redefined-builtin.out b/tests/qapi-schema/redefined-builtin.out index b0a9aea..e69de29 100644 --- a/tests/qapi-schema/redefined-builtin.out +++ b/tests/qapi-schema/redefined-builtin.out @@ -1,3 +0,0 @@ -[OrderedDict([('type', 'size'), ('data', OrderedDict([('myint', 'size')]))])] -[] -[OrderedDict([('type', 'size'), ('data', OrderedDict([('myint', 'size')]))])] diff --git a/tests/qapi-schema/redefined-command.err b/tests/qapi-schema/redefined-command.err index e69de29..82ae256 100644 --- a/tests/qapi-schema/redefined-command.err +++ b/tests/qapi-schema/redefined-command.err @@ -0,0 +1 @@ +tests/qapi-schema/redefined-command.json:3: command 'foo' is already defined diff --git a/tests/qapi-schema/redefined-command.exit b/tests/qapi-schema/redefined-command.exit index 573541a..d00491f 100644 --- a/tests/qapi-schema/redefined-command.exit +++ b/tests/qapi-schema/redefined-command.exit @@ -1 +1 @@ -0 +1 diff --git a/tests/qapi-schema/redefined-command.json b/tests/qapi-schema/redefined-command.json index d8c9975..247e401 100644 --- a/tests/qapi-schema/redefined-command.json +++ b/tests/qapi-schema/redefined-command.json @@ -1,3 +1,3 @@ -# FIXME: we should reject commands defined more than once +# we reject commands defined more than once { 'command': 'foo', 'data': { 'one': 'str' } } { 'command': 'foo', 'data': { '*two': 'str' } } diff --git a/tests/qapi-schema/redefined-command.out b/tests/qapi-schema/redefined-command.out index 44ddba6..e69de29 100644 --- a/tests/qapi-schema/redefined-command.out +++ b/tests/qapi-schema/redefined-command.out @@ -1,4 +0,0 @@ -[OrderedDict([('command', 'foo'), ('data', OrderedDict([('one', 'str')]))]), - OrderedDict([('command', 'foo'), ('data', OrderedDict([('*two', 'str')]))])] -[] -[] diff --git a/tests/qapi-schema/redefined-event.err b/tests/qapi-schema/redefined-event.err index e69de29..35429cb 100644 --- a/tests/qapi-schema/redefined-event.err +++ b/tests/qapi-schema/redefined-event.err @@ -0,0 +1 @@ +tests/qapi-schema/redefined-event.json:3: event 'EVENT_A' is already defined diff --git a/tests/qapi-schema/redefined-event.exit b/tests/qapi-schema/redefined-event.exit index 573541a..d00491f 100644 --- a/tests/qapi-schema/redefined-event.exit +++ b/tests/qapi-schema/redefined-event.exit @@ -1 +1 @@ -0 +1 diff --git a/tests/qapi-schema/redefined-event.json b/tests/qapi-schema/redefined-event.json index 152cce7..7717e91 100644 --- a/tests/qapi-schema/redefined-event.json +++ b/tests/qapi-schema/redefined-event.json @@ -1,3 +1,3 @@ -# FIXME: we should reject duplicate events +# we reject duplicate events { 'event': 'EVENT_A', 'data': { 'myint': 'int' } } { 'event': 'EVENT_A', 'data': { 'myint': 'int' } } diff --git a/tests/qapi-schema/redefined-event.out b/tests/qapi-schema/redefined-event.out index 261b183..e69de29 100644 --- a/tests/qapi-schema/redefined-event.out +++ b/tests/qapi-schema/redefined-event.out @@ -1,4 +0,0 @@ -[OrderedDict([('event', 'EVENT_A'), ('data', OrderedDict([('myint', 'int')]))]), - OrderedDict([('event', 'EVENT_A'), ('data', OrderedDict([('myint', 'int')]))])] -[] -[] diff --git a/tests/qapi-schema/redefined-type.err b/tests/qapi-schema/redefined-type.err index e69de29..06ea78c 100644 --- a/tests/qapi-schema/redefined-type.err +++ b/tests/qapi-schema/redefined-type.err @@ -0,0 +1 @@ +tests/qapi-schema/redefined-type.json:3: struct 'foo' is already defined diff --git a/tests/qapi-schema/redefined-type.exit b/tests/qapi-schema/redefined-type.exit index 573541a..d00491f 100644 --- a/tests/qapi-schema/redefined-type.exit +++ b/tests/qapi-schema/redefined-type.exit @@ -1 +1 @@ -0 +1 diff --git a/tests/qapi-schema/redefined-type.json b/tests/qapi-schema/redefined-type.json index 7972194..e6a5f24 100644 --- a/tests/qapi-schema/redefined-type.json +++ b/tests/qapi-schema/redefined-type.json @@ -1,3 +1,3 @@ -# FIXME: we should reject types defined more than once +# we reject types defined more than once { 'type': 'foo', 'data': { 'one': 'str' } } { 'enum': 'foo', 'data': [ 'two' ] } diff --git a/tests/qapi-schema/redefined-type.out b/tests/qapi-schema/redefined-type.out index b509e57..e69de29 100644 --- a/tests/qapi-schema/redefined-type.out +++ b/tests/qapi-schema/redefined-type.out @@ -1,4 +0,0 @@ -[OrderedDict([('type', 'foo'), ('data', OrderedDict([('one', 'str')]))]), - OrderedDict([('enum', 'foo'), ('data', ['two'])])] -[{'enum_name': 'foo', 'enum_values': ['two']}] -[OrderedDict([('type', 'foo'), ('data', OrderedDict([('one', 'str')]))])]
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 <eblake@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(-)