Eric Blake <ebl...@redhat.com> 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 <ebl...@redhat.com>
> ---
> 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...]
