| Message ID | 1447224690-9743-21-git-send-email-eblake@redhat.com |
|---|---|
| State | New |
| Headers | show |
Eric Blake <eblake@redhat.com> writes: > 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(). With PATCH 19, they're mapped by c_name(N).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. Suggest to briefly mention the new test. > Signed-off-by: Eric Blake <eblake@redhat.com> Patch looks good.
On 11/11/2015 07:53 AM, Markus Armbruster wrote: > Eric Blake <eblake@redhat.com> writes: > >> 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(). > > With PATCH 19, they're mapped by c_name(N).upper(). > Yep, need to reword that, thanks to rebase churn. >> 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. > > Suggest to briefly mention the new test. Will do. > >> Signed-off-by: Eric Blake <eblake@redhat.com> > > Patch looks good. > Hmm - this only enforces member name case clashes. It would also be worth enforcing command/event/type collisions (technically, only command/event clashes are user-visible, but tracking all entities is probably easier) - probably by modifying QAPISchema._def_entity() and .lookup_entity() to do the same trick of keying the map by a munged name. I'll see about adding that as an additional patch.
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' } }
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 <eblake@redhat.com> --- 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/tests/qapi-schema/args-case-clash.out b/tests/qapi-schema/args-case-clash.out new file mode 100644 index 0000000..e69de29