qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Markus Armbruster
Subject: Re: [Qemu-devel] [PATCH v5 22/28] qapi: Whitelist commands that don't return dictionary
Date: Fri, 27 Mar 2015 10:11:02 +0100
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux)

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',
> +]
> +

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 <address@hidden>



reply via email to

[Prev in Thread] Current Thread [Next in Thread]