Re: [Qemu-devel] [PATCH v5 10/28] qapi: Segregate anonymous unions into alternates in generator

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> Special-casing 'discriminator == {}' for handling anonymous unions
> is getting awkward; since this particular type is not always a
> dictionary on the wire, it is easier to treat it as a completely
> different class of type, "alternate", so that if a type is listed
> in the union_types array, we know it is not an anonymous union.
>
> This patch just further segregates union handling, to make sure that
> anonymous unions are not stored in union_types, and splitting up
> check_union() into separate functions.  A future patch will change
> the qapi grammar, and having the segregation already in place will
> make it easier to deal with the distinct meta-type.
>
> Signed-off-by: Eric Blake <address@hidden>
> ---
>  scripts/qapi-types.py |  6 ++--
>  scripts/qapi-visit.py |  4 +--
>  scripts/qapi.py       | 94 
> +++++++++++++++++++++++++++++----------------------
>  3 files changed, 58 insertions(+), 46 deletions(-)
>
> diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
> index 2390887..c9e0201 100644
> --- a/scripts/qapi-types.py
> +++ b/scripts/qapi-types.py
> @@ -170,7 +170,7 @@ typedef enum %(name)s
>
>      return lookup_decl + enum_decl
>
> -def generate_anon_union_qtypes(expr):
> +def generate_alternate_qtypes(expr):
>
>      name = expr['union']
>      members = expr['data']
> @@ -181,7 +181,7 @@ const int %(name)s_qtypes[QTYPE_MAX] = {
>      name=name)
>
>      for key in members:
> -        qtype = find_anonymous_member_qtype(members[key])
> +        qtype = find_alternate_member_qtype(members[key])
>          assert qtype, "Invalid anonymous union member"
>
>          ret += mcgen('''
> @@ -408,7 +408,7 @@ for expr in exprs:
>              fdef.write(generate_enum_lookup('%sKind' % expr['union'],
>                                              expr['data'].keys()))
>          if expr.get('discriminator') == {}:
> -            fdef.write(generate_anon_union_qtypes(expr))
> +            fdef.write(generate_alternate_qtypes(expr))
>      else:
>          continue
>      fdecl.write(ret)
> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
> index 3f82bd4..77b0a1f 100644
> --- a/scripts/qapi-visit.py
> +++ b/scripts/qapi-visit.py
> @@ -237,7 +237,7 @@ void visit_type_%(name)s(Visitor *m, %(name)s *obj, const 
> char *name, Error **er
>  ''',
>                   name=name)
>
> -def generate_visit_anon_union(name, members):
> +def generate_visit_alternate(name, members):
>      ret = mcgen('''
>
>  void visit_type_%(name)s(Visitor *m, %(name)s **obj, const char *name, Error 
> **errp)
> @@ -302,7 +302,7 @@ def generate_visit_union(expr):
>
>      if discriminator == {}:
>          assert not base
> -        return generate_visit_anon_union(name, members)
> +        return generate_visit_alternate(name, members)
>
>      enum_define = discriminator_find_enum_define(expr)
>      if enum_define:
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index 39cc88b..17252e9 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -224,21 +224,16 @@ def find_base_fields(base):
>          return None
>      return base_struct_define['data']
>
> -# Return the qtype of an anonymous union branch, or None on error.
> -def find_anonymous_member_qtype(qapi_type):
> +# Return the qtype of an alternate branch, or None on error.
> +def find_alternate_member_qtype(qapi_type):
>      if builtin_types.has_key(qapi_type):
>          return builtin_types[qapi_type]
>      elif find_struct(qapi_type):
>          return "QTYPE_QDICT"
>      elif find_enum(qapi_type):
>          return "QTYPE_QSTRING"
> -    else:
> -        union = find_union(qapi_type)
> -        if union:
> -            discriminator = union.get('discriminator')
> -            if discriminator == {}:
> -                return None
> -            return "QTYPE_QDICT"
> +    elif find_union(qapi_type):
> +        return "QTYPE_QDICT"
>      return None
>
>  # Return the discriminator enum define if discriminator is specified as an
> @@ -276,22 +271,13 @@ def check_union(expr, expr_info):
>      discriminator = expr.get('discriminator')
>      members = expr['data']
>      values = { 'MAX': '(automatic)' }
> -    types_seen = {}
>
> -    # Three types of unions, determined by discriminator.
> +    # Two types of unions, determined by discriminator.
> +    assert discriminator != {}
>
> -    # If the value of member 'discriminator' is {}, it's an
> -    # anonymous union, and must not have a base.
> -    if discriminator == {}:
> -        enum_define = None
> -        if base:
> -            raise QAPIExprError(expr_info,
> -                                "Anonymous union '%s' must not have a base"
> -                                % name)
> -
> -    # Else if the union object has no member 'discriminator', it's an
> +    # If the union object has no member 'discriminator', it's an
>      # ordinary union.  For now, it must not have a base.
> -    elif not discriminator:
> +    if not discriminator:
>          enum_define = None
>          if base:
>              raise QAPIExprError(expr_info,
> @@ -346,24 +332,46 @@ def check_union(expr, expr_info):
>                                      % (name, key, values[c_key]))
>              values[c_key] = key
>
> -        # Ensure anonymous unions have no type conflicts.
> -        if discriminator == {}:
> -            if isinstance(value, list):
> -                raise QAPIExprError(expr_info,
> -                                    "Anonymous union '%s' member '%s' must "
> -                                    "not be array type" % (name, key))
> -            qtype = find_anonymous_member_qtype(value)
> -            if not qtype:
> -                raise QAPIExprError(expr_info,
> -                                    "Anonymous union '%s' member '%s' has "
> -                                    "invalid type '%s'" % (name, key, value))
> -            if qtype in types_seen:
> -                raise QAPIExprError(expr_info,
> -                                    "Anonymous union '%s' member '%s' has "
> -                                    "same QObject type as member '%s'"
> -                                    % (name, key, types_seen[qtype]))
> -            types_seen[qtype] = key
> +def check_alternate(expr, expr_info):
> +    name = expr['union']
> +    base = expr.get('base')
> +    discriminator = expr.get('discriminator')
> +    members = expr['data']
> +    values = { 'MAX': '(automatic)' }
> +    types_seen = {}
>
> +    assert discriminator == {}
> +    if base:
> +        raise QAPIExprError(expr_info,
> +                            "Anonymous union '%s' must not have a base"
> +                            % name)
> +
> +    # Check every branch
> +    for (key, value) in members.items():
> +        # Check for conflicts in the generated enum
> +        c_key = _generate_enum_string(key)
> +        if c_key in values:
> +            raise QAPIExprError(expr_info,
> +                                "Union '%s' member '%s' clashes with '%s'"
> +                                % (name, key, values[c_key]))
> +        values[c_key] = key
> +
> +        # Ensure alternates have no type conflicts.
> +        if isinstance(value, list):
> +            raise QAPIExprError(expr_info,
> +                                "Anonymous union '%s' member '%s' must "
> +                                "not be array type" % (name, key))
> +        qtype = find_alternate_member_qtype(value)
> +        if not qtype:
> +            raise QAPIExprError(expr_info,
> +                                "Anonymous union '%s' member '%s' has "
> +                                "invalid type '%s'" % (name, key, value))
> +        if qtype in types_seen:
> +            raise QAPIExprError(expr_info,
> +                                "Anonymous union '%s' member '%s' has "
> +                                "same QObject type as member '%s'"
> +                                % (name, key, types_seen[qtype]))
> +        types_seen[qtype] = key
>
>  def check_enum(expr, expr_info):
>      name = expr['enum']
> @@ -393,7 +401,10 @@ def check_exprs(schema):
>          if expr.has_key('enum'):
>              check_enum(expr, info)
>          elif expr.has_key('union'):
> -            check_union(expr, info)
> +            if expr.get('discriminator') == {}:
> +                check_alternate(expr, info)
> +            else:
> +                check_union(expr, info)
>          elif expr.has_key('event'):
>              check_event(expr, info)
>
> @@ -535,7 +546,8 @@ def find_struct(name):
>
>  def add_union(definition):
>      global union_types
> -    union_types.append(definition)
> +    if definition.get('discriminator') != {}:
> +        union_types.append(definition)
>
>  def find_union(name):
>      global union_types

This is the only unobvious hunk.

union_types is used only through find_union.  The hunk makes
find_union(N) return None when N names an anonymous union.

find_union() is used in two places:

* find_alternate_member_qtype()

  Patched further up.  It really wants only non-anonymous unions, and
  this change to find_union() renders its check for anonymous unions
  superfluous.  Good.

* generate_visit_alternate()

  Asserts that each member's type is either a built-in type, a complex
  type, a union type, or an enum type.

  The change relaxes the assertion not to trigger on anonymous union
  types.  Why is that okay?