[libvirt] [PATCH revision RFC 2/2] lxc: Fix return values of veth.c functions - suggested changes

[Laine Stump]

Some suggested changes to your latest patch (I did the review by
applying your patch, then examining the functions that were touched,
focusing just on setting of rc)

Summary:

1) virAsprintf() will return the number of characters in the new
   string on success, not 0, so we need to only set rc if it fails
   (< 0). Assigning rc on success causes the caller to falsely believe
   the function failed.

2) lxcVmCleanup was always doing the if (WIFEXITED() ...) even
   if waitpid had failed. I don't know if the behavior of WIFEXITED
   is defined if waitpid fails, but all the other uses I can find
   avoid calling WIFEXITED and WEXITSTATUS if waitpid fails, so that's
   what I did here.

3) lxcSetupInterfaces - rather than explicitly setting rc from the
   return of functions, since it defaults to -1, I just goto
   error_exit if the functions return < 0. That's just cosmetic. The
   real problem is that rc was being set from brAddInterface, which
   returns > 0 on failure.

4) assigning "rc = -1" at the beginning of each veth.c function is a
   dead store, since it will always be set by the call to virRun(). This
   causes coverity code analysis tool to report problems.
---
 src/lxc/lxc_container.c |    6 ++++--
 src/lxc/lxc_driver.c    |   19 ++++++-------------
 src/lxc/veth.c          |   12 ++++++------
 3 files changed, 16 insertions(+), 21 deletions(-)

diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c
index b19192f..b7f025a 100644
--- a/src/lxc/lxc_container.c
+++ b/src/lxc/lxc_container.c
@@ -249,9 +249,11 @@ 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);
diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
index 614548e..9238f80 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,11 +737,7 @@ 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)) {
+    } else if (WIFEXITED(childStatus)) {
         rc = WEXITSTATUS(childStatus);
         DEBUG("container exited with rc: %d", rc);
     }
@@ -859,8 +855,7 @@ static int lxcSetupInterfaces(virConnectPtr conn,
             strcpy(parentVeth, def->nets[i]->ifname);
         }
         DEBUG("parentVeth: %s, containerVeth: %s", parentVeth, containerVeth);
-        rc = vethCreate(parentVeth, PATH_MAX, containerVeth, PATH_MAX);
-        if (rc < 0)
+        if (vethCreate(parentVeth, PATH_MAX, containerVeth, PATH_MAX) < 0)
             goto error_exit;
 
         if (NULL == def->nets[i]->ifname) {
@@ -884,20 +879,18 @@ static int lxcSetupInterfaces(virConnectPtr conn,
         {
             char macaddr[VIR_MAC_STRING_BUFLEN];
             virFormatMacAddr(def->nets[i]->mac, macaddr);
-            rc = setMacAddr(containerVeth, macaddr);
-            if (rc < 0)
+            if (setMacAddr(containerVeth, macaddr) < 0)
                 goto error_exit;
         }
 
-        if (0 != (rc = brAddInterface(brctl, bridge, parentVeth))) {
+        if (0 != brAddInterface(brctl, bridge, parentVeth)) {
             virReportSystemError(rc,
                                  _("Failed to add %s device to %s"),
                                  parentVeth, bridge);
             goto error_exit;
         }
 
-        rc = vethInterfaceUpOrDown(parentVeth, 1);
-        if (rc < 0)
+        if (vethInterfaceUpOrDown(parentVeth, 1) < 0)
             goto error_exit;
     }
 
diff --git a/src/lxc/veth.c b/src/lxc/veth.c
index 19f11f2..acf28c7 100644
--- a/src/lxc/veth.c
+++ b/src/lxc/veth.c
@@ -85,7 +85,7 @@ 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
     };
@@ -133,7 +133,7 @@ int vethCreate(char* veth1, int veth1MaxLen,
  */
 int vethDelete(const char *veth)
 {
-    int rc = -1;
+    int rc;
     const char *argv[] = {"ip", "link", "del", veth, NULL};
     int cmdResult = 0;
 
@@ -167,7 +167,7 @@ int vethDelete(const char *veth)
  */
 int vethInterfaceUpOrDown(const char* veth, int upOrDown)
 {
-    int rc = -1;
+    int rc;
     const char *argv[] = {"ifconfig", veth, NULL, NULL};
     int cmdResult = 0;
 
@@ -210,7 +210,7 @@ int vethInterfaceUpOrDown(const char* veth, int upOrDown)
  */
 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
@@ -249,7 +249,7 @@ int moveInterfaceToNetNs(const char* iface, int pidInNs)
  */
 int setMacAddr(const char* iface, const char* macaddr)
 {
-    int rc = -1;
+    int rc;
     const char *argv[] = {
         "ip", "link", "set", iface, "address", macaddr, NULL
     };
@@ -280,7 +280,7 @@ int setMacAddr(const char* iface, const char* macaddr)
  */
 int setInterfaceName(const char* iface, const char* new)
 {
-    int rc = -1;
+    int rc;
     const char *argv[] = {
         "ip", "link", "set", iface, "name", new, NULL
     };
-- 
1.7.1.1