Use the fs accessor for readInvalidDerivation

Extend `FSAccessor::readFile` to allow not checking that the path is a valid one, and rewrite `readInvalidDerivation` using this extended `readFile`. Several places in the code use `readInvalidDerivation`, either because they need to read a derivation that has been written in the store but not registered yet, or more generally to prevent a deadlock because `readDerivation` tries to lock the state, so can't be called from a place where the lock is already held. However, `readInvalidDerivation` implicitely assumes that the store is a `LocalFSStore`, which isn't always the case. The concrete motivation for this is that it's required for `nix copy --from someBinaryCache` to work, which is tremendously useful for the tests.
NixOS · Dec 15, 2020 · 7080321 · 7080321
1 parent f2f60bf
commit 7080321
Show file tree

Hide file tree

Showing 6 changed files with 28 additions and 24 deletions.
diff --git a/src/libstore/fs-accessor.hh b/src/libstore/fs-accessor.hh
@@ -25,7 +25,14 @@ public:
 
     virtual StringSet readDirectory(const Path & path) = 0;
 
-    virtual std::string readFile(const Path & path) = 0;
+    /**
+     * Read a file inside the store.
+     *
+     * If `requireValidPath` is set to `true` (the default), the path must be
+     * inside a valid store path, otherwise it just needs to be physically
+     * present (but not necessarily properly registered)
+     */
+    virtual std::string readFile(const Path & path, bool requireValidPath = true) = 0;
 
     virtual std::string readLink(const Path & path) = 0;
 };

diff --git a/src/libstore/local-fs-store.cc b/src/libstore/local-fs-store.cc
@@ -19,10 +19,10 @@ struct LocalStoreAccessor : public FSAccessor
 
     LocalStoreAccessor(ref<LocalFSStore> store) : store(store) { }
 
-    Path toRealPath(const Path & path)
+    Path toRealPath(const Path & path, bool requireValidPath = true)
     {
         auto storePath = store->toStorePath(path).first;
-        if (!store->isValidPath(storePath))
+        if (requireValidPath && !store->isValidPath(storePath))
             throw InvalidPath("path '%1%' is not a valid store path", store->printStorePath(storePath));
         return store->getRealStoreDir() + std::string(path, store->storeDir.size());
     }
@@ -61,9 +61,9 @@ struct LocalStoreAccessor : public FSAccessor
         return res;
     }
 
-    std::string readFile(const Path & path) override
+    std::string readFile(const Path & path, bool requireValidPath = true) override
     {
-        return nix::readFile(toRealPath(path));
+        return nix::readFile(toRealPath(path, requireValidPath));
     }
 
     std::string readLink(const Path & path) override

diff --git a/src/libstore/nar-accessor.cc b/src/libstore/nar-accessor.cc
@@ -203,7 +203,7 @@ struct NarAccessor : public FSAccessor
         return res;
     }
 
-    std::string readFile(const Path & path) override
+    std::string readFile(const Path & path, bool requireValidPath = true) override
     {
         auto i = get(path);
         if (i.type != FSAccessor::Type::tRegular)

diff --git a/src/libstore/remote-fs-accessor.cc b/src/libstore/remote-fs-accessor.cc
@@ -43,13 +43,13 @@ void RemoteFSAccessor::addToCache(std::string_view hashPart, const std::string &
     }
 }
 
-std::pair<ref<FSAccessor>, Path> RemoteFSAccessor::fetch(const Path & path_)
+std::pair<ref<FSAccessor>, Path> RemoteFSAccessor::fetch(const Path & path_, bool requireValidPath)
 {
     auto path = canonPath(path_);
 
     auto [storePath, restPath] = store->toStorePath(path);
 
-    if (!store->isValidPath(storePath))
+    if (requireValidPath && !store->isValidPath(storePath))
         throw InvalidPath("path '%1%' is not a valid store path", store->printStorePath(storePath));
 
     auto i = nars.find(std::string(storePath.hashPart()));
@@ -113,9 +113,9 @@ StringSet RemoteFSAccessor::readDirectory(const Path & path)
     return res.first->readDirectory(res.second);
 }
 
-std::string RemoteFSAccessor::readFile(const Path & path)
+std::string RemoteFSAccessor::readFile(const Path & path, bool requireValidPath)
 {
-    auto res = fetch(path);
+    auto res = fetch(path, requireValidPath);
     return res.first->readFile(res.second);
 }
 

diff --git a/src/libstore/remote-fs-accessor.hh b/src/libstore/remote-fs-accessor.hh
@@ -14,7 +14,7 @@ class RemoteFSAccessor : public FSAccessor
 
     Path cacheDir;
 
-    std::pair<ref<FSAccessor>, Path> fetch(const Path & path_);
+    std::pair<ref<FSAccessor>, Path> fetch(const Path & path_, bool requireValidPath = true);
 
     friend class BinaryCacheStore;
 
@@ -32,7 +32,7 @@ public:
 
     StringSet readDirectory(const Path & path) override;
 
-    std::string readFile(const Path & path) override;
+    std::string readFile(const Path & path, bool requireValidPath = true) override;
 
     std::string readLink(const Path & path) override;
 };

diff --git a/src/libstore/store-api.cc b/src/libstore/store-api.cc
@@ -1018,26 +1018,23 @@ Derivation Store::derivationFromPath(const StorePath & drvPath)
     return readDerivation(drvPath);
 }
 
-
-Derivation Store::readDerivation(const StorePath & drvPath)
+Derivation readDerivationCommon(Store& store, const StorePath& drvPath, bool requireValidPath)
 {
-    auto accessor = getFSAccessor();
+    auto accessor = store.getFSAccessor();
     try {
-        return parseDerivation(*this,
-            accessor->readFile(printStorePath(drvPath)),
+        return parseDerivation(store,
+            accessor->readFile(store.printStorePath(drvPath), requireValidPath),
             Derivation::nameFromPath(drvPath));
     } catch (FormatError & e) {
-        throw Error("error parsing derivation '%s': %s", printStorePath(drvPath), e.msg());
+        throw Error("error parsing derivation '%s': %s", store.printStorePath(drvPath), e.msg());
     }
 }
 
+Derivation Store::readDerivation(const StorePath & drvPath)
+{ return readDerivationCommon(*this, drvPath, true); }
+
 Derivation Store::readInvalidDerivation(const StorePath & drvPath)
-{
-    return parseDerivation(
-        *this,
-        readFile(Store::toRealPath(drvPath)),
-        Derivation::nameFromPath(drvPath));
-}
+{ return readDerivationCommon(*this, drvPath, false); }
 
 }