Re: [Qemu-devel] [PATCH v9 31/37] qapi-visit: Unify struct and union visit

[Markus Armbruster <armbru@redhat.com>]

I'm sending this out even though it's unfinished.  It's probably more
useful for documenting my difficulties and confusion than for improving
the code.

Eric Blake <eblake@redhat.com> writes:

> We are finally at the point where gen_visit_struct() and
> gen_visit_union() can be unified to a generic gen_visit_object().
>
> The generated code for structs and for flat unions is unchanged.
> For simple unions, a new visit_type_FOO_fields() is created,
> wrapping the visit of the non-variant tag field:
>
> |+static void visit_type_ChardevBackend_fields(Visitor *v, ChardevBackend **obj, Error **errp)
> |+{
> |+    Error *err = NULL;
> |+
> |+    visit_type_ChardevBackendKind(v, "type", &(*obj)->type, &err);
> |+    if (err) {
> |+        goto out;
> |+    }
> |+
> |+out:
> |+    error_propagate(errp, err);
> |+}
> |+
> | void visit_type_ChardevBackend(Visitor *v, const char *name, ChardevBackend **obj, Error **errp)
> | {
> |     Error *err = NULL;
> |@@ -2319,7 +2332,7 @@ void visit_type_ChardevBackend(Visitor *
> |     if (!*obj) {
> |         goto out_obj;
> |     }
> |-    visit_type_ChardevBackendKind(v, "type", &(*obj)->type, &err);
> |+    visit_type_ChardevBackend_fields(v, obj, &err);
> |     if (err) {
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> ---
> v9: no change
> v8: rebase to 'name' motion
> v7: rebase to earlier changes
> v6: new patch
> ---
>  scripts/qapi-visit.py | 133 +++++++++++++++++++-------------------------------
>  1 file changed, 51 insertions(+), 82 deletions(-)
>
> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
> index 6d5c3d9..feef17f 100644
> --- a/scripts/qapi-visit.py
> +++ b/scripts/qapi-visit.py
> @@ -109,46 +109,6 @@ out:
>      return ret
>
>
> -def gen_visit_struct(name, base, members):
> -    ret = gen_visit_struct_fields(name, base, members)
> -
> -    # FIXME: if *obj is NULL on entry, and visit_start_struct() assigns to
> -    # *obj, but then visit_type_FOO_fields() fails, we should clean up *obj
> -    # rather than leaving it non-NULL. As currently written, the caller must
> -    # call qapi_free_FOO() to avoid a memory leak of the partial FOO.
> -    ret += mcgen('''
> -
> -void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, Error **errp)
> -{
> -    Error *err = NULL;
> -
> -    visit_start_struct(v, name, (void **)obj, sizeof(%(c_name)s), &err);
> -    if (err) {
> -        goto out;
> -    }
> -    if (!*obj) {
> -        goto out_obj;
> -    }
> -''',
> -                 name=name, c_name=c_name(name))
> -    if (base and not base.is_empty()) or members:
> -        ret += mcgen('''
> -    visit_type_%(c_name)s_fields(v, obj, &err);
> -''',
> -                     c_name=c_name(name))
> -    ret += mcgen('''
> -out_obj:
> -    error_propagate(errp, err);
> -    err = NULL;
> -    visit_end_struct(v, &err);
> -out:
> -    error_propagate(errp, err);
> -}
> -''')
> -
> -    return ret
> -
> -
>  def gen_visit_list(name, element_type):
>      # FIXME: if *obj is NULL on entry, and the first visit_next_list()
>      # assigns to *obj, while a later one fails, we should clean up *obj
> @@ -244,18 +204,24 @@ out:
>      return ret
>

You're merging gen_visit_struct() and gen_visit_union() into
gen_visit_object().  Need to review both the change from
gen_visit_struct() to gen_visit_object(), and from gen_visit_union() to
gen_visit_object().  Diff shows the latter, but not the former.  A
manual diff of the old gen_visit_struct() and new gen_visit_union()
doesn't come out helpful.

Recap differences between structs, flat unions and simple unions:

                    base           members        variants
    struct          may be empty   may be empty   no
    flat union      non-empty      empty          yes
    simple union    empty          just the tag   yes

>
> -def gen_visit_union(name, base, variants):
> +def gen_visit_object(name, base, members, variants):
>      ret = ''
>
>      assert base
>      if not base.is_empty():
>          ret += gen_visit_fields_decl(base)
> +    if members:
> +        ret += gen_visit_struct_fields(name, base, members)

Flat unions have no members: the new code does nothing.

Simple unions have a single member: we now generate a
visit_type_UNION_fields() for it.

Respective part of gen_visit_struct():

       ret = gen_visit_struct_fields(name, base, members)

It looks like you add a gen_visit_fields_decl() when the struct's base
is non-empty.  You actually don't, because the additional one in
gen_visit_object() suppresses the one in gen_visit_struct_fields().  I
think.

> +    if variants:
> +        for var in variants.variants:
> +            # Ugly special case for simple union TODO get rid of it
> +            if not var.simple_union_type():
> +                ret += gen_visit_implicit_struct(var.type)
>
> -    for var in variants.variants:
> -        # Ugly special case for simple union TODO get rid of it
> -        if not var.simple_union_type():
> -            ret += gen_visit_implicit_struct(var.type)
> -

Unions always have variants: no change.

Struct's don't: no change.

> +    # FIXME: if *obj is NULL on entry, and visit_start_struct() assigns to
> +    # *obj, but then visit_type_FOO_fields() fails, we should clean up *obj
> +    # rather than leaving it non-NULL. As currently written, the caller must
> +    # call qapi_free_FOO() to avoid a memory leak of the partial FOO.

Comment inherited from gen_visit_struct().  Does it apply to unions as
well?

>      ret += mcgen('''
>
>  void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, Error **errp)
> @@ -272,61 +238,71 @@ void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, Error
>  ''',
>                   c_name=c_name(name))
>
> -    if not base.is_empty():
> +    if not base.is_empty() or members:
> +        if members:
> +            type_name = c_name(name)
> +            cast = ''
> +        else:
> +            type_name = base.c_name()
> +            cast = '(%s **)' % type_name
>          ret += mcgen('''
> -    visit_type_%(c_name)s_fields(v, (%(c_name)s **)obj, &err);
> +    visit_type_%(c_name)s_fields(v, %(cast)sobj, &err);
>  ''',
> -                     c_name=base.c_name())
> -    else:
> +                     c_name=type_name, cast=cast)
> +        if variants:
> +            ret += gen_err_check(label='out_obj')
> +
> +    if variants:
>          ret += mcgen('''
> -    visit_type_%(c_type)s(v, "%(name)s", &(*obj)->%(c_name)s, &err);
> -''',
> -                     c_type=variants.tag_member.type.c_name(),
> -                     c_name=c_name(variants.tag_member.name),
> -                     name=variants.tag_member.name)
> -    ret += gen_err_check(label='out_obj')

Around here, the diff becomes too hard to read for me.

Please have a look at my "[PATCH 0/3] qapi-visit: Unify struct and union
visit".

> -    ret += mcgen('''
>      if (!visit_start_union(v, !!(*obj)->u.data, &err) || err) {
>          goto out_obj;
>      }
>      switch ((*obj)->%(c_name)s) {
>  ''',
> -                 c_name=c_name(variants.tag_member.name))
> +                     c_name=c_name(variants.tag_member.name))
>
> -    for var in variants.variants:
> -        # TODO ugly special case for simple union
> -        simple_union_type = var.simple_union_type()
> -        ret += mcgen('''
> +        for var in variants.variants:
> +            # TODO ugly special case for simple union
> +            simple_union_type = var.simple_union_type()
> +            ret += mcgen('''
>      case %(case)s:
>  ''',
> -                     case=c_enum_const(variants.tag_member.type.name,
> -                                       var.name))
> -        if simple_union_type:
> -            ret += mcgen('''
> +                         case=c_enum_const(variants.tag_member.type.name,
> +                                           var.name))
> +            if simple_union_type:
> +                ret += mcgen('''
>          visit_type_%(c_type)s(v, "data", &(*obj)->u.%(c_name)s, &err);
>  ''',
> -                         c_type=simple_union_type.c_name(),
> -                         c_name=c_name(var.name))
> -        elif not var.type.is_empty():
> -            ret += mcgen('''
> +                             c_type=simple_union_type.c_name(),
> +                             c_name=c_name(var.name))
> +            elif not var.type.is_empty():
> +                ret += mcgen('''
>          visit_type_implicit_%(c_type)s(v, &(*obj)->u.%(c_name)s, &err);
>  ''',
> -                         c_type=var.type.c_name(),
> -                         c_name=c_name(var.name))
> -        ret += mcgen('''
> +                             c_type=var.type.c_name(),
> +                             c_name=c_name(var.name))
> +            ret += mcgen('''
>          break;
>  ''')
>
> -    ret += mcgen('''
> +        ret += mcgen('''
>      default:
>          abort();
>      }
> +''')
> +
> +    ret += mcgen('''
>  out_obj:
> +''')
> +    if variants:
> +        ret += mcgen('''
>      error_propagate(errp, err);
>      err = NULL;
>      if (*obj) {
>          visit_end_union(v, !!(*obj)->u.data, &err);
>      }
> +''')
> +    ret += mcgen('''
>      error_propagate(errp, err);
>      err = NULL;
>      visit_end_struct(v, &err);
> @@ -387,14 +363,7 @@ class QAPISchemaGenVisitVisitor(QAPISchemaVisitor):
>
>      def visit_object_type(self, name, info, base, members, variants):
>          self.decl += gen_visit_decl(name)
> -        if variants:
> -            if members:
> -                # Members other than variants.tag_member not implemented
> -                assert len(members) == 1
> -                assert members[0] == variants.tag_member
> -            self.defn += gen_visit_union(name, base, variants)
> -        else:
> -            self.defn += gen_visit_struct(name, base, members)
> +        self.defn += gen_visit_object(name, base, members, variants)
>
>      def visit_alternate_type(self, name, info, variants):
>          self.decl += gen_visit_decl(name)