| Message ID | 20200716035532.1407660-1-andrew@daynix.com |
|---|---|
| State | New |
| Headers | show |
| Series | hmp: Changed hmp_netdev_add() using qmp_marshal_netdev_add() | expand |
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 > > <div dir="ltr">Ping<br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Jul 16, 2020 at 6:26 AM <<a href="mailto:andrew@daynix.com">andrew@daynix.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">From: Andrew Melnychenko <<a href="mailto:andrew@daynix.com" target="_blank">andrew@daynix.com</a>><br> <br> There is an issue, that netdev can't be removed if it was added using hmp.<br> The bug appears after 08712fcb851034228b61f75bd922863a984a4f60 commit.<br> It happens because of unclear QemuOpts that was created during<br> hmp_netdev_add(), now it uses qmp analog function -<br> qmp_marshal_netdev_add().<br> <br> Signed-off-by: Andrew Melnychenko <<a href="mailto:andrew@daynix.com" target="_blank">andrew@daynix.com</a>><br> ---<br> monitor/hmp-cmds.c | 15 +++------------<br> 1 file changed, 3 insertions(+), 12 deletions(-)<br> <br> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c<br> index 2b0b58a336..b747935687 100644<br> --- a/monitor/hmp-cmds.c<br> +++ b/monitor/hmp-cmds.c<br> @@ -1597,19 +1597,10 @@ void hmp_migrate(Monitor *mon, const QDict *qdict)<br> void hmp_netdev_add(Monitor *mon, const QDict *qdict)<br> {<br> Error *err = NULL;<br> - QemuOpts *opts;<br> -<br> - opts = qemu_opts_from_qdict(qemu_find_opts("netdev"), qdict, &err);<br> - if (err) {<br> - goto out;<br> - }<br> + QDict *non_constant_dict = qdict_clone_shallow(qdict);<br> <br> - netdev_add(opts, &err);<br> - if (err) {<br> - qemu_opts_del(opts);<br> - }<br> -<br> -out:<br> + qmp_marshal_netdev_add(non_constant_dict, NULL, &err);<br> + qobject_unref(non_constant_dict);<br> hmp_handle_error(mon, err);<br> }<br> <br> -- <br> 2.27.0<br> <br> </blockquote></div>
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?
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) <div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, Nov 20, 2020 at 2:58 PM Markus Armbruster <<a href="mailto:armbru@redhat.com">armbru@redhat.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Andrew Melnichenko <<a href="mailto:andrew@daynix.com" target="_blank">andrew@daynix.com</a>> writes:<br> <br> > Ping<br> ><br> > On Thu, Jul 16, 2020 at 6:26 AM <<a href="mailto:andrew@daynix.com" target="_blank">andrew@daynix.com</a>> wrote:<br> ><br> >> From: Andrew Melnychenko <<a href="mailto:andrew@daynix.com" target="_blank">andrew@daynix.com</a>><br> >><br> >> There is an issue, that netdev can't be removed if it was added using hmp.<br> >> The bug appears after 08712fcb851034228b61f75bd922863a984a4f60 commit.<br> >> It happens because of unclear QemuOpts that was created during<br> >> hmp_netdev_add(), now it uses qmp analog function -<br> >> qmp_marshal_netdev_add().<br> >><br> >> Signed-off-by: Andrew Melnychenko <<a href="mailto:andrew@daynix.com" target="_blank">andrew@daynix.com</a>><br> >> ---<br> >> monitor/hmp-cmds.c | 15 +++------------<br> >> 1 file changed, 3 insertions(+), 12 deletions(-)<br> >><br> >> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c<br> >> index 2b0b58a336..b747935687 100644<br> >> --- a/monitor/hmp-cmds.c<br> >> +++ b/monitor/hmp-cmds.c<br> >> @@ -1597,19 +1597,10 @@ void hmp_migrate(Monitor *mon, const QDict *qdict)<br> >> void hmp_netdev_add(Monitor *mon, const QDict *qdict)<br> >> {<br> >> Error *err = NULL;<br> >> - QemuOpts *opts;<br> >> -<br> >> - opts = qemu_opts_from_qdict(qemu_find_opts("netdev"), qdict, &err);<br> >> - if (err) {<br> >> - goto out;<br> >> - }<br> >> + QDict *non_constant_dict = qdict_clone_shallow(qdict);<br> >><br> >> - netdev_add(opts, &err);<br> >> - if (err) {<br> >> - qemu_opts_del(opts);<br> >> - }<br> >> -<br> >> -out:<br> >> + qmp_marshal_netdev_add(non_constant_dict, NULL, &err);<br> >> + qobject_unref(non_constant_dict);<br> >> hmp_handle_error(mon, err);<br> >> }<br> <br> qmp_marshal_netdev_add() uses the QObject input visitor, which feels<br> wrong for HMP input.<br> <br> What exactly is the problem you're trying to solve? Can you show us a<br> reproducer?<br><br></blockquote><div>The problem was found during work on hotplug/unplug problems with q35</div><div>run q35 VM with netdev and hotpluggable nic (virtio or e1000e)</div><div>unplug the nic (device_del)</div><div>delete the netdev ()</div><div>add netdev with the same id as before - fail (Duplicated ID)</div><div><br></div></div></div>
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' <div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Sat, Nov 21, 2020 at 5:24 PM Yuri Benditovich <<a href="mailto:yuri.benditovich@daynix.com">yuri.benditovich@daynix.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, Nov 20, 2020 at 2:58 PM Markus Armbruster <<a href="mailto:armbru@redhat.com" target="_blank">armbru@redhat.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Andrew Melnichenko <<a href="mailto:andrew@daynix.com" target="_blank">andrew@daynix.com</a>> writes:<br> <br> > Ping<br> ><br> > On Thu, Jul 16, 2020 at 6:26 AM <<a href="mailto:andrew@daynix.com" target="_blank">andrew@daynix.com</a>> wrote:<br> ><br> >> From: Andrew Melnychenko <<a href="mailto:andrew@daynix.com" target="_blank">andrew@daynix.com</a>><br> >><br> >> There is an issue, that netdev can't be removed if it was added using hmp.<br> >> The bug appears after 08712fcb851034228b61f75bd922863a984a4f60 commit.<br> >> It happens because of unclear QemuOpts that was created during<br> >> hmp_netdev_add(), now it uses qmp analog function -<br> >> qmp_marshal_netdev_add().<br> >><br> >> Signed-off-by: Andrew Melnychenko <<a href="mailto:andrew@daynix.com" target="_blank">andrew@daynix.com</a>><br> >> ---<br> >> monitor/hmp-cmds.c | 15 +++------------<br> >> 1 file changed, 3 insertions(+), 12 deletions(-)<br> >><br> >> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c<br> >> index 2b0b58a336..b747935687 100644<br> >> --- a/monitor/hmp-cmds.c<br> >> +++ b/monitor/hmp-cmds.c<br> >> @@ -1597,19 +1597,10 @@ void hmp_migrate(Monitor *mon, const QDict *qdict)<br> >> void hmp_netdev_add(Monitor *mon, const QDict *qdict)<br> >> {<br> >> Error *err = NULL;<br> >> - QemuOpts *opts;<br> >> -<br> >> - opts = qemu_opts_from_qdict(qemu_find_opts("netdev"), qdict, &err);<br> >> - if (err) {<br> >> - goto out;<br> >> - }<br> >> + QDict *non_constant_dict = qdict_clone_shallow(qdict);<br> >><br> >> - netdev_add(opts, &err);<br> >> - if (err) {<br> >> - qemu_opts_del(opts);<br> >> - }<br> >> -<br> >> -out:<br> >> + qmp_marshal_netdev_add(non_constant_dict, NULL, &err);<br> >> + qobject_unref(non_constant_dict);<br> >> hmp_handle_error(mon, err);<br> >> }<br> <br> qmp_marshal_netdev_add() uses the QObject input visitor, which feels<br> wrong for HMP input.<br> <br> What exactly is the problem you're trying to solve? Can you show us a<br> reproducer?<br><br></blockquote><div>The problem was found during work on hotplug/unplug problems with q35</div><div>run q35 VM with netdev and hotpluggable nic (virtio or e1000e)</div><div>unplug the nic (device_del)</div><div>delete the netdev ()</div><div>add netdev with the same id as before - fail (Duplicated ID)</div><div><br></div></div></div></blockquote><div>Q35 is not mandatory for reproduction, the same with '-machine pc'</div><div><br></div></div></div>
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' > > <div dir="ltr"><div>Hi, the bug can be reproduced like that:</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div>QEMU 5.1.50 monitor - type 'help' for more information<br>(qemu) netdev_add type=tap,id=net0,script=/home/and/SRCS/qemu/ifup.sh,downscript=no<br>(qemu) info network<br>hub 0<br> \ hub0port1: __org.qemu.net1: index=0,type=user,net=10.0.2.0,restrict=off<br> \ hub0port0: e1000e.0: index=0,type=nic,model=e1000e,macaddr=52:54:00:12:34:56<br>dnet0: index=0,type=nic,model=virtio-net-pci,macaddr=52:54:00:12:34:57<br>net0: index=0,type=tap,ifname=tap0,script=/home/and/SRCS/qemu/ifup.sh,downscript=no<br>(qemu) netdev_del net0<br>(qemu) info network<br>hub 0<br> \ hub0port1: __org.qemu.net1: index=0,type=user,net=10.0.2.0,restrict=off<br> \ hub0port0: e1000e.0: index=0,type=nic,model=e1000e,macaddr=52:54:00:12:34:56<br>dnet0: index=0,type=nic,model=virtio-net-pci,macaddr=52:54:00:12:34:57<br>(qemu) netdev_add type=tap,id=net0,script=/home/and/SRCS/qemu/ifup.sh,downscript=no<br>Try "help netdev_add" for more information<br>(qemu) info network<br>hub 0<br> \ hub0port1: __org.qemu.net1: index=0,type=user,net=10.0.2.0,restrict=off<br> \ hub0port0: e1000e.0: index=0,type=nic,model=e1000e,macaddr=52:54:00:12:34:56<br>dnet0: index=0,type=nic,model=virtio-net-pci,macaddr=52:54:00:12:34:57<br>(qemu) <br><br></div></blockquote><div><br></div><div>Its still actual bug - I've checked it with the master(2c6605389c1f76973d92b69b85d40d94b8f1092c). </div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Sat, Nov 21, 2020 at 5:31 PM Yuri Benditovich <<a href="mailto:yuri.benditovich@daynix.com">yuri.benditovich@daynix.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Sat, Nov 21, 2020 at 5:24 PM Yuri Benditovich <<a href="mailto:yuri.benditovich@daynix.com" target="_blank">yuri.benditovich@daynix.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, Nov 20, 2020 at 2:58 PM Markus Armbruster <<a href="mailto:armbru@redhat.com" target="_blank">armbru@redhat.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Andrew Melnichenko <<a href="mailto:andrew@daynix.com" target="_blank">andrew@daynix.com</a>> writes:<br> <br> > Ping<br> ><br> > On Thu, Jul 16, 2020 at 6:26 AM <<a href="mailto:andrew@daynix.com" target="_blank">andrew@daynix.com</a>> wrote:<br> ><br> >> From: Andrew Melnychenko <<a href="mailto:andrew@daynix.com" target="_blank">andrew@daynix.com</a>><br> >><br> >> There is an issue, that netdev can't be removed if it was added using hmp.<br> >> The bug appears after 08712fcb851034228b61f75bd922863a984a4f60 commit.<br> >> It happens because of unclear QemuOpts that was created during<br> >> hmp_netdev_add(), now it uses qmp analog function -<br> >> qmp_marshal_netdev_add().<br> >><br> >> Signed-off-by: Andrew Melnychenko <<a href="mailto:andrew@daynix.com" target="_blank">andrew@daynix.com</a>><br> >> ---<br> >> monitor/hmp-cmds.c | 15 +++------------<br> >> 1 file changed, 3 insertions(+), 12 deletions(-)<br> >><br> >> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c<br> >> index 2b0b58a336..b747935687 100644<br> >> --- a/monitor/hmp-cmds.c<br> >> +++ b/monitor/hmp-cmds.c<br> >> @@ -1597,19 +1597,10 @@ void hmp_migrate(Monitor *mon, const QDict *qdict)<br> >> void hmp_netdev_add(Monitor *mon, const QDict *qdict)<br> >> {<br> >> Error *err = NULL;<br> >> - QemuOpts *opts;<br> >> -<br> >> - opts = qemu_opts_from_qdict(qemu_find_opts("netdev"), qdict, &err);<br> >> - if (err) {<br> >> - goto out;<br> >> - }<br> >> + QDict *non_constant_dict = qdict_clone_shallow(qdict);<br> >><br> >> - netdev_add(opts, &err);<br> >> - if (err) {<br> >> - qemu_opts_del(opts);<br> >> - }<br> >> -<br> >> -out:<br> >> + qmp_marshal_netdev_add(non_constant_dict, NULL, &err);<br> >> + qobject_unref(non_constant_dict);<br> >> hmp_handle_error(mon, err);<br> >> }<br> <br> qmp_marshal_netdev_add() uses the QObject input visitor, which feels<br> wrong for HMP input.<br> <br> What exactly is the problem you're trying to solve? Can you show us a<br> reproducer?<br><br></blockquote><div>The problem was found during work on hotplug/unplug problems with q35</div><div>run q35 VM with netdev and hotpluggable nic (virtio or e1000e)</div><div>unplug the nic (device_del)</div><div>delete the netdev ()</div><div>add netdev with the same id as before - fail (Duplicated ID)</div><div><br></div></div></div></blockquote><div>Q35 is not mandatory for reproduction, the same with '-machine pc'</div><div><br></div></div></div> </blockquote></div>
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' >> >> <div dir="ltr">I'm sorry for the confusion. Andrew's description of steps to reproduce the problem is correct.<div>I've described another problem present in the master but not related to this patch.</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Sun, Nov 22, 2020 at 11:45 AM Andrew Melnichenko <<a href="mailto:andrew@daynix.com">andrew@daynix.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div>Hi, the bug can be reproduced like that:</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div>QEMU 5.1.50 monitor - type 'help' for more information<br>(qemu) netdev_add type=tap,id=net0,script=/home/and/SRCS/qemu/ifup.sh,downscript=no<br>(qemu) info network<br>hub 0<br> \ hub0port1: __org.qemu.net1: index=0,type=user,net=10.0.2.0,restrict=off<br> \ hub0port0: e1000e.0: index=0,type=nic,model=e1000e,macaddr=52:54:00:12:34:56<br>dnet0: index=0,type=nic,model=virtio-net-pci,macaddr=52:54:00:12:34:57<br>net0: index=0,type=tap,ifname=tap0,script=/home/and/SRCS/qemu/ifup.sh,downscript=no<br>(qemu) netdev_del net0<br>(qemu) info network<br>hub 0<br> \ hub0port1: __org.qemu.net1: index=0,type=user,net=10.0.2.0,restrict=off<br> \ hub0port0: e1000e.0: index=0,type=nic,model=e1000e,macaddr=52:54:00:12:34:56<br>dnet0: index=0,type=nic,model=virtio-net-pci,macaddr=52:54:00:12:34:57<br>(qemu) netdev_add type=tap,id=net0,script=/home/and/SRCS/qemu/ifup.sh,downscript=no<br>Try "help netdev_add" for more information<br>(qemu) info network<br>hub 0<br> \ hub0port1: __org.qemu.net1: index=0,type=user,net=10.0.2.0,restrict=off<br> \ hub0port0: e1000e.0: index=0,type=nic,model=e1000e,macaddr=52:54:00:12:34:56<br>dnet0: index=0,type=nic,model=virtio-net-pci,macaddr=52:54:00:12:34:57<br>(qemu) <br><br></div></blockquote><div><br></div><div>Its still actual bug - I've checked it with the master(2c6605389c1f76973d92b69b85d40d94b8f1092c). </div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Sat, Nov 21, 2020 at 5:31 PM Yuri Benditovich <<a href="mailto:yuri.benditovich@daynix.com" target="_blank">yuri.benditovich@daynix.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Sat, Nov 21, 2020 at 5:24 PM Yuri Benditovich <<a href="mailto:yuri.benditovich@daynix.com" target="_blank">yuri.benditovich@daynix.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, Nov 20, 2020 at 2:58 PM Markus Armbruster <<a href="mailto:armbru@redhat.com" target="_blank">armbru@redhat.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Andrew Melnichenko <<a href="mailto:andrew@daynix.com" target="_blank">andrew@daynix.com</a>> writes:<br> <br> > Ping<br> ><br> > On Thu, Jul 16, 2020 at 6:26 AM <<a href="mailto:andrew@daynix.com" target="_blank">andrew@daynix.com</a>> wrote:<br> ><br> >> From: Andrew Melnychenko <<a href="mailto:andrew@daynix.com" target="_blank">andrew@daynix.com</a>><br> >><br> >> There is an issue, that netdev can't be removed if it was added using hmp.<br> >> The bug appears after 08712fcb851034228b61f75bd922863a984a4f60 commit.<br> >> It happens because of unclear QemuOpts that was created during<br> >> hmp_netdev_add(), now it uses qmp analog function -<br> >> qmp_marshal_netdev_add().<br> >><br> >> Signed-off-by: Andrew Melnychenko <<a href="mailto:andrew@daynix.com" target="_blank">andrew@daynix.com</a>><br> >> ---<br> >> monitor/hmp-cmds.c | 15 +++------------<br> >> 1 file changed, 3 insertions(+), 12 deletions(-)<br> >><br> >> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c<br> >> index 2b0b58a336..b747935687 100644<br> >> --- a/monitor/hmp-cmds.c<br> >> +++ b/monitor/hmp-cmds.c<br> >> @@ -1597,19 +1597,10 @@ void hmp_migrate(Monitor *mon, const QDict *qdict)<br> >> void hmp_netdev_add(Monitor *mon, const QDict *qdict)<br> >> {<br> >> Error *err = NULL;<br> >> - QemuOpts *opts;<br> >> -<br> >> - opts = qemu_opts_from_qdict(qemu_find_opts("netdev"), qdict, &err);<br> >> - if (err) {<br> >> - goto out;<br> >> - }<br> >> + QDict *non_constant_dict = qdict_clone_shallow(qdict);<br> >><br> >> - netdev_add(opts, &err);<br> >> - if (err) {<br> >> - qemu_opts_del(opts);<br> >> - }<br> >> -<br> >> -out:<br> >> + qmp_marshal_netdev_add(non_constant_dict, NULL, &err);<br> >> + qobject_unref(non_constant_dict);<br> >> hmp_handle_error(mon, err);<br> >> }<br> <br> qmp_marshal_netdev_add() uses the QObject input visitor, which feels<br> wrong for HMP input.<br> <br> What exactly is the problem you're trying to solve? Can you show us a<br> reproducer?<br><br></blockquote><div>The problem was found during work on hotplug/unplug problems with q35</div><div>run q35 VM with netdev and hotpluggable nic (virtio or e1000e)</div><div>unplug the nic (device_del)</div><div>delete the netdev ()</div><div>add netdev with the same id as before - fail (Duplicated ID)</div><div><br></div></div></div></blockquote><div>Q35 is not mandatory for reproduction, the same with '-machine pc'</div><div><br></div></div></div> </blockquote></div> </blockquote></div>
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);
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
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); > > <div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, Nov 23, 2020 at 11:25 AM Markus Armbruster <<a href="mailto:armbru@redhat.com">armbru@redhat.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Andrew Melnichenko <<a href="mailto:andrew@daynix.com" target="_blank">andrew@daynix.com</a>> writes:<br> <br> > --000000000000f73b2205b4aef0c5<br> > Content-Type: text/plain; charset="UTF-8"<br> ><br> > Hi, the bug can be reproduced like that:<br> ><br> >> QEMU 5.1.50 monitor - type 'help' for more information<br> >> (qemu) netdev_add<br> >> type=tap,id=net0,script=/home/and/SRCS/qemu/ifup.sh,downscript=no<br> >> (qemu) info network<br> >> hub 0<br> >> \ hub0port1: __org.qemu.net1: index=0,type=user,net=10.0.2.0,restrict=off<br> >> \ hub0port0: e1000e.0:<br> >> index=0,type=nic,model=e1000e,macaddr=52:54:00:12:34:56<br> >> dnet0: index=0,type=nic,model=virtio-net-pci,macaddr=52:54:00:12:34:57<br> >> net0:<br> >> index=0,type=tap,ifname=tap0,script=/home/and/SRCS/qemu/ifup.sh,downscript=no<br> >> (qemu) netdev_del net0<br> >> (qemu) info network<br> >> hub 0<br> >> \ hub0port1: __org.qemu.net1: index=0,type=user,net=10.0.2.0,restrict=off<br> >> \ hub0port0: e1000e.0:<br> >> index=0,type=nic,model=e1000e,macaddr=52:54:00:12:34:56<br> >> dnet0: index=0,type=nic,model=virtio-net-pci,macaddr=52:54:00:12:34:57<br> >> (qemu) netdev_add<br> >> type=tap,id=net0,script=/home/and/SRCS/qemu/ifup.sh,downscript=no<br> >> Try "help netdev_add" for more information<br> >> (qemu) info network<br> >> hub 0<br> >> \ hub0port1: __org.qemu.net1: index=0,type=user,net=10.0.2.0,restrict=off<br> >> \ hub0port0: e1000e.0:<br> >> index=0,type=nic,model=e1000e,macaddr=52:54:00:12:34:56<br> >> dnet0: index=0,type=nic,model=virtio-net-pci,macaddr=52:54:00:12:34:57<br> >> (qemu)<br> >><br> >><br> > Its still actual bug - I've checked it with the<br> > master(2c6605389c1f76973d92b69b85d40d94b8f1092c).<br> <br> I can see this with an even simpler reproducer:<br> <br> $ qemu-system-x86_64 -S -display none -nodefaults -monitor stdio<br> QEMU 5.1.92 monitor - type 'help' for more information<br> (qemu) netdev_add user,id=net0<br> (qemu) info network<br> net0: index=0,type=user,net=10.0.2.0,restrict=off<br> (qemu) netdev_del net0<br> (qemu) info network<br> (qemu) netdev_add user,id=net0<br> upstream-qemu: Duplicate ID 'net0' for netdev<br> Try "help netdev_add" for more information<br> <br> The appended patch fixes it for me. It relies on nothing using the<br> "netdev" QemuOpts anymore. Eric, what do you think?<br> <br> <br> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c<br> index a6a6684df1..8bc6b7bcc6 100644<br> --- a/monitor/hmp-cmds.c<br> +++ b/monitor/hmp-cmds.c<br> @@ -1638,9 +1638,7 @@ void hmp_netdev_add(Monitor *mon, const QDict *qdict)<br> }<br> <br> netdev_add(opts, &err);<br> - if (err) {<br> - qemu_opts_del(opts);<br> - }<br> + qemu_opts_del(opts);<br> <br></blockquote><div><br></div><div>Unfortunately, if I'm not mistaken, with this fix qemu will be able to create from hmp several devices with the same id</div><div>(which is not expected).</div><div>For example:</div><div>netdev_add user,id=net0</div><div>netdev_add user,id=net0</div><div>info network lists 2 devices net0</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> out:<br> hmp_handle_error(mon, err);<br> <br> </blockquote></div></div>
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);
>>
>>
<div dir="ltr">The patch below solves both issues: with netdev created by hmp and with netdev created from command-line:<div><br></div><div>diff --git a/net/net.c b/net/net.c<br>index bcd5da4aa0..98294f24ed 100644<br>--- a/net/net.c<br>+++ b/net/net.c<br>@@ -1155,6 +1155,11 @@ void qmp_netdev_del(const char *id, Error **errp)<br> }<br><br> qemu_del_net_client(nc);<br>+<br>+ QemuOpts *opts = qemu_opts_find(qemu_find_opts("netdev"), id);<br>+ if (opts) {<br>+ qemu_opts_del(opts);<br>+ }<br> }</div><div><br></div><div>static void netfilter_print_info(Monitor *mon, NetFilterState *nf)<br></div><div><br></div><div>Please let me know if you have any objections.</div><div>If not, I'll send it as a patch.</div><div><br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, Nov 23, 2020 at 5:35 PM Yuri Benditovich <<a href="mailto:yuri.benditovich@daynix.com">yuri.benditovich@daynix.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, Nov 23, 2020 at 11:25 AM Markus Armbruster <<a href="mailto:armbru@redhat.com" target="_blank">armbru@redhat.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Andrew Melnichenko <<a href="mailto:andrew@daynix.com" target="_blank">andrew@daynix.com</a>> writes:<br>
<br>
> --000000000000f73b2205b4aef0c5<br>
> Content-Type: text/plain; charset="UTF-8"<br>
><br>
> Hi, the bug can be reproduced like that:<br>
><br>
>> QEMU 5.1.50 monitor - type 'help' for more information<br>
>> (qemu) netdev_add<br>
>> type=tap,id=net0,script=/home/and/SRCS/qemu/ifup.sh,downscript=no<br>
>> (qemu) info network<br>
>> hub 0<br>
>> \ hub0port1: __org.qemu.net1: index=0,type=user,net=10.0.2.0,restrict=off<br>
>> \ hub0port0: e1000e.0:<br>
>> index=0,type=nic,model=e1000e,macaddr=52:54:00:12:34:56<br>
>> dnet0: index=0,type=nic,model=virtio-net-pci,macaddr=52:54:00:12:34:57<br>
>> net0:<br>
>> index=0,type=tap,ifname=tap0,script=/home/and/SRCS/qemu/ifup.sh,downscript=no<br>
>> (qemu) netdev_del net0<br>
>> (qemu) info network<br>
>> hub 0<br>
>> \ hub0port1: __org.qemu.net1: index=0,type=user,net=10.0.2.0,restrict=off<br>
>> \ hub0port0: e1000e.0:<br>
>> index=0,type=nic,model=e1000e,macaddr=52:54:00:12:34:56<br>
>> dnet0: index=0,type=nic,model=virtio-net-pci,macaddr=52:54:00:12:34:57<br>
>> (qemu) netdev_add<br>
>> type=tap,id=net0,script=/home/and/SRCS/qemu/ifup.sh,downscript=no<br>
>> Try "help netdev_add" for more information<br>
>> (qemu) info network<br>
>> hub 0<br>
>> \ hub0port1: __org.qemu.net1: index=0,type=user,net=10.0.2.0,restrict=off<br>
>> \ hub0port0: e1000e.0:<br>
>> index=0,type=nic,model=e1000e,macaddr=52:54:00:12:34:56<br>
>> dnet0: index=0,type=nic,model=virtio-net-pci,macaddr=52:54:00:12:34:57<br>
>> (qemu)<br>
>><br>
>><br>
> Its still actual bug - I've checked it with the<br>
> master(2c6605389c1f76973d92b69b85d40d94b8f1092c).<br>
<br>
I can see this with an even simpler reproducer:<br>
<br>
$ qemu-system-x86_64 -S -display none -nodefaults -monitor stdio<br>
QEMU 5.1.92 monitor - type 'help' for more information<br>
(qemu) netdev_add user,id=net0<br>
(qemu) info network<br>
net0: index=0,type=user,net=10.0.2.0,restrict=off<br>
(qemu) netdev_del net0<br>
(qemu) info network<br>
(qemu) netdev_add user,id=net0<br>
upstream-qemu: Duplicate ID 'net0' for netdev<br>
Try "help netdev_add" for more information<br>
<br>
The appended patch fixes it for me. It relies on nothing using the<br>
"netdev" QemuOpts anymore. Eric, what do you think?<br>
<br>
<br>
diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c<br>
index a6a6684df1..8bc6b7bcc6 100644<br>
--- a/monitor/hmp-cmds.c<br>
+++ b/monitor/hmp-cmds.c<br>
@@ -1638,9 +1638,7 @@ void hmp_netdev_add(Monitor *mon, const QDict *qdict)<br>
}<br>
<br>
netdev_add(opts, &err);<br>
- if (err) {<br>
- qemu_opts_del(opts);<br>
- }<br>
+ qemu_opts_del(opts);<br>
<br></blockquote><div><br></div><div>Unfortunately, if I'm not mistaken, with this fix qemu will be able to create from hmp several devices with the same id</div><div>(which is not expected).</div><div>For example:</div><div>netdev_add user,id=net0</div><div>netdev_add user,id=net0</div><div>info network lists 2 devices net0</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
out:<br>
hmp_handle_error(mon, err);<br>
<br>
</blockquote></div></div>
</blockquote></div>
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.
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
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 > > <div dir="ltr">Please confirm that this patch is intended to solve only the problem with hmp (and disallow duplicated ids)<div>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? </div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Nov 24, 2020 at 12:21 PM Markus Armbruster <<a href="mailto:armbru@redhat.com">armbru@redhat.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Markus Armbruster <<a href="mailto:armbru@redhat.com" target="_blank">armbru@redhat.com</a>> writes:<br> <br> > This means commit 08712fcb85 "net: Track netdevs in NetClientState<br> > rather than QemuOpt" didn't actually replace QemuOpts completely.<br> ><br> > This affects QMP:<br> ><br> > $ socat "READLINE,history=$HOME/.qmp_history,prompt=QMP>" UNIX-CONNECT:$HOME/work/images/test-qmp <br> > {"QMP": {"version": {"qemu": {"micro": 92, "minor": 1, "major": 5}, "package": "v5.2.0-rc2-19-gff85db769f-dirty"}, "capabilities": ["oob"]}}<br> > QMP>{ "execute": "qmp_capabilities", "arguments": { "enable": ["oob"] } }<br> > {"return": {}}<br> > QMP>{"execute": "netdev_add", "arguments": {"type": "user", "id":"net0"}}<br> > {"return": {}}<br> > QMP>{"execute": "netdev_add", "arguments": {"type": "user", "id":"net0"}}<br> > {"return": {}}<br> ><br> > Results in two netdevs called "net0". Needs fixing.<br> <br> Here's my attempt. If it looks good to you, I'll post it as a proper<br> patch.<br> <br> <br> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c<br> index a6a6684df1..8bc6b7bcc6 100644<br> --- a/monitor/hmp-cmds.c<br> +++ b/monitor/hmp-cmds.c<br> @@ -1638,9 +1638,7 @@ void hmp_netdev_add(Monitor *mon, const QDict *qdict)<br> }<br> <br> netdev_add(opts, &err);<br> - if (err) {<br> - qemu_opts_del(opts);<br> - }<br> + qemu_opts_del(opts);<br> <br> out:<br> hmp_handle_error(mon, err);<br> diff --git a/net/net.c b/net/net.c<br> index 794c652282..eb743aca23 100644<br> --- a/net/net.c<br> +++ b/net/net.c<br> @@ -978,6 +978,7 @@ static int (* const net_client_init_fun[NET_CLIENT_DRIVER__MAX])(<br> static int net_client_init1(const Netdev *netdev, bool is_netdev, Error **errp)<br> {<br> NetClientState *peer = NULL;<br> + NetClientState *nc;<br> <br> if (is_netdev) {<br> if (netdev->type == NET_CLIENT_DRIVER_NIC ||<br> @@ -1005,6 +1006,12 @@ static int net_client_init1(const Netdev *netdev, bool is_netdev, Error **errp)<br> }<br> }<br> <br> + nc = qemu_find_netdev(netdev->id);<br> + if (nc) {<br> + error_setg(errp, "Duplicate ID '%s'", netdev->id);<br> + return -1;<br> + }<br> +<br> if (net_client_init_fun[netdev->type](netdev, netdev->id, peer, errp) < 0) {<br> /* FIXME drop when all init functions store an Error */<br> if (errp && !*errp) {<br> @@ -1015,8 +1022,6 @@ static int net_client_init1(const Netdev *netdev, bool is_netdev, Error **errp)<br> }<br> <br> if (is_netdev) {<br> - NetClientState *nc;<br> -<br> nc = qemu_find_netdev(netdev->id);<br> assert(nc);<br> nc->is_netdev = true;<br> -- <br> 2.26.2<br> <br> </blockquote></div>
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...
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
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 > > <div dir="ltr"><div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Nov 24, 2020 at 3:36 PM Markus Armbruster <<a href="mailto:armbru@redhat.com">armbru@redhat.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Markus Armbruster <<a href="mailto:armbru@redhat.com" target="_blank">armbru@redhat.com</a>> writes:<br> <br> > Yuri Benditovich <<a href="mailto:yuri.benditovich@daynix.com" target="_blank">yuri.benditovich@daynix.com</a>> writes:<br> ><br> >> Please confirm that this patch is intended to solve only the problem with<br> >> hmp (and disallow duplicated ids)<br> ><br> > The intent is to reject duplicate ID and to accept non-duplicate ID, no<br> > matter how the device is created (CLI, HMP, QMP) or a prior instance was<br> > deleted (HMP, QMP).<br> ><br> >> With it the netdev that was added from qemu's command line and was deleted<br> >> (for example by hmp) still can't be created, correct?<br> ><br> > Yet another case; back to the drawing board...<br> <br> Next try. Hope this is one holds water :)<br> <br> <br> diff --git a/net/net.c b/net/net.c<br> index 794c652282..c1dc75fc37 100644<br> --- a/net/net.c<br> +++ b/net/net.c<br> @@ -978,6 +978,7 @@ static int (* const net_client_init_fun[NET_CLIENT_DRIVER__MAX])(<br> static int net_client_init1(const Netdev *netdev, bool is_netdev, Error **errp)<br> {<br> NetClientState *peer = NULL;<br> + NetClientState *nc;<br> <br> if (is_netdev) {<br> if (netdev->type == NET_CLIENT_DRIVER_NIC ||<br> @@ -1005,6 +1006,12 @@ static int net_client_init1(const Netdev *netdev, bool is_netdev, Error **errp)<br> }<br> }<br> <br> + nc = qemu_find_netdev(netdev->id);<br> + if (nc) {<br> + error_setg(errp, "Duplicate ID '%s'", netdev->id);<br> + return -1;<br> + }<br> +<br> if (net_client_init_fun[netdev->type](netdev, netdev->id, peer, errp) < 0) {<br> /* FIXME drop when all init functions store an Error */<br> if (errp && !*errp) {<br> @@ -1015,8 +1022,6 @@ static int net_client_init1(const Netdev *netdev, bool is_netdev, Error **errp)<br> }<br> <br> if (is_netdev) {<br> - NetClientState *nc;<br> -<br> nc = qemu_find_netdev(netdev->id);<br> assert(nc);<br> nc->is_netdev = true;<br> @@ -1137,6 +1142,7 @@ void qmp_netdev_add(Netdev *netdev, Error **errp)<br> void qmp_netdev_del(const char *id, Error **errp)<br> {<br> NetClientState *nc;<br> + QemuOpts *opts;<br> <br> nc = qemu_find_netdev(id);<br> if (!nc) {<br> @@ -1151,6 +1157,16 @@ void qmp_netdev_del(const char *id, Error **errp)<br> }<br> <br> qemu_del_net_client(nc);<br> +<br> + /*<br> + * Wart: we need to delete the QemuOpts associated with netdevs<br> + * created via CLI or HMP, to avoid bogus "Duplicate ID" errors in<br> + * HMP netdev_add.<br> + */<br> + opts = qemu_opts_find(qemu_find_opts("netdev"), id);<br> + if (opts) {<br> + qemu_opts_del(opts);<br> + }<br> }<br> <br></blockquote><div><br></div><div>With this part there is no need to unconditionally delete the options in <span style="color:rgb(80,0,80)">hmp_netdev_add, correct?</span></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> static void netfilter_print_info(Monitor *mon, NetFilterState *nf)<br> -- <br> 2.26.2<br> <br> </blockquote></div></div></div>
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
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?
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?
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 <div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Nov 24, 2020 at 5:46 PM Markus Armbruster <<a href="mailto:armbru@redhat.com">armbru@redhat.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Yuri Benditovich <<a href="mailto:yuri.benditovich@daynix.com" target="_blank">yuri.benditovich@daynix.com</a>> writes:<br> <br> > On Tue, Nov 24, 2020 at 3:36 PM Markus Armbruster <<a href="mailto:armbru@redhat.com" target="_blank">armbru@redhat.com</a>> wrote:<br> ><br> >> Markus Armbruster <<a href="mailto:armbru@redhat.com" target="_blank">armbru@redhat.com</a>> writes:<br> >><br> >> > Yuri Benditovich <<a href="mailto:yuri.benditovich@daynix.com" target="_blank">yuri.benditovich@daynix.com</a>> writes:<br> >> ><br> >> >> Please confirm that this patch is intended to solve only the problem<br> >> with<br> >> >> hmp (and disallow duplicated ids)<br> >> ><br> >> > The intent is to reject duplicate ID and to accept non-duplicate ID, no<br> >> > matter how the device is created (CLI, HMP, QMP) or a prior instance was<br> >> > deleted (HMP, QMP).<br> >> ><br> >> >> With it the netdev that was added from qemu's command line and was<br> >> deleted<br> >> >> (for example by hmp) still can't be created, correct?<br> >> ><br> >> > Yet another case; back to the drawing board...<br> >><br> >> Next try. Hope this is one holds water :)<br> >><br> >><br> >> diff --git a/net/net.c b/net/net.c<br> >> index 794c652282..c1dc75fc37 100644<br> >> --- a/net/net.c<br> >> +++ b/net/net.c<br> >> @@ -978,6 +978,7 @@ static int (* const<br> >> net_client_init_fun[NET_CLIENT_DRIVER__MAX])(<br> >> static int net_client_init1(const Netdev *netdev, bool is_netdev, Error<br> >> **errp)<br> >> {<br> >> NetClientState *peer = NULL;<br> >> + NetClientState *nc;<br> >><br> >> if (is_netdev) {<br> >> if (netdev->type == NET_CLIENT_DRIVER_NIC ||<br> >> @@ -1005,6 +1006,12 @@ static int net_client_init1(const Netdev *netdev,<br> >> bool is_netdev, Error **errp)<br> >> }<br> >> }<br> >><br> >> + nc = qemu_find_netdev(netdev->id);<br> >> + if (nc) {<br> >> + error_setg(errp, "Duplicate ID '%s'", netdev->id);<br> >> + return -1;<br> >> + }<br> >> +<br> >> if (net_client_init_fun[netdev->type](netdev, netdev->id, peer, errp)<br> >> < 0) {<br> >> /* FIXME drop when all init functions store an Error */<br> >> if (errp && !*errp) {<br> >> @@ -1015,8 +1022,6 @@ static int net_client_init1(const Netdev *netdev,<br> >> bool is_netdev, Error **errp)<br> >> }<br> >><br> >> if (is_netdev) {<br> >> - NetClientState *nc;<br> >> -<br> >> nc = qemu_find_netdev(netdev->id);<br> >> assert(nc);<br> >> nc->is_netdev = true;<br> >> @@ -1137,6 +1142,7 @@ void qmp_netdev_add(Netdev *netdev, Error **errp)<br> >> void qmp_netdev_del(const char *id, Error **errp)<br> >> {<br> >> NetClientState *nc;<br> >> + QemuOpts *opts;<br> >><br> >> nc = qemu_find_netdev(id);<br> >> if (!nc) {<br> >> @@ -1151,6 +1157,16 @@ void qmp_netdev_del(const char *id, Error **errp)<br> >> }<br> >><br> >> qemu_del_net_client(nc);<br> >> +<br> >> + /*<br> >> + * Wart: we need to delete the QemuOpts associated with netdevs<br> >> + * created via CLI or HMP, to avoid bogus "Duplicate ID" errors in<br> >> + * HMP netdev_add.<br> >> + */<br> >> + opts = qemu_opts_find(qemu_find_opts("netdev"), id);<br> >> + if (opts) {<br> >> + qemu_opts_del(opts);<br> >> + }<br> >> }<br> >><br> >><br> > With this part there is no need to unconditionally delete the options<br> > in hmp_netdev_add,<br> > correct?<br> <br> Yes.<br> <br> The CLI accumulates -netdev in option group "netdev". It has to, or<br> else -writeconfig doesn't work.<br> <br> Before commit 08712fcb85 "net: Track netdevs in NetClientState rather<br> than QemuOpt", netdev_add added to the option group, and netdev_del<br> removed from it, both for HMP and QMP. Thus, every netdev had a<br> corresponding QemuOpts in this option group.<br> <br> Commit 08712fcb85 dropped this for QMP netdev_add and both netdev_del.<br> Now a netdev has a corresponding QemuOpts only when it was created with<br> CLI or HMP. Two issues:<br> <br> * QMP netdev_add loses its "no duplicate ID" check.<br> <br> My change to net_init_client1() fixes this.<br> <br> * Both netdev_add can leave QemuOpts behind, breaking HMP netdev_add.<br> <br> My change to qmp_netdev_del() fixes this.<br> <br> Questions?<br> <br></blockquote><div>No questions, looking forward for the final patch</div><div>Thanks</div><div> </div></div></div>
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!
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); }