Re: [Qemu-devel] [PATCH v9 23/27] qapi: Simplify visiting of alternate types

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> Previously, working with alternates required two lookup arrays
> and some indirection: for type Foo, we created Foo_qtypes[]
> which maps each qtype to a value of the generated FooKind enum,
> then look up that value in FooKind_lookup[] like we do for other
> union types.
>
> This has a couple of subtle bugs.  First, the generator was
> creating a call with a parameter '(int *) &(*obj)->type' where
> type is an enum type; this is unsafe if the compiler chooses
> to store the enum type in a different size than int, where
> assigning through the wrong size pointer can corrupt data or
> cause a SIGBUS.
>
> Second, since the values of the FooKind enum start at zero, all
> entries of the Foo_qtypes[] array that were not explicitly
> initialized will map to the same branch of the union as the
> first member of the alternate, rather than triggering a desired
> failure in visit_get_next_type().  Fortunately, the bug seldom
> bites; the very next thing the input visitor does is try to
> parse the incoming JSON with the wrong parser, which normally
> fails; the output visitor is not used with a C struct in that
> state, and the dealloc visitor has nothing to clean up (so
> there is no leak).
>
> However, the second bug IS observable in one case: the behavior
> of an alternate that contains a 'number' member but no 'int'
> member differs according to whether the 'number' was first in
> the qapi definition,

This is confusing.  If there is a 'number' but no 'int', and the 'number
is first, what's second?

>                      and when the input being parsed is an
> integer; this is because the 'number' parser accepts QTYPE_QINT
> in addition to the expected QTYPE_QFLOAT.  A later patch will
> worry about fixing alternates to parse all inputs that a
> non-alternate 'number' would accept, for now this is still
> marked FIXME, and the test merely updated to point out that

and tests/test-qmp-input-visitor.c merely updated

> new undesired behavior of 'ans' matches the existing undesired
> behavior of 'asn'.
>
> This patch fixes the validation bug by deleting the indirection,

What's the validation bug?  I guess it's the one involving
default-initialized FooKind_lookup[].

> and modifying get_next_type() to directly assign a qtype_code
> parameter.  This in turn fixes the type-casting bug, as we are
> no longer casting a pointer to enum to a questionable size.
> There is no longer a need to generate an implicit FooKind enum
> associated with the alternate type (since the QMP wire format
> never uses the stringized counterparts of the C union member
> names); that also means we no longer have a collision with an
> alternate branch named 'max'.  Since visit_get_next_type() does
> not know which qtypes are expected, the generated visitor is
> modified to generate an error statement if an unexpected type
> is encountered.
>
> The code relies on a new QAPISchemaAlternateTypeTag subclass
> and the use of a new member.c_type() method for generating
> qapi-types.h; this is because we don't want to expose
> 'qtype_code' as a built-in type usable from .json files.

I'd be totally cool with that, actually.

> The new subtype happens to work by keeping tag_member.type
> as None, although this feels a bit unclean, and may be touched
> up further in a later patch.

I hate special cases like this one.

> Callers now have to know the QTYPE_* mapping when looking at the
> discriminator; but so far, only the testsuite was even using the
> C struct of an alternate types.  I considered the possibility of
> keeping the internal enum FooKind, but initialized differently
> than most generated arrays, as in:
>   typedef enum FooKind {
>       FOO_KIND_A = QTYPE_QDICT,
>       FOO_KIND_B = QTYPE_QINT,
>   } FooKind;
> to create nicer aliases for knowing when to use foo->a or foo->b
> when inspecting foo->type; but it turned out to add too much
> complexity, especially without a client.

Having to use QTYPE_FOOs seems good enough.

> There is a user-visible side effect to this change, but I
> consider it to be an improvement. Previously,
> the invalid QMP command:
>   {"execute":"blockdev-add", "arguments":{"options":
>     {"driver":"raw", "id":"a", "file":true}}}
> failed with:
>   {"error": {"class": "GenericError",
>     "desc": "Invalid parameter type for 'file', expected: QDict"}}
> (visit_get_next_type() succeeded, and the error comes from the
> visit_type_BlockdevOptions() expecting {}; there is no mention of
> the fact that a string would also work).  Now it fails with:
>   {"error": {"class": "GenericError",
>     "desc": "Invalid parameter type for 'file', expected: BlockdevRef"}}
> (the error when the next type doesn't match any expected types for
> the overall alternate).
>
> Signed-off-by: Eric Blake <address@hidden>
>
> ---
> v9: rebase to earlier changes, rework commit message to mention second
> bug fix; move positive test in qapi-schema-test to later patch
> v8: no change
> v7: rebase onto earlier changes, rework how subtype makes things work
> v6: rebase onto tag_member subclass, testsuite, gen_err_check(),
> and info improvements
> ---
>  docs/qapi-code-gen.txt                 |  3 ---
>  include/qapi/visitor-impl.h            |  3 ++-
>  include/qapi/visitor.h                 |  8 ++++++-
>  qapi/qapi-visit-core.c                 |  4 ++--
>  qapi/qmp-input-visitor.c               |  4 ++--
>  scripts/qapi-types.py                  | 36 +---------------------------
>  scripts/qapi-visit.py                  | 14 +++++++----
>  scripts/qapi.py                        | 44 
> +++++++++++++++++++++++++---------
>  tests/qapi-schema/alternate-empty.out  |  1 -
>  tests/qapi-schema/qapi-schema-test.out |  8 -------
>  tests/test-qmp-input-visitor.c         | 31 ++++++++++++------------
>  tests/test-qmp-output-visitor.c        |  4 ++--
>  12 files changed, 74 insertions(+), 86 deletions(-)
>
> diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt
> index 20e6907..9196e5c 100644
> --- a/docs/qapi-code-gen.txt
> +++ b/docs/qapi-code-gen.txt
> @@ -383,9 +383,6 @@ where each branch of the union names a QAPI type.  For 
> example:
>     'data': { 'definition': 'BlockdevOptions',
>               'reference': 'str' } }
>
> -Just like for a simple union, an implicit C enum 'NameKind' is created
> -to enumerate the branches for the alternate 'Name'.
> -
>  Unlike a union, the discriminator string is never passed on the wire
>  for the Client JSON Protocol.  Instead, the value's JSON type serves
>  as an implicit discriminator, which in turn means that an alternate
> diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h
> index 8c0ba57..6d95b36 100644
> --- a/include/qapi/visitor-impl.h
> +++ b/include/qapi/visitor-impl.h
> @@ -32,7 +32,8 @@ struct Visitor
>
>      void (*type_enum)(Visitor *v, int *obj, const char * const strings[],
>                        const char *kind, const char *name, Error **errp);
> -    void (*get_next_type)(Visitor *v, int *kind, const int *qobjects,
> +    /* May be NULL; most useful for input visitors. */
> +    void (*get_next_type)(Visitor *v, qtype_code *type,
>                            const char *name, Error **errp);
>
>      void (*type_int)(Visitor *v, int64_t *obj, const char *name, Error 
> **errp);
> diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
> index cfc19a6..b765993 100644
> --- a/include/qapi/visitor.h
> +++ b/include/qapi/visitor.h
> @@ -41,7 +41,13 @@ GenericList *visit_next_list(Visitor *v, GenericList 
> **list, Error **errp);
>  void visit_end_list(Visitor *v, Error **errp);
>  void visit_optional(Visitor *v, bool *present, const char *name,
>                      Error **errp);
> -void visit_get_next_type(Visitor *v, int *obj, const int *qtypes,
> +
> +/**
> + * Determine the qtype of the item @name in the current object visit.
> + * For input visitors, set address@hidden to the correct qtype of a qapi
> + * alternate type; for other visitors, leave address@hidden unchanged.

Aside: I can't think of a reason for an output visitor to implement this
method.

> + */
> +void visit_get_next_type(Visitor *v, qtype_code *type,
>                           const char *name, Error **errp);
>  void visit_type_enum(Visitor *v, int *obj, const char * const strings[],
>                       const char *kind, const char *name, Error **errp);
> diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
> index 59ed506..3f24daa 100644
> --- a/qapi/qapi-visit-core.c
> +++ b/qapi/qapi-visit-core.c
> @@ -81,11 +81,11 @@ void visit_optional(Visitor *v, bool *present, const char 
> *name,
>      }
>  }
>
> -void visit_get_next_type(Visitor *v, int *obj, const int *qtypes,
> +void visit_get_next_type(Visitor *v, qtype_code *type,
>                           const char *name, Error **errp)
>  {
>      if (v->get_next_type) {
> -        v->get_next_type(v, obj, qtypes, name, errp);
> +        v->get_next_type(v, type, name, errp);
>      }
>  }
>
> diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c
> index eb6e110..c1e7ec8 100644
> --- a/qapi/qmp-input-visitor.c
> +++ b/qapi/qmp-input-visitor.c
> @@ -208,7 +208,7 @@ static void qmp_input_end_list(Visitor *v, Error **errp)
>      qmp_input_pop(qiv, errp);
>  }
>
> -static void qmp_input_get_next_type(Visitor *v, int *kind, const int 
> *qobjects,
> +static void qmp_input_get_next_type(Visitor *v, qtype_code *type,
>                                      const char *name, Error **errp)
>  {
>      QmpInputVisitor *qiv = to_qiv(v);
> @@ -218,7 +218,7 @@ static void qmp_input_get_next_type(Visitor *v, int 
> *kind, const int *qobjects,
>          error_setg(errp, QERR_MISSING_PARAMETER, name ? name : "null");
>          return;
>      }
> -    *kind = qobjects[qobject_type(qobj)];
> +    *type = qobject_type(qobj);
>  }
>
>  static void qmp_input_type_int(Visitor *v, int64_t *obj, const char *name,
> diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
> index 2f2f7df..2ac1405 100644
> --- a/scripts/qapi-types.py
> +++ b/scripts/qapi-types.py
> @@ -47,7 +47,7 @@ def gen_struct_fields(members):
>          ret += mcgen('''
>      %(c_type)s %(c_name)s;
>  ''',
> -                     c_type=memb.type.c_type(), c_name=c_name(memb.name))
> +                     c_type=memb.c_type(), c_name=c_name(memb.name))
>      return ret
>
>
> @@ -101,38 +101,6 @@ static inline %(base)s *qapi_%(c_name)s_base(const 
> %(c_name)s *obj)
>                   c_name=c_name(name), base=base.c_name())
>
>
> -def gen_alternate_qtypes_decl(name):
> -    return mcgen('''
> -
> -extern const int %(c_name)s_qtypes[];
> -''',
> -                 c_name=c_name(name))
> -
> -
> -def gen_alternate_qtypes(name, variants):
> -    ret = mcgen('''
> -
> -const int %(c_name)s_qtypes[QTYPE_MAX] = {
> -''',
> -                c_name=c_name(name))
> -
> -    for var in variants.variants:
> -        qtype = var.type.alternate_qtype()
> -        assert qtype
> -
> -        ret += mcgen('''
> -    [%(qtype)s] = %(enum_const)s,
> -''',
> -                     qtype=qtype,
> -                     enum_const=c_enum_const(variants.tag_member.type.name,
> -                                             var.name))
> -
> -    ret += mcgen('''
> -};
> -''')
> -    return ret
> -
> -
>  def gen_variants(variants):
>      # FIXME: What purpose does data serve, besides preventing a union that
>      # has a branch named 'data'? We use it in qapi-visit.py to decide
> @@ -257,9 +225,7 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor):
>
>      def visit_alternate_type(self, name, info, variants):
>          self._fwdecl += gen_fwd_object_or_array(name)
> -        self._fwdefn += gen_alternate_qtypes(name, variants)
>          self.decl += gen_object(name, None, [variants.tag_member], variants)
> -        self.decl += gen_alternate_qtypes_decl(name)
>          self._gen_type_cleanup(name)
>
>  # If you link code generated from multiple schemata, you want only one
> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
> index 94cd113..af80e6d 100644
> --- a/scripts/qapi-visit.py
> +++ b/scripts/qapi-visit.py
> @@ -193,7 +193,7 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, 
> const char *name, Error
>      if (err) {
>          goto out;
>      }
> -    visit_get_next_type(v, (int*) &(*obj)->type, %(c_name)s_qtypes, name, 
> &err);
> +    visit_get_next_type(v, &(*obj)->type, name, &err);
>      if (err) {
>          goto out_obj;
>      }
> @@ -201,20 +201,22 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s 
> **obj, const char *name, Error
>  ''',
>                  c_name=c_name(name))
>
> +    # FIXME: When 'number' but not 'int' is present in the alternate, we
> +    # should allow QTYPE_INT to promote to QTYPE_FLOAT.
>      for var in variants.variants:
>          ret += mcgen('''
>      case %(case)s:
>          visit_type_%(c_type)s(v, &(*obj)->u.%(c_name)s, name, &err);
>          break;
>  ''',
> -                     case=c_enum_const(variants.tag_member.type.name,
> -                                       var.name),
> +                     case=var.type.alternate_qtype(),
>                       c_type=var.type.c_name(),
>                       c_name=c_name(var.name))
>
>      ret += mcgen('''
>      default:
> -        abort();
> +        error_setg(&err, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
> +                   "%(name)s");

Craptastic error reporting, but it's widely done elsewhere already, and
cleaning it up is out of scope.

>      }
>  out_obj:
>      error_propagate(errp, err);
> @@ -223,7 +225,8 @@ out_obj:
>  out:
>      error_propagate(errp, err);
>  }
> -''')
> +''',
> +                 name=name)
>
>      return ret
>
> @@ -430,6 +433,7 @@ fdef.write(mcgen('''
>
>  fdecl.write(mcgen('''
>  #include "qapi/visitor.h"
> +#include "qapi/qmp/qerror.h"
>  #include "%(prefix)sqapi-types.h"
>
>  ''',
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index f5aa1d5..9a1f0ac 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -627,15 +627,15 @@ def check_union(expr, expr_info):
>  def check_alternate(expr, expr_info):
>      name = expr['alternate']
>      members = expr['data']
> -    values = {'MAX': '(automatic)'}
> +    values = {}
>      types_seen = {}
>
>      # Check every branch
>      for (key, value) in members.items():
>          check_name(expr_info, "Member of alternate '%s'" % name, key)
>
> -        # Check for conflicts in the generated enum
> -        c_key = camel_to_upper(key)
> +        # Check for conflicts in the branch names
> +        c_key = c_name(key)
>          if c_key in values:
>              raise QAPIExprError(expr_info,
>                                  "Alternate '%s' member '%s' clashes with 
> '%s'"
> @@ -1029,13 +1029,17 @@ class QAPISchemaObjectTypeMember(object):
>          assert self.name not in seen
>          seen[self.name] = self
>
> +    def c_type(self):
> +        return self.type.c_type()
> +
>
>  class QAPISchemaObjectTypeVariants(object):
>      def __init__(self, tag_name, tag_member, variants):
>          # Flat unions pass tag_name but not tag_member.
>          # Simple unions and alternates pass tag_member but not tag_name.
>          # After check(), tag_member is always set, and tag_name remains
> -        # a reliable witness of being used by a flat union.
> +        # a reliable witness of being used by a flat union, and
> +        # tag_member.type being None is a reliable witness of an alternate.
>          assert bool(tag_member) != bool(tag_name)
>          assert (isinstance(tag_name, str) or
>                  isinstance(tag_member, QAPISchemaObjectTypeMember))
> @@ -1049,7 +1053,11 @@ class QAPISchemaObjectTypeVariants(object):
>          # seen is non-empty for unions, empty for alternates
>          if self.tag_name:    # flat union
>              self.tag_member = seen[self.tag_name]
> -        assert isinstance(self.tag_member.type, QAPISchemaEnumType)
> +        if seen:
> +            assert isinstance(self.tag_member.type, QAPISchemaEnumType)
> +        else:
> +            assert isinstance(self.tag_member, QAPISchemaAlternateTypeTag)
> +            assert not self.tag_member.type

Awkward.

>          for v in self.variants:
>              # Reset seen array for each variant, since qapi names from one
>              # branch do not affect another branch
> @@ -1062,10 +1070,8 @@ class 
> QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember):
>
>      def check(self, schema, tag_type, seen):
>          QAPISchemaObjectTypeMember.check(self, schema)
> -        assert self.name in tag_type.values
> -        if seen:
> -            # This variant is used within a union; ensure each qapi member
> -            # field does not collide with the union's non-variant members.
> +        if seen:     # in a union
> +            assert self.name in tag_type.values
>              self.type.check_clash(schema, seen)
>
>      # This function exists to support ugly simple union special cases
> @@ -1087,8 +1093,12 @@ class QAPISchemaAlternateType(QAPISchemaType):
>          self.variants = variants
>
>      def check(self, schema):
> -        self.variants.tag_member.check(schema)
>          self.variants.check(schema, {})
> +        # Since we have no enum mapping, we have to check for potential
> +        # case name collisions ourselves.
> +        cases = {}
> +        for var in self.variants.variants:
> +            var.check_clash(cases)
>
>      def json_type(self):
>          return 'value'
> @@ -1097,6 +1107,18 @@ class QAPISchemaAlternateType(QAPISchemaType):
>          visitor.visit_alternate_type(self.name, self.info, self.variants)
>
>
> +class QAPISchemaAlternateTypeTag(QAPISchemaObjectTypeMember):
> +    # TODO: This subclass intentionally leaves self.tag_type as None,
> +    # and intentionally has no check() method. It might be easier to
> +    # reason about the overall QAPISchema if we also subclassed
> +    # QAPISchemaBuiltinType to supply an internal-only 'qtype_code' type.

Sure we need to subclass?  I do hope an instance will do!  Something
like

        self.the_qtype_code_type = QAPISchemaEnumType(':qtype_code', None,
            ['NONE', 'QNULL', ...])

in _def_predefineds().

Also avoids the awkward conditional in
QAPISchemaObjectTypeVariants.check().

Fewer special cases are worth a bit of scaffolding.

> +    def __init__(self):
> +        QAPISchemaObjectTypeMember.__init__(self, 'type', '', False)
> +
> +    def c_type(self):
> +        return 'qtype_code'
> +
> +
>  class QAPISchemaCommand(QAPISchemaEntity):
>      def __init__(self, name, info, arg_type, ret_type, gen, 
> success_response):
>          QAPISchemaEntity.__init__(self, name, info)
> @@ -1290,7 +1312,7 @@ class QAPISchema(object):
>          data = expr['data']
>          variants = [self._make_variant(key, value)
>                      for (key, value) in data.iteritems()]
> -        tag_member = self._make_implicit_tag(name, info, variants)
> +        tag_member = QAPISchemaAlternateTypeTag()
>          self._def_entity(
>              QAPISchemaAlternateType(name, info,
>                                      QAPISchemaObjectTypeVariants(None,
[Tests snipped...]