* Re: modify NetdevUserOptions through QMP in QEMU 6 - how? [not found] ` <007f7313-eeb2-ee6a-ae2e-9341324388c0@redhat.com> @ 2021-12-14 14:53 ` Michael S. Tsirkin 2021-12-15 3:31 ` Jason Wang 2021-12-15 13:38 ` Alexander Sosedkin 0 siblings, 2 replies; 8+ messages in thread From: Michael S. Tsirkin @ 2021-12-14 14:53 UTC (permalink / raw) To: Thomas Huth Cc: Alexander Sosedkin, qemu-devel, Jason Wang, Markus Armbruster, qemu-discuss, Samuel Thibault, Marc-André Lureau On Mon, Dec 13, 2021 at 09:02:15AM +0100, Thomas Huth wrote: > Hi! > > On 10/12/2021 18.02, Alexander Sosedkin wrote: > > With QEMU 5 I could totally issue a QMP netdev_add > > with the same ID to adjust the NetdevUserOptions I want, > > such as restrict or hostfwd. No deleting needed, > > just a netdev_add with what I want changed as a param. > > I'm a little bit surprised that this worked, since AFAIK there is no code in > QEMU to *change* the parameters of a running netdev... likely the code added > a new netdev with the same ID, replacing the old one? > > > With QEMU 6 it started failing, claiming the ID is already used. > > And if I do netdev_del + netdev_add, I just lose connectivity. > > What's even stranger, I still see old netdev attached in info network: > > > > > netdev_del {'id': 'net0'} > > {} > > > human-monitor-command {'command-line': 'info network'} > > virtio-net-pci.0: > > index=0,type=nic,model=virtio-net-pci,macaddr=52:54:00:12:34:56 > > \ net0: index=0,type=user,net=10.0.2.0,restrict=off > > I think that's "normal" - there used to be problems in the past that the > devices (virtio-net-pci in this case) did not like the netdevs to be removed > on the fly. So the netdevs are kept around until you remove the device, too > (i.e. issue a device_del for the virtio-net-pci device). > > > > netdev_add {'type': 'user', 'id': 'net0', 'restrict': False, 'hostfwd': [{'str': 'tcp:127.0.0.1:58239-:22'}]} > > {} > > > human-monitor-command {'command-line': 'info network'} > > unseal: virtio-net-pci.0: > > index=0,type=nic,model=virtio-net-pci,macaddr=52:54:00:12:34:56 > > \ net0: index=0,type=user,net=10.0.2.0,restrict=off > > net0: index=0,type=user,net=10.0.2.0,restrict=off > > > > What's the correct QMP command sequence to modify NetdevUserOptions? > > AFAIK there is no way to modify running netdevs - you'd have to delete the > netdev and the device, and then add both again. But I might have missed > something here, so I CC:-ed some people who might be more familiar with the > details here. > > Thomas > > > > Please CC me on replies. Wow this really goes to show how wide our feature matrix is. Yes it's probably an unintended side effect but yes it did work it seems, so we really should not just break it without warning. Probably this one: commit 831734cce6494032e9233caff4d8442b3a1e7fef Author: Markus Armbruster <armbru@redhat.com> Date: Wed Nov 25 11:02:20 2020 +0100 net: Fix handling of id in netdev_add and netdev_del Jason, what is your take here? Alexander, what happens if we just drop the duplicate ID check? Do things work for you again? Warning: completely untested. Signed-off-by: Michael S. Tsirkin <mst@redhat.com> diff --git a/net/net.c b/net/net.c index f0d14dbfc1..01f5a187b6 100644 --- a/net/net.c +++ b/net/net.c @@ -1055,12 +1055,6 @@ 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) { -- MST ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: modify NetdevUserOptions through QMP in QEMU 6 - how? 2021-12-14 14:53 ` modify NetdevUserOptions through QMP in QEMU 6 - how? Michael S. Tsirkin @ 2021-12-15 3:31 ` Jason Wang 2021-12-15 6:48 ` Markus Armbruster 2021-12-15 7:03 ` Thomas Huth 2021-12-15 13:38 ` Alexander Sosedkin 1 sibling, 2 replies; 8+ messages in thread From: Jason Wang @ 2021-12-15 3:31 UTC (permalink / raw) To: Michael S. Tsirkin Cc: Thomas Huth, Alexander Sosedkin, qemu-devel, Markus Armbruster, qemu-discuss, Samuel Thibault, Marc-André Lureau On Tue, Dec 14, 2021 at 10:53 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > On Mon, Dec 13, 2021 at 09:02:15AM +0100, Thomas Huth wrote: > > Hi! > > > > On 10/12/2021 18.02, Alexander Sosedkin wrote: > > > With QEMU 5 I could totally issue a QMP netdev_add > > > with the same ID to adjust the NetdevUserOptions I want, > > > such as restrict or hostfwd. No deleting needed, > > > just a netdev_add with what I want changed as a param. > > > > I'm a little bit surprised that this worked, since AFAIK there is no code in > > QEMU to *change* the parameters of a running netdev... likely the code added > > a new netdev with the same ID, replacing the old one? > > > > > With QEMU 6 it started failing, claiming the ID is already used. > > > And if I do netdev_del + netdev_add, I just lose connectivity. > > > What's even stranger, I still see old netdev attached in info network: > > > > > > > netdev_del {'id': 'net0'} > > > {} > > > > human-monitor-command {'command-line': 'info network'} > > > virtio-net-pci.0: > > > index=0,type=nic,model=virtio-net-pci,macaddr=52:54:00:12:34:56 > > > \ net0: index=0,type=user,net=10.0.2.0,restrict=off > > > > I think that's "normal" - there used to be problems in the past that the > > devices (virtio-net-pci in this case) did not like the netdevs to be removed > > on the fly. So the netdevs are kept around until you remove the device, too > > (i.e. issue a device_del for the virtio-net-pci device). > > > > > > netdev_add {'type': 'user', 'id': 'net0', 'restrict': False, 'hostfwd': [{'str': 'tcp:127.0.0.1:58239-:22'}]} > > > {} > > > > human-monitor-command {'command-line': 'info network'} > > > unseal: virtio-net-pci.0: > > > index=0,type=nic,model=virtio-net-pci,macaddr=52:54:00:12:34:56 > > > \ net0: index=0,type=user,net=10.0.2.0,restrict=off > > > net0: index=0,type=user,net=10.0.2.0,restrict=off > > > > > > What's the correct QMP command sequence to modify NetdevUserOptions? > > > > AFAIK there is no way to modify running netdevs - you'd have to delete the > > netdev and the device, and then add both again. But I might have missed > > something here, so I CC:-ed some people who might be more familiar with the > > details here. > > > > Thomas > > > > > > > Please CC me on replies. > > > Wow this really goes to show how wide our feature matrix is. > > Yes it's probably an unintended side effect but yes it > did work it seems, so we really should not just break it > without warning. > > > Probably this one: > > commit 831734cce6494032e9233caff4d8442b3a1e7fef > Author: Markus Armbruster <armbru@redhat.com> > Date: Wed Nov 25 11:02:20 2020 +0100 > > net: Fix handling of id in netdev_add and netdev_del > > > > Jason, what is your take here? I might be wrong, but I agree with Thomas. Adding a netdev with the same ID looks wrong, if it works, it looks like a bug. And I don't think we support changing netdev properties. Thanks > > > Alexander, what happens if we just drop the duplicate ID check? Do > things work for you again? > Warning: completely untested. > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > > diff --git a/net/net.c b/net/net.c > index f0d14dbfc1..01f5a187b6 100644 > --- a/net/net.c > +++ b/net/net.c > @@ -1055,12 +1055,6 @@ 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) { > -- > MST > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: modify NetdevUserOptions through QMP in QEMU 6 - how? 2021-12-15 3:31 ` Jason Wang @ 2021-12-15 6:48 ` Markus Armbruster 2021-12-15 7:22 ` Michael S. Tsirkin 2021-12-15 7:03 ` Thomas Huth 1 sibling, 1 reply; 8+ messages in thread From: Markus Armbruster @ 2021-12-15 6:48 UTC (permalink / raw) To: Jason Wang Cc: Thomas Huth, Alexander Sosedkin, Michael S. Tsirkin, qemu-devel, qemu-discuss, Samuel Thibault, Marc-André Lureau Jason Wang <jasowang@redhat.com> writes: > On Tue, Dec 14, 2021 at 10:53 PM Michael S. Tsirkin <mst@redhat.com> wrote: >> >> On Mon, Dec 13, 2021 at 09:02:15AM +0100, Thomas Huth wrote: >> > Hi! >> > >> > On 10/12/2021 18.02, Alexander Sosedkin wrote: >> > > With QEMU 5 I could totally issue a QMP netdev_add >> > > with the same ID to adjust the NetdevUserOptions I want, >> > > such as restrict or hostfwd. No deleting needed, >> > > just a netdev_add with what I want changed as a param. >> > >> > I'm a little bit surprised that this worked, since AFAIK there is no code in >> > QEMU to *change* the parameters of a running netdev... likely the code added >> > a new netdev with the same ID, replacing the old one? >> > >> > > With QEMU 6 it started failing, claiming the ID is already used. >> > > And if I do netdev_del + netdev_add, I just lose connectivity. >> > > What's even stranger, I still see old netdev attached in info network: >> > > >> > > > netdev_del {'id': 'net0'} >> > > {} >> > > > human-monitor-command {'command-line': 'info network'} >> > > virtio-net-pci.0: >> > > index=0,type=nic,model=virtio-net-pci,macaddr=52:54:00:12:34:56 >> > > \ net0: index=0,type=user,net=10.0.2.0,restrict=off >> > >> > I think that's "normal" - there used to be problems in the past that the >> > devices (virtio-net-pci in this case) did not like the netdevs to be removed >> > on the fly. So the netdevs are kept around until you remove the device, too >> > (i.e. issue a device_del for the virtio-net-pci device). >> > >> > > > netdev_add {'type': 'user', 'id': 'net0', 'restrict': False, 'hostfwd': [{'str': 'tcp:127.0.0.1:58239-:22'}]} >> > > {} >> > > > human-monitor-command {'command-line': 'info network'} >> > > unseal: virtio-net-pci.0: >> > > index=0,type=nic,model=virtio-net-pci,macaddr=52:54:00:12:34:56 >> > > \ net0: index=0,type=user,net=10.0.2.0,restrict=off >> > > net0: index=0,type=user,net=10.0.2.0,restrict=off >> > > >> > > What's the correct QMP command sequence to modify NetdevUserOptions? >> > >> > AFAIK there is no way to modify running netdevs - you'd have to delete the >> > netdev and the device, and then add both again. But I might have missed >> > something here, so I CC:-ed some people who might be more familiar with the >> > details here. >> > >> > Thomas >> > >> > >> > > Please CC me on replies. >> >> >> Wow this really goes to show how wide our feature matrix is. >> >> Yes it's probably an unintended side effect but yes it >> did work it seems, so we really should not just break it >> without warning. Depends. See below. >> Probably this one: >> >> commit 831734cce6494032e9233caff4d8442b3a1e7fef >> Author: Markus Armbruster <armbru@redhat.com> >> Date: Wed Nov 25 11:02:20 2020 +0100 >> >> net: Fix handling of id in netdev_add and netdev_del 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> Reviewed-by: Eric Blake <eblake@redhat.com> Signed-off-by: Jason Wang <jasowang@redhat.com> Both issues were regressions. Like Thomas, I'm surprised that adding a netdev with a duplicate ID changes parameters. Unintended side effect of a regression. I suspect it only ever "worked" between commit 08712fcb85 "net: Track netdevs in NetClientState rather than QemuOpt" (v5.0.0) and commit 831734cce6 "net: Fix handling of id in netdev_add and netdev_del" (v6.0.0). Got a reproducer for me so I can double-check? >> Jason, what is your take here? > > I might be wrong, but I agree with Thomas. Adding a netdev with the > same ID looks wrong, if it works, it looks like a bug. And I don't > think we support changing netdev properties. Ability to adjust backend parameters feels like a valid feature request. But we shouldn't do it by exploiting a bug's side effect. The bug may have other side effects, possibly bad ones. "ID is unique" is an invariant. Code may rely on it. We don't know what happens when we violate it. [...] ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: modify NetdevUserOptions through QMP in QEMU 6 - how? 2021-12-15 6:48 ` Markus Armbruster @ 2021-12-15 7:22 ` Michael S. Tsirkin 0 siblings, 0 replies; 8+ messages in thread From: Michael S. Tsirkin @ 2021-12-15 7:22 UTC (permalink / raw) To: Markus Armbruster Cc: Thomas Huth, Alexander Sosedkin, Jason Wang, qemu-devel, qemu-discuss, Samuel Thibault, Marc-André Lureau On Wed, Dec 15, 2021 at 07:48:06AM +0100, Markus Armbruster wrote: > Jason Wang <jasowang@redhat.com> writes: > > > On Tue, Dec 14, 2021 at 10:53 PM Michael S. Tsirkin <mst@redhat.com> wrote: > >> > >> On Mon, Dec 13, 2021 at 09:02:15AM +0100, Thomas Huth wrote: > >> > Hi! > >> > > >> > On 10/12/2021 18.02, Alexander Sosedkin wrote: > >> > > With QEMU 5 I could totally issue a QMP netdev_add > >> > > with the same ID to adjust the NetdevUserOptions I want, > >> > > such as restrict or hostfwd. No deleting needed, > >> > > just a netdev_add with what I want changed as a param. > >> > > >> > I'm a little bit surprised that this worked, since AFAIK there is no code in > >> > QEMU to *change* the parameters of a running netdev... likely the code added > >> > a new netdev with the same ID, replacing the old one? > >> > > >> > > With QEMU 6 it started failing, claiming the ID is already used. > >> > > And if I do netdev_del + netdev_add, I just lose connectivity. > >> > > What's even stranger, I still see old netdev attached in info network: > >> > > > >> > > > netdev_del {'id': 'net0'} > >> > > {} > >> > > > human-monitor-command {'command-line': 'info network'} > >> > > virtio-net-pci.0: > >> > > index=0,type=nic,model=virtio-net-pci,macaddr=52:54:00:12:34:56 > >> > > \ net0: index=0,type=user,net=10.0.2.0,restrict=off > >> > > >> > I think that's "normal" - there used to be problems in the past that the > >> > devices (virtio-net-pci in this case) did not like the netdevs to be removed > >> > on the fly. So the netdevs are kept around until you remove the device, too > >> > (i.e. issue a device_del for the virtio-net-pci device). > >> > > >> > > > netdev_add {'type': 'user', 'id': 'net0', 'restrict': False, 'hostfwd': [{'str': 'tcp:127.0.0.1:58239-:22'}]} > >> > > {} > >> > > > human-monitor-command {'command-line': 'info network'} > >> > > unseal: virtio-net-pci.0: > >> > > index=0,type=nic,model=virtio-net-pci,macaddr=52:54:00:12:34:56 > >> > > \ net0: index=0,type=user,net=10.0.2.0,restrict=off > >> > > net0: index=0,type=user,net=10.0.2.0,restrict=off > >> > > > >> > > What's the correct QMP command sequence to modify NetdevUserOptions? > >> > > >> > AFAIK there is no way to modify running netdevs - you'd have to delete the > >> > netdev and the device, and then add both again. But I might have missed > >> > something here, so I CC:-ed some people who might be more familiar with the > >> > details here. > >> > > >> > Thomas > >> > > >> > > >> > > Please CC me on replies. > >> > >> > >> Wow this really goes to show how wide our feature matrix is. > >> > >> Yes it's probably an unintended side effect but yes it > >> did work it seems, so we really should not just break it > >> without warning. > > Depends. See below. > > >> Probably this one: > >> > >> commit 831734cce6494032e9233caff4d8442b3a1e7fef > >> Author: Markus Armbruster <armbru@redhat.com> > >> Date: Wed Nov 25 11:02:20 2020 +0100 > >> > >> net: Fix handling of id in netdev_add and netdev_del > > 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> > Reviewed-by: Eric Blake <eblake@redhat.com> > Signed-off-by: Jason Wang <jasowang@redhat.com> > > Both issues were regressions. > > Like Thomas, I'm surprised that adding a netdev with a duplicate ID > changes parameters. Unintended side effect of a regression. > > I suspect it only ever "worked" between commit 08712fcb85 "net: Track > netdevs in NetClientState rather than QemuOpt" (v5.0.0) and commit > 831734cce6 "net: Fix handling of id in netdev_add and netdev_del" > (v6.0.0). > > Got a reproducer for me so I can double-check? Alexander? > >> Jason, what is your take here? > > > > I might be wrong, but I agree with Thomas. Adding a netdev with the > > same ID looks wrong, if it works, it looks like a bug. And I don't > > think we support changing netdev properties. > > Ability to adjust backend parameters feels like a valid feature request. > But we shouldn't do it by exploiting a bug's side effect. The bug may > have other side effects, possibly bad ones. "ID is unique" is an > invariant. Code may rely on it. We don't know what happens when we > violate it. > > [...] ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: modify NetdevUserOptions through QMP in QEMU 6 - how? 2021-12-15 3:31 ` Jason Wang 2021-12-15 6:48 ` Markus Armbruster @ 2021-12-15 7:03 ` Thomas Huth 2021-12-15 7:19 ` Michael S. Tsirkin 2021-12-15 7:21 ` Michael S. Tsirkin 1 sibling, 2 replies; 8+ messages in thread From: Thomas Huth @ 2021-12-15 7:03 UTC (permalink / raw) To: Jason Wang, Michael S. Tsirkin Cc: Alexander Sosedkin, qemu-devel, Markus Armbruster, Samuel Thibault, Marc-André Lureau, qemu-discuss On 15/12/2021 04.31, Jason Wang wrote: > On Tue, Dec 14, 2021 at 10:53 PM Michael S. Tsirkin <mst@redhat.com> wrote: >> >> On Mon, Dec 13, 2021 at 09:02:15AM +0100, Thomas Huth wrote: >>> Hi! >>> >>> On 10/12/2021 18.02, Alexander Sosedkin wrote: >>>> With QEMU 5 I could totally issue a QMP netdev_add >>>> with the same ID to adjust the NetdevUserOptions I want, >>>> such as restrict or hostfwd. No deleting needed, >>>> just a netdev_add with what I want changed as a param. >>> >>> I'm a little bit surprised that this worked, since AFAIK there is no code in >>> QEMU to *change* the parameters of a running netdev... likely the code added >>> a new netdev with the same ID, replacing the old one? >>> >>>> With QEMU 6 it started failing, claiming the ID is already used. >>>> And if I do netdev_del + netdev_add, I just lose connectivity. >>>> What's even stranger, I still see old netdev attached in info network: >>>> >>>>> netdev_del {'id': 'net0'} >>>> {} >>>>> human-monitor-command {'command-line': 'info network'} >>>> virtio-net-pci.0: >>>> index=0,type=nic,model=virtio-net-pci,macaddr=52:54:00:12:34:56 >>>> \ net0: index=0,type=user,net=10.0.2.0,restrict=off >>> >>> I think that's "normal" - there used to be problems in the past that the >>> devices (virtio-net-pci in this case) did not like the netdevs to be removed >>> on the fly. So the netdevs are kept around until you remove the device, too >>> (i.e. issue a device_del for the virtio-net-pci device). >>> >>>>> netdev_add {'type': 'user', 'id': 'net0', 'restrict': False, 'hostfwd': [{'str': 'tcp:127.0.0.1:58239-:22'}]} >>>> {} >>>>> human-monitor-command {'command-line': 'info network'} >>>> unseal: virtio-net-pci.0: >>>> index=0,type=nic,model=virtio-net-pci,macaddr=52:54:00:12:34:56 >>>> \ net0: index=0,type=user,net=10.0.2.0,restrict=off >>>> net0: index=0,type=user,net=10.0.2.0,restrict=off >>>> >>>> What's the correct QMP command sequence to modify NetdevUserOptions? >>> >>> AFAIK there is no way to modify running netdevs - you'd have to delete the >>> netdev and the device, and then add both again. But I might have missed >>> something here, so I CC:-ed some people who might be more familiar with the >>> details here. >>> >>> Thomas >>> >>> >>>> Please CC me on replies. >> >> >> Wow this really goes to show how wide our feature matrix is. >> >> Yes it's probably an unintended side effect but yes it >> did work it seems, so we really should not just break it >> without warning. >> >> >> Probably this one: >> >> commit 831734cce6494032e9233caff4d8442b3a1e7fef >> Author: Markus Armbruster <armbru@redhat.com> >> Date: Wed Nov 25 11:02:20 2020 +0100 >> >> net: Fix handling of id in netdev_add and netdev_del >> >> >> >> Jason, what is your take here? > > I might be wrong, but I agree with Thomas. Adding a netdev with the > same ID looks wrong, if it works, it looks like a bug. It certainly calls for trouble as soon as you try to delete the netdev again - does it delete the first (inactive) instance? Does it delete the second active one? Does it delete both? (Otherwise it will leave a dangling instance behind) ... So if changing netdev parameters on the fly is something that we want, we should implement this properly instead indeed, and not via such an accidental bug. Thomas ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: modify NetdevUserOptions through QMP in QEMU 6 - how? 2021-12-15 7:03 ` Thomas Huth @ 2021-12-15 7:19 ` Michael S. Tsirkin 2021-12-15 7:21 ` Michael S. Tsirkin 1 sibling, 0 replies; 8+ messages in thread From: Michael S. Tsirkin @ 2021-12-15 7:19 UTC (permalink / raw) To: Thomas Huth Cc: Alexander Sosedkin, qemu-devel, Jason Wang, Markus Armbruster, qemu-discuss, Samuel Thibault, Marc-André Lureau On Wed, Dec 15, 2021 at 08:03:50AM +0100, Thomas Huth wrote: > On 15/12/2021 04.31, Jason Wang wrote: > > On Tue, Dec 14, 2021 at 10:53 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > > > On Mon, Dec 13, 2021 at 09:02:15AM +0100, Thomas Huth wrote: > > > > Hi! > > > > > > > > On 10/12/2021 18.02, Alexander Sosedkin wrote: > > > > > With QEMU 5 I could totally issue a QMP netdev_add > > > > > with the same ID to adjust the NetdevUserOptions I want, > > > > > such as restrict or hostfwd. No deleting needed, > > > > > just a netdev_add with what I want changed as a param. > > > > > > > > I'm a little bit surprised that this worked, since AFAIK there is no code in > > > > QEMU to *change* the parameters of a running netdev... likely the code added > > > > a new netdev with the same ID, replacing the old one? > > > > > > > > > With QEMU 6 it started failing, claiming the ID is already used. > > > > > And if I do netdev_del + netdev_add, I just lose connectivity. > > > > > What's even stranger, I still see old netdev attached in info network: > > > > > > > > > > > netdev_del {'id': 'net0'} > > > > > {} > > > > > > human-monitor-command {'command-line': 'info network'} > > > > > virtio-net-pci.0: > > > > > index=0,type=nic,model=virtio-net-pci,macaddr=52:54:00:12:34:56 > > > > > \ net0: index=0,type=user,net=10.0.2.0,restrict=off > > > > > > > > I think that's "normal" - there used to be problems in the past that the > > > > devices (virtio-net-pci in this case) did not like the netdevs to be removed > > > > on the fly. So the netdevs are kept around until you remove the device, too > > > > (i.e. issue a device_del for the virtio-net-pci device). > > > > > > > > > > netdev_add {'type': 'user', 'id': 'net0', 'restrict': False, 'hostfwd': [{'str': 'tcp:127.0.0.1:58239-:22'}]} > > > > > {} > > > > > > human-monitor-command {'command-line': 'info network'} > > > > > unseal: virtio-net-pci.0: > > > > > index=0,type=nic,model=virtio-net-pci,macaddr=52:54:00:12:34:56 > > > > > \ net0: index=0,type=user,net=10.0.2.0,restrict=off > > > > > net0: index=0,type=user,net=10.0.2.0,restrict=off > > > > > > > > > > What's the correct QMP command sequence to modify NetdevUserOptions? > > > > > > > > AFAIK there is no way to modify running netdevs - you'd have to delete the > > > > netdev and the device, and then add both again. But I might have missed > > > > something here, so I CC:-ed some people who might be more familiar with the > > > > details here. > > > > > > > > Thomas > > > > > > > > > > > > > Please CC me on replies. > > > > > > > > > Wow this really goes to show how wide our feature matrix is. > > > > > > Yes it's probably an unintended side effect but yes it > > > did work it seems, so we really should not just break it > > > without warning. > > > > > > > > > Probably this one: > > > > > > commit 831734cce6494032e9233caff4d8442b3a1e7fef > > > Author: Markus Armbruster <armbru@redhat.com> > > > Date: Wed Nov 25 11:02:20 2020 +0100 > > > > > > net: Fix handling of id in netdev_add and netdev_del > > > > > > > > > > > > Jason, what is your take here? > > > > I might be wrong, but I agree with Thomas. Adding a netdev with the > > same ID looks wrong, if it works, it looks like a bug. > > It certainly calls for trouble as soon as you try to delete the netdev again > - does it delete the first (inactive) instance? Does it delete the second > active one? Does it delete both? (Otherwise it will leave a dangling > instance behind) ... > So if changing netdev parameters on the fly is something that we want, we > should implement this properly instead indeed, and not via such an > accidental bug. > > Thomas Alexander, could you supply a reporoducer so we can check in which QEMU versions it worked? If it worked for a long time, then even if it was a result of a bug it's an accidental ABI and we should not just break it. -- MST ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: modify NetdevUserOptions through QMP in QEMU 6 - how? 2021-12-15 7:03 ` Thomas Huth 2021-12-15 7:19 ` Michael S. Tsirkin @ 2021-12-15 7:21 ` Michael S. Tsirkin 1 sibling, 0 replies; 8+ messages in thread From: Michael S. Tsirkin @ 2021-12-15 7:21 UTC (permalink / raw) To: Thomas Huth Cc: Alexander Sosedkin, qemu-devel, Jason Wang, Markus Armbruster, qemu-discuss, Samuel Thibault, Marc-André Lureau On Wed, Dec 15, 2021 at 08:03:50AM +0100, Thomas Huth wrote: > So if changing netdev parameters on the fly is something that we want, we > should implement this properly instead indeed, and not via such an > accidental bug. How to do it is a separate thing, users don't really care at all. -- MST ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: modify NetdevUserOptions through QMP in QEMU 6 - how? 2021-12-14 14:53 ` modify NetdevUserOptions through QMP in QEMU 6 - how? Michael S. Tsirkin 2021-12-15 3:31 ` Jason Wang @ 2021-12-15 13:38 ` Alexander Sosedkin 1 sibling, 0 replies; 8+ messages in thread From: Alexander Sosedkin @ 2021-12-15 13:38 UTC (permalink / raw) To: Michael S. Tsirkin Cc: Thomas Huth, qemu-devel, Jason Wang, Markus Armbruster, qemu-discuss, Samuel Thibault, Marc-André Lureau [-- Attachment #1: Type: text/plain, Size: 4906 bytes --] On Tue, Dec 14, 2021 at 3:53 PM Michael S. Tsirkin <mst@redhat.com> wrote: > On Mon, Dec 13, 2021 at 09:02:15AM +0100, Thomas Huth wrote: > > On 10/12/2021 18.02, Alexander Sosedkin wrote: > > > With QEMU 5 I could totally issue a QMP netdev_add > > > with the same ID to adjust the NetdevUserOptions I want, > > > such as restrict or hostfwd. No deleting needed, > > > just a netdev_add with what I want changed as a param. > > > > I'm a little bit surprised that this worked, since AFAIK there is no code in > > QEMU to *change* the parameters of a running netdev... likely the code added > > a new netdev with the same ID, replacing the old one? > > > > > With QEMU 6 it started failing, claiming the ID is already used. > > > And if I do netdev_del + netdev_add, I just lose connectivity. > > > What's even stranger, I still see old netdev attached in info network: > > > > > > > netdev_del {'id': 'net0'} > > > {} > > > > human-monitor-command {'command-line': 'info network'} > > > virtio-net-pci.0: > > > index=0,type=nic,model=virtio-net-pci,macaddr=52:54:00:12:34:56 > > > \ net0: index=0,type=user,net=10.0.2.0,restrict=off > > > > I think that's "normal" - there used to be problems in the past that the > > devices (virtio-net-pci in this case) did not like the netdevs to be removed > > on the fly. So the netdevs are kept around until you remove the device, too > > (i.e. issue a device_del for the virtio-net-pci device). > > > > > > netdev_add {'type': 'user', 'id': 'net0', 'restrict': False, 'hostfwd': [{'str': 'tcp:127.0.0.1:58239-:22'}]} > > > {} > > > > human-monitor-command {'command-line': 'info network'} > > > unseal: virtio-net-pci.0: > > > index=0,type=nic,model=virtio-net-pci,macaddr=52:54:00:12:34:56 > > > \ net0: index=0,type=user,net=10.0.2.0,restrict=off > > > net0: index=0,type=user,net=10.0.2.0,restrict=off > > > > > > What's the correct QMP command sequence to modify NetdevUserOptions? > > > > AFAIK there is no way to modify running netdevs - you'd have to delete the > > netdev and the device, and then add both again. But I might have missed > > something here, so I CC:-ed some people who might be more familiar with the > > details here. > > > > Thomas > > > > > > > Please CC me on replies. > > > Wow this really goes to show how wide our feature matrix is. For real. I consider myself an amateur QEMU abuser and I can use the tiny corner I've worked with as a blanket. > Yes it's probably an unintended side effect but yes it > did work it seems, so we really should not just break it > without warning. > > > Probably this one: > > commit 831734cce6494032e9233caff4d8442b3a1e7fef > Author: Markus Armbruster <armbru@redhat.com> > Date: Wed Nov 25 11:02:20 2020 +0100 > > net: Fix handling of id in netdev_add and netdev_del > > > > Jason, what is your take here? > > > Alexander, what happens if we just drop the duplicate ID check? Do > things work for you again? > Warning: completely untested. > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > > diff --git a/net/net.c b/net/net.c > index f0d14dbfc1..01f5a187b6 100644 > --- a/net/net.c > +++ b/net/net.c > @@ -1055,12 +1055,6 @@ 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) { > -- > MST > > (in other email) > Got a reproducer for me so I can double-check? Yeah, I've sketched one out in Python, see attachments. qemu-netdev-reproducer.py sets up a user netdev with restrict=off, then netdev_adds a weird thing, unrestricting and adding a hostfwd entry. qemu-netdev.qemu5.log is the output of it reconfiguring great on Fedora 34, even though it adds a duplicate netdev judging by `info network`: \ net0: index=0,type=user,net=10.0.2.0,restrict=on \ net0: index=0,type=user,net=10.0.2.0,restrict=off qemu-netdev.qemu6.log is how it fails with duplicate ID now (vendor=nixpkgs, but I've seen same error reports from Fedora 35): {'error': {'class': 'GenericError', 'desc': "Duplicate ID 'net0'"}} qemu-netdev.qemu6-nocheck.log is the behaviour with your patch applied, behaving the same way QEMU 5 does. Hope that answers all the questions from the thread; if not, please re-raise your questions and I'll do my best to answer them timely. Just to stress it, I'm not barging in with "you broke the old behaviour, revert things". I recognize that I've been doing weird things and merely ask what's the way forward to get the features I used to have access to. Not gonna lie, I would like to see it reverted, but having a proper way to reconfigure would be even better. Thank you. [-- Attachment #2: qemu-netdev-reproducer.py --] [-- Type: text/x-python, Size: 2636 bytes --] # fluff import atexit import json import socket import subprocess import time QMP_PORT = 12345 TEST_PORT = 12346 QEMU = 'qemu-system-x86_64' ARGS = ['-nographic', '-qmp', f'tcp:127.0.0.1:{QMP_PORT},server,nowait,nodelay', '-device', 'virtio-net-pci,netdev=net0', '-netdev', 'user,id=net0,restrict=on'] p = subprocess.Popen([QEMU] + ARGS, stdout=subprocess.DEVNULL) atexit.register(p.kill) sock = socket.socket(socket.AF_INET, socket.SOCK_STREAM) sock.setsockopt(socket.IPPROTO_TCP, socket.TCP_NODELAY, 1) time.sleep(.2) try: print('QMP connecting, attempt 1/2') sock.connect(('127.0.0.1', QMP_PORT)) except ConnectionRefusedError: print('QMP connecting, attempt 2/2') time.sleep(2) sock.connect(('127.0.0.1', QMP_PORT)) print('QMP connected...') def execute(cmd, **kwargs): sock.send(json.dumps( {'execute': cmd, 'arguments': kwargs} if kwargs else {'execute': cmd} ).encode()) print('executed:', cmd, kwargs) def expect(validator_func=None): r = b'' while True: r += sock.recv(1) if r[-1] == ord('\n'): reply = json.loads(r.decode()) print(f'received: {reply}') if validator_func is not None: assert validator_func(reply) return reply execute('qmp_capabilities') expect(lambda r: 'QMP' in r) # inspect as started up execute('human-monitor-command', **{'command-line': 'info network'}) expect(lambda r: r == {'return': {}}) reply = expect(lambda r: set(r.keys()) == {'return'}) # Should be: # virtio-net-pci.0: index=0,type=nic,model=virtio-net-pci,macaddr=... # \ net0: index=0,type=user,net=10.0.2.0,restrict=on assert('restrict=on' in reply['return']) print() # modify execute('netdev_add', type='user', id='net0', restrict=False, # unrestrict hostfwd=[{'str': f'tcp:127.0.0.1:{TEST_PORT}-:1'}]) # ^ *not* a list of strings as the documentation claims # inspect modified execute('human-monitor-command', **{'command-line': 'info network'}) expect(lambda r: r == {'return': {}}) reply = expect(lambda r: set(r.keys()) == {'return'}) # And now we have two net0s: # virtio-net-pci.0: index=0,type=nic,model=virtio-net-pci,macaddr=... # \ net0: index=0,type=user,net=10.0.2.0,restrict=on # \ net0: index=0,type=user,net=10.0.2.0,restrict=off assert('restrict=on' in reply['return']) assert('restrict=off' in reply['return']) # connect to modified sock_test = socket.socket(socket.AF_INET, socket.SOCK_STREAM) time.sleep(.2) sock_test.connect(('127.0.0.1', TEST_PORT)) sock_test.close() print(f'port {TEST_PORT} accepting connections') [-- Attachment #3: qemu-netdev.qemu5.log --] [-- Type: text/x-log, Size: 929 bytes --] QMP connecting, attempt 1/2 QMP connected... executed: qmp_capabilities {} received: {'QMP': {'version': {'qemu': {'micro': 0, 'minor': 2, 'major': 5}, 'package': 'qemu-5.2.0-8.fc34'}, 'capabilities': ['oob']}} executed: human-monitor-command {'command-line': 'info network'} received: {'return': {}} received: {'return': 'virtio-net-pci.0: index=0,type=nic,model=virtio-net-pci,macaddr=52:54:00:12:34:56\r\n \\ net0: index=0,type=user,net=10.0.2.0,restrict=on\r\n'} executed: netdev_add {'type': 'user', 'id': 'net0', 'restrict': False, 'hostfwd': [{'str': 'tcp:127.0.0.1:12346-:1'}]} executed: human-monitor-command {'command-line': 'info network'} received: {'return': {}} received: {'return': 'virtio-net-pci.0: index=0,type=nic,model=virtio-net-pci,macaddr=52:54:00:12:34:56\r\n \\ net0: index=0,type=user,net=10.0.2.0,restrict=on\r\nnet0: index=0,type=user,net=10.0.2.0,restrict=off\r\n'} port 12346 accepting connections [-- Attachment #4: qemu-netdev.qemu6.log --] [-- Type: text/x-log, Size: 1004 bytes --] QMP connecting, attempt 1/2 QMP connected... executed: qmp_capabilities {} received: {'QMP': {'version': {'qemu': {'micro': 0, 'minor': 1, 'major': 6}, 'package': ''}, 'capabilities': ['oob']}} executed: human-monitor-command {'command-line': 'info network'} received: {'return': {}} received: {'return': 'virtio-net-pci.0: index=0,type=nic,model=virtio-net-pci,macaddr=52:54:00:12:34:56\r\n \\ net0: index=0,type=user,net=10.0.2.0,restrict=on\r\n'} executed: netdev_add {'type': 'user', 'id': 'net0', 'restrict': False, 'hostfwd': [{'str': 'tcp:127.0.0.1:12346-:1'}]} executed: human-monitor-command {'command-line': 'info network'} received: {'error': {'class': 'GenericError', 'desc': "Duplicate ID 'net0'"}} Traceback (most recent call last): File "/home/asosedki/reproducers/qemu-netdev-reproducer.py", line 79, in <module> expect(lambda r: r == {'return': {}}) File "/home/asosedki/reproducers/qemu-netdev-reproducer.py", line 48, in expect assert validator_func(reply) AssertionError [-- Attachment #5: qemu-netdev.qemu6-nocheck.log --] [-- Type: text/x-log, Size: 912 bytes --] QMP connecting, attempt 1/2 QMP connected... executed: qmp_capabilities {} received: {'QMP': {'version': {'qemu': {'micro': 0, 'minor': 1, 'major': 6}, 'package': ''}, 'capabilities': ['oob']}} executed: human-monitor-command {'command-line': 'info network'} received: {'return': {}} received: {'return': 'virtio-net-pci.0: index=0,type=nic,model=virtio-net-pci,macaddr=52:54:00:12:34:56\r\n \\ net0: index=0,type=user,net=10.0.2.0,restrict=on\r\n'} executed: netdev_add {'type': 'user', 'id': 'net0', 'restrict': False, 'hostfwd': [{'str': 'tcp:127.0.0.1:12346-:1'}]} executed: human-monitor-command {'command-line': 'info network'} received: {'return': {}} received: {'return': 'virtio-net-pci.0: index=0,type=nic,model=virtio-net-pci,macaddr=52:54:00:12:34:56\r\n \\ net0: index=0,type=user,net=10.0.2.0,restrict=on\r\nnet0: index=0,type=user,net=10.0.2.0,restrict=off\r\n'} port 12346 accepting connections ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2021-12-15 14:36 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <CABMV8QOQzLRjm1bMTPz66FXOWaO7kYiZOG1G3ZmLHnznxVv1Yg@mail.gmail.com> [not found] ` <007f7313-eeb2-ee6a-ae2e-9341324388c0@redhat.com> 2021-12-14 14:53 ` modify NetdevUserOptions through QMP in QEMU 6 - how? Michael S. Tsirkin 2021-12-15 3:31 ` Jason Wang 2021-12-15 6:48 ` Markus Armbruster 2021-12-15 7:22 ` Michael S. Tsirkin 2021-12-15 7:03 ` Thomas Huth 2021-12-15 7:19 ` Michael S. Tsirkin 2021-12-15 7:21 ` Michael S. Tsirkin 2021-12-15 13:38 ` Alexander Sosedkin
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.