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