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

[Markus Armbruster <armbru@redhat.com>]

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

> On 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?