Re: [Qemu-devel] [PATCH v4 06/19] qapi: Better error messages for bad enums

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> The previous commit demonstrated that the generator had several
> flaws with less-than-perfect enums:
> - an enum that listed the same string twice (or two variant
> strings that map to the same C enum) ended up generating an
> invalid C enum
> - because the generator adds a _MAX terminator to each enum,
> the use of an enum member 'max' can also cause this clash
> - if an enum omits 'data', the generator left a python stack
> trace rather than a graceful message
> - an enum used a non-array 'data' was silently accepted by
> the parser
> - an enum that used non-string members in the 'data' member
> was silently accepted by the parser
>
> Add check_enum to cover these situations, and update testcases
> to match.  While valid .json files won't trigger any of these
> cases, we might as well be nicer to developers that make a typo
> while trying to add new QAPI code.
>
> Signed-off-by: Eric Blake <address@hidden>
> ---
>  scripts/qapi.py                          | 32 
> ++++++++++++++++++++++++++++----
>  tests/qapi-schema/enum-clash-member.err  |  1 +
>  tests/qapi-schema/enum-clash-member.exit |  2 +-
>  tests/qapi-schema/enum-clash-member.json |  2 +-
>  tests/qapi-schema/enum-clash-member.out  |  3 ---
>  tests/qapi-schema/enum-dict-member.err   |  1 +
>  tests/qapi-schema/enum-dict-member.exit  |  2 +-
>  tests/qapi-schema/enum-dict-member.json  |  2 +-
>  tests/qapi-schema/enum-dict-member.out   |  3 ---
>  tests/qapi-schema/enum-max-member.err    |  1 +
>  tests/qapi-schema/enum-max-member.exit   |  2 +-
>  tests/qapi-schema/enum-max-member.json   |  4 ++--
>  tests/qapi-schema/enum-max-member.out    |  3 ---
>  tests/qapi-schema/enum-missing-data.err  |  7 +------
>  tests/qapi-schema/enum-missing-data.json |  2 +-
>  tests/qapi-schema/enum-wrong-data.err    |  1 +
>  tests/qapi-schema/enum-wrong-data.exit   |  2 +-
>  tests/qapi-schema/enum-wrong-data.json   |  2 +-
>  tests/qapi-schema/enum-wrong-data.out    |  3 ---
>  19 files changed, 43 insertions(+), 32 deletions(-)
>
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index 77d46aa..85aa8bf 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -2,7 +2,7 @@
>  # QAPI helper library
>  #
>  # Copyright IBM, Corp. 2011
> -# Copyright (c) 2013 Red Hat Inc.
> +# Copyright (c) 2013-2014 Red Hat Inc.
>  #
>  # Authors:
>  #  Anthony Liguori <address@hidden>
> @@ -316,13 +316,37 @@ def check_union(expr, expr_info):
>          # Todo: add checking for values. Key is checked as above, value can 
> be
>          # also checked here, but we need more functions to handle array case.
>
> +def check_enum(expr, expr_info):
> +    name = expr['enum']
> +    members = expr.get('data')
> +    values = { 'MAX': '(automatic)' }
> +
> +    if not isinstance(members, list):
> +        raise QAPIExprError(expr_info,
> +                            "enum '%s' requires an array for 'data'" % name)
> +    for member in members:
> +        if not isinstance(member, basestring):
> +            raise QAPIExprError(expr_info,
> +                                "enum '%s' member '%s' is not a string"
> +                                % (name, member))
> +        key = _generate_enum_string(member)
> +        if key in values:
> +            raise QAPIExprError(expr_info,
> +                                "enum '%s' member '%s' clashes with '%s'"
> +                                % (name, member, values[key]))
> +        values[key] = member
> +
>  def check_exprs(schema):
>      for expr_elem in schema.exprs:
>          expr = expr_elem['expr']
> +        info = expr_elem['info']
> +
>          if expr.has_key('union'):
> -            check_union(expr, expr_elem['info'])
> +            check_union(expr, info)
>          if expr.has_key('event'):
> -            check_event(expr, expr_elem['info'])
> +            check_event(expr, info)
> +        if expr.has_key('enum'):
> +            check_enum(expr, info)
>
>  def parse_schema(input_file):
>      try:
> @@ -336,7 +360,7 @@ def parse_schema(input_file):
>      for expr_elem in schema.exprs:
>          expr = expr_elem['expr']
>          if expr.has_key('enum'):
> -            add_enum(expr['enum'], expr['data'])
> +            add_enum(expr['enum'], expr.get('data'))
>          elif expr.has_key('union'):
>              add_union(expr)
>          elif expr.has_key('type'):
> diff --git a/tests/qapi-schema/enum-clash-member.err 
> b/tests/qapi-schema/enum-clash-member.err
> index e69de29..3731fc2 100644
> --- a/tests/qapi-schema/enum-clash-member.err
> +++ b/tests/qapi-schema/enum-clash-member.err
> @@ -0,0 +1 @@
> +tests/qapi-schema/enum-clash-member.json:2: enum 'MyEnum' member 'ONE' 
> clashes with 'one'
> diff --git a/tests/qapi-schema/enum-clash-member.exit 
> b/tests/qapi-schema/enum-clash-member.exit
> index 573541a..d00491f 100644
> --- a/tests/qapi-schema/enum-clash-member.exit
> +++ b/tests/qapi-schema/enum-clash-member.exit
> @@ -1 +1 @@
> -0
> +1
> diff --git a/tests/qapi-schema/enum-clash-member.json 
> b/tests/qapi-schema/enum-clash-member.json
> index cb4b428..c668ff5 100644
> --- a/tests/qapi-schema/enum-clash-member.json
> +++ b/tests/qapi-schema/enum-clash-member.json
> @@ -1,2 +1,2 @@
> -# FIXME: we should reject enums where members will clash in C switch
> +# we reject enums where members will clash in C switch
>  { 'enum': 'MyEnum', 'data': [ 'one', 'ONE' ] }

Actually in PATCH 05 already: "clash in C switch".  I guess you mean we
generate an enum with clashing enumeration constants.  In the test case,
we'd generate MY_ENUM_ONE (I think) both for 'one' and 'ONE'.  Correct?

> diff --git a/tests/qapi-schema/enum-clash-member.out 
> b/tests/qapi-schema/enum-clash-member.out
> index 0814459..e69de29 100644
> --- a/tests/qapi-schema/enum-clash-member.out
> +++ b/tests/qapi-schema/enum-clash-member.out
> @@ -1,3 +0,0 @@
> -[OrderedDict([('enum', 'MyEnum'), ('data', ['one', 'ONE'])])]
> -[{'enum_name': 'MyEnum', 'enum_values': ['one', 'ONE']}]
> -[]
> diff --git a/tests/qapi-schema/enum-dict-member.err 
> b/tests/qapi-schema/enum-dict-member.err
> index e69de29..f5a8ffe 100644
> --- a/tests/qapi-schema/enum-dict-member.err
> +++ b/tests/qapi-schema/enum-dict-member.err
> @@ -0,0 +1 @@
> +tests/qapi-schema/enum-dict-member.json:2: enum 'MyEnum' member 
> 'OrderedDict([('value', 'str')])' is not a string

Error message is in terms of implementation instead of source.  Since
this is merely a development tool, it'll do.  Same elsewhere, and I'm
not going to flag it.  Precedents in master quite possible.

> diff --git a/tests/qapi-schema/enum-dict-member.exit 
> b/tests/qapi-schema/enum-dict-member.exit
> index 573541a..d00491f 100644
> --- a/tests/qapi-schema/enum-dict-member.exit
> +++ b/tests/qapi-schema/enum-dict-member.exit
> @@ -1 +1 @@
> -0
> +1
> diff --git a/tests/qapi-schema/enum-dict-member.json 
> b/tests/qapi-schema/enum-dict-member.json
> index de4d6bf..79672e0 100644
> --- a/tests/qapi-schema/enum-dict-member.json
> +++ b/tests/qapi-schema/enum-dict-member.json
> @@ -1,2 +1,2 @@
> -# FIXME: we should reject any enum member that is not a string
> +# we reject any enum member that is not a string
>  { 'enum': 'MyEnum', 'data': [ { 'value': 'str' } ] }
> diff --git a/tests/qapi-schema/enum-dict-member.out 
> b/tests/qapi-schema/enum-dict-member.out
> index 8b293f8..e69de29 100644
> --- a/tests/qapi-schema/enum-dict-member.out
> +++ b/tests/qapi-schema/enum-dict-member.out
> @@ -1,3 +0,0 @@
> -[OrderedDict([('enum', 'MyEnum'), ('data', [OrderedDict([('value', 
> 'str')])])])]
> -[{'enum_name': 'MyEnum', 'enum_values': [OrderedDict([('value', 'str')])]}]
> -[]
> diff --git a/tests/qapi-schema/enum-max-member.err 
> b/tests/qapi-schema/enum-max-member.err
> index e69de29..a6c5db1 100644
> --- a/tests/qapi-schema/enum-max-member.err
> +++ b/tests/qapi-schema/enum-max-member.err
> @@ -0,0 +1 @@
> +tests/qapi-schema/enum-max-member.json:3: enum 'MyEnum' member 'max' clashes 
> with '(automatic)'
> diff --git a/tests/qapi-schema/enum-max-member.exit 
> b/tests/qapi-schema/enum-max-member.exit
> index 573541a..d00491f 100644
> --- a/tests/qapi-schema/enum-max-member.exit
> +++ b/tests/qapi-schema/enum-max-member.exit
> @@ -1 +1 @@
> -0
> +1
> diff --git a/tests/qapi-schema/enum-max-member.json 
> b/tests/qapi-schema/enum-max-member.json
> index ea854c4..6af4662 100644
> --- a/tests/qapi-schema/enum-max-member.json
> +++ b/tests/qapi-schema/enum-max-member.json
> @@ -1,3 +1,3 @@
> -# FIXME: we should either reject user-supplied 'max', or munge the implicit
> -# max value we generate at the end of an array
> +# we reject user-supplied 'max' for clashing with implicit enum end
> +# FIXME: should we instead munge the the implicit value to avoid the clash?

Or pick an identifier for the max member so that it cannot clash with
the ones we generate for the user's members.

The generator picks identifiers pretty much thoughtlessly in general.
If something clashes, the C compiler spits it out, and you get to fiddle
with the .json.

[...]

Reviewed-by: Markus Armbruster <address@hidden>