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

2016-10-20 Thread Eric Blake
On 10/20/2016 02:38 AM, Markus Armbruster wrote:

>> @@ -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.

Netdev is not ABI only because we decided to NOT apply the last patch of
QAPI-fying it in 2.7 while deciding to handle the back-compat
(non-?)issues that existing netdev_add QMP command accepts both 1 and
"1", but using the Netdev type would accept only 1.

While working towards making netdev_add use QAPI, I intentionally left
NetLegacy unchanged; but since NetLegacy is solely used by the command
line, feel free to rearrange that type as long as CLI back-compat is kept.

-- 
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 v14 20/21] net: convert to QObjectInputVisitor for -net/-netdev parsing

2016-10-20 Thread Markus Armbruster
"Daniel P. Berrange"  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 
> ---
>  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_