Re: [Qemu-devel] [PATCH v4 07/10] qapi: Don't special-case simple union wrappers

[Markus Armbruster <armbru@redhat.com>]

Eric Blake <eblake@redhat.com> 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 by using
> the work we started by unboxing flat union and alternate
> branches, coupled with the ability to visit the members of an
> implicit type, we can now expose the simple union's implicit
> type in qapi-types.h:
>
> | struct _obj_ImageInfoSpecificQCow2_wrapper {
> |     ImageInfoSpecificQCow2 *data;
> | };
> |
> | struct _obj_ImageInfoSpecificVmdk_wrapper {
> |     ImageInfoSpecificVmdk *data;
> | };
> ...
> | struct ImageInfoSpecific {
> |     ImageInfoSpecificKind type;
> |     union { /* union tag is @type */
> |         void *data;
> |-        ImageInfoSpecificQCow2 *qcow2;
> |-        ImageInfoSpecificVmdk *vmdk;
> |+        _obj_ImageInfoSpecificQCow2_wrapper qcow2;
> |+        _obj_ImageInfoSpecificVmdk_wrapper vmdk;
> |     } u;
> | };
>
> Doing this removes asymmetry between QAPI's QMP side and its
> C side (both sides now expose 'data'), and means that the
> treatment of a simple union as sugar for a flat union is now
> equivalent in both languages (previously the two approaches used
> a different layer of dereferencing, where the simple union could
> be converted to a flat union with equivalent C layout but
> different {} on the wire, or to an equivalent QMP wire form
> but with different C representation).  Using the implicit type
> also lets us get rid of the simple_union_type() hack.
>
> Of course, now all clients of simple unions have to adjust from
> using su->u.member to using su->u.member.data; 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 also affected by the layout change:
>
> |@@ -7393,10 +7393,10 @@ void visit_type_ImageInfoSpecific_member
> |     }
> |     switch (obj->type) {
> |     case IMAGE_INFO_SPECIFIC_KIND_QCOW2:
> |-        visit_type_ImageInfoSpecificQCow2(v, "data", &obj->u.qcow2, &err);
> |+        visit_type__obj_ImageInfoSpecificQCow2_wrapper_members(v, &obj->u.qcow2, &err);
> |         break;
> |     case IMAGE_INFO_SPECIFIC_KIND_VMDK:
> |-        visit_type_ImageInfoSpecificVmdk(v, "data", &obj->u.vmdk, &err);
> |+        visit_type__obj_ImageInfoSpecificVmdk_wrapper_members(v, &obj->u.vmdk, &err);
> |         break;
> |     default:
> |         abort();
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
>
> ---
> v4: improve commit message, rebase onto exposed implicit types
> [no v3]
> v2: rebase onto s/fields/members/ change, changes in master; pick
> up missing net/ files
> ---
>  scripts/qapi.py                 | 11 ------
>  scripts/qapi-types.py           |  8 +---
>  scripts/qapi-visit.py           | 22 ++---------
>  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, 246 insertions(+), 269 deletions(-)
>
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index 797ac7a..0574b8b 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -1006,7 +1006,6 @@ class QAPISchemaObjectType(QAPISchemaType):
>          return c_name(self.name) + pointer_suffix
>
>      def c_unboxed_type(self):
> -        assert not self.is_implicit()

Doesn't this belong into PATCH 04?

>          return c_name(self.name)
>
>      def json_type(self):
> @@ -1126,16 +1125,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 fa2d6af..b8499ac 100644
> --- a/scripts/qapi-types.py
> +++ b/scripts/qapi-types.py
> @@ -121,16 +121,10 @@ 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()
> -        if simple_union_type:
> -            typ = simple_union_type.c_type()
> -        else:
> -            typ = var.type.c_unboxed_type()
>          ret += mcgen('''
>          %(c_type)s %(c_name)s;
>  ''',
> -                     c_type=typ,
> +                     c_type=var.type.c_unboxed_type(),
>                       c_name=c_name(var.name))
>
>      ret += mcgen('''
> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
> index b4303a7..afbaeeb 100644
> --- a/scripts/qapi-visit.py
> +++ b/scripts/qapi-visit.py
> @@ -60,29 +60,15 @@ void visit_type_%(c_name)s_members(Visitor *v, %(c_name)s *obj, Error **errp)
>                       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('''
>      case %(case)s:
> +        visit_type_%(c_type)s_members(v, &obj->u.%(c_name)s, &err);
> +        break;
>  ''',
>                           case=c_enum_const(variants.tag_member.type.name,
>                                             var.name,
> -                                           variants.tag_member.type.prefix))
> -            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))
> -            else:
> -                ret += mcgen('''
> -        visit_type_%(c_type)s_members(v, &obj->u.%(c_name)s, &err);
> -''',
> -                             c_type=var.type.c_name(),
> -                             c_name=c_name(var.name))
> -            ret += mcgen('''
> -        break;
> -''')
> +                                           variants.tag_member.type.prefix),
> +                         c_type=var.type.c_name(), c_name=c_name(var.name))
>
>          ret += mcgen('''
>      default:

This stupid special case has given us enough trouble, good to see it
gone!

[Tedious part snipped, it looks good to me...]