util: restructure virNetDevReadNetConfig() to eliminate false error logs

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.

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;
 }

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;
 

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));