From d6ded4029afabd123c5578a33c4c7f9a6468044b Mon Sep 17 00:00:00 2001

Message-Id: <d6ded4029afabd123c5578a33c4c7f9a6468044b.1381871411.git.jdenemar@redhat.com>

From: "Daniel P. Berrange" <berrange@redhat.com>

Date: Mon, 7 Oct 2013 17:17:30 +0100

Subject: [PATCH] Retry veth device creation on failure

For

  https://bugzilla.redhat.com/show_bug.cgi?id=1014604

The veth device creation code run in two steps, first it looks

for two free veth device names, then it runs ip link to create

the veth pair. There is an obvious race between finding free

names and creating them, when guests are started in parallel.

Rewrite the code to loop and re-try creation if it fails, to

deal with the race condition.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>

(cherry picked from commit f2e53555eb6971635e526e93f9de07a76daf61cb)

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

---

 src/util/virnetdevveth.c | 151 +++++++++++++++++++++++++++++------------------

 1 file changed, 92 insertions(+), 59 deletions(-)

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

index c0d32c4..1ccfc54 100644

--- a/src/util/virnetdevveth.c

+++ b/src/util/virnetdevveth.c

@@ -33,13 +33,26 @@

 #include "virfile.h"

 #include "virstring.h"

 #include "virutil.h"

+#include "virnetdev.h"

 #define VIR_FROM_THIS VIR_FROM_NONE

 /* Functions */

+

+static int virNetDevVethExists(int devNum)

+{

+    int ret;

+    char *path = NULL;

+    if (virAsprintf(&path, "/sys/class/net/veth%d/", devNum) < 0)

+        return -1;

+    ret = virFileExists(path) ? 1 : 0;

+    VIR_DEBUG("Checked dev veth%d usage: %d", devNum, ret);

+    VIR_FREE(path);

+    return ret;

+}

+

 /**

- * virNetDevVethGetFreeName:

- * @veth: pointer to store returned name for veth device

+ * virNetDevVethGetFreeNum:

  * @startDev: device number to start at (x in vethx)

  *

  * Looks in /sys/class/net/ to find the first available veth device

@@ -47,25 +60,23 @@

  *

  * Returns non-negative device number on success or -1 in case of error

  */

-static int virNetDevVethGetFreeName(char **veth, int startDev)

+static int virNetDevVethGetFreeNum(int startDev)

 {

-    int devNum = startDev-1;

-    char *path = NULL;

+    int devNum;

-    VIR_DEBUG("Find free from veth%d", startDev);

-    do {

-        VIR_FREE(path);

-        ++devNum;

-        if (virAsprintf(&path, "/sys/class/net/veth%d/", devNum) < 0)

-            return -1;

-        VIR_DEBUG("Probe %s", path);

-    } while (virFileExists(path));

-    VIR_FREE(path);

+#define MAX_DEV_NUM 65536

-    if (virAsprintf(veth, "veth%d", devNum) < 0)

-        return -1;

+    for (devNum = startDev; devNum < MAX_DEV_NUM; devNum++) {

+        int ret = virNetDevVethExists(devNum);

+        if (ret < 0)

+            return -1;

+        if (ret == 0)

+            return devNum;

+    }

-    return devNum;

+    virReportError(VIR_ERR_INTERNAL_ERROR, "%s",

+                   _("No free veth devices available"));

+    return -1;

 }

 /**

@@ -95,57 +106,79 @@ static int virNetDevVethGetFreeName(char **veth, int startDev)

  */

 int virNetDevVethCreate(char** veth1, char** veth2)

 {

-    int rc = -1;

-    const char *argv[] = {

-        "ip", "link", "add", NULL, "type", "veth", "peer", "name", NULL, NULL

-    };

-    int vethDev = 0;

-    bool veth1_alloc = false;

-    bool veth2_alloc = false;

-

-    VIR_DEBUG("Host: %s guest: %s", NULLSTR(*veth1), NULLSTR(*veth2));

-

-    if (*veth1 == NULL) {

-        if ((vethDev = virNetDevVethGetFreeName(veth1, vethDev)) < 0)

-            goto cleanup;

-        VIR_DEBUG("Assigned host: %s", *veth1);

-        veth1_alloc = true;

-        vethDev++;

-    }

-    argv[3] = *veth1;

+    int ret = -1;

+    char *veth1auto = NULL;

+    char *veth2auto = NULL;

+    int vethNum = 0;

+    size_t i;

+

+    /*

+     * We might race with other containers, but this is reasonably

+     * unlikely, so don't do too many retries for device creation

+     */

+#define MAX_VETH_RETRIES 10

+

+    for (i = 0; i < MAX_VETH_RETRIES; i++) {

+        int status;

+        if (!*veth1) {

+            int veth1num;

+            if ((veth1num = virNetDevVethGetFreeNum(vethNum)) < 0)

+                goto cleanup;

+

+            if (virAsprintf(&veth1auto, "veth%d", veth1num) < 0)

+                goto cleanup;

+            vethNum = veth1num + 1;

+        }

+        if (!*veth2) {

+            int veth2num;

+            if ((veth2num = virNetDevVethGetFreeNum(vethNum)) < 0)

+                goto cleanup;

+

+            if (virAsprintf(&veth2auto, "veth%d", veth2num) < 0)

+                goto cleanup;

+            vethNum = veth2num + 1;

+        }

+

+        virCommandPtr cmd = virCommandNew("ip");

+        virCommandAddArgList(cmd, "link", "add",

+                             *veth1 ? *veth1 : veth1auto,

+                             "type", "veth", "peer", "name",

+                             *veth2 ? *veth2 : veth2auto,

+                             NULL);

-    while (*veth2 == NULL) {

-        if ((vethDev = virNetDevVethGetFreeName(veth2, vethDev)) < 0) {

-            if (veth1_alloc)

-                VIR_FREE(*veth1);

+        if (virCommandRun(cmd, &status) < 0)

             goto cleanup;

-        }

-        /* Just make sure they didn't accidentally get same name */

-        if (STREQ(*veth1, *veth2)) {

-            vethDev++;

-            VIR_FREE(*veth2);

-            continue;

+        if (status == 0) {

+            if (veth1auto) {

+                *veth1 = veth1auto;

+                veth1auto = NULL;

+            }

+            if (veth2auto) {

+                *veth2 = veth2auto;

+                veth2auto = NULL;

+            }

+            VIR_DEBUG("Create Host: %s guest: %s", *veth1, *veth2);

+            ret = 0;

+            goto cleanup;

         }

-        VIR_DEBUG("Assigned guest: %s", *veth2);

-        veth2_alloc = true;

-    }

-    argv[8] = *veth2;

-

-    VIR_DEBUG("Create Host: %s guest: %s", *veth1, *veth2);

-    if (virRun(argv, NULL) < 0) {

-        if (veth1_alloc)

-            VIR_FREE(*veth1);

-        if (veth2_alloc)

-            VIR_FREE(*veth2);

-        goto cleanup;

+        VIR_DEBUG("Failed to create veth host: %s guest: %s: %d",

+                  *veth1 ? *veth1 : veth1auto,

+                  *veth1 ? *veth1 : veth1auto,

+                  status);

+        VIR_FREE(veth1auto);

+        VIR_FREE(veth2auto);

     }

-    rc = 0;

+    virReportError(VIR_ERR_INTERNAL_ERROR,

+                   _("Failed to allocate free veth pair after %d attempts"),

+                   MAX_VETH_RETRIES);

 cleanup:

-    return rc;

+    VIR_FREE(veth1auto);

+    VIR_FREE(veth2auto);

+    return ret;

 }

 /**

-- 

1.8.3.2