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

[Qemu-devel] [PATCH v2 0/3] 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.

v2:
* PATCH 1+2: Since: added [Eric], commit message tweaked
* PATCH 3: new

Markus Armbruster (3):
  numa: Flatten simple union NumaOptions
  net: Flatten simple union NetLegacyOptions
  i386/cpu: net: Flatten simple union GuestPanicInformationType

 net/net.c         | 44 ++++++++++++++++++++++----------------------
 numa.c            |  4 ++--
 qapi-schema.json  | 35 +++++++++++++++++++++++++++++++++--
 target/i386/cpu.c | 17 ++++++-----------
 vl.c              | 12 ++++++------
 5 files changed, 69 insertions(+), 43 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH v2 1/3] numa: Flatten simple union NumaOptions

[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>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 numa.c           |  4 ++--
 qapi-schema.json | 10 ++++++++++
 2 files changed, 12 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 e9a6364..a448ea8 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -5571,6 +5571,14 @@
             'events' : [ 'InputEvent' ] } }
 
 ##
+# @NumaOptionsType:
+#
+# Since: 2.1
+##
+{ 'enum': 'NumaOptionsType',
+  'data': [ 'node' ] }
+
+##
 # @NumaOptions:
 #
 # A discriminated record of NUMA options. (for OptsVisitor)
@@ -5578,6 +5586,8 @@
 # Since: 2.1
 ##
 { 'union': 'NumaOptions',
+  'base': { 'type': 'NumaOptionsType' },
+  'discriminator': 'type',
   'data': {
     'node': 'NumaNodeOptions' }}
 
-- 
2.7.4

[Qemu-devel] [PATCH v2 2/3] net: Flatten simple union NetLegacyOptions

[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>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 net/net.c        | 44 ++++++++++++++++++++++----------------------
 qapi-schema.json | 11 +++++++++++
 2 files changed, 33 insertions(+), 22 deletions(-)

diff --git a/net/net.c b/net/net.c
index fb7af3a..0ac3b9e 100644
--- a/net/net.c
+++ b/net/net.c
@@ -993,47 +993,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();
@@ -1048,7 +1048,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 a448ea8..5347781 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -3959,6 +3959,15 @@
     'opts':  'NetLegacyOptions' } }
 
 ##
+# @NetLegacyOptionsType:
+#
+# Since: 1.2
+##
+{ '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
@@ -3966,6 +3975,8 @@
 # Since: 1.2
 ##
 { 'union': 'NetLegacyOptions',
+  'base': { 'type': 'NetLegacyOptionsType' },
+  'discriminator': 'type',
   'data': {
     'none':     'NetdevNoneOptions',
     'nic':      'NetLegacyNicOptions',
-- 
2.7.4

[Qemu-devel] [PATCH v2 3/3] i386/cpu: net: Flatten simple union GuestPanicInformationType

[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.  Fix up recent commit d187e08 accordingly.

Cc: Anton Nefedov <anton.nefedov@virtuozzo.com>
Cc: Denis V. Lunev <den@openvz.org>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 qapi-schema.json  | 14 ++++++++++++--
 target/i386/cpu.c | 17 ++++++-----------
 vl.c              | 12 ++++++------
 3 files changed, 24 insertions(+), 19 deletions(-)

diff --git a/qapi-schema.json b/qapi-schema.json
index 5347781..0eef37c 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -5893,14 +5893,24 @@
   'data': [ 'pause', 'poweroff' ] }
 
 ##
+# @GuestPanicInformationType:
+#
+# Since: 2.9
+##
+{ 'enum': 'GuestPanicInformationType',
+  'data': [ 'hyper-v' ] }
+
+##
 # @GuestPanicInformation:
 #
 # Information about a guest panic
 #
 # Since: 2.9
 ##
-{'union': 'GuestPanicInformation',
- 'data': { 'hyper-v': 'GuestPanicInformationHyperV' } }
+{ 'union': 'GuestPanicInformation',
+  'base': { 'type': 'GuestPanicInformationType' },
+  'discriminator': 'type',
+  'data': { 'hyper-v': 'GuestPanicInformationHyperV' } }
 
 ##
 # @GuestPanicInformationHyperV:
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index fd7add2..c3e6b74 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -3502,19 +3502,14 @@ static GuestPanicInformation *x86_cpu_get_crash_info(CPUState *cs)
     GuestPanicInformation *panic_info = NULL;
 
     if (env->features[FEAT_HYPERV_EDX] & HV_X64_GUEST_CRASH_MSR_AVAILABLE) {
-        GuestPanicInformationHyperV *panic_info_hv =
-            g_malloc0(sizeof(GuestPanicInformationHyperV));
         panic_info = g_malloc0(sizeof(GuestPanicInformation));
-
-        panic_info->type = GUEST_PANIC_INFORMATION_KIND_HYPER_V;
-        panic_info->u.hyper_v.data = panic_info_hv;
-
+        panic_info->type = GUEST_PANIC_INFORMATION_TYPE_HYPER_V;
         assert(HV_X64_MSR_CRASH_PARAMS >= 5);
-        panic_info_hv->arg1 = env->msr_hv_crash_params[0];
-        panic_info_hv->arg2 = env->msr_hv_crash_params[1];
-        panic_info_hv->arg3 = env->msr_hv_crash_params[2];
-        panic_info_hv->arg4 = env->msr_hv_crash_params[3];
-        panic_info_hv->arg5 = env->msr_hv_crash_params[4];
+        panic_info->u.hyper_v.arg1 = env->msr_hv_crash_params[0];
+        panic_info->u.hyper_v.arg2 = env->msr_hv_crash_params[1];
+        panic_info->u.hyper_v.arg3 = env->msr_hv_crash_params[2];
+        panic_info->u.hyper_v.arg4 = env->msr_hv_crash_params[3];
+        panic_info->u.hyper_v.arg5 = env->msr_hv_crash_params[4];
     }
 
     return panic_info;
diff --git a/vl.c b/vl.c
index b5d0a19..e307ae0 100644
--- a/vl.c
+++ b/vl.c
@@ -1697,14 +1697,14 @@ void qemu_system_guest_panicked(GuestPanicInformation *info)
     }
 
     if (info) {
-        if (info->type == GUEST_PANIC_INFORMATION_KIND_HYPER_V) {
+        if (info->type == GUEST_PANIC_INFORMATION_TYPE_HYPER_V) {
             qemu_log_mask(LOG_GUEST_ERROR, "HV crash parameters: (%#"PRIx64
                           " %#"PRIx64" %#"PRIx64" %#"PRIx64" %#"PRIx64")\n",
-                          info->u.hyper_v.data->arg1,
-                          info->u.hyper_v.data->arg2,
-                          info->u.hyper_v.data->arg3,
-                          info->u.hyper_v.data->arg4,
-                          info->u.hyper_v.data->arg5);
+                          info->u.hyper_v.arg1,
+                          info->u.hyper_v.arg2,
+                          info->u.hyper_v.arg3,
+                          info->u.hyper_v.arg4,
+                          info->u.hyper_v.arg5);
         }
         qapi_free_GuestPanicInformation(info);
     }
-- 
2.7.4

Re: [Qemu-devel] [PATCH v2 3/3] i386/cpu: net: Flatten simple union GuestPanicInformationType

[Eric Blake]

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

On 02/21/2017 02:46 PM, 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.  Fix up recent commit d187e08 accordingly.
> 
> Cc: Anton Nefedov <anton.nefedov@virtuozzo.com>
> Cc: Denis V. Lunev <den@openvz.org>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  qapi-schema.json  | 14 ++++++++++++--
>  target/i386/cpu.c | 17 ++++++-----------
>  vl.c              | 12 ++++++------
>  3 files changed, 24 insertions(+), 19 deletions(-)

Very similar to Anton's patch which is on Paolo's queue:
https://lists.gnu.org/archive/html/qemu-devel/2017-02/msg04448.html

I don't care which version goes in, but will leave it to Markus and
Paolo to decide which queue it should go through.

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 v2 3/3] i386/cpu: net: Flatten simple union GuestPanicInformationType

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 02/21/2017 02:46 PM, 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.  Fix up recent commit d187e08 accordingly.
>> 
>> Cc: Anton Nefedov <anton.nefedov@virtuozzo.com>
>> Cc: Denis V. Lunev <den@openvz.org>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  qapi-schema.json  | 14 ++++++++++++--
>>  target/i386/cpu.c | 17 ++++++-----------
>>  vl.c              | 12 ++++++------
>>  3 files changed, 24 insertions(+), 19 deletions(-)
>
> Very similar to Anton's patch which is on Paolo's queue:
> https://lists.gnu.org/archive/html/qemu-devel/2017-02/msg04448.html

Wasn't cc'ed, so I missed it.  No biggie.

> I don't care which version goes in, but will leave it to Markus and
> Paolo to decide which queue it should go through.

My commit message is more verbose, and I fix space style in the QAPI
schema.  On the other hand, Anton has an extra QAPI schema comment line.

Picking Anton's is probably simpler, because it's 2/3, and mine looks
like it conflicts with his 1/3.

> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks!