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

[Markus Armbruster <armbru@redhat.com>]

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.