From 224b56f10e1bb754d403c106eb9d1b947fc30414 Mon Sep 17 00:00:00 2001
From: Eelco Dolstra <edolstra@gmail.com>
Date: Tue, 3 Jan 2023 14:51:23 +0100
Subject: [PATCH] Move creation of the temp roots file into its own function

This also moves the file handle into its own Sync object so we're not
holding the _state while acquiring the file lock. There was no real
deadlock risk here since locking a newly created file cannot block,
but it's still a bit nicer.
---
 src/libstore/gc.cc          | 52 +++++++++++++++++++++----------------
 src/libstore/local-store.cc |  6 ++---
 src/libstore/local-store.hh | 12 ++++++---
 3 files changed, 41 insertions(+), 29 deletions(-)

diff --git a/src/libstore/gc.cc b/src/libstore/gc.cc
index 5d91829f1..f8c29593f 100644
--- a/src/libstore/gc.cc
+++ b/src/libstore/gc.cc
@@ -77,37 +77,43 @@ Path LocalFSStore::addPermRoot(const StorePath & storePath, const Path & _gcRoot
 }
 
 
-void LocalStore::addTempRoot(const StorePath & path)
+void LocalStore::createTempRootsFile()
 {
-    auto state(_state.lock());
+    auto fdTempRoots(_fdTempRoots.lock());
 
     /* Create the temporary roots file for this process. */
-    if (!state->fdTempRoots) {
+    if (*fdTempRoots) return;
 
-        while (1) {
-            if (pathExists(fnTempRoots))
-                /* It *must* be stale, since there can be no two
-                   processes with the same pid. */
-                unlink(fnTempRoots.c_str());
+    while (1) {
+        if (pathExists(fnTempRoots))
+            /* It *must* be stale, since there can be no two
+               processes with the same pid. */
+            unlink(fnTempRoots.c_str());
 
-            state->fdTempRoots = openLockFile(fnTempRoots, true);
+        *fdTempRoots = openLockFile(fnTempRoots, true);
 
-            debug("acquiring write lock on '%s'", fnTempRoots);
-            lockFile(state->fdTempRoots.get(), ltWrite, true);
+        debug("acquiring write lock on '%s'", fnTempRoots);
+        lockFile(fdTempRoots->get(), ltWrite, true);
 
-            /* Check whether the garbage collector didn't get in our
-               way. */
-            struct stat st;
-            if (fstat(state->fdTempRoots.get(), &st) == -1)
-                throw SysError("statting '%1%'", fnTempRoots);
-            if (st.st_size == 0) break;
-
-            /* The garbage collector deleted this file before we could
-               get a lock.  (It won't delete the file after we get a
-               lock.)  Try again. */
-        }
+        /* Check whether the garbage collector didn't get in our
+           way. */
+        struct stat st;
+        if (fstat(fdTempRoots->get(), &st) == -1)
+            throw SysError("statting '%1%'", fnTempRoots);
+        if (st.st_size == 0) break;
 
+        /* The garbage collector deleted this file before we could get
+           a lock.  (It won't delete the file after we get a lock.)
+           Try again. */
     }
+}
+
+
+void LocalStore::addTempRoot(const StorePath & path)
+{
+    createTempRootsFile();
+
+    auto state(_state.lock());
 
     if (!state->fdGCLock)
         state->fdGCLock = openGCLock();
@@ -162,7 +168,7 @@ void LocalStore::addTempRoot(const StorePath & path)
 
     /* Append the store path to the temporary roots file. */
     auto s = printStorePath(path) + '\0';
-    writeFull(state->fdTempRoots.get(), s);
+    writeFull(_fdTempRoots.lock()->get(), s);
 }
 
 
diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc
index 3bab10af9..be21e3ca0 100644
--- a/src/libstore/local-store.cc
+++ b/src/libstore/local-store.cc
@@ -441,9 +441,9 @@ LocalStore::~LocalStore()
     }
 
     try {
-        auto state(_state.lock());
-        if (state->fdTempRoots) {
-            state->fdTempRoots = -1;
+        auto fdTempRoots(_fdTempRoots.lock());
+        if (*fdTempRoots) {
+            *fdTempRoots = -1;
             unlink(fnTempRoots.c_str());
         }
     } catch (...) {
diff --git a/src/libstore/local-store.hh b/src/libstore/local-store.hh
index 4579c2f62..9ea0c0bf7 100644
--- a/src/libstore/local-store.hh
+++ b/src/libstore/local-store.hh
@@ -62,9 +62,6 @@ private:
         /* The global GC lock  */
         AutoCloseFD fdGCLock;
 
-        /* The file to which we write our temporary roots. */
-        AutoCloseFD fdTempRoots;
-
         /* Connection to the garbage collector. */
         AutoCloseFD fdRootsSocket;
 
@@ -156,6 +153,15 @@ public:
 
     void addTempRoot(const StorePath & path) override;
 
+private:
+
+    void createTempRootsFile();
+
+    /* The file to which we write our temporary roots. */
+    Sync<AutoCloseFD> _fdTempRoots;
+
+public:
+
     void addIndirectRoot(const Path & path) override;
 
 private: