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

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

[Michael S. Tsirkin]

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

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
>

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.

[...]

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

[Michael S. Tsirkin]

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

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

[Thomas Huth]

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

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

[Michael S. Tsirkin]

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

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

[Michael S. Tsirkin]

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

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

[Alexander Sosedkin]

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