On Sat, Jul 24, 2010 at 11:44 PM, Ryota Ozaki <ozaki.ryota@xxxxxxxxx> wrote:
> Hi Laine,
>
> On Sat, Jul 24, 2010 at 2:58 AM, Laine Stump <laine@xxxxxxxxx> 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;
}
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list