Re: [libvirt] [PATCHv3] lxc: Fix return values of veth.c functions - complete

From: Ryota Ozaki <ozaki.ryota(a)gmail.com&gt; From: Ryota Ozaki <ozaki.ryota(a)gmail.com&gt; 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. --- Changes from Ozaki's original patch: 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. Also changed to return -1 if waitpid fails or   WEXITSTATUS != 0. 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 (note error log of errno from brAddInterface   fixed in v3). 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.

 po/POTFILES.in           |    1 +  src/lxc/lxc_container.c  |   18 +++----  src/lxc/lxc_controller.c |   11 +---  src/lxc/lxc_driver.c     |   33 ++++--------  src/lxc/veth.c           |  129 ++++++++++++++++++++++++++-------------------  src/lxc/veth.h           |   19 +++++--  6 files changed, 107 insertions(+), 104 deletions(-) diff --git a/po/POTFILES.in b/po/POTFILES.in index dad1f8d..8a148f3 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 4371dba..b7f025a 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 d8b7bc7..7dc51a5 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 4fc1ecd..bc48b59 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 34f7faa..acf28c7 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 523bf9c..1ec1ec8 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 */ -- 1.7.1.1 -- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list