From nobody Sun May 19 16:27:54 2024 Delivered-To: importer@patchew.org Received-SPF: pass (zohomail.com: domain of redhat.com designates 170.10.129.124 as permitted sender) client-ip=170.10.129.124; envelope-from=libvir-list-bounces@redhat.com; helo=us-smtp-delivery-124.mimecast.com; Authentication-Results: mx.zohomail.com; dkim=pass; spf=pass (zohomail.com: domain of redhat.com designates 170.10.129.124 as permitted sender) smtp.mailfrom=libvir-list-bounces@redhat.com; dmarc=pass(p=none dis=none) header.from=redhat.com ARC-Seal: i=1; a=rsa-sha256; t=1638329881; cv=none; d=zohomail.com; s=zohoarc; b=DaRioKqrx91xbbyL8FSPb+3ACzVdQQN4SnnZgG+Hg94vB57vhU5nnIeNrRKCG8ztQz5UHAdZluXr7pZuxwm+EDdHMQptV4LHoVIuXnZwMHhpVpQPrSf3LUcoGdGEBiHrD7WIQT26Nady64Y3zUPTvo2u33Ocy1VPQCUSgFKN6D4= ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=zohomail.com; s=zohoarc; t=1638329881; h=Content-Type:Content-Transfer-Encoding:Cc:Date:From:List-Subscribe:List-Post:List-Id:List-Archive:List-Help:List-Unsubscribe:MIME-Version:Message-ID:Sender:Subject:To; bh=4oHNuxXH2jmPq56b2ZiA6n2R4w5vFLGrOnrcdxkdubo=; b=dVHEh+Gge3JOel0ijYXNFhjOM2xEBC6taUt1x50CJykNVo37dKamdyK0/4sxgi/4tg2BnVDF7G5STNre28t2DMgBWs/BlLvd136nlNWmeN9/DY6Xwws+9aV40Ai3qezUijIRD3q7DOuLKbTof6J6CQRHr8zfEJilFstLGgRKb18= ARC-Authentication-Results: i=1; mx.zohomail.com; dkim=pass; spf=pass (zohomail.com: domain of redhat.com designates 170.10.129.124 as permitted sender) smtp.mailfrom=libvir-list-bounces@redhat.com; dmarc=pass header.from= (p=none dis=none) Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by mx.zohomail.com with SMTPS id 1638329881611665.1210754490127; Tue, 30 Nov 2021 19:38:01 -0800 (PST) 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-457-X0HA9_aYOg2ntoy9BJzmvw-1; Tue, 30 Nov 2021 22:37:58 -0500 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 5004C190A7A1; Wed, 1 Dec 2021 03:37:52 +0000 (UTC) Received: from colo-mx.corp.redhat.com (colo-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.21]) by smtp.corp.redhat.com (Postfix) with ESMTPS id D339410016F7; Wed, 1 Dec 2021 03:37:50 +0000 (UTC) Received: from lists01.pubmisc.prod.ext.phx2.redhat.com (lists01.pubmisc.prod.ext.phx2.redhat.com [10.5.19.33]) by colo-mx.corp.redhat.com (Postfix) with ESMTP id B15C44BB7C; Wed, 1 Dec 2021 03:37:46 +0000 (UTC) Received: from smtp.corp.redhat.com (int-mx07.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by lists01.pubmisc.prod.ext.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id 1B13bjTR003173 for ; Tue, 30 Nov 2021 22:37:45 -0500 Received: by smtp.corp.redhat.com (Postfix) id 160F210016F7; Wed, 1 Dec 2021 03:37:45 +0000 (UTC) Received: from vhost2.laine.org (ovpn-112-23.phx2.redhat.com [10.3.112.23]) by smtp.corp.redhat.com (Postfix) with ESMTP id B8D351002388; Wed, 1 Dec 2021 03:37:38 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1638329879; h=from:from:sender:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type: content-transfer-encoding:content-transfer-encoding:list-id:list-help: list-unsubscribe:list-subscribe:list-post; bh=4oHNuxXH2jmPq56b2ZiA6n2R4w5vFLGrOnrcdxkdubo=; b=KgnFEhmLLadEk5fVb4ceDxa6Sk76UEtEsAcR8PIxYOX6HeNz+i9XpTsBXyRHBbE3nlR5kl cpIUcsXet6wV4U72Wu58iLZz00l3s/+pWE2TiEgnubDUSKJWddJsl89AMaOwI69kRy0nW1 wKoVMbHbJFfBCLl9M0WkUmms8qoIyH4= X-MC-Unique: X0HA9_aYOg2ntoy9BJzmvw-1 From: Laine Stump To: libvir-list@redhat.com Subject: [PATCH] util: fix erroneous requirement for phys_port_id to get ifname of a VF Date: Tue, 30 Nov 2021 22:37:37 -0500 Message-Id: <20211201033737.3986842-1-laine@redhat.com> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.84 on 10.5.11.22 X-loop: libvir-list@redhat.com Cc: Peter Krempa X-BeenThere: libvir-list@redhat.com X-Mailman-Version: 2.1.12 Precedence: junk List-Id: Development discussions about the libvirt library & tools List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: libvir-list-bounces@redhat.com Errors-To: libvir-list-bounces@redhat.com X-Scanned-By: MIMEDefang 2.84 on 10.5.11.22 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=libvir-list-bounces@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Transfer-Encoding: quoted-printable X-ZohoMail-DKIM: pass (identity @redhat.com) X-ZM-MESSAGEID: 1638329883475100001 Content-Type: text/plain; charset="utf-8" Commit 795e9e05c3 (libvirt-7.7.0) refactored the code in virpci.c and virnetdev.c that gathered lists of the Virtual Functions (VF) of an SRIOV Physical Function (PF) to simplify the code. Unfortunately the simplification made the assumption that a VF's netdev interface name should only be retrieved if the PF had a valid phys_port_id. That is an incorrect assumption - only a small handful of (now previous-generation) Mellanox SRIOV cards actually use phys_port_id (this is for an odd design where there are multiple physical network ports on a single PCI address); all other SRIOV cards (including new Mellanox cards) have a file in sysfs called phys_port_id, but it can't be read, and so the pfPhysPortID string is NULL. The result of this logic error is that virtual networks that are a pool of VFs to be used for macvtap connections will be unable to start, giving an errror like this: VF 0 of SRIOV PF enp130s0f0 couldn't be added to the interface pool becaus= e it isn't bound to a network driver - possibly in use elsewhere The simple/quick solution to this is to not imply that "pfPhysPortID =3D=3D NULL" is the same as "don't fill in the VF's netdev ifname". Instead, add a bool getIfName to the args of virNetDevGetVirtualFunctionsFull() so that we can still get the ifname filled in when pfPhysPortID is NULL. Resolves: https://bugzilla.redhat.com/2025432 Fixes: 795e9e05c3b6b9ef3abe6f6078a6373a136ec23b Signed-off-by: Laine Stump --- On one hand this is a regression with an apparently simple fix (that has been tested to solve the problem), and it's always good to fix regressions before a release rather than after. On the other hand it has been broken since August, and nobody complained until last week (and that was a QE tester, not an actual end-user), so it seems the bug is in functionality that isn't used much in the field (or at least no downstream with a used of the functionality has made a release since then that was installed by said user). I've grown increasingly wary of pushing anything just before a release, especially when it modifies the args of a function that has multiple definitions for different platforms (although CI is supposed to be thorough enough to catch those types of problems these days). So I'm all for pushing this right after the release, rather than before. But if anyone sees a reason for doing otherwise, we can talk about it in about 10 hours when I sit down at the keyboard again :-). P.S. I'm planning to make a followup that eliminates the pfPhysPortID arg completely, but wanted the bugfix to be as stripped down as possible. src/util/virnetdev.c | 2 +- src/util/virpci.c | 20 ++++++++++++-------- src/util/virpci.h | 1 + 3 files changed, 14 insertions(+), 9 deletions(-) diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index 58f7360a0f..300d7e4f45 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -1231,7 +1231,7 @@ virNetDevGetVirtualFunctions(const char *pfname, if (virNetDevSysfsFile(&pf_sysfs_device_link, pfname, "device") < 0) return -1; =20 - if (virPCIGetVirtualFunctionsFull(pf_sysfs_device_link, vfs, pfPhysPor= tID) < 0) + if (virPCIGetVirtualFunctionsFull(pf_sysfs_device_link, true, vfs, pfP= hysPortID) < 0) return -1; =20 return 0; diff --git a/src/util/virpci.c b/src/util/virpci.c index 2d12e28004..b7b987dd63 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -2263,7 +2263,7 @@ int virPCIGetVirtualFunctions(const char *sysfs_path, virPCIVirtualFunctionList **vfs) { - return virPCIGetVirtualFunctionsFull(sysfs_path, vfs, NULL); + return virPCIGetVirtualFunctionsFull(sysfs_path, false, vfs, NULL); } =20 =20 @@ -2339,15 +2339,18 @@ virPCIGetPhysicalFunction(const char *vf_sysfs_path, /** * virPCIGetVirtualFunctionsFull: * @sysfs_path: path to physical function sysfs entry + * @getIfName: true if the ifname of the VF should also be filled in, + * false to only fill in the PCI address. * @vfs: filled with the virtual function data - * @pfPhysPortID: Optional physical port id. If provided the network inter= face - * name of the VFs is queried too. + * @pfPhysPortID: Optional physical port id. if non-null it will be used + * when determining the ifname. * * * Returns virtual functions of a physical function. */ int virPCIGetVirtualFunctionsFull(const char *sysfs_path, + bool getIfName, virPCIVirtualFunctionList **vfs, const char *pfPhysPortID) { @@ -2390,11 +2393,10 @@ virPCIGetVirtualFunctionsFull(const char *sysfs_pat= h, return -1; } =20 - if (pfPhysPortID) { - if (virPCIGetNetName(device_link, 0, pfPhysPortID, &fnc.ifname= ) < 0) { - g_free(fnc.addr); - return -1; - } + if (getIfName && + virPCIGetNetName(device_link, 0, pfPhysPortID, &fnc.ifname) < = 0) { + g_free(fnc.addr); + return -1; } =20 VIR_APPEND_ELEMENT(list->functions, list->nfunctions, fnc); @@ -2711,8 +2713,10 @@ virPCIGetPhysicalFunction(const char *vf_sysfs_path = G_GNUC_UNUSED, =20 int virPCIGetVirtualFunctionsFull(const char *sysfs_path G_GNUC_UNUSED, + bool getIfName G_GNUC_UNUSED, virPCIVirtualFunctionList **vfs G_GNUC_UNUSE= D, const char *pfPhysPortID G_GNUC_UNUSED) + { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _(unsupported)); return -1; diff --git a/src/util/virpci.h b/src/util/virpci.h index 3346321ec9..2f64c447cc 100644 --- a/src/util/virpci.h +++ b/src/util/virpci.h @@ -229,6 +229,7 @@ void virPCIVirtualFunctionListFree(virPCIVirtualFunctio= nList *list); G_DEFINE_AUTOPTR_CLEANUP_FUNC(virPCIVirtualFunctionList, virPCIVirtualFunc= tionListFree); =20 int virPCIGetVirtualFunctionsFull(const char *sysfs_path, + bool getIfName, virPCIVirtualFunctionList **vfs, const char *pfPhysPortID); int virPCIGetVirtualFunctions(const char *sysfs_path, --=20 2.33.1