[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]