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

[Markus Armbruster]

Eric Blake <address@hidden> 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 <address@hidden>
> ---
>  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
[...]

Thinking on introspection, I started to wonder whether there's actually
a command returning a union, yet.  So I applied the appended stupid
patch on top, and found the following commands returning (list of)
non-struct type:

* qapi-schema.json:

  'ringbuf-read'                built-in type 'str'
  'human-monitor-command'       built-in type 'str'
  'query-migrate-cache-size'    built-in type 'int'
  'query-tpm-models'            enum type 'TpmModel'
  'query-tpm-types'             enum type 'TpmType'
  'query-memory-devices'        union type 'MemoryDeviceInfo'

* qga/qapi-schema.json:

  'guest-sync-delimited'        built-in type 'int'
  'guest-sync'                  built-in type 'int'
  'guest-get-time'              built-in type 'int'
  'guest-file-open'             built-in type 'int'
  'guest-fsfreeze-status'       enum type 'GuestFsfreezeStatus'
  'guest-fsfreeze-freeze'       built-in type 'int'
  'guest-fsfreeze-freeze-list'  built-in type 'int'
  'guest-fsfreeze-thaw'         built-in type 'int'
  'guest-set-vcpus'             built-in type 'int'

The sole example for union is 'MemoryDeviceInfo'.  It has one case %-}


diff --git a/scripts/qapi.py b/scripts/qapi.py
index 8e5b4ad..c4c6a6e 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -353,9 +353,12 @@ def check_type(expr_info, source, data, allow_array = 
False,
                                 "%s uses unknown type '%s'"
                                 % (source, data))
         if not all_names[data] in allowed_metas:
-            raise QAPIExprError(expr_info,
-                                "%s cannot use %s type '%s'"
-                                % (source, all_names[data], data))
+#            raise QAPIExprError(expr_info,
+#                                "%s cannot use %s type '%s'"
+#                                % (source, all_names[data], data))
+            print >>sys.stderr, QAPIExprError(expr_info,
+                                            "%s cannot use %s type '%s'"
+                                            % (source, all_names[data], data))
         return
 
     # data is a dictionary, check that each member is okay
@@ -387,6 +390,7 @@ def check_command(expr, expr_info):
     returns_meta = ['union', 'struct']
     if name in returns_whitelist:
         returns_meta += ['built-in', 'alternate', 'enum']
+    returns_meta = ['struct']
     check_type(expr_info, "'returns' for command '%s'" % name,
                expr.get('returns'), allow_array=True,
                allowed_metas=returns_meta, allow_optional=True,