Re: [Qemu-devel] [PATCH v13 12/14] qapi: Enforce (or whitelist) case conventions on qapi members

[Eric Blake]

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