On Thu, Jul 29, 2010 at 12:18:24PM -0400, Laine Stump wrote:
Some suggested changes to your latest patch (I did the review by applying your patch, then examining the functions that were touched, focusing just on setting of rc)
Summary:
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.
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.
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. --- src/lxc/lxc_container.c | 6 ++++-- src/lxc/lxc_driver.c | 19 ++++++------------- src/lxc/veth.c | 12 ++++++------ 3 files changed, 16 insertions(+), 21 deletions(-)
Okay, looks fine too, ACK, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/