Re: [Qemu-devel] [PATCH v6 07/12] qapi: Detect collisions in C member names

[Eric Blake]

On 10/02/2015 07:19 AM, Markus Armbruster wrote:
> Eric Blake <address@hidden> writes:
> 
>> Detect attempts to declare two object members that would result
>> in the same C member name, by keying the 'seen' dictionary off
>> of the C name rather than the qapi name.  Doing this was made
>> easier by adding a member.c_name() helper function.
> 
> This gains protection against colliding C names.  It keeps protection
> against colliding QMP names as long as QMP name collision implies C name
> collision.  I think that's the case, but it's definitely worth spelling
> out in the code, and possibly in the commit message.
> 
>> As this is the first error raised within the QAPISchema*.check()
>> methods, we also have to pass 'info' through the call stack, and
>> fix the overall 'try' to display errors detected during the
>> check() phase.
> 
> Could also be a separate patch, if the parts are easier to review.

I'm still debating about that.


>> +    def check(self, schema, info, all_members, seen):
>> +        if self.c_name() in seen:
>> +            raise QAPIExprError(info,
>> +                                "%s collides with %s"
>> +                                % (self.describe(),
>> +                                   seen[self.c_name()].describe()))
>>          self.type = schema.lookup_type(self._type_name)
>>          assert self.type
>>          all_members.append(self)
>> -        seen[self.name] = self
>> +        seen[self.c_name()] = self
>> +
>> +    def c_name(self):
>> +        return c_name(self.name)
> 
> Why wrap function c_name() in a method?  Why not simply call the
> function?

'self.c_name()' is shorter than 'c_name(self.name)'.  And I already had
long lines with that seen[self.c_name()].describe() pattern.

> 
> It's method in QAPISchemaEntity only because this lets us add special
> cases in a neat way.

True, but I _did_ mention in the commit message that I did it for less
typing.

But as to special cases, yes, I have one in mind (although I have not
played with it yet).  In v5 19/46 Simplify visiting of alternate types,
I want to change alternates to have variants.tag_member == None, and the
generated C code to use 'qtype_code type;' as the tag variable.  In the
v5 representation, it led to a lot of special-casing (many uses of
QAPISchemaObjectTypeVariants became more complex because tag_member was
not always defined).  But now that I've been working on these front-end
patches, my idea was to do something like:

class QAPISchemaVariantTag(QAPISchemaObjectTypeMember):
    def __init__(self, info):
        QAPISchemaObjectTypeMember(self, 'type', None, False)

    def c_type(self):
        return 'qtype_code'

and then, we WOULD need member.c_type() rather than member.type.c_type()
(at which point having member.c_name() to match member.c_type() makes
more sense).

>> @@ -1028,23 +1037,24 @@ class QAPISchemaObjectTypeVariants(object):
>>          self.tag_member = tag_member
>>          self.variants = variants
>>
>> -    def check(self, schema, members, seen):
>> +    def check(self, schema, info, members, seen):
>>          if self.tag_name:
>> -            self.tag_member = seen[self.tag_name]
>> +            self.tag_member = seen[c_name(self.tag_name)]
>> +            assert self.tag_name == self.tag_member.name
> 
> Assertion should hold before the patch, but it's kind of trivial then.

My worry here was that if I have:

{ 'enum': 'Enum', 'data': ['a'] }
{ 'base': 'Base', 'data': { 'b-c': 'Enum' } }
{ 'union': 'Flat', 'base': 'Base', 'discriminator': 'b_c',
  'data': { 'a': 'Struct...' } }

the old ad hoc parser rejects 'b_c' as not being a member of Enum (bad
discriminator).  But once we convert from the old parser to the check()
method (basically, anywhere the check() methods now have an assert will
become if statements that raise an exception), we need to make sure that
we don't get confused by the fact that seen[c_name('b_c')] maps to the
same value as seen[c_name('b-c')], so that we are still flagging the
flat union as invalid.

> 
> I'm fine with not splitting this patch.  I'd be also fine with splitting
> it up.  Your choice.

I'm still thinking about it; may depend on how much other refactoring I do.

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

signature.asc
Description: OpenPGP digital signature