[PATCH v3 0/2] net: Drop legacy "name" from -net and remove NetLegacy

[PATCH v3 0/2] net: Drop legacy "name" from -net and remove NetLegacy

[Thomas Huth]

Since commit b4983c570c7a ("net: Remove deprecated [hub_id name] tuple of
'hostfwd_add' / 'hostfwd_remove'"), the "name" parameter is not used
internally anymore. And it's been marked as deprecated since QEMU v3.1,
so it is time to remove the "name" parameter from -net now. Once this
has been done, we can also drop the obsolete NetLegacy structure since
there is no major difference between Netdev and NetLegacy anymore.

v3:
 - Do not make "id" in Netdev optional, but rather assign a temporary
   "id" for -net before we call the options visitor function.
 - Changed some "void *" to "Netdev *" now

v2:
 - Rebased to master (use the deprecated.rst instead of qemu-deprecated.texi)

Thomas Huth (2):
  net: Drop the legacy "name" parameter from the -net option
  net: Drop the NetLegacy structure, always use Netdev instead

 docs/system/deprecated.rst | 15 ++++---
 net/net.c                  | 87 ++++++--------------------------------
 qapi/net.json              | 49 ---------------------
 3 files changed, 23 insertions(+), 128 deletions(-)

-- 
2.18.1

[PATCH v3 1/2] net: Drop the legacy "name" parameter from the -net option

[Thomas Huth]

It's been deprecated since QEMU v3.1, so it's time to finally
remove it. The "id" parameter can simply be used instead.

Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 docs/system/deprecated.rst | 15 +++++++++------
 net/net.c                  | 10 +---------
 qapi/net.json              |  3 ---
 3 files changed, 10 insertions(+), 18 deletions(-)

diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst
index 3142fac386..1b87c8f0db 100644
--- a/docs/system/deprecated.rst
+++ b/docs/system/deprecated.rst
@@ -47,12 +47,6 @@ The 'file' driver for drives is no longer appropriate for character or host
 devices and will only accept regular files (S_IFREG). The correct driver
 for these file types is 'host_cdrom' or 'host_device' as appropriate.
 
-``-net ...,name=``\ *name* (since 3.1)
-''''''''''''''''''''''''''''''''''''''
-
-The ``name`` parameter of the ``-net`` option is a synonym
-for the ``id`` parameter, which should now be used instead.
-
 ``-smp`` (invalid topologies) (since 3.1)
 '''''''''''''''''''''''''''''''''''''''''
 
@@ -470,6 +464,15 @@ What follows is a record of recently removed, formerly deprecated
 features that serves as a record for users who have encountered
 trouble after a recent upgrade.
 
+System emulator command line arguments
+--------------------------------------
+
+``-net ...,name=``\ *name* (removed in 5.1)
+'''''''''''''''''''''''''''''''''''''''''''
+
+The ``name`` parameter of the ``-net`` option was a synonym
+for the ``id`` parameter, which should now be used instead.
+
 QEMU Machine Protocol (QMP) commands
 ------------------------------------
 
diff --git a/net/net.c b/net/net.c
index 38778e831d..a24da66e66 100644
--- a/net/net.c
+++ b/net/net.c
@@ -969,12 +969,10 @@ static int net_client_init1(const void *object, bool is_netdev, Error **errp)
 {
     Netdev legacy = {0};
     const Netdev *netdev;
-    const char *name;
     NetClientState *peer = NULL;
 
     if (is_netdev) {
         netdev = object;
-        name = netdev->id;
 
         if (netdev->type == NET_CLIENT_DRIVER_NIC ||
             !net_client_init_fun[netdev->type]) {
@@ -987,12 +985,6 @@ static int net_client_init1(const void *object, bool is_netdev, Error **errp)
         const NetLegacyOptions *opts = net->opts;
         legacy.id = net->id;
         netdev = &legacy;
-        /* missing optional values have been initialized to "all bits zero" */
-        name = net->has_id ? net->id : net->name;
-
-        if (net->has_name) {
-            warn_report("The 'name' parameter is deprecated, use 'id' instead");
-        }
 
         /* Map the old options to the new flat type */
         switch (opts->type) {
@@ -1052,7 +1044,7 @@ static int net_client_init1(const void *object, bool is_netdev, Error **errp)
         }
     }
 
-    if (net_client_init_fun[netdev->type](netdev, name, peer, errp) < 0) {
+    if (net_client_init_fun[netdev->type](netdev, netdev->id, peer, errp) < 0) {
         /* FIXME drop when all init functions store an Error */
         if (errp && !*errp) {
             error_setg(errp, QERR_DEVICE_INIT_FAILED,
diff --git a/qapi/net.json b/qapi/net.json
index cebb1b52e3..fc7c95f6d8 100644
--- a/qapi/net.json
+++ b/qapi/net.json
@@ -474,8 +474,6 @@
 #
 # @id: identifier for monitor commands
 #
-# @name: identifier for monitor commands, ignored if @id is present
-#
 # @opts: device type specific properties (legacy)
 #
 # Since: 1.2
@@ -483,7 +481,6 @@
 { 'struct': 'NetLegacy',
   'data': {
     '*id':   'str',
-    '*name': 'str',
     'opts':  'NetLegacyOptions' } }
 
 ##
-- 
2.18.1

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

[Thomas Huth]

Now that the "name" parameter is gone, there is hardly any difference
between NetLegacy and Netdev anymore, so we can drop NetLegacy and always
use Netdev to simplify the code quite a bit.

The only two differences that were really left between Netdev and NetLegacy:

1) NetLegacy does not allow a "hubport" type. We can continue to block
   this with a simple check in net_client_init1() for this type.

2) The "id" parameter was optional in NetLegacy (and an internal id
   was chosen via assign_name() during initialization), but it is mandatory
   for Netdev. To avoid that the visitor code bails out here, we have to
   add an internal id to the QemuOpts already earlier now.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 Note: I did not rename the "is_netdev" parameter of the function (as
 Eric suggested) - you really have to think of "-netdev" vs. "-net"
 here and not about "Netdev" vs. "NetLegacy". But if this "is_netdev"
 thing still confuses us in the future, we can still rename it with an
 additional follow-up patch later instead.

 net/net.c     | 77 +++++++++------------------------------------------
 qapi/net.json | 46 ------------------------------
 2 files changed, 13 insertions(+), 110 deletions(-)

diff --git a/net/net.c b/net/net.c
index a24da66e66..d4cc5bd73e 100644
--- a/net/net.c
+++ b/net/net.c
@@ -965,15 +965,11 @@ static int (* const net_client_init_fun[NET_CLIENT_DRIVER__MAX])(
 };
 
 
-static int net_client_init1(const void *object, bool is_netdev, Error **errp)
+static int net_client_init1(const Netdev *netdev, bool is_netdev, Error **errp)
 {
-    Netdev legacy = {0};
-    const Netdev *netdev;
     NetClientState *peer = NULL;
 
     if (is_netdev) {
-        netdev = object;
-
         if (netdev->type == NET_CLIENT_DRIVER_NIC ||
             !net_client_init_fun[netdev->type]) {
             error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "type",
@@ -981,56 +977,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 +990,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);
         }
     }
@@ -1100,7 +1051,7 @@ static void show_netdevs(void)
 static int net_client_init(QemuOpts *opts, bool is_netdev, Error **errp)
 {
     gchar **substrings = NULL;
-    void *object = NULL;
+    Netdev *object = NULL;
     Error *err = NULL;
     int ret = -1;
     Visitor *v = opts_visitor_new(opts);
@@ -1143,21 +1094,19 @@ 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);
+    /* Create an ID for -net if the user did not specify one */
+    if (!is_netdev && !qemu_opts_id(opts)) {
+        static int idx;
+        qemu_opts_set_id(opts, g_strdup_printf("__org.qemu.net%i", idx++));
     }
 
+    visit_type_Netdev(v, NULL, &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 fc7c95f6d8..9244c9af56 100644
--- a/qapi/net.json
+++ b/qapi/net.json
@@ -467,52 +467,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
-##
-{ '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:
 #
-- 
2.18.1

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

[Eric Blake]

On 5/18/20 1:01 PM, Thomas Huth wrote:
> Now that the "name" parameter is gone, there is hardly any difference
> between NetLegacy and Netdev anymore, so we can drop NetLegacy and always
> use Netdev to simplify the code quite a bit.
> 
> The only two differences that were really left between Netdev and NetLegacy:
> 
> 1) NetLegacy does not allow a "hubport" type. We can continue to block
>     this with a simple check in net_client_init1() for this type.
> 
> 2) The "id" parameter was optional in NetLegacy (and an internal id
>     was chosen via assign_name() during initialization), but it is mandatory
>     for Netdev. To avoid that the visitor code bails out here, we have to
>     add an internal id to the QemuOpts already earlier now.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>   Note: I did not rename the "is_netdev" parameter of the function (as
>   Eric suggested) - you really have to think of "-netdev" vs. "-net"
>   here and not about "Netdev" vs. "NetLegacy". But if this "is_netdev"
>   thing still confuses us in the future, we can still rename it with an
>   additional follow-up patch later instead.

Works for me. It still might be nice mentioning "-netdev" vs. "-net" in 
the commit message (and the fact that "-net" was what was previously 
using the legacy type).  But with the explanations you've given, the 
code looks correct, and a commit message tweak does not change:

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH v3 0/2] net: Drop legacy "name" from -net and remove NetLegacy

[Thomas Huth]

On 18/05/2020 20.01, Thomas Huth wrote:
> Since commit b4983c570c7a ("net: Remove deprecated [hub_id name] tuple of
> 'hostfwd_add' / 'hostfwd_remove'"), the "name" parameter is not used
> internally anymore. And it's been marked as deprecated since QEMU v3.1,
> so it is time to remove the "name" parameter from -net now. Once this
> has been done, we can also drop the obsolete NetLegacy structure since
> there is no major difference between Netdev and NetLegacy anymore.
> 
> v3:
>  - Do not make "id" in Netdev optional, but rather assign a temporary
>    "id" for -net before we call the options visitor function.
>  - Changed some "void *" to "Netdev *" now
> 
> v2:
>  - Rebased to master (use the deprecated.rst instead of qemu-deprecated.texi)
> 
> Thomas Huth (2):
>   net: Drop the legacy "name" parameter from the -net option
>   net: Drop the NetLegacy structure, always use Netdev instead

Ping!

Jason, do you think these patches are OK now, and if so, could you
please queue them for your next net pull request?

 Thanks,
  Thomas

Re: [PATCH v3 0/2] net: Drop legacy "name" from -net and remove NetLegacy

[Jason Wang]

On 2020/6/15 下午8:06, Thomas Huth wrote:
> On 18/05/2020 20.01, Thomas Huth wrote:
>> Since commit b4983c570c7a ("net: Remove deprecated [hub_id name] tuple of
>> 'hostfwd_add' / 'hostfwd_remove'"), the "name" parameter is not used
>> internally anymore. And it's been marked as deprecated since QEMU v3.1,
>> so it is time to remove the "name" parameter from -net now. Once this
>> has been done, we can also drop the obsolete NetLegacy structure since
>> there is no major difference between Netdev and NetLegacy anymore.
>>
>> v3:
>>   - Do not make "id" in Netdev optional, but rather assign a temporary
>>     "id" for -net before we call the options visitor function.
>>   - Changed some "void *" to "Netdev *" now
>>
>> v2:
>>   - Rebased to master (use the deprecated.rst instead of qemu-deprecated.texi)
>>
>> Thomas Huth (2):
>>    net: Drop the legacy "name" parameter from the -net option
>>    net: Drop the NetLegacy structure, always use Netdev instead
> Ping!
>
> Jason, do you think these patches are OK now, and if so, could you
> please queue them for your next net pull request?


Queued.

Sorry for the late.

Thanks


>
>   Thanks,
>    Thomas
>
>