Re: [Qemu-devel] [PATCH v5 11/46] qapi: Don't use info as witness of implicit object type

[Eric Blake <eblake@redhat.com>]

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

On 09/28/2015 06:43 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
>> A future patch will enable error reporting from the various
>> check() methods.  But to report an error on an implicit type,
>> we'll need to associate a location with the type (the same
>> location as the top-level entity that is causing the creation
>> of the implicit type), and once we do that, keying off of
>> whether foo.info exists is no longer a viable way to determine
>> if foo is an implicit type.
> 
> Ensuring error messages are good even for implicit types could be hard.
> But pretty much anything's better than error messages without location
> information.

Especially since the current implementation crashes hard when trying to
report an error with info=None.

> 
>> Rename the info member to _info (so that sub-classes can still
>> use it, but external code should not), add an is_implicit()
>> method to QAPISchemaObjectType, and adjust the visitor to pass
>> another parameter about whether the type is implicit.
> 
> I have doubts on the rename.

Fair enough; the proposal to separate it into its own patch, so we can
then discard or easily revert it, sounds like the right approach.


>>  class QAPISchemaObjectType(QAPISchemaType):
>> @@ -961,21 +961,25 @@ class QAPISchemaObjectType(QAPISchemaType):
>>              self.variants.check(schema, members, seen)
>>          self.members = members
>>
>> +    def is_implicit(self):
>> +        return self.name[0] == ':'
>> +
> 
> The predicate could be defined on any QAPISchemaType, or even any
> QAPISchemaEntity, but right now we only ever want to test it for
> objects.  Okay.

Yeah, I thought about that.  All builtin types are implicit, all array
types are implicit, no commands or events are implicit, and we didn't
make any different generated output based on whether enums were explicit
or implicit, so that leaves just objects.

>> +++ b/tests/qapi-schema/test-qapi.py
>> @@ -22,7 +22,7 @@ class QAPISchemaTestVisitor(QAPISchemaVisitor):
>>          if prefix:
>>              print '    prefix %s' % prefix
>>
>> -    def visit_object_type(self, name, info, base, members, variants):
>> +    def visit_object_type(self, name, info, base, members, variants, implicit):
>>          print 'object %s' % name
>>          if base:
>>              print '    base %s' % base.name
> 
> Three of our visitors implement visit_object_type():

Another idea would be change the signature from:

def visit_object_type(self (QAPISchemaVisitor), name (str),
                      info (dict), base (QEMUSchemaObjectType),
                      members (list of QEMUSchemaObjectTypeMember),
                      variants (QAPISchemaObjectTypeVariants),
                      implicit (bool))

to:

def visit_object_type(self, object (QEMUSchemaObjectType))

and let callers dereference object.name, object.info, object.base,
object.members (or object.local_members), object.variants, and
object.is_implicit() as they see fit. (In fact, in one of my later
patches, I already wished I had access to the actual
QEMUSchemaObjectType object rather than its breakdown of parts to begin
with, and ended up doing a schema.lookupType(name) just to get back to
that piece of information).


> 
> * test-qapi.py doesn't care about implicit (implicitness is obvious
>   enough from the name here).
> 
> * qapi-types.py and qapi-visit.py ignore implicit object types.  Hmm.
> 
>   qapi-introspect.py has a similar need: it wants to ignore *all* types.
>   Two ways to ignore entities seem one too many.  Preexisting, but your
>   patch makes it stand out a bit more.
> 
>   Could we reuse the existing mechanism somehow (and keep method
>   visit_object_type() simple)?
> 
>   To reuse it without changes, we'd have to make implicit object types a
>   separate class, so that QAPISchema.visit()'s isinstance() test can be
>   put to work.

Maybe. Would also make implementing is_implicit() easy (which type did I
instantiate) rather than hacky (does name start with ':').

> 
>   Another option is generalizing QAPISchema's filter.  How?
> 
>   A third option is to abandon QAPISchema's filter, and make
>   qapi-introspect.py filter in the visitor methods, just like we filter
>   implicit objects.

I'm still thinking about this one.

> 
> Patch could be split into
> 
> A. Encapsulate the "is implicit" predicate in a method, i.e. replace
>    not o.info by o.is_implicit().
> 
> B. Clean up how we filter out implicit objects.  May better go before A,
>    not sure.
> 
> C. Rename .info to ._info.  Not sure we even want this part.

Yes, I'll go along with a split somewhere along these lines before
reposting this patch for v6, although I'm going to have to sleep on it
before deciding how to clean up the filtering.

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