lockfile: Remove lockfile_lock timeout argument

lockfile_lock() accepts a timeout argument but, aside from unit tests
pertaining to timeout, its value is always 0. Since this feature relies on
a periodic SIGALRM signal, which is not a given if we're not caching time,
the cleanest solution is just to remove it.

Signed-off-by: Leo Alterman <lalterman@nicira.com>

diff --git a/lib/lockfile.c b/lib/lockfile.c
index 7ac34659b36f0637f9fd83d2c2a84c33d30e5197..db84aebb40cec38dd0bc590b22109104a0556eac 100644 (file)
--- a/lib/lockfile.c
+++ b/lib/lockfile.c
@@ -55,8 +55,7 @@ struct lockfile {
 static struct hmap lock_table = HMAP_INITIALIZER(&lock_table);
 
 static void lockfile_unhash(struct lockfile *);
-static int lockfile_try_lock(const char *name, bool block,
-                             struct lockfile **lockfilep);
+static int lockfile_try_lock(const char *name, struct lockfile **lockfilep);
 
 /* Returns the name of the lockfile that would be created for locking a file
  * named 'filename_'.  The caller is responsible for freeing the returned name,
@@ -87,56 +86,33 @@ lockfile_name(const char *filename_)
 /* Locks the configuration file against modification by other processes and
  * re-reads it from disk.
  *
- * The 'timeout' specifies the maximum number of milliseconds to wait for the
- * config file to become free.  Use 0 to avoid waiting or INT_MAX to wait
- * forever.
- *
  * Returns 0 on success, otherwise a positive errno value.  On success,
  * '*lockfilep' is set to point to a new "struct lockfile *" that may be
  * unlocked with lockfile_unlock().  On failure, '*lockfilep' is set to
- * NULL. */
+ * NULL.  Will not block if the lock cannot be immediately acquired. */
 int
-lockfile_lock(const char *file, int timeout, struct lockfile **lockfilep)
+lockfile_lock(const char *file, struct lockfile **lockfilep)
 {
     /* Only exclusive ("write") locks are supported.  This is not a problem
      * because the Open vSwitch code that currently uses lock files does so in
      * stylized ways such that any number of readers may access a file while it
      * is being written. */
-    long long int warn_elapsed = 1000;
-    long long int start, elapsed;
     char *lock_name;
     int error;
 
     COVERAGE_INC(lockfile_lock);
 
     lock_name = lockfile_name(file);
-    time_refresh();
-    start = time_msec();
-
-    do {
-        error = lockfile_try_lock(lock_name, timeout > 0, lockfilep);
-        time_refresh();
-        elapsed = time_msec() - start;
-        if (elapsed > warn_elapsed) {
-            warn_elapsed *= 2;
-            VLOG_WARN("%s: waiting for lock file, %lld ms elapsed",
-                      lock_name, elapsed);
-        }
-    } while (error == EINTR && (timeout == INT_MAX || elapsed < timeout));
-
-    if (error == EINTR) {
-        COVERAGE_INC(lockfile_timeout);
-        VLOG_WARN("%s: giving up on lock file after %lld ms",
-                  lock_name, elapsed);
-        error = ETIMEDOUT;
-    } else if (error) {
+
+    error = lockfile_try_lock(lock_name, lockfilep);
+
+    if (error) {
         COVERAGE_INC(lockfile_error);
         if (error == EACCES) {
             error = EAGAIN;
         }
-        VLOG_WARN("%s: failed to lock file "
-                  "(after %lld ms, with %d-ms timeout): %s",
-                  lock_name, elapsed, timeout, strerror(error));
+        VLOG_WARN("%s: failed to lock file: %s",
+                  lock_name, strerror(error));
     }
 
     free(lock_name);
@@ -225,7 +201,7 @@ lockfile_register(const char *name, dev_t device, ino_t inode, int fd)
 }
 
 static int
-lockfile_try_lock(const char *name, bool block, struct lockfile **lockfilep)
+lockfile_try_lock(const char *name, struct lockfile **lockfilep)
 {
     struct flock l;
     struct stat s;
@@ -268,7 +244,7 @@ lockfile_try_lock(const char *name, bool block, struct lockfile **lockfilep)
     l.l_len = 0;
 
     time_disable_restart();
-    error = fcntl(fd, block ? F_SETLKW : F_SETLK, &l) == -1 ? errno : 0;
+    error = fcntl(fd, F_SETLK, &l) == -1 ? errno : 0;
     time_enable_restart();
 
     if (!error) {

diff --git a/lib/lockfile.h b/lib/lockfile.h
index 202134b41c318c5adbacdc42c65ce64830644557..3e6b54c38d203c8fbecb0699195900033b7e21bc 100644 (file)
--- a/lib/lockfile.h
+++ b/lib/lockfile.h
@@ -19,7 +19,7 @@
 struct lockfile;
 
 char *lockfile_name(const char *file);
-int lockfile_lock(const char *file, int timeout, struct lockfile **);
+int lockfile_lock(const char *file, struct lockfile **);
 void lockfile_unlock(struct lockfile *);
 void lockfile_postfork(void);
 

diff --git a/ovsdb/file.c b/ovsdb/file.c
index 9e2dd5079923e996db8e12637f4ed50e8b5a343a..43fcb95643e93f387e69a2fb21887099a2a97f5e 100644 (file)
--- a/ovsdb/file.c
+++ b/ovsdb/file.c
@@ -649,7 +649,7 @@ ovsdb_file_compact(struct ovsdb_file *file)
 
     /* Lock temporary file. */
     tmp_name = xasprintf("%s.tmp", file->file_name);
-    retval = lockfile_lock(tmp_name, 0, &tmp_lock);
+    retval = lockfile_lock(tmp_name, &tmp_lock);
     if (retval) {
         error = ovsdb_io_error(retval, "could not get lock on %s", tmp_name);
         goto exit;

diff --git a/ovsdb/log.c b/ovsdb/log.c
index 9b882dc4961e281862224fb78650139fad6f9f2c..b79535ac814b12b3073b6a66a7830888449cd394 100644 (file)
--- a/ovsdb/log.c
+++ b/ovsdb/log.c
@@ -80,7 +80,7 @@ ovsdb_log_open(const char *name, enum ovsdb_log_open_mode open_mode,
         locking = open_mode != OVSDB_LOG_READ_ONLY;
     }
     if (locking) {
-        int retval = lockfile_lock(name, 0, &lockfile);
+        int retval = lockfile_lock(name, &lockfile);
         if (retval) {
             error = ovsdb_io_error(retval, "%s: failed to lock lockfile",
                                    name);

diff --git a/ovsdb/ovsdb-tool.c b/ovsdb/ovsdb-tool.c
index f31bdd1064005fdc72f7f900ad3bcafbd9a8b2e5..6b75f4971860aabba66a506c22ff4c5f05dc5616 100644 (file)
--- a/ovsdb/ovsdb-tool.c
+++ b/ovsdb/ovsdb-tool.c
@@ -229,14 +229,14 @@ compact_or_convert(const char *src_name_, const char *dst_name_,
 
     /* Lock the source, if we will be replacing it. */
     if (in_place) {
-        retval = lockfile_lock(src_name, 0, &src_lock);
+        retval = lockfile_lock(src_name, &src_lock);
         if (retval) {
             ovs_fatal(retval, "%s: failed to lock lockfile", src_name);
         }
     }
 
     /* Get (temporary) destination and lock it. */
-    retval = lockfile_lock(dst_name, 0, &dst_lock);
+    retval = lockfile_lock(dst_name, &dst_lock);
     if (retval) {
         ovs_fatal(retval, "%s: failed to lock lockfile", dst_name);
     }

diff --git a/tests/lockfile.at b/tests/lockfile.at
index 602cfab447947b04af2f9fde47d79884e8e5cfde..877cc872fc257512179ae683397d09e8e4baa1a9 100644 (file)
--- a/tests/lockfile.at
+++ b/tests/lockfile.at
@@ -15,8 +15,6 @@ CHECK_LOCKFILE([lock_blocks_same_process_twice], [0])
 CHECK_LOCKFILE([lock_blocks_other_process], [1])
 CHECK_LOCKFILE([lock_twice_blocks_other_process], [1])
 CHECK_LOCKFILE([lock_and_unlock_allows_other_process], [1])
-CHECK_LOCKFILE([lock_timeout_gets_the_lock], [1])
-CHECK_LOCKFILE([lock_timeout_runs_out], [1])
 CHECK_LOCKFILE([lock_multiple], [0])
 CHECK_LOCKFILE([lock_symlink], [0])
 CHECK_LOCKFILE([lock_symlink_to_dir], [0])

diff --git a/tests/test-lockfile.c b/tests/test-lockfile.c
index 412010b30690eea1c11c43d21412629b838826b0..b37fd225badc443b8882c75475f724c494737945 100644 (file)
--- a/tests/test-lockfile.c
+++ b/tests/test-lockfile.c
@@ -54,7 +54,7 @@ run_lock_and_unlock(void)
 {
     struct lockfile *lockfile;
 
-    CHECK(lockfile_lock("file", 0, &lockfile), 0);
+    CHECK(lockfile_lock("file", &lockfile), 0);
     lockfile_unlock(lockfile);
 }
 
@@ -63,10 +63,10 @@ run_lock_and_unlock_twice(void)
 {
     struct lockfile *lockfile;
 
-    CHECK(lockfile_lock("file", 0, &lockfile), 0);
+    CHECK(lockfile_lock("file", &lockfile), 0);
     lockfile_unlock(lockfile);
 
-    CHECK(lockfile_lock("file", 0, &lockfile), 0);
+    CHECK(lockfile_lock("file", &lockfile), 0);
     lockfile_unlock(lockfile);
 }
 
@@ -75,8 +75,8 @@ run_lock_blocks_same_process(void)
 {
     struct lockfile *lockfile;
 
-    CHECK(lockfile_lock("file", 0, &lockfile), 0);
-    CHECK(lockfile_lock("file", 0, &lockfile), EDEADLK);
+    CHECK(lockfile_lock("file", &lockfile), 0);
+    CHECK(lockfile_lock("file", &lockfile), EDEADLK);
     lockfile_unlock(lockfile);
 }
 
@@ -85,9 +85,9 @@ run_lock_blocks_same_process_twice(void)
 {
     struct lockfile *lockfile;
 
-    CHECK(lockfile_lock("file", 0, &lockfile), 0);
-    CHECK(lockfile_lock("file", 0, &lockfile), EDEADLK);
-    CHECK(lockfile_lock("file", 0, &lockfile), EDEADLK);
+    CHECK(lockfile_lock("file", &lockfile), 0);
+    CHECK(lockfile_lock("file", &lockfile), EDEADLK);
+    CHECK(lockfile_lock("file", &lockfile), EDEADLK);
     lockfile_unlock(lockfile);
 }
 
@@ -118,10 +118,10 @@ run_lock_blocks_other_process(void)
      * this function that does the wait() call. */
     static struct lockfile *lockfile;
 
-    CHECK(lockfile_lock("file", 0, &lockfile), 0);
+    CHECK(lockfile_lock("file", &lockfile), 0);
     if (do_fork() == CHILD) {
         lockfile_unlock(lockfile);
-        CHECK(lockfile_lock("file", 0, &lockfile), EAGAIN);
+        CHECK(lockfile_lock("file", &lockfile), EAGAIN);
         exit(11);
     }
 }
@@ -131,10 +131,10 @@ run_lock_twice_blocks_other_process(void)
 {
     struct lockfile *lockfile, *dummy;
 
-    CHECK(lockfile_lock("file", 0, &lockfile), 0);
-    CHECK(lockfile_lock("file", 0, &dummy), EDEADLK);
+    CHECK(lockfile_lock("file", &lockfile), 0);
+    CHECK(lockfile_lock("file", &dummy), EDEADLK);
     if (do_fork() == CHILD) {
-        CHECK(lockfile_lock("file", 0, &dummy), EAGAIN);
+        CHECK(lockfile_lock("file", &dummy), EAGAIN);
         exit(11);
     }
 }
@@ -144,72 +144,31 @@ run_lock_and_unlock_allows_other_process(void)
 {
     struct lockfile *lockfile;
 
-    CHECK(lockfile_lock("file", 0, &lockfile), 0);
+    CHECK(lockfile_lock("file", &lockfile), 0);
     lockfile_unlock(lockfile);
 
     if (do_fork() == CHILD) {
-        CHECK(lockfile_lock("file", 0, &lockfile), 0);
+        CHECK(lockfile_lock("file", &lockfile), 0);
         exit(11);
     }
 }
 
-static void
-run_lock_timeout_gets_the_lock(void)
-{
-    struct lockfile *lockfile;
-
-    CHECK(lockfile_lock("file", 0, &lockfile), 0);
-
-    if (do_fork() == CHILD) {
-        lockfile_unlock(lockfile);
-        CHECK(lockfile_lock("file", TIME_UPDATE_INTERVAL * 3, &lockfile), 0);
-        exit(11);
-    } else {
-        long long int now = time_msec();
-        while (time_msec() < now + TIME_UPDATE_INTERVAL) {
-            pause();
-        }
-        lockfile_unlock(lockfile);
-    }
-}
-
-static void
-run_lock_timeout_runs_out(void)
-{
-    struct lockfile *lockfile;
-
-    CHECK(lockfile_lock("file", 0, &lockfile), 0);
-
-    if (do_fork() == CHILD) {
-        lockfile_unlock(lockfile);
-        CHECK(lockfile_lock("file", TIME_UPDATE_INTERVAL, &lockfile),
-              ETIMEDOUT);
-        exit(11);
-    } else {
-        long long int now = time_msec();
-        while (time_msec() < now + TIME_UPDATE_INTERVAL * 3) {
-            pause();
-        }
-        lockfile_unlock(lockfile);
-    }
-}
-
 static void
 run_lock_multiple(void)
 {
     struct lockfile *a, *b, *c, *dummy;
 
-    CHECK(lockfile_lock("a", 0, &a), 0);
-    CHECK(lockfile_lock("b", 0, &b), 0);
-    CHECK(lockfile_lock("c", 0, &c), 0);
+    CHECK(lockfile_lock("a", &a), 0);
+    CHECK(lockfile_lock("b", &b), 0);
+    CHECK(lockfile_lock("c", &c), 0);
 
     lockfile_unlock(a);
-    CHECK(lockfile_lock("a", 0, &a), 0);
-    CHECK(lockfile_lock("a", 0, &dummy), EDEADLK);
+    CHECK(lockfile_lock("a", &a), 0);
+    CHECK(lockfile_lock("a", &dummy), EDEADLK);
     lockfile_unlock(a);
 
     lockfile_unlock(b);
-    CHECK(lockfile_lock("a", 0, &a), 0);
+    CHECK(lockfile_lock("a", &a), 0);
 
     lockfile_unlock(c);
     lockfile_unlock(a);
@@ -231,14 +190,14 @@ run_lock_symlink(void)
     CHECK(stat(".b.~lock~", &s), -1);
     CHECK(errno, ENOENT);
 
-    CHECK(lockfile_lock("a", 0, &a), 0);
-    CHECK(lockfile_lock("a", 0, &dummy), EDEADLK);
-    CHECK(lockfile_lock("b", 0, &dummy), EDEADLK);
+    CHECK(lockfile_lock("a", &a), 0);
+    CHECK(lockfile_lock("a", &dummy), EDEADLK);
+    CHECK(lockfile_lock("b", &dummy), EDEADLK);
     lockfile_unlock(a);
 
-    CHECK(lockfile_lock("b", 0, &b), 0);
-    CHECK(lockfile_lock("b", 0, &dummy), EDEADLK);
-    CHECK(lockfile_lock("a", 0, &dummy), EDEADLK);
+    CHECK(lockfile_lock("b", &b), 0);
+    CHECK(lockfile_lock("b", &dummy), EDEADLK);
+    CHECK(lockfile_lock("a", &dummy), EDEADLK);
     lockfile_unlock(b);
 
     CHECK(lstat(".a.~lock~", &s), 0);
@@ -268,12 +227,12 @@ run_lock_symlink_to_dir(void)
     CHECK(S_ISLNK(s.st_mode) != 0, 1);
 
     /* Lock 'a'. */
-    CHECK(lockfile_lock("a", 0, &a), 0);
+    CHECK(lockfile_lock("a", &a), 0);
     CHECK(lstat("dir/.b.~lock~", &s), 0);
     CHECK(S_ISREG(s.st_mode) != 0, 1);
     CHECK(lstat(".a.~lock~", &s), -1);
     CHECK(errno, ENOENT);
-    CHECK(lockfile_lock("dir/b", 0, &dummy), EDEADLK);
+    CHECK(lockfile_lock("dir/b", &dummy), EDEADLK);
 
     lockfile_unlock(a);
 }
@@ -300,8 +259,6 @@ static const struct test tests[] = {
     TEST(lock_blocks_other_process),
     TEST(lock_twice_blocks_other_process),
     TEST(lock_and_unlock_allows_other_process),
-    TEST(lock_timeout_gets_the_lock),
-    TEST(lock_timeout_runs_out),
     TEST(lock_multiple),
     TEST(lock_symlink),
     TEST(lock_symlink_to_dir),