Re: [Qemu-devel] [PATCH 2/3] qapi-visit: Expose visit_type_FOO_fields()

[Markus Armbruster <armbru@redhat.com>]

Eric Blake <eblake@redhat.com> writes:

> Dan Berrange reported a case where he needs to work with a
> QCryptoBlockOptions union type using the OptsVisitor, but only
> visit one of the branches of that type (the discriminator is not
> visited directly, but learned externally).  When things were
> boxed, it was easy: just visit the variant directly, which took
> care of both allocating the variant and visiting its fields.  But
> now that things are unboxed, we need a way to visit the fields
> without allocation, done by exposing visit_type_FOO_fields() to
> the user.  Of course, this should only be done for objects, not
> lists, so we need another flag to gen_visit_decl().
>
> Since the function is now public, we no longer need to preserve
> topological ordering via struct_fields_seen.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
>
> ---
> Minor conflicts with pending series "qapi implicit types"; I can
> rebase whichever series gets reviewed second.
> ---
>  scripts/qapi-visit.py | 47 +++++++++++++----------------------------------
>  1 file changed, 13 insertions(+), 34 deletions(-)
>

Let me review how this thing works for an object type FOO before your
patch.

gen_visit_object() generates visit_type_FOO() with external linkage, and
gen_visit_struct_fields() generates visit_type_FOO_fields() with
internal linkage.

gen_visit_decl() generates a declaration of visit_type_FOO(), and
gen_visit_fields_decl() generates one for visit_type_FOO_fields() unless
it's already in scope.

visit_type_FOO_fields() is always called by visit_type_FOO(), and
sometimes called elsewhere.

We generate visit_type_FOO_fields() right before visit_type_FOO(), so
it's in scope there.  Anything that generates uses elsewhere must call
gen_visit_fields_decl().

Your patch generates visit_type_FOO_fields() declarations into the
header, which renders the "if already in scope" logic useless, along
with the need to call gen_visit_fields_decl() before generating
visit_type_FOO_fields() uses outside visit_type_FOO().

> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
> index 2308268..35efe7c 100644
> --- a/scripts/qapi-visit.py
> +++ b/scripts/qapi-visit.py
> @@ -15,52 +15,33 @@
>  from qapi import *
>  import re
>
> -# visit_type_FOO_fields() is always emitted; track if a forward declaration
> -# or implementation has already been output.
> -struct_fields_seen = set()
>
> -
> -def gen_visit_decl(name, scalar=False):
> +def gen_visit_decl(name, scalar=False, list=False):
> +    ret = ''
>      c_type = c_name(name) + ' *'
>      if not scalar:
> +        if not list:
> +            ret += mcgen('''
> +void visit_type_%(c_name)s_fields(Visitor *v, %(c_type)sobj, Error **errp);
> +''',
> +                         c_name=c_name(name), c_type=c_type)
>          c_type += '*'
> -    return mcgen('''
> +    ret += mcgen('''
>  void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_type)sobj, Error **errp);
>  ''',
>                   c_name=c_name(name), c_type=c_type)
> -
> -
> -def gen_visit_fields_decl(typ):
> -    if typ.name in struct_fields_seen:
> -        return ''
> -    struct_fields_seen.add(typ.name)
> -    return mcgen('''
> -
> -static void visit_type_%(c_type)s_fields(Visitor *v, %(c_type)s *obj, Error **errp);
> -''',
> -                 c_type=typ.c_name())
> +    return ret

This folds gen_visit_fields_decl() into gen_visit_decl() with the
necessary changes: drop the "is already in scope" conditional, switch to
external linkage.

Since gen_visit_decl() is called for non-object types as well, it gets a
new optional parameter to suppress generation of
visit_type_FOO_fields().  The parameter is named @list, which makes no
sense to me.  See more below.

I don't like do-everything functions with suppressor flags much.  Can we
structure this as a set of do-one-thing functions?  Possibly with
convenience functions collecting common sets.

>  def gen_visit_struct_fields(name, base, members, variants):
> -    ret = ''
> +    ret = mcgen('''
>
> -    if base:
> -        ret += gen_visit_fields_decl(base)
> -    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_fields_decl(var.type)

Drop gen_visit_fields_decl() calls.  Good.

> -
> -    struct_fields_seen.add(name)

Drop code supporting "if already in scope" conditionals.  Good.

> -    ret += mcgen('''
> -
> -static void visit_type_%(c_name)s_fields(Visitor *v, %(c_name)s *obj, Error **errp)
> +void visit_type_%(c_name)s_fields(Visitor *v, %(c_name)s *obj, Error **errp)

Change linkage.  Good.

>  {
>      Error *err = NULL;
>
>  ''',
> -                 c_name=c_name(name))
> +                c_name=c_name(name))
>
>      if base:
>          ret += mcgen('''
> @@ -173,8 +154,6 @@ def gen_visit_alternate(name, variants):
>      for var in variants.variants:
>          if var.type.alternate_qtype() == 'QTYPE_QINT':
>              promote_int = 'false'
> -        if isinstance(var.type, QAPISchemaObjectType):
> -            ret += gen_visit_fields_decl(var.type)

Drop gen_visit_fields_decl() calls.  Good.

>
>      ret += mcgen('''
>
> @@ -305,7 +284,7 @@ class QAPISchemaGenVisitVisitor(QAPISchemaVisitor):
       def visit_enum_type(self, name, info, values, prefix):
           # Special case for our lone builtin enum type
           # TODO use something cleaner than existence of info
           if not info:
               self._btin += gen_visit_decl(name, scalar=True)

No change, because scalar=True effectively implies list=True.

               if do_builtins:
                   self.defn += gen_visit_enum(name)
           else:
               self.decl += gen_visit_decl(name, scalar=True)

Likewise.

>              self.defn += gen_visit_enum(name)
>
>      def visit_array_type(self, name, info, element_type):
> -        decl = gen_visit_decl(name)
> +        decl = gen_visit_decl(name, list=True)


No change, because list=True suppresses it.

>          defn = gen_visit_list(name, element_type)
>          if isinstance(element_type, QAPISchemaBuiltinType):
>              self._btin += decl
               if do_builtins:
                   self.defn += defn
           else:
               self.decl += decl
               self.defn += defn

       def visit_object_type(self, name, info, base, members, variants):
           self.decl += gen_visit_decl(name)

Here, we now generate an additional visit_type_FOO_fields() declaration.

           self.defn += gen_visit_object(name, base, members, variants)

       def visit_alternate_type(self, name, info, variants):
           self.decl += gen_visit_decl(name)

Bug: here too.

    $ grep _BlockdevRef_fields q*[ch]
    qapi-visit.h:void visit_type_BlockdevRef_fields(Visitor *v, BlockdevRef *obj, Error **errp);

           self.defn += gen_visit_alternate(name, variants)

Your choice of an optional argument to keep things unchanged effectively
hid the places that changed.  Failed to fool me today, but don't expect
to remain lucky that way :)

One more thing: I never liked the name _fields.  C struct and union
types don't have fields, they have members.  Likewiese, JSON objects.
We shouldn't gratitously invent terminology.  We've done that quite a
bit around QMP (JSON object vs. C QDict, JSON array vs. QList, QFLoat is
really a double, ...).  Cleaning that up completely is probably hopeless
by now.  But we can rename visit_type_FOO_fields() to something more
sensible, say visit_type_FOO_members(), or even
visit_type_FOO_unboxed().