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

[Thomas Huth]

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 <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)?

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