[Qemu-devel] [PATCH 0/2] Flatten simple unions where we still can

[Qemu-devel] [PATCH 0/2] Flatten simple unions where we still can

[Markus Armbruster]

Simple unions are simpler than flat unions in the schema, but more
complicated in C and on the QMP wire: there's extra indirection in C
and extra nesting on the wire, both pointless.  They're best avoided
in new code.

But we can do more: we can flatten the simple unions that aren't used
in QMP.

In the longer run, I'd love to get rid of simple unions entirely, and
reduce the notational overhead of flat unions in the schema.

Markus Armbruster (2):
  numa: Turn simple union NumaOptions into a flat union
  net: Turn simple union NetLegacyOptions into a flat union

 net/net.c        | 44 ++++++++++++++++++++++----------------------
 numa.c           |  4 ++--
 qapi-schema.json | 17 +++++++++++++++++
 3 files changed, 41 insertions(+), 24 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH 1/2] numa: Turn simple union NumaOptions into a flat union

[Markus Armbruster]

Simple unions are simpler than flat unions in the schema, but more
complicated in C and on the QMP wire: there's extra indirection in C
and extra nesting on the wire, both pointless.  They're best avoided
in new code.

NumaOptions isn't new, but it's only used internally, not in QMP.
Convert it to a flat union.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 numa.c           | 4 ++--
 qapi-schema.json | 8 ++++++++
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/numa.c b/numa.c
index 9f56be9..e01cb54 100644
--- a/numa.c
+++ b/numa.c
@@ -228,8 +228,8 @@ static int parse_numa(void *opaque, QemuOpts *opts, Error **errp)
     }
 
     switch (object->type) {
-    case NUMA_OPTIONS_KIND_NODE:
-        numa_node_parse(object->u.node.data, opts, &err);
+    case NUMA_OPTIONS_TYPE_NODE:
+        numa_node_parse(&object->u.node, opts, &err);
         if (err) {
             goto end;
         }
diff --git a/qapi-schema.json b/qapi-schema.json
index cbdffdd..f9a9941 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -5545,6 +5545,12 @@
             'events' : [ 'InputEvent' ] } }
 
 ##
+# @NumaOptionsType:
+##
+{ 'enum': 'NumaOptionsType',
+  'data': [ 'node' ] }
+
+##
 # @NumaOptions:
 #
 # A discriminated record of NUMA options. (for OptsVisitor)
@@ -5552,6 +5558,8 @@
 # Since: 2.1
 ##
 { 'union': 'NumaOptions',
+  'base': { 'type': 'NumaOptionsType' },
+  'discriminator': 'type',
   'data': {
     'node': 'NumaNodeOptions' }}
 
-- 
2.7.4

[Qemu-devel] [PATCH 2/2] net: Turn simple union NetLegacyOptions into a flat union

[Markus Armbruster]

Simple unions are simpler than flat unions in the schema, but more
complicated in C and on the QMP wire: there's extra indirection in C
and extra nesting on the wire, both pointless.  They're best avoided
in new code.

NetLegacyOptions isn't new, but it's only used internally, not in QMP.
Convert it to a flat union.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 net/net.c        | 44 ++++++++++++++++++++++----------------------
 qapi-schema.json |  9 +++++++++
 2 files changed, 31 insertions(+), 22 deletions(-)

diff --git a/net/net.c b/net/net.c
index 939fe31..9e0d309b4 100644
--- a/net/net.c
+++ b/net/net.c
@@ -992,47 +992,47 @@ static int net_client_init1(const void *object, bool is_netdev, Error **errp)
 
         /* Map the old options to the new flat type */
         switch (opts->type) {
-        case NET_LEGACY_OPTIONS_KIND_NONE:
+        case NET_LEGACY_OPTIONS_TYPE_NONE:
             return 0; /* nothing to do */
-        case NET_LEGACY_OPTIONS_KIND_NIC:
+        case NET_LEGACY_OPTIONS_TYPE_NIC:
             legacy.type = NET_CLIENT_DRIVER_NIC;
-            legacy.u.nic = *opts->u.nic.data;
+            legacy.u.nic = opts->u.nic;
             break;
-        case NET_LEGACY_OPTIONS_KIND_USER:
+        case NET_LEGACY_OPTIONS_TYPE_USER:
             legacy.type = NET_CLIENT_DRIVER_USER;
-            legacy.u.user = *opts->u.user.data;
+            legacy.u.user = opts->u.user;
             break;
-        case NET_LEGACY_OPTIONS_KIND_TAP:
+        case NET_LEGACY_OPTIONS_TYPE_TAP:
             legacy.type = NET_CLIENT_DRIVER_TAP;
-            legacy.u.tap = *opts->u.tap.data;
+            legacy.u.tap = opts->u.tap;
             break;
-        case NET_LEGACY_OPTIONS_KIND_L2TPV3:
+        case NET_LEGACY_OPTIONS_TYPE_L2TPV3:
             legacy.type = NET_CLIENT_DRIVER_L2TPV3;
-            legacy.u.l2tpv3 = *opts->u.l2tpv3.data;
+            legacy.u.l2tpv3 = opts->u.l2tpv3;
             break;
-        case NET_LEGACY_OPTIONS_KIND_SOCKET:
+        case NET_LEGACY_OPTIONS_TYPE_SOCKET:
             legacy.type = NET_CLIENT_DRIVER_SOCKET;
-            legacy.u.socket = *opts->u.socket.data;
+            legacy.u.socket = opts->u.socket;
             break;
-        case NET_LEGACY_OPTIONS_KIND_VDE:
+        case NET_LEGACY_OPTIONS_TYPE_VDE:
             legacy.type = NET_CLIENT_DRIVER_VDE;
-            legacy.u.vde = *opts->u.vde.data;
+            legacy.u.vde = opts->u.vde;
             break;
-        case NET_LEGACY_OPTIONS_KIND_DUMP:
+        case NET_LEGACY_OPTIONS_TYPE_DUMP:
             legacy.type = NET_CLIENT_DRIVER_DUMP;
-            legacy.u.dump = *opts->u.dump.data;
+            legacy.u.dump = opts->u.dump;
             break;
-        case NET_LEGACY_OPTIONS_KIND_BRIDGE:
+        case NET_LEGACY_OPTIONS_TYPE_BRIDGE:
             legacy.type = NET_CLIENT_DRIVER_BRIDGE;
-            legacy.u.bridge = *opts->u.bridge.data;
+            legacy.u.bridge = opts->u.bridge;
             break;
-        case NET_LEGACY_OPTIONS_KIND_NETMAP:
+        case NET_LEGACY_OPTIONS_TYPE_NETMAP:
             legacy.type = NET_CLIENT_DRIVER_NETMAP;
-            legacy.u.netmap = *opts->u.netmap.data;
+            legacy.u.netmap = opts->u.netmap;
             break;
-        case NET_LEGACY_OPTIONS_KIND_VHOST_USER:
+        case NET_LEGACY_OPTIONS_TYPE_VHOST_USER:
             legacy.type = NET_CLIENT_DRIVER_VHOST_USER;
-            legacy.u.vhost_user = *opts->u.vhost_user.data;
+            legacy.u.vhost_user = opts->u.vhost_user;
             break;
         default:
             abort();
@@ -1047,7 +1047,7 @@ static int net_client_init1(const void *object, bool is_netdev, Error **errp)
 
         /* Do not add to a vlan if it's a nic with a netdev= parameter. */
         if (netdev->type != NET_CLIENT_DRIVER_NIC ||
-            !opts->u.nic.data->has_netdev) {
+            !opts->u.nic.has_netdev) {
             peer = net_hub_add_port(net->has_vlan ? net->vlan : 0, NULL);
         }
     }
diff --git a/qapi-schema.json b/qapi-schema.json
index f9a9941..8ed5e2a 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -3934,6 +3934,13 @@
     'opts':  'NetLegacyOptions' } }
 
 ##
+# @NetLegacyOptionsType:
+##
+{ 'enum': 'NetLegacyOptionsType',
+  'data': ['none', 'nic', 'user', 'tap', 'l2tpv3', 'socket', 'vde',
+           'dump', 'bridge', 'netmap', 'vhost-user'] }
+
+##
 # @NetLegacyOptions:
 #
 # Like Netdev, but for use only by the legacy command line options
@@ -3941,6 +3948,8 @@
 # Since: 1.2
 ##
 { 'union': 'NetLegacyOptions',
+  'base': { 'type': 'NetLegacyOptionsType' },
+  'discriminator': 'type',
   'data': {
     'none':     'NetdevNoneOptions',
     'nic':      'NetLegacyNicOptions',
-- 
2.7.4

Re: [Qemu-devel] [PATCH 1/2] numa: Turn simple union NumaOptions into a flat union

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1060 bytes --]

On 02/08/2017 10:04 AM, Markus Armbruster wrote:
> Simple unions are simpler than flat unions in the schema, but more
> complicated in C and on the QMP wire: there's extra indirection in C
> and extra nesting on the wire, both pointless.  They're best avoided
> in new code.
> 
> NumaOptions isn't new, but it's only used internally, not in QMP.
> Convert it to a flat union.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  numa.c           | 4 ++--
>  qapi-schema.json | 8 ++++++++
>  2 files changed, 10 insertions(+), 2 deletions(-)
> 

> +++ b/qapi-schema.json
> @@ -5545,6 +5545,12 @@
>              'events' : [ 'InputEvent' ] } }
>  
>  ##
> +# @NumaOptionsType:
> +##

Do we want a 'Since: 2.1' designation (since the enum has effectively
existed, even if implicitly, for as long as NumaOptions has been around)?

With or without that extra line,
Reviewed-by: Eric Blake <eblake@redhat.com>


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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 2/2] net: Turn simple union NetLegacyOptions into a flat union

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1011 bytes --]

On 02/08/2017 10:04 AM, Markus Armbruster wrote:
> Simple unions are simpler than flat unions in the schema, but more
> complicated in C and on the QMP wire: there's extra indirection in C
> and extra nesting on the wire, both pointless.  They're best avoided
> in new code.
> 
> NetLegacyOptions isn't new, but it's only used internally, not in QMP.
> Convert it to a flat union.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  net/net.c        | 44 ++++++++++++++++++++++----------------------
>  qapi-schema.json |  9 +++++++++
>  2 files changed, 31 insertions(+), 22 deletions(-)
> 

> +++ b/qapi-schema.json
> @@ -3934,6 +3934,13 @@
>      'opts':  'NetLegacyOptions' } }
>  
>  ##
> +# @NetLegacyOptionsType:
> +##

Same question as previous patch about a 'Since: 1.2' designation.  Same
end result of:
Reviewed-by: Eric Blake <eblake@redhat.com>

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 1/2] numa: Turn simple union NumaOptions into a flat union

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 02/08/2017 10:04 AM, Markus Armbruster wrote:
>> Simple unions are simpler than flat unions in the schema, but more
>> complicated in C and on the QMP wire: there's extra indirection in C
>> and extra nesting on the wire, both pointless.  They're best avoided
>> in new code.
>> 
>> NumaOptions isn't new, but it's only used internally, not in QMP.
>> Convert it to a flat union.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  numa.c           | 4 ++--
>>  qapi-schema.json | 8 ++++++++
>>  2 files changed, 10 insertions(+), 2 deletions(-)
>> 
>
>> +++ b/qapi-schema.json
>> @@ -5545,6 +5545,12 @@
>>              'events' : [ 'InputEvent' ] } }
>>  
>>  ##
>> +# @NumaOptionsType:
>> +##
>
> Do we want a 'Since: 2.1' designation (since the enum has effectively
> existed, even if implicitly, for as long as NumaOptions has been around)?

Version tracking is useful only for things visible external interfaces,
which this is not.  Tool support to require it for external and reject
it for internal stuff would be nice.  Until then, we need to rely on
review to get external stuff tracked properly.

Tracking internal stuff is busywork.  If we think it's less work than
deciding internal vs. external, we can slap on "Since:" blindly.

> With or without that extra line,
> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks!