Re: [Qemu-devel] [PATCH v10 30/30] qapi: Forbid case-insensitive clashes

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> We have toyed on list with the idea of 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.
>
> Another benefit of enforcing a restriction against
> case-insensitive is that we no longer have to worry about the
> case 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_uppper().  Having the

camel_to_upper()

> 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.
>
> Of course, if we never implement a case-insensitive QMP
> extension, or find a legitimate reason to need qapi members
> that differ only by case, we could always relax this
> restriction.  But it is easier to relax later than it is to
> wish we had the restriction in place earlier.
>
> Signed-off-by: Eric Blake <address@hidden>

I think "make QMP case-insensitive" is a very weak justification for
anything.

Your second reason is much stronger: forbidding case-insensitive clashes
lets the generators shout without fear of introducing clashes in
generated code.

However, since camel_to_upper() does more than just shout, forbidding
case-insensitive clashes protects against clashes in generated code only
for names where camel_to_upper() just shouts.  It does for lower case
names, i.e. almost all of them.

The commit would make more sense if we first defanged c_enum_const():
make it mangle the const_name part like c_name(const_name).upper().
This is independent of the argument I'm having with Dan on how the
prefix should be mangled.

One more good argument for forbidding case-insensitive clashes: an
interface that sports names differing only in case is a bad interface.

> ---
> v10: new patch
> ---
>  docs/qapi-code-gen.txt                 | 5 +++++
>  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 | 5 +++++
>  tests/qapi-schema/args-case-clash.out  | 0
>  7 files changed, 15 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..13cec10 100644
> --- a/docs/qapi-code-gen.txt
> +++ b/docs/qapi-code-gen.txt
> @@ -116,6 +116,11 @@ names should be ALL_CAPS with words separated by 
> underscore.  Field
>  names cannot start with 'has-' or 'has_', as this is reserved for
>  tracking optional fields.
>
> +For now, Client JSON Protocol is case-sensitive, but future extensions
> +may allow for case-insensitive recognition of command and event names,
> +or of member field names.  As such, the generator rejects attempts to
> +create entities that only differ by case.
> +

This suggests we're actually planning to make QMP case-insensitive.
Let's avoid that.  Perhaps:

    Since we don't want interfaces with different names that differ only
    in case, the generator rejects such names.

>  Any name (command, event, type, field, or enum value) beginning with
>  "x-" is marked experimental, and may be withdrawn or changed
>  incompatibly in a future release.  Downstream vendors may add
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index 3a359cb..7e7ad6e 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -1039,7 +1039,7 @@ class QAPISchemaObjectTypeMember(object):
>          assert self.type
>
>      def check_clash(self, info, seen):
> -        name = c_name(self.name)
> +        name = c_name(self.name).upper()
>          if name in seen:
>              raise QAPIExprError(info,
>                                  "%s collides with %s"
> @@ -1087,7 +1087,7 @@ class QAPISchemaObjectTypeVariants(object):
>
>      def check(self, schema, seen):
>          if self.tag_name:    # 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
>          tag_type = self.tag_member.type
>          assert isinstance(tag_type, QAPISchemaEnumType)

If we ever acquire more lookups in seen, we'll need a lookup function,
so we don't duplicate c_name(NAME).upper().  But this will do for now.

> 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..5495ab8
> --- /dev/null
> +++ b/tests/qapi-schema/args-case-clash.err
> @@ -0,0 +1 @@
> +tests/qapi-schema/args-case-clash.json:5: 'A' (argument of oops) collides 
> with 'a' (argument 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..55ae488
> --- /dev/null
> +++ b/tests/qapi-schema/args-case-clash.json
> @@ -0,0 +1,5 @@
> +# C member name collision
> +# Reject members that clash case-insensitively (our mapping to C names
> +# preserves case, but allowing these members now would prevent a future
> +# relaxing of QMP to be case-insensitive).

Again, let's not suggest we're planning to make QMP case-insensitive.

> +{ '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