Re: [Qemu-devel] [PATCH v7 09/14] qapi: Track owner of each object member

[Eric Blake]

On 10/09/2015 07:17 AM, Markus Armbruster wrote:
> Eric Blake <address@hidden> writes:
> 
>> Future commits will migrate semantic checking away from parsing
>> and over to the various QAPISchema*.check() methods.  But to
>> report an error message about an incorrect semantic use of a
>> member of an object type, it helps to know which type, command,
>> or event owns the member.  Rather than making all the check()
>> methods have to pass around additional information, it is easier
>> to have each member track the name of the type that owns it in
>> the first place.
> 
> Making members track their owner is easy enough (your patch is proof),
> but asserting it's easier suggests you tried the other approach, too.
> Did you?

Yes, my first attempt was indeed to pass the QAPISchemaEntity being
checked to each QAPISchemaObjectTypeMember.check() call...

> 
> In fact, passing the owner to the check() methods would be easy enough.
> QAPISchemaObjectType.check() passes self to its local members' .check().
> Covers non-variant members.  Variant members take a bit more effort,
> because more classes (and thus more check() methods) are involved.
> QAPISchemaObjectType.check() and QAPISchemaAlternateType.check() pass
> self to QAPISchemaObjectTypeVariants.check(), which passes it on to
> QAPISchemaObjectTypeVariant.check(), which passes it on to
> QAPISchemaObjectTypeMember.check().
> 
> I suspect the technique becomes cumbersome only when you start passing
> members to helper functions: you have to pass the owner, too.  If
> members know their owner, passing a member suffices.

...One of the other advantages of my approach that you don't get when
passing the owner to each check() call is that when it comes to base
classes, the owner of a member inherited from a base class should be the
base class, but the ObjectType that is calling the .check() method is
instead the derived class.  As soon as I realized that in my first
approach, my next attempt was to pass more information through each
element of the seen[] array, as in:
  seen[m.name] = {'member':m, 'owner':type}
until I  realized that it really was easier to just keep things with:
  seen[m.name] = m
and have m.owner do the right thing.  Having the member track the base
class owner makes the error messages much nicer (as in "'name' (member
of Sub) collides with 'name' (member of Base)" in a subsequent patch),
which was not possible when the only thing being passed down was the
current ObjectType being checked.

>> +++ b/scripts/qapi.py
>> @@ -961,8 +961,16 @@ class QAPISchemaObjectType(QAPISchemaType):
>>          assert base is None or isinstance(base, str)
>>          for m in local_members:
>>              assert isinstance(m, QAPISchemaObjectTypeMember)
>> -        assert (variants is None or
>> -                isinstance(variants, QAPISchemaObjectTypeVariants))
>> +            assert not m.owner
>> +            m.owner = name
>> +        if variants is not None:
>> +            assert isinstance(variants, QAPISchemaObjectTypeVariants)
>> +            if variants.tag_member:
>> +                assert not variants.tag_member.owner
>> +                variants.tag_member.owner = name
>> +            for v in variants.variants:
>> +                assert not v.owner
>> +                v.owner = name
> 
> Works, but rummaging in instances of other classes is not so nice.
> Could instead do
> 
>         for m in local_members:
>             m.set_owner(name)
>         if variants is not None:
>             variants.set_owner(name)
> 
> with the obvious set_owner() methods in QAPISchemaObjectTypeMember,
> QAPISchemaObjectTypeVariants, QAPISchemaObjectTypeVariant.

Yeah, I was debating about a set_owner() method - sounds like it is the
way to go, and that I ought to post a v8 spin of this series.

>> @@ -1034,6 +1044,15 @@ class QAPISchemaObjectTypeMember(object):
>>      def c_name(self):
>>          return c_name(self.name)
>>
>> +    def describe(self):
>> +        source = self.owner
>> +        if source.startswith(':obj-'):
>> +            source = source[5:]
>> +        return "'%s' (%s of %s)" % (self.name, self._describe(), source)
>> +
>> +    def _describe(self):
>> +        return 'member'
> 
> A simple class variable would do, wouldn't it?

Are class variables polymorphic?  (I guess I have to go figure out the
python lookup rules for self.variable, to see if they differ from
self.method())

>> @@ -1042,6 +1061,9 @@ class 
>> QAPISchemaObjectTypeUnionTag(QAPISchemaObjectTypeMember):
>>          assert self.type.is_implicit(QAPISchemaEnumType)
>>          return 'kind'
>>
>> +    def describe(self):
>> +        return "'kind' (implicit tag of %s)" % self.owner
>> +
> 
> Let's compare to the inherited describe() with this one.
> 
> Why no need to strip a ':obj-' prefix here?

Because no union type is implicit, so there is no ':obj-' prefix to strip.

> 
> This prints "'kind' (implicit tag of ...)".  The inherited describe()
> would print "'type' (member of ...).
> 
> The 'kind' vs. 'type' difference doesn't matter, because neither name
> occurs in the schema anyway.
> 
> "implicit tag of" is an improvement over "member of".
> 
> However, the inherited describe() already has a hook to replace the part
> before the "of".  Why do we need to override it wholesale anyway?

The parent version prints self.name ('type'), but as long as we have the
type/kind mismatch, we want to print the C name that is conflicting
('kind') and not self.name.  But once I override describe() for that, it
was easier to do a wholesale override.  It all goes away later in the
patch that renames kind to type.  Maybe some comments here would help?


>> @@ -1222,7 +1251,7 @@ class QAPISchema(object):
>>      def _make_implicit_object_type(self, name, info, role, members):
>>          if not members:
>>              return None
>> -        name = ':obj-%s-%s' % (name, role)
>> +        name = ':obj-%s %s' % (name, role)
>>          if not self.lookup_entity(name, QAPISchemaObjectType):
>>              self._def_entity(QAPISchemaObjectType(name, info, None,
>>                                                    members, None))
> 
> I know I suggested this, but I'm having second thoughts about spaces in
> name.  The test output (visible below) becomes a bit confusing, and
> unnecessarily hard to parse by ad hoc scripts (yes, I've done such
> things from time to time).
> 
> We could keep
> 
>         name = ':obj-%s-%s' % (name, role)
> 
> here, and replace the '-' by ' ' in describe().  Problematic if name or
> role can also contain '-'.  Use a more suitable character to separate
> the two then.

Name can contain '-', role does not. Can always search for the last '-'.
Or we could reorder things to put role first.  Or use ':obj:%s:%s'.  But
yes, I agree that we can keep the implicit name space-free, and make
describe() smarter at reverse-engineering it back into a human-readable
description.

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

signature.asc
Description: OpenPGP digital signature