Re: [Qemu-ppc] [Qemu-devel] [PATCH 3/5] qapi: change Netdev and NetLegacy into a flat union

[Markus Armbruster]

"Kővágó, Zoltán" <address@hidden> writes:

> Signed-off-by: Kővágó, Zoltán <address@hidden>
[...]
> diff --git a/hw/arm/musicpal.c b/hw/arm/musicpal.c
> index a3b1314..72e2f8f 100644
> --- a/hw/arm/musicpal.c
> +++ b/hw/arm/musicpal.c
> @@ -379,7 +379,7 @@ static void eth_cleanup(NetClientState *nc)
>  }
>  
>  static NetClientInfo net_mv88w8618_info = {
> -    .type = NET_CLIENT_OPTIONS_KIND_NIC,
> +    .type = NET_CLIENT_DRIVER_NIC,
>      .size = sizeof(NICState),
>      .can_receive = eth_can_receive,
>      .receive = eth_receive,
[Many more renames snipped, mind-numbing, hope it's nothing but renames...]
> diff --git a/net/clients.h b/net/clients.h
> index d47530e..2aac0ee 100644
> --- a/net/clients.h
> +++ b/net/clients.h
> @@ -27,39 +27,39 @@
>  #include "net/net.h"
>  #include "qapi-types.h"
>  
> -int net_init_dump(const NetClientOptions *opts, const char *name,
> +int net_init_dump(const void *opts, const char *name,
>                    NetClientState *peer, Error **errp);

Why do we need to bypass the type system now?

Hmm, the reason is the conversion to flat unions loses type
NetClientOptions, member of both Netdev and NetLegacy, because the type
gets inlined into both containing types.  More below.

>  
>  #ifdef CONFIG_SLIRP
> -int net_init_slirp(const NetClientOptions *opts, const char *name,
> +int net_init_slirp(const void *opts, const char *name,
>                     NetClientState *peer, Error **errp);
>  #endif
>  
> -int net_init_hubport(const NetClientOptions *opts, const char *name,
> +int net_init_hubport(const void *opts, const char *name,
>                       NetClientState *peer, Error **errp);
>  
> -int net_init_socket(const NetClientOptions *opts, const char *name,
> +int net_init_socket(const void *opts, const char *name,
>                      NetClientState *peer, Error **errp);
>  
> -int net_init_tap(const NetClientOptions *opts, const char *name,
> +int net_init_tap(const void *opts, const char *name,
>                   NetClientState *peer, Error **errp);
>  
> -int net_init_bridge(const NetClientOptions *opts, const char *name,
> +int net_init_bridge(const void *opts, const char *name,
>                      NetClientState *peer, Error **errp);
>  
> -int net_init_l2tpv3(const NetClientOptions *opts, const char *name,
> +int net_init_l2tpv3(const void *opts, const char *name,
>                      NetClientState *peer, Error **errp);
>  #ifdef CONFIG_VDE
> -int net_init_vde(const NetClientOptions *opts, const char *name,
> +int net_init_vde(const void *opts, const char *name,
>                   NetClientState *peer, Error **errp);
>  #endif
>  
>  #ifdef CONFIG_NETMAP
> -int net_init_netmap(const NetClientOptions *opts, const char *name,
> +int net_init_netmap(const void *opts, const char *name,
>                      NetClientState *peer, Error **errp);
>  #endif
>  
> -int net_init_vhost_user(const NetClientOptions *opts, const char *name,
> +int net_init_vhost_user(const void *opts, const char *name,
>                          NetClientState *peer, Error **errp);
>  
>  #endif /* QEMU_NET_CLIENTS_H */
> diff --git a/net/dump.c b/net/dump.c
> index 02c8064..6aca19d 100644
> --- a/net/dump.c
> +++ b/net/dump.c
> @@ -94,7 +94,7 @@ static void dump_cleanup(NetClientState *nc)
>  }
>  
>  static NetClientInfo net_dump_info = {
> -    .type = NET_CLIENT_OPTIONS_KIND_DUMP,
> +    .type = NET_CLIENT_DRIVER_DUMP,
>      .size = sizeof(DumpState),
>      .receive = dump_receive,
>      .cleanup = dump_cleanup,
> @@ -146,16 +146,13 @@ static int net_dump_init(NetClientState *peer, const 
> char *device,
>      return 0;
>  }
>  
> -int net_init_dump(const NetClientOptions *opts, const char *name,
> +int net_init_dump(const void *opts, const char *name,
>                    NetClientState *peer, Error **errp)
>  {
>      int len;
>      const char *file;
>      char def_file[128];
> -    const NetdevDumpOptions *dump;
> -
> -    assert(opts->kind == NET_CLIENT_OPTIONS_KIND_DUMP);
> -    dump = opts->dump;
> +    const NetdevDumpOptions *dump = opts;
>  
>      assert(peer);
>  

Before the patch: we pass tagged union NetClientOptions to the tag's
callback, and fetch the tag's union member.

After: we pass the union member directly.  Because all callbacks have to
have the same type, we need one that can hold all the union members.
Since they're all pointers, void * works.

A union of all members would do as well.  The QAPI code generator
actually generates it, but gives it no name.  Fixable.

Further down, I challenge having separate types Netdev and Netlegacy.
If we used the same one for both, we could pass it to the callbacks, I
think.

[More of the same...]
> diff --git a/net/net.c b/net/net.c
> index cc36c7b..5f230a5 100644
> --- a/net/net.c
> +++ b/net/net.c
[...]
> @@ -878,32 +875,32 @@ static int net_init_nic(const NetClientOptions *opts, 
> const char *name,
>  }
>  
>  
> -static int (* const net_client_init_fun[NET_CLIENT_OPTIONS_KIND_MAX])(
> -    const NetClientOptions *opts,
> +static int (* const net_client_init_fun[NET_CLIENT_DRIVER_MAX])(
> +    const void *opts,
>      const char *name,
>      NetClientState *peer, Error **errp) = {
> -        [NET_CLIENT_OPTIONS_KIND_NIC]       = net_init_nic,
> +        [NET_CLIENT_DRIVER_NIC]       = net_init_nic,
>  #ifdef CONFIG_SLIRP
> -        [NET_CLIENT_OPTIONS_KIND_USER]      = net_init_slirp,
> +        [NET_CLIENT_DRIVER_USER]      = net_init_slirp,
>  #endif
> -        [NET_CLIENT_OPTIONS_KIND_TAP]       = net_init_tap,
> -        [NET_CLIENT_OPTIONS_KIND_SOCKET]    = net_init_socket,
> +        [NET_CLIENT_DRIVER_TAP]       = net_init_tap,
> +        [NET_CLIENT_DRIVER_SOCKET]    = net_init_socket,
>  #ifdef CONFIG_VDE
> -        [NET_CLIENT_OPTIONS_KIND_VDE]       = net_init_vde,
> +        [NET_CLIENT_DRIVER_VDE]       = net_init_vde,
>  #endif
>  #ifdef CONFIG_NETMAP
> -        [NET_CLIENT_OPTIONS_KIND_NETMAP]    = net_init_netmap,
> +        [NET_CLIENT_DRIVER_NETMAP]    = net_init_netmap,
>  #endif
> -        [NET_CLIENT_OPTIONS_KIND_DUMP]      = net_init_dump,
> +        [NET_CLIENT_DRIVER_DUMP]      = net_init_dump,
>  #ifdef CONFIG_NET_BRIDGE
> -        [NET_CLIENT_OPTIONS_KIND_BRIDGE]    = net_init_bridge,
> +        [NET_CLIENT_DRIVER_BRIDGE]    = net_init_bridge,
>  #endif
> -        [NET_CLIENT_OPTIONS_KIND_HUBPORT]   = net_init_hubport,
> +        [NET_CLIENT_DRIVER_HUBPORT]   = net_init_hubport,
>  #ifdef CONFIG_VHOST_NET_USED
> -        [NET_CLIENT_OPTIONS_KIND_VHOST_USER] = net_init_vhost_user,
> +        [NET_CLIENT_DRIVER_VHOST_USER] = net_init_vhost_user,
>  #endif
>  #ifdef CONFIG_L2TPV3
> -        [NET_CLIENT_OPTIONS_KIND_L2TPV3]    = net_init_l2tpv3,
> +        [NET_CLIENT_DRIVER_L2TPV3]    = net_init_l2tpv3,
>  #endif
>  };
>  
> @@ -914,35 +911,37 @@ static int net_client_init1(const void *object, int 
> is_netdev, Error **errp)
>          const Netdev    *netdev;
>          const NetLegacy *net;
>      } u;
> -    const NetClientOptions *opts;
> +    NetClientDriver kind;
> +    const void *opts;
>      const char *name;
>  
>      if (is_netdev) {
>          u.netdev = object;
> -        opts = u.netdev->opts;
> +        kind = u.netdev->kind;
> +        opts = u.netdev->data;
>          name = u.netdev->id;
>  
> -        switch (opts->kind) {
> +        switch (kind) {
>  #ifdef CONFIG_SLIRP
> -        case NET_CLIENT_OPTIONS_KIND_USER:
> +        case NET_CLIENT_DRIVER_USER:
>  #endif
> -        case NET_CLIENT_OPTIONS_KIND_TAP:
> -        case NET_CLIENT_OPTIONS_KIND_SOCKET:
> +        case NET_CLIENT_DRIVER_TAP:
> +        case NET_CLIENT_DRIVER_SOCKET:
>  #ifdef CONFIG_VDE
> -        case NET_CLIENT_OPTIONS_KIND_VDE:
> +        case NET_CLIENT_DRIVER_VDE:
>  #endif
>  #ifdef CONFIG_NETMAP
> -        case NET_CLIENT_OPTIONS_KIND_NETMAP:
> +        case NET_CLIENT_DRIVER_NETMAP:
>  #endif
>  #ifdef CONFIG_NET_BRIDGE
> -        case NET_CLIENT_OPTIONS_KIND_BRIDGE:
> +        case NET_CLIENT_DRIVER_BRIDGE:
>  #endif
> -        case NET_CLIENT_OPTIONS_KIND_HUBPORT:
> +        case NET_CLIENT_DRIVER_HUBPORT:
>  #ifdef CONFIG_VHOST_NET_USED
> -        case NET_CLIENT_OPTIONS_KIND_VHOST_USER:
> +        case NET_CLIENT_DRIVER_VHOST_USER:
>  #endif
>  #ifdef CONFIG_L2TPV3
> -        case NET_CLIENT_OPTIONS_KIND_L2TPV3:
> +        case NET_CLIENT_DRIVER_L2TPV3:
>  #endif
>              break;
>  
> @@ -953,8 +952,9 @@ static int net_client_init1(const void *object, int 
> is_netdev, Error **errp)
>          }
>      } else {
>          u.net = object;
> -        opts = u.net->opts;
> -        if (opts->kind == NET_CLIENT_OPTIONS_KIND_HUBPORT) {
> +        kind = u.net->kind;
> +        opts = u.net->data;
> +        if (kind == NET_CLIENT_DRIVER_HUBPORT) {
>              error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "type",
>                         "a net type");
>              return -1;
> @@ -963,22 +963,22 @@ static int net_client_init1(const void *object, int 
> is_netdev, Error **errp)
>          name = u.net->has_id ? u.net->id : u.net->name;
>      }
>  
> -    if (net_client_init_fun[opts->kind]) {
> +    if (net_client_init_fun[kind]) {
>          NetClientState *peer = NULL;
>  
>          /* Do not add to a vlan if it's a -netdev or a nic with a netdev=
>           * parameter. */
>          if (!is_netdev &&
> -            (opts->kind != NET_CLIENT_OPTIONS_KIND_NIC ||
> -             !opts->nic->has_netdev)) {
> +            (kind != NET_CLIENT_DRIVER_NIC ||
> +             !((NetLegacyNicOptions *) opts)->has_netdev)) {
>              peer = net_hub_add_port(u.net->has_vlan ? u.net->vlan : 0, NULL);
>          }
>  
> -        if (net_client_init_fun[opts->kind](opts, name, peer, errp) < 0) {
> +        if (net_client_init_fun[kind](opts, name, peer, errp) < 0) {
>              /* FIXME drop when all init functions store an Error */
>              if (errp && !*errp) {
>                  error_setg(errp, QERR_DEVICE_INIT_FAILED,
> -                           NetClientOptionsKind_lookup[opts->kind]);
> +                           NetClientDriver_lookup[kind]);
>              }
>              return -1;
>          }
[...]
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 7ebf99e..2a3cfe3 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -2475,34 +2475,20 @@
>      '*queues':        'uint32' } }
>  
>  ##
> -# @NetClientOptions
> +# @NetClientDriver
>  #
> -# A discriminated record of network device traits.
> -#
> -# Since 1.2
> -#
> -# 'l2tpv3' - since 2.1
> +# Possible netdev drivers.
>  #
> +# Since 2.4
>  ##
> -{ 'union': 'NetClientOptions',
> -  'data': {
> -    'none':     'NetdevNoneOptions',
> -    'nic':      'NetLegacyNicOptions',
> -    'user':     'NetdevUserOptions',
> -    'tap':      'NetdevTapOptions',
> -    'l2tpv3':   'NetdevL2TPv3Options',
> -    'socket':   'NetdevSocketOptions',
> -    'vde':      'NetdevVdeOptions',
> -    'dump':     'NetdevDumpOptions',
> -    'bridge':   'NetdevBridgeOptions',
> -    'hubport':  'NetdevHubPortOptions',
> -    'netmap':   'NetdevNetmapOptions',
> -    'vhost-user': 'NetdevVhostUserOptions' } }
> +{ 'enum': 'NetClientDriver',
> +  'data': [ 'none', 'nic', 'user', 'tap', 'l2tpv3', 'socket', 'vde', 'dump',
> +            'bridge', 'hubport', 'netmap', 'vhost-user' ] }

Diff is confusing around here.  You could add your new stuff after the
changed stuff, where it won't upset diff, and move it to where you want
it in a separate patch.

If we could name the enum NetClientOptionsKind first, then rename it,
this patch would split into an interesting and a mechanical part.
Unfortunately, the qapi code generators don't let us.  Meh.

We could turn that error into a warning, though.

Anyway, this hunk results in:

   ##
   # @NetClientDriver
   #
   # Possible netdev drivers.
   #
   # Since 2.4
   ##
   { 'enum': 'NetClientDriver',
     'data': [ 'none', 'nic', 'user', 'tap', 'l2tpv3', 'socket', 'vde', 'dump',
               'bridge', 'hubport', 'netmap', 'vhost-user' ] }

I'd prefer "Available netdev drivers."

>  
>  ##
> -# @NetLegacy
> +# @NetLegacyBase
>  #
> -# Captures the configuration of a network device; legacy.
> +# Captures the common configuration of a network device; legacy.
>  #
>  # @vlan: #optional vlan number
>  #
> @@ -2512,30 +2498,87 @@
>  #
>  # @opts: device type specific properties (legacy)
>  #
> -# Since 1.2
> +# @type: the netdev driver to use
> +#
> +# Since 2.4
>  ##
> -{ 'struct': 'NetLegacy',
> +{ 'struct': 'NetLegacyBase',
>    'data': {
>      '*vlan': 'int32',
>      '*id':   'str',
>      '*name': 'str',
> -    'opts':  'NetClientOptions' } }
> +    'type': 'NetClientDriver' } }

Results in:

   ##
   # @NetLegacyBase
   #
   # Captures the common configuration of a network device; legacy.
   #
   # @vlan: #optional vlan number
   #
   # @id: #optional identifier for monitor commands
   #
   # @name: #optional identifier for monitor commands, ignored if @id is present
   #
   # @opts: device type specific properties (legacy)
   #
   # @type: the netdev driver to use
   #
   # Since 2.4
   ##
   { 'struct': 'NetLegacy',
     'data': {
       '*vlan': 'int32',
       '*id':   'str',
       '*name': 'str',
       'type': 'NetClientDriver' } }

Documents nonexistent member @opts, oops.

>  
>  ##
> -# @Netdev
> +# @NetLegacy
>  #
> -# Captures the configuration of a network device.
> +# Captures the configuration of a network device; legacy.
> +#
> +# Since 1.2
> +#
> +# 'l2tpv3' - since 2.1
> +##
> +{ 'union': 'NetLegacy',
> +  'base': 'NetLegacyBase',
> +  'discriminator': 'type',
> +  'data': {
> +    'none':     'NetdevNoneOptions',
> +    'nic':      'NetLegacyNicOptions',
> +    'user':     'NetdevUserOptions',
> +    'tap':      'NetdevTapOptions',
> +    'l2tpv3':   'NetdevL2TPv3Options',
> +    'socket':   'NetdevSocketOptions',
> +    'vde':      'NetdevVdeOptions',
> +    'dump':     'NetdevDumpOptions',
> +    'bridge':   'NetdevBridgeOptions',
> +    'hubport':  'NetdevHubPortOptions',
> +    'netmap':   'NetdevNetmapOptions',
> +    'vhost-user': 'NetdevVhostUserOptions' } }

Results in:

   ##
   # @NetLegacy
   #
   # Captures the configuration of a network device; legacy.
   #
   # Since 1.2
   #
   # 'l2tpv3' - since 2.1
   ##
   { 'union': 'NetLegacy',
     'base': 'NetLegacyBase',
     'discriminator': 'type',
     'data': {
       'none':     'NetdevNoneOptions',
       'nic':      'NetLegacyNicOptions',
       'user':     'NetdevUserOptions',
       'tap':      'NetdevTapOptions',
       'l2tpv3':   'NetdevL2TPv3Options',
       'socket':   'NetdevSocketOptions',
       'vde':      'NetdevVdeOptions',
       'dump':     'NetdevDumpOptions',
       'bridge':   'NetdevBridgeOptions',
       'hubport':  'NetdevHubPortOptions',
       'netmap':   'NetdevNetmapOptions',
       'vhost-user': 'NetdevVhostUserOptions' } }

Changed from:

   ##
   # @NetLegacy
   #
   # Captures the configuration of a network device; legacy.
   #
   # @vlan: #optional vlan number
   #
   # @id: #optional identifier for monitor commands
   #
   # @name: #optional identifier for monitor commands, ignored if @id is present
   #
   # @opts: device type specific properties (legacy)
   #
   # Since 1.2
   ##
   { 'struct': 'NetLegacy',
     'data': {
       '*vlan': 'int32',
       '*id':   'str',
       '*name': 'str',
       'opts':  'NetClientOptions' } }

where NetClientOptions is

   ##
   # @NetClientOptions
   #
   # A discriminated record of network device traits.
   #
   # Since 1.2
   #
   # 'l2tpv3' - since 2.1
   #
   ##
   { 'union': 'NetClientOptions',
     'data': {
       'none':     'NetdevNoneOptions',
       'nic':      'NetLegacyNicOptions',
       'user':     'NetdevUserOptions',
       'tap':      'NetdevTapOptions',
       'l2tpv3':   'NetdevL2TPv3Options',
       'socket':   'NetdevSocketOptions',
       'vde':      'NetdevVdeOptions',
       'dump':     'NetdevDumpOptions',
       'bridge':   'NetdevBridgeOptions',
       'hubport':  'NetdevHubPortOptions',
       'netmap':   'NetdevNetmapOptions',
       'vhost-user': 'NetdevVhostUserOptions' } }

Faithful conversion.

Since the changed types aren't used by any command or event (yet), we
don't have to worry about backward compatibility.

> +
> +
> +##
> +# @NetdevBase
> +#
> +# Captures the common configuration of a network device.
>  #
>  # @id: identifier for monitor commands.
>  #
>  # @opts: device type specific properties

@opts doesn't exist.

>  #
> -# Since 1.2
> +# @type: the netdev driver to use
> +#
> +# Since 2.4
>  ##
> -{ 'struct': 'Netdev',
> +{ 'struct': 'NetdevBase',
>    'data': {
>      'id':   'str',
> -    'opts': 'NetClientOptions' } }
> +    'type': 'NetClientDriver' } }
> +
> +##
> +# @Netdev
> +#
> +# Captures the configuration of a network device.
> +#
> +# Since 1.2
> +#
> +# 'l2tpv3' - since 2.1
> +##
> +{ 'union': 'Netdev',
> +  'base': 'NetdevBase',
> +  'discriminator': 'type',
> +  'data': {
> +    'none':     'NetdevNoneOptions',
> +    'nic':      'NetLegacyNicOptions',
> +    'user':     'NetdevUserOptions',
> +    'tap':      'NetdevTapOptions',
> +    'l2tpv3':   'NetdevL2TPv3Options',
> +    'socket':   'NetdevSocketOptions',
> +    'vde':      'NetdevVdeOptions',
> +    'dump':     'NetdevDumpOptions',
> +    'bridge':   'NetdevBridgeOptions',
> +    'hubport':  'NetdevHubPortOptions',
> +    'netmap':   'NetdevNetmapOptions',
> +    'vhost-user': 'NetdevVhostUserOptions' } }
>  
>  ##
>  # @InetSocketAddress

Again, faithful conversion.

Netdev is identical to NetLegacy, except base is NetdevBase instead of
NetLegacyBase.

NetLegacyBase is NetdevBase plus optional members vlan and name.

Therefore, NetLegacy is Netdev plus optional members vlan and name.

Making the two separate types enables some compiler type checking.  Not
sure it's worth the trouble, but that's not your fault, you're only
converting stuff.

Flat unions carry a bit of notational overhead, but I hope to ease that
later on.

Overall, just a few nits, and a type system cheat I'd prefer to avoid.