From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-5.8 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,HTML_MESSAGE,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 37A9EC5519F for ; Wed, 25 Nov 2020 08:56:35 +0000 (UTC) Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 0E4F8206F7 for ; Wed, 25 Nov 2020 08:56:33 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=daynix-com.20150623.gappssmtp.com header.i=@daynix-com.20150623.gappssmtp.com header.b="tUz4tk8B" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 0E4F8206F7 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=daynix.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Received: from localhost ([::1]:57906 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1khqbH-0006bY-GH for qemu-devel@archiver.kernel.org; Wed, 25 Nov 2020 03:56:31 -0500 Received: from eggs.gnu.org ([2001:470:142:3::10]:50460) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1khqZr-0005hm-5v for qemu-devel@nongnu.org; Wed, 25 Nov 2020 03:55:03 -0500 Received: from mail-oo1-xc42.google.com ([2607:f8b0:4864:20::c42]:34254) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1khqZo-0001OC-Dh for qemu-devel@nongnu.org; Wed, 25 Nov 2020 03:55:02 -0500 Received: by mail-oo1-xc42.google.com with SMTP id l10so317822ooh.1 for ; Wed, 25 Nov 2020 00:54:56 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=daynix-com.20150623.gappssmtp.com; s=20150623; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=PAZEAGuMhmOcEV5WNzYxhTDbEV7LwlxWEtuY1MviPC4=; b=tUz4tk8BMY2QNeKxw1LfU1QSUzGrQXlZpXL0CR0ZixBBQpP2WXy8mw6mnBamuPe/uK zrNdcXok8mcLAiwJIX+NeuIqg19KZv6elPr43al9N9k8mjLHIt/+hXnbyr+CXFl3pHDz Eo8Lw1sGGY+T4zGowa86bIU5R38OcMyhnoTi+kAVWd3zB4E81aHc+Woshx9M3ibRs7tB CG1yRxmT8mXosPCh/L/wp2ykogsXu9L8rOznYifsSaXMbBda9e2hk0tJUsCBOiH0ETMM d+dW2R70b87HvXpPkKswTYd+OArD3ZDGLGH/XloNESegw7Ql/+xGQbv4PTnzFeDEWApC 0LaQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=PAZEAGuMhmOcEV5WNzYxhTDbEV7LwlxWEtuY1MviPC4=; b=pBGJhytMRa0BGFnCR+fb+zmEcNOwAsv6P5j+L9MduYUp70nQxRBpkICuZYiTScfd2O XefFJniFUeR36ZOythTYAM8w2mycN5r5O6Cgd26nyNa+neRlonXkTaJfE/ouUJlkFCK/ 4hfh6qknz2w8Lko80ZTGwnUcRZLp/tIwcBA89MUiIeSmgseF5y2ImdDLHs4ueM8ke+Sy CiC3/9fpzg4+r9nbmMxREloXGCIY4NyGO7AxQ4ZwKWWDMDrRrmBlrRSdmuEPnuOU3qzU gaqYZB9Bz0rjS+Bd49RqHAswJLncKkWTX4ub3CkwgoAdPsiSxWUbd3teFdI5qszRbit9 fmTg== X-Gm-Message-State: AOAM530tYs7cLRstUHikFPEAgiHTaOFFVKvaUq4fZ4GoWyWflflL+IPY ENwmWG6gmBo36Tp+ZI/IvL98k8WS8jdTpMwsyoVyGQ== X-Google-Smtp-Source: ABdhPJzyWG5l82d+1+OiUSIJsjdZiQMyUo63UTmrC5H+XYZxYHGnLI5sEQLjAD7jKqa9G604JOUpJXZmskFMotQDubg= X-Received: by 2002:a4a:e882:: with SMTP id g2mr60578ooe.55.1606294495004; Wed, 25 Nov 2020 00:54:55 -0800 (PST) MIME-Version: 1.0 References: <20200716035532.1407660-1-andrew@daynix.com> <874klk5gnc.fsf@dusky.pond.sub.org> <87lfesv2zu.fsf@dusky.pond.sub.org> <87blfnp20k.fsf@dusky.pond.sub.org> <87lferm4x5.fsf@dusky.pond.sub.org> <87tutej3dc.fsf@dusky.pond.sub.org> <87blfmj2qx.fsf@dusky.pond.sub.org> <87y2iqg3m3.fsf@dusky.pond.sub.org> In-Reply-To: <87y2iqg3m3.fsf@dusky.pond.sub.org> From: Yuri Benditovich Date: Wed, 25 Nov 2020 10:54:44 +0200 Message-ID: Subject: Re: [PATCH] hmp: Changed hmp_netdev_add() using qmp_marshal_netdev_add() To: Markus Armbruster Content-Type: multipart/alternative; boundary="000000000000c79aea05b4ea94e6" Received-SPF: none client-ip=2607:f8b0:4864:20::c42; envelope-from=yuri.benditovich@daynix.com; helo=mail-oo1-xc42.google.com X-Spam_score_int: -18 X-Spam_score: -1.9 X-Spam_bar: - X-Spam_report: (-1.9 / 5.0 requ) BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, HTML_MESSAGE=0.001, RCVD_IN_DNSWL_NONE=-0.0001, SPF_HELO_NONE=0.001, SPF_NONE=0.001 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Yan Vugenfirer , Andrew Melnichenko , qemu-devel@nongnu.org, "Dr. David Alan Gilbert" Errors-To: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Sender: "Qemu-devel" --000000000000c79aea05b4ea94e6 Content-Type: text/plain; charset="UTF-8" On Tue, Nov 24, 2020 at 5:46 PM Markus Armbruster wrote: > Yuri Benditovich writes: > > > On Tue, Nov 24, 2020 at 3:36 PM Markus Armbruster > wrote: > > > >> Markus Armbruster writes: > >> > >> > Yuri Benditovich 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 --000000000000c79aea05b4ea94e6 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


=
On Tue, Nov 24, 2020 at 5:46 PM Marku= s 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-duplic= ate ID, no
>> > matter how the device is created (CLI, HMP, QMP) or a prior i= nstance 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.=C2=A0 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])(
>>=C2=A0 static int net_client_init1(const Netdev *netdev, bool is_ne= tdev, Error
>> **errp)
>>=C2=A0 {
>>=C2=A0 =C2=A0 =C2=A0 NetClientState *peer =3D NULL;
>> +=C2=A0 =C2=A0 NetClientState *nc;
>>
>>=C2=A0 =C2=A0 =C2=A0 if (is_netdev) {
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (netdev->type =3D=3D NET_C= LIENT_DRIVER_NIC ||
>> @@ -1005,6 +1006,12 @@ static int net_client_init1(const Netdev *n= etdev,
>> bool is_netdev, Error **errp)
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 }
>>=C2=A0 =C2=A0 =C2=A0 }
>>
>> +=C2=A0 =C2=A0 nc =3D qemu_find_netdev(netdev->id);
>> +=C2=A0 =C2=A0 if (nc) {
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 error_setg(errp, "Duplicate ID &= #39;%s'", netdev->id);
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 return -1;
>> +=C2=A0 =C2=A0 }
>> +
>>=C2=A0 =C2=A0 =C2=A0 if (net_client_init_fun[netdev->type](netde= v, netdev->id, peer, errp)
>> < 0) {
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 /* FIXME drop when all init func= tions store an Error */
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (errp && !*errp) { >> @@ -1015,8 +1022,6 @@ static int net_client_init1(const Netdev *ne= tdev,
>> bool is_netdev, Error **errp)
>>=C2=A0 =C2=A0 =C2=A0 }
>>
>>=C2=A0 =C2=A0 =C2=A0 if (is_netdev) {
>> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 NetClientState *nc;
>> -
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 nc =3D qemu_find_netdev(netdev-&= gt;id);
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 assert(nc);
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 nc->is_netdev =3D true;
>> @@ -1137,6 +1142,7 @@ void qmp_netdev_add(Netdev *netdev, Error **= errp)
>>=C2=A0 void qmp_netdev_del(const char *id, Error **errp)
>>=C2=A0 {
>>=C2=A0 =C2=A0 =C2=A0 NetClientState *nc;
>> +=C2=A0 =C2=A0 QemuOpts *opts;
>>
>>=C2=A0 =C2=A0 =C2=A0 nc =3D qemu_find_netdev(id);
>>=C2=A0 =C2=A0 =C2=A0 if (!nc) {
>> @@ -1151,6 +1157,16 @@ void qmp_netdev_del(const char *id, Error *= *errp)
>>=C2=A0 =C2=A0 =C2=A0 }
>>
>>=C2=A0 =C2=A0 =C2=A0 qemu_del_net_client(nc);
>> +
>> +=C2=A0 =C2=A0 /*
>> +=C2=A0 =C2=A0 =C2=A0* Wart: we need to delete the QemuOpts associ= ated with netdevs
>> +=C2=A0 =C2=A0 =C2=A0* created via CLI or HMP, to avoid bogus &quo= t;Duplicate ID" errors in
>> +=C2=A0 =C2=A0 =C2=A0* HMP netdev_add.
>> +=C2=A0 =C2=A0 =C2=A0*/
>> +=C2=A0 =C2=A0 opts =3D qemu_opts_find(qemu_find_opts("netdev= "), id);
>> +=C2=A0 =C2=A0 if (opts) {
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 qemu_opts_del(opts);
>> +=C2=A0 =C2=A0 }
>>=C2=A0 }
>>
>>
> With this part there is no need to unconditionally delete the options<= br> > in hmp_netdev_add,
> correct?

Yes.

The CLI accumulates -netdev in option group "netdev".=C2=A0 It ha= s to, or
else -writeconfig doesn't work.

Before commit 08712fcb85 "net: Track netdevs in NetClientState rather<= br> than QemuOpt", netdev_add added to the option group, and netdev_del removed from it, both for HMP and QMP.=C2=A0 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.=C2=A0 Two issues:

* QMP netdev_add loses its "no duplicate ID" check.

=C2=A0 My change to net_init_client1() fixes this.

* Both netdev_add can leave QemuOpts behind, breaking HMP netdev_add.

=C2=A0 My change to qmp_netdev_del() fixes this.

Questions?

No questions, looking forward for the final patch
Thanks
=C2=A0
--000000000000c79aea05b4ea94e6--