Re: [Qemu-devel] [PATCH v4 04/19] qapi: Document type-safety considerations

[Eric Blake <eblake@redhat.com>]

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

On 09/22/2014 06:37 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 

>>  === Union types ===
>>
>> +Usage: { 'union': 'str', 'data': 'dict', '*base': 'complex-type-name',
>> +         '*discriminator': 'enum-type-name' }
>> +or:    { 'union': 'str', 'data': 'dict', 'discriminator': {} }
>> +
> 
> Suggest to split usage into simple union, flat union and anonymous
> union, like this:
> 
> Usage: { 'union': 'str', 'data': 'dict', '*base': 'complex-type-name' }

^ simple

> or:    { 'union': 'str', 'data': 'dict', 'base': 'complex-type-name',
>          '*discriminator': 'enum-type-name' }

^ flat

> or:    { 'union': 'str', 'data': 'dict', 'discriminator': {} }

^ anonymous

Except that someday, I'd like to have:

{ 'union': str', 'data': 'dict', 'discriminator': 'enum-type-name' }

which, when compared with the simple and flat versions, means that base
and discriminator are equally optional.  But you are right that we don't
have that form yet, and that the code currently requires that if base is
specified, then discriminator is non-optional.  I'll have to tweak this.


>> +
>> +In rare cases, QAPI cannot express a type-safe representation of a
>> +corresponding QMP command.  In these cases, if the command expression
>> +includes the key 'gen' with value 'no', then the 'data' or 'returns'
> 
> The implementation actually ignores the value of 'gen', but specifying
> it must be 'no' doesn't hurt.

Actually, see patch 14/19 later in the series, where I fix the code to
enforce that it must be 'no' :)

> 
>> +member that intends to bypass generated type-safety and do its own
>> +manual validation should use '**' rather than a valid type name.
>> +Please try to avoid adding new commands that rely on this, and instead
>> +use type-safe unions.  For an example of bypass usage:
>> +
>> + { 'command': 'netdev_add',
>> +   'data': {'type': 'str', 'id': 'str', '*props': '**'},
>> +   'gen': 'no' }
>> +
>> +Normally, the QAPI schema is used to describe synchronous exchanges,
>> +where a response is expected.  But in some cases, the action of a
>> +command is expected to change state in a way that a successful
>> +response is not possible (a failure message still returns a
>> +dictionary).  In this case, the command expression should include the
>> +optional key 'success-response' with value 'no'.
> 
> Learned something new here.

To date, only qga uses this form.


>> +
>> +Events are defined with the keyword 'event'.  It is not allowed to
>> +name an event 'MAX', since the generator also produces a C enumeration
>> +of all event names with a generated _MAX value at the end.
> 
> One of the several places where the generator can thoughtlessly produce
> clashing identifiers.  You're documenting one of them, which is an
> improvement of sorts.

I also enhance things later in the series to enforce this documentation :)


> 
> Major improvement, thank you very much!

I've trimmed your other comments (such as suggested line breaks) because
I agree with them, and will incorporate them into either v5 (if the
series needs respinning) or a followup patch (if this is the only patch
that needs improvement).

-- 
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: 539 bytes --]