Re: [libvirt] [PATCH] lxc: Fix return value handlings of vethInterfaceUpOrDown and moveInterfaceToNetNs
On 07/26/2010 08:31 AM, Ryota Ozaki wrote:
On Mon, Jul 26, 2010 at 6:51 PM, Daniel P.
Berrange<berrange(a)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(a)gmail.com>
wrote:
>>> Hi Laine,
>>>
>>> On Sat, Jul 24, 2010 at 2:58 AM, Laine Stump<laine(a)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.