Eric Blake <ebl...@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 <ebl...@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 <arm...@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 <arm...@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
