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

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



On Fri, Jul 30, 2010 at 2:32 AM, Laine Stump <laine laine org> wrote:
> From: Ryota Ozaki <ozaki ryota gmail com>
>
> From: Ryota Ozaki <ozaki ryota gmail com>
>
> 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.

ACK. Thanks, Laine!

  ozaki-r

>
>
>  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 redhat com
> https://www.redhat.com/mailman/listinfo/libvir-list
>


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