Re: [Qemu-devel] [PATCH v2 14/19] qapi: Don't special-case simple union wrappers

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> Simple unions were carrying a special case that hid their 'data'
> QMP member from the resulting C struct, via the hack method
> QAPISchemaObjectTypeVariant.simple_union_type().  But using the
> work we started by unboxing flat union and alternate branches, we
> expose the simple union's implicit type in qapi-types.h as an
> anonymous type, and drop our last use of the hack.
>
> | struct ImageInfoSpecific {
> |     ImageInfoSpecificKind type;
> |     union { /* union tag is @type */
> |         void *data;
> |-        ImageInfoSpecificQCow2 *qcow2;
> |-        ImageInfoSpecificVmdk *vmdk;
> |+        struct {
> |+            ImageInfoSpecificQCow2 *data;
> |+        } qcow2;
> |+        struct {
> |+            ImageInfoSpecificVmdk *data;
> |+        } vmdk;
> |     } u;
> | };
>
> All clients of simple unions have to adjust from using su->u.member
> to now using su->u.member.data;

By now, a reader not familiar with the code may wonder why this is an
improvement.  It is, because

* it removes an asymmetry between QAPI's QMP side and QAPI's C side
  (both now have 'data'), and

* it hopefully turns simple unions into sugar for flat unions as
  described in qapi-code-gen.txt, where before their equivalence only
  applied to the QMP side, not to the C side.

And that's well worth having to type .data in a few places.

Can we work that into the commit message?

>                                 while this touches a number of
> files in the tree, some earlier cleanup patches helped minimize
> the change to the initialization of a temporary variable rather
> than every single member access.  The generated qapi-visit.c code
> is included in the files affected by the layout change, with a

Suggest "is also affected by the layout change".

> diff that looks like:
>
> |@@ -4510,10 +4567,16 @@ static void visit_type_ImageInfoSpecific
> |     }
> |     switch (obj->type) {
> |     case IMAGE_INFO_SPECIFIC_KIND_QCOW2:
> |-        visit_type_ImageInfoSpecificQCow2(v, "data", &obj->u.qcow2, &err);
> |+        visit_type_ImageInfoSpecificQCow2(v, "data", &obj->u.qcow2.data, 
> &err);
> |+        if (err) {
> |+            goto out;
> |+        }
> |         break;
> |     case IMAGE_INFO_SPECIFIC_KIND_VMDK:
> |-        visit_type_ImageInfoSpecificVmdk(v, "data", &obj->u.vmdk, &err);
> |+        visit_type_ImageInfoSpecificVmdk(v, "data", &obj->u.vmdk.data, 
> &err);
> |+        if (err) {
> |+            goto out;
> |+        }
> |         break;
> |     default:
> |         abort();
> |     }
> |
> | out:
> |     error_propagate(errp, err);
>
> The added error checks there are a side effect of visiting all
> members of each implicit struct (there is only one such member for
> simple unions); but do not change semantics,

I'd say something like "Because we now use the general code for visiting
struct members, which must cope with multiple members, we get additional
error checks.  They're obviously harmless, and not worth suppressing in
the generator."

>                                              and will be important
> simple unions); but do not change semantics, and will be important
> when later patches allow for flat unions with anonymous branches
> with more than one member.  That future work will look like:
> { 'union': 'Foo', 'base': 'Base', 'discriminator': 'type',
>   'data': { 'branch1': { 'anonymous': 'str', 'number': 'int' },
>             'branch2': 'Named' } }

I'd probably forgo this remark on future plans.  It's not necessary to
justify the patch, and it could distract readers.

>
> Signed-off-by: Eric Blake <address@hidden>
>
> ---
> v2: rebase onto s/fields/members/ change, changes in master; pick
> up missing net/ files
> ---
>  scripts/qapi.py                 | 10 -----
>  scripts/qapi-types.py           | 22 ++++++++---
>  scripts/qapi-visit.py           | 15 +++-----
>  backends/baum.c                 |  2 +-
>  backends/msmouse.c              |  2 +-
>  block/nbd.c                     |  6 +--
>  block/qcow2.c                   |  6 +--
>  block/vmdk.c                    |  8 ++--
>  blockdev.c                      | 24 ++++++------
>  hmp.c                           |  8 ++--
>  hw/char/escc.c                  |  2 +-
>  hw/input/hid.c                  |  8 ++--
>  hw/input/ps2.c                  |  6 +--
>  hw/input/virtio-input-hid.c     |  8 ++--
>  hw/mem/pc-dimm.c                |  2 +-
>  net/dump.c                      |  2 +-
>  net/hub.c                       |  2 +-
>  net/l2tpv3.c                    |  2 +-
>  net/net.c                       |  4 +-
>  net/netmap.c                    |  2 +-
>  net/slirp.c                     |  2 +-
>  net/socket.c                    |  2 +-
>  net/tap-win32.c                 |  2 +-
>  net/tap.c                       |  4 +-
>  net/vde.c                       |  2 +-
>  net/vhost-user.c                |  2 +-
>  numa.c                          |  4 +-
>  qemu-char.c                     | 82 
> +++++++++++++++++++++--------------------
>  qemu-nbd.c                      |  6 +--
>  replay/replay-input.c           | 44 +++++++++++-----------
>  spice-qemu-char.c               | 14 ++++---
>  tests/test-io-channel-socket.c  | 40 ++++++++++----------
>  tests/test-qmp-commands.c       |  2 +-
>  tests/test-qmp-input-visitor.c  | 25 +++++++------
>  tests/test-qmp-output-visitor.c | 24 ++++++------
>  tpm.c                           |  2 +-
>  ui/console.c                    |  4 +-
>  ui/input-keymap.c               | 10 ++---
>  ui/input-legacy.c               |  8 ++--
>  ui/input.c                      | 34 ++++++++---------
>  ui/vnc-auth-sasl.c              |  3 +-
>  ui/vnc.c                        | 29 ++++++++-------
>  util/qemu-sockets.c             | 35 +++++++++---------
>  43 files changed, 263 insertions(+), 258 deletions(-)
>
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index 83080b3..38121c5 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -1115,16 +1115,6 @@ class 
> QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember):
>      def __init__(self, name, typ):
>          QAPISchemaObjectTypeMember.__init__(self, name, typ, False)
>
> -    # This function exists to support ugly simple union special cases
> -    # TODO get rid of them, and drop the function
> -    def simple_union_type(self):
> -        if (self.type.is_implicit() and
> -                isinstance(self.type, QAPISchemaObjectType)):
> -            assert len(self.type.members) == 1
> -            assert not self.type.variants
> -            return self.type.members[0].type
> -        return None
> -
>
>  class QAPISchemaAlternateType(QAPISchemaType):
>      def __init__(self, name, info, variants):
> diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
> index 6c1923d..1f090e6 100644
> --- a/scripts/qapi-types.py
> +++ b/scripts/qapi-types.py
> @@ -122,14 +122,24 @@ def gen_variants(variants):
>                  c_name=c_name(variants.tag_member.name))
>
>      for var in variants.variants:
> -        # Ugly special case for simple union TODO get rid of it
> -        simple_union_type = var.simple_union_type()
> -        typ = simple_union_type or var.type
> -        ret += mcgen('''
> +        if (isinstance(var.type, QAPISchemaObjectType) and
> +                var.type.is_implicit()):

Uh, this condition is exactly var.type.simple_union_type() != None.  I'm
afraid we still have a special case.

> +            ret += mcgen('''
> +        struct {
> +''')
> +            push_indent(8)
> +            ret += gen_struct_members(var.type.members)
> +            pop_indent(8)
> +            ret += mcgen('''
> +        } %(c_name)s;
> +''',
> +                         c_name=c_name(var.name))
> +        else:
> +            ret += mcgen('''
>          %(c_type)s %(c_name)s;
>  ''',
> -                     c_type=typ.c_type(is_unboxed=not simple_union_type),
> -                     c_name=c_name(var.name))
> +                         c_type=var.type.c_type(is_unboxed=True),
> +                         c_name=c_name(var.name))
>
>      ret += mcgen('''
>      } u;

Before:

       for var in variants.variants:
           # Ugly special case for simple union TODO get rid of it
           simple_union_type = var.simple_union_type()
           typ = simple_union_type or var.type
           ret += mcgen('''
           %(c_type)s %(c_name)s;
   ''',
                        c_type=typ.c_type(is_unboxed=not simple_union_type),
                        c_name=c_name(var.name))

Special treatment for simple unions: don't unbox.

After:

       for var in variants.variants:
           if (isinstance(var.type, QAPISchemaObjectType) and
                   var.type.is_implicit()):
               ret += mcgen('''
           struct {
   ''')
               push_indent(8)
               ret += gen_struct_members(var.type.members)
               pop_indent(8)
               ret += mcgen('''
           } %(c_name)s;
   ''',
                            c_name=c_name(var.name))
           else:
               ret += mcgen('''
           %(c_type)s %(c_name)s;
   ''',
                            c_type=var.type.c_type(is_unboxed=True),
                            c_name=c_name(var.name))

Special treatment for simple unions: instead of a member

    TypeOfBranch name_of_branch;

we generate one

    struct {
        TypeOfBranch data;
    } name_of_branch;

Example: qapi-schema-test.json's only simple union
UserDefNativeListUnion

    { 'union': 'UserDefNativeListUnion',
      'data': { 'integer': ['int'],
                [more of the same...] } }

qapi-schema-test.out:

    object UserDefNativeListUnion
        member type: UserDefNativeListUnionKind optional=False
        case integer: :obj-intList-wrapper
        [more of the same...]

where

    object :obj-intList-wrapper
        member data: intList optional=False

qapi-types.h:

    struct UserDefNativeListUnion {
        UserDefNativeListUnionKind type;
        union { /* union tag is @type */
            struct {
                intList *data;
            } integer;
            [more of the same...]
        } u;
    }

Without the special case, we'd get

    typedef struct :obj-intList-wrapper :obj-intList-wrapper;

    struct :obj-intList-wrapper {
        intList *data;
    } :obj-intList-wrapper;

    struct UserDefNativeListUnion {
        UserDefNativeListUnionKind type;
        union { /* union tag is @type */
            :obj-intList-wrapper integer;
            [more of the same...]
        } u;
    }

except QAPISchemaObjectType.c_name() would refuse to cooperate in
creating this nonsense.

Conclusion: you replace one special case by another one.  The
improvement is in that the new special case is less special.  Instead of
"if simple union variant, do something else", we now have

    Use the C type corresponding to the type, except when it's an
    implicit object type, use an anonymous struct type, because we don't
    have a C type then.

Should we have a C type even then?  We'd need to give it a reserved
name.

On first glance, the new special case is just as special at the old one:
it applies to simple unions.  But that's not necessarily so.  We could
make use of it elsewhere if we wanted.  We'd have to factor the code out
of the "for variants" loop, of course.  In other words, it's still
special, but its specialness is less arbitrary.  That's why it's an
improvement.

Next is the visit update for this change of the type layout.

> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
> index a9de393..e281d21 100644
> --- a/scripts/qapi-visit.py
> +++ b/scripts/qapi-visit.py
> @@ -34,24 +34,20 @@ void visit_type_%(c_name)s_members(Visitor *v, %(c_name)s 
> *obj, Error **errp);
>                   c_name=c_name(name))
>
>
> -def gen_visit_members_call(typ, c_name):
> +def gen_visit_members_call(typ, direct_name, implicit_name=None):

As noted in my review of PATCH 10, these are C expressions, not names.

>      ret = ''
>      assert isinstance(typ, QAPISchemaObjectType)
>      if typ.is_empty():
>          pass
>      elif typ.is_implicit():
> -        # TODO ugly special case for simple union
> -        assert len(typ.members) == 1
> +        assert implicit_name
>          assert not typ.variants
> -        ret += mcgen('''
> -    visit_type_%(c_type)s(v, "data", %(c_name)s, &err);
> -''',
> -                     c_type=typ.members[0].type.c_name(), c_name=c_name)
> +        ret += gen_visit_members(typ.members, prefix=implicit_name)
>      else:
>          ret += mcgen('''
>      visit_type_%(c_type)s_members(v, %(c_name)s, &err);
>  ''',
> -                     c_type=typ.c_name(), c_name=c_name)
> +                     c_type=typ.c_name(), c_name=direct_name)
>      return ret
>
>

Note that direct_name is only used when !typ.is_implicit(), and
implicit_name is only used when typ.is_implicit().

Further note that despite its name, gen_visit_members_call() doesn't
generate a call when typ.is_implicit().

Separate function for implicit type?

Before:

       if typ.is_empty():
           pass
       elif typ.is_implicit():
           # TODO ugly special case for simple union
           assert len(typ.members) == 1
           assert not typ.variants
           ret += mcgen('''
       visit_type_%(c_type)s(v, "data", %(c_name)s, &err);
   ''',
                        c_type=typ.members[0].type.c_name(), c_name=c_name)
       else:
           ret += mcgen('''
       visit_type_%(c_type)s_members(v, %(c_name)s, &err);
   ''',
                        c_type=typ.c_name(), c_name=c_name)

Special treatment for simple unions: don't unbox.  To visit a boxed
member of type T, we call visit_type_T().  To visit an unboxed one, we
call visit_type_T_members().

After:

       if typ.is_empty():
           pass
       elif typ.is_implicit():
           assert implicit_name
           assert not typ.variants
           ret += gen_visit_members(typ.members, prefix=implicit_name)
       else:
           ret += mcgen('''
       visit_type_%(c_type)s_members(v, %(c_name)s, &err);
   ''',
                        c_type=typ.c_name(), c_name=direct_name)

Special treatment for member of implicit type: generate inline code to
visit its members, because visit_type_T_members() doesn't exist then.

Should it exist?

> @@ -86,7 +82,8 @@ void visit_type_%(c_name)s_members(Visitor *v, %(c_name)s 
> *obj, Error **errp)
>                                             variants.tag_member.type.prefix))
>              push_indent()
>              ret += gen_visit_members_call(var.type,
> -                                          '&obj->u.' + c_name(var.name))
> +                                          '&obj->u.' + c_name(var.name),
> +                                          'obj->u.' + c_name(var.name) + '.')
>              pop_indent()
>              ret += mcgen('''
>          break;

On to the boring part.

> diff --git a/backends/baum.c b/backends/baum.c
> index c11320e..eef3467 100644
> --- a/backends/baum.c
> +++ b/backends/baum.c
> @@ -567,7 +567,7 @@ static CharDriverState *chr_baum_init(const char *id,
>                                        ChardevReturn *ret,
>                                        Error **errp)
>  {
> -    ChardevCommon *common = backend->u.braille;
> +    ChardevCommon *common = backend->u.braille.data;
>      BaumDriverState *baum;
>      CharDriverState *chr;
>      brlapi_handle_t *handle;

Many trivial updates like this one.  The only interesting question is
whether you got them all.  What did you do to find them?

[...]