From e97ab8d35d2582029353dc322c229bf5a6a931ba Mon Sep 17 00:00:00 2001

Message-Id: <e97ab8d35d2582029353dc322c229bf5a6a931ba@dist-git>

From: Laine Stump <laine@laine.org>

Date: Mon, 14 Aug 2017 21:28:21 -0400

Subject: [PATCH] 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.

Resolves: https://bugzilla.redhat.com/1460082

(cherry picked from commit 9a08168301466dde301adedb9b567590b5968e32)

Signed-off-by: Jiri Denemark <jdenemar@redhat.com>

---

 src/util/virhostdev.c       | 122 ++++++++++++++++++++++++++++----------------

 src/util/virnetdev.c        |  16 ++++--

 src/util/virnetdevmacvlan.c |   5 +-

 3 files changed, 94 insertions(+), 49 deletions(-)

diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c

index 102fd85c16..cca9d81a4c 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;

+        /* 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.

+         */

-        ret = virNetDevReadNetConfig(linkdev, vf, stateDir, &adminMAC, &vlan, &MAC);

-        if (ret < 0 && oldStateDir)

-            ret = virNetDevReadNetConfig(linkdev, vf, oldStateDir,

-                                         &adminMAC, &vlan, &MAC);

+        /* 1) standard location */

+        if (virNetDevReadNetConfig(linkdev, vf, stateDir,

+                                   &adminMAC, &vlan, &MAC) < 0) {

+            goto cleanup;

+        }

-        if (ret < 0) {

-            /* see if the config was saved using the PF's "port 2"

-             * netdev for the file name.

-             */

+        /* 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) {

-                ret = virNetDevReadNetConfig(linkdev, vf, stateDir,

-                                             &adminMAC, &vlan, &MAC);

+            if (virHostdevNetDevice(hostdev, 1, &linkdev, &vf) < 0 ||

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

-            }

-

-            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 05112d9772..476f5334d1 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 97c87701cf..fb41bf934c 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.14.1