From: "Daniel P. Berrange" <berrange(a)redhat.com&gt; In the cgroups APIs we have a virCgroupKillPainfully function which does the loop sending SIGTERM, then SIGKILL and waiting for the process to exit. There is similar functionality for simple processes in qemuProcessKill, but it is tangled with the QEMU code. Untangle it to provide a virProcessKillPainfuly function

--- src/libvirt_private.syms | 1 + src/qemu/qemu_driver.c | 8 ++--- src/qemu/qemu_process.c | 79 ++++++++---------------------------------------- src/util/virprocess.c | 57 ++++++++++++++++++++++++++++++++++ src/util/virprocess.h | 2 ++ 5 files changed, 76 insertions(+), 71 deletions(-)

+++ b/src/qemu/qemu_driver.c @@ -2006,13 +2006,11 @@ qemuDomainDestroyFlags(virDomainPtr dom, * it now means the job will be released */ if (flags & VIR_DOMAIN_DESTROY_GRACEFUL) { - if (qemuProcessKill(driver, vm, 0) < 0) { - virReportError(VIR_ERR_OPERATION_FAILED, "%s", - _("failed to kill qemu process with SIGTERM")); + if (qemuProcessKill(driver, vm, 0) < 0) goto cleanup; - } } else { - ignore_value(qemuProcessKill(driver, vm, VIR_QEMU_PROCESS_KILL_FORCE)); + if (qemuProcessKill(driver, vm, VIR_QEMU_PROCESS_KILL_FORCE) < 0) + goto cleanup;

+++ b/src/util/virprocess.c @@ -235,3 +235,60 @@ int virProcessKill(pid_t pid, int sig) return kill(pid, sig); #endif } + + +/* + * Try to kill the process and verify it has exited + * + * Returns 0 if it was killed gracefully, 1 if it + * was killed forcably, -1 if it is still alive,

+ * or another error occurred. + */ +int +virProcessKillPainfully(pid_t pid, bool force) +{ + int i, ret = -1; + const char *signame = "TERM"; + + VIR_DEBUG("vpid=%d force=%d", pid, force); + + /* This loop sends SIGTERM, then waits a few iterations (10 seconds) + * to see if it dies. If the process still hasn't exited, and + * @force is requested, a SIGKILL will be sent, and this will + * wait upto 5 seconds more for the process to exit before

+ * returning. + * + * Note that setting @force could result in dataloss for the process.

On Wed, Sep 26, 2012 at 10:07:42AM -0600, Eric Blake wrote: >> +++ b/src/qemu/qemu_driver.c >> @@ -2006,13 +2006,11 @@ qemuDomainDestroyFlags(virDomainPtr dom, >> * it now means the job will be released >> */ >> if (flags & VIR_DOMAIN_DESTROY_GRACEFUL) { >> - if (qemuProcessKill(driver, vm, 0) < 0) { >> - virReportError(VIR_ERR_OPERATION_FAILED, "%s", >> - _("failed to kill qemu process with SIGTERM")); >> + if (qemuProcessKill(driver, vm, 0) < 0) >> goto cleanup; >> - } >> } else { >> - ignore_value(qemuProcessKill(driver, vm, VIR_QEMU_PROCESS_KILL_FORCE)); >> + if (qemuProcessKill(driver, vm, VIR_QEMU_PROCESS_KILL_FORCE) < 0) >> + goto cleanup; > > Why are you changing this from ignore_value() into a cleanup path on > failure? Hmm, this is a semantic change, so I guess I ought to have it as a separate patch. The virDomainDestroy() function is supposed to guarantee that the domain has been killed. In the current code we were returning 0, even if qemuProcessKill failed to kill the process even with SIGKILL. If even SIGKILL fails, we really need to return an error code so apps can see that virDomainDestroy failed, and not think everything was fine.

On 09/26/2012 10:44 AM, Daniel P. Berrange wrote: > From: "Daniel P. Berrange" <berrange(a)redhat.com&gt; > > In the cgroups APIs we have a virCgroupKillPainfully function > which does the loop sending SIGTERM, then SIGKILL and waiting > for the process to exit. There is similar functionality for > simple processes in qemuProcessKill, but it is tangled with > the QEMU code. Untangle it to provide a virProcessKillPainfuly > function > --- > src/libvirt_private.syms | 1 + > src/qemu/qemu_driver.c | 8 ++--- > src/qemu/qemu_process.c | 79 ++++++++---------------------------------------- > src/util/virprocess.c | 57 ++++++++++++++++++++++++++++++++++ > src/util/virprocess.h | 2 ++ > 5 files changed, 76 insertions(+), 71 deletions(-) > > diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms > index 4635a4d..dab607a 100644 > --- a/src/libvirt_private.syms > +++ b/src/libvirt_private.syms > @@ -1708,6 +1708,7 @@ virPidFileDeletePath; > # virprocess.h > virProcessAbort; > virProcessKill; > +virProcessKillPainfully; > virProcessTranslateStatus; > virProcessWait; > > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c > index 7ac53ac..22fef7a 100644 > --- a/src/qemu/qemu_driver.c > +++ b/src/qemu/qemu_driver.c > @@ -2006,13 +2006,11 @@ qemuDomainDestroyFlags(virDomainPtr dom, > * it now means the job will be released > */ > if (flags & VIR_DOMAIN_DESTROY_GRACEFUL) { > - if (qemuProcessKill(driver, vm, 0) < 0) { > - virReportError(VIR_ERR_OPERATION_FAILED, "%s", > - _("failed to kill qemu process with SIGTERM")); > + if (qemuProcessKill(driver, vm, 0) < 0) > goto cleanup; > - } > } else { > - ignore_value(qemuProcessKill(driver, vm, VIR_QEMU_PROCESS_KILL_FORCE)); > + if (qemuProcessKill(driver, vm, VIR_QEMU_PROCESS_KILL_FORCE) < 0) > + goto cleanup; > } > > /* We need to prevent monitor EOF callback from doing our work (and sending > diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c > index 3cd30af..70b72af 100644 > --- a/src/qemu/qemu_process.c > +++ b/src/qemu/qemu_process.c > @@ -3877,9 +3877,7 @@ int > qemuProcessKill(struct qemud_driver *driver, > virDomainObjPtr vm, unsigned int flags) > { > - int i, ret = -1; > - const char *signame = "TERM"; > - bool driver_unlocked = false; > + int ret; > > VIR_DEBUG("vm=%s pid=%d flags=%x", > vm->def->name, vm->pid, flags); > @@ -3891,78 +3889,27 @@ qemuProcessKill(struct qemud_driver *driver, > } > } > > - /* This loop sends SIGTERM (or SIGKILL if flags has > - * VIR_QEMU_PROCESS_KILL_FORCE and VIR_QEMU_PROCESS_KILL_NOWAIT), > - * then waits a few iterations (10 seconds) to see if it dies. If > - * the qemu process still hasn't exited, and > - * VIR_QEMU_PROCESS_KILL_FORCE is requested, a SIGKILL will then > - * be sent, and qemuProcessKill will wait up to 5 seconds more for > - * the process to exit before returning. Note that the FORCE mode > - * could result in lost data in the guest, so it should only be > - * used if the guest is hung and can't be destroyed in any other > - * manner. > - */ > - for (i = 0 ; i < 75; i++) { > - int signum; > - if (i == 0) { > - if ((flags & VIR_QEMU_PROCESS_KILL_FORCE) && > - (flags & VIR_QEMU_PROCESS_KILL_NOWAIT)) { > - signum = SIGKILL; /* kill it immediately */ > - signame="KILL"; > - } else { > - signum = SIGTERM; /* kindly suggest it should exit */ > - } > - } else if ((i == 50) & (flags & VIR_QEMU_PROCESS_KILL_FORCE)) { > - VIR_WARN("Timed out waiting after SIG%s to process %d, " > - "sending SIGKILL", signame, vm->pid); > - signum = SIGKILL; /* kill it after a grace period */ > - signame="KILL"; > - } else { > - signum = 0; /* Just check for existence */ > - } > - > - if (virProcessKill(vm->pid, signum) < 0) { > - if (errno != ESRCH) { > - char ebuf[1024]; > - VIR_WARN("Failed to terminate process %d with SIG%s: %s", > - vm->pid, signame, > - virStrerror(errno, ebuf, sizeof(ebuf))); > - goto cleanup; > - } > - ret = 0; > - goto cleanup; /* process is dead */ > - } > + if ((flags & VIR_QEMU_PROCESS_KILL_NOWAIT)) { > + virProcessKill(vm->pid, > + (flags & VIR_QEMU_PROCESS_KILL_FORCE) ? > + SIGKILL : SIGTERM); > + return 0; > + } > > - if (i == 0 && (flags & VIR_QEMU_PROCESS_KILL_NOWAIT)) { > - ret = 0; > - goto cleanup; > - } > + if (driver) > + qemuDriverUnlock(driver); > > - if (driver && !driver_unlocked) { > - /* THREADS.txt says we can't hold the driver lock while sleeping */ > - qemuDriverUnlock(driver); > - driver_unlocked = true; > - } > + ret = virProcessKillPainfully(vm->pid, > + !!(flags & VIR_QEMU_PROCESS_KILL_FORCE)); > > - usleep(200 * 1000); > - } > - VIR_WARN("Timed out waiting after SIG%s to process %d", signame, vm->pid); > -cleanup: > - if (driver_unlocked) { > - /* We had unlocked the driver, so re-lock it. THREADS.txt says > - * we can't have the domain locked when locking the driver, so > - * we must first unlock the domain. BUT, before we can unlock > - * the domain, we need to add a ref to it in case there aren't > - * any active jobs (analysis of all callers didn't reveal such > - * a case, but there are too many to maintain certainty, so we > - * will do this as a precaution). > - */ > + if (driver) { > virObjectRef(vm); > virDomainObjUnlock(vm); > qemuDriverLock(driver); > virDomainObjLock(vm); > virObjectUnref(vm); > } > + > return ret; > } > > diff --git a/src/util/virprocess.c b/src/util/virprocess.c > index 958f5f7..c70aa58 100644 > --- a/src/util/virprocess.c > +++ b/src/util/virprocess.c > @@ -235,3 +235,60 @@ int virProcessKill(pid_t pid, int sig) > return kill(pid, sig); > #endif > } > + > + > +/* > + * Try to kill the process and verify it has exited > + * > + * Returns 0 if it was killed gracefully, 1 if it > + * was killed forcably, -1 if it is still alive, > + * or another error occurred. > + */ > +int > +virProcessKillPainfully(pid_t pid, bool force) > +{ > + int i, ret = -1; > + const char *signame = "TERM"; > + > + VIR_DEBUG("vpid=%d force=%d", pid, force); > + > + /* This loop sends SIGTERM, then waits a few iterations (10 seconds) > + * to see if it dies. If the process still hasn't exited, and > + * @force is requested, a SIGKILL will be sent, and this will > + * wait upto 5 seconds more for the process to exit before > + * returning. > + * > + * Note that setting @force could result in dataloss for the process. > + */ > + for (i = 0 ; i < 75; i++) { > + int signum; > + if (i == 0) { > + signum = SIGTERM; /* kindly suggest it should exit */ > + } else if ((i == 50) & force) { > + VIR_DEBUG("Timed out waiting after SIGTERM to process %d, " > + "sending SIGKILL", pid); > + signum = SIGKILL; /* kill it after a grace period */ > + signame = "KILL"; > + } else { > + signum = 0; /* Just check for existence */ > + } Hmm. I just added a similar kill loop in bridge_driver.c. The main difference is that it doesn't wait as long (not for any particular reason, though). I guess I should change it to use this. (I actually considered making a utility function at the time, but it was too close to release, so I didn't want to touch other drivers...)