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

[Eric Blake]

On 12/5/19 11:36 PM, 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 <address@hidden>
> ---

Initial focus on the QAPI change:

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

Making id optional here...

>     'discriminator': 'type',
>     'data': {
>       'nic':      'NetLegacyNicOptions',
> @@ -481,55 +481,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
> -#
> -# 'vlan': dropped in 3.0
> -# 'name': dropped in 5.0
> -##
> -{ 'struct': 'NetLegacy',
> -  'data': {
> -    '*id':   'str',
> -    'opts':  'NetLegacyOptions' } }

to match how it was here.  Should 'id' have been made mandatory in 1/2, 
when deleting 'name' (after all, id was optional only when name was in use)?

> -
> -##
> -# @NetLegacyOptionsType:
> -#
> -# Since: 1.2
> -##
> -{ 'enum': 'NetLegacyOptionsType',
> -  'data': ['none', 'nic', 'user', 'tap', 'l2tpv3', 'socket', 'vde',
> -           'bridge', 'netmap', 'vhost-user'] }

Comparing this to the branches of Netdev:

We are losing 'none', while gaining 'hubport'.  The gain is not 
problematic, and I guess you are declaring that the use of 'none' has 
been deprecated long enough to not be a problem.

> -
> -##
> -# @NetLegacyOptions:
> -#
> -# Like Netdev, but for use only by the legacy command line options
> -#
> -# Since: 1.2
> -##
> -{ 'union': 'NetLegacyOptions',
> -  'base': { 'type': 'NetLegacyOptionsType' },
> -  'discriminator': 'type',
> -  'data': {
> -    'nic':      'NetLegacyNicOptions',

Should we rename this to NetdevNicOptions, now that we are getting rid 
of other NetLegacy names?

> -    'user':     'NetdevUserOptions',
> -    'tap':      'NetdevTapOptions',
> -    'l2tpv3':   'NetdevL2TPv3Options',
> -    'socket':   'NetdevSocketOptions',
> -    'vde':      'NetdevVdeOptions',
> -    'bridge':   'NetdevBridgeOptions',
> -    'netmap':   'NetdevNetmapOptions',
> -    'vhost-user': 'NetdevVhostUserOptions' } }

But I concur that all branches of the Netdev union have the same types 
as what you are removing here from NetLegacyOptions, so the 
consolidation looks sane.

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