On Thu, Sep 27, 2012 at 04:31:10PM -0400, Laine Stump wrote: > On 09/26/2012 10:44 AM, Daniel P. Berrange wrote: > > From: "Daniel P. Berrange" <berra...@redhat.com> > > > > 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...)
Yep, sounds good. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list