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 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 smtp.lore.kernel.org (Postfix) with ESMTPS id 6D05FC433EF for ; Wed, 15 Dec 2021 06:51:41 +0000 (UTC) Received: from localhost ([::1]:51036 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1mxO8a-0002oc-5k for qemu-devel@archiver.kernel.org; Wed, 15 Dec 2021 01:51:40 -0500 Received: from eggs.gnu.org ([209.51.188.92]:41660) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1mxO5r-0001P1-R8 for qemu-devel@nongnu.org; Wed, 15 Dec 2021 01:48:53 -0500 Received: from us-smtp-delivery-124.mimecast.com ([170.10.133.124]:28081) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1mxO5o-0003DB-VV for qemu-devel@nongnu.org; Wed, 15 Dec 2021 01:48:50 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1639550927; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=7hklJp3KcE1N5ripvG/57QdBheDWiyh/WzV7wLf5qQ4=; b=ZxFmyZvqUjOuJ2MsTXacwtYWbEXDIBtP8hqhjF9E09O/CMyPbouNc2m7SbotKGSIV1zqjv mWYtSxFEBK+ruYSSOlKTA4Da1k4K+c6IIkO8kG/0ElZbgNXrwbrd6t4JjDCD248UiU5pJS rNubmmRa8hEBnK5Uye22B7WtHgXLXSU= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-126-80y_BcbjMH-KqSQiFdRevA-1; Wed, 15 Dec 2021 01:48:44 -0500 X-MC-Unique: 80y_BcbjMH-KqSQiFdRevA-1 Received: from smtp.corp.redhat.com (int-mx07.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 9153A801962; Wed, 15 Dec 2021 06:48:43 +0000 (UTC) Received: from blackfin.pond.sub.org (ovpn-112-2.ams2.redhat.com [10.36.112.2]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 3C8C8109C8EB; Wed, 15 Dec 2021 06:48:08 +0000 (UTC) Received: by blackfin.pond.sub.org (Postfix, from userid 1000) id 94316113865F; Wed, 15 Dec 2021 07:48:06 +0100 (CET) From: Markus Armbruster To: Jason Wang Subject: Re: modify NetdevUserOptions through QMP in QEMU 6 - how? References: <007f7313-eeb2-ee6a-ae2e-9341324388c0@redhat.com> <20211214094355-mutt-send-email-mst@kernel.org> Date: Wed, 15 Dec 2021 07:48:06 +0100 In-Reply-To: (Jason Wang's message of "Wed, 15 Dec 2021 11:31:36 +0800") Message-ID: <87fsqunz2x.fsf@dusky.pond.sub.org> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/27.2 (gnu/linux) MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.84 on 10.5.11.22 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=armbru@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain Received-SPF: pass client-ip=170.10.133.124; envelope-from=armbru@redhat.com; helo=us-smtp-delivery-124.mimecast.com X-Spam_score_int: -34 X-Spam_score: -3.5 X-Spam_bar: --- X-Spam_report: (-3.5 / 5.0 requ) BAYES_00=-1.9, DKIMWL_WL_HIGH=-0.719, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, RCVD_IN_DNSWL_LOW=-0.7, RCVD_IN_MSPIKE_H2=-0.001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001 autolearn=unavailable autolearn_force=no X-Spam_action: no action X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Thomas Huth , Alexander Sosedkin , "Michael S. Tsirkin" , qemu-devel , qemu-discuss@nongnu.org, Samuel Thibault , =?utf-8?Q?Marc-Andr=C3=A9?= Lureau Errors-To: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Sender: "Qemu-devel" Jason Wang writes: > On Tue, Dec 14, 2021 at 10:53 PM Michael S. Tsirkin wrote: >> >> On Mon, Dec 13, 2021 at 09:02:15AM +0100, Thomas Huth wrote: >> > Hi! >> > >> > On 10/12/2021 18.02, Alexander Sosedkin wrote: >> > > With QEMU 5 I could totally issue a QMP netdev_add >> > > with the same ID to adjust the NetdevUserOptions I want, >> > > such as restrict or hostfwd. No deleting needed, >> > > just a netdev_add with what I want changed as a param. >> > >> > I'm a little bit surprised that this worked, since AFAIK there is no code in >> > QEMU to *change* the parameters of a running netdev... likely the code added >> > a new netdev with the same ID, replacing the old one? >> > >> > > With QEMU 6 it started failing, claiming the ID is already used. >> > > And if I do netdev_del + netdev_add, I just lose connectivity. >> > > What's even stranger, I still see old netdev attached in info network: >> > > >> > > > netdev_del {'id': 'net0'} >> > > {} >> > > > human-monitor-command {'command-line': 'info network'} >> > > virtio-net-pci.0: >> > > index=0,type=nic,model=virtio-net-pci,macaddr=52:54:00:12:34:56 >> > > \ net0: index=0,type=user,net=10.0.2.0,restrict=off >> > >> > I think that's "normal" - there used to be problems in the past that the >> > devices (virtio-net-pci in this case) did not like the netdevs to be removed >> > on the fly. So the netdevs are kept around until you remove the device, too >> > (i.e. issue a device_del for the virtio-net-pci device). >> > >> > > > netdev_add {'type': 'user', 'id': 'net0', 'restrict': False, 'hostfwd': [{'str': 'tcp:127.0.0.1:58239-:22'}]} >> > > {} >> > > > human-monitor-command {'command-line': 'info network'} >> > > unseal: virtio-net-pci.0: >> > > index=0,type=nic,model=virtio-net-pci,macaddr=52:54:00:12:34:56 >> > > \ net0: index=0,type=user,net=10.0.2.0,restrict=off >> > > net0: index=0,type=user,net=10.0.2.0,restrict=off >> > > >> > > What's the correct QMP command sequence to modify NetdevUserOptions? >> > >> > AFAIK there is no way to modify running netdevs - you'd have to delete the >> > netdev and the device, and then add both again. But I might have missed >> > something here, so I CC:-ed some people who might be more familiar with the >> > details here. >> > >> > Thomas >> > >> > >> > > Please CC me on replies. >> >> >> Wow this really goes to show how wide our feature matrix is. >> >> Yes it's probably an unintended side effect but yes it >> did work it seems, so we really should not just break it >> without warning. Depends. See below. >> Probably this one: >> >> commit 831734cce6494032e9233caff4d8442b3a1e7fef >> Author: Markus Armbruster >> Date: Wed Nov 25 11:02:20 2020 +0100 >> >> net: Fix handling of id in netdev_add and netdev_del CLI -netdev accumulates in option group "netdev". 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 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 and HMP netdev_del can leave QemuOpts behind, breaking HMP netdev_add. Reproducer: $ qemu-system-x86_64 -S -display none -nodefaults -monitor stdio QEMU 5.1.92 monitor - type 'help' for more information (qemu) netdev_add user,id=net0 (qemu) info network net0: index=0,type=user,net=10.0.2.0,restrict=off (qemu) netdev_del net0 (qemu) info network (qemu) netdev_add user,id=net0 upstream-qemu: Duplicate ID 'net0' for netdev Try "help netdev_add" for more information Fix by restoring the QemuOpts deletion in qmp_netdev_del(), but with a guard, because the QemuOpts need not exist. * QMP netdev_add loses its "no duplicate ID" check. Reproducer: $ qemu-system-x86_64 -S -display none -qmp stdio {"QMP": {"version": {"qemu": {"micro": 92, "minor": 1, "major": 5}, "package": "v5.2.0-rc2-1-g02c1f0142c"}, "capabilities": ["oob"]}} {"execute": "qmp_capabilities"} {"return": {}} {"execute": "netdev_add", "arguments": {"type": "user", "id":"net0"}} {"return": {}} {"execute": "netdev_add", "arguments": {"type": "user", "id":"net0"}} {"return": {}} Fix by adding a duplicate ID check to net_client_init1() to replace the lost one. The check is redundant for callers where QemuOpts still checks, i.e. for CLI and HMP. Reported-by: Andrew Melnichenko Fixes: 08712fcb851034228b61f75bd922863a984a4f60 Cc: qemu-stable@nongnu.org Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Signed-off-by: Jason Wang Both issues were regressions. Like Thomas, I'm surprised that adding a netdev with a duplicate ID changes parameters. Unintended side effect of a regression. I suspect it only ever "worked" between commit 08712fcb85 "net: Track netdevs in NetClientState rather than QemuOpt" (v5.0.0) and commit 831734cce6 "net: Fix handling of id in netdev_add and netdev_del" (v6.0.0). Got a reproducer for me so I can double-check? >> Jason, what is your take here? > > I might be wrong, but I agree with Thomas. Adding a netdev with the > same ID looks wrong, if it works, it looks like a bug. And I don't > think we support changing netdev properties. Ability to adjust backend parameters feels like a valid feature request. But we shouldn't do it by exploiting a bug's side effect. The bug may have other side effects, possibly bad ones. "ID is unique" is an invariant. Code may rely on it. We don't know what happens when we violate it. [...]