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

[Ryota Ozaki]

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 at 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 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.
>
> --
> libvir-list mailing list
> libvir-list at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
>