From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41490) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZwPGB-0006VU-DC for qemu-devel@nongnu.org; Wed, 11 Nov 2015 01:52:19 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZwPFw-0005eb-SE for qemu-devel@nongnu.org; Wed, 11 Nov 2015 01:51:59 -0500 Received: from mx1.redhat.com ([209.132.183.28]:53484) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZwPFw-0005eH-Kc for qemu-devel@nongnu.org; Wed, 11 Nov 2015 01:51:44 -0500 From: Eric Blake Date: Tue, 10 Nov 2015 23:51:22 -0700 Message-Id: <1447224690-9743-21-git-send-email-eblake@redhat.com> In-Reply-To: <1447224690-9743-1-git-send-email-eblake@redhat.com> References: <1447224690-9743-1-git-send-email-eblake@redhat.com> Subject: [Qemu-devel] [PATCH v11 20/28] qapi: Forbid case-insensitive clashes List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: qemu-devel@nongnu.org Cc: armbru@redhat.com, Michael Roth In general, designing user interfaces that rely on case distinction is poor practice. Another benefit of enforcing a restriction against case-insensitive clashes is that we no longer have to worry about the situation of enum values that could be distinguished by case if mapped by c_name(), but which cannot be distinguished when mapped to C as ALL_CAPS by camel_to_upper(). Thus, having the generator look for case collisions up front will prevent developers from worrying whether different munging rules for member names compared to enum values as a discriminator will cause any problems in qapi unions. There is also the possibility that we may want to add a future extension to QMP of teaching it to be case-insensitive (the user could request command 'Quit' instead of 'quit', or could spell a struct field as 'CPU' instead of 'cpu'). But for that to be a practical extension, we cannot break backwards compatibility with any existing struct that was already relying on case sensitivity. Fortunately, after the previous patch cleaned up CpuInfo, there are no such existing qapi structs. Of course, the idea of a future extension is not as strong of a reason to make the change. At any rate, it is easier to be strict now, and relax things later if we find a reason to need case-sensitive QMP members, than it would be to wish we had the restriction in place. Signed-off-by: Eric Blake --- v11: rebase to latest, don't focus so hard on future case-insensitive extensions, adjust commit message v10: new patch --- docs/qapi-code-gen.txt | 3 +++ scripts/qapi.py | 4 ++-- tests/Makefile | 1 + tests/qapi-schema/args-case-clash.err | 1 + tests/qapi-schema/args-case-clash.exit | 1 + tests/qapi-schema/args-case-clash.json | 4 ++++ tests/qapi-schema/args-case-clash.out | 0 7 files changed, 12 insertions(+), 2 deletions(-) create mode 100644 tests/qapi-schema/args-case-clash.err create mode 100644 tests/qapi-schema/args-case-clash.exit create mode 100644 tests/qapi-schema/args-case-clash.json create mode 100644 tests/qapi-schema/args-case-clash.out diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt index f9fa6f3..54a6a7b 100644 --- a/docs/qapi-code-gen.txt +++ b/docs/qapi-code-gen.txt @@ -102,6 +102,9 @@ single-dimension array of that type; multi-dimension arrays are not directly supported (although an array of a complex struct that contains an array member is possible). +Client JSON Protocol is case-sensitive. However, the generator +rejects attempts to create entities that differ only in case. + Types, commands, and events share a common namespace. Therefore, generally speaking, type definitions should always use CamelCase for user-defined type names, while built-in types are lowercase. Type diff --git a/scripts/qapi.py b/scripts/qapi.py index d0f2308..ed7a32b 100644 --- a/scripts/qapi.py +++ b/scripts/qapi.py @@ -1040,7 +1040,7 @@ class QAPISchemaObjectTypeMember(object): assert self.type def check_clash(self, info, seen): - cname = c_name(self.name) + cname = c_name(self.name).upper() if cname in seen: raise QAPIExprError(info, "%s collides with %s" @@ -1085,7 +1085,7 @@ class QAPISchemaObjectTypeVariants(object): def check(self, schema, seen): if not self.tag_member: # flat union - self.tag_member = seen[c_name(self.tag_name)] + self.tag_member = seen[c_name(self.tag_name).upper()] assert self.tag_name == self.tag_member.name assert isinstance(self.tag_member.type, QAPISchemaEnumType) for v in self.variants: diff --git a/tests/Makefile b/tests/Makefile index c84c6cb..d1c6817 100644 --- a/tests/Makefile +++ b/tests/Makefile @@ -239,6 +239,7 @@ qapi-schema += args-alternate.json qapi-schema += args-any.json qapi-schema += args-array-empty.json qapi-schema += args-array-unknown.json +qapi-schema += args-case-clash.json qapi-schema += args-int.json qapi-schema += args-invalid.json qapi-schema += args-member-array-bad.json diff --git a/tests/qapi-schema/args-case-clash.err b/tests/qapi-schema/args-case-clash.err new file mode 100644 index 0000000..0fafe75 --- /dev/null +++ b/tests/qapi-schema/args-case-clash.err @@ -0,0 +1 @@ +tests/qapi-schema/args-case-clash.json:4: 'A' (parameter of oops) collides with 'a' (parameter of oops) diff --git a/tests/qapi-schema/args-case-clash.exit b/tests/qapi-schema/args-case-clash.exit new file mode 100644 index 0000000..d00491f --- /dev/null +++ b/tests/qapi-schema/args-case-clash.exit @@ -0,0 +1 @@ +1 diff --git a/tests/qapi-schema/args-case-clash.json b/tests/qapi-schema/args-case-clash.json new file mode 100644 index 0000000..e6f0625 --- /dev/null +++ b/tests/qapi-schema/args-case-clash.json @@ -0,0 +1,4 @@ +# C member name collision +# Reject members that clash case-insensitively, even though our mapping to +# C names preserves case. +{ 'command': 'oops', 'data': { 'a': 'str', 'A': 'int' } } diff --git a/tests/qapi-schema/args-case-clash.out b/tests/qapi-schema/args-case-clash.out new file mode 100644 index 0000000..e69de29 -- 2.4.3