Re: [Qemu-devel] [PATCH v10 27/30] qapi: Track owner of each object member

[Eric Blake <eblake@redhat.com>]

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

On 11/09/2015 07:26 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> 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.  In particular, when a member is
>> inherited from a base type, it is desirable to associate the
>> member name with the base type (and not the type calling
>> member.check()).
>>

>>
>> +    def _pretty_owner(self):
>> +        # See QAPISchema._make_implicit_object_type() - reverse the
>> +        # mapping there to create a nice human-readable description
> 
> This comment confused me briefly.  It applies to the following
> conditionals if-part, but not its else-part.  Move it into the if-part?

Done.


>> @@ -1082,9 +1117,11 @@ class QAPISchemaAlternateType(QAPISchemaType):
>>          QAPISchemaType.__init__(self, name, info)
>>          assert isinstance(variants, QAPISchemaObjectTypeVariants)
>>          assert not variants.tag_name
>> +        variants.set_owner(name)
>>          self.variants = variants
>>
>>      def check(self, schema):
>> +        self.variants.tag_member.set_owner(self.name)
>>          self.variants.tag_member.check(schema)
>>          self.variants.check(schema, {})
>>
> 
> Odd: all other .set_owner() calls are in .__init__() or .set_owner().
> Can we move this one to __init__() for consistency?

Yes, done.

> 
> I think we can: __init__() requires its variants argument to have a
> tag_member (it even asserts not variants.tag_name).
> 
>> @@ -1212,6 +1249,7 @@ class QAPISchema(object):
>>      def _make_implicit_object_type(self, name, info, role, members):
>>          if not members:
>>              return None
>> +        # See also QAPISchemaObjectTypeMember.describe()
> 
> Should this point to ._pretty_owner() instead?

Good call.

> 
>>          name = ':obj-%s-%s' % (name, role)
>>          if not self.lookup_entity(name, QAPISchemaObjectType):
>>              self._def_entity(QAPISchemaObjectType(name, info, None,
>> @@ -1315,7 +1353,7 @@ class QAPISchema(object):
>>          data = expr.get('data')
>>          if isinstance(data, OrderedDict):
>>              data = self._make_implicit_object_type(
>> -                name, info, 'arg', self._make_members(data, info))
>> +                name, info, 'data', self._make_members(data, info))
> 
> This is necessary only to make .pretty_owner() say 'data of EVENT_NAME'
> instead of 'argument of EVENT_NAME'.  Do we really need different
> wording for commands and events?
> 
> I'd make it say 'parameter of' for both commands and events.  I
> habitually use "parameter" for formal parameters, and "argument" for
> actual arguments.  Guess I've worked with the C standard too much...

'parameter' sounds better for both, at which point the messages no
longer differ,...

> 
> For what it's worth, the QAPI schema language uses 'data' for both (and
> many other things, too), and the introspection schema uses 'arg-type'
> for both.
> 
>>          self._def_entity(QAPISchemaEvent(name, info, data))
>>
>>      def _def_exprs(self):
>> diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
>> index 786024e..f78ef04 100644
>> --- a/tests/qapi-schema/qapi-schema-test.out
>> +++ b/tests/qapi-schema/qapi-schema-test.out
>> @@ -1,9 +1,9 @@
>>  object :empty
>> -object :obj-EVENT_C-arg
>> +object :obj-EVENT_C-data
>>      member a: int optional=True
>>      member b: UserDefOne optional=True
>>      member c: str optional=False
>> -object :obj-EVENT_D-arg
>> +object :obj-EVENT_D-data

...so I no longer need to rename the implicit struct for events to allow
the differentiation.  v11 is thus a smaller patch :)

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