Re: [Qemu-devel] [PATCH v9 30/37] qapi: Canonicalize missing object to :empty

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> Now that we elide unnecessary visits of empty types, we can
> start using the special ':empty' type in more places.  By using
> the empty type as the base class of every explicit struct or
> union, and as the default data for any command or event, we can
> simplify later logic in qapi-{visit,commands,event} by merely
> checking whether the type is empty, without also having to worry
> whether a type was even supplied.
>
> Note that gen_object() in qapi-types still has to check for a
> base, because it is also called for alternates (which have no
> base).

What about the one in gen_visit_struct()?

        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))

> No change to generated code.
>
> Signed-off-by: Eric Blake <address@hidden>
>
> ---
> v9: squash in more related changes
> v8: rebase to earlier changes
> v7: rebase to earlier changes
> v6: new patch
> ---
>  scripts/qapi-commands.py                | 17 +++++++------
>  scripts/qapi-event.py                   |  5 ++--
>  scripts/qapi-types.py                   |  4 +--
>  scripts/qapi-visit.py                   | 12 +++++----
>  scripts/qapi.py                         | 25 +++++++++---------
>  tests/qapi-schema/event-case.out        |  2 +-
>  tests/qapi-schema/flat-union-empty.out  |  1 +
>  tests/qapi-schema/ident-with-escape.out |  1 +
>  tests/qapi-schema/indented-expr.out     |  4 +--
>  tests/qapi-schema/qapi-schema-test.out  | 45 
> ++++++++++++++++++++++++++++++---
>  tests/qapi-schema/union-clash-data.out  |  2 ++
>  tests/qapi-schema/union-empty.out       |  1 +
>  12 files changed, 83 insertions(+), 36 deletions(-)
>
> diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
> index 00ee565..9d455c3 100644
> --- a/scripts/qapi-commands.py
> +++ b/scripts/qapi-commands.py
> @@ -29,11 +29,11 @@ def gen_call(name, arg_type, ret_type):
>      ret = ''
>
>      argstr = ''
> -    if arg_type:
> -        for memb in arg_type.members:
> -            if memb.optional:
> -                argstr += 'has_%s, ' % c_name(memb.name)
> -            argstr += '%s, ' % c_name(memb.name)
> +    assert arg_type
> +    for memb in arg_type.members:

The assertion doesn't buy us much.  If arg_type is true, but of the
wrong type, the assertion holds, and arg_type.members throws an error.
If it false, then arg_type.members's error is just as useful as an
AssertionError.

More of the same below.

> +        if memb.optional:
> +            argstr += 'has_%s, ' % c_name(memb.name)
> +        argstr += '%s, ' % c_name(memb.name)
>
>      lhs = ''
>      if ret_type:
> @@ -65,7 +65,8 @@ def gen_marshal_vars(arg_type, ret_type):
>  ''',
>                       c_type=ret_type.c_type())
>
> -    if arg_type and not arg_type.is_empty():
> +    assert arg_type
> +    if not arg_type.is_empty():
>          ret += mcgen('''
>      QmpInputVisitor *qiv = qmp_input_visitor_new_strict(QOBJECT(args));
>      QapiDeallocVisitor *qdv;
> @@ -97,7 +98,7 @@ def gen_marshal_vars(arg_type, ret_type):
>  def gen_marshal_input_visit(arg_type, dealloc=False):
>      ret = ''
>
> -    if not arg_type or arg_type.is_empty():
> +    if arg_type.is_empty():
>          return ret
>
>      if dealloc:
> @@ -177,7 +178,7 @@ def gen_marshal(name, arg_type, ret_type):
>
>      # 'goto out' produced by gen_marshal_input_visit->gen_visit_fields()
>      # for each arg_type member, and by gen_call() for ret_type
> -    if (arg_type and not arg_type.is_empty()) or ret_type:
> +    if not arg_type.is_empty() or ret_type:
>          ret += mcgen('''
>
>  out:
> diff --git a/scripts/qapi-event.py b/scripts/qapi-event.py
> index 56c93a8..cc55de7 100644
> --- a/scripts/qapi-event.py
> +++ b/scripts/qapi-event.py
> @@ -39,7 +39,8 @@ def gen_event_send(name, arg_type):
>  ''',
>                  proto=gen_event_send_proto(name, arg_type))
>
> -    if arg_type and not arg_type.is_empty():
> +    assert arg_type
> +    if not arg_type.is_empty():
>          ret += mcgen('''
>      QmpOutputVisitor *qov;
>      Visitor *v;
> @@ -88,7 +89,7 @@ out_obj:
>  ''',
>                   c_enum=c_enum_const(event_enum_name, name))
>
> -    if arg_type and not arg_type.is_empty():
> +    if not arg_type.is_empty():
>          ret += mcgen('''
>  out:
>      qmp_output_visitor_cleanup(qov);
> diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
> index c70fae1..5cf20c2 100644
> --- a/scripts/qapi-types.py
> +++ b/scripts/qapi-types.py
> @@ -58,7 +58,7 @@ struct %(c_name)s {
>  ''',
>                  c_name=c_name(name))
>
> -    if base:
> +    if base and not base.is_empty():
>          ret += mcgen('''
>      /* Members inherited from %(c_name)s: */
>  ''',

Here, you go straight from X to X and not X.is_empty().  Elsewhere, you
take two steps: one patch goes from X to X and X.is_empty(), changing
ouput when X is an empty struct, and this one to just X.is_empty(),
without changing output.

I understand you need to keep checking base because alternates have
none.

I suspect that if base is empty, this hunk changes output, unlike the
other ones, namely the comments

    /* Members inherited from %(c_name)s: */
    /* Own members: */

go away.

If that's true, it should either be a separate patch, or be mentioned in
the commit message.  A test case would be nice.

> @@ -222,7 +222,7 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor):
>      def visit_object_type(self, name, info, base, members, variants):
>          self._fwdecl += gen_fwd_object_or_array(name)
>          self.decl += gen_object(name, base, members, variants)
> -        if base:
> +        if not base.is_implicit():

Why .is_implicit() instead of .is_empty() here?

>              self.decl += gen_upcast(name, base)
>          self._gen_type_cleanup(name)
>
> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
> index 6537a20..6d5c3d9 100644
> --- a/scripts/qapi-visit.py
> +++ b/scripts/qapi-visit.py
> @@ -74,10 +74,11 @@ static void visit_type_implicit_%(c_type)s(Visitor *v, 
> %(c_type)s **obj, Error *
>  def gen_visit_struct_fields(name, base, members):
>      ret = ''
>
> -    if (not base or base.is_empty()) and not members:
> +    assert base
> +    if base.is_empty() and not members:
>          return ret
>
> -    if base and not base.is_empty():
> +    if not base.is_empty():
>          ret += gen_visit_fields_decl(base)
>
>      struct_fields_seen.add(name)
> @@ -90,7 +91,7 @@ static void visit_type_%(c_name)s_fields(Visitor *v, 
> %(c_name)s **obj, Error **e
>  ''',
>                   c_name=c_name(name))
>
> -    if base and not base.is_empty():
> +    if not base.is_empty():
>          ret += mcgen('''
>      visit_type_%(c_type)s_fields(v, (%(c_type)s **)obj, &err);
>  ''',
> @@ -246,7 +247,8 @@ out:
>  def gen_visit_union(name, base, variants):
>      ret = ''
>
> -    if base:
> +    assert base
> +    if not base.is_empty():
>          ret += gen_visit_fields_decl(base)

Here, you go straight from X to X.is_empty().  Elsewhere, you take two
steps: one patch goes from X to X and X.is_empty(), changing ouput when
X is an empty struct, and this one to just X.is_empty(), without
changing output.

The first step is actually PATCH 29: it changes gen_visit_fields_decl()
not to generate anything for an empty type.

I think it should also make this call unconditional.

>
>      for var in variants.variants:
> @@ -270,7 +272,7 @@ void visit_type_%(c_name)s(Visitor *v, const char *name, 
> %(c_name)s **obj, Error
>  ''',
>                   c_name=c_name(name))
>
> -    if base:
> +    if not base.is_empty():

Another one, let's see what this does.

>          ret += mcgen('''
>      visit_type_%(c_name)s_fields(v, (%(c_name)s **)obj, &err);
>  ''',
                        c_name=base.c_name())
       else:
           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)

The condition is really flat union (the common members are in the base)
vs. simple union (the common member is tag_member).  If flat union, we
generate the base visit, else the tag_member visit.  Together, this
generates the visit of common members.

Your change breaks the condition: now simple unions have a base, too.
You fix it up to test for empty base instead.  Okay.

> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index a13c110..e9006e4 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -922,11 +922,11 @@ class QAPISchemaArrayType(QAPISchemaType):
>
>  class QAPISchemaObjectType(QAPISchemaType):
>      def __init__(self, name, info, base, local_members, variants):
> -        # struct has local_members, optional base, and no variants
> +        # struct has local_members, base, and no variants
>          # flat union has base, variants, and no local_members
> -        # simple union has local_members, variants, and no base
> +        # simple union has local_members, variants, and base of :empty
>          QAPISchemaType.__init__(self, name, info)
> -        assert base is None or isinstance(base, str)
> +        assert isinstance(base, str) or name == ':empty'

No longer tight: if name == ':empty', base must be None, else it must be
a str.

>          for m in local_members:
>              assert isinstance(m, QAPISchemaObjectTypeMember)
>              m.set_owner(name)
> @@ -1264,11 +1264,11 @@ class QAPISchema(object):
>
>      def _make_implicit_object_type(self, name, info, role, members):
>          if not members:
> -            return None
> +            return ':empty'
>          # See also QAPISchemaObjectTypeMember._pretty_owner()
>          name = ':obj-%s-%s' % (name, role)
>          if not self.lookup_entity(name, QAPISchemaObjectType):
> -            self._def_entity(QAPISchemaObjectType(name, info, None,
> +            self._def_entity(QAPISchemaObjectType(name, info, ':empty',
>                                                    members, None))
>          return name
>
> @@ -1295,7 +1295,7 @@ class QAPISchema(object):
>
>      def _def_struct_type(self, expr, info):
>          name = expr['struct']
> -        base = expr.get('base')
> +        base = expr.get('base', ':empty')
>          data = expr['data']
>          self._def_entity(QAPISchemaObjectType(name, info, base,
>                                                self._make_members(data, info),
> @@ -1315,7 +1315,7 @@ class QAPISchema(object):
>      def _def_union_type(self, expr, info):
>          name = expr['union']
>          data = expr['data']
> -        base = expr.get('base')
> +        base = expr.get('base', ':empty')
>          tag_name = expr.get('discriminator')
>          tag_member = None
>          if tag_name:
> @@ -1349,11 +1349,11 @@ class QAPISchema(object):
>
>      def _def_command(self, expr, info):
>          name = expr['command']
> -        data = expr.get('data')
> +        data = expr.get('data', {})

expr.get('data', {}) returns a str or OrderedDict if expr has the key,
else a dict.  Unclean.

>          rets = expr.get('returns')
>          gen = expr.get('gen', True)
>          success_response = expr.get('success-response', True)
> -        if isinstance(data, OrderedDict):
> +        if isinstance(data, dict):

The test works since OrderedDict is also a dict.

>              data = self._make_implicit_object_type(
>                  name, info, 'arg', self._make_members(data, info))

I remember screwing up dict vs. OrderedDict once, and it failed when
some code really expected an OrderedDict.  _make_members() looks like
it's okay with dict, but why risk it?

Why not simply data = expr.get('data', ':empty') ?

>          if isinstance(rets, list):
> @@ -1364,8 +1364,8 @@ class QAPISchema(object):
>
>      def _def_event(self, expr, info):
>          name = expr['event']
> -        data = expr.get('data')
> -        if isinstance(data, OrderedDict):
> +        data = expr.get('data', {})
> +        if isinstance(data, dict):
>              data = self._make_implicit_object_type(
>                  name, info, 'arg', self._make_members(data, info))
>          self._def_entity(QAPISchemaEvent(name, info, data))

Same here.

> @@ -1612,8 +1612,7 @@ extern const char *const %(c_name)s_lookup[];
>
>
>  def gen_params(arg_type, extra):
> -    if not arg_type:
> -        return extra
> +    assert arg_type
>      assert not arg_type.variants
>      ret = ''
>      sep = ''
[Test output diffs snipped...]

Overall, a somewhat scary change, because all users of base need to be
checked.  The ones you actually changed are easy enough to see for a
reviewer.  Missed ones aren't.  Fortunately, we got a decent test suite.

Not sure this is worth the churn by itself, but let me read further to
see it in more context.