Re: [Qemu-devel] [PATCH] fixup! qapi: Forbid case-insensitive clashes

[Eric Blake]

On 11/19/2015 06:32 AM, Markus Armbruster wrote:
> Eric Blake <address@hidden> writes:
> 
>> [Replace the old commit message with this:]
>>
>> qapi: Forbid case-insensitive clashes
>>

>> However, we DO have to care about the fact that we have a
>> command 'stop' and an event 'STOP' (currently the only case
>> collision of any of our .json entities).  To solve that, add
>> a new QAPISchemaEntity.c_namecase() that returns a munged
>> version of the name that can be used in checking for case
>> clashes,
> 
> For [-._] clashes, too.  Easiest fix it to just say "checking for
> clashes".
> 
>>          and override it in QAPISchemaEvent to use a different
>> case (thus, indexing by .c_namecase() will create two natural
>> key partitions), then add a new _clash_dict to
>> QAPISchema._def_entity() that checks for case collisions after
> 
> To QAPISchema, actually.  In _def_entity(), we only add an entry.

Easy fixes.

> 
>> ruling out identical spellings.  This works because all
>> entities have at least a leading letter in their name.
>>
>> We could go further and enforce that events are named with
>> 'ALL_CAPS' and that nothing else is named in that manner; but
>> that can be done as a followup if desired.  We could also
>> separate commands from type names (the way we already separated
>> events into a separate namespace), but then we'd have to make
>> .c_namecase() pick something other than the convenient upper()
>> vs. lower() in order to partition dictionary keys into three
>> sets (perhaps a leading digit, since all entities start with a
>> letter).
> 
> The upper / lower partition works for events / anything else, and is
> kind of cute, although two separate clash dictionaries might be clearer.
> I'd like to sketch that next, to help us decide.

Okay, sounds like I should post a v12.5 with the alternative of 2
separate clash dictionaries (3 dictionaries total), and as a complete
patch rather than a fixup.


> | @@ -800,6 +800,9 @@ class QAPISchemaEntity(object):
> |      def c_name(self):
> |          return c_name(self.name)
> |  
> | +    def c_namecase(self):
> | +        return c_name(self.name).lower()
> | +
> |      def check(self, schema):
> |          pass
> |  
> 
> The entities are: commands, events, the various types.  This
> implementation of c_namecase() covers commands and types.  It gets
> overwritten for events.
> 
> Commands should not use upper case.  If we enforced that, the .lower()
> would be superfluous there.  But it isn't wrong.

I won't need c_namecase() with separate dictionaries.  Once we have the
two approaches to compare, we can decide which one looks nicer.

> 
> | @@ -1040,7 +1043,7 @@ class QAPISchemaObjectTypeMember(object):
> |          assert self.type
> |  
> |      def check_clash(self, info, seen):
> | -        cname = c_name(self.name)
> | +        cname = c_name(self.name).lower()
> |          if cname in seen:
> |              raise QAPIExprError(info,
> |                                  "%s collides with %s"
> 
> Not an entity, therefore QAPISchemaEntity.c_namecase() isn't inherited.
> 
> Members should not use upper case.  If we enforced that, the .lower()
> would be superfluous.

Even if we enforce it, we have to whitelist exceptions for backward
compatibility; those exceptions would have to use .lower(), or we can
just skip collision detection on those whitelisted types (where the
existence of the whitelist reminds us to maintain no case collisions by
hand).


> | @@ -1184,12 +1187,16 @@ class QAPISchemaEvent(QAPISchemaEntity):
> |      def visit(self, visitor):
> |          visitor.visit_event(self.name, self.info, self.arg_type)
> |  
> | +    def c_namecase(self):
> | +        return c_name(self.name).upper()
> | +
> 
> Event names should not use lower case.  If we enforced that, the
> .upper() would be superfluous.

Looks like we could enforce this one without a whitelist.

> 
> If we enforced our naming conventions, namely types in CamelCase, events
> in ALL_CAPS, everything else lower-case, the c_namecase() would become
> slightly simpler: it's same as c_name() except for types, where it
> additionally folds to lower case.  The name would be misleading then,
> however.  Anyway, we can consider that once we enforce naming
> conventions.

And enforcing conventions certainly won't happen any sooner than the
rest of the pending queue is flushed.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature