Re: [PATCH v2 2/2] net: Drop the NetLegacy structure, always use Netdev instead

[Eric Blake <eblake@redhat.com>]

On 5/12/20 7:31 AM, Thomas Huth wrote:
> Now that the "name" parameter is gone, there is hardly any difference
> between NetLegacy and Netdev anymore. Drop NetLegacy and always use
> Netdev to simplify the code quite a bit.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---

> +++ b/net/net.c
> @@ -967,13 +967,14 @@ static int (* const net_client_init_fun[NET_CLIENT_DRIVER__MAX])(
>   
>   static int net_client_init1(const void *object, bool is_netdev, Error **errp)

Why do we still need the 'is_netdev' parameter?  If all callers are 
passing in a netdev, then either this parameter needs to be renamed to 
capture the actual difference between callers, or it can be dropped 
altogether.

>   {
> -    Netdev legacy = {0};
> -    const Netdev *netdev;
> +    const Netdev *netdev = object;
>       NetClientState *peer = NULL;
>   
>       if (is_netdev) {
> -        netdev = object;
> -
> +        if (!netdev->has_id) {
> +            error_setg(errp, QERR_MISSING_PARAMETER, "id");
> +            return -1;
> +        }

You wouldn't need this if 'id' remained mandatory.

> @@ -981,56 +982,11 @@ static int net_client_init1(const void *object, bool is_netdev, Error **errp)
>               return -1;
>           }
>       } else {
> -        const NetLegacy *net = object;
> -        const NetLegacyOptions *opts = net->opts;
> -        legacy.id = net->id;
> -        netdev = &legacy;
> -
> -        /* Map the old options to the new flat type */
> -        switch (opts->type) {
> -        case NET_LEGACY_OPTIONS_TYPE_NONE:
> +        if (netdev->type == NET_CLIENT_DRIVER_NONE) {
>               return 0; /* nothing to do */


> -
> -        if (!net_client_init_fun[netdev->type]) {
> +        if (netdev->type == NET_CLIENT_DRIVER_HUBPORT ||
> +            !net_client_init_fun[netdev->type]) {
>               error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "type",
>                          "a net backend type (maybe it is not compiled "
>                          "into this binary)");

So maybe we still want this legacy-handling code, but renaming 
'is_netdev' to 'legacy_handling' may make more sense.

> @@ -1039,7 +995,7 @@ static int net_client_init1(const void *object, bool is_netdev, Error **errp)
>   
>           /* Do not add to a hub if it's a nic with a netdev= parameter. */
>           if (netdev->type != NET_CLIENT_DRIVER_NIC ||
> -            !opts->u.nic.has_netdev) {
> +            !netdev->u.nic.has_netdev) {
>               peer = net_hub_add_port(0, NULL, NULL);
>           }
>       }
> @@ -1143,21 +1099,13 @@ static int net_client_init(QemuOpts *opts, bool is_netdev, Error **errp)
>           }
>       }
>   
> -    if (is_netdev) {
> -        visit_type_Netdev(v, NULL, (Netdev **)&object, &err);
> -    } else {
> -        visit_type_NetLegacy(v, NULL, (NetLegacy **)&object, &err);
> -    }
> +    visit_type_Netdev(v, NULL, (Netdev **)&object, &err);

Why do we need to cast?  If all callers are passing a Netdev, can't we 
just give 'object' the correct type to begin with?


> +++ b/qapi/net.json
> @@ -453,7 +453,7 @@
>   #        'l2tpv3' - since 2.1
>   ##
>   { 'union': 'Netdev',
> -  'base': { 'id': 'str', 'type': 'NetClientDriver' },
> +  'base': { '*id': 'str', 'type': 'NetClientDriver' },

I don't think we need to make 'id' optional.

>     'discriminator': 'type',
>     'data': {
>       'nic':      'NetLegacyNicOptions',
> @@ -467,52 +467,6 @@
>       'netmap':   'NetdevNetmapOptions',
>       'vhost-user': 'NetdevVhostUserOptions' } }
>   
> -##
> -# @NetLegacy:
> -#
> -# Captures the configuration of a network device; legacy.
> -#
> -# @id: identifier for monitor commands
> -#
> -# @opts: device type specific properties (legacy)
> -#
> -# Since: 1.2
> -##
> -{ 'struct': 'NetLegacy',
> -  'data': {
> -    '*id':   'str',
> -    'opts':  'NetLegacyOptions' } }
> -
> -##
> -# @NetLegacyOptionsType:
> -#
> -# Since: 1.2
> -##
> -{ 'enum': 'NetLegacyOptionsType',
> -  'data': ['none', 'nic', 'user', 'tap', 'l2tpv3', 'socket', 'vde',
> -           'bridge', 'netmap', 'vhost-user'] }

NetLegacyOptionsType differs from NetClientDriver only by missing 
'hubport', which your code above special-cased, so on that front, the 
simplification makes sense.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org