lxc: Fix return values of veth.c functions

Previously, the functions in src/lxc/veth.c could sometimes return
positive values on failure rather than -1. This made accurate error
reporting difficult, and led to one failure to catch an error in a
calling function.

This patch makes all the functions in veth.c consistently return 0 on
success, and -1 on failure. It also fixes up the callers to the veth.c
functions where necessary.

Note that this patch may be related to the bug:

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

It will not fix the bug, but should unveil what happens.

* po/POTFILES.in - add veth.c, which previously had no translatable strings
* src/lxc/lxc_controller.c
* src/lxc/lxc_container.c
* src/lxc/lxc_driver.c    - fixup callers to veth.c, and remove error logs,
                            as they are now done in veth.c
* src/lxc/veth.c - make all functions consistently return -1 on error.
* src/lxc/veth.h - use ATTRIBUTE_NONNULL to protect against NULL args.

6 files changed, 107 insertions, 104 deletions

diff --git a/po/POTFILES.in b/po/POTFILES.in
index dad1f8daf..8a148f394 100644
--- a/po/POTFILES.in
+++ b/po/POTFILES.in
@@ -33,6 +33,7 @@ src/lxc/lxc_container.c
 src/lxc/lxc_conf.c
 src/lxc/lxc_controller.c
 src/lxc/lxc_driver.c
+src/lxc/veth.c
 src/network/bridge_driver.c
 src/node_device/node_device_driver.c
 src/node_device/node_device_hal.c
diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c
index 4371dba3c..b7f025ae9 100644
--- a/src/lxc/lxc_container.c
+++ b/src/lxc/lxc_container.c
@@ -249,26 +249,22 @@ static int lxcContainerRenameAndEnableInterfaces(unsigned int nveths,
     char *newname = NULL;
 
     for (i = 0 ; i < nveths ; i++) {
-        rc = virAsprintf(&newname, "eth%d", i);
-        if (rc < 0)
+        if (virAsprintf(&newname, "eth%d", i) < 0) {
+            virReportOOMError();
+            rc = -1;
             goto error_out;
+        }
 
         DEBUG("Renaming %s to %s", veths[i], newname);
         rc = setInterfaceName(veths[i], newname);
-        if (0 != rc) {
-            VIR_ERROR(_("Failed to rename %s to %s (%d)"),
-                      veths[i], newname, rc);
-            rc = -1;
+        if (rc < 0)
             goto error_out;
-        }
 
         DEBUG("Enabling %s", newname);
         rc = vethInterfaceUpOrDown(newname, 1);
-        if (0 != rc) {
-            VIR_ERROR(_("Failed to enable %s (%d)"), newname, rc);
-            rc = -1;
+        if (rc < 0)
             goto error_out;
-        }
+
         VIR_FREE(newname);
     }
 
diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c
index d8b7bc7bf..7dc51a554 100644
--- a/src/lxc/lxc_controller.c
+++ b/src/lxc/lxc_controller.c
@@ -477,12 +477,8 @@ static int lxcControllerMoveInterfaces(unsigned int nveths,
 {
     unsigned int i;
     for (i = 0 ; i < nveths ; i++)
-        if (moveInterfaceToNetNs(veths[i], container) < 0) {
-            lxcError(VIR_ERR_INTERNAL_ERROR,
-                     _("Failed to move interface %s to ns %d"),
-                     veths[i], container);
+        if (moveInterfaceToNetNs(veths[i], container) < 0)
             return -1;
-        }
 
     return 0;
 }
@@ -502,10 +498,7 @@ static int lxcControllerCleanupInterfaces(unsigned int nveths,
 {
     unsigned int i;
     for (i = 0 ; i < nveths ; i++)
-        if (vethDelete(veths[i]) < 0)
-            lxcError(VIR_ERR_INTERNAL_ERROR,
-                     _("Failed to delete veth: %s"), veths[i]);
-            /* will continue to try to cleanup any other interfaces */
+        vethDelete(veths[i]);
 
     return 0;
 }
diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
index b28669289..6fe20b1e0 100644
--- a/src/lxc/lxc_driver.c
+++ b/src/lxc/lxc_driver.c
@@ -722,7 +722,7 @@ cleanup:
 static int lxcVmCleanup(lxc_driver_t *driver,
                         virDomainObjPtr  vm)
 {
-    int rc = -1;
+    int rc = 0;
     int waitRc;
     int childStatus = -1;
     virCgroupPtr cgroup;
@@ -737,13 +737,10 @@ static int lxcVmCleanup(lxc_driver_t *driver,
         virReportSystemError(errno,
                              _("waitpid failed to wait for container %d: %d"),
                              vm->pid, waitRc);
-    }
-
-    rc = 0;
-
-    if (WIFEXITED(childStatus)) {
-        rc = WEXITSTATUS(childStatus);
-        DEBUG("container exited with rc: %d", rc);
+        rc = -1;
+    } else if (WIFEXITED(childStatus)) {
+        DEBUG("container exited with rc: %d", WEXITSTATUS(childStatus));
+        rc = -1;
     }
 
     /* now that we know it's stopped call the hook if present */
@@ -859,11 +856,9 @@ static int lxcSetupInterfaces(virConnectPtr conn,
             strcpy(parentVeth, def->nets[i]->ifname);
         }
         DEBUG("parentVeth: %s, containerVeth: %s", parentVeth, containerVeth);
-        if (0 != (rc = vethCreate(parentVeth, PATH_MAX, containerVeth, PATH_MAX))) {
-            lxcError(VIR_ERR_INTERNAL_ERROR,
-                     _("Failed to create veth device pair: %d"), rc);
+        if (vethCreate(parentVeth, PATH_MAX, containerVeth, PATH_MAX) < 0)
             goto error_exit;
-        }
+
         if (NULL == def->nets[i]->ifname) {
             def->nets[i]->ifname = strdup(parentVeth);
         }
@@ -885,28 +880,20 @@ static int lxcSetupInterfaces(virConnectPtr conn,
         {
             char macaddr[VIR_MAC_STRING_BUFLEN];
             virFormatMacAddr(def->nets[i]->mac, macaddr);
-            if (0 != (rc = setMacAddr(containerVeth, macaddr))) {
-                virReportSystemError(rc,
-                                     _("Failed to set %s to %s"),
-                                     macaddr, containerVeth);
+            if (setMacAddr(containerVeth, macaddr) < 0)
                 goto error_exit;
-            }
         }
 
         if (0 != (rc = brAddInterface(brctl, bridge, parentVeth))) {
             virReportSystemError(rc,
                                  _("Failed to add %s device to %s"),
                                  parentVeth, bridge);
+            rc = -1;
             goto error_exit;
         }
 
-        if (0 != (rc = vethInterfaceUpOrDown(parentVeth, 1))) {
-            virReportSystemError(rc,
-                                 _("Failed to enable %s device"),
-                                 parentVeth);
+        if (vethInterfaceUpOrDown(parentVeth, 1) < 0)
             goto error_exit;
-        }
-
     }
 
     rc = 0;
diff --git a/src/lxc/veth.c b/src/lxc/veth.c
index 34f7faa07..acf28c71f 100644
--- a/src/lxc/veth.c
+++ b/src/lxc/veth.c
@@ -13,12 +13,21 @@
 
 #include <string.h>
 #include <stdio.h>
+#include <sys/types.h>
+#include <sys/wait.h>
 
 #include "veth.h"
 #include "internal.h"
 #include "logging.h"
 #include "memory.h"
 #include "util.h"
+#include "virterror_internal.h"
+
+#define VIR_FROM_THIS VIR_FROM_LXC
+
+#define vethError(code, ...)                                  \
+    virReportErrorHelper(NULL, VIR_FROM_LXC, code, __FILE__,  \
+                         __FUNCTION__, __LINE__, __VA_ARGS__)
 
 /* Functions */
 /**
@@ -76,17 +85,13 @@ static int getFreeVethName(char *veth, int maxLen, int startDev)
 int vethCreate(char* veth1, int veth1MaxLen,
                char* veth2, int veth2MaxLen)
 {
-    int rc = -1;
+    int rc;
     const char *argv[] = {
         "ip", "link", "add", veth1, "type", "veth", "peer", "name", veth2, NULL
     };
-    int cmdResult;
+    int cmdResult = 0;
     int vethDev = 0;
 
-    if ((NULL == veth1) || (NULL == veth2)) {
-        goto error_out;
-    }
-
     DEBUG("veth1: %s veth2: %s", veth1, veth2);
 
     while ((1 > strlen(veth1)) || STREQ(veth1, veth2)) {
@@ -104,11 +109,14 @@ int vethCreate(char* veth1, int veth1MaxLen,
     DEBUG("veth1: %s veth2: %s", veth1, veth2);
     rc = virRun(argv, &cmdResult);
 
-    if (0 == rc) {
-       rc = cmdResult;
+    if (rc != 0 ||
+        (WIFEXITED(cmdResult) && WEXITSTATUS(cmdResult) != 0)) {
+        vethError(VIR_ERR_INTERNAL_ERROR,
+                  _("Failed to create veth device pair '%s', '%s': %d"),
+                  veth1, veth2, WEXITSTATUS(cmdResult));
+        rc = -1;
     }
 
-error_out:
     return rc;
 }
 
@@ -125,23 +133,25 @@ error_out:
  */
 int vethDelete(const char *veth)
 {
-    int rc = -1;
+    int rc;
     const char *argv[] = {"ip", "link", "del", veth, NULL};
-    int cmdResult;
-
-    if (NULL == veth) {
-        goto error_out;
-    }
+    int cmdResult = 0;
 
     DEBUG("veth: %s", veth);
 
     rc = virRun(argv, &cmdResult);
 
-    if (0 == rc) {
-       rc = cmdResult;
+    if (rc != 0 ||
+        (WIFEXITED(cmdResult) && WEXITSTATUS(cmdResult) != 0)) {
+        /*
+         * Prevent overwriting an error log which may be set
+         * where an actual failure occurs.
+         */
+        VIR_DEBUG(_("Failed to delete '%s' (%d)"),
+                  veth, WEXITSTATUS(cmdResult));
+        rc = -1;
     }
 
-error_out:
     return rc;
 }
 
@@ -157,13 +167,9 @@ error_out:
  */
 int vethInterfaceUpOrDown(const char* veth, int upOrDown)
 {
-    int rc = -1;
+    int rc;
     const char *argv[] = {"ifconfig", veth, NULL, NULL};
-    int cmdResult;
-
-    if (NULL == veth) {
-        goto error_out;
-    }
+    int cmdResult = 0;
 
     if (0 == upOrDown)
         argv[2] = "down";
@@ -172,11 +178,22 @@ int vethInterfaceUpOrDown(const char* veth, int upOrDown)
 
     rc = virRun(argv, &cmdResult);
 
-    if (0 == rc) {
-       rc = cmdResult;
+    if (rc != 0 ||
+        (WIFEXITED(cmdResult) && WEXITSTATUS(cmdResult) != 0)) {
+        if (0 == upOrDown)
+            /*
+             * Prevent overwriting an error log which may be set
+             * where an actual failure occurs.
+             */
+            VIR_DEBUG(_("Failed to disable '%s' (%d)"),
+                      veth, WEXITSTATUS(cmdResult));
+        else
+            vethError(VIR_ERR_INTERNAL_ERROR,
+                      _("Failed to enable '%s' (%d)"),
+                      veth, WEXITSTATUS(cmdResult));
+        rc = -1;
     }
 
-error_out:
     return rc;
 }
 
@@ -193,26 +210,28 @@ error_out:
  */
 int moveInterfaceToNetNs(const char* iface, int pidInNs)
 {
-    int rc = -1;
+    int rc;
     char *pid = NULL;
     const char *argv[] = {
         "ip", "link", "set", iface, "netns", NULL, NULL
     };
-    int cmdResult;
+    int cmdResult = 0;
 
-    if (NULL == iface) {
-        goto error_out;
+    if (virAsprintf(&pid, "%d", pidInNs) == -1) {
+        virReportOOMError();
+        return -1;
     }
 
-    if (virAsprintf(&pid, "%d", pidInNs) == -1)
-        goto error_out;
-
     argv[5] = pid;
     rc = virRun(argv, &cmdResult);
-    if (0 == rc)
-        rc = cmdResult;
+    if (rc != 0 ||
+        (WIFEXITED(cmdResult) && WEXITSTATUS(cmdResult) != 0)) {
+        vethError(VIR_ERR_INTERNAL_ERROR,
+                  _("Failed to move '%s' into NS(pid=%d) (%d)"),
+                  iface, pidInNs, WEXITSTATUS(cmdResult));
+        rc = -1;
+    }
 
-error_out:
     VIR_FREE(pid);
     return rc;
 }
@@ -230,21 +249,21 @@ error_out:
  */
 int setMacAddr(const char* iface, const char* macaddr)
 {
-    int rc = -1;
+    int rc;
     const char *argv[] = {
         "ip", "link", "set", iface, "address", macaddr, NULL
     };
-    int cmdResult;
-
-    if (NULL == iface) {
-        goto error_out;
-    }
+    int cmdResult = 0;
 
     rc = virRun(argv, &cmdResult);
-    if (0 == rc)
-        rc = cmdResult;
+    if (rc != 0 ||
+        (WIFEXITED(cmdResult) && WEXITSTATUS(cmdResult) != 0)) {
+        vethError(VIR_ERR_INTERNAL_ERROR,
+                  _("Failed to set '%s' to '%s' (%d)"),
+                  macaddr, iface, WEXITSTATUS(cmdResult));
+        rc = -1;
+    }
 
-error_out:
     return rc;
 }
 
@@ -261,20 +280,20 @@ error_out:
  */
 int setInterfaceName(const char* iface, const char* new)
 {
-    int rc = -1;
+    int rc;
     const char *argv[] = {
         "ip", "link", "set", iface, "name", new, NULL
     };
-    int cmdResult;
-
-    if (NULL == iface || NULL == new) {
-        goto error_out;
-    }
+    int cmdResult = 0;
 
     rc = virRun(argv, &cmdResult);
-    if (0 == rc)
-        rc = cmdResult;
+    if (rc != 0 ||
+        (WIFEXITED(cmdResult) && WEXITSTATUS(cmdResult) != 0)) {
+        vethError(VIR_ERR_INTERNAL_ERROR,
+                  _("Failed to set '%s' to '%s' (%d)"),
+                  new, iface, WEXITSTATUS(cmdResult));
+        rc = -1;
+    }
 
-error_out:
     return rc;
 }
diff --git a/src/lxc/veth.h b/src/lxc/veth.h
index 523bf9c23..1ec1ec8cc 100644
--- a/src/lxc/veth.h
+++ b/src/lxc/veth.h
@@ -13,14 +13,21 @@
 # define VETH_H
 
 # include <config.h>
+# include "internal.h"
 
 /* Function declarations */
 int vethCreate(char* veth1, int veth1MaxLen, char* veth2,
-               int veth2MaxLen);
-int vethDelete(const char* veth);
-int vethInterfaceUpOrDown(const char* veth, int upOrDown);
-int moveInterfaceToNetNs(const char *iface, int pidInNs);
-int setMacAddr(const char* iface, const char* macaddr);
-int setInterfaceName(const char* iface, const char* new);
+               int veth2MaxLen)
+    ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3);
+int vethDelete(const char* veth)
+    ATTRIBUTE_NONNULL(1);
+int vethInterfaceUpOrDown(const char* veth, int upOrDown)
+    ATTRIBUTE_NONNULL(1);
+int moveInterfaceToNetNs(const char *iface, int pidInNs)
+    ATTRIBUTE_NONNULL(1);
+int setMacAddr(const char* iface, const char* macaddr)
+    ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
+int setInterfaceName(const char* iface, const char* new)
+    ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
 
 #endif /* VETH_H */