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

[Markus Armbruster]

Eric Blake <address@hidden> writes:

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

You could also try a local variable: cnam = c_name(self.name).

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

I'm afraid I don't have enough context to grok this late on Friday :)

>>> @@ -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 had already flagged a few assertions as "holds before the patch" when
I got here, and felt an urge to flag this one, too, for completeness.  I
don't object to you adding the assertion.

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