Re: [Qemu-devel] [PATCH v3 19/49] qapi: factor out check_known_keys()

[Markus Armbruster]

Marc-André Lureau <address@hidden> writes:

> The following patches are going to need similar checks from various
> code path. This refactoring will report all conflicting keys (instead
> of the first one encountered).
>
> Modify unknown-expr-key to check plural form.
>
> Signed-off-by: Marc-André Lureau <address@hidden>
> ---
>  scripts/qapi/common.py                  | 27 ++++++++++++++++++-------
>  tests/qapi-schema/alternate-base.err    |  2 +-
>  tests/qapi-schema/double-type.err       |  2 +-
>  tests/qapi-schema/enum-missing-data.err |  2 +-
>  tests/qapi-schema/unknown-expr-key.err  |  2 +-
>  tests/qapi-schema/unknown-expr-key.json |  2 +-
>  6 files changed, 25 insertions(+), 12 deletions(-)
>
> diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
> index 4d19146064..fdbb5f1823 100644
> --- a/scripts/qapi/common.py
> +++ b/scripts/qapi/common.py
> @@ -879,6 +879,24 @@ def check_struct(expr, info):
>                 allow_metas=['struct'])
>  
>  
> +def check_known_keys(info, source, keys, required, optional):
> +
> +    def pprint(elems):
> +        return ', '.join("'" + e + "'" for e in sorted(elems))
> +
> +    missing = set(required) - set(keys)
> +    if missing:
> +        raise QAPISemError(info, "%s must have %s key%s"
> +                           % (source, pprint(missing),
> +                              's' if len(missing) > 1 else ''))
> +    allowed = set(required + optional)
> +    unknown = set(keys) - allowed
> +    if unknown:
> +        raise QAPISemError(info, "%s has unknown key%s %s (allowed: %s)"
> +                           % (source, 's' if len(unknown) > 1 else '',
> +                              pprint(unknown), pprint(allowed)))
> +
> +
>  def check_keys(expr_elem, meta, required, optional=[]):
>      expr = expr_elem['expr']
>      info = expr_elem['info']
> @@ -886,10 +904,9 @@ def check_keys(expr_elem, meta, required, optional=[]):
>      if not isinstance(name, str):
>          raise QAPISemError(info, "'%s' key must have a string value" % meta)
>      required = required + [meta]
> +    source = "%s '%s'" % (meta, name)
> +    check_known_keys(info, source, expr, required, optional)
>      for (key, value) in expr.items():
> -        if key not in required and key not in optional:
> -            raise QAPISemError(info, "Unknown key '%s' in %s '%s'"
> -                               % (key, meta, name))
>          if (key == 'gen' or key == 'success-response') and value is not 
> False:
>              raise QAPISemError(info,
>                                 "'%s' of %s '%s' should only use false value"
> @@ -900,10 +917,6 @@ def check_keys(expr_elem, meta, required, optional=[]):
>                                 % (key, meta, name))
>          if key == 'if':
>              check_if(expr, info)
> -    for key in required:
> -        if key not in expr:
> -            raise QAPISemError(info, "Key '%s' is missing from %s '%s'"
> -                               % (key, meta, name))
>  
>  
>  def check_exprs(exprs):
> diff --git a/tests/qapi-schema/alternate-base.err 
> b/tests/qapi-schema/alternate-base.err
> index 30d8a34373..2b09c4c7a3 100644
> --- a/tests/qapi-schema/alternate-base.err
> +++ b/tests/qapi-schema/alternate-base.err
> @@ -1 +1 @@
> -tests/qapi-schema/alternate-base.json:4: Unknown key 'base' in alternate 
> 'Alt'
> +tests/qapi-schema/alternate-base.json:4: alternate 'Alt' has unknown key 
> 'base' (allowed: 'alternate', 'data', 'if')

The change in the error message proper, i.e. up to the parenthesis,
doesn't feel like an improvment to me.  The old message starts with
what's wrong.  The new one starts with where's something wrong.

I'd do the parenthesis with error_append_hint(), and say something like
"Valid keys are ...".  See below for a more complete example.

> diff --git a/tests/qapi-schema/double-type.err 
> b/tests/qapi-schema/double-type.err
> index f9613c6d6b..a8c5637659 100644
> --- a/tests/qapi-schema/double-type.err
> +++ b/tests/qapi-schema/double-type.err
> @@ -1 +1 @@
> -tests/qapi-schema/double-type.json:2: Unknown key 'command' in struct 'bar'
> +tests/qapi-schema/double-type.json:2: struct 'bar' has unknown key 'command' 
> (allowed: 'base', 'data', 'if', 'struct')
> diff --git a/tests/qapi-schema/enum-missing-data.err 
> b/tests/qapi-schema/enum-missing-data.err
> index ba4873ae69..68e286badc 100644
> --- a/tests/qapi-schema/enum-missing-data.err
> +++ b/tests/qapi-schema/enum-missing-data.err
> @@ -1 +1 @@
> -tests/qapi-schema/enum-missing-data.json:2: Key 'data' is missing from enum 
> 'MyEnum'
> +tests/qapi-schema/enum-missing-data.json:2: enum 'MyEnum' must have 'data' 
> key

Again, the error message change doesn't feel like an improvement to me.

> diff --git a/tests/qapi-schema/unknown-expr-key.err 
> b/tests/qapi-schema/unknown-expr-key.err
> index 12f5ed5b43..d9f4e41cac 100644
> --- a/tests/qapi-schema/unknown-expr-key.err
> +++ b/tests/qapi-schema/unknown-expr-key.err
> @@ -1 +1 @@
> -tests/qapi-schema/unknown-expr-key.json:2: Unknown key 'bogus' in struct 
> 'bar'
> +tests/qapi-schema/unknown-expr-key.json:2: struct 'bar' has unknown keys 
> 'bogus', 'foo' (allowed: 'base', 'data', 'if', 'struct')
> diff --git a/tests/qapi-schema/unknown-expr-key.json 
> b/tests/qapi-schema/unknown-expr-key.json

Suggest something like

    tests/qapi-schema/unknown-expr-key.json:2: Unknown keys 'bogus', 'foo' in 
struct 'bar'
    Valid keys are 'base', 'data', 'if', 'struct'.

> index 3b2be00cc4..5bcb8efd1d 100644
> --- a/tests/qapi-schema/unknown-expr-key.json
> +++ b/tests/qapi-schema/unknown-expr-key.json
> @@ -1,2 +1,2 @@
>  # we reject an expression with unknown top-level keys
> -{ 'struct': 'bar', 'data': { 'string': 'str'}, 'bogus': { } }
> +{ 'struct': 'bar', 'data': { 'string': 'str'}, 'bogus': { }, 'foo': { } }

The patch does two things: 1. factor out a helper for later use in other
places, 2. report all offending keys instead of just the first one.
Split it up, please.