Re: [libvirt] [PATCHv3] lxc: Fix return values of veth.c functions - complete

On Fri, Jul 30, 2010 at 2:32 AM, Laine Stump&lt;laine(a)laine.org&gt; wrote: > From: Ryota Ozaki&lt;ozaki.ryota(a)gmail.com&gt; > > Previously, the functions in src/lxc/veth.c could sometimes return > positive values on failure rather than -1. This made accurate error > reporting difficult, and led to one failure to catch an error in a > calling function. > > This patch makes all the functions in veth.c consistently return 0 on > success, and -1 on failure. It also fixes up the callers to the veth.c > functions where necessary. > > Note that this patch may be related to the bug: > > https://bugzilla.redhat.com/show_bug.cgi?id=607496. > > It will not fix the bug, but should unveil what happens. > > * po/POTFILES.in - add veth.c, which previously had no translatable strings > * src/lxc/lxc_controller.c > * src/lxc/lxc_container.c > * src/lxc/lxc_driver.c - fixup callers to veth.c, and remove error logs, > as they are now done in veth.c > * src/lxc/veth.c - make all functions consistently return -1 on error. > * src/lxc/veth.h - use ATTRIBUTE_NONNULL to protect against NULL args. > --- > Changes from Ozaki's original patch: > > 1) virAsprintf() will return the number of characters in the new > string on success, not 0, so we need to only set rc if it fails > (< 0). Assigning rc on success causes the caller to falsely believe > the function failed. > > 2) lxcVmCleanup was always doing the if (WIFEXITED() ...) even > if waitpid had failed. I don't know if the behavior of WIFEXITED > is defined if waitpid fails, but all the other uses I can find > avoid calling WIFEXITED and WEXITSTATUS if waitpid fails, so that's > what I did here. Also changed to return -1 if waitpid fails or > WEXITSTATUS != 0. > > 3) lxcSetupInterfaces - rather than explicitly setting rc from the > return of functions, since it defaults to -1, I just goto > error_exit if the functions return< 0. That's just cosmetic. The > real problem is that rc was being set from brAddInterface, which > returns> 0 on failure (note error log of errno from brAddInterface > fixed in v3). > > 4) assigning "rc = -1" at the beginning of each veth.c function is a > dead store, since it will always be set by the call to virRun(). This > causes coverity code analysis tool to report problems. ACK. Thanks, Laine!