[libvirt] [PATCH v2 1/2] util: Introduce virPidFileForceCleanupPath
This function is used to cleanup a pidfile doing whatever it takes, even killing the owning process. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- v2: - Don't use "/proc", but simply just try to acquire the pidfile. - https://www.redhat.com/archives/libvir-list/2014-October/msg00320.html src/libvirt_private.syms | 1 + src/util/virpidfile.c | 42 ++++++++++++++++++++++++++++++++++++++++++ src/util/virpidfile.h | 2 ++ 3 files changed, 45 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index d6265ac..30d100d 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1810,6 +1810,7 @@ virPidFileBuildPath; virPidFileConstructPath; virPidFileDelete; virPidFileDeletePath; +virPidFileForceCleanupPath; virPidFileRead; virPidFileReadIfAlive; virPidFileReadPath; diff --git a/src/util/virpidfile.c b/src/util/virpidfile.c index a3b8846..a64a1cf 100644 --- a/src/util/virpidfile.c +++ b/src/util/virpidfile.c @@ -37,6 +37,7 @@ #include "c-ctype.h" #include "areadlink.h" #include "virstring.h" +#include "virprocess.h" #define VIR_FROM_THIS VIR_FROM_NONE @@ -567,3 +568,44 @@ virPidFileConstructPath(bool privileged, VIR_FREE(rundir); return ret; } + + +/** + * virPidFileForceCleanupPath: + * + * Check if the pidfile is left around and clean it up whatever it + * takes. This doesn't raise an error. This function must not be + * called multiple times with the same path, be it in threads or + * processes. + * + * Returns 0 if the pidfile was successfully cleaned up, -1 otherwise. + */ +int +virPidFileForceCleanupPath(const char *path) +{ + pid_t pid = 0; + int fd = -1; + + if (!virFileExists(path)) + return 0; + + if (virPidFileReadPath(path, &pid) < 0) + return -1; + + if (virPidFileAcquirePath(path, false, 0) == 0) { + virPidFileReleasePath(path, fd); + } else { + virResetLastError(); + + /* Only kill the process if the pid is valid one. 0 means + * there is somebody else doing the same pidfile cleanup + * machinery. */ + if (pid) + virProcessKillPainfully(pid, true); + + if (virPidFileDeletePath(path) < 0) + return -1; + } + + return 0; +} diff --git a/src/util/virpidfile.h b/src/util/virpidfile.h index ca1dbff..eb6516c 100644 --- a/src/util/virpidfile.h +++ b/src/util/virpidfile.h @@ -74,4 +74,6 @@ int virPidFileConstructPath(bool privileged, const char *progname, char **pidfile); +int virPidFileForceCleanupPath(const char *path) ATTRIBUTE_NONNULL(1); + #endif /* __VIR_PIDFILE_H__ */ -- 2.1.2
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. Reported-by: Wang Yufei <james.wangyufei@huawei.com> Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- v2: - Don't use "/proc", but simply just try to acquire the pidfile. - https://www.redhat.com/archives/libvir-list/2014-October/msg00320.html src/qemu/qemu_capabilities.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 6fcb5c7..8aedf3f 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -3243,6 +3243,8 @@ virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps, config.data.nix.path = monpath; config.data.nix.listen = false; + virPidFileForceCleanupPath(pidfile); + VIR_DEBUG("Try to get caps via QMP qemuCaps=%p", qemuCaps); /* -- 2.1.2
On 10/12/14 14:12, 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
s/VM/qemu process/ ? The qemu isn't running anything so it might confuse someone.
starting, it cannot start qemu process because the one that's already running does have the pidfile flock()'d.
Reported-by: Wang Yufei <james.wangyufei@huawei.com>
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- v2: - Don't use "/proc", but simply just try to acquire the pidfile. - https://www.redhat.com/archives/libvir-list/2014-October/msg00320.html
src/qemu/qemu_capabilities.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 6fcb5c7..8aedf3f 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -3243,6 +3243,8 @@ virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps, config.data.nix.path = monpath; config.data.nix.listen = false;
+ virPidFileForceCleanupPath(pidfile); + VIR_DEBUG("Try to get caps via QMP qemuCaps=%p", qemuCaps);
/*
ACK. It is a bugfix, but the code path is is critical. I'm leaning towards pushing this before the freeze but I'd feel better with yet another opinion. Peter
On Thu, Oct 30, 2014 at 10:42:05AM +0100, Peter Krempa wrote:
On 10/12/14 14:12, 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
s/VM/qemu process/ ? The qemu isn't running anything so it might confuse someone.
starting, it cannot start qemu process because the one that's already running does have the pidfile flock()'d.
Reported-by: Wang Yufei <james.wangyufei@huawei.com>
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- v2: - Don't use "/proc", but simply just try to acquire the pidfile. - https://www.redhat.com/archives/libvir-list/2014-October/msg00320.html
src/qemu/qemu_capabilities.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 6fcb5c7..8aedf3f 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -3243,6 +3243,8 @@ virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps, config.data.nix.path = monpath; config.data.nix.listen = false;
+ virPidFileForceCleanupPath(pidfile); + VIR_DEBUG("Try to get caps via QMP qemuCaps=%p", qemuCaps);
/*
ACK. It is a bugfix, but the code path is is critical. I'm leaning towards pushing this before the freeze but I'd feel better with yet another opinion.
I fixed both things you've pointed out and I'll leave the push after release since I already broke the build once :) Martin
Ping. On Sun, Oct 12, 2014 at 02:12:20PM +0200, Martin Kletzander wrote:
This function is used to cleanup a pidfile doing whatever it takes, even killing the owning process.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- v2: - Don't use "/proc", but simply just try to acquire the pidfile. - https://www.redhat.com/archives/libvir-list/2014-October/msg00320.html
src/libvirt_private.syms | 1 + src/util/virpidfile.c | 42 ++++++++++++++++++++++++++++++++++++++++++ src/util/virpidfile.h | 2 ++ 3 files changed, 45 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index d6265ac..30d100d 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1810,6 +1810,7 @@ virPidFileBuildPath; virPidFileConstructPath; virPidFileDelete; virPidFileDeletePath; +virPidFileForceCleanupPath; virPidFileRead; virPidFileReadIfAlive; virPidFileReadPath; diff --git a/src/util/virpidfile.c b/src/util/virpidfile.c index a3b8846..a64a1cf 100644 --- a/src/util/virpidfile.c +++ b/src/util/virpidfile.c @@ -37,6 +37,7 @@ #include "c-ctype.h" #include "areadlink.h" #include "virstring.h" +#include "virprocess.h"
#define VIR_FROM_THIS VIR_FROM_NONE
@@ -567,3 +568,44 @@ virPidFileConstructPath(bool privileged, VIR_FREE(rundir); return ret; } + + +/** + * virPidFileForceCleanupPath: + * + * Check if the pidfile is left around and clean it up whatever it + * takes. This doesn't raise an error. This function must not be + * called multiple times with the same path, be it in threads or + * processes. + * + * Returns 0 if the pidfile was successfully cleaned up, -1 otherwise. + */ +int +virPidFileForceCleanupPath(const char *path) +{ + pid_t pid = 0; + int fd = -1; + + if (!virFileExists(path)) + return 0; + + if (virPidFileReadPath(path, &pid) < 0) + return -1; + + if (virPidFileAcquirePath(path, false, 0) == 0) { + virPidFileReleasePath(path, fd); + } else { + virResetLastError(); + + /* Only kill the process if the pid is valid one. 0 means + * there is somebody else doing the same pidfile cleanup + * machinery. */ + if (pid) + virProcessKillPainfully(pid, true); + + if (virPidFileDeletePath(path) < 0) + return -1; + } + + return 0; +} diff --git a/src/util/virpidfile.h b/src/util/virpidfile.h index ca1dbff..eb6516c 100644 --- a/src/util/virpidfile.h +++ b/src/util/virpidfile.h @@ -74,4 +74,6 @@ int virPidFileConstructPath(bool privileged, const char *progname, char **pidfile);
+int virPidFileForceCleanupPath(const char *path) ATTRIBUTE_NONNULL(1); + #endif /* __VIR_PIDFILE_H__ */ -- 2.1.2
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 10/12/14 14:12, Martin Kletzander wrote:
This function is used to cleanup a pidfile doing whatever it takes, even killing the owning process.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- v2: - Don't use "/proc", but simply just try to acquire the pidfile. - https://www.redhat.com/archives/libvir-list/2014-October/msg00320.html
src/libvirt_private.syms | 1 + src/util/virpidfile.c | 42 ++++++++++++++++++++++++++++++++++++++++++ src/util/virpidfile.h | 2 ++ 3 files changed, 45 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index d6265ac..30d100d 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1810,6 +1810,7 @@ virPidFileBuildPath; virPidFileConstructPath; virPidFileDelete; virPidFileDeletePath; +virPidFileForceCleanupPath; virPidFileRead; virPidFileReadIfAlive; virPidFileReadPath; diff --git a/src/util/virpidfile.c b/src/util/virpidfile.c index a3b8846..a64a1cf 100644 --- a/src/util/virpidfile.c +++ b/src/util/virpidfile.c @@ -37,6 +37,7 @@ #include "c-ctype.h" #include "areadlink.h" #include "virstring.h" +#include "virprocess.h"
#define VIR_FROM_THIS VIR_FROM_NONE
@@ -567,3 +568,44 @@ virPidFileConstructPath(bool privileged, VIR_FREE(rundir); return ret; } + + +/** + * virPidFileForceCleanupPath: + * + * Check if the pidfile is left around and clean it up whatever it + * takes. This doesn't raise an error. This function must not be + * called multiple times with the same path, be it in threads or + * processes. + * + * Returns 0 if the pidfile was successfully cleaned up, -1 otherwise.
Possibly worth mentioning that this function doesn't set libvirt errors.
+ */ +int +virPidFileForceCleanupPath(const char *path)
ACK, Peter
participants (2)
-
Martin Kletzander -
Peter Krempa