Hi Jim,
Thanks for the review. Answers below -
Jim Meyering wrote:
> Dave Leskovec <dlesko@xxxxxxxxxxxxxxxxxx> 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
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list