Re: [Qemu-devel] [PATCH v14 13/15] qapi: Enforce (or whitelist) case conventions on qapi members

[Markus Armbruster <armbru@redhat.com>]

Eric Blake <eblake@redhat.com> writes:

> On 12/02/2015 06:41 AM, Eric Blake wrote:
>> On 12/02/2015 04:51 AM, Markus Armbruster wrote:
>>> This is the fixup I mentioned in the v13 thread.  The "Unreachable and
>>> not implemented" hunk should probably be its own patch.
>> 
>> In fact, that hunk...
>> 
>>>
>>> diff --git a/scripts/qapi.py b/scripts/qapi.py
>>> index 6d38d7c..870e476 100644
>>> --- a/scripts/qapi.py
>>> +++ b/scripts/qapi.py
>> 
>>> @@ -1073,7 +1071,8 @@ class QAPISchemaMember(object):
>>>                  return '(parameter of %s)' % owner[:-4]
>>>              else:
>>>                  assert owner.endswith('-wrapper')
>>> -                return '(branch of %s)' % owner[:-8]
>>> +                # Unreachable and not implemented
>>> +                assert False
>>>          if owner.endswith('Kind'):
>>>              # See QAPISchema._make_implicit_enum_type()
>>>              return '(branch of %s)' % owner[:-4]
>> 
>> ...should probably just be squashed directly into commit 8f3a05b on your
>> current qapi-next branch, since it hasn't landed upstream yet.
>> 
>> Your fixup looks sane, and eliminates the need for 12/15.  So I'm fine
>> if you'd like to make that change when updating qapi-next.
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>
> Scratch that.
>
> With your patch, the positive tests no longer work in isolation.  You
> were getting lucky that things sorted such that 'Foo' was checked for
> correctness prior to 'UuidInfo'; but if you comment out the 'Foo'
> declaration, or rename from 'Foo' to something else that hashes after
> 'UuidInfo', then args-member-case and union-branch-case start reporting
> failures about UuidInfo (and only enum-member-case honors the
> whitelist).

You're right.

>              That's because your change to qapi.py would require the
> whitelist to contain ':obj-UuidInfo-args' and 'UuidInfoKind',
> respectively (with my approach of info['name'], the whitelist containing
> 'UuidInfo' was sufficient).
>
> Maybe we need to modify qapi.py as follows:
>
> diff --git i/scripts/qapi.py w/scripts/qapi.py
> index 04c4c8d..1325da1 100644
> --- i/scripts/qapi.py
> +++ w/scripts/qapi.py
> @@ -71,6 +71,10 @@ case_whitelist = [
>      'QapiErrorClass',       # all members, visible through errors
>      'UuidInfo',             # UUID, visible through query-uuid
>      'X86CPURegister32',     # all members, visible indirectly through
> qom-get
> +
> +    # For use in the testsuite
> +    ':obj-x-UuidInfo-arg',  # args-member-case
> +    'x-UuidInfoList',       # union-branch-case

Corrected to 'x-UuidInfoKind' in your actual fixup patch.

>  ]
>
>  enum_types = []
>
> index 1bc823a..193eb66 100644
> --- i/tests/qapi-schema/args-member-case.json
> +++ w/tests/qapi-schema/args-member-case.json
> @@ -1,3 +1,3 @@
>  # Member names should be 'lower-case' unless the struct/command is
> whitelisted
> -{ 'command': 'UuidInfo', 'data': { 'Arg': 'int' } }
> +{ 'command': 'x-UuidInfo', 'data': { 'Arg': 'int' } }

Will fail as soon as we enforce the command naming convention.  To avoid
that, we need to add a suitable name to the whitelist.

However, we don't actually have any command parameters to whitelist.
Why bother testing it then?

>  { 'command': 'Foo', 'data': { 'Arg': 'int' } }
> index a5951f1..4f0988a 100644
> --- i/tests/qapi-schema/union-branch-case.json
> +++ w/tests/qapi-schema/union-branch-case.json
> @@ -1,3 +1,3 @@
>  # Branch names should be 'lower-case' unless the union is whitelisted
> -{ 'union': 'UuidInfo', 'data': { 'Branch': 'int' } }
> +{ 'union': 'x-UuidInfo', 'data': { 'Branch': 'int' } }
>  { 'union': 'Foo', 'data': { 'Branch': 'int' } }

Likewise, we don't actually have any simple union branches to whitelist.
Why test?

Appending my current fixup.


>From 44f07a40c8b9b5d1f24833028b5dacde1fd50c80 Mon Sep 17 00:00:00 2001
From: Markus Armbruster <armbru@redhat.com>
Date: Wed, 2 Dec 2015 17:41:55 +0100
Subject: [PATCH] fixup! qapi: Enforce (or whitelist) case conventions on qapi
 members

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 scripts/qapi.py                          | 6 ++----
 tests/qapi-schema/args-member-case.err   | 2 +-
 tests/qapi-schema/args-member-case.json  | 3 +--
 tests/qapi-schema/enum-member-case.err   | 2 +-
 tests/qapi-schema/enum-member-case.json  | 4 ++--
 tests/qapi-schema/union-branch-case.err  | 2 +-
 tests/qapi-schema/union-branch-case.json | 3 +--
 7 files changed, 9 insertions(+), 13 deletions(-)

diff --git a/scripts/qapi.py b/scripts/qapi.py
index 0aed6d4..74a561a 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -63,7 +63,6 @@ returns_whitelist = [
 case_whitelist = [
     # From QMP:
     'ACPISlotType',         # DIMM, visible through query-acpi-ospm-status
-    'CpuInfo',              # CPU, PC, visible through query-cpu
     'CpuInfoBase',          # CPU, visible through query-cpu
     'CpuInfoMIPS',          # PC, visible through query-cpu
     'CpuInfoTricore',       # PC, visible through query-cpu
@@ -1053,10 +1052,9 @@ 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:
+        if cname.lower() != cname and self.owner not in case_whitelist:
             raise QAPIExprError(info,
-                                "Member '%s' of '%s' should use lowercase"
-                                % (self.name, info['name']))
+                                "%s should not use uppercase" % self.describe())
         if cname in seen:
             raise QAPIExprError(info,
                                 "%s collides with %s"
diff --git a/tests/qapi-schema/args-member-case.err b/tests/qapi-schema/args-member-case.err
index 7bace48..19c4426 100644
--- a/tests/qapi-schema/args-member-case.err
+++ b/tests/qapi-schema/args-member-case.err
@@ -1 +1 @@
-tests/qapi-schema/args-member-case.json:3: Member 'Arg' of 'Foo' should use lowercase
+tests/qapi-schema/args-member-case.json:2: 'Arg' (parameter of no-way-this-will-get-whitelisted) should not use uppercase
diff --git a/tests/qapi-schema/args-member-case.json b/tests/qapi-schema/args-member-case.json
index 1bc823a..93439be 100644
--- a/tests/qapi-schema/args-member-case.json
+++ b/tests/qapi-schema/args-member-case.json
@@ -1,3 +1,2 @@
 # Member names should be 'lower-case' unless the struct/command is whitelisted
-{ 'command': 'UuidInfo', 'data': { 'Arg': 'int' } }
-{ 'command': 'Foo', 'data': { 'Arg': 'int' } }
+{ 'command': 'no-way-this-will-get-whitelisted', 'data': { 'Arg': 'int' } }
diff --git a/tests/qapi-schema/enum-member-case.err b/tests/qapi-schema/enum-member-case.err
index e50b12a..b652e9a 100644
--- a/tests/qapi-schema/enum-member-case.err
+++ b/tests/qapi-schema/enum-member-case.err
@@ -1 +1 @@
-tests/qapi-schema/enum-member-case.json:3: Member 'Value' of 'Foo' should use lowercase
+tests/qapi-schema/enum-member-case.json:3: 'Value' (member of NoWayThisWillGetWhitelisted) should not use uppercase
diff --git a/tests/qapi-schema/enum-member-case.json b/tests/qapi-schema/enum-member-case.json
index 5101275..2096b35 100644
--- a/tests/qapi-schema/enum-member-case.json
+++ b/tests/qapi-schema/enum-member-case.json
@@ -1,3 +1,3 @@
 # Member names should be 'lower-case' unless the enum is whitelisted
-{ 'enum': 'UuidInfo', 'data': [ 'Value' ] }
-{ 'enum': 'Foo', 'data': [ 'Value' ] }
+{ 'enum': 'UuidInfo', 'data': [ 'Value' ] } # UuidInfo is whitelisted
+{ 'enum': 'NoWayThisWillGetWhitelisted', 'data': [ 'Value' ] }
diff --git a/tests/qapi-schema/union-branch-case.err b/tests/qapi-schema/union-branch-case.err
index 6c6b740..1152190 100644
--- a/tests/qapi-schema/union-branch-case.err
+++ b/tests/qapi-schema/union-branch-case.err
@@ -1 +1 @@
-tests/qapi-schema/union-branch-case.json:3: Member 'Branch' of 'Foo' should use lowercase
+tests/qapi-schema/union-branch-case.json:2: 'Branch' (branch of NoWayThisWillGetWhitelisted) should not use uppercase
diff --git a/tests/qapi-schema/union-branch-case.json b/tests/qapi-schema/union-branch-case.json
index a5951f1..e6565dc 100644
--- a/tests/qapi-schema/union-branch-case.json
+++ b/tests/qapi-schema/union-branch-case.json
@@ -1,3 +1,2 @@
 # Branch names should be 'lower-case' unless the union is whitelisted
-{ 'union': 'UuidInfo', 'data': { 'Branch': 'int' } }
-{ 'union': 'Foo', 'data': { 'Branch': 'int' } }
+{ 'union': 'NoWayThisWillGetWhitelisted', 'data': { 'Branch': 'int' } }
-- 
2.4.3