Re: [Qemu-devel] [PATCH v6 06/12] qapi: Track owner of each object member

[Eric Blake]

On 10/02/2015 03:50 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, we need 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 who owns it in the first place.
>>
>> The source information is intended for human consumption in
>> error messages, and a new describe() method is added to access
>> the resulting information.  For example, given the qapi:
>>  { 'command': 'foo', 'data': { 'string': 'str' } }
>> an implementation of visit_command() that calls
>>  arg_type.members[0].describe()
>> will see "'string' (member of foo arguments)".
> 
> Peeking ahead a bit, I see two describe(), one for ordinary members
> returning
> 
>     "'%s' (member of %s)" % (self.name, self._owner)
> 
> and one for variant members returning
> 
>     "'%s' (branch of %s)" % (self.name, self._owner)
> 
> The name _owner makes me expect it's the owning types name, but that's
> not always the case, as we shall see.  How is it related to info and to
> the owning type's name then?
> 
> In your example (implicit arguments type):
> 
>     arg_type.members[0]._owner is 'foo arguments'.
> 
>     arg_type.members[0] has no info.

Well, none of the members have info - they are not subclassed from
QAPISchemaEntity.

> 
>     arg_type.name is ':obj-foo-arg'
> 
>     arg_type.info is something like
> 
>         info {'line': 10, 'parent': None, 'file': 'example-schema.json'}
> 
>     pointing to the definition of command 'foo'.  It's actually the
>     command's info, inherited by its implicit argument type.
> 
> Here, _owner is merely a variation of the owning type's name geared for
> human readers.

Oh, I think I see where you are going with this - why is 'owner' a
string, rather than the actual QAPISchemaType python object?  And is it
worth following the pattern used in other classes, where __init__ gets a
string naming the type, and then check() resolves that name to the
actual type?  At which point, we could do member.owner.info to access
the info of the type that owns the member?

But there's a chicken-and-egg situation - we don't know what the type
will be named until we call _make_implicit_object_type(), but that
function requires that we have already pre-constructed the
QAPISchemaObjectTypeMembers array (which means we can't pre-construct
the members with the type name embedded).

We'd have to refactor things to generate the type name, then construct
the members, then construct the type (doable, but probably involves
splitting this patch for ease of review).

> 
> Example of explicit arguments type:
> 
>     { 'struct': 'BarArgs', 'data': { 'string': 'str' } }
>     { 'command': 'bar', 'data': 'BarArgs' }
> 
> Here, we get:
> 
>     arg_type.members[0]._owner is 'BarArgs'.
> 
>     arg_type.members[0] has no info.

Again, because NO members have info.

> 
>     arg_type.name is 'BarArgs'
> 
>     arg_type.info is something like
> 
>         info {'line': 12, 'parent': None, 'file': 'example-schema.json'}
> 
>     pointing to the definition of command 'bar.  Again, it's the
>     command's info, inherited by its implicit argument type.
> 
> Here, _owner *is* the owning type's name.
> 
> So, _owner is a more readable name we make up when the other name for
> the same thing isn't readable.  However, we make up that other name,
> too!  Begs the question why we don't simply make it readable right away.
> 
> Naturally, we still need to make up names collision-free.  But as far as
> I can tell, nothing stops us from picking ':obj-foo arguments' instead
> of ':obj-foo-arg', and when we talk to users strip off the common prefix
> ':obj-' we prepend to avoid collisions.

Might be doable, but then we'd have to generate the implicit object name
prior to creating its Member objects (thus splitting
_make_implicit_object_type() into two parts).

> 
>> Where implicit types are involved, the code intentionally tries
>> to pick the name of the owner of that implicit type, rather than
>> the type name itself (a user reading the error message should be
>> able to grep for the problem in their original file, but will not
>> be able to locate a generated implicit name).
>>
>> No change to generated code.
>>
>> Signed-off-by: Eric Blake <address@hidden>
> 
> Let's discuss the above before I review the actual patch closely.

What do you think - would that refactoring be worth it?

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

signature.asc
Description: OpenPGP digital signature