Re: [Qemu-devel] [PATCH v4 10/19] qapi: Better error messages for duplicated expressions

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> The previous commit demonstrated that the generator overlooked
> duplicate expressions:
> - a complex type reusing a built-in type name
> - redeclaration of a type name, whether by the same or different
> metatype
> - redeclaration of a command or event
> - lack of tracking of 'size' as a built-in type
>
> Add a global array to track all known types and their source,
> as well as enhancing check_exprs to also check for duplicate
> events and commands.  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                          | 71 
> +++++++++++++++++++++++++-------
>  tests/qapi-schema/redefined-builtin.err  |  1 +
>  tests/qapi-schema/redefined-builtin.exit |  2 +-
>  tests/qapi-schema/redefined-builtin.json |  2 +-
>  tests/qapi-schema/redefined-builtin.out  |  3 --
>  tests/qapi-schema/redefined-command.err  |  1 +
>  tests/qapi-schema/redefined-command.exit |  2 +-
>  tests/qapi-schema/redefined-command.json |  2 +-
>  tests/qapi-schema/redefined-command.out  |  4 --
>  tests/qapi-schema/redefined-event.err    |  1 +
>  tests/qapi-schema/redefined-event.exit   |  2 +-
>  tests/qapi-schema/redefined-event.json   |  2 +-
>  tests/qapi-schema/redefined-event.out    |  4 --
>  tests/qapi-schema/redefined-type.err     |  1 +
>  tests/qapi-schema/redefined-type.exit    |  2 +-
>  tests/qapi-schema/redefined-type.json    |  2 +-
>  tests/qapi-schema/redefined-type.out     |  4 --
>  17 files changed, 69 insertions(+), 37 deletions(-)
>
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index 8fbc45f..bf243fa 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -19,8 +19,15 @@ import sys
>  builtin_types = [
>      'str', 'int', 'number', 'bool',
>      'int8', 'int16', 'int32', 'int64',
> -    'uint8', 'uint16', 'uint32', 'uint64'
> +    'uint8', 'uint16', 'uint32', 'uint64',
> +    'size'
>  ]

I figure we want a blank line here.

Adding 'size' should have the following effects:

* Type sizeList is generated into qapi-types.h

* Declaration of qapi_free_sizeList() is generated into qapi-types.h,
  definition into qapi-types.c

* generate_visit_anon_union() no longer fails an assertion when it runs
  into a member of type 'size'

* Declaration of visit_type_sizeList() is generated into qapi-visit.h,
  definition into qapi-visit.c

Make sense.

How can we be sure we now got all built-in types covered?  Documentation
says yes, but it cannot be trusted.  I figure the best evidence we have
is c_type().  Looks good.

Aside: have a look at how it recognizes event names: "name ==
name.upper()".  Ugh!  I guess this patch lets us clean it up to "name in
events".

I think you should add 'size' to builtin_type_qtypes[], too.

> +enum_types = []
> +struct_types = []
> +union_types = []

Semi-related code motion.  Okay.

> +all_types = {}
> +commands = []
> +events = ['MAX']
>
>  builtin_type_qtypes = {
>      'str':      'QTYPE_QSTRING',
> @@ -248,8 +255,22 @@ def discriminator_find_enum_define(expr):
>
>      return find_enum(discriminator_type)
>
> +def check_command(expr, expr_info):
> +    global commands
> +    name = expr['command']
> +    if name in commands:
> +        raise QAPIExprError(expr_info,
> +                            "command '%s' is already defined" % name)
> +    commands.append(name)
> +

This rejects duplicate commands.  Good.

>  def check_event(expr, expr_info):
> +    global events
> +    name = expr['event']
>      params = expr.get('data')
> +    if name in events:
> +        raise QAPIExprError(expr_info,
> +                            "event '%s' is already defined" % name)
> +    events.append(name)
>      if params:
>          for argname, argentry, optional, structured in parse_args(params):
>              if structured:

This rejects duplicate events.  Good.

> @@ -347,6 +368,8 @@ def check_exprs(schema):
>              check_event(expr, info)
>          if expr.has_key('enum'):
>              check_enum(expr, info)
> +        if expr.has_key('command'):
> +            check_command(expr, info)
>
>  def check_keys(expr_elem, meta, required, optional=[]):
>      expr = expr_elem['expr']
> @@ -370,6 +393,9 @@ def check_keys(expr_elem, meta, required, optional=[]):
>
>
>  def parse_schema(input_file):
> +    global all_types
> +    exprs = []
> +
>      # First pass: read entire file into memory
>      try:
>          schema = QAPISchema(open(input_file, "r"))
> @@ -377,16 +403,17 @@ def parse_schema(input_file):
>          print >>sys.stderr, e
>          exit(1)
>
> -    exprs = []
> -
>      try:
>          # Next pass: learn the types and check for valid expression keys. At
>          # this point, top-level 'include' has already been flattened.
> +        for builtin in builtin_types:
> +            all_types[builtin] = 'built-in'
>          for expr_elem in schema.exprs:
>              expr = expr_elem['expr']
> +            info = expr_elem['info']
>              if expr.has_key('enum'):
>                  check_keys(expr_elem, 'enum', ['data'])
> -                add_enum(expr['enum'], expr['data'])
> +                add_enum(expr['enum'], info, expr['data'])
>              elif expr.has_key('union'):
>                  # Two styles of union, based on discriminator
>                  discriminator = expr.get('discriminator')
> @@ -395,10 +422,10 @@ def parse_schema(input_file):
>                  else:
>                      check_keys(expr_elem, 'union', ['data'],
>                                 ['base', 'discriminator'])
> -                add_union(expr)
> +                add_union(expr, info)
>              elif expr.has_key('type'):
>                  check_keys(expr_elem, 'type', ['data'], ['base'])
> -                add_struct(expr)
> +                add_struct(expr, info)
>              elif expr.has_key('command'):
>                  check_keys(expr_elem, 'command', [],
>                             ['data', 'returns', 'gen', 'success-response'])
> @@ -414,7 +441,7 @@ def parse_schema(input_file):
>              expr = expr_elem['expr']
>              if expr.has_key('union'):
>                  if not discriminator_find_enum_define(expr):
> -                    add_enum('%sKind' % expr['union'])
> +                    add_enum('%sKind' % expr['union'], expr_elem['info'])
>
>          # Final pass - validate that exprs make sense
>          check_exprs(schema)
> @@ -508,12 +535,15 @@ def type_name(name):
>          return c_list_type(name[0])
>      return name
>
> -enum_types = []
> -struct_types = []
> -union_types = []
> -
> -def add_struct(definition):
> +def add_struct(definition, info):
>      global struct_types
> +    global all_types
> +    name = definition['type']
> +    if name in all_types:
> +        raise QAPIExprError(info,
> +                            "%s '%s' is already defined"
> +                            %(all_types[name], name))
> +    all_types[name] = 'struct'
>      struct_types.append(definition)
>
>  def find_struct(name):
> @@ -523,8 +553,15 @@ def find_struct(name):
>              return struct
>      return None
>
> -def add_union(definition):
> +def add_union(definition, info):
>      global union_types
> +    global all_types
> +    name = definition['union']
> +    if name in all_types:
> +        raise QAPIExprError(info,
> +                            "%s '%s' is already defined"
> +                            %(all_types[name], name))
> +    all_types[name] = 'union'
>      union_types.append(definition)
>
>  def find_union(name):
> @@ -534,8 +571,14 @@ def find_union(name):
>              return union
>      return None
>
> -def add_enum(name, enum_values = None):
> +def add_enum(name, info, enum_values = None):
>      global enum_types
> +    global all_types
> +    if name in all_types:
> +        raise QAPIExprError(info,
> +                            "%s '%s' is already defined"
> +                            %(all_types[name], name))
> +    all_types[name] = 'enum'
>      enum_types.append({"enum_name": name, "enum_values": enum_values})
>
>  def find_enum(name):

These hunks reject duplicate types of any kind: enum, complex
(a.k.a. struct), union.

We have separate name spaces for events, commands and types.  Works for
me.  A single name space would work for me, too.

[Tests snipped, they look good...]