Re: [Qemu-devel] [PATCH v13 12/14] qapi: Enforce (or whitelist) case conventions on qapi members

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> We document that members of enums and objects should be
> 'lower-case', although we were not enforcing it.  We have to
> whitelist a few pre-existing entities that violate the norms.
> Add three new tests to expose the new error message, each of
> which first uses the whitelisted name 'UuidInfo' to prove the
> whitelist works, then triggers the failure.

I guess a whitelist is the simplest solution that could possibly work.
Moreover, it matches the existing solution for allowing non-dictionary
returns.  Drawback: they apply to all schemata.  Good enough at least
for now.

> Note that by adding this check, we have effectively forbidden
> an entity with a case-insensitive clash of member names, for
> any entity that is not on the whitelist (although there is
> still the possibility to clash via '-' vs. '_').

Yes.

Still to be done: enforcing entity naming conventions.

> Signed-off-by: Eric Blake <address@hidden>
> ---
>  scripts/qapi.py                          | 19 +++++++++++++++++++
>  tests/Makefile                           |  3 +++
>  tests/qapi-schema/args-member-case.err   |  1 +
>  tests/qapi-schema/args-member-case.exit  |  1 +
>  tests/qapi-schema/args-member-case.json  |  3 +++
>  tests/qapi-schema/args-member-case.out   |  0
>  tests/qapi-schema/enum-member-case.err   |  1 +
>  tests/qapi-schema/enum-member-case.exit  |  1 +
>  tests/qapi-schema/enum-member-case.json  |  3 +++
>  tests/qapi-schema/enum-member-case.out   |  0
>  tests/qapi-schema/union-branch-case.err  |  1 +
>  tests/qapi-schema/union-branch-case.exit |  1 +
>  tests/qapi-schema/union-branch-case.json |  3 +++
>  tests/qapi-schema/union-branch-case.out  |  0
>  14 files changed, 37 insertions(+)
>  create mode 100644 tests/qapi-schema/args-member-case.err
>  create mode 100644 tests/qapi-schema/args-member-case.exit
>  create mode 100644 tests/qapi-schema/args-member-case.json
>  create mode 100644 tests/qapi-schema/args-member-case.out
>  create mode 100644 tests/qapi-schema/enum-member-case.err
>  create mode 100644 tests/qapi-schema/enum-member-case.exit
>  create mode 100644 tests/qapi-schema/enum-member-case.json
>  create mode 100644 tests/qapi-schema/enum-member-case.out
>  create mode 100644 tests/qapi-schema/union-branch-case.err
>  create mode 100644 tests/qapi-schema/union-branch-case.exit
>  create mode 100644 tests/qapi-schema/union-branch-case.json
>  create mode 100644 tests/qapi-schema/union-branch-case.out
>
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index ff3fccb..00eb43e 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -59,6 +59,21 @@ returns_whitelist = [
>      'guest-sync-delimited',
>  ]
>
> +# Whitelist of entities allowed to violate case conventions
> +case_whitelist = [

Double-checking for accuracy:

> +    # From QMP:
> +    'ACPISlotType',

Because of enum member DIMM.

Visible in event ACPI_DEVICE_OST and command query-acpi-ospm-status,
both since 2.1.

> +    'CpuInfo',

Because of CpuInfoBase.

Visible in command query-cpus since 0.14.

> +    'CpuInfoBase',

Because of struct member CPU.

Visible since v1.0.

> +    'CpuInfoMIPS',
> +    'CpuInfoTricore',

Because of struct member PC.

Visible since v1.0 and v2.2.

> +    'InputAxis',
> +    'InputButton',

Because of all enum members,

Visible in x-input-send-event since 2.0.  To be fixed when
x-input-send-event loses x-.  TODO comment there?

> +    'QapiErrorClass',

Because of all enum members.

Visible in error replies since forever.

> +    'UuidInfo',

Because of struct member UUID.

Visible in command query-uuid since 0.14.

> +    'X86CPURegister32',

Because of all enum members.

*Not* visible in QMP, thus fixable.  Fix or TODO comment, please.

> +]
> +
>  enum_types = []
>  struct_types = []
>  union_types = []
> @@ -1039,6 +1054,10 @@ class QAPISchemaMember(object):
>
>      def check_clash(self, info, seen):
>          cname = c_name(self.name)
> +        if cname.lower() != cname and info['name'] not in case_whitelist:
> +            raise QAPIExprError(info,
> +                                "Member '%s' of '%s' should use lowercase"
> +                                % (self.name, info['name']))
>          if cname in seen:
>              raise QAPIExprError(info,
>                                  "%s collides with %s"
> diff --git a/tests/Makefile b/tests/Makefile
> index e377c70..ca386e9 100644
> --- a/tests/Makefile
> +++ b/tests/Makefile
> @@ -246,6 +246,7 @@ qapi-schema += args-array-unknown.json
>  qapi-schema += args-int.json
>  qapi-schema += args-invalid.json
>  qapi-schema += args-member-array-bad.json
> +qapi-schema += args-member-case.json
>  qapi-schema += args-member-unknown.json
>  qapi-schema += args-name-clash.json
>  qapi-schema += args-union.json
> @@ -267,6 +268,7 @@ qapi-schema += enum-bad-prefix.json
>  qapi-schema += enum-clash-member.json
>  qapi-schema += enum-dict-member.json
>  qapi-schema += enum-int-member.json
> +qapi-schema += enum-member-case.json
>  qapi-schema += enum-missing-data.json
>  qapi-schema += enum-wrong-data.json
>  qapi-schema += escape-outside-string.json
> @@ -341,6 +343,7 @@ qapi-schema += unclosed-string.json
>  qapi-schema += unicode-str.json
>  qapi-schema += union-bad-branch.json
>  qapi-schema += union-base-no-discriminator.json
> +qapi-schema += union-branch-case.json
>  qapi-schema += union-clash-branches.json
>  qapi-schema += union-clash-data.json
>  qapi-schema += union-empty.json
> diff --git a/tests/qapi-schema/args-member-case.err 
> b/tests/qapi-schema/args-member-case.err
> new file mode 100644
> index 0000000..7bace48
> --- /dev/null
> +++ b/tests/qapi-schema/args-member-case.err
> @@ -0,0 +1 @@
> +tests/qapi-schema/args-member-case.json:3: Member 'Arg' of 'Foo' should use 
> lowercase
> diff --git a/tests/qapi-schema/args-member-case.exit 
> b/tests/qapi-schema/args-member-case.exit
> new file mode 100644
> index 0000000..d00491f
> --- /dev/null
> +++ b/tests/qapi-schema/args-member-case.exit
> @@ -0,0 +1 @@
> +1
> diff --git a/tests/qapi-schema/args-member-case.json 
> b/tests/qapi-schema/args-member-case.json
> new file mode 100644
> index 0000000..1bc823a
> --- /dev/null
> +++ b/tests/qapi-schema/args-member-case.json
> @@ -0,0 +1,3 @@
> +# Member names should be 'lower-case' unless the struct/command is 
> whitelisted
> +{ 'command': 'UuidInfo', 'data': { 'Arg': 'int' } }
> +{ 'command': 'Foo', 'data': { 'Arg': 'int' } }

We normally put positive tests in qapi-schema-test.json, but I think
keeping this one here makes more sense.

[More of the same...]