On 09/26/2012 10:44 AM, Daniel P. Berrange wrote:
> From: "Daniel P. Berrange" <berrange@xxxxxxxxxx>
>
> 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...)
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list