Re: [Qemu-devel] [PATCH v12 27/36] qapi: Forbid case-insensitive clashes

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 5140 bytes --]

On 11/18/2015 10:08 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
>> In general, designing user interfaces that rely on case
>> distinction is poor practice.  Another benefit of enforcing
>> a restriction against case-insensitive clashes is that we
>> no longer have to worry about the situation of enum values
>> that could be distinguished by case if mapped by c_name(),
>> but which cannot be distinguished when mapped to C as
>> ALL_CAPS by c_enum_const() [via c_name(name, False).upper()].
>> Thus, having the generator look for case collisions up front
>> will prevent developers from worrying whether different
>> munging rules for member names compared to enum values as a
>> discriminator will cause any problems in qapi unions.
>>

>> @@ -378,9 +379,9 @@ def check_name(expr_info, source, name, allow_optional=False,
>>      # code always prefixes it with the enum name
>>      if enum_member and membername[0].isdigit():
>>          membername = 'D' + membername
>> -    # Reserve the entire 'q_' namespace for c_name()
>> +    # Reserve the entire 'q_'/'Q_' namespace for c_name()
>>      if not valid_name.match(membername) or \
>> -       c_name(membername, False).startswith('q_'):
>> +       c_name(membername, False).upper().startswith('Q_'):
>>          raise QAPIExprError(expr_info,
>>                              "%s uses invalid name '%s'" % (source, name))

I'll switch to lower() here, for consistency.

> Now let me try claw back some clarity.
> 
> First try: refine your approach.
> 
> Observe that all but one caller of lookup_entity() actually look up
> types.  The one caller that doesn't is _def_entity().  Change it to:
> 
>         cname = c_name(ent.name)
>         if isinstance(ent, QAPISchemaEvent):
>             cname = cname.upper()
>         else:
>             cname = cname.lower()
>         if self._lookup_entity(cname)

I think lookup_entity would need two parameters: the munged name, and
the actual name...

>             raise QAPIExprError(ent.info,
>                                 "Entity '%s' already defined" % end.name)
>         if cname in self._entity_dict:
>             raise QAPIExprError(ent.info,
>                                 "Entity '%s' collides with '%s'"
>                                 % (ent.name, self._entity_dict[cname].name))
> 
> where _lookup_entity() is back to the much simpler version:
> 
>         ent = self._entity_dict.get(cname)
>         if ent and ent.name != cname

...because ent.name does not have to match cname.

>             ent = None
> 
> lookup_type() becomes:
> 
>     def lookup_type(self, name, typ=QAPISchemaType):
>         return self._lookup_entity(name, typ)

I don't know that lookup_type needs an optional parameter.

> 
> and the third caller _make_implicit_object_type() goes from
> 
>         if not self.lookup_entity(name, QAPISchemaObjectType):
> 
> to
> 
>         if not self.lookup_type(name, QAPISchemaObjectType):

And a plain lookup_type(name) is what we should have been using here all
along.

> 
> Another possible improvement is hiding the case-folding logic in
> methods.  Have a QAPISchemaEntity.c_namecase() that returns
> self.c_name().lower().  Overwrite it in QAPISchemaEvent to return
> .upper() instead.  Use it to make _def_entity() less ugly.
> 
> Probably not worthwhile unless more uses of .c_namecase() pop up.

I kind of like it. I think I'll propose a 26.5 that implements that,
then redo 27 on top of it (without reposting the entire series).

> 
> Second approach: don't use _entity_dict for clash detection!  Have a
> second dictionary.

I debated about doing that the first time around.  But it's probably a
bit easier to follow, so I think I'll do it.


>> @@ -1390,7 +1414,8 @@ class QAPISchema(object):
>>
>>      def visit(self, visitor):
>>          visitor.visit_begin(self)
>> -        for (name, entity) in sorted(self._entity_dict.items()):
>> +        for entity in sorted(self._entity_dict.values(),
>> +                             key=attrgetter('name')):
>>              if visitor.visit_needed(entity):
>>                  entity.visit(visitor)
>>          visitor.visit_end()
> 
> Cool trick.

and with two dicts, I wouldn't need this trick.

>> +++ b/tests/qapi-schema/command-type-case-clash.err
>> @@ -0,0 +1 @@
>> +tests/qapi-schema/command-type-case-clash.json:3: Entity 'foo' collides with 'Foo'
> 
> The message's location is foo's.  An additional message "'Foo' defined
> here" would be nice.  Just an idea, could be done later.

The way we currently generate a single line message is by raising a
python exception.  I guess we could rework things to build up lines at a
time, then finally raise the complete multi-line message on a pre-formed
string, rather than the current building the message as a parameter to
the exception constructor.  But any changes on this front will have to
wait for later.

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]