Re: modify NetdevUserOptions through QMP in QEMU 6 - how?

["Michael S. Tsirkin" <mst@redhat.com>]

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.
> 
> [...]