| Message ID | 1428206887-7921-12-git-send-email-eblake@redhat.com |
|---|---|
| State | New |
| Headers | show |
Eric Blake <eblake@redhat.com> writes: > Previous commits demonstrated that the generator had several > flaws with less-than-perfect unions: > - a simple union that listed the same branch twice (or two variant > names that map to the same C enumerator, including the implicit > MAX sentinel) ended up generating invalid C code > - an anonymous union that listed two branches with the same qtype > ended up generating invalid C code > - the generator crashed on anonymous union attempts to use an > array type > - the generator was silently ignoring a base type for anonymous > unions > - the generator allowed unknown types or nested anonymous unions > as a branch in an anonymous union > > Signed-off-by: Eric Blake <eblake@redhat.com> [...] > diff --git a/tests/qapi-schema/alternate-array.json b/tests/qapi-schema/alternate-array.json > index c2965cf..77970d9 100644 > --- a/tests/qapi-schema/alternate-array.json > +++ b/tests/qapi-schema/alternate-array.json > @@ -1,4 +1,4 @@ > -# FIXME: we do not support array branches of anonymous unions yet > +# we do not support array branches of anonymous unions yet > # TODO: should we support this? > { 'type': 'One', > 'data': { 'name': 'str' } } FIXME added in PATCH 08, demoted to plain comment now. Any particular reason for the churn? [...] Regardless, Reviewed-by: Markus Armbruster <armbru@redhat.com>
On 04/27/2015 12:15 PM, Markus Armbruster wrote: > Eric Blake <eblake@redhat.com> writes: > >> Previous commits demonstrated that the generator had several >> flaws with less-than-perfect unions: >> +++ b/tests/qapi-schema/alternate-array.json >> @@ -1,4 +1,4 @@ >> -# FIXME: we do not support array branches of anonymous unions yet >> +# we do not support array branches of anonymous unions yet >> # TODO: should we support this? >> { 'type': 'One', >> 'data': { 'name': 'str' } } > > FIXME added in PATCH 08, demoted to plain comment now. Any particular > reason for the churn? Poor rebasing skills on my part. I should have nuked the 'FIXME' out of commit 8, instead of churning here. > > [...] > > Regardless, Yeah, the churn doesn't hurt, and unless there's a good reason for a respin, I'd rather see this series go in near the beginning of 2.4 since the change to 'struct' later in the series WILL cause merge conflicts to additions to *.json files, so sooner is better than later and fighting repetitive rebases. > > Reviewed-by: Markus Armbruster <armbru@redhat.com> > >
On 04/27/2015 12:32 PM, Eric Blake wrote: > On 04/27/2015 12:15 PM, Markus Armbruster wrote: >> Eric Blake <eblake@redhat.com> writes: >> >>> Previous commits demonstrated that the generator had several >>> flaws with less-than-perfect unions: > >>> +++ b/tests/qapi-schema/alternate-array.json >>> @@ -1,4 +1,4 @@ >>> -# FIXME: we do not support array branches of anonymous unions yet >>> +# we do not support array branches of anonymous unions yet >>> # TODO: should we support this? >>> { 'type': 'One', >>> 'data': { 'name': 'str' } } >> >> FIXME added in PATCH 08, demoted to plain comment now. Any particular >> reason for the churn? > > Poor rebasing skills on my part. I should have nuked the 'FIXME' out of > commit 8, instead of churning here. Actually, commit 8 was demonstrating that we didn't flag arrays as invalid even though they don't work, and this commit fixes things to start rejecting them. Meanwhile, the TODO reminds us that we may once again want to accept them, but in a working configuration. At any rate, minor wording tweaks should make it more obvious.
diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py index f6fb930..2390887 100644 --- a/scripts/qapi-types.py +++ b/scripts/qapi-types.py @@ -181,17 +181,8 @@ const int %(name)s_qtypes[QTYPE_MAX] = { name=name) for key in members: - qapi_type = members[key] - if builtin_types.has_key(qapi_type): - qtype = builtin_types[qapi_type] - elif find_struct(qapi_type): - qtype = "QTYPE_QDICT" - elif find_union(qapi_type): - qtype = "QTYPE_QDICT" - elif find_enum(qapi_type): - qtype = "QTYPE_QSTRING" - else: - assert False, "Invalid anonymous union member" + qtype = find_anonymous_member_qtype(members[key]) + assert qtype, "Invalid anonymous union member" ret += mcgen(''' [ %(qtype)s ] = %(abbrev)s_KIND_%(enum)s, diff --git a/scripts/qapi.py b/scripts/qapi.py index 438468e..5f0f699 100644 --- a/scripts/qapi.py +++ b/scripts/qapi.py @@ -224,6 +224,23 @@ def find_base_fields(base): return None return base_struct_define['data'] +# Return the qtype of an anonymous union branch, or None on error. +def find_anonymous_member_qtype(qapi_type): + if builtin_types.has_key(qapi_type): + return builtin_types[qapi_type] + elif find_struct(qapi_type): + return "QTYPE_QDICT" + elif find_enum(qapi_type): + return "QTYPE_QSTRING" + else: + union = find_union(qapi_type) + if union: + discriminator = union.get('discriminator') + if discriminator == {}: + return None + return "QTYPE_QDICT" + return None + # Return the discriminator enum define if discriminator is specified as an # enum type, otherwise return None. def discriminator_find_enum_define(expr): @@ -258,6 +275,8 @@ def check_union(expr, expr_info): base = expr.get('base') discriminator = expr.get('discriminator') members = expr['data'] + values = { 'MAX': '(automatic)' } + types_seen = {} # If the object has a member 'base', its value must name a complex type, # and there must be a discriminator. @@ -266,26 +285,35 @@ def check_union(expr, expr_info): raise QAPIExprError(expr_info, "Union '%s' requires a discriminator to go " "along with base" %name) - 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 a # simple union. If 'discriminator' is {}, it is an anonymous union. - if not discriminator or discriminator == {}: + if discriminator is None or discriminator == {}: enum_define = None + if base is not None: + raise QAPIExprError(expr_info, + "Union '%s' must not have a base" + % name) # Else, it's a flat union. else: - # The object must have a member 'base'. - if not base: + # The object must have a string member 'base'. + if not isinstance(base, str): raise QAPIExprError(expr_info, - "Flat union '%s' must have a base field" + "Flat union '%s' must have a string base field" % name) + base_fields = find_base_fields(base) + if not base_fields: + raise QAPIExprError(expr_info, + "Base '%s' is not a valid type" + % base) + # The value of member 'discriminator' must name a member of the # base type. + if not isinstance(discriminator, str): + raise QAPIExprError(expr_info, + "Flat union '%s' discriminator must be a string" + % name) discriminator_type = base_fields.get(discriminator) if not discriminator_type: raise QAPIExprError(expr_info, @@ -301,15 +329,42 @@ def check_union(expr, expr_info): # Check every branch for (key, value) in members.items(): - # If this named member's value names an enum type, then all members + # If the discriminator 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: add checking for values. Key is checked as above, value can be - # also checked here, but we need more functions to handle array case. + if enum_define: + if 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"])) + + # Otherwise, check for conflicts in the generated enum + else: + c_key = _generate_enum_string(key) + if c_key in values: + raise QAPIExprError(expr_info, + "Union '%s' member '%s' clashes with '%s'" + % (name, key, values[c_key])) + values[c_key] = key + + # Ensure anonymous unions have no type conflicts. + if discriminator == {}: + if isinstance(value, list): + raise QAPIExprError(expr_info, + "Anonymous union '%s' member '%s' must " + "not be array type" % (name, key)) + qtype = find_anonymous_member_qtype(value) + if not qtype: + raise QAPIExprError(expr_info, + "Anonymous union '%s' member '%s' has " + "invalid type '%s'" % (name, key, value)) + if qtype in types_seen: + raise QAPIExprError(expr_info, + "Anonymous union '%s' member '%s' can't " + "be distinguished from member '%s'" + % (name, key, types_seen[qtype])) + types_seen[qtype] = key + def check_enum(expr, expr_info): name = expr['enum'] diff --git a/tests/qapi-schema/alternate-array.err b/tests/qapi-schema/alternate-array.err index e69de29..8d6ccc7 100644 --- a/tests/qapi-schema/alternate-array.err +++ b/tests/qapi-schema/alternate-array.err @@ -0,0 +1 @@ +tests/qapi-schema/alternate-array.json:5: Anonymous union 'MyUnion' member 'two' must not be array type diff --git a/tests/qapi-schema/alternate-array.exit b/tests/qapi-schema/alternate-array.exit index 573541a..d00491f 100644 --- a/tests/qapi-schema/alternate-array.exit +++ b/tests/qapi-schema/alternate-array.exit @@ -1 +1 @@ -0 +1 diff --git a/tests/qapi-schema/alternate-array.json b/tests/qapi-schema/alternate-array.json index c2965cf..77970d9 100644 --- a/tests/qapi-schema/alternate-array.json +++ b/tests/qapi-schema/alternate-array.json @@ -1,4 +1,4 @@ -# FIXME: we do not support array branches of anonymous unions yet +# we do not support array branches of anonymous unions yet # TODO: should we support this? { 'type': 'One', 'data': { 'name': 'str' } } diff --git a/tests/qapi-schema/alternate-array.out b/tests/qapi-schema/alternate-array.out index 90dc22c..e69de29 100644 --- a/tests/qapi-schema/alternate-array.out +++ b/tests/qapi-schema/alternate-array.out @@ -1,4 +0,0 @@ -[OrderedDict([('type', 'One'), ('data', OrderedDict([('name', 'str')]))]), - OrderedDict([('union', 'MyUnion'), ('discriminator', OrderedDict()), ('data', OrderedDict([('one', 'One'), ('two', ['int'])]))])] -[{'enum_name': 'MyUnionKind', 'enum_values': None}] -[OrderedDict([('type', 'One'), ('data', OrderedDict([('name', 'str')]))])] diff --git a/tests/qapi-schema/alternate-base.err b/tests/qapi-schema/alternate-base.err index e69de29..85595b2 100644 --- a/tests/qapi-schema/alternate-base.err +++ b/tests/qapi-schema/alternate-base.err @@ -0,0 +1 @@ +tests/qapi-schema/alternate-base.json:4: Union 'MyUnion' must not have a base diff --git a/tests/qapi-schema/alternate-base.exit b/tests/qapi-schema/alternate-base.exit index 573541a..d00491f 100644 --- a/tests/qapi-schema/alternate-base.exit +++ b/tests/qapi-schema/alternate-base.exit @@ -1 +1 @@ -0 +1 diff --git a/tests/qapi-schema/alternate-base.json b/tests/qapi-schema/alternate-base.json index 2d36db1..dad7f02 100644 --- a/tests/qapi-schema/alternate-base.json +++ b/tests/qapi-schema/alternate-base.json @@ -1,4 +1,4 @@ -# FIXME: we should reject anonymous union with base type +# we reject anonymous union with base type { 'type': 'Base', 'data': { 'string': 'str' } } { 'union': 'MyUnion', diff --git a/tests/qapi-schema/alternate-base.out b/tests/qapi-schema/alternate-base.out index 7fb31f5..e69de29 100644 --- a/tests/qapi-schema/alternate-base.out +++ b/tests/qapi-schema/alternate-base.out @@ -1,4 +0,0 @@ -[OrderedDict([('type', 'Base'), ('data', OrderedDict([('string', 'str')]))]), - OrderedDict([('union', 'MyUnion'), ('base', 'Base'), ('discriminator', OrderedDict()), ('data', OrderedDict([('number', 'int')]))])] -[{'enum_name': 'MyUnionKind', 'enum_values': None}] -[OrderedDict([('type', 'Base'), ('data', OrderedDict([('string', 'str')]))])] diff --git a/tests/qapi-schema/alternate-clash.err b/tests/qapi-schema/alternate-clash.err index e69de29..1130c12 100644 --- a/tests/qapi-schema/alternate-clash.err +++ b/tests/qapi-schema/alternate-clash.err @@ -0,0 +1 @@ +tests/qapi-schema/alternate-clash.json:2: Union 'Union1' member 'ONE' clashes with 'one' diff --git a/tests/qapi-schema/alternate-clash.exit b/tests/qapi-schema/alternate-clash.exit index 573541a..d00491f 100644 --- a/tests/qapi-schema/alternate-clash.exit +++ b/tests/qapi-schema/alternate-clash.exit @@ -1 +1 @@ -0 +1 diff --git a/tests/qapi-schema/alternate-clash.json b/tests/qapi-schema/alternate-clash.json index 7e2ef23..fa2d27e 100644 --- a/tests/qapi-schema/alternate-clash.json +++ b/tests/qapi-schema/alternate-clash.json @@ -1,4 +1,4 @@ -# FIXME: we should detect C enum collisions in an anonymous union +# we detect C enum collisions in an anonymous union { 'union': 'Union1', 'discriminator': {}, 'data': { 'one': 'str', 'ONE': 'int' } } diff --git a/tests/qapi-schema/alternate-clash.out b/tests/qapi-schema/alternate-clash.out index c6687fa..e69de29 100644 --- a/tests/qapi-schema/alternate-clash.out +++ b/tests/qapi-schema/alternate-clash.out @@ -1,3 +0,0 @@ -[OrderedDict([('union', 'Union1'), ('discriminator', OrderedDict()), ('data', OrderedDict([('one', 'str'), ('ONE', 'int')]))])] -[{'enum_name': 'Union1Kind', 'enum_values': None}] -[] diff --git a/tests/qapi-schema/alternate-conflict-dict.err b/tests/qapi-schema/alternate-conflict-dict.err index e69de29..2d4550c 100644 --- a/tests/qapi-schema/alternate-conflict-dict.err +++ b/tests/qapi-schema/alternate-conflict-dict.err @@ -0,0 +1 @@ +tests/qapi-schema/alternate-conflict-dict.json:6: Anonymous union 'MyUnion' member 'two' can't be distinguished from member 'one' diff --git a/tests/qapi-schema/alternate-conflict-dict.exit b/tests/qapi-schema/alternate-conflict-dict.exit index 573541a..d00491f 100644 --- a/tests/qapi-schema/alternate-conflict-dict.exit +++ b/tests/qapi-schema/alternate-conflict-dict.exit @@ -1 +1 @@ -0 +1 diff --git a/tests/qapi-schema/alternate-conflict-dict.json b/tests/qapi-schema/alternate-conflict-dict.json index d2ed9de..ded302e 100644 --- a/tests/qapi-schema/alternate-conflict-dict.json +++ b/tests/qapi-schema/alternate-conflict-dict.json @@ -1,4 +1,4 @@ -# FIXME: we should reject anonymous unions with multiple object branches +# we reject anonymous unions with multiple object branches { 'type': 'One', 'data': { 'name': 'str' } } { 'type': 'Two', diff --git a/tests/qapi-schema/alternate-conflict-dict.out b/tests/qapi-schema/alternate-conflict-dict.out index b9ac945..e69de29 100644 --- a/tests/qapi-schema/alternate-conflict-dict.out +++ b/tests/qapi-schema/alternate-conflict-dict.out @@ -1,6 +0,0 @@ -[OrderedDict([('type', 'One'), ('data', OrderedDict([('name', 'str')]))]), - OrderedDict([('type', 'Two'), ('data', OrderedDict([('value', 'int')]))]), - OrderedDict([('union', 'MyUnion'), ('discriminator', OrderedDict()), ('data', OrderedDict([('one', 'One'), ('two', 'Two')]))])] -[{'enum_name': 'MyUnionKind', 'enum_values': None}] -[OrderedDict([('type', 'One'), ('data', OrderedDict([('name', 'str')]))]), - OrderedDict([('type', 'Two'), ('data', OrderedDict([('value', 'int')]))])] diff --git a/tests/qapi-schema/alternate-conflict-string.err b/tests/qapi-schema/alternate-conflict-string.err index e69de29..271ddcd 100644 --- a/tests/qapi-schema/alternate-conflict-string.err +++ b/tests/qapi-schema/alternate-conflict-string.err @@ -0,0 +1 @@ +tests/qapi-schema/alternate-conflict-string.json:4: Anonymous union 'MyUnion' member 'two' can't be distinguished from member 'one' diff --git a/tests/qapi-schema/alternate-conflict-string.exit b/tests/qapi-schema/alternate-conflict-string.exit index 573541a..d00491f 100644 --- a/tests/qapi-schema/alternate-conflict-string.exit +++ b/tests/qapi-schema/alternate-conflict-string.exit @@ -1 +1 @@ -0 +1 diff --git a/tests/qapi-schema/alternate-conflict-string.json b/tests/qapi-schema/alternate-conflict-string.json index 35245a3..3834a3d 100644 --- a/tests/qapi-schema/alternate-conflict-string.json +++ b/tests/qapi-schema/alternate-conflict-string.json @@ -1,4 +1,4 @@ -# FIXME: we should reject anonymous unions with multiple string-like branches +# we reject anonymous unions with multiple string-like branches { 'enum': 'Enum', 'data': [ 'hello', 'world' ] } { 'union': 'MyUnion', diff --git a/tests/qapi-schema/alternate-conflict-string.out b/tests/qapi-schema/alternate-conflict-string.out index e7b39a2..e69de29 100644 --- a/tests/qapi-schema/alternate-conflict-string.out +++ b/tests/qapi-schema/alternate-conflict-string.out @@ -1,5 +0,0 @@ -[OrderedDict([('enum', 'Enum'), ('data', ['hello', 'world'])]), - OrderedDict([('union', 'MyUnion'), ('discriminator', OrderedDict()), ('data', OrderedDict([('one', 'str'), ('two', 'Enum')]))])] -[{'enum_name': 'Enum', 'enum_values': ['hello', 'world']}, - {'enum_name': 'MyUnionKind', 'enum_values': None}] -[] diff --git a/tests/qapi-schema/alternate-nested.err b/tests/qapi-schema/alternate-nested.err index e69de29..59df96e 100644 --- a/tests/qapi-schema/alternate-nested.err +++ b/tests/qapi-schema/alternate-nested.err @@ -0,0 +1 @@ +tests/qapi-schema/alternate-nested.json:5: Anonymous union 'Union2' member 'nested' has invalid type 'Union1' diff --git a/tests/qapi-schema/alternate-nested.exit b/tests/qapi-schema/alternate-nested.exit index 573541a..d00491f 100644 --- a/tests/qapi-schema/alternate-nested.exit +++ b/tests/qapi-schema/alternate-nested.exit @@ -1 +1 @@ -0 +1 diff --git a/tests/qapi-schema/alternate-nested.json b/tests/qapi-schema/alternate-nested.json index d5812bf..ed2b6b7 100644 --- a/tests/qapi-schema/alternate-nested.json +++ b/tests/qapi-schema/alternate-nested.json @@ -1,4 +1,4 @@ -# FIXME: we should reject a nested anonymous union branch +# we reject a nested anonymous union branch { 'union': 'Union1', 'discriminator': {}, 'data': { 'name': 'str', 'value': 'int' } } diff --git a/tests/qapi-schema/alternate-nested.out b/tests/qapi-schema/alternate-nested.out index 0137c1f..e69de29 100644 --- a/tests/qapi-schema/alternate-nested.out +++ b/tests/qapi-schema/alternate-nested.out @@ -1,5 +0,0 @@ -[OrderedDict([('union', 'Union1'), ('discriminator', OrderedDict()), ('data', OrderedDict([('name', 'str'), ('value', 'int')]))]), - OrderedDict([('union', 'Union2'), ('discriminator', OrderedDict()), ('data', OrderedDict([('nested', 'Union1')]))])] -[{'enum_name': 'Union1Kind', 'enum_values': None}, - {'enum_name': 'Union2Kind', 'enum_values': None}] -[] diff --git a/tests/qapi-schema/alternate-unknown.err b/tests/qapi-schema/alternate-unknown.err index e69de29..bf8e9ae 100644 --- a/tests/qapi-schema/alternate-unknown.err +++ b/tests/qapi-schema/alternate-unknown.err @@ -0,0 +1 @@ +tests/qapi-schema/alternate-unknown.json:2: Anonymous union 'Union' member 'unknown' has invalid type 'MissingType' diff --git a/tests/qapi-schema/alternate-unknown.exit b/tests/qapi-schema/alternate-unknown.exit index 573541a..d00491f 100644 --- a/tests/qapi-schema/alternate-unknown.exit +++ b/tests/qapi-schema/alternate-unknown.exit @@ -1 +1 @@ -0 +1 diff --git a/tests/qapi-schema/alternate-unknown.json b/tests/qapi-schema/alternate-unknown.json index 0bab9c2..0c305c2 100644 --- a/tests/qapi-schema/alternate-unknown.json +++ b/tests/qapi-schema/alternate-unknown.json @@ -1,4 +1,4 @@ -# FIXME: we should reject an anonymous union with unknown type in branch +# we reject an anonymous union with unknown type in branch { 'union': 'Union', 'discriminator': {}, 'data': { 'unknown': 'MissingType' } } diff --git a/tests/qapi-schema/alternate-unknown.out b/tests/qapi-schema/alternate-unknown.out index 0911cdc..e69de29 100644 --- a/tests/qapi-schema/alternate-unknown.out +++ b/tests/qapi-schema/alternate-unknown.out @@ -1,3 +0,0 @@ -[OrderedDict([('union', 'Union'), ('discriminator', OrderedDict()), ('data', OrderedDict([('unknown', 'MissingType')]))])] -[{'enum_name': 'UnionKind', 'enum_values': None}] -[] diff --git a/tests/qapi-schema/flat-union-bad-base.err b/tests/qapi-schema/flat-union-bad-base.err index 5962ff4..f9c31b2 100644 --- a/tests/qapi-schema/flat-union-bad-base.err +++ b/tests/qapi-schema/flat-union-bad-base.err @@ -1 +1 @@ -tests/qapi-schema/flat-union-bad-base.json:9: Base 'OrderedDict([('enum1', 'TestEnum'), ('kind', 'str')])' is not a valid type +tests/qapi-schema/flat-union-bad-base.json:9: Flat union 'TestUnion' must have a string base field diff --git a/tests/qapi-schema/flat-union-bad-base.json b/tests/qapi-schema/flat-union-bad-base.json index 6c14132..bb0f02d 100644 --- a/tests/qapi-schema/flat-union-bad-base.json +++ b/tests/qapi-schema/flat-union-bad-base.json @@ -1,4 +1,4 @@ -# FIXME: poor message: we require the base to be an existing complex type +# we require the base to be an existing complex type # TODO: should we allow an anonymous inline base type? { 'enum': 'TestEnum', 'data': [ 'value1', 'value2' ] } diff --git a/tests/qapi-schema/flat-union-bad-discriminator.err b/tests/qapi-schema/flat-union-bad-discriminator.err index e69de29..1661c52 100644 --- a/tests/qapi-schema/flat-union-bad-discriminator.err +++ b/tests/qapi-schema/flat-union-bad-discriminator.err @@ -0,0 +1 @@ +tests/qapi-schema/flat-union-bad-discriminator.json:10: Flat union 'TestUnion' discriminator must be a string diff --git a/tests/qapi-schema/flat-union-bad-discriminator.exit b/tests/qapi-schema/flat-union-bad-discriminator.exit index 573541a..d00491f 100644 --- a/tests/qapi-schema/flat-union-bad-discriminator.exit +++ b/tests/qapi-schema/flat-union-bad-discriminator.exit @@ -1 +1 @@ -0 +1 diff --git a/tests/qapi-schema/flat-union-bad-discriminator.json b/tests/qapi-schema/flat-union-bad-discriminator.json index 1599a59..3ce43e8 100644 --- a/tests/qapi-schema/flat-union-bad-discriminator.json +++ b/tests/qapi-schema/flat-union-bad-discriminator.json @@ -1,4 +1,4 @@ -# FIXME: we should require the discriminator to be a string naming a base-type member +# we require the discriminator to be a string naming a base-type member { 'enum': 'TestEnum', 'data': [ 'value1', 'value2' ] } { 'type': 'TestBase', diff --git a/tests/qapi-schema/flat-union-bad-discriminator.out b/tests/qapi-schema/flat-union-bad-discriminator.out index b6ce217..e69de29 100644 --- a/tests/qapi-schema/flat-union-bad-discriminator.out +++ b/tests/qapi-schema/flat-union-bad-discriminator.out @@ -1,10 +0,0 @@ -[OrderedDict([('enum', 'TestEnum'), ('data', ['value1', 'value2'])]), - OrderedDict([('type', 'TestBase'), ('data', OrderedDict([('enum1', 'TestEnum'), ('kind', 'str')]))]), - OrderedDict([('type', 'TestTypeA'), ('data', OrderedDict([('string', 'str')]))]), - OrderedDict([('type', 'TestTypeB'), ('data', OrderedDict([('integer', 'int')]))]), - OrderedDict([('union', 'TestUnion'), ('base', 'TestBase'), ('discriminator', []), ('data', OrderedDict([('kind1', 'TestTypeA'), ('kind2', 'TestTypeB')]))])] -[{'enum_name': 'TestEnum', 'enum_values': ['value1', 'value2']}, - {'enum_name': 'TestUnionKind', 'enum_values': None}] -[OrderedDict([('type', 'TestBase'), ('data', OrderedDict([('enum1', 'TestEnum'), ('kind', 'str')]))]), - OrderedDict([('type', 'TestTypeA'), ('data', OrderedDict([('string', 'str')]))]), - OrderedDict([('type', 'TestTypeB'), ('data', OrderedDict([('integer', 'int')]))])] diff --git a/tests/qapi-schema/flat-union-inline.err b/tests/qapi-schema/flat-union-inline.err index 51fbe54..ec58627 100644 --- a/tests/qapi-schema/flat-union-inline.err +++ b/tests/qapi-schema/flat-union-inline.err @@ -1 +1 @@ -tests/qapi-schema/flat-union-inline.json:7: Base 'OrderedDict([('enum1', 'TestEnum'), ('kind', 'str')])' is not a valid type +tests/qapi-schema/flat-union-inline.json:7: Flat union 'TestUnion' must have a string base field diff --git a/tests/qapi-schema/flat-union-inline.json b/tests/qapi-schema/flat-union-inline.json index 2bdffeb..f3da117 100644 --- a/tests/qapi-schema/flat-union-inline.json +++ b/tests/qapi-schema/flat-union-inline.json @@ -1,4 +1,4 @@ -# FIXME: poor message: we require branches to be a complex type name +# we require branches to be a complex type name # TODO: should we allow anonymous inline types? { 'enum': 'TestEnum', 'data': [ 'value1', 'value2' ] } diff --git a/tests/qapi-schema/flat-union-no-base.err b/tests/qapi-schema/flat-union-no-base.err index 97323a0..bb3f708 100644 --- a/tests/qapi-schema/flat-union-no-base.err +++ b/tests/qapi-schema/flat-union-no-base.err @@ -1 +1 @@ -tests/qapi-schema/flat-union-no-base.json:9: Flat union 'TestUnion' must have a base field +tests/qapi-schema/flat-union-no-base.json:9: Flat union 'TestUnion' must have a string base field diff --git a/tests/qapi-schema/flat-union-no-base.json b/tests/qapi-schema/flat-union-no-base.json index 08a0247..9547bb8 100644 --- a/tests/qapi-schema/flat-union-no-base.json +++ b/tests/qapi-schema/flat-union-no-base.json @@ -1,4 +1,4 @@ -# FIXME: flat unions should require a base +# flat unions require a base # TODO: simple unions should be able to use an enum discriminator { 'type': 'TestTypeA', 'data': { 'string': 'str' } } diff --git a/tests/qapi-schema/union-bad-branch.err b/tests/qapi-schema/union-bad-branch.err index e69de29..8822735 100644 --- a/tests/qapi-schema/union-bad-branch.err +++ b/tests/qapi-schema/union-bad-branch.err @@ -0,0 +1 @@ +tests/qapi-schema/union-bad-branch.json:6: Union 'MyUnion' member 'ONE' clashes with 'one' diff --git a/tests/qapi-schema/union-bad-branch.exit b/tests/qapi-schema/union-bad-branch.exit index 573541a..d00491f 100644 --- a/tests/qapi-schema/union-bad-branch.exit +++ b/tests/qapi-schema/union-bad-branch.exit @@ -1 +1 @@ -0 +1 diff --git a/tests/qapi-schema/union-bad-branch.json b/tests/qapi-schema/union-bad-branch.json index 11e46de..4303666 100644 --- a/tests/qapi-schema/union-bad-branch.json +++ b/tests/qapi-schema/union-bad-branch.json @@ -1,4 +1,4 @@ -# FIXME: we should reject normal unions where branches would collide in C +# we reject normal unions where branches would collide in C { 'type': 'One', 'data': { 'string': 'str' } } { 'type': 'Two', diff --git a/tests/qapi-schema/union-bad-branch.out b/tests/qapi-schema/union-bad-branch.out index 6baf01b..e69de29 100644 --- a/tests/qapi-schema/union-bad-branch.out +++ b/tests/qapi-schema/union-bad-branch.out @@ -1,6 +0,0 @@ -[OrderedDict([('type', 'One'), ('data', OrderedDict([('string', 'str')]))]), - OrderedDict([('type', 'Two'), ('data', OrderedDict([('number', 'int')]))]), - OrderedDict([('union', 'MyUnion'), ('data', OrderedDict([('one', 'One'), ('ONE', 'Two')]))])] -[{'enum_name': 'MyUnionKind', 'enum_values': None}] -[OrderedDict([('type', 'One'), ('data', OrderedDict([('string', 'str')]))]), - OrderedDict([('type', 'Two'), ('data', OrderedDict([('number', 'int')]))])] diff --git a/tests/qapi-schema/union-max.err b/tests/qapi-schema/union-max.err index e69de29..55ce439 100644 --- a/tests/qapi-schema/union-max.err +++ b/tests/qapi-schema/union-max.err @@ -0,0 +1 @@ +tests/qapi-schema/union-max.json:2: Union 'Union' member 'max' clashes with '(automatic)' diff --git a/tests/qapi-schema/union-max.exit b/tests/qapi-schema/union-max.exit index 573541a..d00491f 100644 --- a/tests/qapi-schema/union-max.exit +++ b/tests/qapi-schema/union-max.exit @@ -1 +1 @@ -0 +1 diff --git a/tests/qapi-schema/union-max.json b/tests/qapi-schema/union-max.json index 45648c4..d6ad986 100644 --- a/tests/qapi-schema/union-max.json +++ b/tests/qapi-schema/union-max.json @@ -1,3 +1,3 @@ -# FIXME: we should reject 'max' branch in a union, for collision with C enum +# we reject 'max' branch in a union, for collision with C enum { 'union': 'Union', 'data': { 'max': 'int' } } diff --git a/tests/qapi-schema/union-max.out b/tests/qapi-schema/union-max.out index 2757d36..e69de29 100644 --- a/tests/qapi-schema/union-max.out +++ b/tests/qapi-schema/union-max.out @@ -1,3 +0,0 @@ -[OrderedDict([('union', 'Union'), ('data', OrderedDict([('max', 'int')]))])] -[{'enum_name': 'UnionKind', 'enum_values': None}] -[]
Previous commits demonstrated that the generator had several flaws with less-than-perfect unions: - a simple union that listed the same branch twice (or two variant names that map to the same C enumerator, including the implicit MAX sentinel) ended up generating invalid C code - an anonymous union that listed two branches with the same qtype ended up generating invalid C code - the generator crashed on anonymous union attempts to use an array type - the generator was silently ignoring a base type for anonymous unions - the generator allowed unknown types or nested anonymous unions as a branch in an anonymous union Signed-off-by: Eric Blake <eblake@redhat.com> --- v6: rebase tests on top of tweaks earlier in series; subject changed from v5:8/28; s/ordinary/simple/; tweak error message for alternate-conflict-string; drop fixme in union-base-no-discriminator; split out removal of simple-with-base into earlier commit --- scripts/qapi-types.py | 13 +--- scripts/qapi.py | 89 +++++++++++++++++----- tests/qapi-schema/alternate-array.err | 1 + tests/qapi-schema/alternate-array.exit | 2 +- tests/qapi-schema/alternate-array.json | 2 +- tests/qapi-schema/alternate-array.out | 4 - tests/qapi-schema/alternate-base.err | 1 + tests/qapi-schema/alternate-base.exit | 2 +- tests/qapi-schema/alternate-base.json | 2 +- tests/qapi-schema/alternate-base.out | 4 - tests/qapi-schema/alternate-clash.err | 1 + tests/qapi-schema/alternate-clash.exit | 2 +- tests/qapi-schema/alternate-clash.json | 2 +- tests/qapi-schema/alternate-clash.out | 3 - tests/qapi-schema/alternate-conflict-dict.err | 1 + tests/qapi-schema/alternate-conflict-dict.exit | 2 +- tests/qapi-schema/alternate-conflict-dict.json | 2 +- tests/qapi-schema/alternate-conflict-dict.out | 6 -- tests/qapi-schema/alternate-conflict-string.err | 1 + tests/qapi-schema/alternate-conflict-string.exit | 2 +- tests/qapi-schema/alternate-conflict-string.json | 2 +- tests/qapi-schema/alternate-conflict-string.out | 5 -- tests/qapi-schema/alternate-nested.err | 1 + tests/qapi-schema/alternate-nested.exit | 2 +- tests/qapi-schema/alternate-nested.json | 2 +- tests/qapi-schema/alternate-nested.out | 5 -- tests/qapi-schema/alternate-unknown.err | 1 + tests/qapi-schema/alternate-unknown.exit | 2 +- tests/qapi-schema/alternate-unknown.json | 2 +- tests/qapi-schema/alternate-unknown.out | 3 - tests/qapi-schema/flat-union-bad-base.err | 2 +- tests/qapi-schema/flat-union-bad-base.json | 2 +- tests/qapi-schema/flat-union-bad-discriminator.err | 1 + .../qapi-schema/flat-union-bad-discriminator.exit | 2 +- .../qapi-schema/flat-union-bad-discriminator.json | 2 +- tests/qapi-schema/flat-union-bad-discriminator.out | 10 --- tests/qapi-schema/flat-union-inline.err | 2 +- tests/qapi-schema/flat-union-inline.json | 2 +- tests/qapi-schema/flat-union-no-base.err | 2 +- tests/qapi-schema/flat-union-no-base.json | 2 +- tests/qapi-schema/union-bad-branch.err | 1 + tests/qapi-schema/union-bad-branch.exit | 2 +- tests/qapi-schema/union-bad-branch.json | 2 +- tests/qapi-schema/union-bad-branch.out | 6 -- tests/qapi-schema/union-max.err | 1 + tests/qapi-schema/union-max.exit | 2 +- tests/qapi-schema/union-max.json | 2 +- tests/qapi-schema/union-max.out | 3 - 48 files changed, 110 insertions(+), 103 deletions(-)