On 07/26/2010 08:31 AM, Ryota Ozaki wrote:
On Mon, Jul 26, 2010 at 6:51 PM, Daniel P. Berrange<berrange@xxxxxxxxxx> 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@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?
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@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list