| Message ID | 1427227433-5030-11-git-send-email-eblake@redhat.com |
|---|---|
| State | New |
| Headers | show |
Eric Blake <eblake@redhat.com> writes: > Special-casing 'discriminator == {}' for handling anonymous unions > is getting awkward; since this particular type is not always a > dictionary on the wire, it is easier to treat it as a completely > different class of type, "alternate", so that if a type is listed > in the union_types array, we know it is not an anonymous union. > > This patch just further segregates union handling, to make sure that > anonymous unions are not stored in union_types, and splitting up > check_union() into separate functions. A future patch will change > the qapi grammar, and having the segregation already in place will > make it easier to deal with the distinct meta-type. > > Signed-off-by: Eric Blake <eblake@redhat.com> > --- > scripts/qapi-types.py | 6 ++-- > scripts/qapi-visit.py | 4 +-- > scripts/qapi.py | 94 +++++++++++++++++++++++++++++---------------------- > 3 files changed, 58 insertions(+), 46 deletions(-) > > diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py > index 2390887..c9e0201 100644 > --- a/scripts/qapi-types.py > +++ b/scripts/qapi-types.py > @@ -170,7 +170,7 @@ typedef enum %(name)s > > return lookup_decl + enum_decl > > -def generate_anon_union_qtypes(expr): > +def generate_alternate_qtypes(expr): > > name = expr['union'] > members = expr['data'] > @@ -181,7 +181,7 @@ const int %(name)s_qtypes[QTYPE_MAX] = { > name=name) > > for key in members: > - qtype = find_anonymous_member_qtype(members[key]) > + qtype = find_alternate_member_qtype(members[key]) > assert qtype, "Invalid anonymous union member" > > ret += mcgen(''' > @@ -408,7 +408,7 @@ for expr in exprs: > fdef.write(generate_enum_lookup('%sKind' % expr['union'], > expr['data'].keys())) > if expr.get('discriminator') == {}: > - fdef.write(generate_anon_union_qtypes(expr)) > + fdef.write(generate_alternate_qtypes(expr)) > else: > continue > fdecl.write(ret) > diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py > index 3f82bd4..77b0a1f 100644 > --- a/scripts/qapi-visit.py > +++ b/scripts/qapi-visit.py > @@ -237,7 +237,7 @@ void visit_type_%(name)s(Visitor *m, %(name)s *obj, const char *name, Error **er > ''', > name=name) > > -def generate_visit_anon_union(name, members): > +def generate_visit_alternate(name, members): > ret = mcgen(''' > > void visit_type_%(name)s(Visitor *m, %(name)s **obj, const char *name, Error **errp) > @@ -302,7 +302,7 @@ def generate_visit_union(expr): > > if discriminator == {}: > assert not base > - return generate_visit_anon_union(name, members) > + return generate_visit_alternate(name, members) > > enum_define = discriminator_find_enum_define(expr) > if enum_define: > diff --git a/scripts/qapi.py b/scripts/qapi.py > index 39cc88b..17252e9 100644 > --- a/scripts/qapi.py > +++ b/scripts/qapi.py > @@ -224,21 +224,16 @@ 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): > +# Return the qtype of an alternate branch, or None on error. > +def find_alternate_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" > + elif find_union(qapi_type): > + return "QTYPE_QDICT" > return None > > # Return the discriminator enum define if discriminator is specified as an > @@ -276,22 +271,13 @@ def check_union(expr, expr_info): > discriminator = expr.get('discriminator') > members = expr['data'] > values = { 'MAX': '(automatic)' } > - types_seen = {} > > - # Three types of unions, determined by discriminator. > + # Two types of unions, determined by discriminator. > + assert discriminator != {} > > - # If the value of member 'discriminator' is {}, it's an > - # anonymous union, and must not have a base. > - if discriminator == {}: > - enum_define = None > - if base: > - raise QAPIExprError(expr_info, > - "Anonymous union '%s' must not have a base" > - % name) > - > - # Else if the union object has no member 'discriminator', it's an > + # If the union object has no member 'discriminator', it's an > # ordinary union. For now, it must not have a base. > - elif not discriminator: > + if not discriminator: > enum_define = None > if base: > raise QAPIExprError(expr_info, > @@ -346,24 +332,46 @@ def check_union(expr, expr_info): > % (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' has " > - "same QObject type as member '%s'" > - % (name, key, types_seen[qtype])) > - types_seen[qtype] = key > +def check_alternate(expr, expr_info): > + name = expr['union'] > + base = expr.get('base') > + discriminator = expr.get('discriminator') > + members = expr['data'] > + values = { 'MAX': '(automatic)' } > + types_seen = {} > > + assert discriminator == {} > + if base: > + raise QAPIExprError(expr_info, > + "Anonymous union '%s' must not have a base" > + % name) > + > + # Check every branch > + for (key, value) in members.items(): > + # Check for conflicts in the generated enum > + 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 alternates have no type conflicts. > + if isinstance(value, list): > + raise QAPIExprError(expr_info, > + "Anonymous union '%s' member '%s' must " > + "not be array type" % (name, key)) > + qtype = find_alternate_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' has " > + "same QObject type as member '%s'" > + % (name, key, types_seen[qtype])) > + types_seen[qtype] = key > > def check_enum(expr, expr_info): > name = expr['enum'] > @@ -393,7 +401,10 @@ def check_exprs(schema): > if expr.has_key('enum'): > check_enum(expr, info) > elif expr.has_key('union'): > - check_union(expr, info) > + if expr.get('discriminator') == {}: > + check_alternate(expr, info) > + else: > + check_union(expr, info) > elif expr.has_key('event'): > check_event(expr, info) > > @@ -535,7 +546,8 @@ def find_struct(name): > > def add_union(definition): > global union_types > - union_types.append(definition) > + if definition.get('discriminator') != {}: > + union_types.append(definition) > > def find_union(name): > global union_types This is the only unobvious hunk. union_types is used only through find_union. The hunk makes find_union(N) return None when N names an anonymous union. find_union() is used in two places: * find_alternate_member_qtype() Patched further up. It really wants only non-anonymous unions, and this change to find_union() renders its check for anonymous unions superfluous. Good. * generate_visit_alternate() Asserts that each member's type is either a built-in type, a complex type, a union type, or an enum type. The change relaxes the assertion not to trigger on anonymous union types. Why is that okay?
On 03/26/2015 08:47 AM, Markus Armbruster wrote: > Eric Blake <eblake@redhat.com> writes: > >> Special-casing 'discriminator == {}' for handling anonymous unions >> is getting awkward; since this particular type is not always a >> dictionary on the wire, it is easier to treat it as a completely >> different class of type, "alternate", so that if a type is listed >> in the union_types array, we know it is not an anonymous union. >> >> This patch just further segregates union handling, to make sure that >> anonymous unions are not stored in union_types, and splitting up >> check_union() into separate functions. A future patch will change >> the qapi grammar, and having the segregation already in place will >> make it easier to deal with the distinct meta-type. >> >> Signed-off-by: Eric Blake <eblake@redhat.com> >> --- >> @@ -535,7 +546,8 @@ def find_struct(name): >> >> def add_union(definition): >> global union_types >> - union_types.append(definition) >> + if definition.get('discriminator') != {}: >> + union_types.append(definition) >> >> def find_union(name): >> global union_types > > This is the only unobvious hunk. > > union_types is used only through find_union. The hunk makes > find_union(N) return None when N names an anonymous union. > > find_union() is used in two places: > > * find_alternate_member_qtype() > > Patched further up. It really wants only non-anonymous unions, and > this change to find_union() renders its check for anonymous unions > superfluous. Good. > > * generate_visit_alternate() > > Asserts that each member's type is either a built-in type, a complex > type, a union type, or an enum type. > > The change relaxes the assertion not to trigger on anonymous union > types. Why is that okay? No, the change tightens the assertion so that it will now fail on an anonymous union nested as a branch of another anonymous union (where before it could silently pass the assertion), because the anonymous union is no longer found by find_union(). And this is okay because the earlier change to find_alternate_member_qtype means that we don't allow nested anonymous unions, so making the assertion fail if an anonymous union gets through anyway is correct.
Eric Blake <eblake@redhat.com> writes: > On 03/26/2015 08:47 AM, Markus Armbruster wrote: >> Eric Blake <eblake@redhat.com> writes: >> >>> Special-casing 'discriminator == {}' for handling anonymous unions >>> is getting awkward; since this particular type is not always a >>> dictionary on the wire, it is easier to treat it as a completely >>> different class of type, "alternate", so that if a type is listed >>> in the union_types array, we know it is not an anonymous union. >>> >>> This patch just further segregates union handling, to make sure that >>> anonymous unions are not stored in union_types, and splitting up >>> check_union() into separate functions. A future patch will change >>> the qapi grammar, and having the segregation already in place will >>> make it easier to deal with the distinct meta-type. >>> >>> Signed-off-by: Eric Blake <eblake@redhat.com> >>> --- > >>> @@ -535,7 +546,8 @@ def find_struct(name): >>> >>> def add_union(definition): >>> global union_types >>> - union_types.append(definition) >>> + if definition.get('discriminator') != {}: >>> + union_types.append(definition) >>> >>> def find_union(name): >>> global union_types >> >> This is the only unobvious hunk. >> >> union_types is used only through find_union. The hunk makes >> find_union(N) return None when N names an anonymous union. >> >> find_union() is used in two places: >> >> * find_alternate_member_qtype() >> >> Patched further up. It really wants only non-anonymous unions, and >> this change to find_union() renders its check for anonymous unions >> superfluous. Good. >> >> * generate_visit_alternate() >> >> Asserts that each member's type is either a built-in type, a complex >> type, a union type, or an enum type. >> >> The change relaxes the assertion not to trigger on anonymous union >> types. Why is that okay? > > No, the change tightens the assertion so that it will now fail on an > anonymous union nested as a branch of another anonymous union (where > before it could silently pass the assertion), because the anonymous > union is no longer found by find_union(). And this is okay because the > earlier change to find_alternate_member_qtype means that we don't allow > nested anonymous unions, so making the assertion fail if an anonymous > union gets through anyway is correct. Thanks for unconfusing me. Reviewed-by: Markus Armbruster <armbru@redhat.com>
diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py index 2390887..c9e0201 100644 --- a/scripts/qapi-types.py +++ b/scripts/qapi-types.py @@ -170,7 +170,7 @@ typedef enum %(name)s return lookup_decl + enum_decl -def generate_anon_union_qtypes(expr): +def generate_alternate_qtypes(expr): name = expr['union'] members = expr['data'] @@ -181,7 +181,7 @@ const int %(name)s_qtypes[QTYPE_MAX] = { name=name) for key in members: - qtype = find_anonymous_member_qtype(members[key]) + qtype = find_alternate_member_qtype(members[key]) assert qtype, "Invalid anonymous union member" ret += mcgen(''' @@ -408,7 +408,7 @@ for expr in exprs: fdef.write(generate_enum_lookup('%sKind' % expr['union'], expr['data'].keys())) if expr.get('discriminator') == {}: - fdef.write(generate_anon_union_qtypes(expr)) + fdef.write(generate_alternate_qtypes(expr)) else: continue fdecl.write(ret) diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py index 3f82bd4..77b0a1f 100644 --- a/scripts/qapi-visit.py +++ b/scripts/qapi-visit.py @@ -237,7 +237,7 @@ void visit_type_%(name)s(Visitor *m, %(name)s *obj, const char *name, Error **er ''', name=name) -def generate_visit_anon_union(name, members): +def generate_visit_alternate(name, members): ret = mcgen(''' void visit_type_%(name)s(Visitor *m, %(name)s **obj, const char *name, Error **errp) @@ -302,7 +302,7 @@ def generate_visit_union(expr): if discriminator == {}: assert not base - return generate_visit_anon_union(name, members) + return generate_visit_alternate(name, members) enum_define = discriminator_find_enum_define(expr) if enum_define: diff --git a/scripts/qapi.py b/scripts/qapi.py index 39cc88b..17252e9 100644 --- a/scripts/qapi.py +++ b/scripts/qapi.py @@ -224,21 +224,16 @@ 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): +# Return the qtype of an alternate branch, or None on error. +def find_alternate_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" + elif find_union(qapi_type): + return "QTYPE_QDICT" return None # Return the discriminator enum define if discriminator is specified as an @@ -276,22 +271,13 @@ def check_union(expr, expr_info): discriminator = expr.get('discriminator') members = expr['data'] values = { 'MAX': '(automatic)' } - types_seen = {} - # Three types of unions, determined by discriminator. + # Two types of unions, determined by discriminator. + assert discriminator != {} - # If the value of member 'discriminator' is {}, it's an - # anonymous union, and must not have a base. - if discriminator == {}: - enum_define = None - if base: - raise QAPIExprError(expr_info, - "Anonymous union '%s' must not have a base" - % name) - - # Else if the union object has no member 'discriminator', it's an + # If the union object has no member 'discriminator', it's an # ordinary union. For now, it must not have a base. - elif not discriminator: + if not discriminator: enum_define = None if base: raise QAPIExprError(expr_info, @@ -346,24 +332,46 @@ def check_union(expr, expr_info): % (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' has " - "same QObject type as member '%s'" - % (name, key, types_seen[qtype])) - types_seen[qtype] = key +def check_alternate(expr, expr_info): + name = expr['union'] + base = expr.get('base') + discriminator = expr.get('discriminator') + members = expr['data'] + values = { 'MAX': '(automatic)' } + types_seen = {} + assert discriminator == {} + if base: + raise QAPIExprError(expr_info, + "Anonymous union '%s' must not have a base" + % name) + + # Check every branch + for (key, value) in members.items(): + # Check for conflicts in the generated enum + 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 alternates have no type conflicts. + if isinstance(value, list): + raise QAPIExprError(expr_info, + "Anonymous union '%s' member '%s' must " + "not be array type" % (name, key)) + qtype = find_alternate_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' has " + "same QObject type as member '%s'" + % (name, key, types_seen[qtype])) + types_seen[qtype] = key def check_enum(expr, expr_info): name = expr['enum'] @@ -393,7 +401,10 @@ def check_exprs(schema): if expr.has_key('enum'): check_enum(expr, info) elif expr.has_key('union'): - check_union(expr, info) + if expr.get('discriminator') == {}: + check_alternate(expr, info) + else: + check_union(expr, info) elif expr.has_key('event'): check_event(expr, info) @@ -535,7 +546,8 @@ def find_struct(name): def add_union(definition): global union_types - union_types.append(definition) + if definition.get('discriminator') != {}: + union_types.append(definition) def find_union(name): global union_types
Special-casing 'discriminator == {}' for handling anonymous unions is getting awkward; since this particular type is not always a dictionary on the wire, it is easier to treat it as a completely different class of type, "alternate", so that if a type is listed in the union_types array, we know it is not an anonymous union. This patch just further segregates union handling, to make sure that anonymous unions are not stored in union_types, and splitting up check_union() into separate functions. A future patch will change the qapi grammar, and having the segregation already in place will make it easier to deal with the distinct meta-type. Signed-off-by: Eric Blake <eblake@redhat.com> --- scripts/qapi-types.py | 6 ++-- scripts/qapi-visit.py | 4 +-- scripts/qapi.py | 94 +++++++++++++++++++++++++++++---------------------- 3 files changed, 58 insertions(+), 46 deletions(-)