| Message ID | 1427227433-5030-23-git-send-email-eblake@redhat.com |
|---|---|
| State | New |
| Headers | show |
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> > --- > scripts/qapi.py | 30 ++++++++++++++++++++++++++++-- > tests/qapi-schema/returns-alternate.err | 1 + > tests/qapi-schema/returns-alternate.exit | 2 +- > tests/qapi-schema/returns-alternate.json | 2 +- > tests/qapi-schema/returns-alternate.out | 4 ---- > tests/qapi-schema/returns-int.json | 3 ++- > tests/qapi-schema/returns-int.out | 2 +- > tests/qapi-schema/returns-whitelist.err | 1 + > tests/qapi-schema/returns-whitelist.exit | 2 +- > tests/qapi-schema/returns-whitelist.json | 2 +- > tests/qapi-schema/returns-whitelist.out | 7 ------- > 11 files changed, 37 insertions(+), 19 deletions(-) > > diff --git a/scripts/qapi.py b/scripts/qapi.py > index ed5385a..9421431 100644 > --- a/scripts/qapi.py > +++ b/scripts/qapi.py > @@ -33,6 +33,30 @@ builtin_types = { > 'size': 'QTYPE_QINT', > } > > +# Whitelist of commands allowed to return a non-dictionary > +returns_whitelist = [ > + # From QMP: > + 'human-monitor-command', > + 'query-migrate-cache-size', > + 'query-tpm-models', > + 'query-tpm-types', > + 'ringbuf-read', > + > + # From QGA: > + 'guest-file-open', > + 'guest-fsfreeze-freeze', > + 'guest-fsfreeze-freeze-list', > + 'guest-fsfreeze-status', > + 'guest-fsfreeze-thaw', > + 'guest-get-time', > + 'guest-set-vcpus', > + 'guest-sync', > + 'guest-sync-delimited', > + > + # From qapi-schema-test: > + 'user_def_cmd3', > +] > + Since there's just one whitelist, all schemata share it, and that means it's too permissive for each of them. Sloppy, but good enough. If the sloppiness bothers us, here are two alternatives: * Program takes the whitelist as argument, say scripts/qapi-commands.py --legacy-returns qmp-legacy-returns ... * Leave enforcing to C If a command 'frobnicate' returns a non-dictionary, generate something like #ifndef FROBNICATE_LEGACY_RETURN_OK #error Command 'frobnicate' should return a dictionary #endif Then manually define the macros necessary to keep the current use working in a suitable header. > enum_types = [] > struct_types = [] > union_types = [] > @@ -350,10 +374,12 @@ def check_command(expr, expr_info): > check_type(expr_info, "'data' for command '%s'" % name, > expr.get('data'), > allowed_metas=['union', 'struct'], allow_optional=True) > + returns_meta = ['union', 'struct'] > + if name in returns_whitelist: > + returns_meta += ['built-in', 'alternate', 'enum'] > check_type(expr_info, "'returns' for command '%s'" % name, > expr.get('returns'), allow_array=True, > - allowed_metas=['built-in', 'union', 'alternate', 'struct', > - 'enum'], allow_optional=True) > + allowed_metas=returns_meta, allow_optional=True) > > def check_event(expr, expr_info): > global events [...] Reviewed-by: Markus Armbruster <armbru@redhat.com>
On 03/27/2015 03:11 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> >> --- > Since there's just one whitelist, all schemata share it, and that means > it's too permissive for each of them. Sloppy, but good enough. > > If the sloppiness bothers us, here are two alternatives: Doesn't bother me enough to use either alternative, but I _will_ update the commit message to at least remind someone of the potential for tightening things, should the need arise in the future. > > * Program takes the whitelist as argument, say > > scripts/qapi-commands.py --legacy-returns qmp-legacy-returns ... Early exit (generator fails if you don't whitelist), but noisy (have to touch a Makefile in addition to the .json schema). > > * Leave enforcing to C > > If a command 'frobnicate' returns a non-dictionary, generate something > like > > #ifndef FROBNICATE_LEGACY_RETURN_OK > #error Command 'frobnicate' should return a dictionary > #endif > > Then manually define the macros necessary to keep the current use > working in a suitable header. Late exit (failure does not occur at python generation time, but only at C compilation time) - and the point of this series was to promote several late exits into early exits. But slightly cleaner than having to touch Makefiles to add in the whitelist.
diff --git a/scripts/qapi.py b/scripts/qapi.py index ed5385a..9421431 100644 --- a/scripts/qapi.py +++ b/scripts/qapi.py @@ -33,6 +33,30 @@ builtin_types = { 'size': 'QTYPE_QINT', } +# Whitelist of commands allowed to return a non-dictionary +returns_whitelist = [ + # From QMP: + 'human-monitor-command', + 'query-migrate-cache-size', + 'query-tpm-models', + 'query-tpm-types', + 'ringbuf-read', + + # From QGA: + 'guest-file-open', + 'guest-fsfreeze-freeze', + 'guest-fsfreeze-freeze-list', + 'guest-fsfreeze-status', + 'guest-fsfreeze-thaw', + 'guest-get-time', + 'guest-set-vcpus', + 'guest-sync', + 'guest-sync-delimited', + + # From qapi-schema-test: + 'user_def_cmd3', +] + enum_types = [] struct_types = [] union_types = [] @@ -350,10 +374,12 @@ def check_command(expr, expr_info): check_type(expr_info, "'data' for command '%s'" % name, expr.get('data'), allowed_metas=['union', 'struct'], allow_optional=True) + returns_meta = ['union', 'struct'] + if name in returns_whitelist: + returns_meta += ['built-in', 'alternate', 'enum'] check_type(expr_info, "'returns' for command '%s'" % name, expr.get('returns'), allow_array=True, - allowed_metas=['built-in', 'union', 'alternate', 'struct', - 'enum'], allow_optional=True) + allowed_metas=returns_meta, allow_optional=True) def check_event(expr, expr_info): global events diff --git a/tests/qapi-schema/returns-alternate.err b/tests/qapi-schema/returns-alternate.err index e69de29..dfbb419 100644 --- a/tests/qapi-schema/returns-alternate.err +++ b/tests/qapi-schema/returns-alternate.err @@ -0,0 +1 @@ +tests/qapi-schema/returns-alternate.json:3: 'returns' for command 'oops' cannot use alternate type 'Alt' diff --git a/tests/qapi-schema/returns-alternate.exit b/tests/qapi-schema/returns-alternate.exit index 573541a..d00491f 100644 --- a/tests/qapi-schema/returns-alternate.exit +++ b/tests/qapi-schema/returns-alternate.exit @@ -1 +1 @@ -0 +1 diff --git a/tests/qapi-schema/returns-alternate.json b/tests/qapi-schema/returns-alternate.json index b3b91fd..972390c 100644 --- a/tests/qapi-schema/returns-alternate.json +++ b/tests/qapi-schema/returns-alternate.json @@ -1,3 +1,3 @@ -# FIXME: we should reject returns if it is an alternate type +# we reject returns if it is an alternate type { 'alternate': 'Alt', 'data': { 'a': 'int', 'b': 'str' } } { 'command': 'oops', 'returns': 'Alt' } diff --git a/tests/qapi-schema/returns-alternate.out b/tests/qapi-schema/returns-alternate.out index 8a03ed3..e69de29 100644 --- a/tests/qapi-schema/returns-alternate.out +++ b/tests/qapi-schema/returns-alternate.out @@ -1,4 +0,0 @@ -[OrderedDict([('alternate', 'Alt'), ('data', OrderedDict([('a', 'int'), ('b', 'str')]))]), - OrderedDict([('command', 'oops'), ('returns', 'Alt')])] -[{'enum_name': 'AltKind', 'enum_values': None}] -[] diff --git a/tests/qapi-schema/returns-int.json b/tests/qapi-schema/returns-int.json index 7888fb1..870ec63 100644 --- a/tests/qapi-schema/returns-int.json +++ b/tests/qapi-schema/returns-int.json @@ -1,2 +1,3 @@ # It is okay (although not extensible) to return a non-dictionary -{ 'command': 'okay', 'returns': 'int' } +# But to make it work, the name must be in a whitelist +{ 'command': 'guest-get-time', 'returns': 'int' } diff --git a/tests/qapi-schema/returns-int.out b/tests/qapi-schema/returns-int.out index 36b00a9..70b3ac5 100644 --- a/tests/qapi-schema/returns-int.out +++ b/tests/qapi-schema/returns-int.out @@ -1,3 +1,3 @@ -[OrderedDict([('command', 'okay'), ('returns', 'int')])] +[OrderedDict([('command', 'guest-get-time'), ('returns', 'int')])] [] [] diff --git a/tests/qapi-schema/returns-whitelist.err b/tests/qapi-schema/returns-whitelist.err index e69de29..f47c1ee 100644 --- a/tests/qapi-schema/returns-whitelist.err +++ b/tests/qapi-schema/returns-whitelist.err @@ -0,0 +1 @@ +tests/qapi-schema/returns-whitelist.json:10: 'returns' for command 'no-way-this-will-get-whitelisted' cannot use built-in type 'int' diff --git a/tests/qapi-schema/returns-whitelist.exit b/tests/qapi-schema/returns-whitelist.exit index 573541a..d00491f 100644 --- a/tests/qapi-schema/returns-whitelist.exit +++ b/tests/qapi-schema/returns-whitelist.exit @@ -1 +1 @@ -0 +1 diff --git a/tests/qapi-schema/returns-whitelist.json b/tests/qapi-schema/returns-whitelist.json index 8328563..e8b3cea 100644 --- a/tests/qapi-schema/returns-whitelist.json +++ b/tests/qapi-schema/returns-whitelist.json @@ -1,4 +1,4 @@ -# FIXME: we should enforce that 'returns' be a dict or array of dict unless whitelisted +# we enforce that 'returns' be a dict or array of dict unless whitelisted { 'command': 'human-monitor-command', 'data': {'command-line': 'str', '*cpu-index': 'int'}, 'returns': 'str' } diff --git a/tests/qapi-schema/returns-whitelist.out b/tests/qapi-schema/returns-whitelist.out index 2adcd8b..e69de29 100644 --- a/tests/qapi-schema/returns-whitelist.out +++ b/tests/qapi-schema/returns-whitelist.out @@ -1,7 +0,0 @@ -[OrderedDict([('command', 'human-monitor-command'), ('data', OrderedDict([('command-line', 'str'), ('*cpu-index', 'int')])), ('returns', 'str')]), - OrderedDict([('enum', 'TpmModel'), ('data', ['tpm-tis'])]), - OrderedDict([('command', 'query-tpm-models'), ('returns', ['TpmModel'])]), - OrderedDict([('command', 'guest-get-time'), ('returns', 'int')]), - OrderedDict([('command', 'no-way-this-will-get-whitelisted'), ('returns', ['int'])])] -[{'enum_name': 'TpmModel', 'enum_values': ['tpm-tis']}] -[]
...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> --- scripts/qapi.py | 30 ++++++++++++++++++++++++++++-- tests/qapi-schema/returns-alternate.err | 1 + tests/qapi-schema/returns-alternate.exit | 2 +- tests/qapi-schema/returns-alternate.json | 2 +- tests/qapi-schema/returns-alternate.out | 4 ---- tests/qapi-schema/returns-int.json | 3 ++- tests/qapi-schema/returns-int.out | 2 +- tests/qapi-schema/returns-whitelist.err | 1 + tests/qapi-schema/returns-whitelist.exit | 2 +- tests/qapi-schema/returns-whitelist.json | 2 +- tests/qapi-schema/returns-whitelist.out | 7 ------- 11 files changed, 37 insertions(+), 19 deletions(-)