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