[libvirt] [PATCH 05/14] Rewrite pidfile handling to be crash safe

[Daniel P. Berrange]

From: "Daniel P. Berrange" <berrange at redhat.com>

If libvirtd crashes then the pidfile may not be cleaned up,
making a later restart fail, even though the original process
no longer exists.

Instead of simply using file creation as the test for successful
pidfile acquisition, actually take out a lock on the pidfile.

To avoid races, after locking the pidfile, verify that the
inode hasn't changed.

Also make sure the unprivileged libvirtd now acquires the
pidfile, instead of relying on the UNIX socket to ensure
mutual exclusion

* daemon/libvirtd.c: Take a fcntl() based lock on the pidfile
---
 daemon/libvirtd.c |   93 +++++++++++++++++++++++++++++++++-------------------
 1 files changed, 59 insertions(+), 34 deletions(-)

diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c
index 06d2077..bb388d4 100644
--- a/daemon/libvirtd.c
+++ b/daemon/libvirtd.c
@@ -53,6 +53,7 @@
 #include "hooks.h"
 #include "uuid.h"
 #include "virtaudit.h"
+#include "intprops.h"
 
 #ifdef WITH_DRIVER_MODULES
 # include "driver.h"
@@ -258,42 +259,65 @@ static int daemonForkIntoBackground(const char *argv0)
     }
 }
 
-static int daemonWritePidFile(const char *pidFile, const char *argv0)
-{
+static int
+daemonAcquirePidFile(const char *argv0, const char *pidFile) {
     int fd;
-    FILE *fh;
     char ebuf[1024];
+    unsigned long long pid = (unsigned long long)getpid();
+    char pidstr[INT_BUFSIZE_BOUND(pid)];
 
     if (pidFile[0] == '\0')
         return 0;
 
-    if ((fd = open(pidFile, O_WRONLY|O_CREAT|O_EXCL, 0644)) < 0) {
-        VIR_ERROR(_("Failed to open pid file '%s' : %s"),
-                  pidFile, virStrerror(errno, ebuf, sizeof ebuf));
-        return -1;
-    }
+    while (1) {
+        struct stat a, b;
+        if ((fd = open(pidFile, O_WRONLY|O_CREAT, 0644)) < 0) {
+            VIR_ERROR(_("%s: Failed to open pid file '%s' : %s"),
+                      argv0, pidFile, virStrerror(errno, ebuf, sizeof ebuf));
+            return -1;
+        }
+
+        if (fstat(fd, &b) < 0) {
+            VIR_ERROR(_("%s: Pid file '%s' disappeared: %s"),
+                      argv0, pidFile, virStrerror(errno, ebuf, sizeof ebuf));
+            VIR_FORCE_CLOSE(fd);
+            return -1;
+        }
+
+        if (virFileLock(fd, false, 0, 1) < 0) {
+            VIR_ERROR(_("%s: Failed to acquire pid file '%s' : %s"),
+                      argv0, pidFile, virStrerror(errno, ebuf, sizeof ebuf));
+            VIR_FORCE_CLOSE(fd);
+            return -1;
+        }
+
+        /* Now make sure the pidfile we locked is the same
+         * one that now exists on the filesystem
+         */
+
+        if (stat(pidFile, &a) < 0) {
+            VIR_DEBUG("%s: Pid file '%s' disappeared: %s",
+                      argv0, pidFile, virStrerror(errno, ebuf, sizeof ebuf));
+            VIR_FORCE_CLOSE(fd);
+            continue;
+        }
 
-    if (!(fh = VIR_FDOPEN(fd, "w"))) {
-        VIR_ERROR(_("Failed to fdopen pid file '%s' : %s"),
-                  pidFile, virStrerror(errno, ebuf, sizeof ebuf));
+        if (a.st_ino == b.st_ino)
+            break;
+
+        VIR_DEBUG("%s: Pid file '%s' was recreated",
+                  argv0, pidFile);
         VIR_FORCE_CLOSE(fd);
-        return -1;
     }
 
-    if (fprintf(fh, "%lu\n", (unsigned long)getpid()) < 0) {
-        VIR_ERROR(_("%s: Failed to write to pid file '%s' : %s"),
-                  argv0, pidFile, virStrerror(errno, ebuf, sizeof ebuf));
-        VIR_FORCE_FCLOSE(fh);
-        return -1;
-    }
+    snprintf(pidstr, sizeof(pidstr), "%llu", pid);
 
-    if (VIR_FCLOSE(fh) == EOF) {
-        VIR_ERROR(_("%s: Failed to close pid file '%s' : %s"),
-                  argv0, pidFile, virStrerror(errno, ebuf, sizeof ebuf));
-        return -1;
+    if (safewrite(fd, pidstr, strlen(pidstr)) < 0) {
+            VIR_ERROR(_("Failed to write to pid file '%s' : %s"),
+                      pidFile, virStrerror(errno, ebuf, sizeof ebuf));
     }
 
-    return 0;
+    return fd;
 }
 
 
@@ -1269,6 +1293,7 @@ int main(int argc, char **argv) {
     char *remote_config_file = NULL;
     int statuswrite = -1;
     int ret = 1;
+    int pid_file_fd = -1;
     char *pid_file = NULL;
     char *sock_file = NULL;
     char *sock_file_ro = NULL;
@@ -1382,7 +1407,7 @@ int main(int argc, char **argv) {
     if (daemonSetupLogging(config, privileged, verbose, godaemon) < 0)
         exit(EXIT_FAILURE);
 
-    if (!pid_file && privileged &&
+    if (!pid_file &&
         daemonPidFilePath(privileged,
                           &pid_file) < 0)
         exit(EXIT_FAILURE);
@@ -1409,14 +1434,6 @@ int main(int argc, char **argv) {
         }
     }
 
-    /* If we have a pidfile set, claim it now, exiting if already taken */
-    if (pid_file != NULL &&
-        daemonWritePidFile(pid_file, argv[0]) < 0) {
-        VIR_FREE(pid_file); /* Prevent unlinking of someone else's pid ! */
-        ret = VIR_DAEMON_ERR_PIDFILE;
-        goto cleanup;
-    }
-
     /* Ensure the rundir exists (on tmpfs on some systems) */
     if (privileged) {
         const char *rundir = LOCALSTATEDIR "/run/libvirt";
@@ -1436,6 +1453,12 @@ int main(int argc, char **argv) {
         umask(old_umask);
     }
 
+    /* Try to claim the pidfile, existing if we can't */
+    if ((pid_file_fd = daemonAcquirePidFile(argv[0], pid_file)) < 0) {
+        ret = VIR_DAEMON_ERR_PIDFILE;
+        goto cleanup;
+    }
+
     if (!(srv = virNetServerNew(config->min_workers,
                                 config->max_workers,
                                 config->max_clients,
@@ -1570,8 +1593,10 @@ cleanup:
         }
         VIR_FORCE_CLOSE(statuswrite);
     }
-    if (pid_file)
-        unlink (pid_file);
+    if (pid_file_fd != -1) {
+        unlink(pid_file);
+        VIR_FORCE_CLOSE(pid_file_fd);
+    }
 
     VIR_FREE(sock_file);
     VIR_FREE(sock_file_ro);
-- 
1.7.6