On Wed, Dec 15, 2021 at 07:48:06AM +0100, Markus Armbruster wrote:
> Jason Wang <jasow...@redhat.com> writes:
>
> > On Tue, Dec 14, 2021 at 10:53 PM Michael S. Tsirkin <m...@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 <arm...@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 <and...@daynix.com>
> Fixes: 08712fcb851034228b61f75bd922863a984a4f60
> Cc: qemu-sta...@nongnu.org
> Signed-off-by: Markus Armbruster <arm...@redhat.com>
> Reviewed-by: Eric Blake <ebl...@redhat.com>
> Signed-off-by: Jason Wang <jasow...@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.
>
> [...]
