Hi Laine, Hmm...I was eager to a conclusion. We need some more discussion to a better way. Nonetheless, I hope this fix and another which I found later to be included in the next release that is expected soon. Is it OK for you, Laine? Of course, I'll continue this discussion until we have a consensus. ozaki-r On Mon, Jul 26, 2010 at 11:37 PM, Laine Stump <laine@laine.org> wrote:
On 07/26/2010 08:31 AM, Ryota Ozaki wrote:
On Mon, Jul 26, 2010 at 6:51 PM, Daniel P. Berrange<berrange@redhat.com> wrote:
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
It looks better than mine ;-) I'll rewrite my patch in such a way.
Laine, is it ok for you too?
Doing that is fine when all possible failures in the function have an associated errno. In the case of virRun'ing an external program that could return a non-zero exit code, this is unfortunately not the case, so those functions that call virRun will need to report the error themselves and return -1.
When non-0 exits from the called program are all failures, the simplest way to do it is, as you say, to just not pass a pointer to a resultcode to virRun (as long as the error message reported by virRun is useful enough - remember that it gives the name of the program being run, and "virRun", but not the name of the calling function).
setMacAddr gives another example of a failure mode that breaks the "return -errno" paradigm - it checks for one of the arguments being NULL, and fails in that case. If it's important to maintain "-errno on failure" in one of those cases, possibly examining the code will show that the arg in question is never NULL, in which case you can mark it in its prototype with ATTRIBUTE_NONNULL and just eliminate that failure from the code.
It seems that in general, the -errno convention works better at lower levels where all the failures are related to some system call failing, but at some point higher in the call chain the possibility of failure due to config, etc, comes in, there is no valid errno to describe a problem, and then you need to start reporting the errors and returning -1.
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list