Re: [PATCH] qemu: Kill processes used for QMP caps probing

[Jiri Denemark <jdenemar@xxxxxxxxxx>]

On Tue, Oct 02, 2012 at 13:08:13 +0200, Peter Krempa wrote:
> On 10/02/12 11:07, Jiri Denemark wrote:
> > Since libvirt switched to QMP capabilities probing recently, it starts
> > QEMU process used for this probing with -daemonize, which means
> > virCommandAbort can no longer reach these processes. As a result of
> > that, restarting libvirtd will leave several new QEMU processes behind.
> > Let's use QEMU's -pidfile and use it to kill the process when QMP caps
> > probing is done.
> > ---
> >   src/qemu/qemu_capabilities.c | 51 ++++++++++++++++++++++++++++++++++++++------
> >   src/qemu/qemu_capabilities.h |  5 +++--
> >   src/qemu/qemu_driver.c       |  4 +++-
> >   3 files changed, 51 insertions(+), 9 deletions(-)
> >
> > diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> > index 20b350a..bfefa92 100644
> > --- a/src/qemu/qemu_capabilities.c
> > +++ b/src/qemu/qemu_capabilities.c
> 
> > @@ -2300,12 +2317,32 @@ cleanup:
> >       virCommandFree(cmd);
> >       VIR_FREE(monarg);
> >       VIR_FREE(monpath);
> > +
> > +    if (pidfile) {
> > +        char ebuf[1024];
> > +        pid_t pid;
> > +        int rc;
> > +
> > +        if ((rc = virPidFileReadPath(pidfile, &pid)) < 0) {
> > +            VIR_DEBUG("Failed to read pidfile %s: %d",
> > +                      pidfile, virStrerror(-rc, ebuf, sizeof(ebuf)));
> 
> I'd use VIR_WARN here

I don't think it's a good idea. The pidfile may be missing for several valid
reasons, for example, because we jumped to cleanup before we even tried to
start qemu or when we started qemu but it failed.

> > +        } else {
> > +            VIR_DEBUG("Killing QMP caps process %lld", (long long) pid);
> > +            if (virProcessKill(pid, SIGKILL) < 0)
> > +                VIR_DEBUG("Failed to kill process %lld: %s",
> 
> and here. I think that if we possibly leave a running process behind, we 
> should be a little louder about that.

However, I agree with this and I actually turned this into VIR_ERROR. I
squashed the following diff:

    @@ -2328,8 +2328,8 @@ cleanup:
                           pidfile, virStrerror(-rc, ebuf, sizeof(ebuf)));
             } else {
                 VIR_DEBUG("Killing QMP caps process %lld", (long long) pid);
    -            if (virProcessKill(pid, SIGKILL) < 0)
    -                VIR_DEBUG("Failed to kill process %lld: %s",
    +            if (virProcessKill(pid, SIGKILL) < 0 && errno != ESRCH)
    +                VIR_ERROR(_("Failed to kill process %lld: %s"),
                               (long long) pid,
                               virStrerror(errno, ebuf, sizeof(ebuf)));
             }

> ACK.

Thanks and pushed.

Jirka

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list