[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Qemu-devel] [PULL 26/40] qapi: Whitelist commands that don't return dic
From: |
Markus Armbruster |
Subject: |
[Qemu-devel] [PULL 26/40] qapi: Whitelist commands that don't return dictionary |
Date: |
Tue, 5 May 2015 18:47:12 +0200 |
From: Eric Blake <address@hidden>
...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 <address@hidden>
Reviewed-by: Markus Armbruster <address@hidden>
Signed-off-by: Markus Armbruster <address@hidden>
---
scripts/qapi.py | 31 ++++++++++++++++++++++++++++---
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(+), 20 deletions(-)
diff --git a/scripts/qapi.py b/scripts/qapi.py
index 5bc32e3..2402d05 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -32,6 +32,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 = []
@@ -354,11 +378,12 @@ def check_command(expr, expr_info):
check_type(expr_info, "'data' for command '%s'" % name,
expr.get('data'), allow_dict=True, allow_optional=True,
allow_metas=['union', 'struct'])
+ 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, allow_dict=True,
- allow_optional=True,
- allow_metas=['built-in', 'union', 'alternate', 'struct',
- 'enum'])
+ allow_optional=True, allow_metas=returns_meta)
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..a41f019 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 'array of 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']}]
-[]
--
1.9.3
- [Qemu-devel] [PULL 11/40] qapi: Tighten checking of unions, (continued)
- [Qemu-devel] [PULL 11/40] qapi: Tighten checking of unions, Markus Armbruster, 2015/05/05
- [Qemu-devel] [PULL 02/40] qapi: Document type-safety considerations, Markus Armbruster, 2015/05/05
- [Qemu-devel] [PULL 21/40] qapi: Allow true, false and null in schema json, Markus Armbruster, 2015/05/05
- [Qemu-devel] [PULL 19/40] qapi: Add tests of redefined expressions, Markus Armbruster, 2015/05/05
- [Qemu-devel] [PULL 27/40] qapi: More rigorous checking for type safety bypass, Markus Armbruster, 2015/05/05
- [Qemu-devel] [PULL 31/40] qapi: Forbid 'type' in schema, Markus Armbruster, 2015/05/05
- [Qemu-devel] [PULL 18/40] qapi: Better error messages for bad expressions, Markus Armbruster, 2015/05/05
- [Qemu-devel] [PULL 16/40] qapi: Use 'alternate' to replace anonymous union, Markus Armbruster, 2015/05/05
- [Qemu-devel] [PULL 22/40] qapi: Unify type bypass and add tests, Markus Armbruster, 2015/05/05
- [Qemu-devel] [PULL 17/40] qapi: Add some expr tests, Markus Armbruster, 2015/05/05
- [Qemu-devel] [PULL 26/40] qapi: Whitelist commands that don't return dictionary,
Markus Armbruster <=
- [Qemu-devel] [PULL 25/40] qapi: Require valid names, Markus Armbruster, 2015/05/05
- [Qemu-devel] [PULL 08/40] qapi: Add some union tests, Markus Armbruster, 2015/05/05
- [Qemu-devel] [PULL 20/40] qapi: Better error messages for duplicated expressions, Markus Armbruster, 2015/05/05
- [Qemu-devel] [PULL 24/40] qapi: More rigourous checking of types, Markus Armbruster, 2015/05/05
- [Qemu-devel] [PULL 29/40] qapi: Document 'struct' metatype, Markus Armbruster, 2015/05/05
- [Qemu-devel] [PULL 40/40] qapi: Check for member name conflicts with a base class, Markus Armbruster, 2015/05/05
- [Qemu-devel] [PULL 33/40] qapi: Drop tests for inline nested structs, Markus Armbruster, 2015/05/05
- [Qemu-devel] [PULL 35/40] qapi: Drop inline nested structs in query-pci, Markus Armbruster, 2015/05/05
- [Qemu-devel] [PULL 23/40] qapi: Add some type check tests, Markus Armbruster, 2015/05/05
- [Qemu-devel] [PULL 39/40] qapi: Support (subset of) \u escapes in strings, Markus Armbruster, 2015/05/05