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>

diff --git a/scripts/qapi.py b/scripts/qapi.py
index 5bc32e311d844d8f5e1af4bba039dbec76ffa937..2402d053e4c6ae17159c68b9863a495ebd53ae65 100644 (file)
--- 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 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391..dfbb419cac287e386e90fef2addec8f57ce35858 100644 (file)
--- 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 573541ac9702dd3969c9bc859d2b91ec1f7e6e56..d00491fd7e5bb6fa28c517a0bb32b8b506539d4d 100644 (file)
--- 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 b3b91fd3d4fe80e6b35ce0c50cfcc58ab5d43129..972390c06b0825a8fe57cfc7d49a3df13ed4dcc9 100644 (file)
--- 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 8a03ed3adf0dfc53843ada385e304e80f71eb81a..e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 100644 (file)
--- 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 7888fb1b082b7f83104453d9a7aa757ba457eb3d..870ec6366be398646c236be6a35bb7aea72c1d81 100644 (file)
--- 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 36b00a9f3b626a608ce9279d0c0d562366eb1031..70b3ac5e6fe52f12b552dd5e5db0febb04243fa2 100644 (file)
--- 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 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391..a41f019a52e445e5a7087a275eabfe5ce9039c30 100644 (file)
--- 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 573541ac9702dd3969c9bc859d2b91ec1f7e6e56..d00491fd7e5bb6fa28c517a0bb32b8b506539d4d 100644 (file)
--- 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 8328563b4e269c1f8dea38a0af6d1b1eeb673007..e8b3cea39613c0f5cea18e72384f22ed17d6fd27 100644 (file)
--- 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 2adcd8ba10763f2cbd1db001c6f428611421ca11..e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 100644 (file)
--- 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']}]
-[]