[Libvir] [PATCH 2/2] lxc: Shutdown and destroy container

[Dave Leskovec]

Hi Jim,

Responses inline below and I've attached an updated patch.  Thanks!

Jim Meyering wrote:
> Dave Leskovec <dlesko at linux.vnet.ibm.com> wrote:
> 
>> This is a repost of the shutdown and destroy container support.  Changes in this
> 
> A few comments:
> ...
>> +static int lxcDomainDestroy(virDomainPtr dom)
>> +{
>> +    int rc = -1;
>> +    lxc_driver_t *driver = (lxc_driver_t*)dom->conn->privateData;
>> +    lxc_vm_t *vm = lxcFindVMByID(driver, dom->id);
>> +    int childStatus;
>> +
>> +    if (!vm) {
>> +        lxcError(dom->conn, dom, VIR_ERR_INVALID_DOMAIN,
>> +                 _("no domain with id %d"), dom->id);
>> +        goto error_out;
>> +    }
>> +
>> +    if (0 > (kill(vm->def->id, SIGKILL))) {
>> +        if (ESRCH != errno) {
>> +            lxcError(dom->conn, dom, VIR_ERR_INTERNAL_ERROR,
>> +                     _("sending SIGKILL failed: %s"), strerror(errno));
>> +
>> +            goto error_out;
> 
> I wondered...
> If this kill fails due to non-ESRCH, is it worth trying to kill vm->pid?
> Seeing that the only other kill errno values are EINVAL and EPERM,
> I guess not.

Ya, if we get one of those errors for the container, chances are about 100% that
we'll get it for the tty process as well.

> 
>> +        }
> 
> Does the ESRCH case deserve some sort of diagnostic?  (I don't know)

This would happen if the container has already exited.  This may be the case if
the container was running some job that ran til completion and exited or
crashed.  We don't really know what the intended behavior was so I don't think
we can call it an error.

> 
>> +    }
>> +
>> +    vm->state = VIR_DOMAIN_SHUTDOWN;
>> +
>> +    waitpid(vm->def->id, &childStatus, 0);
> 
> How about waiting only if the signal was successfully sent?
> And detect waitpid failure.

If we failed to send the signal and errno != ESRCH, then we won't get here.  If
errno = ESRCH, then we still may need to wait on the process to get rid of the
zombie and capture the exit status.  Added some error handling for the waitpid()
calls.

> 
>> +    rc = WEXITSTATUS(childStatus);
>> +    DEBUG("container exited with rc: %d", rc);
>> +
>> +    /* also need to kill tty forward process */
>> +    /* wrap this with error handling etc.
> 
> yes ;-)

:-) added this and removed the comments

> 
>>> in the right place? */
> 
> Looks ok to me.
> 
>> +    /* also wait for the process */
>> +    kill(vm->pid, SIGKILL);
> ...

Thanks!

-- 
Best Regards,
Dave Leskovec
IBM Linux Technology Center
Open Virtualization


-------------- next part --------------
A non-text attachment was scrubbed...
Name: lxc_stop.patch
Type: text/x-patch
Size: 3931 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20080409/237bb7eb/attachment-0001.bin>