From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56893) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zgu9G-0002d1-Ce for qemu-devel@nongnu.org; Tue, 29 Sep 2015 08:36:52 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Zgu9D-0001WD-5e for qemu-devel@nongnu.org; Tue, 29 Sep 2015 08:36:46 -0400 Received: from mx1.redhat.com ([209.132.183.28]:50571) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zgu9C-0001W3-VX for qemu-devel@nongnu.org; Tue, 29 Sep 2015 08:36:43 -0400 From: Markus Armbruster References: <1443497249-15361-1-git-send-email-eblake@redhat.com> <1443497249-15361-7-git-send-email-eblake@redhat.com> Date: Tue, 29 Sep 2015 14:36:38 +0200 In-Reply-To: <1443497249-15361-7-git-send-email-eblake@redhat.com> (Eric Blake's message of "Mon, 28 Sep 2015 21:27:19 -0600") Message-ID: <87612tl7vt.fsf@blackfin.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH v6 06/16] qapi: Avoid assertion failure on union 'type' collision List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: marcandre.lureau@redhat.com, qemu-devel@nongnu.org, ehabkost@redhat.com, Michael Roth Eric Blake writes: > The previous commit added two tests that triggered an assertion > failure. It's fairly straightforward to avoid the failure by > just outright forbidding the collision between a union's tag > values and its discriminator name (including the implicit name > 'kind' supplied for simple unions [*]). Ultimately, we'd like > to move the collision detection into QAPISchema*.check(), but > for now it is easier just to enhance the existing checks. > > [*] Of course, down the road, we have plans to rename the simple > union tag name to 'type' to match the QMP wire name, but the > idea of the collision will still be present even then. > > Technically, we could avoid the collision by naming the C union > members representing each enum value as '_case_value' rather > than 'value'; but until we have an actual qapi client (and not > just our testsuite) that has a legitimate reason to match a > case label to the name of a QMP key and needs the name munging > to satisfy the compiler, it's easier to just reject the qapi > as invalid. > > Signed-off-by: Eric Blake > > --- > v6: new patch > --- > scripts/qapi.py | 10 ++++++++-- > tests/qapi-schema/flat-union-clash-type.err | 17 +---------------- > tests/qapi-schema/union-clash-type.err | 17 +---------------- > 3 files changed, 10 insertions(+), 34 deletions(-) > > diff --git a/scripts/qapi.py b/scripts/qapi.py > index 7e8a99d..ec450a6 100644 > --- a/scripts/qapi.py > +++ b/scripts/qapi.py > @@ -544,7 +544,7 @@ def check_union(expr, expr_info): > base = expr.get('base') > discriminator = expr.get('discriminator') > members = expr['data'] > - values = {'MAX': '(automatic)'} > + values = {'MAX': '(automatic)', 'KIND': '(automatic)'} > > # Two types of unions, determined by discriminator. > > @@ -603,13 +603,19 @@ def check_union(expr, expr_info): > " of branch '%s'" % key) > > # If the discriminator names an enum type, then all members > - # of 'data' must also be members of the enum type. > + # of 'data' must also be members of the enum type, which in turn > + # must not collide with the discriminator name. > if enum_define: > if key not in enum_define['enum_values']: > raise QAPIExprError(expr_info, > "Discriminator value '%s' is not found in " > "enum '%s'" % > (key, enum_define["enum_name"])) > + if discriminator in enum_define['enum_values']: > + raise QAPIExprError(expr_info, > + "Discriminator name '%s' collides with " > + "enum value in '%s'" % > + (discriminator, enum_define["enum_name"])) > > # Otherwise, check for conflicts in the generated enum > else: > diff --git a/tests/qapi-schema/flat-union-clash-type.err b/tests/qapi-schema/flat-union-clash-type.err > index 6e64d1d..879beb8 100644 > --- a/tests/qapi-schema/flat-union-clash-type.err > +++ b/tests/qapi-schema/flat-union-clash-type.err > @@ -1,16 +1 @@ > -Traceback (most recent call last): > - File "tests/qapi-schema/test-qapi.py", line 55, in > - schema = QAPISchema(sys.argv[1]) > - File "scripts/qapi.py", line 1116, in __init__ > - self.check() > - File "scripts/qapi.py", line 1299, in check > - ent.check(self) > - File "scripts/qapi.py", line 962, in check > - self.variants.check(schema, members, seen) > - File "scripts/qapi.py", line 1024, in check > - v.check(schema, self.tag_member.type, vseen) > - File "scripts/qapi.py", line 1032, in check > - QAPISchemaObjectTypeMember.check(self, schema, [], seen) > - File "scripts/qapi.py", line 994, in check > - assert self.name not in seen > -AssertionError > +tests/qapi-schema/flat-union-clash-type.json:8: Discriminator name 'type' collides with enum value in 'TestEnum' > diff --git a/tests/qapi-schema/union-clash-type.err b/tests/qapi-schema/union-clash-type.err > index 6e64d1d..773900b 100644 > --- a/tests/qapi-schema/union-clash-type.err > +++ b/tests/qapi-schema/union-clash-type.err > @@ -1,16 +1 @@ > -Traceback (most recent call last): > - File "tests/qapi-schema/test-qapi.py", line 55, in > - schema = QAPISchema(sys.argv[1]) > - File "scripts/qapi.py", line 1116, in __init__ > - self.check() > - File "scripts/qapi.py", line 1299, in check > - ent.check(self) > - File "scripts/qapi.py", line 962, in check > - self.variants.check(schema, members, seen) > - File "scripts/qapi.py", line 1024, in check > - v.check(schema, self.tag_member.type, vseen) > - File "scripts/qapi.py", line 1032, in check > - QAPISchemaObjectTypeMember.check(self, schema, [], seen) > - File "scripts/qapi.py", line 994, in check > - assert self.name not in seen > -AssertionError > +tests/qapi-schema/union-clash-type.json:2: Union 'TestUnion' member 'kind' clashes with '(automatic)' Preexisting: this error message sucks. But improving it isn't this patch's job.