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/ dan...@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/ -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
