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