Re: [Qemu-devel] [PATCH v5 16/28] 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 or command reusing a built-in type name
> - redeclaration of a type name, whether by the same or different
> metatype
> - redeclaration of a command or event
> - collision of a type with implicit 'Kind' enum for a union
> - collision with an implicit MAX enum constant, or with various
> case spellings of events
>
> Since the c_type() function in the generator treats all names
> as being in the same namespace, this patch adds a global array
> to track all known names and their source, to prevent collisions
> before it can cause further problems.  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                          | 68 
> +++++++++++++++++++++++++-------
>  tests/qapi-schema/bad-type-dict.err      |  2 +-
>  tests/qapi-schema/command-int.err        |  1 +
>  tests/qapi-schema/command-int.exit       |  2 +-
>  tests/qapi-schema/command-int.json       |  2 +-
>  tests/qapi-schema/command-int.out        |  3 --
>  tests/qapi-schema/enum-union-clash.err   |  1 +
>  tests/qapi-schema/enum-union-clash.exit  |  2 +-
>  tests/qapi-schema/enum-union-clash.json  |  2 +-
>  tests/qapi-schema/enum-union-clash.out   |  5 ---
>  tests/qapi-schema/event-case.err         |  1 +
>  tests/qapi-schema/event-case.exit        |  2 +-
>  tests/qapi-schema/event-case.json        |  2 +-
>  tests/qapi-schema/event-case.out         |  3 --
>  tests/qapi-schema/event-max.err          |  1 +
>  tests/qapi-schema/event-max.exit         |  2 +-
>  tests/qapi-schema/event-max.json         |  2 +-
>  tests/qapi-schema/event-max.out          |  3 --
>  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 --
>  34 files changed, 78 insertions(+), 61 deletions(-)
>
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index 90eb3bc..5d0dc91 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -32,6 +32,12 @@ builtin_types = {
>      'size':     'QTYPE_QINT',
>  }
>
> +enum_types = []
> +struct_types = []
> +union_types = []
> +events = []
> +all_names = {}
> +
>  def error_path(parent):
>      res = ""
>      while parent:
> @@ -256,7 +262,17 @@ def discriminator_find_enum_define(expr):
>      return find_enum(discriminator_type)
>
>  def check_event(expr, expr_info):
> +    global events
> +    name = expr['event']
>      params = expr.get('data')
> +
> +    if name == 'MAX':
> +        raise QAPIExprError(expr_info, "Event name 'MAX' cannot be created")
> +    if name != name.upper():
> +        raise QAPIExprError(expr_info, "Event name '%s' should be upper case"
> +                            % name)
> +    events.append(name)
> +
>      if params:
>          for argname, argentry, optional, structured in parse_args(params):
>              if structured:
> @@ -408,7 +424,7 @@ def check_keys(expr_elem, meta, required, optional=[]):
>      name = expr[meta]
>      if not isinstance(name, str):
>          raise QAPIExprError(info,
> -                            "%s key must have a string value" % meta)
> +                            "'%s' key must have a string value" % meta)
>      required = required + [ meta ]
>      for (key, value) in expr.items():
>          if not key in required and not key in optional:

Let's squash this hunk into PATCH 14.

> @@ -423,6 +439,9 @@ def check_keys(expr_elem, meta, required, optional=[]):
>
>
>  def parse_schema(input_file):
> +    global all_names
> +    exprs = []
> +
>      # First pass: read entire file into memory
>      try:
>          schema = QAPISchema(open(input_file, "r"))
> @@ -430,30 +449,34 @@ 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.keys():
> +            all_names[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'):
>                  check_keys(expr_elem, 'union', ['data'],
>                             ['base', 'discriminator'])
> -                add_union(expr)
> +                add_union(expr, info)
>              elif expr.has_key('alternate'):
>                  check_keys(expr_elem, 'alternate', ['data'])
> +                add_name(expr['alternate'], info, 'alternate')
>              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'])
> +                add_name(expr['command'], info, 'command')
>              elif expr.has_key('event'):
>                  check_keys(expr_elem, 'event', [], ['data'])
> +                add_name(expr['event'], info, 'event')
>              else:
>                  raise QAPIExprError(expr_elem['info'],
>                                      "Expression is missing metatype")
> @@ -464,9 +487,11 @@ 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'],
> +                             implicit=True)
>              elif expr.has_key('alternate'):
> -                add_enum('%sKind' % expr['alternate'])
> +                add_enum('%sKind' % expr['alternate'], expr_elem['info'],
> +                         implicit=True)
>
>          # Final pass - validate that exprs make sense
>          check_exprs(schema)
> @@ -560,12 +585,22 @@ def type_name(name):
>          return c_list_type(name[0])
>      return name
>
> -enum_types = []
> -struct_types = []
> -union_types = []
> +def add_name(name, info, meta, implicit = False):
> +    global all_names
> +    if name in all_names:
> +        raise QAPIExprError(info,
> +                            "%s '%s' is already defined"
> +                            %(all_names[name], name))
> +    if not implicit and name[-4:] == 'Kind':
> +        raise QAPIExprError(info,
> +                            "%s '%s' should not end in 'Kind'"
> +                            %(meta, name))
> +    all_names[name] = meta
>
> -def add_struct(definition):
> +def add_struct(definition, info):
>      global struct_types
> +    name = definition['type']
> +    add_name(name, info, 'struct')
>      struct_types.append(definition)
>
>  def find_struct(name):
> @@ -575,8 +610,10 @@ def find_struct(name):
>              return struct
>      return None
>
> -def add_union(definition):
> +def add_union(definition, info):
>      global union_types
> +    name = definition['union']
> +    add_name(name, info, 'union')
>      union_types.append(definition)
>
>  def find_union(name):
> @@ -586,8 +623,9 @@ def find_union(name):
>              return union
>      return None
>
> -def add_enum(name, enum_values = None):
> +def add_enum(name, info, enum_values = None, implicit = False):
>      global enum_types
> +    add_name(name, info, 'enum', implicit)
>      enum_types.append({"enum_name": name, "enum_values": enum_values})
>
>  def find_enum(name):
> @@ -629,7 +667,7 @@ def c_type(name, is_param=False):
>          return name
>      elif name == None or len(name) == 0:
>          return 'void'
> -    elif name == name.upper():
> +    elif name in events:
>          return '%sEvent *%s' % (camel_case(name), eatspace)
>      else:
>          return '%s *%s' % (name, eatspace)

One instance kitten-killing grossness down :)

[Tests look good...]

Since the misplaced hunk is just polish:

Reviewed-by: Markus Armbruster <address@hidden>