[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Markus Armbruster
Subject: Re: [Qemu-devel] [PATCH v5 22/28] qapi: Whitelist commands that don't return dictionary
Date: Sun, 29 Mar 2015 12:22:01 +0200
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux)

Eric Blake <address@hidden> writes:

> On 03/27/2015 10:19 AM, Markus Armbruster wrote:
>> Eric Blake <address@hidden> 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 <address@hidden>
>>> ---
>> 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.]


reply via email to

[Prev in Thread] Current Thread [Next in Thread]