From: Scott Worley <scottworley@scottworley.com>
Date: Wed, 13 Sep 2023 21:56:30 +0000 (-0700)
Subject: Also take flock locks
X-Git-Tag: v1.0.0~3
X-Git-Url: http://git.scottworley.com/tl-append/commitdiff_plain/d212cd99e7ef72e0add4ef772e328835a972ae6b

Also take flock locks

There are two kinds of locks, so for interoperability, everyone has to
always take both kinds, to avoid the possibility of one client using one
kind and another client using another kind & missing each other.  :(

I wonder what order we're supposed to take them in?  If we don't all
agree, we get deadlocks!  Here, I take them in alphabetical order
(fcntl then flock), in the hope of this being a legible Schelling point.

The test harness does exercise the "take locks in the 'wrong' order" case.
It probably deadlocks with some tiny probability on heavily-loaded
systems.  Sorry.  If this hurts you, feel free to remove it.
---

diff --git a/tl-append-test.c b/tl-append-test.c
index 04c679e..e712c6f 100644
--- a/tl-append-test.c
+++ b/tl-append-test.c
@@ -255,7 +255,7 @@ static void write_and_read_two_lines() {
 static void write_to_locked_log(char *lock_types[]) {
   remove_logfile();
   ex_t e1 = write_to_tl_append("begin\n");
-  FILE *f = fopen(FILENAME, "a");
+  FILE *f = fopen(FILENAME, "ae");
   if (f == NULL)
     die_err("Couldn't open file for locking");
   for (int i = 0; lock_types[i]; i++)
@@ -313,7 +313,7 @@ int main() {
   write_to_locked_log((char *[]){NULL});
   write_to_locked_log((char *[]){"fcntl", NULL});
   write_to_locked_log((char *[]){"flock", NULL});
-  write_to_locked_log((char *[]){"flock", "fcntl", NULL});
+  write_to_locked_log((char *[]){"flock", "fcntl", NULL}); /* Deadlock risk! */
   write_to_locked_log((char *[]){"fcntl", "flock", NULL});
   write_concurrently();
 }
diff --git a/tl-append.c b/tl-append.c
index 1b3a9d0..4edd58a 100644
--- a/tl-append.c
+++ b/tl-append.c
@@ -5,6 +5,7 @@
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
+#include <sys/file.h>
 #include <time.h>
 #include <unistd.h>
 
@@ -22,18 +23,22 @@ const struct timespec ACKNOWLEDGE_DELAY = {0, 300000000};
 typedef struct {
   int interactive;
   int fcntl_lock;
+  int flock_lock;
 } conf_t;
 
 conf_t parse_command_line(int argc, char *argv[]) {
   conf_t conf;
   conf.interactive = 0;
   conf.fcntl_lock = 1;
+  conf.flock_lock = 1;
 
   for (int i = 1; i < argc; i++) {
     if (strcmp(argv[i], "-i") == 0 && isatty(2))
       conf.interactive = 1;
     if (strcmp(argv[i], "--no-fnctl-lock") == 0)
       conf.fcntl_lock = 0;
+    if (strcmp(argv[i], "--no-flock-lock") == 0)
+      conf.flock_lock = 0;
   }
 
   return conf;
@@ -65,7 +70,7 @@ static void write_line(const char *now, FILE *f, const char *line) {
     die("Error writing to output file");
 }
 
-static void take_lock(conf_t *conf, FILE *f) {
+static void take_fcntl_lock(conf_t *conf, FILE *f) {
   if (!conf->fcntl_lock)
     return;
   struct flock lock;
@@ -79,12 +84,34 @@ static void take_lock(conf_t *conf, FILE *f) {
   if (fcntl(fd, F_SETLK, &lock) == 0)
     return;
   if (errno != EACCES && errno != EAGAIN)
-    die_err("Couldn't take lock");
+    die_err("Couldn't take fcntl lock");
   if (fputs(WAITING, stderr) == EOF)
     die("Error writing waiting message");
   if (fcntl(fd, F_SETLKW, &lock) == 0)
     return;
-  die_err("Couldn't take lock");
+  die_err("Couldn't take fcntl lock");
+}
+
+static void take_flock_lock(conf_t *conf, FILE *f) {
+  if (!conf->flock_lock)
+    return;
+  int fd = fileno(f);
+  if (fd == -1)
+    die_err("Couldn't get file descriptor for locking");
+  if (flock(fd, LOCK_EX | LOCK_NB) == 0)
+    return;
+  if (errno != EWOULDBLOCK)
+    die_err("Couldn't take flock lock");
+  if (fputs(WAITING, stderr) == EOF)
+    die("Error writing waiting message");
+  if (flock(fd, LOCK_EX) == 0)
+    return;
+  die_err("Couldn't take flock lock");
+}
+
+static void take_lock(conf_t *conf, FILE *f) {
+  take_fcntl_lock(conf, f);
+  take_flock_lock(conf, f);
 }
 
 static void write_acknowledgment(conf_t *conf) {