Re: [Qemu-devel] [PATCH v10 10/13] qapi: Don't box struct branch of alternate

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> There's no reason to do two malloc's for an alternate type visiting
> a QAPI struct; let's just inline the struct directly as the C union
> branch of the struct.
>
> Surprisingly, no clients were actually using the struct member prior
> to this patch; some testsuite coverage is added to avoid future
> regressions.
>
> Ultimately, we want to do the same treatment for QAPI unions, but
> as that touches a lot more client code, it is better as a separate
> patch.  So in the meantime, I had to hack in a way to test if we
> are visiting an alternate type, within qapi-types:gen_variants();
> the hack is possible because an earlier patch guaranteed that all
> alternates have at least two branches, with at most one object
> branch; the hack will go away in a later patch.

Suggest:

  Ultimately, we want to do the same treatment for QAPI unions, but as
  that touches a lot more client code, it is better as a separate patch.
  The two share gen_variants(), and I had to hack in a way to test if we
  are visiting an alternate type: look for a non-object branch.  This
  works because alternates have at least two branches, with at most one
  object branch, and unions have only object branches.  The hack will go
  away in a later patch.

> The generated code difference to qapi-types.h is relatively small,
> made possible by a new c_type(is_member) parameter in qapi.py:

Let's drop the "made possible" clause here.

>
> | struct BlockdevRef {
> |     QType type;
> |     union { /* union tag is @type */
> |         void *data;
> |-        BlockdevOptions *definition;
> |+        BlockdevOptions definition;
> |         char *reference;
> |     } u;
> | };
>
> meanwhile, in qapi-visit.h, we create a new visit_type_alternate_Foo(),
> comparable to visit_type_implicit_Foo():
>
> |+static void visit_type_alternate_BlockdevOptions(Visitor *v, const char 
> *name, BlockdevOptions *obj, Error **errp)
> |+{
> |+    Error *err = NULL;
> |+
> |+    visit_start_struct(v, name, NULL, 0, &err);
> |+    if (err) {
> |+        goto out;
> |+    }
> |+    visit_type_BlockdevOptions_fields(v, obj, &err);
> |+    error_propagate(errp, err);
> |+    err = NULL;
> |+    visit_end_struct(v, &err);
> |+out:
> |+    error_propagate(errp, err);
> |+}

This is in addition to visit_type_BlockdevOptions(), so we need another
name.

I can't quite see how the function is tied to alternates, though.

>
> and use it like this:
>
> |     switch ((*obj)->type) {
> |     case QTYPE_QDICT:
> |-        visit_type_BlockdevOptions(v, name, &(*obj)->u.definition, &err);
> |+        visit_type_alternate_BlockdevOptions(v, name, 
> &(*obj)->u.definition, &err);

Let's compare the two functions.  First visit_type_BlockdevOptions():

    void visit_type_BlockdevOptions(Visitor *v,
                                    const char *name,
                                    BlockdevOptions **obj,
                                    Error **errp)
    {
        Error *err = NULL;

        visit_start_struct(v, name, (void **)obj, sizeof(BlockdevOptions), 
&err);
        if (err) {
            goto out;
        }
        if (!*obj) {
            goto out_obj;
        }
        visit_type_BlockdevOptions_fields(v, *obj, &err);
        if (err) {
            goto out_obj;
        }
        if (!visit_start_union(v, !!(*obj)->u.data, &err) || err) {
            goto out_obj;
        }
        switch ((*obj)->driver) {
        case BLOCKDEV_DRIVER_ARCHIPELAGO:
            visit_type_implicit_BlockdevOptionsArchipelago(v, 
&(*obj)->u.archipelago, &err);
            break;
        [All the other cases...]
        default:
            abort();
        }
    out_obj:
        error_propagate(errp, err);
        err = NULL;
        visit_end_struct(v, &err);
    out:
        error_propagate(errp, err);
    }

Now visit_type_alternate_BlockdevOptions(), with differences annotated
with //:

    static void visit_type_alternate_BlockdevOptions(Visitor *v,
                                    const char *name,
                                    BlockdevOptions *obj, // one * less
                                    Error **errp)
    {
        Error *err = NULL;

        visit_start_struct(v, name, NULL, 0, &err); // NULL instead of &obj
                                                    // suppresses malloc
        if (err) {
            goto out;
        }
        // null check dropped (obj can't be null)
        visit_type_BlockdevOptions_fields(v, obj, &err);
        // visit_start_union() + switch dropped
        error_propagate(errp, err);
        err = NULL;
        visit_end_struct(v, &err);
    out:
        error_propagate(errp, err);
    }

Why can we drop visit_start_union() + switch?

> |         break;
> |     case QTYPE_QSTRING:
> |         visit_type_str(v, name, &(*obj)->u.reference, &err);
>
> Signed-off-by: Eric Blake <address@hidden>
>
> ---
> v10: new patch
> ---
>  scripts/qapi-types.py                   | 10 ++++++-
>  scripts/qapi-visit.py                   | 49 
> +++++++++++++++++++++++++++++----
>  scripts/qapi.py                         | 10 ++++---
>  tests/test-qmp-input-visitor.c          | 29 ++++++++++++++++++-
>  tests/qapi-schema/qapi-schema-test.json |  2 ++
>  tests/qapi-schema/qapi-schema-test.out  |  2 ++
>  6 files changed, 91 insertions(+), 11 deletions(-)
>
> diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
> index 2f23432..aba2847 100644
> --- a/scripts/qapi-types.py
> +++ b/scripts/qapi-types.py
> @@ -116,6 +116,14 @@ static inline %(base)s *qapi_%(c_name)s_base(const 
> %(c_name)s *obj)
>
>
>  def gen_variants(variants):
> +    # HACK: Determine if this is an alternate (at least one variant
> +    # is not an object); unions have all branches as objects.
> +    inline = False
> +    for v in variants.variants:
> +        if not isinstance(v.type, QAPISchemaObjectType):
> +            inline = True
> +            break
> +
>      # 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
>      # whether to bypass the switch statement if visiting the discriminator
> @@ -136,7 +144,7 @@ def gen_variants(variants):
>          ret += mcgen('''
>          %(c_type)s %(c_name)s;
>  ''',
> -                     c_type=typ.c_type(),
> +                     c_type=typ.c_type(is_member=inline),
>                       c_name=c_name(var.name))
>
>      ret += mcgen('''
> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
> index 9ff0337..948bde4 100644
> --- a/scripts/qapi-visit.py
> +++ b/scripts/qapi-visit.py
> @@ -15,10 +15,14 @@
>  from qapi import *
>  import re
>
> -# visit_type_FOO_implicit() is emitted as needed; track if it has already
> +# visit_type_implicit_FOO() is emitted as needed; track if it has already
>  # been output.
>  implicit_structs_seen = set()
>
> +# visit_type_alternate_FOO() is emitted as needed; track if it has already
> +# been output.
> +alternate_structs_seen = set()
> +
>  # visit_type_FOO_fields() is always emitted; track if a forward declaration
>  # or implementation has already been output.
>  struct_fields_seen = set()
> @@ -71,6 +75,35 @@ static void visit_type_implicit_%(c_type)s(Visitor *v, 
> %(c_type)s **obj, Error *
>      return ret
>
>
> +def gen_visit_alternate_struct(typ):
> +    if typ in alternate_structs_seen:
> +        return ''
> +    alternate_structs_seen.add(typ)
> +
> +    ret = gen_visit_fields_decl(typ)
> +
> +    ret += mcgen('''
> +
> +static void visit_type_alternate_%(c_type)s(Visitor *v, const char *name, 
> %(c_type)s *obj, Error **errp)
> +{
> +    Error *err = NULL;
> +
> +    visit_start_struct(v, name, NULL, 0, &err);
> +    if (err) {
> +        goto out;
> +    }
> +    visit_type_%(c_type)s_fields(v, obj, &err);
> +    error_propagate(errp, err);
> +    err = NULL;
> +    visit_end_struct(v, &err);
> +out:
> +    error_propagate(errp, err);
> +}
> +''',
> +                 c_type=typ.c_name())
> +    return ret
> +
> +
>  def gen_visit_struct_fields(name, base, members):
>      ret = ''
>
> @@ -158,11 +191,14 @@ void visit_type_%(c_name)s(Visitor *v, const char 
> *name, %(c_name)s *obj, Error
>
>  def gen_visit_alternate(name, variants):
>      promote_int = 'true'
> +    ret = ''
>      for var in variants.variants:
>          if var.type.alternate_qtype() == 'QTYPE_QINT':
>              promote_int = 'false'
> +        if isinstance(var.type, QAPISchemaObjectType):
> +            ret += gen_visit_alternate_struct(var.type)
>
> -    ret = mcgen('''
> +    ret += mcgen('''
>
>  void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, 
> Error **errp)
>  {
> @@ -178,15 +214,18 @@ void visit_type_%(c_name)s(Visitor *v, const char 
> *name, %(c_name)s **obj, Error
>      }
>      switch ((*obj)->type) {
>  ''',
> -                c_name=c_name(name), promote_int=promote_int)
> +                 c_name=c_name(name), promote_int=promote_int)
>
>      for var in variants.variants:
> +        prefix = 'visit_type_'
> +        if isinstance(var.type, QAPISchemaObjectType):
> +            prefix += 'alternate_'
>          ret += mcgen('''
>      case %(case)s:
> -        visit_type_%(c_type)s(v, name, &(*obj)->u.%(c_name)s, &err);
> +        %(prefix)s%(c_type)s(v, name, &(*obj)->u.%(c_name)s, &err);
>          break;
>  ''',
> -                     case=var.type.alternate_qtype(),
> +                     prefix=prefix, case=var.type.alternate_qtype(),
>                       c_type=var.type.c_name(),
>                       c_name=c_name(var.name))
>
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index 4d75d75..dbb58da 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -821,7 +821,7 @@ class QAPISchemaVisitor(object):
>
>
>  class QAPISchemaType(QAPISchemaEntity):
> -    def c_type(self, is_param=False):
> +    def c_type(self, is_param=False, is_member=False):
>          return c_name(self.name) + pointer_suffix
>
>      def c_null(self):
> @@ -854,7 +854,7 @@ class QAPISchemaBuiltinType(QAPISchemaType):
>      def c_name(self):
>          return self.name
>
> -    def c_type(self, is_param=False):
> +    def c_type(self, is_param=False, is_member=False):
>          if is_param and self.name == 'str':
>              return 'const ' + self._c_type_name
>          return self._c_type_name
> @@ -888,7 +888,7 @@ class QAPISchemaEnumType(QAPISchemaType):
>          # See QAPISchema._make_implicit_enum_type()
>          return self.name.endswith('Kind')
>
> -    def c_type(self, is_param=False):
> +    def c_type(self, is_param=False, is_member=False):
>          return c_name(self.name)
>
>      def member_names(self):
> @@ -984,8 +984,10 @@ class QAPISchemaObjectType(QAPISchemaType):
>          assert not self.is_implicit()
>          return QAPISchemaType.c_name(self)
>
> -    def c_type(self, is_param=False):
> +    def c_type(self, is_param=False, is_member=False):
>          assert not self.is_implicit()
> +        if is_member:
> +            return c_name(self.name)
>          return QAPISchemaType.c_type(self)

To understand this, you have to peel off OO abstraction:
QAPISchemaType.c_type(self) returns c_name(self.name) + pointer_suffix.

I think we should keep it peeled off in the source code.

>
>      def json_type(self):
[tests/ diff looks good]