12:32 p.m.
From: Ryota Ozaki <ozaki.ryota(a)gmail.com>
From: Ryota Ozaki <ozaki.ryota(a)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.
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