[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v13 12/14] qapi: Enforce (or whitelist) case con
From: |
Eric Blake |
Subject: |
Re: [Qemu-devel] [PATCH v13 12/14] qapi: Enforce (or whitelist) case conventions on qapi members |
Date: |
Tue, 1 Dec 2015 09:12:39 -0700 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0 |
On 11/27/2015 02:42 AM, Markus Armbruster wrote:
> 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.
>>
>> 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. '_').
>>
>> Signed-off-by: Eric Blake <address@hidden>
> [...]
>> @@ -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"
>
> As far as I can tell, this is the only use of info['name'] in this
> series.
Yes, although I may find more uses for it later.
>
> Can you give an example where info['name'] != self.owner?
Sure; this triggers lots of debug lines before crashing[1]:
diff --git i/scripts/qapi.py w/scripts/qapi.py
index 6a77db4..ec59682 100644
--- i/scripts/qapi.py
+++ w/scripts/qapi.py
@@ -1054,6 +1054,8 @@ class QAPISchemaMember(object):
def check_clash(self, info, seen):
cname = c_name(self.name)
+ if info['name'] != self.owner:
+ print ' ** checking differs in %s, owner is %s' %
(info['name'], self.owner)
if cname.lower() != cname and info['name'] not in case_whitelist:
raise QAPIExprError(info,
"Member '%s' of '%s' should use lowercase"
The very first one is:
** checking differs in block_passwd, owner is :obj-block_passwd-arg
Remember, QAPISchemaMember.owner is the innermost (possibly-implicit)
type that owns the member, while info['name'] is the name of the
top-level entity that encloses the member. So the two are not always
equal. member._pretty_owner() converts from an implicit struct name
back to the top-level entity, but not directly (it is a human-readable
phrase, not the plain entity name).
Furthermore, look at CpuInfo's member 'CPU': there, we have two call
paths (one with info['name'] == 'CpuInfo', the other with it as
'CpuInfoBase') but both call paths would see only self.owner ==
'CpuInfoBase'. The whitelist covers both struct names. Perhaps
whitelisting only 'self.owner' names would be sufficient; but then the
whitelist would have to use implicit type names rather than entity names
from the .json file.
[1] The crash is "TypeError: 'NoneType' object has no attribute
'__getitem__'" at the point where QType is being tested. Normally,
QType is well-formed, so even though it is a builtin type and therefore
has info == None, the 'cname.lower() != cname' test never fails and we
short-circuit past an attempt to dereference None; but not so with my
temporary print hack.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
- Re: [Qemu-devel] [PATCH v13 12/14] qapi: Enforce (or whitelist) case conventions on qapi members,
Eric Blake <=