[libvirt] [PATCH] lxc: Fix return value handlings of vethInterfaceUpOrDown and moveInterfaceToNetNs

[Laine Stump]

  On 07/26/2010 08:31 AM, Ryota Ozaki wrote:
> On Mon, Jul 26, 2010 at 6:51 PM, Daniel P. Berrange<berrange at 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 at gmail.com>  wrote:
>>>> Hi Laine,
>>>>
>>>> On Sat, Jul 24, 2010 at 2:58 AM, Laine Stump<laine at 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.