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

Re: [libvirt] [PATCH] lxc: Fix return value handlings of vethInterfaceUpOrDown and moveInterfaceToNetNs



On Sat, Jul 24, 2010 at 11:44 PM, Ryota Ozaki <ozaki ryota gmail com> wrote:
> Hi Laine,
>
> On Sat, Jul 24, 2010 at 2:58 AM, Laine Stump <laine laine org> wrote:
>>  On 07/23/2010 01:25 PM, Ryota Ozaki wrote:
>>>
>>> Both may return a positive value when they fail. We should check
>>> if the value is not zero instead of checking if it's negative.
>>
>> I notice that  lxcSetupInterfaces has a comment saying that it returns -1 on
>> failure, but it actually just passes on what is returned by
>> vethInterfaceUpOrDown.
>
> Oh, I didn't know that.
>
> Additionally, I found that other functions, e.g., setMacAddr, are also handled
> with the wrong way. And also handling return values with virReportSystemError
> is also wrong because it expects an errno, not an exit code. We have to fix
> all of them ;-<
>
>>
>> Would you be willing to consider instead just changing vethInterfaceUpOrDown
>> and moveInterfaceToNetNs to return -1 in all error cases (and checking the
>> return for < 0), rather than grabbing the exit code of the exec'ed command?
>> None of the callers do anything with that extra information anyway, and it
>> would help to make the return values more consistent (which makes it easier
>> to catch bugs like this, or eliminates them altogether ;-)
>
> Yeah, I'm also a bit annoying with the return values. Hmm, but we now show error
> messages with the return values outside the functions. Without that, we have to
> show the error message in the functions or some other place, otherwise we lose
> useful information of errors. It seems not good idea.
>
> One option is to let virRun show an error message by passing NULL to the second
> argument (status) of it, like brSetEnableSTP in util/bridge.c, and
> always return -1
> on a failure. It'd satisfy what you suggest.
>
> Honestly said, I cannot decide. Anyone has any suggestions on that?

Anyway, I've created a patch following Laine's suggestion. Certainly it
looks pretty cleaner than so far ;-)

  ozaki-r

diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c
index 4371dba..5671852 100644
--- a/src/lxc/lxc_container.c
+++ b/src/lxc/lxc_container.c
@@ -244,29 +244,24 @@ static int lxcContainerWaitForContinue(int control)
 static int lxcContainerRenameAndEnableInterfaces(unsigned int nveths,
                                                  char **veths)
 {
-    int rc = 0;
+    int rc = -1;
     unsigned int i;
     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)
             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 (setInterfaceName(veths[i], newname) < 0) {
+            VIR_ERROR(_("Failed to rename %s to %s"),
+                      veths[i], newname);
             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 (vethInterfaceUpOrDown(newname, 1) < 0) {
+            VIR_ERROR(_("Failed to enable %s"), newname);
             goto error_out;
         }
         VIR_FREE(newname);
@@ -274,8 +269,12 @@ static int
lxcContainerRenameAndEnableInterfaces(unsigned int nveths,

     /* enable lo device only if there were other net devices */
     if (veths)
-        rc = vethInterfaceUpOrDown("lo", 1);
+        if (vethInterfaceUpOrDown("lo", 1) < 0) {
+            VIR_ERROR("%s", _("Failed to enable lo"));
+            goto error_out;
+        }

+    rc = 0;
 error_out:
     VIR_FREE(newname);
     return rc;
diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
index 462bc9c..773ee2e 100644
--- a/src/lxc/lxc_driver.c
+++ b/src/lxc/lxc_driver.c
@@ -859,9 +859,10 @@ 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))) {
+        if (vethCreate(parentVeth, PATH_MAX, containerVeth, PATH_MAX) < 0) {
             lxcError(VIR_ERR_INTERNAL_ERROR,
-                     _("Failed to create veth device pair: %d"), rc);
+                     _("Failed to create veth device pair: %s, %s"),
+                     parentVeth, containerVeth);
             goto error_exit;
         }
         if (NULL == def->nets[i]->ifname) {
@@ -885,10 +886,10 @@ 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) {
+                lxcError(VIR_ERR_INTERNAL_ERROR,
+                         _("Failed to set %s to %s"),
+                         macaddr, containerVeth);
                 goto error_exit;
             }
         }
@@ -900,10 +901,10 @@ static int lxcSetupInterfaces(virConnectPtr conn,
             goto error_exit;
         }

-        if (0 != (rc = vethInterfaceUpOrDown(parentVeth, 1))) {
-            virReportSystemError(rc,
-                                 _("Failed to enable %s device"),
-                                 parentVeth);
+        if (vethInterfaceUpOrDown(parentVeth, 1) < 0) {
+            lxcError(VIR_ERR_INTERNAL_ERROR,
+                     _("Failed to enable %s device"),
+                     parentVeth);
             goto error_exit;
         }

diff --git a/src/lxc/veth.c b/src/lxc/veth.c
index 34f7faa..39c3a50 100644
--- a/src/lxc/veth.c
+++ b/src/lxc/veth.c
@@ -76,16 +76,13 @@ static int getFreeVethName(char *veth, int maxLen,
int startDev)
 int vethCreate(char* veth1, int veth1MaxLen,
                char* veth2, int veth2MaxLen)
 {
-    int rc = -1;
     const char *argv[] = {
         "ip", "link", "add", veth1, "type", "veth", "peer", "name", veth2, NULL
     };
-    int cmdResult;
     int vethDev = 0;

-    if ((NULL == veth1) || (NULL == veth2)) {
-        goto error_out;
-    }
+    if ((NULL == veth1) || (NULL == veth2))
+        return -1;

     DEBUG("veth1: %s veth2: %s", veth1, veth2);

@@ -102,14 +99,10 @@ int vethCreate(char* veth1, int veth1MaxLen,
     }

     DEBUG("veth1: %s veth2: %s", veth1, veth2);
-    rc = virRun(argv, &cmdResult);
-
-    if (0 == rc) {
-       rc = cmdResult;
-    }
+    if (virRun(argv, NULL) < 0)
+        return -1;

-error_out:
-    return rc;
+    return 0;
 }

 /**
@@ -125,24 +118,17 @@ error_out:
  */
 int vethDelete(const char *veth)
 {
-    int rc = -1;
     const char *argv[] = {"ip", "link", "del", veth, NULL};
-    int cmdResult;

-    if (NULL == veth) {
-        goto error_out;
-    }
+    if (NULL == veth)
+        return -1;

     DEBUG("veth: %s", veth);

-    rc = virRun(argv, &cmdResult);
-
-    if (0 == rc) {
-       rc = cmdResult;
-    }
+    if (virRun(argv, NULL) < 0)
+        return -1;

-error_out:
-    return rc;
+    return 0;
 }

 /**
@@ -157,27 +143,20 @@ error_out:
  */
 int vethInterfaceUpOrDown(const char* veth, int upOrDown)
 {
-    int rc = -1;
     const char *argv[] = {"ifconfig", veth, NULL, NULL};
-    int cmdResult;

-    if (NULL == veth) {
-        goto error_out;
-    }
+    if (NULL == veth)
+        return -1;

     if (0 == upOrDown)
         argv[2] = "down";
     else
         argv[2] = "up";

-    rc = virRun(argv, &cmdResult);
-
-    if (0 == rc) {
-       rc = cmdResult;
-    }
+    if (virRun(argv, NULL) < 0)
+        return -1;

-error_out:
-    return rc;
+    return 0;
 }

 /**
@@ -198,19 +177,17 @@ int moveInterfaceToNetNs(const char* iface, int pidInNs)
     const char *argv[] = {
         "ip", "link", "set", iface, "netns", NULL, NULL
     };
-    int cmdResult;

-    if (NULL == iface) {
+    if (NULL == iface)
         goto error_out;
-    }

     if (virAsprintf(&pid, "%d", pidInNs) == -1)
         goto error_out;

     argv[5] = pid;
-    rc = virRun(argv, &cmdResult);
-    if (0 == rc)
-        rc = cmdResult;
+    rc = virRun(argv, NULL);
+    if (rc < 0)
+        rc = -1;

 error_out:
     VIR_FREE(pid);
@@ -230,22 +207,17 @@ error_out:
  */
 int setMacAddr(const char* iface, const char* macaddr)
 {
-    int rc = -1;
     const char *argv[] = {
         "ip", "link", "set", iface, "address", macaddr, NULL
     };
-    int cmdResult;

-    if (NULL == iface) {
-        goto error_out;
-    }
+    if (NULL == iface)
+        return -1;

-    rc = virRun(argv, &cmdResult);
-    if (0 == rc)
-        rc = cmdResult;
+    if (virRun(argv, NULL) < 0)
+        return -1;

-error_out:
-    return rc;
+    return 0;
 }

 /**
@@ -261,20 +233,15 @@ error_out:
  */
 int setInterfaceName(const char* iface, const char* new)
 {
-    int rc = -1;
     const char *argv[] = {
         "ip", "link", "set", iface, "name", new, NULL
     };
-    int cmdResult;

-    if (NULL == iface || NULL == new) {
-        goto error_out;
-    }
+    if (NULL == iface || NULL == new)
+        return -1;

-    rc = virRun(argv, &cmdResult);
-    if (0 == rc)
-        rc = cmdResult;
+    if (virRun(argv, NULL) < 0)
+        return -1;

-error_out:
-    return rc;
+    return 0;
 }


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