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.6 required=3.0 tests=BAYES_00,DKIM_INVALID, DKIM_SIGNED,HEADER_FROM_DIFFERENT_DOMAINS,HTML_MESSAGE,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS 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 079A0C2D0E4 for ; Tue, 24 Nov 2020 11:38:03 +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 245C52075A for ; Tue, 24 Nov 2020 11:38:01 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=fail reason="signature verification failed" (2048-bit key) header.d=daynix-com.20150623.gappssmtp.com header.i=@daynix-com.20150623.gappssmtp.com header.b="pPbC8iKz" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 245C52075A 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]:52842 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1khWdz-00006B-Ie for qemu-devel@archiver.kernel.org; Tue, 24 Nov 2020 06:37:59 -0500 Received: from eggs.gnu.org ([2001:470:142:3::10]:47864) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1khWd9-0007uX-BW for qemu-devel@nongnu.org; Tue, 24 Nov 2020 06:37:07 -0500 Received: from mail-ot1-x342.google.com ([2607:f8b0:4864:20::342]:44165) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1khWd7-0002rV-AK for qemu-devel@nongnu.org; Tue, 24 Nov 2020 06:37:07 -0500 Received: by mail-ot1-x342.google.com with SMTP id f16so19000642otl.11 for ; Tue, 24 Nov 2020 03:37:04 -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=9IxN94mRo7FNAgnnupAHU04Xgv2COA1pOCuCrOZVMFM=; b=pPbC8iKzyhpVo8iJQ/OYzkHO1NqqfPUT8viDEMyToGYAi1B52FDml5eXxeL/MFBxvb ggkRFUfoXLlGNH3eHWgazQtw55aNWXyX41X+NedsOyNla2W5RPOFDTsxOXvhlBDzzoZB ogDjlgUqbWqvoExTP1Nk+tFNcSrDhlgPjzdZRRptN31yTFIn7PKMJ0dTnNAzbjuQpw8Z mV0xou2rMELGPyCfbqIiZfGNyxz97LrE2K07mB347bRBioiorIx3iprVhHqBgOoaqLc4 OXSVNo1b0PNidQ+h43pLg3EneK6glk3urVqMgBI+bntOWfTTfqNZCVgoVz9q2PntB16I 1P9w== 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=9IxN94mRo7FNAgnnupAHU04Xgv2COA1pOCuCrOZVMFM=; b=q3ts8x0bE+xQjCDFZEbjG4DF3SssZlsjJaODbvDUtfIINNuMUcfYQwe6Yr4QKVeLzB ZTAPOeQ/rE4tC97ELU7gEUDSARQ3Dxb5ZZicKp1I1bamds8QLElegrRKCkFBL/jmZO7N WOgk+uVD7hQ4e+XgjdGS6weUIZCdqkIxBJK6/cbn4CAnT+XKMV1fwjVURPtF1xcbGGpu 8V8TXknRUFFS+Gfamee+cq4UNmXReBaaU2A2I4ca0arWhD6oYmLYzeujryn6ncwMlio8 2rj3Y3uM91O8HiCHFP0ytiCyjmSVez5jyOs3hPIVrIkO/UlYaB4e3IHpn6/WAKhHv6/6 jsog== X-Gm-Message-State: AOAM533ZgWCO7okreUvNMDY2XTHqK9WBpiBi12koZve3iayquHS3TO3v g6p/kJHNhOOAsEwMFeNOQnhzizADWjEamrZMpNqE+w== X-Google-Smtp-Source: ABdhPJw7prSAhLA8xxotpP8BXRvIJ8CKVr+lpXUtY+M5Sv91hMFcVw6dySsDyAYwjAmN+kuGA4B1kr2k/IP49eLDQCo= X-Received: by 2002:a9d:4715:: with SMTP id a21mr2973739otf.220.1606217824164; Tue, 24 Nov 2020 03:37:04 -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> In-Reply-To: <87lferm4x5.fsf@dusky.pond.sub.org> From: Yuri Benditovich Date: Tue, 24 Nov 2020 13:36:53 +0200 Message-ID: Subject: Re: [PATCH] hmp: Changed hmp_netdev_add() using qmp_marshal_netdev_add() To: Markus Armbruster Content-Type: multipart/alternative; boundary="000000000000d7772605b4d8baae" Received-SPF: none client-ip=2607:f8b0:4864:20::342; envelope-from=yuri.benditovich@daynix.com; helo=mail-ot1-x342.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" --000000000000d7772605b4d8baae Content-Type: text/plain; charset="UTF-8" Please confirm that this patch is intended to solve only the problem with hmp (and disallow duplicated ids) 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? On Tue, Nov 24, 2020 at 12:21 PM Markus Armbruster wrote: > Markus Armbruster writes: > > > This means commit 08712fcb85 "net: Track netdevs in NetClientState > > rather than QemuOpt" didn't actually replace QemuOpts completely. > > > > This affects QMP: > > > > $ socat "READLINE,history=$HOME/.qmp_history,prompt=QMP>" > UNIX-CONNECT:$HOME/work/images/test-qmp > > {"QMP": {"version": {"qemu": {"micro": 92, "minor": 1, "major": 5}, > "package": "v5.2.0-rc2-19-gff85db769f-dirty"}, "capabilities": ["oob"]}} > > QMP>{ "execute": "qmp_capabilities", "arguments": { "enable": > ["oob"] } } > > {"return": {}} > > QMP>{"execute": "netdev_add", "arguments": {"type": "user", > "id":"net0"}} > > {"return": {}} > > QMP>{"execute": "netdev_add", "arguments": {"type": "user", > "id":"net0"}} > > {"return": {}} > > > > Results in two netdevs called "net0". Needs fixing. > > Here's my attempt. If it looks good to you, I'll post it as a proper > patch. > > > diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c > index a6a6684df1..8bc6b7bcc6 100644 > --- a/monitor/hmp-cmds.c > +++ b/monitor/hmp-cmds.c > @@ -1638,9 +1638,7 @@ void hmp_netdev_add(Monitor *mon, const QDict *qdict) > } > > netdev_add(opts, &err); > - if (err) { > - qemu_opts_del(opts); > - } > + qemu_opts_del(opts); > > out: > hmp_handle_error(mon, err); > diff --git a/net/net.c b/net/net.c > index 794c652282..eb743aca23 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; > -- > 2.26.2 > > --000000000000d7772605b4d8baae Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Please confirm that this patch is intended to solve only t= he problem with hmp (and disallow duplicated ids)
With it the netdev th= at was added from qemu's command line and was deleted (for example by h= mp) still can't be created, correct?=C2=A0=C2=A0

On Tue, Nov 24, 2= 020 at 12:21 PM Markus Armbruster <= armbru@redhat.com> wrote:
Markus Armbruster <armbru@redhat.com> writes:

> This means commit 08712fcb85 "net: Track netdevs in NetClientStat= e
> rather than QemuOpt" didn't actually replace QemuOpts complet= ely.
>
> This affects QMP:
>
>=C2=A0 =C2=A0 =C2=A0$ socat "READLINE,history=3D$HOME/.qmp_history= ,prompt=3DQMP>" UNIX-CONNECT:$HOME/work/images/test-qmp
>=C2=A0 =C2=A0 =C2=A0{"QMP": {"version": {"qemu= ": {"micro": 92, "minor": 1, "major": 5}= , "package": "v5.2.0-rc2-19-gff85db769f-dirty"}, "= capabilities": ["oob"]}}
>=C2=A0 =C2=A0 =C2=A0QMP>{ "execute": "qmp_capabilitie= s", "arguments": { "enable": ["oob"] } }=
>=C2=A0 =C2=A0 =C2=A0{"return": {}}
>=C2=A0 =C2=A0 =C2=A0QMP>{"execute": "netdev_add"= , "arguments": {"type": "user", "id"= ;:"net0"}}
>=C2=A0 =C2=A0 =C2=A0{"return": {}}
>=C2=A0 =C2=A0 =C2=A0QMP>{"execute": "netdev_add"= , "arguments": {"type": "user", "id"= ;:"net0"}}
>=C2=A0 =C2=A0 =C2=A0{"return": {}}
>
> Results in two netdevs called "net0".=C2=A0 Needs fixing.
Here's my attempt.=C2=A0 If it looks good to you, I'll post it as a= proper
patch.


diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index a6a6684df1..8bc6b7bcc6 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -1638,9 +1638,7 @@ void hmp_netdev_add(Monitor *mon, const QDict *qdict)=
=C2=A0 =C2=A0 =C2=A0}

=C2=A0 =C2=A0 =C2=A0netdev_add(opts, &err);
-=C2=A0 =C2=A0 if (err) {
-=C2=A0 =C2=A0 =C2=A0 =C2=A0 qemu_opts_del(opts);
-=C2=A0 =C2=A0 }
+=C2=A0 =C2=A0 qemu_opts_del(opts);

=C2=A0out:
=C2=A0 =C2=A0 =C2=A0hmp_handle_error(mon, err);
diff --git a/net/net.c b/net/net.c
index 794c652282..eb743aca23 100644
--- a/net/net.c
+++ b/net/net.c
@@ -978,6 +978,7 @@ static int (* const net_client_init_fun[NET_CLIENT_DRIV= ER__MAX])(
=C2=A0static int net_client_init1(const Netdev *netdev, bool is_netdev, Err= or **errp)
=C2=A0{
=C2=A0 =C2=A0 =C2=A0NetClientState *peer =3D NULL;
+=C2=A0 =C2=A0 NetClientState *nc;

=C2=A0 =C2=A0 =C2=A0if (is_netdev) {
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (netdev->type =3D=3D NET_CLIENT_DRI= VER_NIC ||
@@ -1005,6 +1006,12 @@ static int net_client_init1(const Netdev *netdev, bo= ol 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 '%s= 9;", netdev->id);
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 return -1;
+=C2=A0 =C2=A0 }
+
=C2=A0 =C2=A0 =C2=A0if (net_client_init_fun[netdev->type](netdev, netdev= ->id, peer, errp) < 0) {
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0/* FIXME drop when all init functions sto= re an Error */
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (errp && !*errp) {
@@ -1015,8 +1022,6 @@ static int net_client_init1(const Netdev *netdev, boo= l is_netdev, Error **errp)
=C2=A0 =C2=A0 =C2=A0}

=C2=A0 =C2=A0 =C2=A0if (is_netdev) {
-=C2=A0 =C2=A0 =C2=A0 =C2=A0 NetClientState *nc;
-
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0nc =3D qemu_find_netdev(netdev->id); =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0assert(nc);
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0nc->is_netdev =3D true;
--
2.26.2

--000000000000d7772605b4d8baae--