From: Ryota Ozaki <ozaki.ryota@xxxxxxxxx> 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 would not fix the bug, but would 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. --- po/POTFILES.in | 1 + src/lxc/lxc_container.c | 12 +---- src/lxc/lxc_controller.c | 11 +---- src/lxc/lxc_driver.c | 22 +++------ src/lxc/veth.c | 117 ++++++++++++++++++++++++++------------------- src/lxc/veth.h | 19 +++++-- 6 files changed, 94 insertions(+), 88 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..b19192f 100644 --- a/src/lxc/lxc_container.c +++ b/src/lxc/lxc_container.c @@ -255,20 +255,14 @@ static int lxcContainerRenameAndEnableInterfaces(unsigned int nveths, 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..614548e 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -859,11 +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))) { - lxcError(VIR_ERR_INTERNAL_ERROR, - _("Failed to create veth device pair: %d"), rc); + rc = vethCreate(parentVeth, PATH_MAX, containerVeth, PATH_MAX); + if (rc < 0) goto error_exit; - } + if (NULL == def->nets[i]->ifname) { def->nets[i]->ifname = strdup(parentVeth); } @@ -885,12 +884,9 @@ 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); + rc = setMacAddr(containerVeth, macaddr); + if (rc < 0) goto error_exit; - } } if (0 != (rc = brAddInterface(brctl, bridge, parentVeth))) { @@ -900,13 +896,9 @@ static int lxcSetupInterfaces(virConnectPtr conn, goto error_exit; } - if (0 != (rc = vethInterfaceUpOrDown(parentVeth, 1))) { - virReportSystemError(rc, - _("Failed to enable %s device"), - parentVeth); + rc = vethInterfaceUpOrDown(parentVeth, 1); + if (rc < 0) goto error_exit; - } - } rc = 0; diff --git a/src/lxc/veth.c b/src/lxc/veth.c index 34f7faa..19f11f2 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 */ /** @@ -80,13 +89,9 @@ int vethCreate(char* veth1, int veth1MaxLen, 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; } @@ -127,21 +135,23 @@ int vethDelete(const char *veth) { int rc = -1; 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; } @@ -159,11 +169,7 @@ 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; - } + 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; } @@ -198,21 +215,23 @@ int moveInterfaceToNetNs(const char* iface, int pidInNs) 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; } @@ -234,17 +253,17 @@ int setMacAddr(const char* iface, const char* macaddr) 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; } @@ -265,16 +284,16 @@ int setInterfaceName(const char* iface, const char* new) 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@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list