[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.]
Yes.
- Re: [Qemu-devel] [PATCH v5 06/28] qapi: Add some union tests, (continued)
[Qemu-devel] [PATCH v5 26/28] qapi: Drop inline nested type in query-version, Eric Blake, 2015/03/24
[Qemu-devel] [PATCH v5 22/28] qapi: Whitelist commands that don't return dictionary, Eric Blake, 2015/03/24
[Qemu-devel] [PATCH v5 23/28] qapi: More rigorous checking for type safety bypass, Eric Blake, 2015/03/24
[Qemu-devel] [PATCH v5 19/28] qapi: Add some type check tests, Eric Blake, 2015/03/24
[Qemu-devel] [PATCH v5 16/28] qapi: Better error messages for duplicated expressions, Eric Blake, 2015/03/24