fixup! [LibOS] Implement POSIX locks (fcntl)

Signed-off-by: Paweł Marczewski <pawel@invisiblethingslab.com>

LibOS/shim/include/shim_fs.h

@@ -154,12 +154,12 @@ struct shim_dentry {
     /* Filesystem-specific data. Protected by `lock`. */
     void* data;
 
-    /* File lock information, stored in the main process. See shim_fs_lock.c. */
+    /* File lock information, stored only in the main process. See shim_fs_lock.c. */
     struct fs_lock* fs_lock;
 
     /* True if the file might have locks placed by current process. Used in processes other than
      * main process, to prevent unnecessary IPC calls on handle close. See shim_fs_lock.c. */
-    bool maybe_has_locks;
+    bool maybe_has_fs_locks;
 
     struct shim_lock lock;
     REFTYPE ref_count;

LibOS/shim/include/shim_fs_lock.h

@@ -4,7 +4,7 @@
  */
 
 /*
- * File locks. Currently POSIX locks are implemented.
+ * File locks. Currently only POSIX locks are implemented.
  */
 
 #ifndef SHIM_FS_LOCK_H_
@@ -125,4 +125,4 @@ int posix_lock_set_from_ipc(const char* path, struct posix_lock* pl, bool wait,
  */
 int posix_lock_get_from_ipc(const char* path, struct posix_lock* pl, struct posix_lock* out_pl);
 
-#endif /* SHIM_FS_LOCK_H */
+#endif /* SHIM_FS_LOCK_H_ */

LibOS/shim/src/fs/shim_fs_lock.c

@@ -56,7 +56,7 @@ struct fs_lock {
 static LISTP_TYPE(fs_lock) g_fs_lock_list = LISTP_INIT;
 
 /* Global lock for all operations on filesystem locks, including access to dentry `fs_lock` and
- * `maybe_has_locks` fields. */
+ * `maybe_has_fs_locks` fields. */
 static struct shim_lock g_fs_lock_lock;
 
 int init_fs_lock(void) {
@@ -429,12 +429,12 @@ static int posix_lock_set_or_add_request(struct shim_dentry* dent, struct posix_
 int posix_lock_set(struct shim_dentry* dent, struct posix_lock* pl, bool wait) {
     int ret;
     if (g_process_ipc_ids.leader_vmid) {
-        /* In the IPC version, we use `dent->maybe_has_locks` to short-circuit unlocking files that
+        /* In the IPC version, we use `dent->maybe_has_fs_locks` to short-circuit unlocking files that
          * we never locked. This is to prevent unnecessary IPC calls on a handle. */
         lock(&g_fs_lock_lock);
         if (pl->type == F_RDLCK || pl->type == F_WRLCK) {
-            dent->maybe_has_locks = true;
-        } else if (!dent->maybe_has_locks) {
+            dent->maybe_has_fs_locks = true;
+        } else if (!dent->maybe_has_fs_locks) {
             /* We know we're not holding any locks for the file */
             unlock(&g_fs_lock_lock);
             return 0;

LibOS/shim/test/ltp/ltp.cfg

@@ -369,7 +369,7 @@ timeout = 60
 [fcntl14_64]
 timeout = 60
 
-# no deadlock detection
+# no deadlock detection for POSIX locks
 [fcntl17]
 skip = yes
 

LibOS/shim/test/regression/fcntl_lock.c

@@ -91,8 +91,13 @@ static int try_fcntl(int cmd, struct flock* fl) {
 
     if (ret != -1 && ret != 0)
         errx(1, "fcntl returned unexpected value");
-    if (ret == -1 && errno != EACCES && errno != EAGAIN)
-        err(1, "fcntl");
+    if (ret == -1) {
+        /* We permit -1 only for F_SETLK, and only with EACCES or EAGAIN errors (which means the
+         * lock could not be placed immediately). */
+        if (!(cmd == F_SETLK && (errno == EACCES || errno == EAGAIN))) {
+            err(1, "fcntl");
+        }
+    }
     return ret;
 }
 
@@ -108,6 +113,8 @@ static bool try_lock(int cmd, int type, int whence, long int start, long int len
     int ret = try_fcntl(cmd, &fl);
 
     if (cmd == F_GETLK) {
+        assert(ret == 0);
+
         return fl.l_type == F_UNLCK;
     } else {
         return ret == 0;
@@ -141,7 +148,7 @@ static void unlock(long int start, long int len) {
         errx(1, "unlock failed");
 }
 
-static void lock_ok(int type, long int start, long int len) {
+static void lock(int type, long int start, long int len) {
     assert(type == F_RDLCK || type == F_WRLCK);
 
     if (!try_lock(F_GETLK, type, SEEK_SET, start, len)
@@ -166,14 +173,14 @@ static void lock_fail(int type, long int start, long int len) {
  * at Graphene debug output).
  */
 static void test_ranges() {
-    printf("test ranges...\n");
+    printf("testing ranges...\n");
     unlock(0, 0);
 
     /* Lock some ranges, check joining adjacent ranges */
-    lock_ok(F_RDLCK, 10, 10);
-    lock_ok(F_RDLCK, 30, 10);
-    lock_ok(F_RDLCK, 20, 10);
-    lock_ok(F_RDLCK, 1000, 0);
+    lock(F_RDLCK, 10, 10);
+    lock(F_RDLCK, 30, 10);
+    lock(F_RDLCK, 20, 10);
+    lock(F_RDLCK, 1000, 0);
 
     /* Unlock some ranges, check subtracting and splitting ranges */
     unlock(5, 10);
@@ -182,8 +189,8 @@ static void test_ranges() {
     unlock(950, 100);
 
     /* Overwrite with write lock */
-    lock_ok(F_WRLCK, 0, 30);
-    lock_ok(F_WRLCK, 30, 30);
+    lock(F_WRLCK, 0, 30);
+    lock(F_WRLCK, 30, 30);
 }
 
 static void wait_for_child(void) {
@@ -235,7 +242,7 @@ static void read_pipe(int pipe[2]) {
 
 /* Test: child takes a lock and then exits. The lock should be released. */
 static void test_child_exit() {
-    printf("test child exit...\n");
+    printf("testing child exit...\n");
     unlock(0, 0);
 
     int pipes[2][2];
@@ -246,7 +253,7 @@ static void test_child_exit() {
         err(1, "fork");
 
     if (pid == 0) {
-        lock_ok(F_WRLCK, 0, 100);
+        lock(F_WRLCK, 0, 100);
         write_pipe(pipes[0]);
         read_pipe(pipes[1]);
         exit(0);
@@ -263,7 +270,7 @@ static void test_child_exit() {
 
 /* Test: child takes a lock, and then closes a duplicated FD. The lock should be released. */
 static void test_file_close() {
-    printf("test file close...\n");
+    printf("testing file close...\n");
     unlock(0, 0);
 
     int pipes[2][2];
@@ -274,7 +281,7 @@ static void test_file_close() {
         err(1, "fork");
 
     if (pid == 0) {
-        lock_ok(F_WRLCK, 0, 100);
+        lock(F_WRLCK, 0, 100);
         write_pipe(pipes[0]);
         read_pipe(pipes[1]);
 
@@ -301,20 +308,20 @@ static void test_file_close() {
 
 /* Test: child waits for parent to release a lock. */
 static void test_child_wait() {
-    printf("test child wait...\n");
+    printf("testing child wait...\n");
     unlock(0, 0);
 
     int pipes[2][2];
     open_pipes(pipes);
 
-    lock_ok(F_RDLCK, 0, 100);
+    lock(F_RDLCK, 0, 100);
 
     pid_t pid = fork();
     if (pid < 0)
         err(1, "fork");
 
     if (pid == 0) {
-        lock_ok(F_RDLCK, 0, 100);
+        lock(F_RDLCK, 0, 100);
         lock_fail(F_WRLCK, 0, 100);
         write_pipe(pipes[0]);
         lock_wait_ok(F_WRLCK, 0, 100);
@@ -330,7 +337,7 @@ static void test_child_wait() {
 
 /* Test: parent waits for child to release a lock. */
 static void test_parent_wait() {
-    printf("test parent wait...\n");
+    printf("testing parent wait...\n");
     unlock(0, 0);
 
     int pipes[2][2];
@@ -341,7 +348,7 @@ static void test_parent_wait() {
         err(1, "fork");
 
     if (pid == 0) {
-        lock_ok(F_RDLCK, 0, 100);
+        lock(F_RDLCK, 0, 100);
         write_pipe(pipes[0]);
         read_pipe(pipes[1]);
         unlock(0, 100);
@@ -354,7 +361,7 @@ static void test_parent_wait() {
     read_pipe(pipes[0]);
 
     /* read lock should succeed */
-    lock_ok(F_RDLCK, 0, 100);
+    lock(F_RDLCK, 0, 100);
     lock_fail(F_WRLCK, 0, 100);
     write_pipe(pipes[1]);
     lock_wait_ok(F_WRLCK, 0, 100);
@@ -366,7 +373,7 @@ static void test_parent_wait() {
 
 /* Test: check that a range until EOF (len == 0) is handled correctly. */
 static void test_range_with_eof() {
-    printf("test range with EOF...\n");
+    printf("testing range with EOF...\n");
     unlock(0, 0);
 
     int pipes[2][2];
@@ -378,7 +385,7 @@ static void test_range_with_eof() {
 
     if (pid == 0) {
         /* lock [100 .. EOF] */
-        lock_ok(F_WRLCK, 100, 0);
+        lock(F_WRLCK, 100, 0);
         write_pipe(pipes[0]);
         read_pipe(pipes[1]);
         exit(0);
@@ -411,6 +418,9 @@ int main(void) {
     if (close(g_fd) < 0)
         err(1, "close");
 
+    if (unlink(TEST_FILE) < 0)
+        err(1, "unlink");
+
     printf("TEST OK\n");
     return 0;
 }

LibOS/shim/test/regression/test_libos.py

@@ -586,7 +586,8 @@ def test_110_fcntl_lock(self):
         try:
             stdout, _ = self.run_binary(['fcntl_lock'])
         finally:
-            os.remove('tmp/lock_file')
+            if os.path.exists('tmp/lock_file'):
+                os.remove('tmp/lock_file')
         self.assertIn('TEST OK', stdout)
 
 class TC_31_Syscall(RegressionTestCase):