From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55277) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Yb8eC-0004tm-Bp for qemu-devel@nongnu.org; Thu, 26 Mar 2015 10:20:38 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Yb8e8-0004Rg-VN for qemu-devel@nongnu.org; Thu, 26 Mar 2015 10:20:36 -0400 Received: from mx1.redhat.com ([209.132.183.28]:41490) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Yb8e8-0004RI-Kx for qemu-devel@nongnu.org; Thu, 26 Mar 2015 10:20:32 -0400 From: Markus Armbruster References: <1427227433-5030-1-git-send-email-eblake@redhat.com> <1427227433-5030-9-git-send-email-eblake@redhat.com> Date: Thu, 26 Mar 2015 15:20:19 +0100 In-Reply-To: <1427227433-5030-9-git-send-email-eblake@redhat.com> (Eric Blake's message of "Tue, 24 Mar 2015 14:03:33 -0600") Message-ID: <87sicr263g.fsf@blackfin.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH v5 08/28] qapi: Better error messages for bad unions List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: kwolf@redhat.com, lcapitulino@redhat.com, famz@redhat.com, qemu-devel@nongnu.org, wenchaoqemu@gmail.com Subject suggests you're just polishing error messages here. In fact, you're fixing the generator to detect errors. Suggest something like qapi: Tighten checking of unions Eric Blake writes: > Previous commits demonstrated that the generator had several > flaws with less-than-perfect unions: > - make the use of a base without discriminator a hard error, > since the previous patch removed all remaining uses of it > - 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 > - checking 'if discriminator' prior to 'if discriminator == {}' > leads to dead code in python, and ended up processing anonymous > unions as if they were simple unions > - 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 > --- [...] > 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 3ce8c33..fc7b7f1 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,24 +275,28 @@ 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. > - if base: > - base_fields = find_base_fields(base) > - if not base_fields: > - raise QAPIExprError(expr_info, > - "Base '%s' is not a valid type" > - % base) > + # Three types of unions, determined by discriminator. > > - # If the union object has no member 'discriminator', it's an > - # ordinary union. > - if not 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 value of member 'discriminator' is {}, it's an > - # anonymous union. > - elif discriminator == {}: > + # Else if the union object has no member 'discriminator', it's an > + # ordinary union. For now, it must not have a base. Since you're touching this anyway, you could s/ordinary/simple/. > + elif not discriminator: > enum_define = None > + if base: > + raise QAPIExprError(expr_info, > + "Simple union '%s' must not have a base" > + % name) > > # Else, it's a flat union. > else: > @@ -284,6 +305,12 @@ def check_union(expr, expr_info): > raise QAPIExprError(expr_info, > "Flat union '%s' must have a 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. > discriminator_type = base_fields.get(discriminator) > @@ -301,15 +328,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' 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'] [...] > diff --git a/tests/qapi-schema/alternate-conflict-string.err b/tests/qapi-schema/alternate-conflict-string.err > index e69de29..a3b7b6d 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' has same QObject type as member 'one' Explains the problem in terms of the implementation. That's okay for a development tool, but what about "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 5fd1a47..a1b0bea 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/union-base-no-discriminator.err b/tests/qapi-schema/union-base-no-discriminator.err > index e69de29..f33392d 100644 > --- a/tests/qapi-schema/union-base-no-discriminator.err > +++ b/tests/qapi-schema/union-base-no-discriminator.err > @@ -0,0 +1 @@ > +tests/qapi-schema/union-base-no-discriminator.json:12: Simple union 'TestUnion' must not have a base > diff --git a/tests/qapi-schema/union-base-no-discriminator.exit b/tests/qapi-schema/union-base-no-discriminator.exit > index 573541a..d00491f 100644 > --- a/tests/qapi-schema/union-base-no-discriminator.exit > +++ b/tests/qapi-schema/union-base-no-discriminator.exit > @@ -1 +1 @@ > -0 > +1 > diff --git a/tests/qapi-schema/union-base-no-discriminator.json b/tests/qapi-schema/union-base-no-discriminator.json > index b5da546..548a633 100644 > --- a/tests/qapi-schema/union-base-no-discriminator.json > +++ b/tests/qapi-schema/union-base-no-discriminator.json > @@ -1,4 +1,5 @@ > -# FIXME: either allow base in non-flat unions, or diagnose missing discriminator > +# for now, we reject a base for non-flat unions > +# FIXME: might be a useful extension to allow later This isn't a FIXME, it's an idea :) > { 'type': 'TestTypeA', > 'data': { 'string': 'str' } } > > diff --git a/tests/qapi-schema/union-base-no-discriminator.out b/tests/qapi-schema/union-base-no-discriminator.out > index 505fd57..e69de29 100644 > --- a/tests/qapi-schema/union-base-no-discriminator.out > +++ b/tests/qapi-schema/union-base-no-discriminator.out > @@ -1,8 +0,0 @@ > -[OrderedDict([('type', 'TestTypeA'), ('data', OrderedDict([('string', 'str')]))]), > - OrderedDict([('type', 'TestTypeB'), ('data', OrderedDict([('integer', 'int')]))]), > - OrderedDict([('type', 'Base'), ('data', OrderedDict([('string', 'str')]))]), > - OrderedDict([('union', 'TestUnion'), ('base', 'Base'), ('data', OrderedDict([('value1', 'TestTypeA'), ('value2', 'TestTypeB')]))])] > -[{'enum_name': 'TestUnionKind', 'enum_values': None}] > -[OrderedDict([('type', 'TestTypeA'), ('data', OrderedDict([('string', 'str')]))]), > - OrderedDict([('type', 'TestTypeB'), ('data', OrderedDict([('integer', 'int')]))]), > - OrderedDict([('type', 'Base'), ('data', OrderedDict([('string', 'str')]))])] > 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)' Cryptic error message, but it'll do. > 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}] > -[] Feel free to address my comments on top. Reviewed-by: Markus Armbruster