[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



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.

For veth.c all functions match the latter case while bridge.c has both.
If we don't take care about the consistency between veth.c and bridge.c,
we can focus on how to treat the latter case. (Ideally keeping the consistency
is better, but it seems difficult...)

>
> 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 -

Yes.

> remember that it gives the name of the program being run, and "virRun", but
> not the name of the calling function).

Agreed. That'll lose useful information for debugging. One option is
to re-report
an error in the calling function. It will lead reporting two messages,
but it should
be more useful than less reports.

One concern aobut virRun's error reporting is that it shows standard
errors through
DEBUG so by default it shows nothing while they are important for ifconfig
and ip commands because their error messages may be according to errno.

>
> 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.

That seems good idea. One exception is virAsprintf in moveInterfaceToNetNs,
although we can simply use virReportOOMError() for it and return -ENOMEM.

>
> 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.

Actually I had an experience that 'ip' command was not installed in a container
and I got an error not relating to errno. In the case, it was useful for me that
reporting errors in both a caller function and virRun.

I have now notified that mixing use of -errno and -1 is not good idea because
-1 is interpreted as -EPERM. (That's a reason why errnos and exit codes are
returned as is?)

  ozaki-r

>
> --
> libvir-list mailing list
> libvir-list redhat com
> https://www.redhat.com/mailman/listinfo/libvir-list
>


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]