From nobody Fri Dec 19 14:28:08 2025 Delivered-To: importer@patchew.org Received-SPF: pass (zohomail.com: domain of lists.libvirt.org designates 8.43.85.245 as permitted sender) client-ip=8.43.85.245; envelope-from=devel-bounces@lists.libvirt.org; helo=lists.libvirt.org; Authentication-Results: mx.zohomail.com; dkim=fail; spf=pass (zohomail.com: domain of lists.libvirt.org designates 8.43.85.245 as permitted sender) smtp.mailfrom=devel-bounces@lists.libvirt.org; dmarc=fail(p=none dis=none) header.from=redhat.com Return-Path: Received: from lists.libvirt.org (lists.libvirt.org [8.43.85.245]) by mx.zohomail.com with SMTPS id 1726673294934602.7557486937754; Wed, 18 Sep 2024 08:28:14 -0700 (PDT) Received: by lists.libvirt.org (Postfix, from userid 996) id DD18F159A; Wed, 18 Sep 2024 11:28:13 -0400 (EDT) Received: from lists.libvirt.org (localhost [IPv6:::1]) by lists.libvirt.org (Postfix) with ESMTP id 118EE152D; Wed, 18 Sep 2024 11:26:45 -0400 (EDT) Received: by lists.libvirt.org (Postfix, from userid 996) id 316511462; Wed, 18 Sep 2024 11:26:39 -0400 (EDT) Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by lists.libvirt.org (Postfix) with ESMTPS id 8BFDB1459 for ; Wed, 18 Sep 2024 11:26:38 -0400 (EDT) Received: from mx-prod-mc-02.mail-002.prod.us-west-2.aws.redhat.com (ec2-54-186-198-63.us-west-2.compute.amazonaws.com [54.186.198.63]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-442-MLgO1LPzOxSqDrX_53b58Q-1; Wed, 18 Sep 2024 11:26:33 -0400 Received: from mx-prod-int-01.mail-002.prod.us-west-2.aws.redhat.com (mx-prod-int-01.mail-002.prod.us-west-2.aws.redhat.com [10.30.177.4]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mx-prod-mc-02.mail-002.prod.us-west-2.aws.redhat.com (Postfix) with ESMTPS id 12B2019560A7 for ; Wed, 18 Sep 2024 15:26:33 +0000 (UTC) Received: from vhost3.router.laine.org (unknown [10.22.17.190]) by mx-prod-int-01.mail-002.prod.us-west-2.aws.redhat.com (Postfix) with ESMTP id 77DC630001A1 for ; Wed, 18 Sep 2024 15:26:32 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on lists.libvirt.org X-Spam-Level: X-Spam-Status: No, score=-0.5 required=5.0 tests=DKIM_INVALID,DKIM_SIGNED, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE autolearn=unavailable autolearn_force=no version=3.4.4 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1726673198; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=xgoedzSnpQYsUlRet5JLLV5mw1ATcqaGdB6lnZaFn5A=; b=H4YjMiTwLISUBDh/j0bvTmxVREjNvYMAQ/DONo7CBHPqLGZo1hqkElwUx1+adlzoCCGd3F GI3g7PYEgi2eDWE6xURggQd9RzuTbzy8AVt10rgkD014MU+CsHqQFktQ6jhxD95pfn7V1T MFpJVkhEGcH3KWaHSofhh/Ezm6r5rC4= X-MC-Unique: MLgO1LPzOxSqDrX_53b58Q-1 From: Laine Stump To: devel@lists.libvirt.org Subject: [PATCH 1/4] qemu: prevent unnecessarily failing live interface update Date: Wed, 18 Sep 2024 11:26:27 -0400 Message-ID: <20240918152630.875195-2-laine@redhat.com> In-Reply-To: <20240918152630.875195-1-laine@redhat.com> References: <20240918152630.875195-1-laine@redhat.com> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 3.4.1 on 10.30.177.4 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Transfer-Encoding: quoted-printable Message-ID-Hash: OXWC7IZ5HY6VFO45W6SABYAAE37TFTLF X-Message-ID-Hash: OXWC7IZ5HY6VFO45W6SABYAAE37TFTLF X-MailFrom: laine@redhat.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; header-match-config-1; header-match-config-2; header-match-config-3; header-match-devel.lists.libvirt.org-0; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; suspicious-header X-Mailman-Version: 3.2.2 Precedence: list List-Id: Development discussions about the libvirt library & tools Archived-At: List-Archive: List-Help: List-Post: List-Subscribe: List-Unsubscribe: X-ZohoMail-DKIM: fail (Header signature does not verify) X-ZM-MESSAGEID: 1726673296700116600 Content-Type: text/plain; charset="utf-8"; x-default="true" Attempts to use update-device to modify just the link state of a guest interface were failing due to a supposed attempt to modify something in the interface that can't be modified live (even though the only thing that was changing was the link state, which *can* be modified live). It turned out that this failure happened because the guest interface in question was type=3D'network', and the network in question was a 'direct' network that provides each guest interface with one device from a pool of network devices. As a part of qemuDomainChangeNet() we would always allocate a new port from the network driver for the updated interface definition (by way of calling virDomainNetAllocateActualDevice(newdev)), and this new port (ie the ActualNetDef in newdev) would of course be allocated a new host device from the pool (which would of course be different from the one currently in use by the guest interface (in olddev)). Because direct interfaces don't support changing the host device in a live update, this would cause the update to fail. The solution to this is to realize that as long as the interface doesn't get switched to a different network as a part of the update, the network port information (ie the ActualNetDef) will not change as a part of updating the guest interface itself. So for sake of comparison we can just point the newdev at the ActualNetDef of olddev, and then clear out one or the other when we're done (to avoid a double free or, more likely, attempt to reference freed memory). (If, on the other hand, the name of the network has changed, or if the interface type has changed to type=3D'network' from something else, then we *do* need to allocate a new port (actual device) from the network driver (as we used to do in all cases when the new type was 'network'), and also indicate that we'll need to replace olddev in the domain with newdev (because either of these changes is major enough that we shouldn't just try to fix up olddev) Resolves: https://issues.redhat.com/browse/RHEL-7036 Signed-off-by: Laine Stump Reviewed-by: Michal Privoznik --- src/qemu/qemu_hotplug.c | 92 +++++++++++++++++++++++++++++++++++------ 1 file changed, 80 insertions(+), 12 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 75b97cf736..a187466c5b 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -3675,6 +3675,7 @@ qemuDomainChangeNet(virQEMUDriver *driver, virDomainNetDef **devslot =3D NULL; virDomainNetDef *olddev; virDomainNetType oldType, newType; + bool actualSame =3D false; bool needReconnect =3D false; bool needBridgeChange =3D false; bool needFilterChange =3D false; @@ -3895,15 +3896,49 @@ qemuDomainChangeNet(virQEMUDriver *driver, * free it if we fail for any reason */ if (newdev->type =3D=3D VIR_DOMAIN_NET_TYPE_NETWORK) { - if (!(conn =3D virGetConnectNetwork())) - goto cleanup; - if (virDomainNetAllocateActualDevice(conn, vm->def, newdev) < 0) - goto cleanup; - } + if (olddev->type =3D=3D VIR_DOMAIN_NET_TYPE_NETWORK + && STREQ(olddev->data.network.name, newdev->data.network.name)= ) { + /* old and new are type=3D'network', and the network name + * hasn't changed. In this case we *don't* want to get a + * new port ("actual device") from the network because we + * can use the old one (since it hasn't changed). + * + * So instead we just duplicate *the pointer to* the + * actualNetDef from olddev to newdev so that comparisons + * of actualNetDef will show no change. If the update is + * successful, we will clear the actualNetDef pointer from + * olddev before destroying it (or if the update fails, + * then we need to clear the pointer from newdev before + * destroying it) + */ + newdev->data.network.actual =3D olddev->data.network.actual; + memcpy(newdev->data.network.portid, olddev->data.network.porti= d, + sizeof(newdev->data.network.portid)); + actualSame =3D true; /* old and new actual are pointing to sam= e object */ + } else { + /* either the olddev wasn't type=3D'network', or else the + * name of the network changed. In this case we *do* want + * to get a new port from the new network (because we know + * that it *will* change), and then if the update is + * successful, we will release the port ("actual device") + * in olddev. Or if the update is a failure, we will + * release this new port + */ + if (!(conn =3D virGetConnectNetwork()) + || virDomainNetAllocateActualDevice(conn, vm->def, newdev)= < 0) { + goto cleanup; + } =20 - /* final validation now that we have full info on the type */ - if (qemuDomainValidateActualNetDef(newdev, priv->qemuCaps) < 0) - goto cleanup; + /* final validation now that we have full info on the type */ + if (qemuDomainValidateActualNetDef(newdev, priv->qemuCaps) < 0) + goto cleanup; + + /* since there is a new actual, we definitely will want to + * replace olddev with newdev in the domain + */ + needReplaceDevDef =3D true; + } + } =20 newType =3D virDomainNetGetActualType(newdev); =20 @@ -4169,7 +4204,21 @@ qemuDomainChangeNet(virQEMUDriver *driver, =20 /* this function doesn't work with HOSTDEV networks yet, thus * no need to change the pointer in the hostdev structure */ - if (olddev->type =3D=3D VIR_DOMAIN_NET_TYPE_NETWORK) { + if (actualSame) { + /* olddev and newdev have both been pointing at the + * same actual device object. Now that we know we're + * going to use newdev and dispose of olddev, we clear + * olddev->...actual so it doesn't get freed by upcoming + * virDomainNetDefFree(olddev) (which would be + * catastrophic because it is still being used by + * newdev) + */ + olddev->data.network.actual =3D NULL; + + } else if (olddev->type =3D=3D VIR_DOMAIN_NET_TYPE_NETWORK) { + /* olddev had a port (actual device) and we aren't + * reusing it in newdev, so we need to release it + */ if (conn || (conn =3D virGetConnectNetwork())) virDomainNetReleaseActualDevice(conn, olddev); else @@ -4211,10 +4260,29 @@ qemuDomainChangeNet(virQEMUDriver *driver, * that the changes were minor enough that we didn't need to * replace the entire device object. */ - if (newdev && newdev->type =3D=3D VIR_DOMAIN_NET_TYPE_NETWORK && conn) - virDomainNetReleaseActualDevice(conn, newdev); - virErrorRestore(&save_err); + if (newdev) { + if (actualSame) { + /* newdev->...actual was previously pointing to the + * olddev->...actual, but we've decided to free newdev and + * continue using olddev. So we need to clear + * newdev->...actual to avoid freeing the actualNetDef while + * olddev is still using it. + */ + newdev->data.network.actual =3D NULL; =20 + } else if (newdev->type =3D=3D VIR_DOMAIN_NET_TYPE_NETWORK) { + /* we had allocated a new port (actual device) for newdev, + * but now we're not going to use it, so release it back to + * the network + */ + if (conn || (conn =3D virGetConnectNetwork())) + virDomainNetReleaseActualDevice(conn, newdev); + else + VIR_WARN("Unable to release network device '%s'", NULLSTR(= newdev->ifname)); + } + } + + virErrorRestore(&save_err); return ret; } =20 --=20 2.46.0 From nobody Fri Dec 19 14:28:08 2025 Delivered-To: importer@patchew.org Received-SPF: pass (zohomail.com: domain of lists.libvirt.org designates 8.43.85.245 as permitted sender) client-ip=8.43.85.245; envelope-from=devel-bounces@lists.libvirt.org; helo=lists.libvirt.org; Authentication-Results: mx.zohomail.com; dkim=fail; spf=pass (zohomail.com: domain of lists.libvirt.org designates 8.43.85.245 as permitted sender) smtp.mailfrom=devel-bounces@lists.libvirt.org; dmarc=fail(p=none dis=none) header.from=redhat.com Return-Path: Received: from lists.libvirt.org (lists.libvirt.org [8.43.85.245]) by mx.zohomail.com with SMTPS id 1726673241160989.1077961080207; Wed, 18 Sep 2024 08:27:21 -0700 (PDT) Received: by lists.libvirt.org (Postfix, from userid 996) id 110101A84; Wed, 18 Sep 2024 11:27:20 -0400 (EDT) Received: from lists.libvirt.org (localhost [IPv6:::1]) by lists.libvirt.org (Postfix) with ESMTP id 9D94B14E4; Wed, 18 Sep 2024 11:26:40 -0400 (EDT) Received: by lists.libvirt.org (Postfix, from userid 996) id 41B82146A; Wed, 18 Sep 2024 11:26:37 -0400 (EDT) Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by lists.libvirt.org (Postfix) with ESMTPS id C4C5A1459 for ; Wed, 18 Sep 2024 11:26:36 -0400 (EDT) Received: from mx-prod-mc-04.mail-002.prod.us-west-2.aws.redhat.com (ec2-54-186-198-63.us-west-2.compute.amazonaws.com [54.186.198.63]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-149-vPs6Y2vJOp-8rgMDUuHVtg-1; Wed, 18 Sep 2024 11:26:34 -0400 Received: from mx-prod-int-01.mail-002.prod.us-west-2.aws.redhat.com (mx-prod-int-01.mail-002.prod.us-west-2.aws.redhat.com [10.30.177.4]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mx-prod-mc-04.mail-002.prod.us-west-2.aws.redhat.com (Postfix) with ESMTPS id D728C1955F42 for ; Wed, 18 Sep 2024 15:26:33 +0000 (UTC) Received: from vhost3.router.laine.org (unknown [10.22.17.190]) by mx-prod-int-01.mail-002.prod.us-west-2.aws.redhat.com (Postfix) with ESMTP id 2ED7730001A1 for ; Wed, 18 Sep 2024 15:26:33 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on lists.libvirt.org X-Spam-Level: X-Spam-Status: No, score=-0.5 required=5.0 tests=DKIM_INVALID,DKIM_SIGNED, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE autolearn=unavailable autolearn_force=no version=3.4.4 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1726673196; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=htsuMyxnrN/MFCwRVGeFcEH1YUWbgtOzT2L9zvMy74c=; b=hi1MAFqIcsm+BQaqNLo7rLQhSCEPZtRuih3JF0xFjBqnv3ZqfAMsrCzheDCBFGcAOZeV+O xPZgQo2VplZQPo6FL1WWWtZdY7jJrv6aMeSH1ye4UQagvKVW7jf5pJP+AmLadEhHawjAcV L6c5teev6cB7xge+iDvOFl9aIje2KRc= X-MC-Unique: vPs6Y2vJOp-8rgMDUuHVtg-1 From: Laine Stump To: devel@lists.libvirt.org Subject: [PATCH 2/4] util: don't return early from virNetDevTapReattachBridge() if "force" is true Date: Wed, 18 Sep 2024 11:26:28 -0400 Message-ID: <20240918152630.875195-3-laine@redhat.com> In-Reply-To: <20240918152630.875195-1-laine@redhat.com> References: <20240918152630.875195-1-laine@redhat.com> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 3.4.1 on 10.30.177.4 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Transfer-Encoding: quoted-printable Message-ID-Hash: 4AZE42RJKWOIRQ4RGQPXOTUMEVMUXR6M X-Message-ID-Hash: 4AZE42RJKWOIRQ4RGQPXOTUMEVMUXR6M X-MailFrom: laine@redhat.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; header-match-config-1; header-match-config-2; header-match-config-3; header-match-devel.lists.libvirt.org-0; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; suspicious-header X-Mailman-Version: 3.2.2 Precedence: list List-Id: Development discussions about the libvirt library & tools Archived-At: List-Archive: List-Help: List-Post: List-Subscribe: List-Unsubscribe: X-ZohoMail-DKIM: fail (Header signature does not verify) X-ZM-MESSAGEID: 1726673242166116600 Content-Type: text/plain; charset="utf-8"; x-default="true" It can be useful to force an interface to be detached/reattached from its bridge even if it's the same bridge - possibly something like the virtualport profileID has changed, and a detach/attach cycle will get it connected with the new profileID. The one and only current use of virNetDevTapReattachBridge() sets force to false, to preserve current behavior. An upcoming patch will use it with force set to true. Signed-off-by: Laine Stump Reviewed-by: Michal Privoznik --- src/conf/domain_conf.c | 2 +- src/util/virnetdevtap.c | 8 ++++++-- src/util/virnetdevtap.h | 3 ++- 3 files changed, 9 insertions(+), 4 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index cf4b1b2aef..963322f2f6 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -30630,7 +30630,7 @@ virDomainNetNotifyActualDevice(virConnectPtr conn, virDomainNetGetActualVirtP= ortProfile(iface), virDomainNetGetActualVlan(= iface), virDomainNetGetActualPortO= ptionsIsolated(iface), - iface->mtu, NULL)); + iface->mtu, NULL, false)); } } =20 diff --git a/src/util/virnetdevtap.c b/src/util/virnetdevtap.c index 3d7f680599..2701ba6dfc 100644 --- a/src/util/virnetdevtap.c +++ b/src/util/virnetdevtap.c @@ -510,6 +510,9 @@ virNetDevTapAttachBridge(const char *tapname, * @virtVlan: vlan tag info * @mtu: requested MTU for port (or 0 for "default") * @actualMTU: MTU actually set for port (after accounting for bridge's MT= U) + * @force: set true to force detach/reattach even if the bridge name is un= changed + * (this can be useful if, for example, the profileid of the + * changes) * * Ensures that the tap device (@tapname) is connected to the bridge * (@brname), potentially removing it from any existing bridge that @@ -526,7 +529,8 @@ virNetDevTapReattachBridge(const char *tapname, const virNetDevVlan *virtVlan, virTristateBool isolatedPort, unsigned int mtu, - unsigned int *actualMTU) + unsigned int *actualMTU, + bool force) { bool useOVS =3D false; g_autofree char *master =3D NULL; @@ -542,7 +546,7 @@ virNetDevTapReattachBridge(const char *tapname, } =20 /* Nothing more todo if we're on the right bridge already */ - if (STREQ_NULLABLE(brname, master)) + if (STREQ_NULLABLE(brname, master) && !force) return 0; =20 /* disconnect from current (incorrect) bridge, if any */ diff --git a/src/util/virnetdevtap.h b/src/util/virnetdevtap.h index c9d29c0384..9ebe0ee9ed 100644 --- a/src/util/virnetdevtap.h +++ b/src/util/virnetdevtap.h @@ -82,7 +82,8 @@ virNetDevTapReattachBridge(const char *tapname, const virNetDevVlan *virtVlan, virTristateBool isolatedPort, unsigned int mtu, - unsigned int *actualMTU) + unsigned int *actualMTU, + bool force) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) G_GNUC_WARN_UNUSED_RESULT; =20 --=20 2.46.0 From nobody Fri Dec 19 14:28:08 2025 Delivered-To: importer@patchew.org Received-SPF: pass (zohomail.com: domain of lists.libvirt.org designates 8.43.85.245 as permitted sender) client-ip=8.43.85.245; envelope-from=devel-bounces@lists.libvirt.org; helo=lists.libvirt.org; Authentication-Results: mx.zohomail.com; dkim=fail; spf=pass (zohomail.com: domain of lists.libvirt.org designates 8.43.85.245 as permitted sender) smtp.mailfrom=devel-bounces@lists.libvirt.org; dmarc=fail(p=none dis=none) header.from=redhat.com Return-Path: Received: from lists.libvirt.org (lists.libvirt.org [8.43.85.245]) by mx.zohomail.com with SMTPS id 1726673264074939.4927407090385; Wed, 18 Sep 2024 08:27:44 -0700 (PDT) Received: by lists.libvirt.org (Postfix, from userid 996) id 073D514C5; Wed, 18 Sep 2024 11:27:43 -0400 (EDT) Received: from lists.libvirt.org (localhost [IPv6:::1]) by lists.libvirt.org (Postfix) with ESMTP id 4087619BE; Wed, 18 Sep 2024 11:26:42 -0400 (EDT) Received: by lists.libvirt.org (Postfix, from userid 996) id 85BE71462; Wed, 18 Sep 2024 11:26:38 -0400 (EDT) Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by lists.libvirt.org (Postfix) with ESMTPS id 0DBE71462 for ; Wed, 18 Sep 2024 11:26:38 -0400 (EDT) Received: from mx-prod-mc-02.mail-002.prod.us-west-2.aws.redhat.com (ec2-54-186-198-63.us-west-2.compute.amazonaws.com [54.186.198.63]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-179-vzL7F0gbMRmJWgSwFsYLxQ-1; Wed, 18 Sep 2024 11:26:35 -0400 Received: from mx-prod-int-01.mail-002.prod.us-west-2.aws.redhat.com (mx-prod-int-01.mail-002.prod.us-west-2.aws.redhat.com [10.30.177.4]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mx-prod-mc-02.mail-002.prod.us-west-2.aws.redhat.com (Postfix) with ESMTPS id 77EAB1953943 for ; Wed, 18 Sep 2024 15:26:34 +0000 (UTC) Received: from vhost3.router.laine.org (unknown [10.22.17.190]) by mx-prod-int-01.mail-002.prod.us-west-2.aws.redhat.com (Postfix) with ESMTP id DC14F30001A1 for ; Wed, 18 Sep 2024 15:26:33 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on lists.libvirt.org X-Spam-Level: X-Spam-Status: No, score=-0.5 required=5.0 tests=DKIM_INVALID,DKIM_SIGNED, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE autolearn=unavailable autolearn_force=no version=3.4.4 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1726673197; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=kwIr/TCEjfm7Rifz+nakqjL0js37Npjtj2Rq/Qwyphg=; b=YXE5rwQ2e+zE/BUhzxZIGuj4go51dPO5LccDRqSVSaa/4g6hCLA5YdTl3o8A996bLPXxyK 4B1eRT8o47eHc3xyRh4e+Z0MjxgzqZTDrTfyU2MopR1bNzKRxIupdarfhBrqdHDINnHoi2 OTjrtVeovve0d3Vqnq3iuGigxlQ62F0= X-MC-Unique: vzL7F0gbMRmJWgSwFsYLxQ-1 From: Laine Stump To: devel@lists.libvirt.org Subject: [PATCH 3/4] qemu: replace open-coded remove/attach bridge with virNetDevTapReattachBridge() Date: Wed, 18 Sep 2024 11:26:29 -0400 Message-ID: <20240918152630.875195-4-laine@redhat.com> In-Reply-To: <20240918152630.875195-1-laine@redhat.com> References: <20240918152630.875195-1-laine@redhat.com> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 3.4.1 on 10.30.177.4 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Transfer-Encoding: quoted-printable Message-ID-Hash: Y6WBWSAAT7OFTLRNBGBCBWYI4B2BA3RI X-Message-ID-Hash: Y6WBWSAAT7OFTLRNBGBCBWYI4B2BA3RI X-MailFrom: laine@redhat.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; header-match-config-1; header-match-config-2; header-match-config-3; header-match-devel.lists.libvirt.org-0; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; suspicious-header X-Mailman-Version: 3.2.2 Precedence: list List-Id: Development discussions about the libvirt library & tools Archived-At: List-Archive: List-Help: List-Post: List-Subscribe: List-Unsubscribe: X-ZohoMail-DKIM: fail (Header signature does not verify) X-ZM-MESSAGEID: 1726673266130116600 Content-Type: text/plain; charset="utf-8"; x-default="true" The new function does what the old qemuDomainChangeNetbridge() did manually, except that: 1) the new function supports changing from a bridge of one type to another, e.g. from a Linux host bridge to an OVS bridge. (previously that wasn't handled) 2) the new function doesn't emit audit log messages. This is actually a good thing, because the old code would just log a "detach" followed immediately by "attach" for the same MAC address, so it's essentially a NOP. (the audit logs don't have any more detailed info about the connection - just the VM name and MAC address, so it makes no sense to log the detach/attach pair as it's not providing any information). Signed-off-by: Laine Stump Reviewed-by: Michal Privoznik --- src/qemu/qemu_hotplug.c | 55 ++++++++++------------------------------- 1 file changed, 13 insertions(+), 42 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index a187466c5b..4291feba29 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -55,6 +55,7 @@ #include "virnetdevbridge.h" #include "virnetdevopenvswitch.h" #include "virnetdevmidonet.h" +#include "virnetdevtap.h" #include "device_conf.h" #include "storage_source.h" #include "storage_source_conf.h" @@ -3484,7 +3485,6 @@ qemuDomainChangeNetBridge(virDomainObj *vm, virDomainNetDef *olddev, virDomainNetDef *newdev) { - int ret =3D -1; const char *oldbridge =3D virDomainNetGetActualBridgeName(olddev); const char *newbridge =3D virDomainNetGetActualBridgeName(newdev); =20 @@ -3498,50 +3498,21 @@ qemuDomainChangeNetBridge(virDomainObj *vm, =20 if (virNetDevExists(newbridge) !=3D 1) { virReportError(VIR_ERR_OPERATION_FAILED, - _("bridge %1$s doesn't exist"), newbridge); + _("cannot add domain %1$s device %2$s to nonexisten= t bridge %3$s"), + vm->def->name, newdev->ifname, newbridge); return -1; } =20 - ret =3D virNetDevBridgeRemovePort(oldbridge, olddev->ifname); - virDomainAuditNet(vm, olddev, NULL, "detach", ret =3D=3D 0); - if (ret < 0) { - /* warn but continue - possibly the old network - * had been destroyed and reconstructed, leaving the - * tap device orphaned. - */ - VIR_WARN("Unable to detach device %s from bridge %s", - olddev->ifname, oldbridge); - } - - ret =3D virNetDevBridgeAddPort(newbridge, olddev->ifname); - if (ret =3D=3D 0 && - virDomainNetGetActualPortOptionsIsolated(newdev) =3D=3D VIR_TRISTA= TE_BOOL_YES) { - - ret =3D virNetDevBridgePortSetIsolated(newbridge, olddev->ifname, = true); - if (ret < 0) { - virErrorPtr err; - - virErrorPreserveLast(&err); - ignore_value(virNetDevBridgeRemovePort(newbridge, olddev->ifna= me)); - virErrorRestore(&err); - } - } - virDomainAuditNet(vm, NULL, newdev, "attach", ret =3D=3D 0); - if (ret < 0) { - virErrorPtr err; - - virErrorPreserveLast(&err); - ret =3D virNetDevBridgeAddPort(oldbridge, olddev->ifname); - if (ret =3D=3D 0 && - virDomainNetGetActualPortOptionsIsolated(olddev) =3D=3D VIR_TR= ISTATE_BOOL_YES) { - ignore_value(virNetDevBridgePortSetIsolated(newbridge, olddev-= >ifname, true)); - } - virDomainAuditNet(vm, NULL, olddev, "attach", ret =3D=3D 0); - virErrorRestore(&err); - return -1; - } - /* caller will replace entire olddev with newdev in domain nets list */ - return 0; + /* force the detach/reattach (final arg) to make sure we pick up virtu= alport changes + * even if the bridge name hasn't changed + */ + return virNetDevTapReattachBridge(newdev->ifname, + virDomainNetGetActualBridgeName(newd= ev), + &newdev->mac, vm->def->uuid, + virDomainNetGetActualVirtPortProfile= (newdev), + virDomainNetGetActualVlan(newdev), + virDomainNetGetActualPortOptionsIsol= ated(newdev), + newdev->mtu, NULL, true); } =20 static int --=20 2.46.0 From nobody Fri Dec 19 14:28:08 2025 Delivered-To: importer@patchew.org Received-SPF: pass (zohomail.com: domain of lists.libvirt.org designates 8.43.85.245 as permitted sender) client-ip=8.43.85.245; envelope-from=devel-bounces@lists.libvirt.org; helo=lists.libvirt.org; Authentication-Results: mx.zohomail.com; dkim=fail; spf=pass (zohomail.com: domain of lists.libvirt.org designates 8.43.85.245 as permitted sender) smtp.mailfrom=devel-bounces@lists.libvirt.org; dmarc=fail(p=none dis=none) header.from=redhat.com Return-Path: Received: from lists.libvirt.org (lists.libvirt.org [8.43.85.245]) by mx.zohomail.com with SMTPS id 1726673327114310.6001380058175; Wed, 18 Sep 2024 08:28:47 -0700 (PDT) Received: by lists.libvirt.org (Postfix, from userid 996) id 135D6154D; Wed, 18 Sep 2024 11:28:46 -0400 (EDT) Received: from lists.libvirt.org (localhost [IPv6:::1]) by lists.libvirt.org (Postfix) with ESMTP id B2FB0157D; Wed, 18 Sep 2024 11:26:46 -0400 (EDT) Received: by lists.libvirt.org (Postfix, from userid 996) id B49931526; Wed, 18 Sep 2024 11:26:40 -0400 (EDT) Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by lists.libvirt.org (Postfix) with ESMTPS id C4CA81462 for ; Wed, 18 Sep 2024 11:26:39 -0400 (EDT) Received: from mx-prod-mc-02.mail-002.prod.us-west-2.aws.redhat.com (ec2-54-186-198-63.us-west-2.compute.amazonaws.com [54.186.198.63]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-340-0-oH3HaQPlW-AjIwulc2pQ-1; Wed, 18 Sep 2024 11:26:36 -0400 Received: from mx-prod-int-01.mail-002.prod.us-west-2.aws.redhat.com (mx-prod-int-01.mail-002.prod.us-west-2.aws.redhat.com [10.30.177.4]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mx-prod-mc-02.mail-002.prod.us-west-2.aws.redhat.com (Postfix) with ESMTPS id 3CF6F1953963 for ; Wed, 18 Sep 2024 15:26:35 +0000 (UTC) Received: from vhost3.router.laine.org (unknown [10.22.17.190]) by mx-prod-int-01.mail-002.prod.us-west-2.aws.redhat.com (Postfix) with ESMTP id 939DE30001A1 for ; Wed, 18 Sep 2024 15:26:34 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on lists.libvirt.org X-Spam-Level: X-Spam-Status: No, score=-0.5 required=5.0 tests=DKIM_INVALID,DKIM_SIGNED, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE autolearn=unavailable autolearn_force=no version=3.4.4 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1726673199; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=joa0fTsXCsIRAaV4NDx2KShJ9vxNVfTZGqSEcSjoqfk=; b=CkmvPQ43IgbobIhMY7+c0rcBsbN4Aq+NZy8H7pJL8OOJ8rIfjclpARhJhsJXD+i6bI6nPt YcTSm9jbDBU+bHMbN7VRPLIFMi4/DwsPPvlo2E0RAaDbFLPCrZQkXdotUZcuOj65ZZ17Uw eEoNpiksfOjSBQqs99iYSniR90JlUsw= X-MC-Unique: 0-oH3HaQPlW-AjIwulc2pQ-1 From: Laine Stump To: devel@lists.libvirt.org Subject: [PATCH 4/4] qemu: rework needBridgeChange/needReconnect decisions in qemuDomainChangeNet() Date: Wed, 18 Sep 2024 11:26:30 -0400 Message-ID: <20240918152630.875195-5-laine@redhat.com> In-Reply-To: <20240918152630.875195-1-laine@redhat.com> References: <20240918152630.875195-1-laine@redhat.com> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 3.4.1 on 10.30.177.4 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Transfer-Encoding: quoted-printable Message-ID-Hash: 3VFJWI3SJKDXRN3IDNJ3RFZFWV7JJSSY X-Message-ID-Hash: 3VFJWI3SJKDXRN3IDNJ3RFZFWV7JJSSY X-MailFrom: laine@redhat.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; header-match-config-1; header-match-config-2; header-match-config-3; header-match-devel.lists.libvirt.org-0; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; suspicious-header X-Mailman-Version: 3.2.2 Precedence: list List-Id: Development discussions about the libvirt library & tools Archived-At: List-Archive: List-Help: List-Post: List-Subscribe: List-Unsubscribe: X-ZohoMail-DKIM: fail (Header signature does not verify) X-ZM-MESSAGEID: 1726673328583116600 Content-Type: text/plain; charset="utf-8"; x-default="true" This patch simplifies (?) the of qemuDomainChangeNet() code while fixing some incorrect decisions about exactly when it's necessary to re-attach an interface's bridge device, or to fail the device update (needReconnect[*]) because the type of connection has changed (or within bridge and direct (macvtap) type because some attribute of the connection has changed that can't actually be modified after the tap/macvtap device of the interface is created). Example 1: it's pointless to require the bridge device to be reattached just because the interface has been switched to a different network (i.e. the name of the network is different), since the new network could be using the same bridge as the old network (very uncommon, but technically possible). Instead we should only care if the name of the *bridge device* changes (or if something in changes - see Example 3). Example 2: wrt changing the "type" of the interface, a change should be allowed if old and new type both used a bridge device (whether or not the name of the bridge changes), or if old and new type are both "direct" *and* the device being linked and macvtap mode remain the same. Any other change in interface type cannot be accommodated and should be a failure (i.e. needReconnect). Example 3: there is no valid reason to fail just because the interface has a element - the could just say "type=3D'openvswitch'" in both the before and after cases (in which case it isn't a change by itself, and so is completely acceptable), and even if the interfaceid changes, or the disappears completely, that can still be reconciled by simply re-attaching the bridge device. (If, on the other hand, the modified is for a type=3D'direct' interface, we can't domodify that, and so must fail (needReconnect).) (I tried splitting this into multiple patches, but they were so intertwined that the intermediate patches made no sense.) [*] "needReconnect" was a flag added to this function way back in 2012, when I still believed that QEMU might someday support connecting a new & different device backend (the way the virtual device connects to the host) to an already existing guest netdev (the virtual device as it appears to the guest). Sadly that has never happened, so for the purposes of qemuDOmainChangeNet() "needReconnect" is equivalent to "fail". Signed-off-by: Laine Stump Reviewed-by: Michal Privoznik --- src/qemu/qemu_hotplug.c | 116 ++++++++++++++++++++++++---------------- 1 file changed, 70 insertions(+), 46 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 4291feba29..74ca704927 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -3646,6 +3646,8 @@ qemuDomainChangeNet(virQEMUDriver *driver, virDomainNetDef **devslot =3D NULL; virDomainNetDef *olddev; virDomainNetType oldType, newType; + const char *oldBridgeName =3D NULL; + const char *newBridgeName =3D NULL; bool actualSame =3D false; bool needReconnect =3D false; bool needBridgeChange =3D false; @@ -3913,6 +3915,9 @@ qemuDomainChangeNet(virQEMUDriver *driver, =20 newType =3D virDomainNetGetActualType(newdev); =20 + oldBridgeName =3D virDomainNetGetActualBridgeName(olddev); + newBridgeName =3D virDomainNetGetActualBridgeName(newdev); + if (newType =3D=3D VIR_DOMAIN_NET_TYPE_HOSTDEV || newType =3D=3D VIR_DOMAIN_NET_TYPE_VDPA) { /* can't turn it into a type=3D'hostdev' or type=3D'vdpa' interfac= e */ @@ -3944,13 +3949,6 @@ qemuDomainChangeNet(virQEMUDriver *driver, break; =20 case VIR_DOMAIN_NET_TYPE_NETWORK: - if (STRNEQ(olddev->data.network.name, newdev->data.network.nam= e)) { - if (virDomainNetGetActualVirtPortProfile(newdev)) - needReconnect =3D true; - else - needBridgeChange =3D true; - } - if (STRNEQ_NULLABLE(olddev->data.network.portgroup, newdev->data.network.portgroup)) { virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", @@ -3991,59 +3989,85 @@ qemuDomainChangeNet(virQEMUDriver *driver, goto cleanup; } } else { - /* interface type has changed. There are a few special cases - * where this can only require a minor (or even no) change, - * but in most cases we need to do a full reconnection. + /* The interface type has changed. The only times when this + * wouldn't *always* require completely recreating the backend + * of the netdev (aka needReconnect, which QEMU doesn't + * support anyway) are: + * + * 1) if oldType and newType are both either _NETWORK or + * _BRIDGE (because both of those end up connecting the tap + * device to a bridge, and that is something that *can* be + * redone without recreating the backend (and will be + * handled below where needBridgeChange is set). * - * As long as both the new and old types use a tap device - * connected to a host bridge (ie VIR_DOMAIN_NET_TYPE_NETWORK - * or VIR_DOMAIN_NET_TYPE_BRIDGE), we just need to connect to - * the new bridge. + * (NB: if either of these is _NETWORK or _BRIDGE, the + * corresponding oldBridgeName/newBridgeName will be + * non-null - this is simpler to check for than checking + * each for both _NETWORK and _BRIDGE) + * + * 2) if oldType and newType are both _DIRECT (and presumably + * will end up specifying the same link device, which is + * checked further down where ActualDirectDev is checked) + * + * These two cases we'll allow through (for further checks + * below)... */ - if ((oldType =3D=3D VIR_DOMAIN_NET_TYPE_NETWORK || - oldType =3D=3D VIR_DOMAIN_NET_TYPE_BRIDGE) && - (newType =3D=3D VIR_DOMAIN_NET_TYPE_NETWORK || - newType =3D=3D VIR_DOMAIN_NET_TYPE_BRIDGE)) { - - needBridgeChange =3D true; + if (!((oldBridgeName && newBridgeName) + || (oldType =3D=3D VIR_DOMAIN_NET_TYPE_DIRECT && + newType =3D=3D VIR_DOMAIN_NET_TYPE_DIRECT))) { + + /* ...for all other combinations, we need a full reconnect + * (which currently isn't, and perhaps probably never will + * be, supported by QEMU, so needReconnect is effectively + * "NOT SUPPORTED") + */ + needReconnect =3D true; + } =20 - } else if (oldType =3D=3D VIR_DOMAIN_NET_TYPE_DIRECT && - newType =3D=3D VIR_DOMAIN_NET_TYPE_DIRECT) { + /* whatever else is done, when the interface type has changed, + * we need to replace olddev with newdev + */ + needReplaceDevDef =3D true; + } =20 - /* this is the case of switching from type=3D'direct' to - * type=3D'network' for a network that itself uses direct - * (macvtap) devices. If the physical device and mode are - * the same, this doesn't require any actual setup - * change. If the physical device or mode *does* change, - * that will be caught in the common section below */ + /* tests that need to be done whether or not type or actualType change= s */ =20 - } else { + /* if both new and old use a bridge device */ + if (newBridgeName) { =20 - /* for all other combinations, we'll need a full reconnect */ - needReconnect =3D true; + if (STRNEQ_NULLABLE(oldBridgeName, newBridgeName)) + needBridgeChange =3D true; =20 + /* A change in virtportprofile also indicates we probably need + * to re-attach the bridge, e.g. if the profileid or type + * changed. + */ + if (!virNetDevVPortProfileEqual(virDomainNetGetActualVirtPortProfi= le(olddev), + virDomainNetGetActualVirtPortProfi= le(newdev))) { + needBridgeChange =3D true; } } =20 - /* now several things that are in multiple (but not all) - * different types, and can be safely compared even for those - * cases where they don't apply to a particular type. + /* if the newType is DIRECT then we've already set needReconnect + * if oldType was anything other than DIRECT. We also need to set + * it if the direct mode or anything in the virtportprofile has + * changed. */ - if (STRNEQ_NULLABLE(virDomainNetGetActualBridgeName(olddev), - virDomainNetGetActualBridgeName(newdev))) { - if (virDomainNetGetActualVirtPortProfile(newdev)) + if (newType =3D=3D VIR_DOMAIN_NET_TYPE_DIRECT) { + if (STRNEQ_NULLABLE(virDomainNetGetActualDirectDev(olddev), + virDomainNetGetActualDirectDev(newdev)) || + virDomainNetGetActualDirectMode(olddev) !=3D virDomainNetGetAc= tualDirectMode(newdev) || + !virNetDevVPortProfileEqual(virDomainNetGetActualVirtPortProfi= le(olddev), + virDomainNetGetActualVirtPortProfi= le(newdev))) { + /* you really can't change much about a macvtap device once it= 's been created */ needReconnect =3D true; - else - needBridgeChange =3D true; + } } =20 - if (STRNEQ_NULLABLE(virDomainNetGetActualDirectDev(olddev), - virDomainNetGetActualDirectDev(newdev)) || - virDomainNetGetActualDirectMode(olddev) !=3D virDomainNetGetActual= DirectMode(newdev) || - !virNetDevVPortProfileEqual(virDomainNetGetActualVirtPortProfile(o= lddev), - virDomainNetGetActualVirtPortProfile(n= ewdev))) { - needReconnect =3D true; - } + /* now several things that are in multiple (but not all) different + * types, and can be safely compared even for those cases where + * they don't apply to a particular type. + */ =20 if (!virNetDevVlanEqual(virDomainNetGetActualVlan(olddev), virDomainNetGetActualVlan(newdev))) { --=20 2.46.0