From c3e0a68c7eeeb4f491c0464392b2146ddec4305a Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Thu, 14 May 2020 13:52:41 +0200 Subject: canonicalisePathMetaData(): Support a UID range --- src/libstore/local-store.cc | 27 +++++++++++++++++---------- 1 file changed, 17 insertions(+), 10 deletions(-) (limited to 'src/libstore/local-store.cc') diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index eed225349..80ebe903f 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -424,7 +424,10 @@ void canonicaliseTimestampAndPermissions(const Path & path) } -static void canonicalisePathMetaData_(const Path & path, uid_t fromUid, InodesSeen & inodesSeen) +static void canonicalisePathMetaData_( + const Path & path, + std::optional> uidRange, + InodesSeen & inodesSeen) { checkInterrupt(); @@ -475,7 +478,7 @@ static void canonicalisePathMetaData_(const Path & path, uid_t fromUid, InodesSe However, ignore files that we chown'ed ourselves previously to ensure that we don't fail on hard links within the same build (i.e. "touch $out/foo; ln $out/foo $out/bar"). */ - if (fromUid != (uid_t) -1 && st.st_uid != fromUid) { + if (uidRange && (st.st_uid < uidRange->first || st.st_uid > uidRange->second)) { assert(!S_ISDIR(st.st_mode)); if (inodesSeen.find(Inode(st.st_dev, st.st_ino)) == inodesSeen.end()) throw BuildError("invalid ownership on file '%1%'", path); @@ -509,14 +512,17 @@ static void canonicalisePathMetaData_(const Path & path, uid_t fromUid, InodesSe if (S_ISDIR(st.st_mode)) { DirEntries entries = readDirectory(path); for (auto & i : entries) - canonicalisePathMetaData_(path + "/" + i.name, fromUid, inodesSeen); + canonicalisePathMetaData_(path + "/" + i.name, uidRange, inodesSeen); } } -void canonicalisePathMetaData(const Path & path, uid_t fromUid, InodesSeen & inodesSeen) +void canonicalisePathMetaData( + const Path & path, + std::optional> uidRange, + InodesSeen & inodesSeen) { - canonicalisePathMetaData_(path, fromUid, inodesSeen); + canonicalisePathMetaData_(path, uidRange, inodesSeen); /* On platforms that don't have lchown(), the top-level path can't be a symlink, since we can't change its ownership. */ @@ -531,10 +537,11 @@ void canonicalisePathMetaData(const Path & path, uid_t fromUid, InodesSeen & ino } -void canonicalisePathMetaData(const Path & path, uid_t fromUid) +void canonicalisePathMetaData(const Path & path, + std::optional> uidRange) { InodesSeen inodesSeen; - canonicalisePathMetaData(path, fromUid, inodesSeen); + canonicalisePathMetaData(path, uidRange, inodesSeen); } @@ -1021,7 +1028,7 @@ void LocalStore::addToStore(const ValidPathInfo & info, Source & source, autoGC(); - canonicalisePathMetaData(realPath, -1); + canonicalisePathMetaData(realPath, {}); optimisePath(realPath); // FIXME: combine with hashPath() @@ -1064,7 +1071,7 @@ StorePath LocalStore::addToStoreFromDump(const string & dump, const string & nam } else writeFile(realPath, dump); - canonicalisePathMetaData(realPath, -1); + canonicalisePathMetaData(realPath, {}); /* Register the SHA-256 hash of the NAR serialisation of the path in the database. We may just have computed it @@ -1134,7 +1141,7 @@ StorePath LocalStore::addTextToStore(const string & name, const string & s, writeFile(realPath, s); - canonicalisePathMetaData(realPath, -1); + canonicalisePathMetaData(realPath, {}); StringSink sink; dumpString(s, sink); -- cgit v1.2.3 From 75b62e52600a44b42693944b50638bf580a2c86e Mon Sep 17 00:00:00 2001 From: John Ericson Date: Thu, 18 Jun 2020 17:54:16 +0000 Subject: Avoid `fmt` when constructor already does it There is a correctnes issue here, but #3724 will fix that. This is just a cleanup for brevity's sake. --- src/libstore/local-store.cc | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) (limited to 'src/libstore/local-store.cc') diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index d77fff963..ece5bb5ef 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -482,18 +482,18 @@ void LocalStore::openDB(State & state, bool create) SQLiteStmt stmt; stmt.create(db, "pragma main.journal_mode;"); if (sqlite3_step(stmt) != SQLITE_ROW) - throwSQLiteError(db, "querying journal mode"); + SQLiteError::throw_(db, "querying journal mode"); prevMode = std::string((const char *) sqlite3_column_text(stmt, 0)); } if (prevMode != mode && sqlite3_exec(db, ("pragma main.journal_mode = " + mode + ";").c_str(), 0, 0, 0) != SQLITE_OK) - throwSQLiteError(db, "setting journal mode"); + SQLiteError::throw_(db, "setting journal mode"); /* Increase the auto-checkpoint interval to 40000 pages. This seems enough to ensure that instantiating the NixOS system derivation is done in a single fsync(). */ if (mode == "wal" && sqlite3_exec(db, "pragma wal_autocheckpoint = 40000;", 0, 0, 0) != SQLITE_OK) - throwSQLiteError(db, "setting autocheckpoint interval"); + SQLiteError::throw_(db, "setting autocheckpoint interval"); /* Initialise the database schema, if necessary. */ if (create) { -- cgit v1.2.3 From 05ec0beb40f5e4e162903570b68837b34811a02d Mon Sep 17 00:00:00 2001 From: John Ericson Date: Wed, 20 Apr 2022 16:57:06 +0000 Subject: Move templated functions to `sqlite-impl.hh` This ensures that use-sites properly trigger new monomorphisations on one hand, and on the other hand keeps the main `sqlite.hh` clean and interface-only. I think that is good practice in general, but in this situation in particular we do indeed have `sqlite.hh` users that don't need the `throw_` function. --- src/libstore/local-store.cc | 1 + 1 file changed, 1 insertion(+) (limited to 'src/libstore/local-store.cc') diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index ece5bb5ef..42cc30cbf 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -10,6 +10,7 @@ #include "topo-sort.hh" #include "finally.hh" #include "compression.hh" +#include "sqlite-impl.hh" #include #include -- cgit v1.2.3 From f63b0f4540b61d8cdac7a3c52ca6e190f7c1b8cf Mon Sep 17 00:00:00 2001 From: John Ericson Date: Wed, 20 Apr 2022 17:37:59 +0000 Subject: Actually, solve this in a lighter-weight way The templating is very superficial --- src/libstore/local-store.cc | 1 - 1 file changed, 1 deletion(-) (limited to 'src/libstore/local-store.cc') diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index 42cc30cbf..ece5bb5ef 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -10,7 +10,6 @@ #include "topo-sort.hh" #include "finally.hh" #include "compression.hh" -#include "sqlite-impl.hh" #include #include -- cgit v1.2.3 From 92656da0b953b92228ef4a252d504c07710e4b47 Mon Sep 17 00:00:00 2001 From: regnat Date: Wed, 3 Nov 2021 16:30:22 +0100 Subject: Fix the gc with indirect self-references via the realisations MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If the derivation `foo` depends on `bar`, and they both have the same output path (because they are CA derivations), then this output path will depend both on the realisation of `foo` and of `bar`, which themselves depend on each other. This confuses SQLite which isn’t able to automatically solve this diamond dependency scheme. Help it by adding a trigger to delete all the references between the relevant realisations. Fix #5320 --- src/libstore/local-store.cc | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) (limited to 'src/libstore/local-store.cc') diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index ece5bb5ef..e64917956 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -81,7 +81,7 @@ int getSchema(Path schemaPath) void migrateCASchema(SQLite& db, Path schemaPath, AutoCloseFD& lockFd) { - const int nixCASchemaVersion = 3; + const int nixCASchemaVersion = 4; int curCASchema = getSchema(schemaPath); if (curCASchema != nixCASchemaVersion) { if (curCASchema > nixCASchemaVersion) { @@ -143,6 +143,19 @@ void migrateCASchema(SQLite& db, Path schemaPath, AutoCloseFD& lockFd) )"); txn.commit(); } + if (curCASchema < 4) { + SQLiteTxn txn(db); + db.exec(R"( + create trigger if not exists DeleteSelfRefsViaRealisations before delete on ValidPaths + begin + delete from RealisationsRefs where realisationReference = ( + select id from Realisations where outputPath = old.id + ); + end; + )"); + txn.commit(); + } + writeFile(schemaPath, fmt("%d", nixCASchemaVersion)); lockFile(lockFd.get(), ltRead, true); } -- cgit v1.2.3 From 86d7a11c6b338a73c30476be13a6cac562e309c3 Mon Sep 17 00:00:00 2001 From: regnat Date: Fri, 5 Nov 2021 11:17:22 +0100 Subject: Make sure to delete all the realisation refs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Deleting just one will only work in the test cases where I didn’t bother creating too many of them :p --- src/libstore/local-store.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'src/libstore/local-store.cc') diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index e64917956..76524b01c 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -148,7 +148,7 @@ void migrateCASchema(SQLite& db, Path schemaPath, AutoCloseFD& lockFd) db.exec(R"( create trigger if not exists DeleteSelfRefsViaRealisations before delete on ValidPaths begin - delete from RealisationsRefs where realisationReference = ( + delete from RealisationsRefs where realisationReference in ( select id from Realisations where outputPath = old.id ); end; -- cgit v1.2.3 From 975b0b52e74168b034ccff23827eff4d626f1c46 Mon Sep 17 00:00:00 2001 From: Sergei Trofimovich Date: Sat, 26 Mar 2022 16:18:51 +0000 Subject: ca: add sqlite index on `RealisationsRefs(realisationReference)` Without the change any CA deletion triggers linear scan on large RealisationsRefs table: sqlite>.eqp full sqlite> delete from RealisationsRefs where realisationReference IN ( select id from Realisations where outputPath = 1234567890 ); QUERY PLAN |--SCAN RealisationsRefs `--LIST SUBQUERY 1 `--SEARCH Realisations USING COVERING INDEX IndexRealisationsRefsOnOutputPath (outputPath=?) With the change it gets turned into a lookup: sqlite> CREATE INDEX IndexRealisationsRefsRealisationReference on RealisationsRefs(realisationReference); sqlite> delete from RealisationsRefs where realisationReference IN ( select id from Realisations where outputPath = 1234567890 ); QUERY PLAN |--SEARCH RealisationsRefs USING INDEX IndexRealisationsRefsRealisationReference (realisationReference=?) `--LIST SUBQUERY 1 `--SEARCH Realisations USING COVERING INDEX IndexRealisationsRefsOnOutputPath (outputPath=?) --- src/libstore/local-store.cc | 2 ++ 1 file changed, 2 insertions(+) (limited to 'src/libstore/local-store.cc') diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index 76524b01c..5cc5c91cc 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -152,6 +152,8 @@ void migrateCASchema(SQLite& db, Path schemaPath, AutoCloseFD& lockFd) select id from Realisations where outputPath = old.id ); end; + -- used by deletion trigger + create index if not exists IndexRealisationsRefsRealisationReference on RealisationsRefs(realisationReference); )"); txn.commit(); } -- cgit v1.2.3 From 1385b2007804c8a0370f2a6555045a00e34b07c7 Mon Sep 17 00:00:00 2001 From: Alain Zscheile Date: Wed, 4 May 2022 07:44:32 +0200 Subject: Get rid of most `.at` calls (#6393) Use one of `get` or `getOr` instead which will either return a null-pointer (with a nicer error message) or a default value when the key is missing. --- src/libstore/local-store.cc | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) (limited to 'src/libstore/local-store.cc') diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index 5cc5c91cc..eba3b0fa5 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -718,7 +718,11 @@ void LocalStore::checkDerivationOutputs(const StorePath & drvPath, const Derivat // somewhat expensive so we do lazily hashesModulo = hashDerivationModulo(*this, drv, true); } - StorePath recomputed = makeOutputPath(i.first, hashesModulo->hashes.at(i.first), drvName); + auto currentOutputHash = get(hashesModulo->hashes, i.first); + if (!currentOutputHash) + throw Error("derivation '%s' has unexpected output '%s' (local-store / hashesModulo) named '%s'", + printStorePath(drvPath), printStorePath(doia.path), i.first); + StorePath recomputed = makeOutputPath(i.first, *currentOutputHash, drvName); if (doia.path != recomputed) throw Error("derivation '%s' has incorrect output '%s', should be '%s'", printStorePath(drvPath), printStorePath(doia.path), printStorePath(recomputed)); -- cgit v1.2.3 From c2de0a232c1cfddb1f1385ffd23dd43a2099458e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Th=C3=A9ophane=20Hufschmitt?= Date: Thu, 17 Mar 2022 15:28:46 +0100 Subject: =?UTF-8?q?Create=20a=20wrapper=20around=20stdlib=E2=80=99s=20`ren?= =?UTF-8?q?ame`?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Directly takes some c++ strings, and gently throws an exception on error (rather than having to inline this logic everywhere) --- src/libstore/local-store.cc | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) (limited to 'src/libstore/local-store.cc') diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index eba3b0fa5..7f708b243 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -1430,8 +1430,7 @@ StorePath LocalStore::addToStoreFromDump(Source & source0, std::string_view name writeFile(realPath, dumpSource); } else { /* Move the temporary path we restored above. */ - if (rename(tempPath.c_str(), realPath.c_str())) - throw Error("renaming '%s' to '%s'", tempPath, realPath); + moveFile(tempPath, realPath); } /* For computing the nar hash. In recursive SHA-256 mode, this @@ -1942,8 +1941,7 @@ void LocalStore::addBuildLog(const StorePath & drvPath, std::string_view log) writeFile(tmpFile, compress("bzip2", log)); - if (rename(tmpFile.c_str(), logPath.c_str()) != 0) - throw SysError("renaming '%1%' to '%2%'", tmpFile, logPath); + moveFile(tmpFile, logPath); } std::optional LocalStore::getVersion() -- cgit v1.2.3 From d71d9e9fbfc972514b0b940181ab7f9e766f41f3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Th=C3=A9ophane=20Hufschmitt?= Date: Wed, 13 Apr 2022 14:10:36 +0200 Subject: moveFile -> renameFile MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `move` tends to have this `mv` connotation of “I will copy it for you if needs be” --- src/libstore/local-store.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'src/libstore/local-store.cc') diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index 7f708b243..2d076f404 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -1430,7 +1430,7 @@ StorePath LocalStore::addToStoreFromDump(Source & source0, std::string_view name writeFile(realPath, dumpSource); } else { /* Move the temporary path we restored above. */ - moveFile(tempPath, realPath); + renameFile(tempPath, realPath); } /* For computing the nar hash. In recursive SHA-256 mode, this @@ -1941,7 +1941,7 @@ void LocalStore::addBuildLog(const StorePath & drvPath, std::string_view log) writeFile(tmpFile, compress("bzip2", log)); - moveFile(tmpFile, logPath); + renameFile(tmpFile, logPath); } std::optional LocalStore::getVersion() -- cgit v1.2.3 From 90f968073338d6ba276994bc281fa5efa5306e17 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Th=C3=A9ophane=20Hufschmitt?= Date: Wed, 13 Apr 2022 14:19:42 +0200 Subject: Only use `renameFile` where needed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In most places the fallback to copying isn’t needed and can actually be bad, so we’d rather not transparently fallback --- src/libstore/local-store.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'src/libstore/local-store.cc') diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index 2d076f404..a272e4301 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -1430,7 +1430,7 @@ StorePath LocalStore::addToStoreFromDump(Source & source0, std::string_view name writeFile(realPath, dumpSource); } else { /* Move the temporary path we restored above. */ - renameFile(tempPath, realPath); + moveFile(tempPath, realPath); } /* For computing the nar hash. In recursive SHA-256 mode, this -- cgit v1.2.3 From 1f041ac54f43093e4f4df1caa630d491ff51c3f8 Mon Sep 17 00:00:00 2001 From: Andrew Brooks Date: Fri, 2 Sep 2022 18:32:35 -0500 Subject: Prevent tempdir from being GC-ed before addToStoreFromDump has renamed it This fixes issue 6823 by placing the tempdir used in LocalStore::addToStoreFromDump outside the Nix store, where automatic GC is no longer a concern. --- src/libstore/local-store.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'src/libstore/local-store.cc') diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index a272e4301..6abd52683 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -1388,7 +1388,7 @@ StorePath LocalStore::addToStoreFromDump(Source & source0, std::string_view name StringSource dumpSource { dump }; ChainSource bothSource { dumpSource, source }; - auto tempDir = createTempDir(realStoreDir, "add"); + auto tempDir = createTempDir("", "add"); delTempDir = std::make_unique(tempDir); tempPath = tempDir + "/x"; -- cgit v1.2.3 From 84fe75a12a085c6b4b8d4ac65a048f569de1252b Mon Sep 17 00:00:00 2001 From: Andrew Brooks Date: Tue, 6 Sep 2022 17:48:00 -0500 Subject: Keep created temp dirs inside store, but protect from GC Implements the approach suggested by feedback on PR #6994, where tempdir paths are created in the store (now with an exclusive lock). As part of this work, the currently-broken and unused `createTempDirInStore` function is updated to create an exclusive lock on the temp directory in the store. The GC now makes a non-blocking attempt to lock any store directories that "look like" the temp directories created by this function, and if it can't acquire one, ignores the directory. --- src/libstore/local-store.cc | 29 +++++++++++++++++++---------- 1 file changed, 19 insertions(+), 10 deletions(-) (limited to 'src/libstore/local-store.cc') diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index 6abd52683..5ee451da3 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -1382,13 +1382,15 @@ StorePath LocalStore::addToStoreFromDump(Source & source0, std::string_view name std::unique_ptr delTempDir; Path tempPath; + Path tempDir; + AutoCloseFD tempDirFd; if (!inMemory) { /* Drain what we pulled so far, and then keep on pulling */ StringSource dumpSource { dump }; ChainSource bothSource { dumpSource, source }; - auto tempDir = createTempDir("", "add"); + std::tie(tempDir, tempDirFd) = createTempDirInStore(); delTempDir = std::make_unique(tempDir); tempPath = tempDir + "/x"; @@ -1431,6 +1433,7 @@ StorePath LocalStore::addToStoreFromDump(Source & source0, std::string_view name } else { /* Move the temporary path we restored above. */ moveFile(tempPath, realPath); + tempDirFd.close(); } /* For computing the nar hash. In recursive SHA-256 mode, this @@ -1507,18 +1510,24 @@ StorePath LocalStore::addTextToStore( /* Create a temporary directory in the store that won't be - garbage-collected. */ -Path LocalStore::createTempDirInStore() + garbage-collected until the returned FD is closed. */ +std::pair LocalStore::createTempDirInStore() { - Path tmpDir; + Path tmpDirFn; + AutoCloseFD tmpDirFd; + bool lockedByUs = false; do { /* There is a slight possibility that `tmpDir' gets deleted by - the GC between createTempDir() and addTempRoot(), so repeat - until `tmpDir' exists. */ - tmpDir = createTempDir(realStoreDir); - addTempRoot(parseStorePath(tmpDir)); - } while (!pathExists(tmpDir)); - return tmpDir; + the GC between createTempDir() and when we acquire a lock on it. + We'll repeat until 'tmpDir' exists and we've locked it. */ + tmpDirFn = createTempDir(realStoreDir, "add"); + tmpDirFd = open(tmpDirFn.c_str(), O_RDONLY | O_DIRECTORY); + if (tmpDirFd.get() < 0) { + continue; + } + lockedByUs = lockFile(tmpDirFd.get(), ltWrite, true); + } while (!pathExists(tmpDirFn) || !lockedByUs); + return {tmpDirFn, std::move(tmpDirFd)}; } -- cgit v1.2.3 From 565d888e0f6a2c66ee7b10f6fe6a97f79fa51732 Mon Sep 17 00:00:00 2001 From: Andrew Brooks Date: Mon, 12 Sep 2022 11:33:23 -0500 Subject: Address PR feedback on #6694 --- src/libstore/local-store.cc | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) (limited to 'src/libstore/local-store.cc') diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index 5ee451da3..0b07cde34 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -1433,7 +1433,6 @@ StorePath LocalStore::addToStoreFromDump(Source & source0, std::string_view name } else { /* Move the temporary path we restored above. */ moveFile(tempPath, realPath); - tempDirFd.close(); } /* For computing the nar hash. In recursive SHA-256 mode, this @@ -1520,7 +1519,7 @@ std::pair LocalStore::createTempDirInStore() /* There is a slight possibility that `tmpDir' gets deleted by the GC between createTempDir() and when we acquire a lock on it. We'll repeat until 'tmpDir' exists and we've locked it. */ - tmpDirFn = createTempDir(realStoreDir, "add"); + tmpDirFn = createTempDir(realStoreDir, "tmp"); tmpDirFd = open(tmpDirFn.c_str(), O_RDONLY | O_DIRECTORY); if (tmpDirFd.get() < 0) { continue; -- cgit v1.2.3 From 1b595026e18afb050de3f62ded8f7180bc8b2b0e Mon Sep 17 00:00:00 2001 From: squalus Date: Mon, 19 Sep 2022 11:15:31 -0700 Subject: Improve durability of schema version file writes - call close explicitly in writeFile to prevent the close exception from being ignored - fsync after writing schema file to flush data to disk - fsync schema file parent to flush metadata to disk https://github.com/NixOS/nix/issues/7064 --- src/libstore/local-store.cc | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) (limited to 'src/libstore/local-store.cc') diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index 0b07cde34..37302d3a8 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -158,7 +158,7 @@ void migrateCASchema(SQLite& db, Path schemaPath, AutoCloseFD& lockFd) txn.commit(); } - writeFile(schemaPath, fmt("%d", nixCASchemaVersion)); + writeFile(schemaPath, fmt("%d", nixCASchemaVersion), 0666, true); lockFile(lockFd.get(), ltRead, true); } } @@ -281,7 +281,7 @@ LocalStore::LocalStore(const Params & params) else if (curSchema == 0) { /* new store */ curSchema = nixSchemaVersion; openDB(*state, true); - writeFile(schemaPath, (format("%1%") % nixSchemaVersion).str()); + writeFile(schemaPath, (format("%1%") % nixSchemaVersion).str(), 0666, true); } else if (curSchema < nixSchemaVersion) { @@ -329,7 +329,7 @@ LocalStore::LocalStore(const Params & params) txn.commit(); } - writeFile(schemaPath, (format("%1%") % nixSchemaVersion).str()); + writeFile(schemaPath, (format("%1%") % nixSchemaVersion).str(), 0666, true); lockFile(globalLock.get(), ltRead, true); } -- cgit v1.2.3 From 752f967c0fe2489fe13d8c2c65c3ecba72064adc Mon Sep 17 00:00:00 2001 From: John Ericson Date: Thu, 22 Sep 2022 10:43:48 -0400 Subject: "valid signature" -> "trustworthy signature" I just had a colleague get confused by the previous phrase for good reason. "valid" sounds like an *objective* criterion, e.g. and *invalid signature* would be one that would be trusted by no one, e.g. because it misformatted or something. What is actually going is that there might be a signature which is perfectly valid to *someone else*, but not to the user, because they don't trust the corresponding public key. This is a *subjective* criterion, because it depends on the arbitrary and personal choice of which public keys to trust. I therefore think "trustworthy" is a better adjective to use. Whether something is worthy of trust is clearly subjective, and then "trust" within that word nicely evokes `trusted-public-keys` and friends. --- src/libstore/local-store.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'src/libstore/local-store.cc') diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index 37302d3a8..b64ae6080 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -751,7 +751,7 @@ void LocalStore::registerDrvOutput(const Realisation & info, CheckSigsFlag check if (checkSigs == NoCheckSigs || !realisationIsUntrusted(info)) registerDrvOutput(info); else - throw Error("cannot register realisation '%s' because it lacks a valid signature", info.outPath.to_string()); + throw Error("cannot register realisation '%s' because it lacks a trustworthy signature", info.outPath.to_string()); } void LocalStore::registerDrvOutput(const Realisation & info) @@ -1266,7 +1266,7 @@ void LocalStore::addToStore(const ValidPathInfo & info, Source & source, RepairFlag repair, CheckSigsFlag checkSigs) { if (checkSigs && pathInfoIsUntrusted(info)) - throw Error("cannot add path '%s' because it lacks a valid signature", printStorePath(info.path)); + throw Error("cannot add path '%s' because it lacks a trustworthy signature", printStorePath(info.path)); addTempRoot(info.path); -- cgit v1.2.3 From a2a8cb10ac17e03691b9f73ae14e5b6edbe66f4e Mon Sep 17 00:00:00 2001 From: John Ericson Date: Thu, 22 Sep 2022 14:36:26 -0400 Subject: Dodge "trusted" vs "trustworthy" by being explicit Hopefully this is best! --- src/libstore/local-store.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'src/libstore/local-store.cc') diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index b64ae6080..d374d4558 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -751,7 +751,7 @@ void LocalStore::registerDrvOutput(const Realisation & info, CheckSigsFlag check if (checkSigs == NoCheckSigs || !realisationIsUntrusted(info)) registerDrvOutput(info); else - throw Error("cannot register realisation '%s' because it lacks a trustworthy signature", info.outPath.to_string()); + throw Error("cannot register realisation '%s' because it lacks a signature by a trusted key", info.outPath.to_string()); } void LocalStore::registerDrvOutput(const Realisation & info) @@ -1266,7 +1266,7 @@ void LocalStore::addToStore(const ValidPathInfo & info, Source & source, RepairFlag repair, CheckSigsFlag checkSigs) { if (checkSigs && pathInfoIsUntrusted(info)) - throw Error("cannot add path '%s' because it lacks a trustworthy signature", printStorePath(info.path)); + throw Error("cannot add path '%s' because it lacks a signature by a trusted key", printStorePath(info.path)); addTempRoot(info.path); -- cgit v1.2.3 From ec45f4b82eaef8da04f4b828b2b06a77aa3f986f Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Mon, 21 Nov 2022 11:12:45 +0100 Subject: Fix indentation --- src/libstore/local-store.cc | 50 ++++++++++++++++++++++----------------------- 1 file changed, 25 insertions(+), 25 deletions(-) (limited to 'src/libstore/local-store.cc') diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index 04223d860..b67668e52 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -897,48 +897,48 @@ void LocalStore::queryPathInfoUncached(const StorePath & path, std::shared_ptr LocalStore::queryPathInfoInternal(State & state, const StorePath & path) { - /* Get the path info. */ + /* Get the path info. */ auto useQueryPathInfo(state.stmts->QueryPathInfo.use()(printStorePath(path))); - if (!useQueryPathInfo.next()) - return std::shared_ptr(); + if (!useQueryPathInfo.next()) + return std::shared_ptr(); - auto id = useQueryPathInfo.getInt(0); + auto id = useQueryPathInfo.getInt(0); - auto narHash = Hash::dummy; - try { - narHash = Hash::parseAnyPrefixed(useQueryPathInfo.getStr(1)); - } catch (BadHash & e) { - throw Error("invalid-path entry for '%s': %s", printStorePath(path), e.what()); - } + auto narHash = Hash::dummy; + try { + narHash = Hash::parseAnyPrefixed(useQueryPathInfo.getStr(1)); + } catch (BadHash & e) { + throw Error("invalid-path entry for '%s': %s", printStorePath(path), e.what()); + } - auto info = std::make_shared(path, narHash); + auto info = std::make_shared(path, narHash); - info->id = id; + info->id = id; - info->registrationTime = useQueryPathInfo.getInt(2); + info->registrationTime = useQueryPathInfo.getInt(2); auto s = (const char *) sqlite3_column_text(state.stmts->QueryPathInfo, 3); - if (s) info->deriver = parseStorePath(s); + if (s) info->deriver = parseStorePath(s); - /* Note that narSize = NULL yields 0. */ - info->narSize = useQueryPathInfo.getInt(4); + /* Note that narSize = NULL yields 0. */ + info->narSize = useQueryPathInfo.getInt(4); - info->ultimate = useQueryPathInfo.getInt(5) == 1; + info->ultimate = useQueryPathInfo.getInt(5) == 1; s = (const char *) sqlite3_column_text(state.stmts->QueryPathInfo, 6); - if (s) info->sigs = tokenizeString(s, " "); + if (s) info->sigs = tokenizeString(s, " "); s = (const char *) sqlite3_column_text(state.stmts->QueryPathInfo, 7); - if (s) info->ca = parseContentAddressOpt(s); + if (s) info->ca = parseContentAddressOpt(s); - /* Get the references. */ + /* Get the references. */ auto useQueryReferences(state.stmts->QueryReferences.use()(info->id)); - while (useQueryReferences.next()) - info->references.insert(parseStorePath(useQueryReferences.getStr(0))); + while (useQueryReferences.next()) + info->references.insert(parseStorePath(useQueryReferences.getStr(0))); - return info; + return info; } @@ -1041,9 +1041,9 @@ LocalStore::queryPartialDerivationOutputMap(const StorePath & path_) auto path = path_; auto outputs = retrySQLite>>([&]() { auto state(_state.lock()); - std::map> outputs; + std::map> outputs; uint64_t drvId; - drvId = queryValidPathId(*state, path); + drvId = queryValidPathId(*state, path); auto use(state->stmts->QueryDerivationOutputs.use()(drvId)); while (use.next()) outputs.insert_or_assign( -- cgit v1.2.3 From 81c3f99b3668a1ea6a37792a5bcc3bc6f39729a2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Na=C3=AFm=20Favier?= Date: Sat, 17 Dec 2022 18:31:00 +0100 Subject: Release shared lock before acquiring exclusive lock In principle, this should avoid deadlocks where two instances of Nix are holding a shared lock on big-lock and are both waiting to get an exclusive lock. However, it seems like `flock(2)` is supposed to do this automatically, so it's not clear whether this is actually where the problem comes from. --- src/libstore/local-store.cc | 2 ++ 1 file changed, 2 insertions(+) (limited to 'src/libstore/local-store.cc') diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index b67668e52..3bab10af9 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -91,6 +91,7 @@ void migrateCASchema(SQLite& db, Path schemaPath, AutoCloseFD& lockFd) if (!lockFile(lockFd.get(), ltWrite, false)) { printInfo("waiting for exclusive access to the Nix store for ca drvs..."); + lockFile(lockFd.get(), ltNone, false); // We have acquired a shared lock; release it to prevent deadlocks lockFile(lockFd.get(), ltWrite, true); } @@ -299,6 +300,7 @@ LocalStore::LocalStore(const Params & params) if (!lockFile(globalLock.get(), ltWrite, false)) { printInfo("waiting for exclusive access to the Nix store..."); + lockFile(globalLock.get(), ltNone, false); // We have acquired a shared lock; release it to prevent deadlocks lockFile(globalLock.get(), ltWrite, true); } -- cgit v1.2.3 From 224b56f10e1bb754d403c106eb9d1b947fc30414 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Tue, 3 Jan 2023 14:51:23 +0100 Subject: 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/local-store.cc | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) (limited to 'src/libstore/local-store.cc') 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 (...) { -- cgit v1.2.3