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

[Yuri Benditovich <yuri.benditovich@daynix.com>]

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

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

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

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