12:25 p.m.
On Sat, Jul 24, 2010 at 11:44 PM, Ryota Ozaki <ozaki.ryota(a)gmail.com> wrote:
Hi Laine,
On Sat, Jul 24, 2010 at 2:58 AM, Laine Stump <laine(a)laine.org> wrote:
> On 07/23/2010 01:25 PM, Ryota Ozaki wrote:
>>
>> Both may return a positive value when they fail. We should check
>> if the value is not zero instead of checking if it's negative.
>
> I notice that lxcSetupInterfaces has a comment saying that it returns -1 on
> failure, but it actually just passes on what is returned by
> vethInterfaceUpOrDown.
Oh, I didn't know that.
Additionally, I found that other functions, e.g., setMacAddr, are also handled
with the wrong way. And also handling return values with virReportSystemError
is also wrong because it expects an errno, not an exit code. We have to fix
all of them ;-<
>
> Would you be willing to consider instead just changing vethInterfaceUpOrDown
> and moveInterfaceToNetNs to return -1 in all error cases (and checking the
> return for < 0), rather than grabbing the exit code of the exec'ed command?
> None of the callers do anything with that extra information anyway, and it
> would help to make the return values more consistent (which makes it easier
> to catch bugs like this, or eliminates them altogether ;-)
Yeah, I'm also a bit annoying with the return values. Hmm, but we now show error
messages with the return values outside the functions. Without that, we have to
show the error message in the functions or some other place, otherwise we lose
useful information of errors. It seems not good idea.
One option is to let virRun show an error message by passing NULL to the second
argument (status) of it, like brSetEnableSTP in util/bridge.c, and
always return -1
on a failure. It'd satisfy what you suggest.
Honestly said, I cannot decide. Anyone has any suggestions on that?
Anyway, I've created a patch following Laine's suggestion. Certainly it
looks pretty cleaner than so far ;-)
ozaki-r
diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c
index 4371dba..5671852 100644
--- a/src/lxc/lxc_container.c
+++ b/src/lxc/lxc_container.c
@@ -244,29 +244,24 @@ static int lxcContainerWaitForContinue(int control)
static int lxcContainerRenameAndEnableInterfaces(unsigned int nveths,
char **veths)
{
- int rc = 0;
+ int rc = -1;
unsigned int i;
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)
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 (setInterfaceName(veths[i], newname) < 0) {
+ VIR_ERROR(_("Failed to rename %s to %s"),
+ veths[i], newname);
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 (vethInterfaceUpOrDown(newname, 1) < 0) {
+ VIR_ERROR(_("Failed to enable %s"), newname);
goto error_out;
}
VIR_FREE(newname);
@@ -274,8 +269,12 @@ static int
lxcContainerRenameAndEnableInterfaces(unsigned int nveths,
/* enable lo device only if there were other net devices */
if (veths)
- rc = vethInterfaceUpOrDown("lo", 1);
+ if (vethInterfaceUpOrDown("lo", 1) < 0) {
+ VIR_ERROR("%s", _("Failed to enable lo"));
+ goto error_out;
+ }
+ rc = 0;
error_out:
VIR_FREE(newname);
return rc;
diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
index 462bc9c..773ee2e 100644
--- a/src/lxc/lxc_driver.c
+++ b/src/lxc/lxc_driver.c
@@ -859,9 +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))) {
+ if (vethCreate(parentVeth, PATH_MAX, containerVeth, PATH_MAX) < 0) {
lxcError(VIR_ERR_INTERNAL_ERROR,
- _("Failed to create veth device pair: %d"), rc);
+ _("Failed to create veth device pair: %s, %s"),
+ parentVeth, containerVeth);
goto error_exit;
}
if (NULL == def->nets[i]->ifname) {
@@ -885,10 +886,10 @@ 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) {
+ lxcError(VIR_ERR_INTERNAL_ERROR,
+ _("Failed to set %s to %s"),
+ macaddr, containerVeth);
goto error_exit;
}
}
@@ -900,10 +901,10 @@ static int lxcSetupInterfaces(virConnectPtr conn,
goto error_exit;
}
- if (0 != (rc = vethInterfaceUpOrDown(parentVeth, 1))) {
- virReportSystemError(rc,
- _("Failed to enable %s device"),
- parentVeth);
+ if (vethInterfaceUpOrDown(parentVeth, 1) < 0) {
+ lxcError(VIR_ERR_INTERNAL_ERROR,
+ _("Failed to enable %s device"),
+ parentVeth);
goto error_exit;
}
diff --git a/src/lxc/veth.c b/src/lxc/veth.c
index 34f7faa..39c3a50 100644
--- a/src/lxc/veth.c
+++ b/src/lxc/veth.c
@@ -76,16 +76,13 @@ static int getFreeVethName(char *veth, int maxLen,
int startDev)
int vethCreate(char* veth1, int veth1MaxLen,
char* veth2, int veth2MaxLen)
{
- int rc = -1;
const char *argv[] = {
"ip", "link", "add", veth1, "type",
"veth", "peer", "name", veth2, NULL
};
- int cmdResult;
int vethDev = 0;
- if ((NULL == veth1) || (NULL == veth2)) {
- goto error_out;
- }
+ if ((NULL == veth1) || (NULL == veth2))
+ return -1;
DEBUG("veth1: %s veth2: %s", veth1, veth2);
@@ -102,14 +99,10 @@ int vethCreate(char* veth1, int veth1MaxLen,
}
DEBUG("veth1: %s veth2: %s", veth1, veth2);
- rc = virRun(argv, &cmdResult);
-
- if (0 == rc) {
- rc = cmdResult;
- }
+ if (virRun(argv, NULL) < 0)
+ return -1;
-error_out:
- return rc;
+ return 0;
}
/**
@@ -125,24 +118,17 @@ error_out:
*/
int vethDelete(const char *veth)
{
- int rc = -1;
const char *argv[] = {"ip", "link", "del", veth,
NULL};
- int cmdResult;
- if (NULL == veth) {
- goto error_out;
- }
+ if (NULL == veth)
+ return -1;
DEBUG("veth: %s", veth);
- rc = virRun(argv, &cmdResult);
-
- if (0 == rc) {
- rc = cmdResult;
- }
+ if (virRun(argv, NULL) < 0)
+ return -1;
-error_out:
- return rc;
+ return 0;
}
/**
@@ -157,27 +143,20 @@ error_out:
*/
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;
- }
+ if (NULL == veth)
+ return -1;
if (0 == upOrDown)
argv[2] = "down";
else
argv[2] = "up";
- rc = virRun(argv, &cmdResult);
-
- if (0 == rc) {
- rc = cmdResult;
- }
+ if (virRun(argv, NULL) < 0)
+ return -1;
-error_out:
- return rc;
+ return 0;
}
/**
@@ -198,19 +177,17 @@ int moveInterfaceToNetNs(const char* iface, int pidInNs)
const char *argv[] = {
"ip", "link", "set", iface, "netns",
NULL, NULL
};
- int cmdResult;
- if (NULL == iface) {
+ if (NULL == iface)
goto error_out;
- }
if (virAsprintf(&pid, "%d", pidInNs) == -1)
goto error_out;
argv[5] = pid;
- rc = virRun(argv, &cmdResult);
- if (0 == rc)
- rc = cmdResult;
+ rc = virRun(argv, NULL);
+ if (rc < 0)
+ rc = -1;
error_out:
VIR_FREE(pid);
@@ -230,22 +207,17 @@ error_out:
*/
int setMacAddr(const char* iface, const char* macaddr)
{
- int rc = -1;
const char *argv[] = {
"ip", "link", "set", iface, "address",
macaddr, NULL
};
- int cmdResult;
- if (NULL == iface) {
- goto error_out;
- }
+ if (NULL == iface)
+ return -1;
- rc = virRun(argv, &cmdResult);
- if (0 == rc)
- rc = cmdResult;
+ if (virRun(argv, NULL) < 0)
+ return -1;
-error_out:
- return rc;
+ return 0;
}
/**
@@ -261,20 +233,15 @@ error_out:
*/
int setInterfaceName(const char* iface, const char* new)
{
- int rc = -1;
const char *argv[] = {
"ip", "link", "set", iface, "name", new,
NULL
};
- int cmdResult;
- if (NULL == iface || NULL == new) {
- goto error_out;
- }
+ if (NULL == iface || NULL == new)
+ return -1;
- rc = virRun(argv, &cmdResult);
- if (0 == rc)
- rc = cmdResult;
+ if (virRun(argv, NULL) < 0)
+ return -1;
-error_out:
- return rc;
+ return 0;
}