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? Thanks, ozaki-r > > (I was recently bitten by a similar bug...) > >> lxcContainerRenameAndEnableInterfaces is expected to return a negative >> value on a failure, so the patch changes the return value to -1 >> if vethInterfaceUpOrDown fails. >> >> 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. >> --- >> src/lxc/lxc_container.c | 7 ++++++- >> src/lxc/lxc_controller.c | 2 +- >> 2 files changed, 7 insertions(+), 2 deletions(-) >> >> diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c >> index 4371dba..c77d262 100644 >> --- a/src/lxc/lxc_container.c >> +++ b/src/lxc/lxc_container.c >> @@ -273,8 +273,13 @@ static int >> lxcContainerRenameAndEnableInterfaces(unsigned int nveths, >> } >> >> /* enable lo device only if there were other net devices */ >> - if (veths) >> + if (veths) { >> rc = vethInterfaceUpOrDown("lo", 1); >> + if (0 != rc) { >> + VIR_ERROR(_("Failed to enable lo (%d)"), rc); >> + rc = -1; >> + } >> + } >> >> error_out: >> VIR_FREE(newname); >> diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c >> index d8b7bc7..9829a69 100644 >> --- a/src/lxc/lxc_controller.c >> +++ b/src/lxc/lxc_controller.c >> @@ -477,7 +477,7 @@ static int lxcControllerMoveInterfaces(unsigned int >> nveths, >> { >> unsigned int i; >> for (i = 0 ; i< nveths ; i++) >> - if (moveInterfaceToNetNs(veths[i], container)< 0) { >> + if (moveInterfaceToNetNs(veths[i], container) != 0) { >> lxcError(VIR_ERR_INTERNAL_ERROR, >> _("Failed to move interface %s to ns %d"), >> veths[i], container); > > -- > libvir-list mailing list > libvir-list@xxxxxxxxxx > https://www.redhat.com/mailman/listinfo/libvir-list > -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list