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

[Jason Wang]

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
>