[PATCH] hmp: Changed hmp_netdev_add() using qmp_marshal_netdev_add()

[PATCH] hmp: Changed hmp_netdev_add() using qmp_marshal_netdev_add()

[andrew]

From: Andrew Melnychenko <andrew@daynix.com>

There is an issue, that netdev can't be removed if it was added using hmp.
The bug appears after 08712fcb851034228b61f75bd922863a984a4f60 commit.
It happens because of unclear QemuOpts that was created during
hmp_netdev_add(), now it uses qmp analog function -
qmp_marshal_netdev_add().

Signed-off-by: Andrew Melnychenko <andrew@daynix.com>
---
 monitor/hmp-cmds.c | 15 +++------------
 1 file changed, 3 insertions(+), 12 deletions(-)

diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index 2b0b58a336..b747935687 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -1597,19 +1597,10 @@ void hmp_migrate(Monitor *mon, const QDict *qdict)
 void hmp_netdev_add(Monitor *mon, const QDict *qdict)
 {
     Error *err = NULL;
-    QemuOpts *opts;
-
-    opts = qemu_opts_from_qdict(qemu_find_opts("netdev"), qdict, &err);
-    if (err) {
-        goto out;
-    }
+    QDict *non_constant_dict = qdict_clone_shallow(qdict);
 
-    netdev_add(opts, &err);
-    if (err) {
-        qemu_opts_del(opts);
-    }
-
-out:
+    qmp_marshal_netdev_add(non_constant_dict, NULL, &err);
+    qobject_unref(non_constant_dict);
     hmp_handle_error(mon, err);
 }
 
-- 
2.27.0

Re: [PATCH] hmp: Changed hmp_netdev_add() using qmp_marshal_netdev_add()

[Andrew Melnichenko]

[-- Attachment #1: Type: text/plain, Size: 1364 bytes --]

Ping

On Thu, Jul 16, 2020 at 6:26 AM <andrew@daynix.com> wrote:

> From: Andrew Melnychenko <andrew@daynix.com>
>
> There is an issue, that netdev can't be removed if it was added using hmp.
> The bug appears after 08712fcb851034228b61f75bd922863a984a4f60 commit.
> It happens because of unclear QemuOpts that was created during
> hmp_netdev_add(), now it uses qmp analog function -
> qmp_marshal_netdev_add().
>
> Signed-off-by: Andrew Melnychenko <andrew@daynix.com>
> ---
>  monitor/hmp-cmds.c | 15 +++------------
>  1 file changed, 3 insertions(+), 12 deletions(-)
>
> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
> index 2b0b58a336..b747935687 100644
> --- a/monitor/hmp-cmds.c
> +++ b/monitor/hmp-cmds.c
> @@ -1597,19 +1597,10 @@ void hmp_migrate(Monitor *mon, const QDict *qdict)
>  void hmp_netdev_add(Monitor *mon, const QDict *qdict)
>  {
>      Error *err = NULL;
> -    QemuOpts *opts;
> -
> -    opts = qemu_opts_from_qdict(qemu_find_opts("netdev"), qdict, &err);
> -    if (err) {
> -        goto out;
> -    }
> +    QDict *non_constant_dict = qdict_clone_shallow(qdict);
>
> -    netdev_add(opts, &err);
> -    if (err) {
> -        qemu_opts_del(opts);
> -    }
> -
> -out:
> +    qmp_marshal_netdev_add(non_constant_dict, NULL, &err);
> +    qobject_unref(non_constant_dict);
>      hmp_handle_error(mon, err);
>  }
>
> --
> 2.27.0
>
>

[-- Attachment #2: Type: text/html, Size: 1966 bytes --]

Re: [PATCH] hmp: Changed hmp_netdev_add() using qmp_marshal_netdev_add()

[Markus Armbruster]

Andrew Melnichenko <andrew@daynix.com> writes:

> Ping
>
> On Thu, Jul 16, 2020 at 6:26 AM <andrew@daynix.com> wrote:
>
>> From: Andrew Melnychenko <andrew@daynix.com>
>>
>> There is an issue, that netdev can't be removed if it was added using hmp.
>> The bug appears after 08712fcb851034228b61f75bd922863a984a4f60 commit.
>> It happens because of unclear QemuOpts that was created during
>> hmp_netdev_add(), now it uses qmp analog function -
>> qmp_marshal_netdev_add().
>>
>> Signed-off-by: Andrew Melnychenko <andrew@daynix.com>
>> ---
>>  monitor/hmp-cmds.c | 15 +++------------
>>  1 file changed, 3 insertions(+), 12 deletions(-)
>>
>> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
>> index 2b0b58a336..b747935687 100644
>> --- a/monitor/hmp-cmds.c
>> +++ b/monitor/hmp-cmds.c
>> @@ -1597,19 +1597,10 @@ void hmp_migrate(Monitor *mon, const QDict *qdict)
>>  void hmp_netdev_add(Monitor *mon, const QDict *qdict)
>>  {
>>      Error *err = NULL;
>> -    QemuOpts *opts;
>> -
>> -    opts = qemu_opts_from_qdict(qemu_find_opts("netdev"), qdict, &err);
>> -    if (err) {
>> -        goto out;
>> -    }
>> +    QDict *non_constant_dict = qdict_clone_shallow(qdict);
>>
>> -    netdev_add(opts, &err);
>> -    if (err) {
>> -        qemu_opts_del(opts);
>> -    }
>> -
>> -out:
>> +    qmp_marshal_netdev_add(non_constant_dict, NULL, &err);
>> +    qobject_unref(non_constant_dict);
>>      hmp_handle_error(mon, err);
>>  }

qmp_marshal_netdev_add() uses the QObject input visitor, which feels
wrong for HMP input.

What exactly is the problem you're trying to solve?  Can you show us a
reproducer?

Re: [PATCH] hmp: Changed hmp_netdev_add() using qmp_marshal_netdev_add()

[Yuri Benditovich]

[-- Attachment #1: Type: text/plain, Size: 2040 bytes --]

On Fri, Nov 20, 2020 at 2:58 PM Markus Armbruster <armbru@redhat.com> wrote:

> Andrew Melnichenko <andrew@daynix.com> writes:
>
> > Ping
> >
> > On Thu, Jul 16, 2020 at 6:26 AM <andrew@daynix.com> wrote:
> >
> >> From: Andrew Melnychenko <andrew@daynix.com>
> >>
> >> There is an issue, that netdev can't be removed if it was added using
> hmp.
> >> The bug appears after 08712fcb851034228b61f75bd922863a984a4f60 commit.
> >> It happens because of unclear QemuOpts that was created during
> >> hmp_netdev_add(), now it uses qmp analog function -
> >> qmp_marshal_netdev_add().
> >>
> >> Signed-off-by: Andrew Melnychenko <andrew@daynix.com>
> >> ---
> >>  monitor/hmp-cmds.c | 15 +++------------
> >>  1 file changed, 3 insertions(+), 12 deletions(-)
> >>
> >> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
> >> index 2b0b58a336..b747935687 100644
> >> --- a/monitor/hmp-cmds.c
> >> +++ b/monitor/hmp-cmds.c
> >> @@ -1597,19 +1597,10 @@ void hmp_migrate(Monitor *mon, const QDict
> *qdict)
> >>  void hmp_netdev_add(Monitor *mon, const QDict *qdict)
> >>  {
> >>      Error *err = NULL;
> >> -    QemuOpts *opts;
> >> -
> >> -    opts = qemu_opts_from_qdict(qemu_find_opts("netdev"), qdict, &err);
> >> -    if (err) {
> >> -        goto out;
> >> -    }
> >> +    QDict *non_constant_dict = qdict_clone_shallow(qdict);
> >>
> >> -    netdev_add(opts, &err);
> >> -    if (err) {
> >> -        qemu_opts_del(opts);
> >> -    }
> >> -
> >> -out:
> >> +    qmp_marshal_netdev_add(non_constant_dict, NULL, &err);
> >> +    qobject_unref(non_constant_dict);
> >>      hmp_handle_error(mon, err);
> >>  }
>
> qmp_marshal_netdev_add() uses the QObject input visitor, which feels
> wrong for HMP input.
>
> What exactly is the problem you're trying to solve?  Can you show us a
> reproducer?
>
> The problem was found during work on hotplug/unplug problems with q35
run q35 VM with netdev and hotpluggable nic (virtio or e1000e)
unplug the nic (device_del)
delete the netdev ()
add netdev with the same id as before - fail (Duplicated ID)

[-- Attachment #2: Type: text/html, Size: 3119 bytes --]

Re: [PATCH] hmp: Changed hmp_netdev_add() using qmp_marshal_netdev_add()

[Yuri Benditovich]

[-- Attachment #1: Type: text/plain, Size: 2274 bytes --]

On Sat, Nov 21, 2020 at 5:24 PM Yuri Benditovich <
yuri.benditovich@daynix.com> wrote:

>
>
> On Fri, Nov 20, 2020 at 2:58 PM Markus Armbruster <armbru@redhat.com>
> wrote:
>
>> Andrew Melnichenko <andrew@daynix.com> writes:
>>
>> > Ping
>> >
>> > On Thu, Jul 16, 2020 at 6:26 AM <andrew@daynix.com> wrote:
>> >
>> >> From: Andrew Melnychenko <andrew@daynix.com>
>> >>
>> >> There is an issue, that netdev can't be removed if it was added using
>> hmp.
>> >> The bug appears after 08712fcb851034228b61f75bd922863a984a4f60 commit.
>> >> It happens because of unclear QemuOpts that was created during
>> >> hmp_netdev_add(), now it uses qmp analog function -
>> >> qmp_marshal_netdev_add().
>> >>
>> >> Signed-off-by: Andrew Melnychenko <andrew@daynix.com>
>> >> ---
>> >>  monitor/hmp-cmds.c | 15 +++------------
>> >>  1 file changed, 3 insertions(+), 12 deletions(-)
>> >>
>> >> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
>> >> index 2b0b58a336..b747935687 100644
>> >> --- a/monitor/hmp-cmds.c
>> >> +++ b/monitor/hmp-cmds.c
>> >> @@ -1597,19 +1597,10 @@ void hmp_migrate(Monitor *mon, const QDict
>> *qdict)
>> >>  void hmp_netdev_add(Monitor *mon, const QDict *qdict)
>> >>  {
>> >>      Error *err = NULL;
>> >> -    QemuOpts *opts;
>> >> -
>> >> -    opts = qemu_opts_from_qdict(qemu_find_opts("netdev"), qdict,
>> &err);
>> >> -    if (err) {
>> >> -        goto out;
>> >> -    }
>> >> +    QDict *non_constant_dict = qdict_clone_shallow(qdict);
>> >>
>> >> -    netdev_add(opts, &err);
>> >> -    if (err) {
>> >> -        qemu_opts_del(opts);
>> >> -    }
>> >> -
>> >> -out:
>> >> +    qmp_marshal_netdev_add(non_constant_dict, NULL, &err);
>> >> +    qobject_unref(non_constant_dict);
>> >>      hmp_handle_error(mon, err);
>> >>  }
>>
>> qmp_marshal_netdev_add() uses the QObject input visitor, which feels
>> wrong for HMP input.
>>
>> What exactly is the problem you're trying to solve?  Can you show us a
>> reproducer?
>>
>> The problem was found during work on hotplug/unplug problems with q35
> run q35 VM with netdev and hotpluggable nic (virtio or e1000e)
> unplug the nic (device_del)
> delete the netdev ()
> add netdev with the same id as before - fail (Duplicated ID)
>
> Q35 is not mandatory for reproduction, the same with '-machine pc'

[-- Attachment #2: Type: text/html, Size: 3634 bytes --]

Re: [PATCH] hmp: Changed hmp_netdev_add() using qmp_marshal_netdev_add()

[Andrew Melnichenko]

[-- Attachment #1: Type: text/plain, Size: 3779 bytes --]

Hi, the bug can be reproduced like that:

> QEMU 5.1.50 monitor - type 'help' for more information
> (qemu) netdev_add
> type=tap,id=net0,script=/home/and/SRCS/qemu/ifup.sh,downscript=no
> (qemu) info network
> hub 0
>  \ hub0port1: __org.qemu.net1: index=0,type=user,net=10.0.2.0,restrict=off
>  \ hub0port0: e1000e.0:
> index=0,type=nic,model=e1000e,macaddr=52:54:00:12:34:56
> dnet0: index=0,type=nic,model=virtio-net-pci,macaddr=52:54:00:12:34:57
> net0:
> index=0,type=tap,ifname=tap0,script=/home/and/SRCS/qemu/ifup.sh,downscript=no
> (qemu) netdev_del net0
> (qemu) info network
> hub 0
>  \ hub0port1: __org.qemu.net1: index=0,type=user,net=10.0.2.0,restrict=off
>  \ hub0port0: e1000e.0:
> index=0,type=nic,model=e1000e,macaddr=52:54:00:12:34:56
> dnet0: index=0,type=nic,model=virtio-net-pci,macaddr=52:54:00:12:34:57
> (qemu) netdev_add
> type=tap,id=net0,script=/home/and/SRCS/qemu/ifup.sh,downscript=no
> Try "help netdev_add" for more information
> (qemu) info network
> hub 0
>  \ hub0port1: __org.qemu.net1: index=0,type=user,net=10.0.2.0,restrict=off
>  \ hub0port0: e1000e.0:
> index=0,type=nic,model=e1000e,macaddr=52:54:00:12:34:56
> dnet0: index=0,type=nic,model=virtio-net-pci,macaddr=52:54:00:12:34:57
> (qemu)
>
>
Its still actual bug - I've checked it with the
master(2c6605389c1f76973d92b69b85d40d94b8f1092c).

On Sat, Nov 21, 2020 at 5:31 PM Yuri Benditovich <
yuri.benditovich@daynix.com> wrote:

>
>
> On Sat, Nov 21, 2020 at 5:24 PM Yuri Benditovich <
> yuri.benditovich@daynix.com> wrote:
>
>>
>>
>> On Fri, Nov 20, 2020 at 2:58 PM Markus Armbruster <armbru@redhat.com>
>> wrote:
>>
>>> Andrew Melnichenko <andrew@daynix.com> writes:
>>>
>>> > Ping
>>> >
>>> > On Thu, Jul 16, 2020 at 6:26 AM <andrew@daynix.com> wrote:
>>> >
>>> >> From: Andrew Melnychenko <andrew@daynix.com>
>>> >>
>>> >> There is an issue, that netdev can't be removed if it was added using
>>> hmp.
>>> >> The bug appears after 08712fcb851034228b61f75bd922863a984a4f60 commit.
>>> >> It happens because of unclear QemuOpts that was created during
>>> >> hmp_netdev_add(), now it uses qmp analog function -
>>> >> qmp_marshal_netdev_add().
>>> >>
>>> >> Signed-off-by: Andrew Melnychenko <andrew@daynix.com>
>>> >> ---
>>> >>  monitor/hmp-cmds.c | 15 +++------------
>>> >>  1 file changed, 3 insertions(+), 12 deletions(-)
>>> >>
>>> >> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
>>> >> index 2b0b58a336..b747935687 100644
>>> >> --- a/monitor/hmp-cmds.c
>>> >> +++ b/monitor/hmp-cmds.c
>>> >> @@ -1597,19 +1597,10 @@ void hmp_migrate(Monitor *mon, const QDict
>>> *qdict)
>>> >>  void hmp_netdev_add(Monitor *mon, const QDict *qdict)
>>> >>  {
>>> >>      Error *err = NULL;
>>> >> -    QemuOpts *opts;
>>> >> -
>>> >> -    opts = qemu_opts_from_qdict(qemu_find_opts("netdev"), qdict,
>>> &err);
>>> >> -    if (err) {
>>> >> -        goto out;
>>> >> -    }
>>> >> +    QDict *non_constant_dict = qdict_clone_shallow(qdict);
>>> >>
>>> >> -    netdev_add(opts, &err);
>>> >> -    if (err) {
>>> >> -        qemu_opts_del(opts);
>>> >> -    }
>>> >> -
>>> >> -out:
>>> >> +    qmp_marshal_netdev_add(non_constant_dict, NULL, &err);
>>> >> +    qobject_unref(non_constant_dict);
>>> >>      hmp_handle_error(mon, err);
>>> >>  }
>>>
>>> qmp_marshal_netdev_add() uses the QObject input visitor, which feels
>>> wrong for HMP input.
>>>
>>> What exactly is the problem you're trying to solve?  Can you show us a
>>> reproducer?
>>>
>>> The problem was found during work on hotplug/unplug problems with q35
>> run q35 VM with netdev and hotpluggable nic (virtio or e1000e)
>> unplug the nic (device_del)
>> delete the netdev ()
>> add netdev with the same id as before - fail (Duplicated ID)
>>
>> Q35 is not mandatory for reproduction, the same with '-machine pc'
>
>

[-- Attachment #2: Type: text/html, Size: 5581 bytes --]

Re: [PATCH] hmp: Changed hmp_netdev_add() using qmp_marshal_netdev_add()

[Yuri Benditovich]

[-- Attachment #1: Type: text/plain, Size: 4162 bytes --]

I'm sorry for the confusion. Andrew's description of steps to reproduce the
problem is correct.
I've described another problem present in the master but not related to
this patch.

On Sun, Nov 22, 2020 at 11:45 AM Andrew Melnichenko <andrew@daynix.com>
wrote:

> Hi, the bug can be reproduced like that:
>
>> QEMU 5.1.50 monitor - type 'help' for more information
>> (qemu) netdev_add
>> type=tap,id=net0,script=/home/and/SRCS/qemu/ifup.sh,downscript=no
>> (qemu) info network
>> hub 0
>>  \ hub0port1: __org.qemu.net1: index=0,type=user,net=10.0.2.0,restrict=off
>>  \ hub0port0: e1000e.0:
>> index=0,type=nic,model=e1000e,macaddr=52:54:00:12:34:56
>> dnet0: index=0,type=nic,model=virtio-net-pci,macaddr=52:54:00:12:34:57
>> net0:
>> index=0,type=tap,ifname=tap0,script=/home/and/SRCS/qemu/ifup.sh,downscript=no
>> (qemu) netdev_del net0
>> (qemu) info network
>> hub 0
>>  \ hub0port1: __org.qemu.net1: index=0,type=user,net=10.0.2.0,restrict=off
>>  \ hub0port0: e1000e.0:
>> index=0,type=nic,model=e1000e,macaddr=52:54:00:12:34:56
>> dnet0: index=0,type=nic,model=virtio-net-pci,macaddr=52:54:00:12:34:57
>> (qemu) netdev_add
>> type=tap,id=net0,script=/home/and/SRCS/qemu/ifup.sh,downscript=no
>> Try "help netdev_add" for more information
>> (qemu) info network
>> hub 0
>>  \ hub0port1: __org.qemu.net1: index=0,type=user,net=10.0.2.0,restrict=off
>>  \ hub0port0: e1000e.0:
>> index=0,type=nic,model=e1000e,macaddr=52:54:00:12:34:56
>> dnet0: index=0,type=nic,model=virtio-net-pci,macaddr=52:54:00:12:34:57
>> (qemu)
>>
>>
> Its still actual bug - I've checked it with the
> master(2c6605389c1f76973d92b69b85d40d94b8f1092c).
>
> On Sat, Nov 21, 2020 at 5:31 PM Yuri Benditovich <
> yuri.benditovich@daynix.com> wrote:
>
>>
>>
>> On Sat, Nov 21, 2020 at 5:24 PM Yuri Benditovich <
>> yuri.benditovich@daynix.com> wrote:
>>
>>>
>>>
>>> On Fri, Nov 20, 2020 at 2:58 PM Markus Armbruster <armbru@redhat.com>
>>> wrote:
>>>
>>>> Andrew Melnichenko <andrew@daynix.com> writes:
>>>>
>>>> > Ping
>>>> >
>>>> > On Thu, Jul 16, 2020 at 6:26 AM <andrew@daynix.com> wrote:
>>>> >
>>>> >> From: Andrew Melnychenko <andrew@daynix.com>
>>>> >>
>>>> >> There is an issue, that netdev can't be removed if it was added
>>>> using hmp.
>>>> >> The bug appears after 08712fcb851034228b61f75bd922863a984a4f60
>>>> commit.
>>>> >> It happens because of unclear QemuOpts that was created during
>>>> >> hmp_netdev_add(), now it uses qmp analog function -
>>>> >> qmp_marshal_netdev_add().
>>>> >>
>>>> >> Signed-off-by: Andrew Melnychenko <andrew@daynix.com>
>>>> >> ---
>>>> >>  monitor/hmp-cmds.c | 15 +++------------
>>>> >>  1 file changed, 3 insertions(+), 12 deletions(-)
>>>> >>
>>>> >> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
>>>> >> index 2b0b58a336..b747935687 100644
>>>> >> --- a/monitor/hmp-cmds.c
>>>> >> +++ b/monitor/hmp-cmds.c
>>>> >> @@ -1597,19 +1597,10 @@ void hmp_migrate(Monitor *mon, const QDict
>>>> *qdict)
>>>> >>  void hmp_netdev_add(Monitor *mon, const QDict *qdict)
>>>> >>  {
>>>> >>      Error *err = NULL;
>>>> >> -    QemuOpts *opts;
>>>> >> -
>>>> >> -    opts = qemu_opts_from_qdict(qemu_find_opts("netdev"), qdict,
>>>> &err);
>>>> >> -    if (err) {
>>>> >> -        goto out;
>>>> >> -    }
>>>> >> +    QDict *non_constant_dict = qdict_clone_shallow(qdict);
>>>> >>
>>>> >> -    netdev_add(opts, &err);
>>>> >> -    if (err) {
>>>> >> -        qemu_opts_del(opts);
>>>> >> -    }
>>>> >> -
>>>> >> -out:
>>>> >> +    qmp_marshal_netdev_add(non_constant_dict, NULL, &err);
>>>> >> +    qobject_unref(non_constant_dict);
>>>> >>      hmp_handle_error(mon, err);
>>>> >>  }
>>>>
>>>> qmp_marshal_netdev_add() uses the QObject input visitor, which feels
>>>> wrong for HMP input.
>>>>
>>>> What exactly is the problem you're trying to solve?  Can you show us a
>>>> reproducer?
>>>>
>>>> The problem was found during work on hotplug/unplug problems with q35
>>> run q35 VM with netdev and hotpluggable nic (virtio or e1000e)
>>> unplug the nic (device_del)
>>> delete the netdev ()
>>> add netdev with the same id as before - fail (Duplicated ID)
>>>
>>> Q35 is not mandatory for reproduction, the same with '-machine pc'
>>
>>

[-- Attachment #2: Type: text/html, Size: 6157 bytes --]

Re: [PATCH] hmp: Changed hmp_netdev_add() using qmp_marshal_netdev_add()

[Markus Armbruster]

Andrew Melnichenko <andrew@daynix.com> writes:

> --000000000000f73b2205b4aef0c5
> Content-Type: text/plain; charset="UTF-8"
>
> Hi, the bug can be reproduced like that:
>
>> QEMU 5.1.50 monitor - type 'help' for more information
>> (qemu) netdev_add
>> type=tap,id=net0,script=/home/and/SRCS/qemu/ifup.sh,downscript=no
>> (qemu) info network
>> hub 0
>>  \ hub0port1: __org.qemu.net1: index=0,type=user,net=10.0.2.0,restrict=off
>>  \ hub0port0: e1000e.0:
>> index=0,type=nic,model=e1000e,macaddr=52:54:00:12:34:56
>> dnet0: index=0,type=nic,model=virtio-net-pci,macaddr=52:54:00:12:34:57
>> net0:
>> index=0,type=tap,ifname=tap0,script=/home/and/SRCS/qemu/ifup.sh,downscript=no
>> (qemu) netdev_del net0
>> (qemu) info network
>> hub 0
>>  \ hub0port1: __org.qemu.net1: index=0,type=user,net=10.0.2.0,restrict=off
>>  \ hub0port0: e1000e.0:
>> index=0,type=nic,model=e1000e,macaddr=52:54:00:12:34:56
>> dnet0: index=0,type=nic,model=virtio-net-pci,macaddr=52:54:00:12:34:57
>> (qemu) netdev_add
>> type=tap,id=net0,script=/home/and/SRCS/qemu/ifup.sh,downscript=no
>> Try "help netdev_add" for more information
>> (qemu) info network
>> hub 0
>>  \ hub0port1: __org.qemu.net1: index=0,type=user,net=10.0.2.0,restrict=off
>>  \ hub0port0: e1000e.0:
>> index=0,type=nic,model=e1000e,macaddr=52:54:00:12:34:56
>> dnet0: index=0,type=nic,model=virtio-net-pci,macaddr=52:54:00:12:34:57
>> (qemu)
>>
>>
> Its still actual bug - I've checked it with the
> master(2c6605389c1f76973d92b69b85d40d94b8f1092c).

I can see this with an even simpler 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

The appended patch fixes it for me.  It relies on nothing using the
"netdev" QemuOpts anymore.  Eric, what do you think?


diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index a6a6684df1..8bc6b7bcc6 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -1638,9 +1638,7 @@ void hmp_netdev_add(Monitor *mon, const QDict *qdict)
     }
 
     netdev_add(opts, &err);
-    if (err) {
-        qemu_opts_del(opts);
-    }
+    qemu_opts_del(opts);
 
 out:
     hmp_handle_error(mon, err);

Re: [PATCH] hmp: Changed hmp_netdev_add() using qmp_marshal_netdev_add()

[Eric Blake]

On 11/23/20 3:25 AM, Markus Armbruster wrote:

>> Its still actual bug - I've checked it with the
>> master(2c6605389c1f76973d92b69b85d40d94b8f1092c).
> 
> I can see this with an even simpler 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
> 
> The appended patch fixes it for me.  It relies on nothing using the
> "netdev" QemuOpts anymore.  Eric, what do you think?

Makes sense to me.  My quick audit for qemu_find_opts("netdev") finds only:

monitor/hmp-cmds.c:    opts =
qemu_opts_from_qdict(qemu_find_opts("netdev"), qdict, &err);

net/net.c:    if (qemu_opts_foreach(qemu_find_opts("netdev"),

softmmu/vl.c:                if
(net_client_parse(qemu_find_opts("netdev"), optarg) == -1) {

where the latter two are related (we gather the command line opts, and
initialize net devs based on them, but never refer to those opts again),
and the first is the one you are proposing to change.


> 
> 
> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
> index a6a6684df1..8bc6b7bcc6 100644
> --- a/monitor/hmp-cmds.c
> +++ b/monitor/hmp-cmds.c
> @@ -1638,9 +1638,7 @@ void hmp_netdev_add(Monitor *mon, const QDict *qdict)
>      }
>  
>      netdev_add(opts, &err);
> -    if (err) {
> -        qemu_opts_del(opts);
> -    }
> +    qemu_opts_del(opts);
>  
>  out:
>      hmp_handle_error(mon, err);
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH] hmp: Changed hmp_netdev_add() using qmp_marshal_netdev_add()

[Yuri Benditovich]

[-- Attachment #1: Type: text/plain, Size: 2941 bytes --]

On Mon, Nov 23, 2020 at 11:25 AM Markus Armbruster <armbru@redhat.com>
wrote:

> Andrew Melnichenko <andrew@daynix.com> writes:
>
> > --000000000000f73b2205b4aef0c5
> > Content-Type: text/plain; charset="UTF-8"
> >
> > Hi, the bug can be reproduced like that:
> >
> >> QEMU 5.1.50 monitor - type 'help' for more information
> >> (qemu) netdev_add
> >> type=tap,id=net0,script=/home/and/SRCS/qemu/ifup.sh,downscript=no
> >> (qemu) info network
> >> hub 0
> >>  \ hub0port1: __org.qemu.net1:
> index=0,type=user,net=10.0.2.0,restrict=off
> >>  \ hub0port0: e1000e.0:
> >> index=0,type=nic,model=e1000e,macaddr=52:54:00:12:34:56
> >> dnet0: index=0,type=nic,model=virtio-net-pci,macaddr=52:54:00:12:34:57
> >> net0:
> >>
> index=0,type=tap,ifname=tap0,script=/home/and/SRCS/qemu/ifup.sh,downscript=no
> >> (qemu) netdev_del net0
> >> (qemu) info network
> >> hub 0
> >>  \ hub0port1: __org.qemu.net1:
> index=0,type=user,net=10.0.2.0,restrict=off
> >>  \ hub0port0: e1000e.0:
> >> index=0,type=nic,model=e1000e,macaddr=52:54:00:12:34:56
> >> dnet0: index=0,type=nic,model=virtio-net-pci,macaddr=52:54:00:12:34:57
> >> (qemu) netdev_add
> >> type=tap,id=net0,script=/home/and/SRCS/qemu/ifup.sh,downscript=no
> >> Try "help netdev_add" for more information
> >> (qemu) info network
> >> hub 0
> >>  \ hub0port1: __org.qemu.net1:
> index=0,type=user,net=10.0.2.0,restrict=off
> >>  \ hub0port0: e1000e.0:
> >> index=0,type=nic,model=e1000e,macaddr=52:54:00:12:34:56
> >> dnet0: index=0,type=nic,model=virtio-net-pci,macaddr=52:54:00:12:34:57
> >> (qemu)
> >>
> >>
> > Its still actual bug - I've checked it with the
> > master(2c6605389c1f76973d92b69b85d40d94b8f1092c).
>
> I can see this with an even simpler 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
>
> The appended patch fixes it for me.  It relies on nothing using the
> "netdev" QemuOpts anymore.  Eric, what do you think?
>
>
> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
> index a6a6684df1..8bc6b7bcc6 100644
> --- a/monitor/hmp-cmds.c
> +++ b/monitor/hmp-cmds.c
> @@ -1638,9 +1638,7 @@ void hmp_netdev_add(Monitor *mon, const QDict *qdict)
>      }
>
>      netdev_add(opts, &err);
> -    if (err) {
> -        qemu_opts_del(opts);
> -    }
> +    qemu_opts_del(opts);
>
>
Unfortunately, if I'm not mistaken, with this fix qemu will be able to
create from hmp several devices with the same id
(which is not expected).
For example:
netdev_add user,id=net0
netdev_add user,id=net0
info network lists 2 devices net0



>  out:
>      hmp_handle_error(mon, err);
>
>

[-- Attachment #2: Type: text/html, Size: 4081 bytes --]

Re: [PATCH] hmp: Changed hmp_netdev_add() using qmp_marshal_netdev_add()

[Yuri Benditovich]

[-- Attachment #1: Type: text/plain, Size: 3736 bytes --]

The patch below solves both issues: with netdev created by hmp and with
netdev created from command-line:

diff --git a/net/net.c b/net/net.c
index bcd5da4aa0..98294f24ed 100644
--- a/net/net.c
+++ b/net/net.c
@@ -1155,6 +1155,11 @@ void qmp_netdev_del(const char *id, Error **errp)
     }

     qemu_del_net_client(nc);
+
+    QemuOpts *opts = qemu_opts_find(qemu_find_opts("netdev"), id);
+    if (opts) {
+        qemu_opts_del(opts);
+    }
 }

static void netfilter_print_info(Monitor *mon, NetFilterState *nf)

Please let me know if you have any objections.
If not, I'll send it as a patch.


On Mon, Nov 23, 2020 at 5:35 PM Yuri Benditovich <
yuri.benditovich@daynix.com> wrote:

>
>
> On Mon, Nov 23, 2020 at 11:25 AM Markus Armbruster <armbru@redhat.com>
> wrote:
>
>> Andrew Melnichenko <andrew@daynix.com> writes:
>>
>> > --000000000000f73b2205b4aef0c5
>> > Content-Type: text/plain; charset="UTF-8"
>> >
>> > Hi, the bug can be reproduced like that:
>> >
>> >> QEMU 5.1.50 monitor - type 'help' for more information
>> >> (qemu) netdev_add
>> >> type=tap,id=net0,script=/home/and/SRCS/qemu/ifup.sh,downscript=no
>> >> (qemu) info network
>> >> hub 0
>> >>  \ hub0port1: __org.qemu.net1:
>> index=0,type=user,net=10.0.2.0,restrict=off
>> >>  \ hub0port0: e1000e.0:
>> >> index=0,type=nic,model=e1000e,macaddr=52:54:00:12:34:56
>> >> dnet0: index=0,type=nic,model=virtio-net-pci,macaddr=52:54:00:12:34:57
>> >> net0:
>> >>
>> index=0,type=tap,ifname=tap0,script=/home/and/SRCS/qemu/ifup.sh,downscript=no
>> >> (qemu) netdev_del net0
>> >> (qemu) info network
>> >> hub 0
>> >>  \ hub0port1: __org.qemu.net1:
>> index=0,type=user,net=10.0.2.0,restrict=off
>> >>  \ hub0port0: e1000e.0:
>> >> index=0,type=nic,model=e1000e,macaddr=52:54:00:12:34:56
>> >> dnet0: index=0,type=nic,model=virtio-net-pci,macaddr=52:54:00:12:34:57
>> >> (qemu) netdev_add
>> >> type=tap,id=net0,script=/home/and/SRCS/qemu/ifup.sh,downscript=no
>> >> Try "help netdev_add" for more information
>> >> (qemu) info network
>> >> hub 0
>> >>  \ hub0port1: __org.qemu.net1:
>> index=0,type=user,net=10.0.2.0,restrict=off
>> >>  \ hub0port0: e1000e.0:
>> >> index=0,type=nic,model=e1000e,macaddr=52:54:00:12:34:56
>> >> dnet0: index=0,type=nic,model=virtio-net-pci,macaddr=52:54:00:12:34:57
>> >> (qemu)
>> >>
>> >>
>> > Its still actual bug - I've checked it with the
>> > master(2c6605389c1f76973d92b69b85d40d94b8f1092c).
>>
>> I can see this with an even simpler 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
>>
>> The appended patch fixes it for me.  It relies on nothing using the
>> "netdev" QemuOpts anymore.  Eric, what do you think?
>>
>>
>> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
>> index a6a6684df1..8bc6b7bcc6 100644
>> --- a/monitor/hmp-cmds.c
>> +++ b/monitor/hmp-cmds.c
>> @@ -1638,9 +1638,7 @@ void hmp_netdev_add(Monitor *mon, const QDict
>> *qdict)
>>      }
>>
>>      netdev_add(opts, &err);
>> -    if (err) {
>> -        qemu_opts_del(opts);
>> -    }
>> +    qemu_opts_del(opts);
>>
>>
> Unfortunately, if I'm not mistaken, with this fix qemu will be able to
> create from hmp several devices with the same id
> (which is not expected).
> For example:
> netdev_add user,id=net0
> netdev_add user,id=net0
> info network lists 2 devices net0
>
>
>
>>  out:
>>      hmp_handle_error(mon, err);
>>
>>

[-- Attachment #2: Type: text/html, Size: 5242 bytes --]

Re: [PATCH] hmp: Changed hmp_netdev_add() using qmp_marshal_netdev_add()

[Markus Armbruster]

Yuri Benditovich <yuri.benditovich@daynix.com> writes:

> On Mon, Nov 23, 2020 at 11:25 AM Markus Armbruster <armbru@redhat.com>
> wrote:
>
>> Andrew Melnichenko <andrew@daynix.com> writes:
>>
>> > --000000000000f73b2205b4aef0c5
>> > Content-Type: text/plain; charset="UTF-8"
>> >
>> > Hi, the bug can be reproduced like that:
>> >
>> >> QEMU 5.1.50 monitor - type 'help' for more information
>> >> (qemu) netdev_add
>> >> type=tap,id=net0,script=/home/and/SRCS/qemu/ifup.sh,downscript=no
>> >> (qemu) info network
>> >> hub 0
>> >>  \ hub0port1: __org.qemu.net1:
>> index=0,type=user,net=10.0.2.0,restrict=off
>> >>  \ hub0port0: e1000e.0:
>> >> index=0,type=nic,model=e1000e,macaddr=52:54:00:12:34:56
>> >> dnet0: index=0,type=nic,model=virtio-net-pci,macaddr=52:54:00:12:34:57
>> >> net0:
>> >>
>> index=0,type=tap,ifname=tap0,script=/home/and/SRCS/qemu/ifup.sh,downscript=no
>> >> (qemu) netdev_del net0
>> >> (qemu) info network
>> >> hub 0
>> >>  \ hub0port1: __org.qemu.net1:
>> index=0,type=user,net=10.0.2.0,restrict=off
>> >>  \ hub0port0: e1000e.0:
>> >> index=0,type=nic,model=e1000e,macaddr=52:54:00:12:34:56
>> >> dnet0: index=0,type=nic,model=virtio-net-pci,macaddr=52:54:00:12:34:57
>> >> (qemu) netdev_add
>> >> type=tap,id=net0,script=/home/and/SRCS/qemu/ifup.sh,downscript=no
>> >> Try "help netdev_add" for more information
>> >> (qemu) info network
>> >> hub 0
>> >>  \ hub0port1: __org.qemu.net1:
>> index=0,type=user,net=10.0.2.0,restrict=off
>> >>  \ hub0port0: e1000e.0:
>> >> index=0,type=nic,model=e1000e,macaddr=52:54:00:12:34:56
>> >> dnet0: index=0,type=nic,model=virtio-net-pci,macaddr=52:54:00:12:34:57
>> >> (qemu)
>> >>
>> >>
>> > Its still actual bug - I've checked it with the
>> > master(2c6605389c1f76973d92b69b85d40d94b8f1092c).
>>
>> I can see this with an even simpler 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
>>
>> The appended patch fixes it for me.  It relies on nothing using the
>> "netdev" QemuOpts anymore.  Eric, what do you think?
>>
>>
>> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
>> index a6a6684df1..8bc6b7bcc6 100644
>> --- a/monitor/hmp-cmds.c
>> +++ b/monitor/hmp-cmds.c
>> @@ -1638,9 +1638,7 @@ void hmp_netdev_add(Monitor *mon, const QDict *qdict)
>>      }
>>
>>      netdev_add(opts, &err);
>> -    if (err) {
>> -        qemu_opts_del(opts);
>> -    }
>> +    qemu_opts_del(opts);
>>
>>
> Unfortunately, if I'm not mistaken, with this fix qemu will be able to
> create from hmp several devices with the same id
> (which is not expected).
> For example:
> netdev_add user,id=net0
> netdev_add user,id=net0
> info network lists 2 devices net0

This means commit 08712fcb85 "net: Track netdevs in NetClientState
rather than QemuOpt" didn't actually replace QemuOpts completely.

This affects QMP:

    $ socat "READLINE,history=$HOME/.qmp_history,prompt=QMP>" UNIX-CONNECT:$HOME/work/images/test-qmp 
    {"QMP": {"version": {"qemu": {"micro": 92, "minor": 1, "major": 5}, "package": "v5.2.0-rc2-19-gff85db769f-dirty"}, "capabilities": ["oob"]}}
    QMP>{ "execute": "qmp_capabilities", "arguments": { "enable": ["oob"] } }
    {"return": {}}
    QMP>{"execute": "netdev_add", "arguments": {"type": "user", "id":"net0"}}
    {"return": {}}
    QMP>{"execute": "netdev_add", "arguments": {"type": "user", "id":"net0"}}
    {"return": {}}

Results in two netdevs called "net0".  Needs fixing.

Re: [PATCH] hmp: Changed hmp_netdev_add() using qmp_marshal_netdev_add()

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> This means commit 08712fcb85 "net: Track netdevs in NetClientState
> rather than QemuOpt" didn't actually replace QemuOpts completely.
>
> This affects QMP:
>
>     $ socat "READLINE,history=$HOME/.qmp_history,prompt=QMP>" UNIX-CONNECT:$HOME/work/images/test-qmp 
>     {"QMP": {"version": {"qemu": {"micro": 92, "minor": 1, "major": 5}, "package": "v5.2.0-rc2-19-gff85db769f-dirty"}, "capabilities": ["oob"]}}
>     QMP>{ "execute": "qmp_capabilities", "arguments": { "enable": ["oob"] } }
>     {"return": {}}
>     QMP>{"execute": "netdev_add", "arguments": {"type": "user", "id":"net0"}}
>     {"return": {}}
>     QMP>{"execute": "netdev_add", "arguments": {"type": "user", "id":"net0"}}
>     {"return": {}}
>
> Results in two netdevs called "net0".  Needs fixing.

Here's my attempt.  If it looks good to you, I'll post it as a proper
patch.


diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index a6a6684df1..8bc6b7bcc6 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -1638,9 +1638,7 @@ void hmp_netdev_add(Monitor *mon, const QDict *qdict)
     }
 
     netdev_add(opts, &err);
-    if (err) {
-        qemu_opts_del(opts);
-    }
+    qemu_opts_del(opts);
 
 out:
     hmp_handle_error(mon, err);
diff --git a/net/net.c b/net/net.c
index 794c652282..eb743aca23 100644
--- a/net/net.c
+++ b/net/net.c
@@ -978,6 +978,7 @@ static int (* const net_client_init_fun[NET_CLIENT_DRIVER__MAX])(
 static int net_client_init1(const Netdev *netdev, bool is_netdev, Error **errp)
 {
     NetClientState *peer = NULL;
+    NetClientState *nc;
 
     if (is_netdev) {
         if (netdev->type == NET_CLIENT_DRIVER_NIC ||
@@ -1005,6 +1006,12 @@ 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) {
@@ -1015,8 +1022,6 @@ static int net_client_init1(const Netdev *netdev, bool is_netdev, Error **errp)
     }
 
     if (is_netdev) {
-        NetClientState *nc;
-
         nc = qemu_find_netdev(netdev->id);
         assert(nc);
         nc->is_netdev = true;
-- 
2.26.2

Re: [PATCH] hmp: Changed hmp_netdev_add() using qmp_marshal_netdev_add()

[Yuri Benditovich]

[-- Attachment #1: Type: text/plain, Size: 2870 bytes --]

Please confirm that this patch is intended to solve only the problem with
hmp (and disallow duplicated ids)
With it the netdev that was added from qemu's command line and was deleted
(for example by hmp) still can't be created, correct?

On Tue, Nov 24, 2020 at 12:21 PM Markus Armbruster <armbru@redhat.com>
wrote:

> Markus Armbruster <armbru@redhat.com> writes:
>
> > This means commit 08712fcb85 "net: Track netdevs in NetClientState
> > rather than QemuOpt" didn't actually replace QemuOpts completely.
> >
> > This affects QMP:
> >
> >     $ socat "READLINE,history=$HOME/.qmp_history,prompt=QMP>"
> UNIX-CONNECT:$HOME/work/images/test-qmp
> >     {"QMP": {"version": {"qemu": {"micro": 92, "minor": 1, "major": 5},
> "package": "v5.2.0-rc2-19-gff85db769f-dirty"}, "capabilities": ["oob"]}}
> >     QMP>{ "execute": "qmp_capabilities", "arguments": { "enable":
> ["oob"] } }
> >     {"return": {}}
> >     QMP>{"execute": "netdev_add", "arguments": {"type": "user",
> "id":"net0"}}
> >     {"return": {}}
> >     QMP>{"execute": "netdev_add", "arguments": {"type": "user",
> "id":"net0"}}
> >     {"return": {}}
> >
> > Results in two netdevs called "net0".  Needs fixing.
>
> Here's my attempt.  If it looks good to you, I'll post it as a proper
> patch.
>
>
> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
> index a6a6684df1..8bc6b7bcc6 100644
> --- a/monitor/hmp-cmds.c
> +++ b/monitor/hmp-cmds.c
> @@ -1638,9 +1638,7 @@ void hmp_netdev_add(Monitor *mon, const QDict *qdict)
>      }
>
>      netdev_add(opts, &err);
> -    if (err) {
> -        qemu_opts_del(opts);
> -    }
> +    qemu_opts_del(opts);
>
>  out:
>      hmp_handle_error(mon, err);
> diff --git a/net/net.c b/net/net.c
> index 794c652282..eb743aca23 100644
> --- a/net/net.c
> +++ b/net/net.c
> @@ -978,6 +978,7 @@ static int (* const
> net_client_init_fun[NET_CLIENT_DRIVER__MAX])(
>  static int net_client_init1(const Netdev *netdev, bool is_netdev, Error
> **errp)
>  {
>      NetClientState *peer = NULL;
> +    NetClientState *nc;
>
>      if (is_netdev) {
>          if (netdev->type == NET_CLIENT_DRIVER_NIC ||
> @@ -1005,6 +1006,12 @@ 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) {
> @@ -1015,8 +1022,6 @@ static int net_client_init1(const Netdev *netdev,
> bool is_netdev, Error **errp)
>      }
>
>      if (is_netdev) {
> -        NetClientState *nc;
> -
>          nc = qemu_find_netdev(netdev->id);
>          assert(nc);
>          nc->is_netdev = true;
> --
> 2.26.2
>
>

[-- Attachment #2: Type: text/html, Size: 4036 bytes --]

Re: [PATCH] hmp: Changed hmp_netdev_add() using qmp_marshal_netdev_add()

[Markus Armbruster]

Yuri Benditovich <yuri.benditovich@daynix.com> writes:

> Please confirm that this patch is intended to solve only the problem with
> hmp (and disallow duplicated ids)

The intent is to reject duplicate ID and to accept non-duplicate ID, no
matter how the device is created (CLI, HMP, QMP) or a prior instance was
deleted (HMP, QMP).

> With it the netdev that was added from qemu's command line and was deleted
> (for example by hmp) still can't be created, correct?

Yet another case; back to the drawing board...

Re: [PATCH] hmp: Changed hmp_netdev_add() using qmp_marshal_netdev_add()

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> Yuri Benditovich <yuri.benditovich@daynix.com> writes:
>
>> Please confirm that this patch is intended to solve only the problem with
>> hmp (and disallow duplicated ids)
>
> The intent is to reject duplicate ID and to accept non-duplicate ID, no
> matter how the device is created (CLI, HMP, QMP) or a prior instance was
> deleted (HMP, QMP).
>
>> With it the netdev that was added from qemu's command line and was deleted
>> (for example by hmp) still can't be created, correct?
>
> Yet another case; back to the drawing board...

Next try.  Hope this is one holds water :)


diff --git a/net/net.c b/net/net.c
index 794c652282..c1dc75fc37 100644
--- a/net/net.c
+++ b/net/net.c
@@ -978,6 +978,7 @@ static int (* const net_client_init_fun[NET_CLIENT_DRIVER__MAX])(
 static int net_client_init1(const Netdev *netdev, bool is_netdev, Error **errp)
 {
     NetClientState *peer = NULL;
+    NetClientState *nc;
 
     if (is_netdev) {
         if (netdev->type == NET_CLIENT_DRIVER_NIC ||
@@ -1005,6 +1006,12 @@ 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) {
@@ -1015,8 +1022,6 @@ static int net_client_init1(const Netdev *netdev, bool is_netdev, Error **errp)
     }
 
     if (is_netdev) {
-        NetClientState *nc;
-
         nc = qemu_find_netdev(netdev->id);
         assert(nc);
         nc->is_netdev = true;
@@ -1137,6 +1142,7 @@ void qmp_netdev_add(Netdev *netdev, Error **errp)
 void qmp_netdev_del(const char *id, Error **errp)
 {
     NetClientState *nc;
+    QemuOpts *opts;
 
     nc = qemu_find_netdev(id);
     if (!nc) {
@@ -1151,6 +1157,16 @@ void qmp_netdev_del(const char *id, Error **errp)
     }
 
     qemu_del_net_client(nc);
+
+    /*
+     * Wart: we need to delete the QemuOpts associated with netdevs
+     * created via CLI or HMP, to avoid bogus "Duplicate ID" errors in
+     * HMP netdev_add.
+     */
+    opts = qemu_opts_find(qemu_find_opts("netdev"), id);
+    if (opts) {
+        qemu_opts_del(opts);
+    }
 }
 
 static void netfilter_print_info(Monitor *mon, NetFilterState *nf)
-- 
2.26.2

Re: [PATCH] hmp: Changed hmp_netdev_add() using qmp_marshal_netdev_add()

[Yuri Benditovich]

[-- Attachment #1: Type: text/plain, Size: 2797 bytes --]

On Tue, Nov 24, 2020 at 3:36 PM Markus Armbruster <armbru@redhat.com> wrote:

> Markus Armbruster <armbru@redhat.com> writes:
>
> > Yuri Benditovich <yuri.benditovich@daynix.com> writes:
> >
> >> Please confirm that this patch is intended to solve only the problem
> with
> >> hmp (and disallow duplicated ids)
> >
> > The intent is to reject duplicate ID and to accept non-duplicate ID, no
> > matter how the device is created (CLI, HMP, QMP) or a prior instance was
> > deleted (HMP, QMP).
> >
> >> With it the netdev that was added from qemu's command line and was
> deleted
> >> (for example by hmp) still can't be created, correct?
> >
> > Yet another case; back to the drawing board...
>
> Next try.  Hope this is one holds water :)
>
>
> diff --git a/net/net.c b/net/net.c
> index 794c652282..c1dc75fc37 100644
> --- a/net/net.c
> +++ b/net/net.c
> @@ -978,6 +978,7 @@ static int (* const
> net_client_init_fun[NET_CLIENT_DRIVER__MAX])(
>  static int net_client_init1(const Netdev *netdev, bool is_netdev, Error
> **errp)
>  {
>      NetClientState *peer = NULL;
> +    NetClientState *nc;
>
>      if (is_netdev) {
>          if (netdev->type == NET_CLIENT_DRIVER_NIC ||
> @@ -1005,6 +1006,12 @@ 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) {
> @@ -1015,8 +1022,6 @@ static int net_client_init1(const Netdev *netdev,
> bool is_netdev, Error **errp)
>      }
>
>      if (is_netdev) {
> -        NetClientState *nc;
> -
>          nc = qemu_find_netdev(netdev->id);
>          assert(nc);
>          nc->is_netdev = true;
> @@ -1137,6 +1142,7 @@ void qmp_netdev_add(Netdev *netdev, Error **errp)
>  void qmp_netdev_del(const char *id, Error **errp)
>  {
>      NetClientState *nc;
> +    QemuOpts *opts;
>
>      nc = qemu_find_netdev(id);
>      if (!nc) {
> @@ -1151,6 +1157,16 @@ void qmp_netdev_del(const char *id, Error **errp)
>      }
>
>      qemu_del_net_client(nc);
> +
> +    /*
> +     * Wart: we need to delete the QemuOpts associated with netdevs
> +     * created via CLI or HMP, to avoid bogus "Duplicate ID" errors in
> +     * HMP netdev_add.
> +     */
> +    opts = qemu_opts_find(qemu_find_opts("netdev"), id);
> +    if (opts) {
> +        qemu_opts_del(opts);
> +    }
>  }
>
>
With this part there is no need to unconditionally delete the options
in hmp_netdev_add,
correct?


>  static void netfilter_print_info(Monitor *mon, NetFilterState *nf)
> --
> 2.26.2
>
>

[-- Attachment #2: Type: text/html, Size: 3952 bytes --]

Re: [PATCH] hmp: Changed hmp_netdev_add() using qmp_marshal_netdev_add()

[Markus Armbruster]

Yuri Benditovich <yuri.benditovich@daynix.com> writes:

> On Tue, Nov 24, 2020 at 3:36 PM Markus Armbruster <armbru@redhat.com> wrote:
>
>> Markus Armbruster <armbru@redhat.com> writes:
>>
>> > Yuri Benditovich <yuri.benditovich@daynix.com> writes:
>> >
>> >> Please confirm that this patch is intended to solve only the problem
>> with
>> >> hmp (and disallow duplicated ids)
>> >
>> > The intent is to reject duplicate ID and to accept non-duplicate ID, no
>> > matter how the device is created (CLI, HMP, QMP) or a prior instance was
>> > deleted (HMP, QMP).
>> >
>> >> With it the netdev that was added from qemu's command line and was
>> deleted
>> >> (for example by hmp) still can't be created, correct?
>> >
>> > Yet another case; back to the drawing board...
>>
>> Next try.  Hope this is one holds water :)
>>
>>
>> diff --git a/net/net.c b/net/net.c
>> index 794c652282..c1dc75fc37 100644
>> --- a/net/net.c
>> +++ b/net/net.c
>> @@ -978,6 +978,7 @@ static int (* const
>> net_client_init_fun[NET_CLIENT_DRIVER__MAX])(
>>  static int net_client_init1(const Netdev *netdev, bool is_netdev, Error
>> **errp)
>>  {
>>      NetClientState *peer = NULL;
>> +    NetClientState *nc;
>>
>>      if (is_netdev) {
>>          if (netdev->type == NET_CLIENT_DRIVER_NIC ||
>> @@ -1005,6 +1006,12 @@ 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) {
>> @@ -1015,8 +1022,6 @@ static int net_client_init1(const Netdev *netdev,
>> bool is_netdev, Error **errp)
>>      }
>>
>>      if (is_netdev) {
>> -        NetClientState *nc;
>> -
>>          nc = qemu_find_netdev(netdev->id);
>>          assert(nc);
>>          nc->is_netdev = true;
>> @@ -1137,6 +1142,7 @@ void qmp_netdev_add(Netdev *netdev, Error **errp)
>>  void qmp_netdev_del(const char *id, Error **errp)
>>  {
>>      NetClientState *nc;
>> +    QemuOpts *opts;
>>
>>      nc = qemu_find_netdev(id);
>>      if (!nc) {
>> @@ -1151,6 +1157,16 @@ void qmp_netdev_del(const char *id, Error **errp)
>>      }
>>
>>      qemu_del_net_client(nc);
>> +
>> +    /*
>> +     * Wart: we need to delete the QemuOpts associated with netdevs
>> +     * created via CLI or HMP, to avoid bogus "Duplicate ID" errors in
>> +     * HMP netdev_add.
>> +     */
>> +    opts = qemu_opts_find(qemu_find_opts("netdev"), id);
>> +    if (opts) {
>> +        qemu_opts_del(opts);
>> +    }
>>  }
>>
>>
> With this part there is no need to unconditionally delete the options
> in hmp_netdev_add,
> correct?

Yes.

The CLI accumulates -netdev in option group "netdev".  It has to, or
else -writeconfig doesn't work.

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 for 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 netdev_add loses its "no duplicate ID" check.

  My change to net_init_client1() fixes this.

* Both netdev_add can leave QemuOpts behind, breaking HMP netdev_add.

  My change to qmp_netdev_del() fixes this.

Questions?

Re: [PATCH] hmp: Changed hmp_netdev_add() using qmp_marshal_netdev_add()

[Yuri Benditovich]

[-- Attachment #1: Type: text/plain, Size: 3980 bytes --]

On Tue, Nov 24, 2020 at 5:46 PM Markus Armbruster <armbru@redhat.com> wrote:

> Yuri Benditovich <yuri.benditovich@daynix.com> writes:
>
> > On Tue, Nov 24, 2020 at 3:36 PM Markus Armbruster <armbru@redhat.com>
> wrote:
> >
> >> Markus Armbruster <armbru@redhat.com> writes:
> >>
> >> > Yuri Benditovich <yuri.benditovich@daynix.com> writes:
> >> >
> >> >> Please confirm that this patch is intended to solve only the problem
> >> with
> >> >> hmp (and disallow duplicated ids)
> >> >
> >> > The intent is to reject duplicate ID and to accept non-duplicate ID,
> no
> >> > matter how the device is created (CLI, HMP, QMP) or a prior instance
> was
> >> > deleted (HMP, QMP).
> >> >
> >> >> With it the netdev that was added from qemu's command line and was
> >> deleted
> >> >> (for example by hmp) still can't be created, correct?
> >> >
> >> > Yet another case; back to the drawing board...
> >>
> >> Next try.  Hope this is one holds water :)
> >>
> >>
> >> diff --git a/net/net.c b/net/net.c
> >> index 794c652282..c1dc75fc37 100644
> >> --- a/net/net.c
> >> +++ b/net/net.c
> >> @@ -978,6 +978,7 @@ static int (* const
> >> net_client_init_fun[NET_CLIENT_DRIVER__MAX])(
> >>  static int net_client_init1(const Netdev *netdev, bool is_netdev, Error
> >> **errp)
> >>  {
> >>      NetClientState *peer = NULL;
> >> +    NetClientState *nc;
> >>
> >>      if (is_netdev) {
> >>          if (netdev->type == NET_CLIENT_DRIVER_NIC ||
> >> @@ -1005,6 +1006,12 @@ 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) {
> >> @@ -1015,8 +1022,6 @@ static int net_client_init1(const Netdev *netdev,
> >> bool is_netdev, Error **errp)
> >>      }
> >>
> >>      if (is_netdev) {
> >> -        NetClientState *nc;
> >> -
> >>          nc = qemu_find_netdev(netdev->id);
> >>          assert(nc);
> >>          nc->is_netdev = true;
> >> @@ -1137,6 +1142,7 @@ void qmp_netdev_add(Netdev *netdev, Error **errp)
> >>  void qmp_netdev_del(const char *id, Error **errp)
> >>  {
> >>      NetClientState *nc;
> >> +    QemuOpts *opts;
> >>
> >>      nc = qemu_find_netdev(id);
> >>      if (!nc) {
> >> @@ -1151,6 +1157,16 @@ void qmp_netdev_del(const char *id, Error **errp)
> >>      }
> >>
> >>      qemu_del_net_client(nc);
> >> +
> >> +    /*
> >> +     * Wart: we need to delete the QemuOpts associated with netdevs
> >> +     * created via CLI or HMP, to avoid bogus "Duplicate ID" errors in
> >> +     * HMP netdev_add.
> >> +     */
> >> +    opts = qemu_opts_find(qemu_find_opts("netdev"), id);
> >> +    if (opts) {
> >> +        qemu_opts_del(opts);
> >> +    }
> >>  }
> >>
> >>
> > With this part there is no need to unconditionally delete the options
> > in hmp_netdev_add,
> > correct?
>
> Yes.
>
> The CLI accumulates -netdev in option group "netdev".  It has to, or
> else -writeconfig doesn't work.
>
> 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 for 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 netdev_add loses its "no duplicate ID" check.
>
>   My change to net_init_client1() fixes this.
>
> * Both netdev_add can leave QemuOpts behind, breaking HMP netdev_add.
>
>   My change to qmp_netdev_del() fixes this.
>
> Questions?
>
> No questions, looking forward for the final patch
Thanks

[-- Attachment #2: Type: text/html, Size: 5744 bytes --]

Re: [PATCH] hmp: Changed hmp_netdev_add() using qmp_marshal_netdev_add()

[Markus Armbruster]

Yuri Benditovich <yuri.benditovich@daynix.com> writes:

> On Tue, Nov 24, 2020 at 5:46 PM Markus Armbruster <armbru@redhat.com> wrote:
[...]
>> The CLI accumulates -netdev in option group "netdev".  It has to, or
>> else -writeconfig doesn't work.
>>
>> 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 for 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 netdev_add loses its "no duplicate ID" check.
>>
>>   My change to net_init_client1() fixes this.
>>
>> * Both netdev_add can leave QemuOpts behind, breaking HMP netdev_add.
>>
>>   My change to qmp_netdev_del() fixes this.
>>
>> Questions?
>>
> No questions, looking forward for the final patch
> Thanks

Posted:

    Subject: [PATCH 0/1] net: Fix handling of id in netdev_add and netdev_del
    Message-Id: <20201125100220.50251-1-armbru@redhat.com>

Thanks for your help, guys, I appreciate it!

Re: [PATCH] hmp: Changed hmp_netdev_add() using qmp_marshal_netdev_add()

[Eric Blake]

On 11/24/20 7:36 AM, Markus Armbruster wrote:
> Markus Armbruster <armbru@redhat.com> writes:
> 
>> Yuri Benditovich <yuri.benditovich@daynix.com> writes:
>>
>>> Please confirm that this patch is intended to solve only the problem with
>>> hmp (and disallow duplicated ids)
>>
>> The intent is to reject duplicate ID and to accept non-duplicate ID, no
>> matter how the device is created (CLI, HMP, QMP) or a prior instance was
>> deleted (HMP, QMP).
>>
>>> With it the netdev that was added from qemu's command line and was deleted
>>> (for example by hmp) still can't be created, correct?
>>
>> Yet another case; back to the drawing board...
> 
> Next try.  Hope this is one holds water :)
> 
> 
> diff --git a/net/net.c b/net/net.c
> index 794c652282..c1dc75fc37 100644
> --- a/net/net.c
> +++ b/net/net.c
> @@ -978,6 +978,7 @@ static int (* const net_client_init_fun[NET_CLIENT_DRIVER__MAX])(
>  static int net_client_init1(const Netdev *netdev, bool is_netdev, Error **errp)
>  {
>      NetClientState *peer = NULL;
> +    NetClientState *nc;
>  
>      if (is_netdev) {
>          if (netdev->type == NET_CLIENT_DRIVER_NIC ||
> @@ -1005,6 +1006,12 @@ 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;
> +    }

Here, we fail if qemu_find_netdev() succeeded, regardless of whether
is_netdev was set...

> +
>      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) {
> @@ -1015,8 +1022,6 @@ static int net_client_init1(const Netdev *netdev, bool is_netdev, Error **errp)
>      }
>  
>      if (is_netdev) {
> -        NetClientState *nc;
> -
>          nc = qemu_find_netdev(netdev->id);
>          assert(nc);

and here, when is_netdev is set, we expect qemu_find_netdev() to
succeed.  Does the first hunk need to be 'if (nc && !is_netdev)' ?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH] hmp: Changed hmp_netdev_add() using qmp_marshal_netdev_add()

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 11/24/20 7:36 AM, Markus Armbruster wrote:
>> Markus Armbruster <armbru@redhat.com> writes:
>> 
>>> Yuri Benditovich <yuri.benditovich@daynix.com> writes:
>>>
>>>> Please confirm that this patch is intended to solve only the problem with
>>>> hmp (and disallow duplicated ids)
>>>
>>> The intent is to reject duplicate ID and to accept non-duplicate ID, no
>>> matter how the device is created (CLI, HMP, QMP) or a prior instance was
>>> deleted (HMP, QMP).
>>>
>>>> With it the netdev that was added from qemu's command line and was deleted
>>>> (for example by hmp) still can't be created, correct?
>>>
>>> Yet another case; back to the drawing board...
>> 
>> Next try.  Hope this is one holds water :)
>> 
>> 
>> diff --git a/net/net.c b/net/net.c
>> index 794c652282..c1dc75fc37 100644
>> --- a/net/net.c
>> +++ b/net/net.c
>> @@ -978,6 +978,7 @@ static int (* const net_client_init_fun[NET_CLIENT_DRIVER__MAX])(
>>  static int net_client_init1(const Netdev *netdev, bool is_netdev, Error **errp)
>>  {
>>      NetClientState *peer = NULL;
>> +    NetClientState *nc;
>>  
>>      if (is_netdev) {
>>          if (netdev->type == NET_CLIENT_DRIVER_NIC ||
>> @@ -1005,6 +1006,12 @@ 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;
>> +    }
>
> Here, we fail if qemu_find_netdev() succeeded, regardless of whether
> is_netdev was set...
>
>> +
>>      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) {
>> @@ -1015,8 +1022,6 @@ static int net_client_init1(const Netdev *netdev, bool is_netdev, Error **errp)
>>      }
>>  
>>      if (is_netdev) {
>> -        NetClientState *nc;
>> -
>>          nc = qemu_find_netdev(netdev->id);
>>          assert(nc);
>
> and here, when is_netdev is set, we expect qemu_find_netdev() to
> succeed.  Does the first hunk need to be 'if (nc && !is_netdev)' ?

My head hurts.

I suspect splitting the function into one function for is_netdev=false
and another one for is_netdev=true would result in more readable code.
Same for net_client_init().

That said, a duplicate ID is to be rejected regardless of is_netdev,
isn't it?

Let's examine how we can get here with is_netdev=false.

Callers:

* net_client_init(): passes its own is_netdev argumet

  Callers:

  - netdev_add(): passes true

  - net_init_client(): passes false

    Caller: net_init_clients() for each -net.  The IDs are all distinct.
    The error check I add in this patch redundant in this case.  It is
    not wrong.

  - net_init_netdev(): passes true

  - net_param_nic(): passes true

* qmp_netdev_add(): passes true

Okay?