[Libvir] [PATCH] lxc: handle SIGCHLD from exiting container

[Dave Leskovec]

Hi Jim,

Thanks for the review.  Answers below -

Jim Meyering wrote:
> Dave Leskovec <dlesko at 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