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

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> 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"

Hmm.

    tests/qapi-schema/args-member-case.json:3: Member 'Arg' of 'Foo' should use 
lowercase

'Foo' *does* use lowercase: 'o'.  Easiest fix is "should not use
uppercase".

>>> +                                % (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)

Crash is easy to avoid: if info and 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"
                                   % (self.name, info['name']))
           if cname in seen:
               raise QAPIExprError(info,
                                   "%s collides with %s"
                                   % (self.describe(), seen[cname].describe()))

Two separate uses of info['name']:

a. Key for the whitelist

b. Error message

> 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.

With the crash avoided, I get 191 distinct lines.  I can sort them into
just give buckets:

1. object vs. base

   Example: in CpuInfo, owner is CpuInfoBase

   self._pretty_owner() returns "(member of CpuInfoBase).  Again, this
   would do for error message use.  For instance,

       { 'struct': 'Foo', 'data': { 'Memb': 'int' } }

   would produce

       foo.json:1: 'Memb' (member of Foo) should use lowercase

   That leaves the whitelist key use.  Using .owner as key would work,
   as you already explained.  The whitelist becomes slightly more
   cumbersome, but also slightly tighter.

2. command / event FOO vs. :obj-FOO-arg

   Example: in block_passwd, owner is :obj-block_passwd-arg

   self._pretty_owner() returns '(parameter of FOO)'.  This would do for
   error message use, just like it does for the "collides with error
   right below.

       "%s should use lowercase" % self.describe()

   yields something like

       tests/qapi-schema/args-member-case.json:3: 'Arg' (parameter of Foo) 
should use lowercase

   Again, .owner works as whitelist key.  This is a case where we'd have
   to use implicit type names.  However, we don't actually have an
   offender to cover.

3. flat union vs. branch

   Example: in BlockdevOptions, owner is BlockdevOptionsArchipelago

   self._pretty_owner() returns '(member of
   BlockdevOptionsArchipelago)'.  Error message using that is again
   satisfactory:

       tests/qapi-schema/union-branch-case.json:3: 'Branch' (branch of Foo) 
should use lowercase

   Again, .owner works as whitelist key.  Saves us whitelist entry
   'CpuInfo', as you observed.

4. simple union vs. implicit tag enum

   Example: in ChardevBackend, owner is ChardevBackendKind

   self._pretty_owner() returns '(branch of ChardevBackend)'.  Again,
   this would do for error message use.  For instance,

       { 'union': 'Foo', 'data': { 'Branch': 'int' } }

   would produce

       foo.json:1: 'Branch' (branch of Foo) should use lowercase

   Again, .owner would work as whitelist key, if we had offenders to
   whitelist.

5. simple union FOO vs. member wrapper :obj-BAR-wrapper

   Example: in ChardevBackend, owner is :obj-ChardevDummy-wrapper

   self._pretty_owner() returns '(branch of BAR)'.  This is actually
   *wrong*: it's a branch of FOO, not a branch of BAR!  We haven't
   noticed until now, because we don't actually reach this code.  We can
   reach it only via .describe(), the only use of .describe() is
   .check_clash(), and we check a simple union's implicit enum before
   its members.  Thus, a simple union member clash will always be found
   and reported for the implicit enum, where the error message is
   correct, and not for the member list, where it would be wrong.

   The obvious knee jerk fix is to replace the incorrect code by an
   assertion with a suitable comment.

   Once we can actually reach it, we can consider *how* we reach it, and
   what the appropriate description for a simple union's wrapped members
   may be.  There's hope it stays unreachable: implicit stuff like this
   should not produce errors.

   Bottom line for now: this case does not matter for the problem at
   hand, namely enforcing "member names are in lower case".

> [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.

As an experiment, I used self.owner and dropped "qapi: Populate
info['name'] for each entity".  I'll post the result as a reply to your
v14.