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