[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH] Move most of qemuProcessKill into virProcessKillPainfully



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" <berrange 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 :|


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]