From 0fa289f559708407ab4384739c0f24258c114b44 Mon Sep 17 00:00:00 2001 From: julia Date: Sat, 18 May 2024 15:38:33 +1000 Subject: [PATCH] Harmonise the Store::queryPathInfoUncached interface MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This: - Consistently returns `nullptr` for a non-existent store path, instead of a mix of `nullptr` and throwing exceptions. - If a store returns "bad" store paths in response to a request (e.g. incorrect hash or name), don't cache this result. This removes some duplication of code at the cache-access layer of queryPathInfo() checking this, and ­allows us to provide more specific errors. Part of #270. Change-Id: I86612c6499b1a37ab872c712c2304d6a3ff19edb --- src/libstore/binary-cache-store.cc | 2 +- src/libstore/local-store.cc | 2 +- src/libstore/remote-store.cc | 6 +++--- src/libstore/store-api.cc | 34 ++++++++++++++++++++---------- src/libstore/store-api.hh | 4 ++++ 5 files changed, 32 insertions(+), 16 deletions(-) diff --git a/src/libstore/binary-cache-store.cc b/src/libstore/binary-cache-store.cc index ea643fd31..3600eca60 100644 --- a/src/libstore/binary-cache-store.cc +++ b/src/libstore/binary-cache-store.cc @@ -361,7 +361,7 @@ std::shared_ptr BinaryCacheStore::queryPathInfoUncached(con auto data = getFile(narInfoFile); - if (!data) return {}; + if (!data) return nullptr; stats.narInfoRead++; diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index f5eaa9f5f..bae5fad7b 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -899,7 +899,7 @@ std::shared_ptr LocalStore::queryPathInfoInternal(State & s auto useQueryPathInfo(state.stmts->QueryPathInfo.use()(printStorePath(path))); if (!useQueryPathInfo.next()) - return std::shared_ptr(); + return nullptr; auto id = useQueryPathInfo.getInt(0); diff --git a/src/libstore/remote-store.cc b/src/libstore/remote-store.cc index 93b1afabd..966dd3fe0 100644 --- a/src/libstore/remote-store.cc +++ b/src/libstore/remote-store.cc @@ -309,14 +309,14 @@ std::shared_ptr RemoteStore::queryPathInfoUncached(const St try { conn.processStderr(); } catch (Error & e) { - // Ugly backwards compatibility hack. + // Ugly backwards compatibility hack. TODO(fj#325): remove. if (e.msg().find("is not valid") != std::string::npos) - throw InvalidPath(std::move(e.info())); + return nullptr; throw; } if (GET_PROTOCOL_MINOR(conn->daemonVersion) >= 17) { bool valid; conn->from >> valid; - if (!valid) throw InvalidPath("path '%s' is not valid", printStorePath(path)); + if (!valid) return nullptr; } return std::make_shared( StorePath{path}, diff --git a/src/libstore/store-api.cc b/src/libstore/store-api.cc index 952958d51..e0e842060 100644 --- a/src/libstore/store-api.cc +++ b/src/libstore/store-api.cc @@ -666,11 +666,19 @@ bool Store::isValidPathUncached(const StorePath & path) } -static bool goodStorePath(const StorePath & expected, const StorePath & actual) +static void ensureGoodStorePath(Store * store, const StorePath & expected, const StorePath & actual) { - return - expected.hashPart() == actual.hashPart() - && (expected.name() == Store::MissingName || expected.name() == actual.name()); + if (expected.hashPart() != actual.hashPart()) { + throw Error( + "the queried store path hash '%s' did not match expected '%s' while querying the store path '%s'", + expected.hashPart(), actual.hashPart(), store->printStorePath(expected) + ); + } else if (expected.name() != Store::MissingName && expected.name() != actual.name()) { + throw Error( + "the queried store path name '%s' did not match expected '%s' while querying the store path '%s'", + expected.name(), actual.name(), store->printStorePath(expected) + ); + } } @@ -683,7 +691,7 @@ ref Store::queryPathInfo(const StorePath & storePath) if (res && res->isKnownNow()) { stats.narInfoReadAverted++; if (!res->didExist()) - throw InvalidPath("path '%s' is not valid", printStorePath(storePath)); + throw InvalidPath("path '%s' does not exist in the store", printStorePath(storePath)); return ref(res->value); } } @@ -696,27 +704,31 @@ ref Store::queryPathInfo(const StorePath & storePath) auto state_(state.lock()); state_->pathInfoCache.upsert(std::string(storePath.to_string()), res.first == NarInfoDiskCache::oInvalid ? PathInfoCacheValue{} : PathInfoCacheValue{ .value = res.second }); - if (res.first == NarInfoDiskCache::oInvalid || - !goodStorePath(storePath, res.second->path)) - throw InvalidPath("path '%s' is not valid", printStorePath(storePath)); + if (res.first == NarInfoDiskCache::oInvalid) + throw InvalidPath("path '%s' does not exist in the store", printStorePath(storePath)); } return ref(res.second); } } auto info = queryPathInfoUncached(storePath); + if (info) { + // first, before we cache anything, check that the store gave us valid data. + ensureGoodStorePath(this, storePath, info->path); + } - if (diskCache) + if (diskCache) { diskCache->upsertNarInfo(getUri(), hashPart, info); + } { auto state_(state.lock()); state_->pathInfoCache.upsert(std::string(storePath.to_string()), PathInfoCacheValue { .value = info }); } - if (!info || !goodStorePath(storePath, info->path)) { + if (!info) { stats.narInfoMissing++; - throw InvalidPath("path '%s' is not valid", printStorePath(storePath)); + throw InvalidPath("path '%s' does not exist in the store", printStorePath(storePath)); } return ref(info); diff --git a/src/libstore/store-api.hh b/src/libstore/store-api.hh index 397ebe759..de4dc3f03 100644 --- a/src/libstore/store-api.hh +++ b/src/libstore/store-api.hh @@ -399,6 +399,10 @@ public: protected: + /** + * Queries the path info without caching. + * Note to implementors: should return `nullptr` when the path is not found. + */ virtual std::shared_ptr queryPathInfoUncached(const StorePath & path) = 0; virtual std::shared_ptr queryRealisationUncached(const DrvOutput &) = 0;