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

2016-03-08 Thread Markus Armbruster
Eric Blake  writes:

> On 03/08/2016 08:59 AM, Markus Armbruster wrote:
>> Eric Blake  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:
>>>
>
>>> +++ 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)
>
> Maybe. Patch 3 kept the assertion out of straight code refactoring, and
> patch 4 didn't use c_unboxed_type(), so this was the first place where I
> had to weaken the assertion.  But moving it into patch 4 doesn't seem
> like it would hurt, as it is still semantically related to the fact that
> we are planning on allowing an unboxed implicit type.

PATCH 04 drops the assertion from c_name().  Let's keep the two
consistent.

>>> -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!
>
> Yeah, it was a nice feeling to get to this point!



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

2016-03-08 Thread Eric Blake
On 03/08/2016 08:59 AM, Markus Armbruster wrote:
> Eric Blake  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:
>>

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

Maybe. Patch 3 kept the assertion out of straight code refactoring, and
patch 4 didn't use c_unboxed_type(), so this was the first place where I
had to weaken the assertion.  But moving it into patch 4 doesn't seem
like it would hurt, as it is still semantically related to the fact that
we are planning on allowing an unboxed implicit type.

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

Yeah, it was a nice feeling to get to this point!

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature


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

2016-03-08 Thread Markus Armbruster
Eric Blake  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 
>
> ---
> 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,