From nobody Fri May 16 05:42:21 2025 Delivered-To: importer@patchew.org Received-SPF: pass (zoho.com: domain of redhat.com designates 209.132.183.28 as permitted sender) client-ip=209.132.183.28; envelope-from=libvir-list-bounces@redhat.com; helo=mx1.redhat.com; Authentication-Results: mx.zohomail.com; spf=pass (zoho.com: domain of redhat.com designates 209.132.183.28 as permitted sender) smtp.mailfrom=libvir-list-bounces@redhat.com Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by mx.zohomail.com with SMTPS id 1502415785948403.1050997729169; Thu, 10 Aug 2017 18:43:05 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id ED84BC07D1C4; Fri, 11 Aug 2017 01:43:03 +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 BB90AA1033; Fri, 11 Aug 2017 01:43:03 +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 5761A388C; Fri, 11 Aug 2017 01:43:03 +0000 (UTC) Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) by lists01.pubmisc.prod.ext.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id v7B1gZpl003177 for ; Thu, 10 Aug 2017 21:42:35 -0400 Received: by smtp.corp.redhat.com (Postfix) id 220689814B; Fri, 11 Aug 2017 01:42:35 +0000 (UTC) Received: from vhost2.laine.org (ovpn-117-36.phx2.redhat.com [10.3.117.36]) by smtp.corp.redhat.com (Postfix) with ESMTP id A318B71C33; Fri, 11 Aug 2017 01:42:34 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com ED84BC07D1C4 Authentication-Results: ext-mx08.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=laine.org Authentication-Results: ext-mx08.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=libvir-list-bounces@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com ED84BC07D1C4 From: Laine Stump To: libvir-list@redhat.com Date: Thu, 10 Aug 2017 21:42:21 -0400 Message-Id: <20170811014222.29548-7-laine@laine.org> In-Reply-To: <20170811014222.29548-1-laine@laine.org> References: <20170811014222.29548-1-laine@laine.org> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.16 X-loop: libvir-list@redhat.com Cc: mprivozn@redhat.com, moshele@mellanox.com Subject: [libvirt] [PATCH v2 6/7] util: restructure virNetDevReadNetConfig() to eliminate false error logs 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: , MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable Sender: libvir-list-bounces@redhat.com Errors-To: libvir-list-bounces@redhat.com X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.32]); Fri, 11 Aug 2017 01:43:05 +0000 (UTC) X-ZohoMail: RSF_0 Z_629925259 SPT_0 Content-Type: text/plain; charset="utf-8" virHostdevRestoreNetConfig() calls virNetDevReadNetConfig() to try and read the "original config" of a netdev, and if that fails, it tries again with a different directory/netdev name. This achieves the desired effect (we end up finding the config wherever it may be), but for each failure, virNetDevReadNetConfig() places a nice error message in the system logs. Experience has shown that false-positive error logs like this lead to erroneous bug reports, and can often mislead those searching for *real* bugs. This patch changes virNetDevReadNetConfig() to explicitly check if the file exists before calling virFileReadAll(); if it doesn't exist, virNetDevReadNetConfig() returns a success, but leaves all the variables holding the results as NULL. (This makes sense if you define the purpose of the function as "read a netdev's config from its config file *if that file exists*). To take advantage of that change, the caller, virHostdevRestoreNetConfig() is modified to fail immediately if virNetDevReadNetConfig() returns an error, and otherwise to try the different directory/netdev name if adminMAC & vlan & MAC are all NULL after the preceding attempt. --- New in V2. src/util/virhostdev.c | 126 ++++++++++++++++++++++++++++------------= ---- src/util/virnetdev.c | 16 ++++-- src/util/virnetdevmacvlan.c | 5 +- 3 files changed, 96 insertions(+), 51 deletions(-) diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c index 102fd85c1..cca9d81a4 100644 --- a/src/util/virhostdev.c +++ b/src/util/virhostdev.c @@ -534,6 +534,10 @@ virHostdevRestoreNetConfig(virDomainHostdevDefPtr host= dev, int ret =3D -1; int vf =3D -1; bool port_profile_associate =3D false; + virMacAddrPtr MAC =3D NULL; + virMacAddrPtr adminMAC =3D NULL; + virNetDevVlanPtr vlan =3D NULL; + =20 /* This is only needed for PCI devices that have been defined * using . For all others, it is a NOP. @@ -559,62 +563,92 @@ virHostdevRestoreNetConfig(virDomainHostdevDefPtr hos= tdev, NULL, port_profile_associate); } else { - virMacAddrPtr MAC =3D NULL; - virMacAddrPtr adminMAC =3D NULL; - virNetDevVlanPtr vlan =3D NULL; - - ret =3D virNetDevReadNetConfig(linkdev, vf, stateDir, &adminMAC, &= vlan, &MAC); - if (ret < 0 && oldStateDir) - ret =3D virNetDevReadNetConfig(linkdev, vf, oldStateDir, - &adminMAC, &vlan, &MAC); - - if (ret < 0) { - /* see if the config was saved using the PF's "port 2" - * netdev for the file name. - */ - VIR_FREE(linkdev); + /* we need to try 3 different places for the config file: + * 1) ${stateDir}/${PF}_vf${vf} + * This is almost always where the saved config is + * + * 2) ${oldStateDir/${PF}_vf${vf} + * saved config is only here if this machine was running a + * (by now *very*) old version of libvirt that saved the + * file in a different directory + * + * 3) ${stateDir}${PF[1]}_vf${VF} + * PF[1] means "the netdev for port 2 of the PF device", and + * is only valid when the PF is a Mellanox dual port NIC with + * a VF that was created in "single port" mode. + * + * NB: if virNetDevReadNetConfig() returns < 0, then it found + * the file, but there was a problem, so we should + * immediately return an error to our caller. If it returns + * 0, but all of the interesting stuff is NULL, that means + * the file wasn't found, so we can/should check other + * locations for it. + */ =20 - if (virHostdevNetDevice(hostdev, 1, &linkdev, &vf) >=3D 0) { - ret =3D virNetDevReadNetConfig(linkdev, vf, stateDir, - &adminMAC, &vlan, &MAC); - } + /* 1) standard location */ + if (virNetDevReadNetConfig(linkdev, vf, stateDir, + &adminMAC, &vlan, &MAC) < 0) { + goto cleanup; } =20 - if (ret =3D=3D 0) { - /* if a MAC was stored for the VF, we should now restore - * that as the adminMAC. We have to do it this way because - * the VF is still not bound to the host's net driver, so - * we can't directly set its MAC (and even after it is - * re-bound to the host net driver, it will still have its - * "administratively set" flag on, and that prohibits the - * VF's net driver from directly setting the MAC - * anyway). But it we set the desired VF MAC as the "admin - * MAC" *now*, then when the VF is re-bound to the host - * net driver (which will happen soon after returning from - * this function), that adminMAC will be set (by the PF) - * as the VF's new initial MAC. - * - * If no MAC was stored for the VF, that means it wasn't - * bound to a net driver before we used it anyway, so the - * adminMAC is all we have, and we can just restore it - * directly. - */ - if (MAC) { - VIR_FREE(adminMAC); - adminMAC =3D MAC; - MAC =3D NULL; + /* 2) "old" (pre-1.2.3 circa 2014) location - whenever we get + * to the point that nobody will ever upgrade directly from + * 1.2.3 (or older) directly to current libvirt, we can + * eliminate this clause + **/ + if (!(adminMAC || vlan || MAC) && oldStateDir && + virNetDevReadNetConfig(linkdev, vf, oldStateDir, + &adminMAC, &vlan, &MAC) < 0) { + goto cleanup; + } + + /* 3) try using the PF's "port 2" netdev as the name of the + * config file + */ + if (!(adminMAC || vlan || MAC)) { + VIR_FREE(linkdev); + + if (virHostdevNetDevice(hostdev, 1, &linkdev, &vf) < 0 || + virNetDevReadNetConfig(linkdev, vf, stateDir, + &adminMAC, &vlan, &MAC) < 0) { + goto cleanup; } + } =20 - ignore_value(virNetDevSetNetConfig(linkdev, vf, - adminMAC, vlan, MAC, true)); + /* if a MAC was stored for the VF, we should now restore + * that as the adminMAC. We have to do it this way because + * the VF is still not bound to the host's net driver, so + * we can't directly set its MAC (and even after it is + * re-bound to the host net driver, it will still have its + * "administratively set" flag on, and that prohibits the + * VF's net driver from directly setting the MAC + * anyway). But it we set the desired VF MAC as the "admin + * MAC" *now*, then when the VF is re-bound to the host + * net driver (which will happen soon after returning from + * this function), that adminMAC will be set (by the PF) + * as the VF's new initial MAC. + * + * If no MAC was stored for the VF, that means it wasn't + * bound to a net driver before we used it anyway, so the + * adminMAC is all we have, and we can just restore it + * directly. + */ + if (MAC) { + VIR_FREE(adminMAC); + adminMAC =3D MAC; + MAC =3D NULL; } =20 - VIR_FREE(MAC); - VIR_FREE(adminMAC); - virNetDevVlanFree(vlan); + ignore_value(virNetDevSetNetConfig(linkdev, vf, + adminMAC, vlan, MAC, true)); } =20 + ret =3D 0; + cleanup: VIR_FREE(linkdev); + VIR_FREE(MAC); + VIR_FREE(adminMAC); + virNetDevVlanFree(vlan); =20 return ret; } diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index 5738a8936..8fc643c93 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -1971,7 +1971,9 @@ virNetDevSaveNetConfig(const char *linkdev, int vf, * @linkdev:@vf from a file in @stateDir. (see virNetDevSaveNetConfig * for details of file name and format). * - * Returns 0 on success, -1 on failure. + * Returns 0 on success, -1 on failure. It is *NOT* considered failure + * if no file is found to read. In that case, adminMAC, vlan, and MAC + * are set to NULL, and success is returned. * * The caller MUST free adminMAC, vlan, and MAC when it is finished * with them (they will be NULL if they weren't found in the file) @@ -2036,8 +2038,8 @@ virNetDevReadNetConfig(const char *linkdev, int vf, if (linkdev && !virFileExists(filePath)) { /* the device may have been stored in a file named for the * VF due to saveVlan =3D=3D false (or an older version of - * libvirt), so reset filePath so we'll try the other - * filename before failing. + * libvirt), so reset filePath and pfDevName so we'll try + * the other filename. */ VIR_FREE(filePath); pfDevName =3D NULL; @@ -2049,6 +2051,14 @@ virNetDevReadNetConfig(const char *linkdev, int vf, goto cleanup; } =20 + if (!virFileExists(filePath)) { + /* having no file to read is not necessarily an error, so we + * just return success, but with MAC, adminMAC, and vlan set to NU= LL + */ + ret =3D 0; + goto cleanup; + } + if (virFileReadAll(filePath, 128, &fileStr) < 0) goto cleanup; =20 diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c index 97c87701c..fb41bf934 100644 --- a/src/util/virnetdevmacvlan.c +++ b/src/util/virnetdevmacvlan.c @@ -1187,8 +1187,9 @@ int virNetDevMacVLanDeleteWithVPortProfile(const char= *ifname, virMacAddrPtr adminMAC =3D NULL; virNetDevVlanPtr vlan =3D NULL; =20 - if (virNetDevReadNetConfig(linkdev, -1, stateDir, - &adminMAC, &vlan, &MAC) =3D=3D 0) { + if ((virNetDevReadNetConfig(linkdev, -1, stateDir, + &adminMAC, &vlan, &MAC) =3D=3D 0) && + (adminMAC || vlan || MAC)) { =20 ignore_value(virNetDevSetNetConfig(linkdev, -1, adminMAC, vlan, MAC, !!vlan= )); --=20 2.13.3 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list