Hi Jim,
Responses inline below and I've attached an updated patch. Thanks!
Jim Meyering wrote:
> Dave Leskovec <dlesko@xxxxxxxxxxxxxxxxxx> 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
---
src/lxc_driver.c | 120 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 118 insertions(+), 2 deletions(-)
Index: b/src/lxc_driver.c
===================================================================
--- a/src/lxc_driver.c 2008-04-09 22:47:41.000000000 -0700
+++ b/src/lxc_driver.c 2008-04-09 23:35:41.000000000 -0700
@@ -803,6 +803,122 @@
return dom;
}
+/**
+ * lxcDomainShutdown:
+ * @dom: Ptr to domain to shutdown
+ *
+ * Sends SIGINT to container root process to request it to shutdown
+ *
+ * Returns 0 on success or -1 in case of error
+ */
+static int lxcDomainShutdown(virDomainPtr dom)
+{
+ int rc = -1;
+ lxc_driver_t *driver = (lxc_driver_t*)dom->conn->privateData;
+ lxc_vm_t *vm = lxcFindVMByID(driver, dom->id);
+
+ 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, SIGINT))) {
+ if (ESRCH != errno) {
+ lxcError(dom->conn, dom, VIR_ERR_INTERNAL_ERROR,
+ _("sending SIGTERM failed: %s"), strerror(errno));
+
+ goto error_out;
+ }
+ }
+
+ vm->state = VIR_DOMAIN_SHUTDOWN;
+
+ rc = 0;
+
+error_out:
+ return rc;
+}
+
+/**
+ * lxcDomainDestroy:
+ * @dom: Ptr to domain to destroy
+ *
+ * Sends SIGKILL to container root process to terminate the container
+ *
+ * Returns 0 on success or -1 in case of error
+ */
+static int lxcDomainDestroy(virDomainPtr dom)
+{
+ int rc = -1;
+ int waitRc;
+ 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;
+ }
+ }
+
+ vm->state = VIR_DOMAIN_SHUTDOWN;
+
+ while (((waitRc = waitpid(vm->def->id, &childStatus, 0)) == -1) &&
+ errno == EINTR);
+
+ if (waitRc != vm->def->id) {
+ lxcError(dom->conn, dom, VIR_ERR_INTERNAL_ERROR,
+ _("waitpid failed to wait for container %d: %d %s"),
+ vm->def->id, waitRc, strerror(errno));
+ goto kill_tty;
+ }
+
+ rc = WEXITSTATUS(childStatus);
+ DEBUG("container exited with rc: %d", rc);
+
+kill_tty:
+ if (0 > (kill(vm->pid, SIGKILL))) {
+ if (ESRCH != errno) {
+ lxcError(dom->conn, dom, VIR_ERR_INTERNAL_ERROR,
+ _("sending SIGKILL to tty process failed: %s"),
+ strerror(errno));
+
+ goto tty_error_out;
+ }
+ }
+
+ while (((waitRc = waitpid(vm->pid, &childStatus, 0)) == -1) &&
+ errno == EINTR);
+
+ if (waitRc != vm->pid) {
+ lxcError(dom->conn, dom, VIR_ERR_INTERNAL_ERROR,
+ _("waitpid failed to wait for tty %d: %d %s"),
+ vm->pid, waitRc, strerror(errno));
+ }
+
+tty_error_out:
+ vm->state = VIR_DOMAIN_SHUTOFF;
+ vm->pid = -1;
+ vm->def->id = -1;
+ driver->nactivevms--;
+ driver->ninactivevms++;
+
+ rc = 0;
+
+error_out:
+ return rc;
+}
+
static int lxcStartup(void)
{
uid_t uid = getuid();
@@ -902,9 +1018,9 @@
lxcDomainLookupByName, /* domainLookupByName */
NULL, /* domainSuspend */
NULL, /* domainResume */
- NULL, /* domainShutdown */
+ lxcDomainShutdown, /* domainShutdown */
NULL, /* domainReboot */
- NULL, /* domainDestroy */
+ lxcDomainDestroy, /* domainDestroy */
lxcGetOSType, /* domainGetOSType */
NULL, /* domainGetMaxMemory */
NULL, /* domainSetMaxMemory */
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list