[2/2] net: Drop the NetLegacy structure, always use Netdev instead
diff mbox series

Message ID 20191206053640.29368-3-thuth@redhat.com
State New
Headers show
Series
  • net: Drop legacy "name" from -net and remove NetLegacy
Related show

Commit Message

Thomas Huth Dec. 6, 2019, 5:36 a.m. UTC
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>
---
 net/net.c     | 74 ++++++++-------------------------------------------
 qapi/net.json | 51 +----------------------------------
 2 files changed, 12 insertions(+), 113 deletions(-)

Comments

Eric Blake Dec. 6, 2019, 3:14 p.m. UTC | #1
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)?

> -
> -##
> -# @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.
Thomas Huth Dec. 9, 2019, 11:33 a.m. UTC | #2
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
Markus Armbruster Dec. 10, 2019, 11:09 a.m. UTC | #3
Thomas Huth <thuth@redhat.com> writes:

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

Took me a minute to see the actual difference.

Here's Netdev:

    { 'union': 'Netdev',
      'base': { 'id': 'str', 'type': 'NetClientDriver' },
      'discriminator': 'type',
      'data': {
        'nic':      'NetLegacyNicOptions',
        'user':     'NetdevUserOptions',
        'tap':      'NetdevTapOptions',
        'l2tpv3':   'NetdevL2TPv3Options',
        'socket':   'NetdevSocketOptions',
        'vde':      'NetdevVdeOptions',
        'bridge':   'NetdevBridgeOptions',
        'hubport':  'NetdevHubPortOptions',
        'netmap':   'NetdevNetmapOptions',
        'vhost-user': 'NetdevVhostUserOptions' } }

    { 'enum': 'NetClientDriver',
      'data': [ 'none', 'nic', 'user', 'tap', 'l2tpv3', 'socket', 'vde',
                'bridge', 'hubport', 'netmap', 'vhost-user' ] }

Here's NetLegacy:

    { 'struct': 'NetLegacy',
      'data': {
        '*id':   'str',
        'opts':  'NetLegacyOptions' } }

    { 'union': 'NetLegacyOptions',
      'base': { 'type': 'NetLegacyOptionsType' },
      'discriminator': 'type',
      'data': {
        'nic':      'NetLegacyNicOptions',
        'user':     'NetdevUserOptions',
        'tap':      'NetdevTapOptions',
        'l2tpv3':   'NetdevL2TPv3Options',
        'socket':   'NetdevSocketOptions',
        'vde':      'NetdevVdeOptions',
        'bridge':   'NetdevBridgeOptions',
        'netmap':   'NetdevNetmapOptions',
        'vhost-user': 'NetdevVhostUserOptions' } }

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

Difference: NetLegacy wraps the union in @opts.

Patch
diff mbox series

diff --git a/net/net.c b/net/net.c
index ee4a76eb3e..cfe524b4d1 100644
--- a/net/net.c
+++ b/net/net.c
@@ -967,13 +967,14 @@  static int (* const net_client_init_fun[NET_CLIENT_DRIVER__MAX])(
 
 static int net_client_init1(const void *object, bool is_netdev, Error **errp)
 {
-    Netdev legacy = {0};
-    const Netdev *netdev;
+    const Netdev *netdev = object;
     NetClientState *peer = NULL;
 
     if (is_netdev) {
-        netdev = object;
-
+        if (!netdev->has_id) {
+            error_setg(errp, QERR_MISSING_PARAMETER, "id");
+            return -1;
+        }
         if (netdev->type == NET_CLIENT_DRIVER_NIC ||
             !net_client_init_fun[netdev->type]) {
             error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "type",
@@ -981,56 +982,11 @@  static int net_client_init1(const void *object, bool is_netdev, Error **errp)
             return -1;
         }
     } else {
-        const NetLegacy *net = object;
-        const NetLegacyOptions *opts = net->opts;
-        legacy.id = net->id;
-        netdev = &legacy;
-
-        /* Map the old options to the new flat type */
-        switch (opts->type) {
-        case NET_LEGACY_OPTIONS_TYPE_NONE:
+        if (netdev->type == NET_CLIENT_DRIVER_NONE) {
             return 0; /* nothing to do */
-        case NET_LEGACY_OPTIONS_TYPE_NIC:
-            legacy.type = NET_CLIENT_DRIVER_NIC;
-            legacy.u.nic = opts->u.nic;
-            break;
-        case NET_LEGACY_OPTIONS_TYPE_USER:
-            legacy.type = NET_CLIENT_DRIVER_USER;
-            legacy.u.user = opts->u.user;
-            break;
-        case NET_LEGACY_OPTIONS_TYPE_TAP:
-            legacy.type = NET_CLIENT_DRIVER_TAP;
-            legacy.u.tap = opts->u.tap;
-            break;
-        case NET_LEGACY_OPTIONS_TYPE_L2TPV3:
-            legacy.type = NET_CLIENT_DRIVER_L2TPV3;
-            legacy.u.l2tpv3 = opts->u.l2tpv3;
-            break;
-        case NET_LEGACY_OPTIONS_TYPE_SOCKET:
-            legacy.type = NET_CLIENT_DRIVER_SOCKET;
-            legacy.u.socket = opts->u.socket;
-            break;
-        case NET_LEGACY_OPTIONS_TYPE_VDE:
-            legacy.type = NET_CLIENT_DRIVER_VDE;
-            legacy.u.vde = opts->u.vde;
-            break;
-        case NET_LEGACY_OPTIONS_TYPE_BRIDGE:
-            legacy.type = NET_CLIENT_DRIVER_BRIDGE;
-            legacy.u.bridge = opts->u.bridge;
-            break;
-        case NET_LEGACY_OPTIONS_TYPE_NETMAP:
-            legacy.type = NET_CLIENT_DRIVER_NETMAP;
-            legacy.u.netmap = opts->u.netmap;
-            break;
-        case NET_LEGACY_OPTIONS_TYPE_VHOST_USER:
-            legacy.type = NET_CLIENT_DRIVER_VHOST_USER;
-            legacy.u.vhost_user = opts->u.vhost_user;
-            break;
-        default:
-            abort();
         }
-
-        if (!net_client_init_fun[netdev->type]) {
+        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)");
@@ -1039,7 +995,7 @@  static int net_client_init1(const void *object, bool is_netdev, Error **errp)
 
         /* Do not add to a hub if it's a nic with a netdev= parameter. */
         if (netdev->type != NET_CLIENT_DRIVER_NIC ||
-            !opts->u.nic.has_netdev) {
+            !netdev->u.nic.has_netdev) {
             peer = net_hub_add_port(0, NULL, NULL);
         }
     }
@@ -1137,21 +1093,13 @@  static int net_client_init(QemuOpts *opts, bool is_netdev, Error **errp)
         }
     }
 
-    if (is_netdev) {
-        visit_type_Netdev(v, NULL, (Netdev **)&object, &err);
-    } else {
-        visit_type_NetLegacy(v, NULL, (NetLegacy **)&object, &err);
-    }
+    visit_type_Netdev(v, NULL, (Netdev **)&object, &err);
 
     if (!err) {
         ret = net_client_init1(object, is_netdev, &err);
     }
 
-    if (is_netdev) {
-        qapi_free_Netdev(object);
-    } else {
-        qapi_free_NetLegacy(object);
-    }
+    qapi_free_Netdev(object);
 
 out:
     error_propagate(errp, err);
diff --git a/qapi/net.json b/qapi/net.json
index ff280ccd16..fcf693924e 100644
--- a/qapi/net.json
+++ b/qapi/net.json
@@ -467,7 +467,7 @@ 
 # 'l2tpv3' - since 2.1
 ##
 { 'union': 'Netdev',
-  'base': { 'id': 'str', 'type': 'NetClientDriver' },
+  'base': { '*id': 'str', 'type': 'NetClientDriver' },
   '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' } }
-
-##
-# @NetLegacyOptionsType:
-#
-# Since: 1.2
-##
-{ 'enum': 'NetLegacyOptionsType',
-  'data': ['none', 'nic', 'user', 'tap', 'l2tpv3', 'socket', 'vde',
-           'bridge', 'netmap', 'vhost-user'] }
-
-##
-# @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',
-    'user':     'NetdevUserOptions',
-    'tap':      'NetdevTapOptions',
-    'l2tpv3':   'NetdevL2TPv3Options',
-    'socket':   'NetdevSocketOptions',
-    'vde':      'NetdevVdeOptions',
-    'bridge':   'NetdevBridgeOptions',
-    'netmap':   'NetdevNetmapOptions',
-    'vhost-user': 'NetdevVhostUserOptions' } }
-
 ##
 # @NetFilterDirection:
 #