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

[Laine Stump]

  On 07/27/2010 11:11 AM, Ryota Ozaki wrote:
> 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:
>>>> 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...)

We can ignore bridge.c for now - there is a lot of code in libvirt that 
doesn't follow the "-errno on failure" convention, so we should try to 
be as narrow as possible. For this cleanup, I like the scope of just 
veth.c and its direct callers.

Another thing I didn't notice when I wrote my first comment on this 
thread is that most of the error conditions in the involved functions 
are due to an external program failing, which will likely be due to the 
program returning a non-0 code. However, that's not *always* the case, 
so we can't really say that the function will always return a valid 
"-exitcode", nor can we say it will always return a valid "-errno" (and 
as you point out, they can conflict).


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

I also recall once in some other code letting virRun display the error 
and the result was that someone else who got the error (which was really 
unimportant) was handed a giant scary looking (and uninformative) error 
message that took their attention away from the real problem, which was 
elsewhere (I forget the details now, but you get the idea). It's 
convenient, but if you want meaningful errors, I would recommend against 
letting virRun report them.

I went through all the calls to all the functions listed in veth.h, and 
I see that they are called from relatively few places, and the error 
logs are almost always to the veth.c function that's being called, not 
to the caller. So I think it could work to have:

1) all calls to virRun in veth.c request cmdResult.

2) all functions in veth.c log the errors themselves (with one possible 
exception - vethDelete() - because it is often called during error 
recovery).

3) all functions in veth.c return -1 to their caller

4) all functions called by those functions also return -1 (this is 
mostly the case already, once you change veth.c, and has no effect on 
those functions' callers).

Logging errors in veth.c functions you have all the information you need 
from virRun, but also know the context in which the program is being 
called (see my notes below) so you can avoid the "always big and scary" 
messages from virRun, and you can consistently return -1.

(The only downside is that you don't have access to the stderr of the 
executed program, but that will be solved when we switch to Dan 
Berrange's new exec library ;-)

Does this sound reasonable and doable? If you agree with it, but think 
it's not doable in time for the release on Friday, but the original bug 
has real consequences (ie it makes something potentially fail), then I 
think it would be okay to push your original patch, with intent to do a 
further cleanup after the release.

(sorry for turning this 2 line bugfix into an odyssey of refactoring - 
I'm just paying it forward ;-)

*****************************
Here are my notes from looking at the functions in veth.c and their callers:

vethCreate

    called from:
         lxcSetupInterfaces - on error, it logs "Failed to create device 
pair %d", rc

    solution - log that error in vethCreate, and have vethCreate return 
-1 on failure

vethDelete

    called from:
      lxcControllerCleanupInterfaces - logs, but doesn't display code
      lxcVmCleanup - ignores, no log
      lxcVmStart - ignores, no log

Both of the cases where there is no log on failure are cases where there 
was already an error, and this is just cleaning up. The other case is, I 
think, as the container is shutting down. There's a good reason for not 
logging an error if you're already in error recovery - the newer error 
message will overwrite the old one (eg, virsh only prints out the last 
error that was logged during any given API call).

    suggestion - since the existing error log doesn't reference the 
return code, just have vethDelete return -1, and continue logging the 
error in the caller (when desired).

vethInterfaceUpOrDown

   called from:
      lxcContainerRenameAndEnableInterfaces - enabling, log error
      lxcVmCleanup - disabling, don't log error
      lxcSetupInterfaces - enabling, log error

This one doesn't log an error if upOrDown is 0, and in the two cases 
where it does log an error, the text is nearly identical.

   suggestion - log errors in vethInterfaceUpOrDown if upOrDown != 0, 
and return -1 on failure. Don't log in the caller.

moveInterfaceToNetNs

   called from: lxcControllerMoveInterfaces - error is logged, but 
doesn't give code.

   suggestion - do the logging in moveInterfaceToNetNs instead, and add 
in the code, then return -1

setMacAddr

   called from: lxcSetupInterfaces - error is logged, but doesn't give code

   suggestion - do the logging in setMacAddr, and add in the code, then 
return -1

setInterfaceName

   called from: lxcContainerRenameAndEnableInterfaces - code is saved, 
then used to log error

   suggestion - log the same error in setInterfaceName that was 
previously logged in the caller, then return -1.