On Tue, Nov 24, 2020 at 5:46 PM Markus Armbruster <arm...@redhat.com> wrote:
> Yuri Benditovich <yuri.benditov...@daynix.com> writes:
>
> > On Tue, Nov 24, 2020 at 3:36 PM Markus Armbruster <arm...@redhat.com>
> wrote:
> >
> >> Markus Armbruster <arm...@redhat.com> writes:
> >>
> >> > Yuri Benditovich <yuri.benditov...@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
