From: Timo Sirainen <timo.sirainen@dovecot.fi>
Date: Tue, 12 Sep 2017 11:54:57 +0000 (+0300)
Subject: lib: file_lock_set_unlink_on_free() - Avoid unlink() if another process is waiting... 
X-Git-Tag: 2.3.0.rc1~1040
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=3581ece16eb740a5003c4c11dd3c7d02839a24a4;p=thirdparty%2Fdovecot%2Fcore.git

lib: file_lock_set_unlink_on_free() - Avoid unlink() if another process is waiting on the lock
---

diff --git a/src/lib/file-create-locked.h b/src/lib/file-create-locked.h
index cba4476707..7d21040347 100644
--- a/src/lib/file-create-locked.h
+++ b/src/lib/file-create-locked.h
@@ -33,7 +33,12 @@ struct file_create_settings {
    If link() fails, opening is retried again. Returns fd on success,
    -1 on error. errno is preserved for the last failed syscall, so most
    importantly ENOENT could mean that the directory doesn't exist and EAGAIN
-   means locking timed out. */
+   means locking timed out.
+
+   If this function is used to create lock files, file_lock_set_unlink_on_free()
+   should be used for the resulting lock. It attempts to avoid unlinking the
+   file if there are already other processes using the lock. That can help to
+   avoid "Creating a locked file ... keeps failing" errors */
 int file_create_locked(const char *path, const struct file_create_settings *set,
 		       struct file_lock **lock_r, bool *created_r,
 		       const char **error_r);
diff --git a/src/lib/file-lock.c b/src/lib/file-lock.c
index 445caf8c7f..e86153ef33 100644
--- a/src/lib/file-lock.c
+++ b/src/lib/file-lock.c
@@ -352,10 +352,20 @@ void file_lock_set_close_on_free(struct file_lock *lock, bool set)
 	lock->close_on_free = set;
 }
 
+static void file_unlock_real(struct file_lock *lock)
+{
+	const char *error;
+
+	if (file_lock_do(lock->fd, lock->path, F_UNLCK,
+			 lock->lock_method, 0, &error) == 0) {
+		/* this shouldn't happen */
+		i_error("file_unlock(%s) failed: %m", lock->path);
+	}
+}
+
 void file_unlock(struct file_lock **_lock)
 {
 	struct file_lock *lock = *_lock;
-	const char *error;
 
 	*_lock = NULL;
 
@@ -364,15 +374,42 @@ void file_unlock(struct file_lock **_lock)
 	   could be deleting the new lock. */
 	i_assert(!lock->unlink_on_free);
 
-	if (file_lock_do(lock->fd, lock->path, F_UNLCK,
-			 lock->lock_method, 0, &error) == 0) {
-		/* this shouldn't happen */
-		i_error("file_unlock(%s) failed: %m", lock->path);
-	}
-
+	file_unlock_real(lock);
 	file_lock_free(&lock);
 }
 
+static void file_try_unlink_locked(struct file_lock *lock)
+{
+	struct file_lock *temp_lock = NULL;
+	struct stat st1, st2;
+	const char *error;
+	int ret;
+
+	file_unlock_real(lock);
+	ret = file_try_lock_error(lock->fd, lock->path, F_WRLCK,
+				  lock->lock_method, &temp_lock, &error);
+	if (ret < 0) {
+		i_error("file_lock_free(): Unexpectedly failed to retry locking %s: %s",
+			lock->path, error);
+	} else if (ret == 0) {
+		/* already locked by someone else */
+	} else if (fstat(lock->fd, &st1) < 0) {
+		/* not expected to happen */
+		i_error("file_lock_free(): fstat(%s) failed: %m", lock->path);
+	} else if (stat(lock->path, &st2) < 0) {
+		if (errno != ENOENT)
+			i_error("file_lock_free(): stat(%s) failed: %m", lock->path);
+	} else if (st1.st_ino != st2.st_ino ||
+		   !CMP_DEV_T(st1.st_dev, st2.st_dev)) {
+		/* lock file was recreated already - don't delete it */
+	} else {
+		/* nobody was waiting on the lock - unlink it */
+		i_unlink(lock->path);
+	}
+	if (temp_lock != NULL)
+		file_lock_free(&temp_lock);
+}
+
 void file_lock_free(struct file_lock **_lock)
 {
 	struct file_lock *lock = *_lock;
@@ -380,7 +417,7 @@ void file_lock_free(struct file_lock **_lock)
 	*_lock = NULL;
 
 	if (lock->unlink_on_free)
-		i_unlink(lock->path);
+		file_try_unlink_locked(lock);
 	if (lock->close_on_free)
 		i_close_fd(&lock->fd);
 
diff --git a/src/lib/file-lock.h b/src/lib/file-lock.h
index 022210600a..e4d6eb19fe 100644
--- a/src/lib/file-lock.h
+++ b/src/lib/file-lock.h
@@ -45,8 +45,9 @@ int file_wait_lock_error(int fd, const char *path, int lock_type,
 /* Change the lock type. WARNING: This isn't an atomic operation!
    The result is the same as file_unlock() + file_try_lock(). */
 int file_lock_try_update(struct file_lock *lock, int lock_type);
-/* When the lock is freed, unlink() the file automatically. This can be useful
-   for files that are only created to exist as lock files. */
+/* When the lock is freed, unlink() the file automatically, unless other
+   processes are already waiting on the lock. This can be useful for files that
+   are only created to exist as lock files. */
 void file_lock_set_unlink_on_free(struct file_lock *lock, bool set);
 /* When the lock is freed, close the fd automatically. This can
    be useful for files that are only created to exist as lock files. */