Hi Jim,
Thanks for the review. Answers below -
Jim Meyering wrote:
Dave Leskovec <dlesko(a)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