Eric Blake <ebl...@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 <ebl...@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...]