Hi Jim, Thanks for the review. Answers below - Jim Meyering wrote:
Dave Leskovec <dlesko@linux.vnet.ibm.com> wrote:
This patch allows the lxc driver to handle SIGCHLD signals from exiting ...
Hi Dave, At least superficially, this looks fine. Two questions:
Index: b/src/driver.h =================================================================== --- a/src/driver.h 2008-04-10 09:54:54.000000000 -0700 +++ b/src/driver.h 2008-04-30 15:36:47.000000000 -0700 @@ -11,6 +11,10 @@
#include <libxml/uri.h>
+#ifndef _SIGNAL_H +#include <signal.h> +#endif
In practice it's fine to include <signal.h> unconditionally, and even multiple times. Have you encountered a version of <signal.h> that may not be included twice? If so, it probably deserves a comment with the details.
No, I don't have any special condition here. This is probably some past conditioning resurfacing briefly. If I remember correctly, it had more to do with compile efficiency rather than avoiding compile failures from multiple inclusions.
...
Index: b/src/lxc_driver.c =================================================================== ... -static int lxcDomainDestroy(virDomainPtr dom) +static int lxcVMCleanup(lxc_driver_t *driver, lxc_vm_t * vm) { int rc = -1; ... - rc = WEXITSTATUS(childStatus); - DEBUG("container exited with rc: %d", rc); + if (WIFEXITED(childStatus)) { + rc = WEXITSTATUS(childStatus); + DEBUG("container exited with rc: %d", rc); + } + + rc = 0;
Didn't you mean to initialize rc=0 before that if block? If not, please add a comment saying why the child failure doesn't affect the function's return value.
Nice. Yes that rc = 0 definitely shouldn't be there. -- Best Regards, Dave Leskovec IBM Linux Technology Center Open Virtualization