[libvirt] [PATCH v2 6/7] util: restructure virNetDevReadNetConfig() to eliminate false error logs
Laine Stump
laine at laine.org
Fri Aug 11 01:42:21 UTC 2017
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 hostdev,
int ret = -1;
int vf = -1;
bool port_profile_associate = false;
+ virMacAddrPtr MAC = NULL;
+ virMacAddrPtr adminMAC = NULL;
+ virNetDevVlanPtr vlan = NULL;
+
/* This is only needed for PCI devices that have been defined
* using <interface type='hostdev'>. For all others, it is a NOP.
@@ -559,62 +563,92 @@ virHostdevRestoreNetConfig(virDomainHostdevDefPtr hostdev,
NULL,
port_profile_associate);
} else {
- virMacAddrPtr MAC = NULL;
- virMacAddrPtr adminMAC = NULL;
- virNetDevVlanPtr vlan = NULL;
-
- ret = virNetDevReadNetConfig(linkdev, vf, stateDir, &adminMAC, &vlan, &MAC);
- if (ret < 0 && oldStateDir)
- ret = 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.
+ */
- if (virHostdevNetDevice(hostdev, 1, &linkdev, &vf) >= 0) {
- ret = virNetDevReadNetConfig(linkdev, vf, stateDir,
- &adminMAC, &vlan, &MAC);
- }
+ /* 1) standard location */
+ if (virNetDevReadNetConfig(linkdev, vf, stateDir,
+ &adminMAC, &vlan, &MAC) < 0) {
+ goto cleanup;
}
- if (ret == 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 = MAC;
- MAC = 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;
}
+ }
- 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 = MAC;
+ MAC = NULL;
}
- VIR_FREE(MAC);
- VIR_FREE(adminMAC);
- virNetDevVlanFree(vlan);
+ ignore_value(virNetDevSetNetConfig(linkdev, vf,
+ adminMAC, vlan, MAC, true));
}
+ ret = 0;
+ cleanup:
VIR_FREE(linkdev);
+ VIR_FREE(MAC);
+ VIR_FREE(adminMAC);
+ virNetDevVlanFree(vlan);
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 == 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 = NULL;
@@ -2049,6 +2051,14 @@ virNetDevReadNetConfig(const char *linkdev, int vf,
goto cleanup;
}
+ 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 NULL
+ */
+ ret = 0;
+ goto cleanup;
+ }
+
if (virFileReadAll(filePath, 128, &fileStr) < 0)
goto cleanup;
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 = NULL;
virNetDevVlanPtr vlan = NULL;
- if (virNetDevReadNetConfig(linkdev, -1, stateDir,
- &adminMAC, &vlan, &MAC) == 0) {
+ if ((virNetDevReadNetConfig(linkdev, -1, stateDir,
+ &adminMAC, &vlan, &MAC) == 0) &&
+ (adminMAC || vlan || MAC)) {
ignore_value(virNetDevSetNetConfig(linkdev, -1,
adminMAC, vlan, MAC, !!vlan));
--
2.13.3
More information about the libvir-list
mailing list