[Qemu-devel] [PATCH v5 22/28] qapi: Whitelist commands that don't return dictionary

[Eric Blake <eblake@redhat.com>]

...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',
+]
+
 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']}]
-[]
-- 
2.1.0