11:18 a.m.
Some suggested changes to your latest patch (I did the review by
applying your patch, then examining the functions that were touched,
focusing just on setting of rc)
Summary:
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.
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.
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.
---
src/lxc/lxc_container.c | 6 ++++--
src/lxc/lxc_driver.c | 19 ++++++-------------
src/lxc/veth.c | 12 ++++++------
3 files changed, 16 insertions(+), 21 deletions(-)
diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c
index b19192f..b7f025a 100644
--- a/src/lxc/lxc_container.c
+++ b/src/lxc/lxc_container.c
@@ -249,9 +249,11 @@ 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);
diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
index 614548e..9238f80 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,11 +737,7 @@ 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)) {
+ } else if (WIFEXITED(childStatus)) {
rc = WEXITSTATUS(childStatus);
DEBUG("container exited with rc: %d", rc);
}
@@ -859,8 +855,7 @@ static int lxcSetupInterfaces(virConnectPtr conn,
strcpy(parentVeth, def->nets[i]->ifname);
}
DEBUG("parentVeth: %s, containerVeth: %s", parentVeth, containerVeth);
- rc = vethCreate(parentVeth, PATH_MAX, containerVeth, PATH_MAX);
- if (rc < 0)
+ if (vethCreate(parentVeth, PATH_MAX, containerVeth, PATH_MAX) < 0)
goto error_exit;
if (NULL == def->nets[i]->ifname) {
@@ -884,20 +879,18 @@ static int lxcSetupInterfaces(virConnectPtr conn,
{
char macaddr[VIR_MAC_STRING_BUFLEN];
virFormatMacAddr(def->nets[i]->mac, macaddr);
- rc = setMacAddr(containerVeth, macaddr);
- if (rc < 0)
+ if (setMacAddr(containerVeth, macaddr) < 0)
goto error_exit;
}
- if (0 != (rc = brAddInterface(brctl, bridge, parentVeth))) {
+ if (0 != brAddInterface(brctl, bridge, parentVeth)) {
virReportSystemError(rc,
_("Failed to add %s device to %s"),
parentVeth, bridge);
goto error_exit;
}
- rc = vethInterfaceUpOrDown(parentVeth, 1);
- if (rc < 0)
+ if (vethInterfaceUpOrDown(parentVeth, 1) < 0)
goto error_exit;
}
diff --git a/src/lxc/veth.c b/src/lxc/veth.c
index 19f11f2..acf28c7 100644
--- a/src/lxc/veth.c
+++ b/src/lxc/veth.c
@@ -85,7 +85,7 @@ 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
};
@@ -133,7 +133,7 @@ int vethCreate(char* veth1, int veth1MaxLen,
*/
int vethDelete(const char *veth)
{
- int rc = -1;
+ int rc;
const char *argv[] = {"ip", "link", "del", veth,
NULL};
int cmdResult = 0;
@@ -167,7 +167,7 @@ int vethDelete(const char *veth)
*/
int vethInterfaceUpOrDown(const char* veth, int upOrDown)
{
- int rc = -1;
+ int rc;
const char *argv[] = {"ifconfig", veth, NULL, NULL};
int cmdResult = 0;
@@ -210,7 +210,7 @@ int vethInterfaceUpOrDown(const char* veth, int upOrDown)
*/
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
@@ -249,7 +249,7 @@ int moveInterfaceToNetNs(const char* iface, int pidInNs)
*/
int setMacAddr(const char* iface, const char* macaddr)
{
- int rc = -1;
+ int rc;
const char *argv[] = {
"ip", "link", "set", iface, "address",
macaddr, NULL
};
@@ -280,7 +280,7 @@ int setMacAddr(const char* iface, const char* macaddr)
*/
int setInterfaceName(const char* iface, const char* new)
{
- int rc = -1;
+ int rc;
const char *argv[] = {
"ip", "link", "set", iface, "name", new,
NULL
};
--
1.7.1.1