Eric Blake <ebl...@redhat.com> writes: > On 03/27/2015 10:19 AM, Markus Armbruster wrote: >> Eric Blake <ebl...@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 <ebl...@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.