[PATCH 0/1] net: Fix handling of id in netdev_add and netdev_del

[PATCH 0/1] net: Fix handling of id in netdev_add and netdev_del

[Markus Armbruster]

This is a regression fix, but the regression is already in 5.0.  I
think it's too late for 5.2.  If I'm right, then the issue should be
documented in the release notes (I can do that).

Markus Armbruster (1):
  net: Fix handling of id in netdev_add and netdev_del

 net/net.c | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

-- 
2.26.2

[PATCH 1/1] net: Fix handling of id in netdev_add and netdev_del

[Markus Armbruster]

CLI -netdev accumulates in option group "netdev".

Before commit 08712fcb85 "net: Track netdevs in NetClientState rather
than QemuOpt", netdev_add added to the option group, and netdev_del
removed from it, both HMP and QMP.  Thus, every netdev had a
corresponding QemuOpts in this option group.

Commit 08712fcb85 dropped this for QMP netdev_add and both netdev_del.
Now a netdev has a corresponding QemuOpts only when it was created
with CLI or HMP.  Two issues:

* QMP and HMP netdev_del can leave QemuOpts behind, breaking HMP
  netdev_add.  Reproducer:

    $ qemu-system-x86_64 -S -display none -nodefaults -monitor stdio
    QEMU 5.1.92 monitor - type 'help' for more information
    (qemu) netdev_add user,id=net0
    (qemu) info network
    net0: index=0,type=user,net=10.0.2.0,restrict=off
    (qemu) netdev_del net0
    (qemu) info network
    (qemu) netdev_add user,id=net0
    upstream-qemu: Duplicate ID 'net0' for netdev
    Try "help netdev_add" for more information

  Fix by restoring the QemuOpts deletion in qmp_netdev_del(), but with
  a guard, because the QemuOpts need not exist.

* QMP netdev_add loses its "no duplicate ID" check.  Reproducer:

    $ qemu-system-x86_64 -S -display none -qmp stdio
    {"QMP": {"version": {"qemu": {"micro": 92, "minor": 1, "major": 5}, "package": "v5.2.0-rc2-1-g02c1f0142c"}, "capabilities": ["oob"]}}
    {"execute": "qmp_capabilities"}
    {"return": {}}
    {"execute": "netdev_add", "arguments": {"type": "user", "id":"net0"}}
    {"return": {}}
    {"execute": "netdev_add", "arguments": {"type": "user", "id":"net0"}}
    {"return": {}}

  Fix by adding a duplicate ID check to net_client_init1() to replace
  the lost one.  The check is redundant for callers where QemuOpts
  still checks, i.e. for CLI and HMP.

Reported-by: Andrew Melnichenko <andrew@daynix.com>
Fixes: 08712fcb851034228b61f75bd922863a984a4f60
Cc: qemu-stable@nongnu.org
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 net/net.c | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/net/net.c b/net/net.c
index 794c652282..c1dc75fc37 100644
--- a/net/net.c
+++ b/net/net.c
@@ -978,6 +978,7 @@ static int (* const net_client_init_fun[NET_CLIENT_DRIVER__MAX])(
 static int net_client_init1(const Netdev *netdev, bool is_netdev, Error **errp)
 {
     NetClientState *peer = NULL;
+    NetClientState *nc;
 
     if (is_netdev) {
         if (netdev->type == NET_CLIENT_DRIVER_NIC ||
@@ -1005,6 +1006,12 @@ static int net_client_init1(const Netdev *netdev, bool is_netdev, Error **errp)
         }
     }
 
+    nc = qemu_find_netdev(netdev->id);
+    if (nc) {
+        error_setg(errp, "Duplicate ID '%s'", netdev->id);
+        return -1;
+    }
+
     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) {
@@ -1015,8 +1022,6 @@ static int net_client_init1(const Netdev *netdev, bool is_netdev, Error **errp)
     }
 
     if (is_netdev) {
-        NetClientState *nc;
-
         nc = qemu_find_netdev(netdev->id);
         assert(nc);
         nc->is_netdev = true;
@@ -1137,6 +1142,7 @@ void qmp_netdev_add(Netdev *netdev, Error **errp)
 void qmp_netdev_del(const char *id, Error **errp)
 {
     NetClientState *nc;
+    QemuOpts *opts;
 
     nc = qemu_find_netdev(id);
     if (!nc) {
@@ -1151,6 +1157,16 @@ void qmp_netdev_del(const char *id, Error **errp)
     }
 
     qemu_del_net_client(nc);
+
+    /*
+     * Wart: we need to delete the QemuOpts associated with netdevs
+     * created via CLI or HMP, to avoid bogus "Duplicate ID" errors in
+     * HMP netdev_add.
+     */
+    opts = qemu_opts_find(qemu_find_opts("netdev"), id);
+    if (opts) {
+        qemu_opts_del(opts);
+    }
 }
 
 static void netfilter_print_info(Monitor *mon, NetFilterState *nf)
-- 
2.26.2

Re: [PATCH 1/1] net: Fix handling of id in netdev_add and netdev_del

[Eric Blake]

On 11/25/20 4:02 AM, Markus Armbruster wrote:
> CLI -netdev accumulates in option group "netdev".
> 
> Before commit 08712fcb85 "net: Track netdevs in NetClientState rather
> than QemuOpt", netdev_add added to the option group, and netdev_del
> removed from it, both HMP and QMP.  Thus, every netdev had a
> corresponding QemuOpts in this option group.
> 
> Commit 08712fcb85 dropped this for QMP netdev_add and both netdev_del.
> Now a netdev has a corresponding QemuOpts only when it was created
> with CLI or HMP.  Two issues:
> 
> * QMP and HMP netdev_del can leave QemuOpts behind, breaking HMP
>   netdev_add.  Reproducer:
> 
>     $ qemu-system-x86_64 -S -display none -nodefaults -monitor stdio
>     QEMU 5.1.92 monitor - type 'help' for more information
>     (qemu) netdev_add user,id=net0
>     (qemu) info network
>     net0: index=0,type=user,net=10.0.2.0,restrict=off
>     (qemu) netdev_del net0
>     (qemu) info network
>     (qemu) netdev_add user,id=net0
>     upstream-qemu: Duplicate ID 'net0' for netdev
>     Try "help netdev_add" for more information
> 
>   Fix by restoring the QemuOpts deletion in qmp_netdev_del(), but with
>   a guard, because the QemuOpts need not exist.
> 
> * QMP netdev_add loses its "no duplicate ID" check.  Reproducer:
> 
>     $ qemu-system-x86_64 -S -display none -qmp stdio
>     {"QMP": {"version": {"qemu": {"micro": 92, "minor": 1, "major": 5}, "package": "v5.2.0-rc2-1-g02c1f0142c"}, "capabilities": ["oob"]}}
>     {"execute": "qmp_capabilities"}
>     {"return": {}}
>     {"execute": "netdev_add", "arguments": {"type": "user", "id":"net0"}}
>     {"return": {}}
>     {"execute": "netdev_add", "arguments": {"type": "user", "id":"net0"}}
>     {"return": {}}
> 
>   Fix by adding a duplicate ID check to net_client_init1() to replace
>   the lost one.  The check is redundant for callers where QemuOpts
>   still checks, i.e. for CLI and HMP.
> 
> Reported-by: Andrew Melnichenko <andrew@daynix.com>
> Fixes: 08712fcb851034228b61f75bd922863a984a4f60
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  net/net.c | 20 ++++++++++++++++++--
>  1 file changed, 18 insertions(+), 2 deletions(-)

The adventure in QAPIfication of netdev_add has been long.  Thanks for
working on this.

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 0/1] net: Fix handling of id in netdev_add and netdev_del

[Jason Wang]

On 2020/11/25 下午6:02, Markus Armbruster wrote:
> This is a regression fix, but the regression is already in 5.0.  I
> think it's too late for 5.2.  If I'm right, then the issue should be
> documented in the release notes (I can do that).


Please do that and I've queued this for the next release.

Thanks


>
> Markus Armbruster (1):
>    net: Fix handling of id in netdev_add and netdev_del
>
>   net/net.c | 20 ++++++++++++++++++--
>   1 file changed, 18 insertions(+), 2 deletions(-)
>

Re: [PATCH 0/1] net: Fix handling of id in netdev_add and netdev_del

[Markus Armbruster]

Jason Wang <jasowang@redhat.com> writes:

> On 2020/11/25 下午6:02, Markus Armbruster wrote:
>> This is a regression fix, but the regression is already in 5.0.  I
>> think it's too late for 5.2.  If I'm right, then the issue should be
>> documented in the release notes (I can do that).
>
>
> Please do that and I've queued this for the next release.

Done.  Thanks!

Re: [PATCH 0/1] net: Fix handling of id in netdev_add and netdev_del

[Markus Armbruster]

Jason Wang <jasowang@redhat.com> writes:

> On 2020/11/25 下午6:02, Markus Armbruster wrote:
>> This is a regression fix, but the regression is already in 5.0.  I
>> think it's too late for 5.2.  If I'm right, then the issue should be
>> documented in the release notes (I can do that).
>
>
> Please do that and I've queued this for the next release.

Ping?

Re: [PATCH 0/1] net: Fix handling of id in netdev_add and netdev_del

[Jason Wang]

On 2021/1/15 下午9:56, Markus Armbruster wrote:
> Jason Wang <jasowang@redhat.com> writes:
>
>> On 2020/11/25 下午6:02, Markus Armbruster wrote:
>>> This is a regression fix, but the regression is already in 5.0.  I
>>> think it's too late for 5.2.  If I'm right, then the issue should be
>>> documented in the release notes (I can do that).
>>
>> Please do that and I've queued this for the next release.
> Ping?
>
>

Hi Markus:

I plan to send a pull request by the end of this week which should 
contain this series.

Thanks