Re: [Qemu-devel] [PATCH v5 22/28] qapi: Whitelist commands that don't return dictionary

[Markus Armbruster <armbru@redhat.com>]

Eric Blake <eblake@redhat.com> writes:

> On 03/27/2015 10:19 AM, Markus Armbruster wrote:
>> Eric Blake <eblake@redhat.com> writes:
>> 
>>> ...or an array of dictionaries.  Although we have to cater to
>>> existing commands, returning a non-dictionary means the command
>>> is not extensible (no new name/value pairs can be added if more
>>> information must be returned in parallel).  By making the
>>> whitelist explicit, any new command that falls foul of this
>>> practice will have to be self-documenting, which will encourage
>>> developers to either justify the action or rework the design to
>>> use a dictionary after all.
>>>
>>> Signed-off-by: Eric Blake <eblake@redhat.com>
>>> ---
>
>
>> 
>> Thinking on introspection, I started to wonder whether there's actually
>> a command returning a union, yet.  So I applied the appended stupid
>> patch on top, and found the following commands returning (list of)
>> non-struct type:
>> 
>> * qapi-schema.json:
>> 
>>   'ringbuf-read'                built-in type 'str'
>>   'human-monitor-command'       built-in type 'str'
>>   'query-migrate-cache-size'    built-in type 'int'
>>   'query-tpm-models'            enum type 'TpmModel'
>
> More precisely, "array of enum type 'TpmModel'" (or "list", depending on
> whether we go with JSON array/object terminology, or QObject dict/list).
>  I wonder if it is worth trying to tweak the error message to more
> precisely track when I strip away the [] earlier in check_type to still
> report sane messages about 'array of ...' if a later check fails.

I figure the error message is understandable enough as is.  But if it
improving it would be easy, then why not.  Could be done on top.

>>   'query-tpm-types'             enum type 'TpmType'
>>   'query-memory-devices'        union type 'MemoryDeviceInfo'
>> 
>> * qga/qapi-schema.json:
>> 
>>   'guest-sync-delimited'        built-in type 'int'
>>   'guest-sync'                  built-in type 'int'
>>   'guest-get-time'              built-in type 'int'
>>   'guest-file-open'             built-in type 'int'
>>   'guest-fsfreeze-status'       enum type 'GuestFsfreezeStatus'
>>   'guest-fsfreeze-freeze'       built-in type 'int'
>>   'guest-fsfreeze-freeze-list'  built-in type 'int'
>>   'guest-fsfreeze-thaw'         built-in type 'int'
>>   'guest-set-vcpus'             built-in type 'int'
>
> Good - your patch found all of my whitelists, plus...
>
>> 
>> The sole example for union is 'MemoryDeviceInfo'.  It has one case %-}
>
> Yeah, MemoryDeviceInfo as a union currently has only one type, but it
> was done that way in case we add other memory devices.  So it was
> actually quite forward-thinking.
>
> ...one additional thing.  But returning (an array of) a union should be
> okay (it is a dictionary, and therefore extensible); this patch was only
> about flagging non-dictionaries.
>
> [side note: again, my idea of renaming 'type' into 'struct' in the .json
> files would make it easier to talk about "complex types" as the set of
> "struct" and "union" types, rather than the current confusion of
> deciding if "type" means all meta-types or just struct meta-types.]

Yes.