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

[Markus Armbruster]

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.

[...]