Re: [Qemu-devel] [RFC PATCH 08/11] qapi: Anonymous unions

[Eric Blake]

On 07/12/2013 02:55 AM, Kevin Wolf wrote:
> Am 11.07.2013 um 21:47 hat Eric Blake geschrieben:
>> On 07/09/2013 03:53 AM, Kevin Wolf wrote:
>>> The discriminator for anonymous unions is the data type. This allows to
>>> have a union type that allows both of these:
>>>
>>>     { 'file': 'my_existing_block_device_id' }
>>>     { 'file': { 'filename': '/tmp/mydisk.qcow2', 'read-only': true } }
>>>
>>> Unions like this are specified in the schema with an empty dict as
>>> discriminator. For this example you could take:
>>>
>>>     { 'union': 'BlockRef',
>>>       'discriminator': {},
>>>       'data': { 'definition': 'BlockOptions'
>>>                 'reference': 'str' } }
>>>     { 'type': 'ExampleObject',
>>>       'data: { 'file': 'BlockRef' } }
>>
>> Yay - a commit message that shows the new QMP wire format, and the
>> qapi-schema.json that generated it.
> 
> Yeah, sorry about these missing examples and detailed documentation. I
> wanted to get an RFC out quickly and probably underestimated how
> important it is even for reviewing an RFC.

The higher volume the list, the more important documentation is :)

> 
> However, I think keeping this information only in the commit messages
> isn't right either. Do we have any of the schema syntax documented
> anywhere? Should I create something in docs/(specs/)?

We have qemu/docs/qapi-code-gen.txt - and you are right - that file
NEEDS to be updated with this series :)

> 
>> [Without reading the patch yet]
>> I take it the 'data' of such a union must be completely distinguishable
>> by type - the visitor for the 'file' key tries each subtype in turn
>> ('BlockOptions' or 'str') until one does not have a parse error.
> 
> Not exactly. It asks the visitor for the type of the next item and then
> uses a lookup table to get the right discriminator enum value for it.

More efficient than trying every visitor; but same idea of the lookup
table being used to say which visitor will succeed.

> 
>> And
>> this changes the earlier patch that required 'discriminator' to be used
>> only when 'base' was also present.  Can it distinguish between two
>> structs, or is the union only allowed to have one struct option and all
>> other options are primitive types?  Should we consider the use of arrays
>> as a union member type?
> 
> As the types it uses for lookup are QObject types and all structs map to
> QDict, only one struct is allowed.

Worth documenting.

> 
> This makes sense anyway, because allowing different structs would mean
> that you potentially get ambiguities when structs look alike (perhaps
> only differing in optional fields or something like that).
> 
> As soon as we find a good use case for anonymous unions of two structs,
> I might change my opinion, though. ;-)
> 
> Arrays are a logical extension of this, though I don't have any use for
> them now. I wouldn't expect them to be too hard to implement.

I don't mind leaving them out for now; as you said, it should be easy
enough to add at the point where the schema makes it worth doing.

> 
>> Hmm, I wonder if this new anonymous union can be useful in helping us
>> write the self-description of introspection in Amos' patches.
> 
> Hm, what's the exact problem there? I know I read that someone (Paolo?)
> wrote that it was impossible, but I didn't immediately understand why
> and didn't feel like thinking a lot about either.

Indeed: if we write a self-describing schema for introspection, then
'enum' uses 'data':['str'], while 'type', 'union', and 'command' use
'data':'QDict'.  If we make an anonymous union with discriminator based
on enum/type/union/command, then the type of data is determined based on
the discriminator, with an array involved.

But it was more than just what data type is tied to 'data', it was also
the fact that qapi-schema.json contains free-form dictionaries
(arbitrary keys, each with a type), while QAPI tries to describe the
wire format of all possible keys, where only values are arbitrary
strings.  Hence I've been arguing all along that we need a
post-processing step that turns qapi-schema's 'key':'Type' into the QMP
wire struct { 'name':'key', 'type':'Type' }, so that 'name' and 'type'
are fixed keys.  I think both issues are at hand - how to represent each
field (whether as short-hand straight from the .json file or a long-hand
struct) and how to distinguish between qapi metatypes (which are
effectively a union between 'type'/'command'/...).

>>> +
>>> +    ret += mcgen('''
>>> +        default:
>>> +            abort();
>>
>> Does this mean I can cause qemu to abort if I pass bogus information on
>> the wire?  Using your commit message example,
>>
>>     { 'file': false }
>>
>> would hit the default case, right?
> 
> No, it wouldn't, but that case fails in an interesting way anyway. :-)
> 
> We would look up the enum kind in BlockRef_qtypes[QTYPE_BOOL], which
> isn't defined and therefore 0. This happens to be the enum value for
> BLOCK_REF_KIND_DEFINITION. So I guess what you get is an error message
> like "invalid argument type: got bool, expected dict".

As long as it is an error message and not a qemu crash, I'm okay.  I
should be able to throw valid JSON but garbage contents at the parser,
without the parser falling over.

> 
>> Does it make sense to allow a union type (possibly only an anonymous
>> union type) in place of a struct type as a top-level type reference?
>> That is, just as we now allow { 'command':'foo', 'data': 'StructType' },
>> would it make sense to allow { 'command':bar', 'data': 'AnonUnion' }?
> 
> Sounds sensible to me. But do we have a use case for it?

Not sure yet; again, one of those things that should be easy enough to
defer until we have a use case, and where introspection may be such a
use case.

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

signature.asc
Description: OpenPGP digital signature