Re: [libvirt] [PATCH 2/2] net: Drop the NetLegacy structure, always use Netdev instead
On 06/12/2019 16.14, Eric Blake wrote:
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 <thuth(a)redhat.com>
> ---
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)?
No, since "id" is still not mandatory for "-net". In case it is
missing,
the code creates an id internally (see assign_name() in net/net.c).
> -
> -##
> -# @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.
'none' still continues to work, it's also a member of NetClientDriver
and was handled later in the patch:
+ if (netdev->type == NET_CLIENT_DRIVER_NONE) {
return 0; /* nothing to do */
'hubport' is blocked in the code now instead:
+ 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)");
> -
> -##
> -# @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?
I still consider "-net nic" as a legacy option that we should remove one
day in the future, so I'd rather keep that name.
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.
Ok, thanks for your review!
Thomas