jcs.org / qemu
qemu with hax to log dma reads & writes jcs.org/2018/11/12/vfio
fork atom

qapi: Whitelist commands that don't return dictionary

...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.

It's a little bit sloppy that we share a single whitelist among
three clients (it's too permissive for each). If this is a
problem, a future patch could tighten things by having the
generator take the whitelist as an argument (as in
scripts/qapi-commands.py --legacy-returns=...), or by having
the generator output C code that requires explicit use of the
whitelist (as in:
#ifndef FROBNICATE_LEGACY_RETURN_OK
# error Command 'frobnicate' should return a dictionary
#endif
then having the callers define appropriate macros). But until
we need such fine-grained separation (if ever), this patch does
the job just fine.

Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>

authored by

Eric Blake and committed by
Markus Armbruster 10d4d997 c9e0a798

+37 -20
+28 -3
scripts/qapi.py
··· 32 32 'size': 'QTYPE_QINT', 33 33 } 34 34 35 + # Whitelist of commands allowed to return a non-dictionary 36 + returns_whitelist = [ 37 + # From QMP: 38 + 'human-monitor-command', 39 + 'query-migrate-cache-size', 40 + 'query-tpm-models', 41 + 'query-tpm-types', 42 + 'ringbuf-read', 43 + 44 + # From QGA: 45 + 'guest-file-open', 46 + 'guest-fsfreeze-freeze', 47 + 'guest-fsfreeze-freeze-list', 48 + 'guest-fsfreeze-status', 49 + 'guest-fsfreeze-thaw', 50 + 'guest-get-time', 51 + 'guest-set-vcpus', 52 + 'guest-sync', 53 + 'guest-sync-delimited', 54 + 55 + # From qapi-schema-test: 56 + 'user_def_cmd3', 57 + ] 58 + 35 59 enum_types = [] 36 60 struct_types = [] 37 61 union_types = [] ··· 354 378 check_type(expr_info, "'data' for command '%s'" % name, 355 379 expr.get('data'), allow_dict=True, allow_optional=True, 356 380 allow_metas=['union', 'struct']) 381 + returns_meta = ['union', 'struct'] 382 + if name in returns_whitelist: 383 + returns_meta += ['built-in', 'alternate', 'enum'] 357 384 check_type(expr_info, "'returns' for command '%s'" % name, 358 385 expr.get('returns'), allow_array=True, allow_dict=True, 359 - allow_optional=True, 360 - allow_metas=['built-in', 'union', 'alternate', 'struct', 361 - 'enum']) 386 + allow_optional=True, allow_metas=returns_meta) 362 387 363 388 def check_event(expr, expr_info): 364 389 global events
+1
tests/qapi-schema/returns-alternate.err
··· 1 + tests/qapi-schema/returns-alternate.json:3: 'returns' for command 'oops' cannot use alternate type 'Alt'
+1 -1
tests/qapi-schema/returns-alternate.exit
··· 1 - 0 1 + 1
+1 -1
tests/qapi-schema/returns-alternate.json
··· 1 - # FIXME: we should reject returns if it is an alternate type 1 + # we reject returns if it is an alternate type 2 2 { 'alternate': 'Alt', 'data': { 'a': 'int', 'b': 'str' } } 3 3 { 'command': 'oops', 'returns': 'Alt' }
-4
tests/qapi-schema/returns-alternate.out
··· 1 - [OrderedDict([('alternate', 'Alt'), ('data', OrderedDict([('a', 'int'), ('b', 'str')]))]), 2 - OrderedDict([('command', 'oops'), ('returns', 'Alt')])] 3 - [{'enum_name': 'AltKind', 'enum_values': None}] 4 - []
+2 -1
tests/qapi-schema/returns-int.json
··· 1 1 # It is okay (although not extensible) to return a non-dictionary 2 - { 'command': 'okay', 'returns': 'int' } 2 + # But to make it work, the name must be in a whitelist 3 + { 'command': 'guest-get-time', 'returns': 'int' }
+1 -1
tests/qapi-schema/returns-int.out
··· 1 - [OrderedDict([('command', 'okay'), ('returns', 'int')])] 1 + [OrderedDict([('command', 'guest-get-time'), ('returns', 'int')])] 2 2 [] 3 3 []
+1
tests/qapi-schema/returns-whitelist.err
··· 1 + tests/qapi-schema/returns-whitelist.json:10: 'returns' for command 'no-way-this-will-get-whitelisted' cannot use built-in type 'array of int'
+1 -1
tests/qapi-schema/returns-whitelist.exit
··· 1 - 0 1 + 1
+1 -1
tests/qapi-schema/returns-whitelist.json
··· 1 - # FIXME: we should enforce that 'returns' be a dict or array of dict unless whitelisted 1 + # we enforce that 'returns' be a dict or array of dict unless whitelisted 2 2 { 'command': 'human-monitor-command', 3 3 'data': {'command-line': 'str', '*cpu-index': 'int'}, 4 4 'returns': 'str' }
-7
tests/qapi-schema/returns-whitelist.out
··· 1 - [OrderedDict([('command', 'human-monitor-command'), ('data', OrderedDict([('command-line', 'str'), ('*cpu-index', 'int')])), ('returns', 'str')]), 2 - OrderedDict([('enum', 'TpmModel'), ('data', ['tpm-tis'])]), 3 - OrderedDict([('command', 'query-tpm-models'), ('returns', ['TpmModel'])]), 4 - OrderedDict([('command', 'guest-get-time'), ('returns', 'int')]), 5 - OrderedDict([('command', 'no-way-this-will-get-whitelisted'), ('returns', ['int'])])] 6 - [{'enum_name': 'TpmModel', 'enum_values': ['tpm-tis']}] 7 - []