[ovs-dev] [PATCH] daemon: Avoid the link() syscall.

[Ethan Jackson]

make_pidfile() depends on the link() system call to atomically
create pidfiles when multiple daemons are started concurrently.
However, this system call isn't available on ESX so an alternative
strategy is necessary.  Fortunately, the approach this patch takes
is cleaner than the original code.

Signed-off-by: Ethan Jackson <ethan at nicira.com>
---
 lib/daemon.c |   62 ++++++++++++++++++++++++++++------------------------------
 1 file changed, 30 insertions(+), 32 deletions(-)

diff --git a/lib/daemon.c b/lib/daemon.c
index 71f6f81..da09eed 100644
--- a/lib/daemon.c
+++ b/lib/daemon.c
@@ -175,13 +175,36 @@ make_pidfile(void)
     int error;
 
     /* Create a temporary pidfile. */
-    tmpfile = xasprintf("%s.tmp%ld", pidfile, pid);
-    fatal_signal_add_file_to_unlink(tmpfile);
+    if (overwrite_pidfile) {
+        tmpfile = xasprintf("%s.tmp%ld", pidfile, pid);
+        fatal_signal_add_file_to_unlink(tmpfile);
+    } else {
+        /* Everyone shares the same file which will be treated as a lock.  To
+         * avoid some uncomfortable race conditions, we can't set up the fatal
+         * signal unlink until we've acquired it. */
+        tmpfile = xasprintf("%s.tmp", pidfile);
+    }
+
     file = fopen(tmpfile, "w+");
     if (!file) {
         VLOG_FATAL("%s: create failed (%s)", tmpfile, strerror(errno));
     }
 
+    error = lock_pidfile(file, F_SETLK);
+    if (error) {
+        /* Looks like we failed to acquire the lock.  Note that, if we failed
+         * for some other reason (and '!overwrite_pidfile'), we will have
+         * left 'tmpfile' as garbage in the file system. */
+        VLOG_FATAL("%s: fcntl(F_SETLK) failed (%s)", tmpfile, strerror(error));
+    }
+
+    if (!overwrite_pidfile) {
+        /* We acquired the lock.  Make sure to clean up on exit, and verify
+         * that we're allowed to create the actual pidfile. */
+        fatal_signal_add_file_to_unlink(tmpfile);
+        check_already_running();
+    }
+
     if (fstat(fileno(file), &s) == -1) {
         VLOG_FATAL("%s: fstat failed (%s)", tmpfile, strerror(errno));
     }
@@ -191,39 +214,14 @@ make_pidfile(void)
         VLOG_FATAL("%s: write failed (%s)", tmpfile, strerror(errno));
     }
 
-    error = lock_pidfile(file, F_SETLK);
-    if (error) {
-        VLOG_FATAL("%s: fcntl(F_SETLK) failed (%s)", tmpfile, strerror(error));
-    }
-
-    /* Rename or link it to the correct name. */
-    if (overwrite_pidfile) {
-        if (rename(tmpfile, pidfile) < 0) {
-            VLOG_FATAL("failed to rename \"%s\" to \"%s\" (%s)",
-                       tmpfile, pidfile, strerror(errno));
-        }
-    } else {
-        do {
-            error = link(tmpfile, pidfile) == -1 ? errno : 0;
-            if (error == EEXIST) {
-                check_already_running();
-            }
-        } while (error == EINTR || error == EEXIST);
-        if (error) {
-            VLOG_FATAL("failed to link \"%s\" as \"%s\" (%s)",
-                       tmpfile, pidfile, strerror(error));
-        }
-    }
-
     /* Ensure that the pidfile will get deleted on exit. */
     fatal_signal_add_file_to_unlink(pidfile);
 
-    /* Delete the temporary pidfile if it still exists. */
-    if (!overwrite_pidfile) {
-        error = fatal_signal_unlink_file_now(tmpfile);
-        if (error) {
-            VLOG_FATAL("%s: unlink failed (%s)", tmpfile, strerror(error));
-        }
+    /* Note that since we locked 'tmpfile', we're the only process allowed to
+     * make this rename, and it's therefore atomic. */
+    if (rename(tmpfile, pidfile) < 0) {
+        VLOG_FATAL("failed to rename \"%s\" to \"%s\" (%s)",
+                   tmpfile, pidfile, strerror(errno));
     }
 
     /* Clean up.
-- 
1.7.9.5