From 2c7557481b4c9d5113a65cc7a75c8acc18031f4e Mon Sep 17 00:00:00 2001 From: John Ericson Date: Fri, 24 Jul 2020 21:02:51 +0000 Subject: [PATCH] `queryDerivationOutputMap` no longer assumes all outputs have a mapping This assumption is broken by CA derivations. Making a PR now to do the breaking daemon change as soon as possible (if it is already too late, we can bump protocol intead). --- src/libstore/build.cc | 10 +++++-- src/libstore/daemon.cc | 4 +-- src/libstore/local-store.cc | 12 +++++--- src/libstore/local-store.hh | 2 +- src/libstore/remote-store.cc | 31 +++++++------------- src/libstore/remote-store.hh | 2 +- src/libstore/store-api.cc | 13 ++++++++- src/libstore/store-api.hh | 10 +++++-- src/libstore/worker-protocol.hh | 52 ++++++++++++++++++++++++++++++++- src/nix-env/nix-env.cc | 2 +- 10 files changed, 102 insertions(+), 36 deletions(-) diff --git a/src/libstore/build.cc b/src/libstore/build.cc index 62294a08c..3a55f24cb 100644 --- a/src/libstore/build.cc +++ b/src/libstore/build.cc @@ -2756,8 +2756,12 @@ struct RestrictedStore : public LocalFSStore void queryReferrers(const StorePath & path, StorePathSet & referrers) override { } - OutputPathMap queryDerivationOutputMap(const StorePath & path) override - { throw Error("queryDerivationOutputMap"); } + std::map> queryDerivationOutputMap(const StorePath & path) override + { + if (!goal.isAllowed(path)) + throw InvalidPath("cannot query output map for unknown path '%s' in recursive Nix", printStorePath(path)); + return next->queryDerivationOutputMap(path); + } std::optional queryPathFromHashPart(const std::string & hashPart) override { throw Error("queryPathFromHashPart"); } @@ -4918,7 +4922,7 @@ void Worker::waitForInput() std::vector buffer(4096); for (auto & k : fds2) { if (pollStatus.at(fdToPollStatus.at(k)).revents) { - ssize_t rd = read(k, buffer.data(), buffer.size()); + ssize_t rd = ::read(k, buffer.data(), buffer.size()); // FIXME: is there a cleaner way to handle pt close // than EIO? Is this even standard? if (rd == 0 || (rd == -1 && errno == EIO)) { diff --git a/src/libstore/daemon.cc b/src/libstore/daemon.cc index 7e16529a5..82833f5c3 100644 --- a/src/libstore/daemon.cc +++ b/src/libstore/daemon.cc @@ -325,9 +325,9 @@ static void performOp(TunnelLogger * logger, ref store, case wopQueryDerivationOutputMap: { auto path = store->parseStorePath(readString(from)); logger->startWork(); - OutputPathMap outputs = store->queryDerivationOutputMap(path); + auto outputs = store->queryDerivationOutputMap(path); logger->stopWork(); - writeOutputPathMap(*store, to, outputs); + write(*store, to, outputs); break; } diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index 9ea71170f..4db4f7d6a 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -774,17 +774,21 @@ StorePathSet LocalStore::queryValidDerivers(const StorePath & path) } -OutputPathMap LocalStore::queryDerivationOutputMap(const StorePath & path) +std::map> LocalStore::queryDerivationOutputMap(const StorePath & path) { - return retrySQLite([&]() { + std::map> outputs; + BasicDerivation drv = readDerivation(path); + for (auto & [outName, _] : drv.outputs) { + outputs.insert_or_assign(outName, std::nullopt); + } + return retrySQLite>>([&]() { auto state(_state.lock()); auto useQueryDerivationOutputs(state->stmtQueryDerivationOutputs.use() (queryValidPathId(*state, path))); - OutputPathMap outputs; while (useQueryDerivationOutputs.next()) - outputs.emplace( + outputs.insert_or_assign( useQueryDerivationOutputs.getStr(0), parseStorePath(useQueryDerivationOutputs.getStr(1)) ); diff --git a/src/libstore/local-store.hh b/src/libstore/local-store.hh index 355c2814f..56ef1d251 100644 --- a/src/libstore/local-store.hh +++ b/src/libstore/local-store.hh @@ -133,7 +133,7 @@ public: StorePathSet queryValidDerivers(const StorePath & path) override; - OutputPathMap queryDerivationOutputMap(const StorePath & path) override; + std::map> queryDerivationOutputMap(const StorePath & path) override; std::optional queryPathFromHashPart(const std::string & hashPart) override; diff --git a/src/libstore/remote-store.cc b/src/libstore/remote-store.cc index 9af4364b7..3b3cdbc02 100644 --- a/src/libstore/remote-store.cc +++ b/src/libstore/remote-store.cc @@ -39,30 +39,21 @@ void writeStorePaths(const Store & store, Sink & out, const StorePathSet & paths out << store.printStorePath(i); } -std::map readOutputPathMap(const Store & store, Source & from) + +StorePath read(const Store & store, Source & from, Proxy _) { - std::map pathMap; - auto rawInput = readStrings(from); - if (rawInput.size() % 2) - throw Error("got an odd number of elements from the daemon when trying to read a output path map"); - auto curInput = rawInput.begin(); - while (curInput != rawInput.end()) { - auto thisKey = *curInput++; - auto thisValue = *curInput++; - pathMap.emplace(thisKey, store.parseStorePath(thisValue)); - } - return pathMap; + auto path = readString(from); + return store.parseStorePath(path); } -void writeOutputPathMap(const Store & store, Sink & out, const std::map & pathMap) + +void write(const Store & store, Sink & out, const StorePath & storePath) { - out << 2*pathMap.size(); - for (auto & i : pathMap) { - out << i.first; - out << store.printStorePath(i.second); - } + auto path = store.printStorePath(storePath); + out << path; } + /* TODO: Separate these store impls into different files, give them better names */ RemoteStore::RemoteStore(const Params & params) : Store(params) @@ -445,12 +436,12 @@ StorePathSet RemoteStore::queryDerivationOutputs(const StorePath & path) } -OutputPathMap RemoteStore::queryDerivationOutputMap(const StorePath & path) +std::map> RemoteStore::queryDerivationOutputMap(const StorePath & path) { auto conn(getConnection()); conn->to << wopQueryDerivationOutputMap << printStorePath(path); conn.processStderr(); - return readOutputPathMap(*this, conn->from); + return read(*this, conn->from, Proxy>> {}); } diff --git a/src/libstore/remote-store.hh b/src/libstore/remote-store.hh index 3c1b78b6a..b39a8de48 100644 --- a/src/libstore/remote-store.hh +++ b/src/libstore/remote-store.hh @@ -51,7 +51,7 @@ public: StorePathSet queryDerivationOutputs(const StorePath & path) override; - OutputPathMap queryDerivationOutputMap(const StorePath & path) override; + std::map> queryDerivationOutputMap(const StorePath & path) override; std::optional queryPathFromHashPart(const std::string & hashPart) override; StorePathSet querySubstitutablePaths(const StorePathSet & paths) override; diff --git a/src/libstore/store-api.cc b/src/libstore/store-api.cc index ab21b0bd5..a624fafdd 100644 --- a/src/libstore/store-api.cc +++ b/src/libstore/store-api.cc @@ -330,9 +330,20 @@ bool Store::PathInfoCacheValue::isKnownNow() return std::chrono::steady_clock::now() < time_point + ttl; } +OutputPathMap Store::queryDerivationOutputMapAssumeTotal(const StorePath & path) { + auto resp = queryDerivationOutputMap(path); + OutputPathMap result; + for (auto & [outName, optOutPath] : resp) { + if (!optOutPath) + throw Error("output '%s' has no store path mapped to it", outName); + result.insert_or_assign(outName, *optOutPath); + } + return result; +} + StorePathSet Store::queryDerivationOutputs(const StorePath & path) { - auto outputMap = this->queryDerivationOutputMap(path); + auto outputMap = this->queryDerivationOutputMapAssumeTotal(path); StorePathSet outputPaths; for (auto & i: outputMap) { outputPaths.emplace(std::move(i.second)); diff --git a/src/libstore/store-api.hh b/src/libstore/store-api.hh index d1cb2035f..a83a2ff10 100644 --- a/src/libstore/store-api.hh +++ b/src/libstore/store-api.hh @@ -423,10 +423,16 @@ public: /* Query the outputs of the derivation denoted by `path'. */ virtual StorePathSet queryDerivationOutputs(const StorePath & path); - /* Query the mapping outputName=>outputPath for the given derivation */ - virtual OutputPathMap queryDerivationOutputMap(const StorePath & path) + /* Query the mapping outputName => outputPath for the given derivation. All + outputs are mentioned so ones mising the mapping are mapped to + `std::nullopt`. */ + virtual std::map> queryDerivationOutputMap(const StorePath & path) { unsupported("queryDerivationOutputMap"); } + /* Query the mapping outputName=>outputPath for the given derivation. + Assume every output has a mapping and throw an exception otherwise. */ + OutputPathMap queryDerivationOutputMapAssumeTotal(const StorePath & path); + /* Query the full store path given the hash part of a valid store path, or empty if the path doesn't exist. */ virtual std::optional queryPathFromHashPart(const std::string & hashPart) = 0; diff --git a/src/libstore/worker-protocol.hh b/src/libstore/worker-protocol.hh index 8b538f6da..6a6e29640 100644 --- a/src/libstore/worker-protocol.hh +++ b/src/libstore/worker-protocol.hh @@ -70,6 +70,56 @@ template T readStorePaths(const Store & store, Source & from); void writeStorePaths(const Store & store, Sink & out, const StorePathSet & paths); -void writeOutputPathMap(const Store & store, Sink & out, const OutputPathMap & paths); +/* To guide overloading */ +template +struct Proxy {}; + +template +std::map read(const Store & store, Source & from, Proxy> _) +{ + std::map resMap; + auto size = (size_t)readInt(from); + while (size--) { + auto thisKey = readString(from); + resMap.insert_or_assign(std::move(thisKey), read(store, from, Proxy {})); + } + return resMap; +} + +template +void write(const Store & store, Sink & out, const std::map & resMap) +{ + out << resMap.size(); + for (auto & i : resMap) { + out << i.first; + write(store, out, i.second); + } +} + +template +std::optional read(const Store & store, Source & from, Proxy> _) +{ + auto tag = readNum(from); + switch (tag) { + case 0: + return std::nullopt; + case 1: + return read(store, from, Proxy {}); + default: + throw Error("got an invalid tag bit for std::optional: %#04x", tag); + } +} + +template +void write(const Store & store, Sink & out, const std::optional & optVal) +{ + out << (optVal ? 1 : 0); + if (optVal) + write(store, out, *optVal); +} + +StorePath read(const Store & store, Source & from, Proxy _); + +void write(const Store & store, Sink & out, const StorePath & storePath); } diff --git a/src/nix-env/nix-env.cc b/src/nix-env/nix-env.cc index ddd036070..d36804658 100644 --- a/src/nix-env/nix-env.cc +++ b/src/nix-env/nix-env.cc @@ -381,7 +381,7 @@ static void queryInstSources(EvalState & state, if (path.isDerivation()) { elem.setDrvPath(state.store->printStorePath(path)); - auto outputs = state.store->queryDerivationOutputMap(path); + auto outputs = state.store->queryDerivationOutputMapAssumeTotal(path); elem.setOutPath(state.store->printStorePath(outputs.at("out"))); if (name.size() >= drvExtension.size() && string(name, name.size() - drvExtension.size()) == drvExtension)