[libvirt] [PATCH 1/2] qemu: make sure capability probing process can start

[Daniel P. Berrange]

On Thu, Oct 09, 2014 at 11:49:34AM +0200, Martin Kletzander wrote:
> On Thu, Oct 09, 2014 at 10:14:48AM +0100, Daniel P. Berrange wrote:
> >On Thu, Oct 09, 2014 at 09:58:30AM +0200, Martin Kletzander wrote:
> >>When daemon is killed right in the middle of probing a qemu binary for
> >>its capabilities, the VM is left running.  Next time the daemon is
> >>starting, it cannot start qemu process because the one that's already
> >>running does have the pidfile flock()'d.
> >
> >I was wondering if there's anything we can easily change in the way
> >we launch the QEMU binary so that it automatically dies when libvirtd
> >exits, rather than us needing to manually kill it.
> >
> >The comments say we have to use daemonize to synchronize with the
> >monitor socket creation and I recall we've tried other approaches
> >to that before which failed.
> >
> >Another idea would be to play with adding '-serial stdio' and then
> >when libvirt died stdio would get a broken pipe but I don't think
> >it is safe to use -serial when we have -M none so that's out.
> >
> >So I guss we don't have much choice but to manually kill.
> >
> >>Reported-by: Wang Yufei <james.wangyufei at huawei.com>
> >>
> >>Signed-off-by: Martin Kletzander <mkletzan at redhat.com>
> >>---
> >> src/qemu/qemu_capabilities.c | 41 +++++++++++++++++++++++++++++++++++++++++
> >> 1 file changed, 41 insertions(+)
> >>
> >>diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> >>index 6fcb5c7..919780e 100644
> >>--- a/src/qemu/qemu_capabilities.c
> >>+++ b/src/qemu/qemu_capabilities.c
> >>@@ -3243,6 +3243,47 @@ virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps,
> >>     config.data.nix.path = monpath;
> >>     config.data.nix.listen = false;
> >>
> >>+    /*
> >>+     * Check if there wasn't some older qemu left running, e.g. if
> >>+     * someone killed libvirtd during the probing phase.
> >>+     */
> >>+    if (virFileExists(pidfile) &&
> >>+        virPidFileReadPath(pidfile, &pid) == 0) {
> >>+        char *cmdpath = NULL;
> >>+        char *cmdline = NULL;
> >>+        int len, pos = 0;
> >>+
> >>+        VIR_DEBUG("Checking if there's an older qemu process left running that "
> >>+                  "was used for capability probing");
> >>+
> >>+        if (virAsprintf(&cmdpath, "/proc/%u/cmdline", pid) < 0)
> >>+            goto cleanup;
> >>+
> >>+        len = virFileReadAll(cmdpath, 1024, &cmdline);
> >>+        VIR_FREE(cmdpath);
> >>+
> >>+        /*
> >>+         * This cycle gets trivially skipped if there was an error in
> >>+         * virFileReadAll().
> >>+         */
> >>+        while (len > pos) {
> >>+            /*
> >>+             * Find the '-pidfile /path/to/capabilities.pidfile' to be
> >>+             * sure we won't kill anyone else.
> >>+             */
> >>+            if (STREQ_NULLABLE(cmdline + pos, "-pidfile") &&
> >>+                (pos += strlen(cmdline + pos) + 1) < len &&
> >>+                STREQ_NULLABLE(cmdline + pos, pidfile)) {
> >
> >Heh fun hack. Did you consider simply trying to connect to the monitor
> >socket to see if the QEMU was still there ?  That would be slightly more
> >portable as it wouldn't rely on Linux /proc
> >
> 
> I wanted to cooperate with qemu somehow, for example killing it if we
> are handling a signal (that wouldn't help if the daemon is SIGKILL'd),
> I thought we can add some timeout option to qemu (or it's parent
> process that would kill it if there was no communication for a while),
> but that doesn't make much sense and so on.  Michal then told me that
> we can just kill it if it still exists.  And that's something that
> would still need to be there if the monitor is not responding, for
> example.
> 
> Since all this machinery happens only after libvirtd has its pidfile
> flock()'d, we can be sure we're not killing anyone else's qemu.
> 
> What would be the platforms this wouldn't run on?  I can only think of
> BSD where /proc doesn't have to be mounted.  Can we somehow require
> that?

Does BSD's /proc actally include the 'cmdline' file ?  I know a number
of OS include /proc but I always thought they have completely different
contents from each other, which is why i was concerned in this case.

Regards,
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 :|