Re: [Qemu-devel] [PATCH v14 20/21] net: convert to QObjectInputVisitor for -net/-netdev parsing

[Markus Armbruster]

"Daniel P. Berrange" <address@hidden> writes:

> The -net/-netdev command line parsing code uses OptsVisitor
> for parsing options to populate NetLegacy or NetDev struct
> respectively. Although those structs have nesting, the
> OptsVisitor flattens them, so we must enable compatibility
> options to auto-create structs. This allows the legacy
> syntax
>
>   -net tap,id=net0,vlan=3,fd=3,script=/bin/ifup-qemu
>
> to be treated as equivalent to the modern QAPI based
> syntax
>
>   -net 
> id=net0,vlan=3,opts.type=tap,opts.data.fd=3,opts.data.script=/bin/ifup-qemu
>
> Signed-off-by: Daniel P. Berrange <address@hidden>
> ---
>  net/net.c | 18 ++++++++++++++++--
>  1 file changed, 16 insertions(+), 2 deletions(-)
>
> diff --git a/net/net.c b/net/net.c
> index ec984bf..de6bf8e 100644
> --- a/net/net.c
> +++ b/net/net.c
> @@ -43,7 +43,7 @@
>  #include "qemu/iov.h"
>  #include "qemu/main-loop.h"
>  #include "qapi-visit.h"
> -#include "qapi/opts-visitor.h"
> +#include "qapi/qobject-input-visitor.h"
>  #include "sysemu/sysemu.h"
>  #include "net/filter.h"
>  #include "qapi/string-output-visitor.h"
> @@ -1069,7 +1069,21 @@ int net_client_init(QemuOpts *opts, bool is_netdev, 
> Error **errp)
>      void *object = NULL;
>      Error *err = NULL;
>      int ret = -1;
> -    Visitor *v = opts_visitor_new(opts);
> +    /*
> +     * Needs autocreate_lists=true in order support existing
> +     * syntax for list options where the bare key is repeated
> +     *
> +     * Needs autocreate_struct_levels=3 in order to deal with
> +     * 3 level nesting in NetLegacy option args, which was
> +     * exposed as a flat namespace with OptVisitor
> +     */
> +    Visitor *v = qobject_input_visitor_new_opts(opts, true, 3, false, true,
> +                                                &err);
> +
> +    if (err) {
> +        error_propagate(errp, err);
> +        return -1;
> +    }
>  
>      {
>          /* Parse convenience option format ip6-net=fec0::0[/64] */

Neither NetLegacy nor Netdev are ABI, so if I understand the problem,
perhaps I can find a way around it.  Let's figure out what exactly
requires levels=3.

Your commit message shows opts.* and opts.data.* in -net.  Anything
else?

I can see opts.* in the QAPI schema: it's NetLegacy member @opts, of
union type NetLegacyOptions.  It's boxed on the wire, as any member of
complex type is.

I can also see opts.data.*: NetLegacyOptions is a simple union, which
wraps 'data': { ... } around the variant on the wire.

The latter wrapping is easy to avoid: make NetLegacyOptions a flat
union.

The former wrapping isn't so easy.  QAPI can't unbox complex members.
The only way to include a type unboxed is making it the base type.  But
base types must be struct, and @opts is a union.

However, if you take a step back you can see that the (flat) argument of
-net / -netdev is fundamentally just a (flat) union!  The nesting is an
artifact of the clumsy way it's defined in the QAPI schema.  Sketch
doing it less clumsily appended; look ma, no nesting!  Compile-tested
only.



diff --git a/net/net.c b/net/net.c
index ec984bf..f2cbf28 100644
--- a/net/net.c
+++ b/net/net.c
@@ -984,55 +984,54 @@ static int net_client_init1(const void *object, bool 
is_netdev, Error **errp)
         }
     } else {
         const NetLegacy *net = object;
-        const NetLegacyOptions *opts = net->opts;
         legacy.id = net->id;
         netdev = &legacy;
         /* missing optional values have been initialized to "all bits zero" */
         name = net->has_id ? net->id : net->name;
 
         /* Map the old options to the new flat type */
-        switch (opts->type) {
-        case NET_LEGACY_OPTIONS_KIND_NONE:
+        switch (net->type) {
+        case NET_LEGACY_TYPE_NONE:
             return 0; /* nothing to do */
-        case NET_LEGACY_OPTIONS_KIND_NIC:
+        case NET_LEGACY_TYPE_NIC:
             legacy.type = NET_CLIENT_DRIVER_NIC;
-            legacy.u.nic = *opts->u.nic.data;
+            legacy.u.nic = net->u.nic;
             break;
-        case NET_LEGACY_OPTIONS_KIND_USER:
+        case NET_LEGACY_TYPE_USER:
             legacy.type = NET_CLIENT_DRIVER_USER;
-            legacy.u.user = *opts->u.user.data;
+            legacy.u.user = net->u.user;
             break;
-        case NET_LEGACY_OPTIONS_KIND_TAP:
+        case NET_LEGACY_TYPE_TAP:
             legacy.type = NET_CLIENT_DRIVER_TAP;
-            legacy.u.tap = *opts->u.tap.data;
+            legacy.u.tap = net->u.tap;
             break;
-        case NET_LEGACY_OPTIONS_KIND_L2TPV3:
+        case NET_LEGACY_TYPE_L2TPV3:
             legacy.type = NET_CLIENT_DRIVER_L2TPV3;
-            legacy.u.l2tpv3 = *opts->u.l2tpv3.data;
+            legacy.u.l2tpv3 = net->u.l2tpv3;
             break;
-        case NET_LEGACY_OPTIONS_KIND_SOCKET:
+        case NET_LEGACY_TYPE_SOCKET:
             legacy.type = NET_CLIENT_DRIVER_SOCKET;
-            legacy.u.socket = *opts->u.socket.data;
+            legacy.u.socket = net->u.socket;
             break;
-        case NET_LEGACY_OPTIONS_KIND_VDE:
+        case NET_LEGACY_TYPE_VDE:
             legacy.type = NET_CLIENT_DRIVER_VDE;
-            legacy.u.vde = *opts->u.vde.data;
+            legacy.u.vde = net->u.vde;
             break;
-        case NET_LEGACY_OPTIONS_KIND_DUMP:
+        case NET_LEGACY_TYPE_DUMP:
             legacy.type = NET_CLIENT_DRIVER_DUMP;
-            legacy.u.dump = *opts->u.dump.data;
+            legacy.u.dump = net->u.dump;
             break;
-        case NET_LEGACY_OPTIONS_KIND_BRIDGE:
+        case NET_LEGACY_TYPE_BRIDGE:
             legacy.type = NET_CLIENT_DRIVER_BRIDGE;
-            legacy.u.bridge = *opts->u.bridge.data;
+            legacy.u.bridge = net->u.bridge;
             break;
-        case NET_LEGACY_OPTIONS_KIND_NETMAP:
+        case NET_LEGACY_TYPE_NETMAP:
             legacy.type = NET_CLIENT_DRIVER_NETMAP;
-            legacy.u.netmap = *opts->u.netmap.data;
+            legacy.u.netmap = net->u.netmap;
             break;
-        case NET_LEGACY_OPTIONS_KIND_VHOST_USER:
+        case NET_LEGACY_TYPE_VHOST_USER:
             legacy.type = NET_CLIENT_DRIVER_VHOST_USER;
-            legacy.u.vhost_user = *opts->u.vhost_user.data;
+            legacy.u.vhost_user = net->u.vhost_user;
             break;
         default:
             abort();
@@ -1047,7 +1046,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) {
+            !net->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 4838258..d863418 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -2915,21 +2915,16 @@
 #
 # Since 1.2
 ##
-{ 'struct': 'NetLegacy',
-  'data': {
+{ 'enum': 'NetLegacyType',
+  'data': [ 'none', 'nic', 'user', 'tap', 'l2tpv3', 'socket', 'vde',
+            'dump', 'bridge', 'netmap', 'vhost-user' ] }
+{ 'union': 'NetLegacy',
+  'base': {
+    'type': 'NetLegacyType',
     '*vlan': 'int32',
     '*id':   'str',
-    '*name': 'str',
-    'opts':  'NetLegacyOptions' } }
-
-##
-# @NetLegacyOptions
-#
-# Like Netdev, but for use only by the legacy command line options
-#
-# Since 1.2
-##
-{ 'union': 'NetLegacyOptions',
+    '*name': 'str' },
+  'discriminator': 'type',
   'data': {
     'none':     'NetdevNoneOptions',
     'nic':      'NetLegacyNicOptions',