* Move addTempRoot() to the store API, and add another function

  syncWithGC() to allow clients to register GC roots without needing
  write access to the global roots directory or the GC lock.

8 files changed, 64 insertions, 13 deletions

diff --git a/src/libstore/build.cc b/src/libstore/build.cc
index ab1011981c94..71560b2d0c51 100644
--- a/src/libstore/build.cc
+++ b/src/libstore/build.cc
@@ -642,7 +642,7 @@ void DerivationGoal::haveStoreExpr()
 
     for (DerivationOutputs::iterator i = drv.outputs.begin();
          i != drv.outputs.end(); ++i)
-        addTempRoot(i->second.path);
+        store->addTempRoot(i->second.path);
 
     /* Check what outputs paths are not already valid. */
     PathSet invalidOutputs = checkPathValidity(false);
@@ -1714,7 +1714,7 @@ void SubstitutionGoal::init()
 {
     trace("init");
 
-    addTempRoot(storePath);
+    store->addTempRoot(storePath);
     
     /* If the path already exists we're done. */
     if (store->isValidPath(storePath)) {
diff --git a/src/libstore/gc.cc b/src/libstore/gc.cc
index 3e4150d89811..56e64369a741 100644
--- a/src/libstore/gc.cc
+++ b/src/libstore/gc.cc
@@ -76,6 +76,12 @@ void createSymlink(const Path & link, const Path & target, bool careful)
 }
 
 
+void LocalStore::syncWithGC()
+{
+    AutoCloseFD fdGCLock = openGCLock(ltRead);
+}
+
+
 Path addPermRoot(const Path & _storePath, const Path & _gcRoot,
     bool indirect, bool allowOutsideRootsDir)
 {
@@ -83,10 +89,6 @@ Path addPermRoot(const Path & _storePath, const Path & _gcRoot,
     Path gcRoot(canonPath(_gcRoot));
     assertStorePath(storePath);
 
-    /* Grab the global GC root.  This prevents the set of permanent
-       roots from increasing while a GC is in progress. */
-    AutoCloseFD fdGCLock = openGCLock(ltRead);
-
     if (indirect) {
         string hash = printHash32(hashString(htSHA1, gcRoot));
         Path realRoot = canonPath((format("%1%/%2%/auto/%3%")
@@ -110,6 +112,11 @@ Path addPermRoot(const Path & _storePath, const Path & _gcRoot,
         createSymlink(gcRoot, storePath, false);
     }
 
+    /* Grab the global GC root, causing us to block while a GC is in
+       progress.  This prevents the set of permanent roots from
+       increasing while a GC is in progress. */
+    store->syncWithGC();
+    
     return gcRoot;
 }
 
@@ -119,7 +126,7 @@ static Path fnTempRoots;
 static AutoCloseFD fdTempRoots;
 
 
-void addTempRoot(const Path & path)
+void LocalStore::addTempRoot(const Path & path)
 {
     /* Create the temporary roots file for this process. */
     if (fdTempRoots == -1) {
diff --git a/src/libstore/gc.hh b/src/libstore/gc.hh
index 3f7242884b59..d4f40afa2ac1 100644
--- a/src/libstore/gc.hh
+++ b/src/libstore/gc.hh
@@ -26,12 +26,6 @@ typedef enum {
 void collectGarbage(GCAction action, const PathSet & pathsToDelete,
     bool ignoreLiveness, PathSet & result, unsigned long long & bytesFreed);
 
-/* Register a temporary GC root.  This root will automatically
-   disappear when this process exits.  WARNING: this function should
-   not be called inside a BDB transaction, otherwise we can
-   deadlock. */
-void addTempRoot(const Path & path);
-
 /* Remove the temporary roots file for this process.  Any temporary
    root becomes garbage after this point unless it has been registered
    as a (permanent) root. */
diff --git a/src/libstore/local-store.hh b/src/libstore/local-store.hh
index 3a7b22048e4d..389be33a37db 100644
--- a/src/libstore/local-store.hh
+++ b/src/libstore/local-store.hh
@@ -58,6 +58,10 @@ public:
     void buildDerivations(const PathSet & drvPaths);
 
     void ensurePath(const Path & path);
+
+    void addTempRoot(const Path & path);
+
+    void syncWithGC();
 };
 
 
diff --git a/src/libstore/remote-store.cc b/src/libstore/remote-store.cc
index 2391fd9e87f1..9b9d74f7e801 100644
--- a/src/libstore/remote-store.cc
+++ b/src/libstore/remote-store.cc
@@ -170,4 +170,19 @@ void RemoteStore::ensurePath(const Path & path)
 }
 
 
+void RemoteStore::addTempRoot(const Path & path)
+{
+    writeInt(wopAddTempRoot, to);
+    writeString(path, to);
+    readInt(from);
+}
+
+
+void RemoteStore::syncWithGC()
+{
+    writeInt(wopSyncWithGC, to);
+    readInt(from);
+}
+
+
 }
diff --git a/src/libstore/remote-store.hh b/src/libstore/remote-store.hh
index 62feee0eacf0..b11191c09d81 100644
--- a/src/libstore/remote-store.hh
+++ b/src/libstore/remote-store.hh
@@ -47,6 +47,10 @@ public:
 
     void ensurePath(const Path & path);
 
+    void addTempRoot(const Path & path);
+
+    void syncWithGC();
+    
 private:
     Pipe toChild;
     Pipe fromChild;
diff --git a/src/libstore/store-api.hh b/src/libstore/store-api.hh
index 6fbe979316f2..67d230ca7725 100644
--- a/src/libstore/store-api.hh
+++ b/src/libstore/store-api.hh
@@ -86,6 +86,31 @@ public:
        may be made valid by running a substitute (if defined for the
        path). */
     virtual void ensurePath(const Path & path) = 0;
+
+    /* Add a store path as a temporary root of the garbage collector.
+       The root disappears as soon as we exit. */
+    virtual void addTempRoot(const Path & path) = 0;
+
+    /* Acquire the global GC lock, then immediately release it.  This
+       function must be called after registering a new permanent root,
+       but before exiting.  Otherwise, it is possible that a running
+       garbage collector doesn't see the new root and deletes the
+       stuff we've just built.  By acquiring the lock briefly, we
+       ensure that either:
+
+       - The collector is already running, and so we block until the
+         collector is finished.  The collector will know about our
+         *temporary* locks, which should include whatever it is we
+         want to register as a permanent lock.
+
+       - The collector isn't running, or it's just started but hasn't
+         acquired the GC lock yet.  In that case we get and release
+         the lock right away, then exit.  The collector scans the
+         permanent root and sees our's.
+
+       In either case the permanent root is seen by the collector. */
+    virtual void syncWithGC() = 0;
+
 };
 
 
diff --git a/src/libstore/worker-protocol.hh b/src/libstore/worker-protocol.hh
index 65f5fc100a17..2700b6719715 100644
--- a/src/libstore/worker-protocol.hh
+++ b/src/libstore/worker-protocol.hh
@@ -18,6 +18,8 @@ typedef enum {
     wopAddTextToStore,
     wopBuildDerivations,
     wopEnsurePath,
+    wopAddTempRoot,
+    wopSyncWithGC
 } WorkerOp;