On Sun, Jul 25, 2010 at 02:25:05AM +0900, Ryota Ozaki wrote:
On Sat, Jul 24, 2010 at 11:44 PM, Ryota Ozaki <ozaki.ryota@gmail.com> wrote:
Hi Laine,
On Sat, Jul 24, 2010 at 2:58 AM, Laine Stump <laine@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?
You could just change return cmdResult to return -cmdResult; That would still let you give the error code, while also keeping the value < 0 Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|