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

[PATCH v2 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.

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                  | 84 ++++++--------------------------------
 qapi/net.json              | 51 +----------------------
 3 files changed, 22 insertions(+), 128 deletions(-)

-- 
2.18.1

[PATCH v2 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 v2 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. 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 | 48 +--------------------------------
 2 files changed, 12 insertions(+), 110 deletions(-)

diff --git a/net/net.c b/net/net.c
index a24da66e66..4177224939 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);
         }
     }
@@ -1143,21 +1099,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 fc7c95f6d8..e344825c43 100644
--- a/qapi/net.json
+++ b/qapi/net.json
@@ -453,7 +453,7 @@
 #        'l2tpv3' - since 2.1
 ##
 { 'union': 'Netdev',
-  'base': { 'id': 'str', 'type': 'NetClientDriver' },
+  'base': { '*id': 'str', 'type': 'NetClientDriver' },
   'discriminator': 'type',
   'data': {
     'nic':      'NetLegacyNicOptions',
@@ -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 v2 1/2] net: Drop the legacy "name" parameter from the -net option

[Eric Blake]

On 5/12/20 7:31 AM, Thomas Huth wrote:
> 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(-)
> 

> +++ 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' } }

Why is 'id' left optional? I'd expect it to be mandatory, now.

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

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

[Eric Blake]

On 5/12/20 7:31 AM, 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>
> ---

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

Why do we still need the 'is_netdev' parameter?  If all callers are 
passing in a netdev, then either this parameter needs to be renamed to 
capture the actual difference between callers, or it can be dropped 
altogether.

>   {
> -    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;
> +        }

You wouldn't need this if 'id' remained mandatory.

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


> -
> -        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)");

So maybe we still want this legacy-handling code, but renaming 
'is_netdev' to 'legacy_handling' may make more sense.

> @@ -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);
>           }
>       }
> @@ -1143,21 +1099,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);

Why do we need to cast?  If all callers are passing a Netdev, can't we 
just give 'object' the correct type to begin with?


> +++ b/qapi/net.json
> @@ -453,7 +453,7 @@
>   #        'l2tpv3' - since 2.1
>   ##
>   { 'union': 'Netdev',
> -  'base': { 'id': 'str', 'type': 'NetClientDriver' },
> +  'base': { '*id': 'str', 'type': 'NetClientDriver' },

I don't think we need to make 'id' optional.

>     'discriminator': 'type',
>     'data': {
>       'nic':      'NetLegacyNicOptions',
> @@ -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'] }

NetLegacyOptionsType differs from NetClientDriver only by missing 
'hubport', which your code above special-cased, so on that front, the 
simplification makes sense.

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

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

[Thomas Huth]

On 12/05/2020 16.26, Eric Blake wrote:
> On 5/12/20 7:31 AM, Thomas Huth wrote:
>> 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(-)
>>
> 
>> +++ 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' } }
> 
> Why is 'id' left optional? I'd expect it to be mandatory, now.

That's because it is still optional, indeed. It is currently perfectly
valid to run "qemu-system-xyz -net user -net nic" without specifying any
"id". If I remove the "*" here, the old syntax breaks. I don't think
that we want this here, so "*id" has to stay optional.
(FWIW, the ID is then created internally, see assign_name() in net/net.c)

 Thomas

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

[Thomas Huth]

On 12/05/2020 16.32, Eric Blake wrote:
> On 5/12/20 7:31 AM, 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>
>> ---
> 
>> +++ 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)
> 
> Why do we still need the 'is_netdev' parameter?

Yes. See net_init_client(), it uses "false" here.

>  If all callers are
> passing in a netdev, then either this parameter needs to be renamed to
> capture the actual difference between callers, or it can be dropped
> altogether.

Don't think of the "Netdev" structure when you're looking at
"is_netdev", rather think about the "-netdev" comand line parameter.
"is_netdev" is used to distinguish between "-netdev" and "-net".

>>   {
>> -    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;
>> +        }
> 
> You wouldn't need this if 'id' remained mandatory.

It's still required for "-net" - see my reply to patch 1/2.

>> @@ -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 */
> 
> 
>> -
>> -        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)");
> 
> So maybe we still want this legacy-handling code, but renaming
> 'is_netdev' to 'legacy_handling' may make more sense.

Hmm, maybe renaming would be good indeed to avoid confusion here... I'll
think about it.

>> @@ -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);
>>           }
>>       }
>> @@ -1143,21 +1099,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);
> 
> Why do we need to cast?  If all callers are passing a Netdev, can't we
> just give 'object' the correct type to begin with?

Agreed, it should be possible to use Netdev* instead of void* for
"object" now. I'll change that in v3.

>> +++ b/qapi/net.json
>> @@ -453,7 +453,7 @@
>>   #        'l2tpv3' - since 2.1
>>   ##
>>   { 'union': 'Netdev',
>> -  'base': { 'id': 'str', 'type': 'NetClientDriver' },
>> +  'base': { '*id': 'str', 'type': 'NetClientDriver' },
> 
> I don't think we need to make 'id' optional.

It's required for "-net" now.

>>     'discriminator': 'type',
>>     'data': {
>>       'nic':      'NetLegacyNicOptions',
>> @@ -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'] }
> 
> NetLegacyOptionsType differs from NetClientDriver only by missing
> 'hubport', which your code above special-cased, so on that front, the
> simplification makes sense.

Thanks for your review!

 Thomas

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

[Eric Blake]

On 5/12/20 10:13 AM, Thomas Huth wrote:

>>> +++ b/qapi/net.json
>>> @@ -453,7 +453,7 @@
>>>    #        'l2tpv3' - since 2.1
>>>    ##
>>>    { 'union': 'Netdev',
>>> -  'base': { 'id': 'str', 'type': 'NetClientDriver' },
>>> +  'base': { '*id': 'str', 'type': 'NetClientDriver' },
>>
>> I don't think we need to make 'id' optional.
> 
> It's required for "-net" now.

But does -net generate it's own id if one is not provided?  Can it still 
be mandatory in the QAPI type, and just figure out how to guarantee that 
the CLI parsing of -net provides a name early enough in the cycle to use 
the QAPI type without making the member optional there?

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

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

[Thomas Huth]

On 12/05/2020 17.51, Eric Blake wrote:
> On 5/12/20 10:13 AM, Thomas Huth wrote:
> 
>>>> +++ b/qapi/net.json
>>>> @@ -453,7 +453,7 @@
>>>>    #        'l2tpv3' - since 2.1
>>>>    ##
>>>>    { 'union': 'Netdev',
>>>> -  'base': { 'id': 'str', 'type': 'NetClientDriver' },
>>>> +  'base': { '*id': 'str', 'type': 'NetClientDriver' },
>>>
>>> I don't think we need to make 'id' optional.
>>
>> It's required for "-net" now.
> 
> But does -net generate it's own id if one is not provided?  Can it still
> be mandatory in the QAPI type, and just figure out how to guarantee that
> the CLI parsing of -net provides a name early enough in the cycle to use
> the QAPI type without making the member optional there?

I guess it could be done - but we'd need to change the internal naming
scheme for this. Currently, the internal id / name is created with
assign_name() during qemu_net_client_setup() - which is the function
that is called from the individual network backends via
qemu_new_net_client() with a "model" string, e.g. net/slirp.c calls it
with model="user", net/tap.c calls it with model="bridge" etc.
The model string is then used in assign_name() to create the internal id
(aka. name in the NetClientState).

If we want to keep the "id" mandatory in the QAPI type, we have to
create the internal id before calling visit_type_Netdev() in
net_client_init(). But the "model" string information is not available
here yet, since this takes place before the init function of the backend
is called. Thus we'd need to come up with a different internal id string
here, e.g. something like "__org.qemu.net-xyz" like it is done in
net_param_nic() already. Do you think it is ok to change the internal
name / id here? I think it should be ok - if the users care about the id
of the netdevs, they should specify the ids on the command line and not
rely on some internal naming schemes... do you agree?

 Thomas