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

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