From a17282fc66acac590d5a0923c97b8c61933a7f11 Mon Sep 17 00:00:00 2001 From: Jade Lovelace Date: Fri, 24 May 2024 20:45:05 -0600 Subject: [PATCH 1/6] Set up minimum protocol version Change-Id: Ibb931109a8328cfb22964542ab53644cc4181f9e --- src/libstore/worker-protocol.hh | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/libstore/worker-protocol.hh b/src/libstore/worker-protocol.hh index dcd54ad16..cc083a551 100644 --- a/src/libstore/worker-protocol.hh +++ b/src/libstore/worker-protocol.hh @@ -9,7 +9,16 @@ namespace nix { #define WORKER_MAGIC_1 0x6e697863 #define WORKER_MAGIC_2 0x6478696f +// This must remain 1.35 (Nix 2.18) forever in Lix, since the protocol has +// diverged in CppNix such that we cannot assign newer versions ourselves, the +// protocol is bad in design and implementation and Lix intends to replace it +// entirely. #define PROTOCOL_VERSION (1 << 8 | 35) +// Nix 2.3 is protocol 1.21 (see RemoteStore::initConnection for client, +// processConnection for server). +#define MIN_SUPPORTED_MINOR_WORKER_PROTO_VERSION 21 +#define MIN_SUPPORTED_WORKER_PROTO_VERSION (1 << 8 | MIN_SUPPORTED_MINOR_WORKER_PROTO_VERSION) + #define GET_PROTOCOL_MAJOR(x) ((x) & 0xff00) #define GET_PROTOCOL_MINOR(x) ((x) & 0x00ff) From 24255748b484984b5fed21c75e0cf836524a4916 Mon Sep 17 00:00:00 2001 From: Jade Lovelace Date: Fri, 24 May 2024 20:45:05 -0600 Subject: [PATCH 2/6] Delete old ValidPathInfo test, fix UnkeyedValidPathInfo The UnkeyedValidPathInfo test was testing an ancient version but not the current version. Doesn't make much sense to me. Change-Id: Ib476a4297d9075f2dcd31a073b3e7b149b2189af --- .../unkeyed-valid-path-info-1.15.bin | Bin 328 -> 0 bytes .../unkeyed-valid-path-info.bin | Bin 0 -> 376 bytes .../worker-protocol/valid-path-info-1.15.bin | Bin 488 -> 0 bytes ...path-info-1.16.bin => valid-path-info.bin} | Bin tests/unit/libstore/worker-protocol.cc | 63 +++--------------- 5 files changed, 8 insertions(+), 55 deletions(-) delete mode 100644 tests/unit/libstore/data/libstore/worker-protocol/unkeyed-valid-path-info-1.15.bin create mode 100644 tests/unit/libstore/data/libstore/worker-protocol/unkeyed-valid-path-info.bin delete mode 100644 tests/unit/libstore/data/libstore/worker-protocol/valid-path-info-1.15.bin rename tests/unit/libstore/data/libstore/worker-protocol/{valid-path-info-1.16.bin => valid-path-info.bin} (100%) diff --git a/tests/unit/libstore/data/libstore/worker-protocol/unkeyed-valid-path-info-1.15.bin b/tests/unit/libstore/data/libstore/worker-protocol/unkeyed-valid-path-info-1.15.bin deleted file mode 100644 index e69ccbe83862b29e3f79810c0c2ff4561a42f86f..0000000000000000000000000000000000000000 GIT binary patch literal 0 HcmV?d00001 literal 328 zcmbu4%?`pK5QOo8D!&WM#q>Qq0R{CUX=_wYUVUsEAs(9a)(w2^a=MH48Q8;TGRcmlTf(6rfXPFjSJ46(yf&k8}#9A2z znmI2!Xw { /** * For serializers that don't care about the minimum version, we - * used the oldest one: 1.0. + * have to use the minimum supported to not throw an assert. */ - WorkerProto::Version defaultVersion = 1 << 8 | 0; + WorkerProto::Version defaultVersion = MIN_SUPPORTED_WORKER_PROTO_VERSION; }; @@ -331,9 +331,9 @@ VERSIONED_CHARACTERIZATION_TEST( VERSIONED_CHARACTERIZATION_TEST( WorkerProtoTest, - unkeyedValidPathInfo_1_15, - "unkeyed-valid-path-info-1.15", - 1 << 8 | 15, + unkeyedValidPathInfo, + "unkeyed-valid-path-info", + defaultVersion, (std::tuple { ({ UnkeyedValidPathInfo info { @@ -363,56 +363,9 @@ VERSIONED_CHARACTERIZATION_TEST( VERSIONED_CHARACTERIZATION_TEST( WorkerProtoTest, - validPathInfo_1_15, - "valid-path-info-1.15", - 1 << 8 | 15, - (std::tuple { - ({ - ValidPathInfo info { - StorePath { - "g1w7hy3qg1w7hy3qg1w7hy3qg1w7hy3q-bar", - }, - UnkeyedValidPathInfo { - Hash::parseSRI("sha256-FePFYIlMuycIXPZbWi7LGEiMmZSX9FMbaQenWBzm1Sc="), - }, - }; - info.registrationTime = 23423; - info.narSize = 34878; - info; - }), - ({ - ValidPathInfo info { - StorePath { - "g1w7hy3qg1w7hy3qg1w7hy3qg1w7hy3q-bar", - }, - UnkeyedValidPathInfo { - Hash::parseSRI("sha256-FePFYIlMuycIXPZbWi7LGEiMmZSX9FMbaQenWBzm1Sc="), - }, - }; - info.deriver = StorePath { - "g1w7hy3qg1w7hy3qg1w7hy3qg1w7hy3q-bar.drv", - }; - info.references = { - // other reference - StorePath { - "g1w7hyyyy1w7hy3qg1w7hy3qgqqqqy3q-foo", - }, - // self reference - StorePath { - "g1w7hy3qg1w7hy3qg1w7hy3qg1w7hy3q-bar", - }, - }; - info.registrationTime = 23423; - info.narSize = 34878; - info; - }), - })) - -VERSIONED_CHARACTERIZATION_TEST( - WorkerProtoTest, - validPathInfo_1_16, - "valid-path-info-1.16", - 1 << 8 | 16, + validPathInfo, + "valid-path-info", + defaultVersion, (std::tuple { ({ ValidPathInfo info { From 7b1d38bc4f287bfb6f057ca8958f1326e8bd9d53 Mon Sep 17 00:00:00 2001 From: Jade Lovelace Date: Fri, 24 May 2024 20:45:05 -0600 Subject: [PATCH 3/6] libstore daemon: remove very old protocol support (<2.3) Change-Id: Ic05f478a659c199a66fe78ae05d357d317ac41b0 --- src/libstore/daemon.cc | 76 ++++++++++--------------- src/libstore/remote-store-connection.hh | 7 +-- 2 files changed, 33 insertions(+), 50 deletions(-) diff --git a/src/libstore/daemon.cc b/src/libstore/daemon.cc index 4b55668cb..e256ad1de 100644 --- a/src/libstore/daemon.cc +++ b/src/libstore/daemon.cc @@ -47,10 +47,15 @@ struct TunnelLogger : public Logger Sync state_; - WorkerProto::Version clientVersion; + /** + * Worker protocol version of the other side. May be newer than this daemon. + */ + const WorkerProto::Version clientVersion; TunnelLogger(FdSink & to, WorkerProto::Version clientVersion) - : to(to), clientVersion(clientVersion) { } + : to(to), clientVersion(clientVersion) { + assert(clientVersion >= MIN_SUPPORTED_WORKER_PROTO_VERSION); + } void enqueueMsg(const std::string & s) { @@ -129,12 +134,6 @@ struct TunnelLogger : public Logger void startActivity(ActivityId act, Verbosity lvl, ActivityType type, const std::string & s, const Fields & fields, ActivityId parent) override { - if (GET_PROTOCOL_MINOR(clientVersion) < 20) { - if (!s.empty()) - log(lvl, s + "..."); - return; - } - StringSink buf; buf << STDERR_START_ACTIVITY << act << lvl << type << s << fields << parent; enqueueMsg(buf.s); @@ -142,7 +141,6 @@ struct TunnelLogger : public Logger void stopActivity(ActivityId act) override { - if (GET_PROTOCOL_MINOR(clientVersion) < 20) return; StringSink buf; buf << STDERR_STOP_ACTIVITY << act; enqueueMsg(buf.s); @@ -150,7 +148,6 @@ struct TunnelLogger : public Logger void result(ActivityId act, ResultType type, const Fields & fields) override { - if (GET_PROTOCOL_MINOR(clientVersion) < 20) return; StringSink buf; buf << STDERR_RESULT << act << type << fields; enqueueMsg(buf.s); @@ -527,22 +524,19 @@ static void performOp(TunnelLogger * logger, ref store, case WorkerProto::Op::BuildPaths: { auto drvs = WorkerProto::Serialise::read(*store, rconn); - BuildMode mode = bmNormal; - if (GET_PROTOCOL_MINOR(clientVersion) >= 15) { - mode = buildModeFromInteger(readInt(from)); + BuildMode mode = buildModeFromInteger(readInt(from)); - /* Repairing is not atomic, so disallowed for "untrusted" - clients. + /* Repairing is not atomic, so disallowed for "untrusted" + clients. - FIXME: layer violation in this message: the daemon code (i.e. - this file) knows whether a client/connection is trusted, but it - does not how how the client was authenticated. The mechanism - need not be getting the UID of the other end of a Unix Domain - Socket. - */ - if (mode == bmRepair && !trusted) - throw Error("repairing is not allowed because you are not in 'trusted-users'"); - } + FIXME: layer violation in this message: the daemon code (i.e. + this file) knows whether a client/connection is trusted, but it + does not how how the client was authenticated. The mechanism + need not be getting the UID of the other end of a Unix Domain + Socket. + */ + if (mode == bmRepair && !trusted) + throw Error("repairing is not allowed because you are not in 'trusted-users'"); logger->startWork(); store->buildPaths(drvs, mode); logger->stopWork(); @@ -746,13 +740,11 @@ static void performOp(TunnelLogger * logger, ref store, clientSettings.buildCores = readInt(from); clientSettings.useSubstitutes = readInt(from); - if (GET_PROTOCOL_MINOR(clientVersion) >= 12) { - unsigned int n = readInt(from); - for (unsigned int i = 0; i < n; i++) { - auto name = readString(from); - auto value = readString(from); - clientSettings.overrides.emplace(name, value); - } + unsigned int n = readInt(from); + for (unsigned int i = 0; i < n; i++) { + auto name = readString(from); + auto value = readString(from); + clientSettings.overrides.emplace(name, value); } logger->startWork(); @@ -822,15 +814,14 @@ static void performOp(TunnelLogger * logger, ref store, try { info = store->queryPathInfo(path); } catch (InvalidPath &) { - if (GET_PROTOCOL_MINOR(clientVersion) < 17) throw; + // The path being invalid isn't fatal here since it will just be + // sent as not present. } logger->stopWork(); if (info) { - if (GET_PROTOCOL_MINOR(clientVersion) >= 17) - to << 1; + to << 1; WorkerProto::write(*store, wconn, static_cast(*info)); } else { - assert(GET_PROTOCOL_MINOR(clientVersion) >= 17); to << 0; } break; @@ -904,12 +895,7 @@ static void performOp(TunnelLogger * logger, ref store, else { std::unique_ptr source; StringSink saved; - if (GET_PROTOCOL_MINOR(clientVersion) >= 21) - source = std::make_unique(from, to); - else { - copyNAR(from, saved); - source = std::make_unique(saved.s); - } + source = std::make_unique(from, to); logger->startWork(); @@ -1011,7 +997,7 @@ void processConnection( to.flush(); WorkerProto::Version clientVersion = readInt(from); - if (clientVersion < 0x10a) + if (clientVersion < MIN_SUPPORTED_WORKER_PROTO_VERSION) throw Error("the Nix client version is too old"); auto tunnelLogger = new TunnelLogger(to, clientVersion); @@ -1027,13 +1013,13 @@ void processConnection( printMsgUsing(prevLogger, lvlDebug, "%d operations", opCount); }); - if (GET_PROTOCOL_MINOR(clientVersion) >= 14 && readInt(from)) { + // FIXME: what is *supposed* to be in this even? + if (readInt(from)) { // Obsolete CPU affinity. readInt(from); } - if (GET_PROTOCOL_MINOR(clientVersion) >= 11) - readInt(from); // obsolete reserveSpace + readInt(from); // obsolete reserveSpace if (GET_PROTOCOL_MINOR(clientVersion) >= 33) to << nixVersion; diff --git a/src/libstore/remote-store-connection.hh b/src/libstore/remote-store-connection.hh index 44328b06b..082874cec 100644 --- a/src/libstore/remote-store-connection.hh +++ b/src/libstore/remote-store-connection.hh @@ -27,11 +27,8 @@ struct RemoteStore::Connection FdSource from; /** - * Worker protocol version used for the connection. - * - * Despite its name, I think it is actually the maximum version both - * sides support. (If the maximum doesn't exist, we would fail to - * establish a connection and produce a value of this type.) + * The worker protocol version of the connected daemon. This may be newer + * than this Lix supports. */ WorkerProto::Version daemonVersion; From 985ce8a865e39857e5c582588ccf5eed90fd0fc8 Mon Sep 17 00:00:00 2001 From: Jade Lovelace Date: Fri, 24 May 2024 20:45:05 -0600 Subject: [PATCH 4/6] libstore client: remove support for <2.3 clients Change-Id: I71c2e8ca644b6187e0084f35e82f3316c9d425b0 --- src/libstore/remote-store.cc | 158 ++++++++++++----------------------- 1 file changed, 55 insertions(+), 103 deletions(-) diff --git a/src/libstore/remote-store.cc b/src/libstore/remote-store.cc index 966dd3fe0..82181b921 100644 --- a/src/libstore/remote-store.cc +++ b/src/libstore/remote-store.cc @@ -75,17 +75,14 @@ void RemoteStore::initConnection(Connection & conn) conn.from >> conn.daemonVersion; if (GET_PROTOCOL_MAJOR(conn.daemonVersion) != GET_PROTOCOL_MAJOR(PROTOCOL_VERSION)) throw Error("Nix daemon protocol version not supported"); - if (GET_PROTOCOL_MINOR(conn.daemonVersion) < 10) + if (GET_PROTOCOL_MINOR(conn.daemonVersion) < MIN_SUPPORTED_MINOR_WORKER_PROTO_VERSION) throw Error("the Nix daemon version is too old"); conn.to << PROTOCOL_VERSION; - if (GET_PROTOCOL_MINOR(conn.daemonVersion) >= 14) { - // Obsolete CPU affinity. - conn.to << 0; - } + // Obsolete CPU affinity. + conn.to << 0; - if (GET_PROTOCOL_MINOR(conn.daemonVersion) >= 11) - conn.to << false; // obsolete reserveSpace + conn.to << false; // obsolete reserveSpace if (GET_PROTOCOL_MINOR(conn.daemonVersion) >= 33) { conn.to.flush(); @@ -126,24 +123,22 @@ void RemoteStore::setOptions(Connection & conn) << settings.buildCores << settings.useSubstitutes; - if (GET_PROTOCOL_MINOR(conn.daemonVersion) >= 12) { - std::map overrides; - settings.getSettings(overrides, true); // libstore settings - fileTransferSettings.getSettings(overrides, true); - overrides.erase(settings.keepFailed.name); - overrides.erase(settings.keepGoing.name); - overrides.erase(settings.tryFallback.name); - overrides.erase(settings.maxBuildJobs.name); - overrides.erase(settings.maxSilentTime.name); - overrides.erase(settings.buildCores.name); - overrides.erase(settings.useSubstitutes.name); - overrides.erase(loggerSettings.showTrace.name); - overrides.erase(experimentalFeatureSettings.experimentalFeatures.name); - overrides.erase(settings.pluginFiles.name); - conn.to << overrides.size(); - for (auto & i : overrides) - conn.to << i.first << i.second.value; - } + std::map overrides; + settings.getSettings(overrides, true); // libstore settings + fileTransferSettings.getSettings(overrides, true); + overrides.erase(settings.keepFailed.name); + overrides.erase(settings.keepGoing.name); + overrides.erase(settings.tryFallback.name); + overrides.erase(settings.maxBuildJobs.name); + overrides.erase(settings.maxSilentTime.name); + overrides.erase(settings.buildCores.name); + overrides.erase(settings.useSubstitutes.name); + overrides.erase(loggerSettings.showTrace.name); + overrides.erase(experimentalFeatureSettings.experimentalFeatures.name); + overrides.erase(settings.pluginFiles.name); + conn.to << overrides.size(); + for (auto & i : overrides) + conn.to << i.first << i.second.value; auto ex = conn.processStderr(); if (ex) std::rethrow_exception(ex); @@ -207,20 +202,13 @@ bool RemoteStore::isValidPathUncached(const StorePath & path) StorePathSet RemoteStore::queryValidPaths(const StorePathSet & paths, SubstituteFlag maybeSubstitute) { auto conn(getConnection()); - if (GET_PROTOCOL_MINOR(conn->daemonVersion) < 12) { - StorePathSet res; - for (auto & i : paths) - if (isValidPath(i)) res.insert(i); - return res; - } else { - conn->to << WorkerProto::Op::QueryValidPaths; - WorkerProto::write(*this, *conn, paths); - if (GET_PROTOCOL_MINOR(conn->daemonVersion) >= 27) { - conn->to << maybeSubstitute; - } - conn.processStderr(); - return WorkerProto::Serialise::read(*this, *conn); + conn->to << WorkerProto::Op::QueryValidPaths; + WorkerProto::write(*this, *conn, paths); + if (GET_PROTOCOL_MINOR(conn->daemonVersion) >= 27) { + conn->to << maybeSubstitute; } + conn.processStderr(); + return WorkerProto::Serialise::read(*this, *conn); } @@ -236,20 +224,10 @@ StorePathSet RemoteStore::queryAllValidPaths() StorePathSet RemoteStore::querySubstitutablePaths(const StorePathSet & paths) { auto conn(getConnection()); - if (GET_PROTOCOL_MINOR(conn->daemonVersion) < 12) { - StorePathSet res; - for (auto & i : paths) { - conn->to << WorkerProto::Op::HasSubstitutes << printStorePath(i); - conn.processStderr(); - if (readInt(conn->from)) res.insert(i); - } - return res; - } else { - conn->to << WorkerProto::Op::QuerySubstitutablePaths; - WorkerProto::write(*this, *conn, paths); - conn.processStderr(); - return WorkerProto::Serialise::read(*this, *conn); - } + conn->to << WorkerProto::Op::QuerySubstitutablePaths; + WorkerProto::write(*this, *conn, paths); + conn.processStderr(); + return WorkerProto::Serialise::read(*this, *conn); } @@ -259,45 +237,25 @@ void RemoteStore::querySubstitutablePathInfos(const StorePathCAMap & pathsMap, S auto conn(getConnection()); - if (GET_PROTOCOL_MINOR(conn->daemonVersion) < 12) { - - for (auto & i : pathsMap) { - SubstitutablePathInfo info; - conn->to << WorkerProto::Op::QuerySubstitutablePathInfo << printStorePath(i.first); - conn.processStderr(); - unsigned int reply = readInt(conn->from); - if (reply == 0) continue; - auto deriver = readString(conn->from); - if (deriver != "") - info.deriver = parseStorePath(deriver); - info.references = WorkerProto::Serialise::read(*this, *conn); - info.downloadSize = readLongLong(conn->from); - info.narSize = readLongLong(conn->from); - infos.insert_or_assign(i.first, std::move(info)); - } - - } else { - - conn->to << WorkerProto::Op::QuerySubstitutablePathInfos; - if (GET_PROTOCOL_MINOR(conn->daemonVersion) < 22) { - StorePathSet paths; - for (auto & path : pathsMap) - paths.insert(path.first); - WorkerProto::write(*this, *conn, paths); - } else - WorkerProto::write(*this, *conn, pathsMap); - conn.processStderr(); - size_t count = readNum(conn->from); - for (size_t n = 0; n < count; n++) { - SubstitutablePathInfo & info(infos[parseStorePath(readString(conn->from))]); - auto deriver = readString(conn->from); - if (deriver != "") - info.deriver = parseStorePath(deriver); - info.references = WorkerProto::Serialise::read(*this, *conn); - info.downloadSize = readLongLong(conn->from); - info.narSize = readLongLong(conn->from); - } + conn->to << WorkerProto::Op::QuerySubstitutablePathInfos; + if (GET_PROTOCOL_MINOR(conn->daemonVersion) < 22) { + StorePathSet paths; + for (auto & path : pathsMap) + paths.insert(path.first); + WorkerProto::write(*this, *conn, paths); + } else + WorkerProto::write(*this, *conn, pathsMap); + conn.processStderr(); + size_t count = readNum(conn->from); + for (size_t n = 0; n < count; n++) { + SubstitutablePathInfo & info(infos[parseStorePath(readString(conn->from))]); + auto deriver = readString(conn->from); + if (deriver != "") + info.deriver = parseStorePath(deriver); + info.references = WorkerProto::Serialise::read(*this, *conn); + info.downloadSize = readLongLong(conn->from); + info.narSize = readLongLong(conn->from); } } @@ -314,10 +272,10 @@ std::shared_ptr RemoteStore::queryPathInfoUncached(const St return nullptr; throw; } - if (GET_PROTOCOL_MINOR(conn->daemonVersion) >= 17) { - bool valid; conn->from >> valid; - if (!valid) return nullptr; - } + + bool valid; conn->from >> valid; + if (!valid) return nullptr; + return std::make_shared( StorePath{path}, WorkerProto::Serialise::read(*this, *conn)); @@ -346,7 +304,7 @@ StorePathSet RemoteStore::queryValidDerivers(const StorePath & path) StorePathSet RemoteStore::queryDerivationOutputs(const StorePath & path) { - if (GET_PROTOCOL_MINOR(getProtocol()) >= 0x16) { + if (GET_PROTOCOL_MINOR(getProtocol()) >= 22) { return Store::queryDerivationOutputs(path); } auto conn(getConnection()); @@ -358,7 +316,7 @@ StorePathSet RemoteStore::queryDerivationOutputs(const StorePath & path) std::map> RemoteStore::queryPartialDerivationOutputMap(const StorePath & path, Store * evalStore_) { - if (GET_PROTOCOL_MINOR(getProtocol()) >= 0x16) { + if (GET_PROTOCOL_MINOR(getProtocol()) >= 22) { if (!evalStore_) { auto conn(getConnection()); conn->to << WorkerProto::Op::QueryDerivationOutputMap << printStorePath(path); @@ -669,13 +627,7 @@ void RemoteStore::buildPaths(const std::vector & drvPaths, BuildMod conn->to << WorkerProto::Op::BuildPaths; assert(GET_PROTOCOL_MINOR(conn->daemonVersion) >= 13); WorkerProto::write(*this, *conn, drvPaths); - if (GET_PROTOCOL_MINOR(conn->daemonVersion) >= 15) - conn->to << buildMode; - else - /* Old daemons did not take a 'buildMode' parameter, so we - need to validate it here on the client side. */ - if (buildMode != bmNormal) - throw Error("repairing or checking is not supported when building through the Nix daemon"); + conn->to << buildMode; conn.processStderr(); readInt(conn->from); } From c22a7f50cb4a476f0c276d8e0d2194da5144ce92 Mon Sep 17 00:00:00 2001 From: Jade Lovelace Date: Fri, 24 May 2024 20:45:05 -0600 Subject: [PATCH 5/6] libstore: refuse to serialise ancient protocols We don't want to deal with these at all, let's stop doing so. (marking this one as the fix commit since its immediate predecessors aren't the complete fix) Fixes: https://git.lix.systems/lix-project/lix/issues/325 Change-Id: Ieea1b0b8ac0f903d1e24e5b3e63cfe12eeec119d --- src/libstore/daemon.cc | 15 +++------------ src/libstore/remote-store-connection.hh | 10 ++-------- src/libstore/remote-store.cc | 9 ++++----- src/libstore/store-api.cc | 11 +++++------ src/libstore/worker-protocol.hh | 8 ++++++++ tests/unit/libstore/protocol.hh | 11 +++-------- 6 files changed, 25 insertions(+), 39 deletions(-) diff --git a/src/libstore/daemon.cc b/src/libstore/daemon.cc index e256ad1de..c89b5f5b3 100644 --- a/src/libstore/daemon.cc +++ b/src/libstore/daemon.cc @@ -264,14 +264,8 @@ static void performOp(TunnelLogger * logger, ref store, TrustedFlag trusted, RecursiveFlag recursive, WorkerProto::Version clientVersion, Source & from, BufferedSink & to, WorkerProto::Op op) { - WorkerProto::ReadConn rconn { - .from = from, - .version = clientVersion, - }; - WorkerProto::WriteConn wconn { - .to = to, - .version = clientVersion, - }; + WorkerProto::ReadConn rconn{from, clientVersion}; + WorkerProto::WriteConn wconn{to, clientVersion}; switch (op) { @@ -1030,10 +1024,7 @@ void processConnection( auto temp = trusted ? store->isTrustedClient() : std::optional { NotTrusted }; - WorkerProto::WriteConn wconn { - .to = to, - .version = clientVersion, - }; + WorkerProto::WriteConn wconn {to, clientVersion}; WorkerProto::write(*store, wconn, temp); } diff --git a/src/libstore/remote-store-connection.hh b/src/libstore/remote-store-connection.hh index 082874cec..1647afd0d 100644 --- a/src/libstore/remote-store-connection.hh +++ b/src/libstore/remote-store-connection.hh @@ -68,10 +68,7 @@ struct RemoteStore::Connection */ operator WorkerProto::ReadConn () { - return WorkerProto::ReadConn { - .from = from, - .version = daemonVersion, - }; + return WorkerProto::ReadConn {from, daemonVersion}; } /** @@ -84,10 +81,7 @@ struct RemoteStore::Connection */ operator WorkerProto::WriteConn () { - return WorkerProto::WriteConn { - .to = to, - .version = daemonVersion, - }; + return WorkerProto::WriteConn {to, daemonVersion}; } virtual ~Connection(); diff --git a/src/libstore/remote-store.cc b/src/libstore/remote-store.cc index 82181b921..d445cbcb6 100644 --- a/src/libstore/remote-store.cc +++ b/src/libstore/remote-store.cc @@ -466,7 +466,7 @@ void RemoteStore::addToStore(const ValidPathInfo & info, Source & source, sink << exportMagic << printStorePath(info.path); - WorkerProto::WriteConn nested { .to = sink, .version = conn->daemonVersion }; + WorkerProto::WriteConn nested { sink, conn->daemonVersion }; WorkerProto::write(*this, nested, info.references); sink << (info.deriver ? printStorePath(*info.deriver) : "") @@ -511,14 +511,13 @@ void RemoteStore::addMultipleToStore( RepairFlag repair, CheckSigsFlag checkSigs) { + auto remoteVersion = getProtocol(); + auto source = sinkToSource([&](Sink & sink) { sink << pathsToCopy.size(); for (auto & [pathInfo, pathSource] : pathsToCopy) { WorkerProto::Serialise::write(*this, - WorkerProto::WriteConn { - .to = sink, - .version = 16, - }, + WorkerProto::WriteConn {sink, remoteVersion}, pathInfo); pathSource->drainInto(sink); } diff --git a/src/libstore/store-api.cc b/src/libstore/store-api.cc index e0e842060..a10e5df0a 100644 --- a/src/libstore/store-api.cc +++ b/src/libstore/store-api.cc @@ -368,15 +368,14 @@ void Store::addMultipleToStore( RepairFlag repair, CheckSigsFlag checkSigs) { + auto remoteVersion = getProtocol(); + auto expected = readNum(source); for (uint64_t i = 0; i < expected; ++i) { - // FIXME we should not be using the worker protocol here, let - // alone the worker protocol with a hard-coded version! + // FIXME we should not be using the worker protocol here at all! auto info = WorkerProto::Serialise::read(*this, - WorkerProto::ReadConn { - .from = source, - .version = 16, - }); + WorkerProto::ReadConn {source, remoteVersion} + ); info.ultimate = false; addToStore(info, source, repair, checkSigs); } diff --git a/src/libstore/worker-protocol.hh b/src/libstore/worker-protocol.hh index cc083a551..5f24c189f 100644 --- a/src/libstore/worker-protocol.hh +++ b/src/libstore/worker-protocol.hh @@ -72,6 +72,10 @@ struct WorkerProto struct ReadConn { Source & from; Version version; + + ReadConn(Source & from, Version version) : from(from), version(version) { + assert(version >= MIN_SUPPORTED_WORKER_PROTO_VERSION); + } }; /** @@ -81,6 +85,10 @@ struct WorkerProto struct WriteConn { Sink & to; Version version; + + WriteConn(Sink & to, Version version) : to(to), version(version) { + assert(version >= MIN_SUPPORTED_WORKER_PROTO_VERSION); + } }; /** diff --git a/tests/unit/libstore/protocol.hh b/tests/unit/libstore/protocol.hh index 7fdd3e11c..f480f4ad1 100644 --- a/tests/unit/libstore/protocol.hh +++ b/tests/unit/libstore/protocol.hh @@ -39,10 +39,8 @@ public: StringSource from { expected }; Proto::template Serialise::read( *LibStoreTest::store, - typename Proto::ReadConn { - .from = from, - .version = version, - }); + typename Proto::ReadConn {from, version} + ); }); ASSERT_EQ(got, value); @@ -60,10 +58,7 @@ public: StringSink to; Proto::write( *LibStoreTest::store, - typename Proto::WriteConn { - .to = to, - .version = version, - }, + typename Proto::WriteConn {to, version}, value); if (testAccept()) From c1f2733dd63eb04a8a192da72835b15f9363869f Mon Sep 17 00:00:00 2001 From: Jade Lovelace Date: Fri, 24 May 2024 20:45:06 -0600 Subject: [PATCH 6/6] libstore client: remove remaining dead code Change-Id: I1764b3878439ff7b20ff64bd4efcf03070bb0e5e --- src/libstore/remote-store.cc | 82 ++++++++------------------------- src/libstore/worker-protocol.cc | 21 ++++----- 2 files changed, 30 insertions(+), 73 deletions(-) diff --git a/src/libstore/remote-store.cc b/src/libstore/remote-store.cc index d445cbcb6..575078289 100644 --- a/src/libstore/remote-store.cc +++ b/src/libstore/remote-store.cc @@ -456,51 +456,21 @@ void RemoteStore::addToStore(const ValidPathInfo & info, Source & source, { auto conn(getConnection()); - if (GET_PROTOCOL_MINOR(conn->daemonVersion) < 18) { - conn->to << WorkerProto::Op::ImportPaths; + conn->to << WorkerProto::Op::AddToStoreNar + << printStorePath(info.path) + << (info.deriver ? printStorePath(*info.deriver) : "") + << info.narHash.to_string(Base16, false); + WorkerProto::write(*this, *conn, info.references); + conn->to << info.registrationTime << info.narSize + << info.ultimate << info.sigs << renderContentAddress(info.ca) + << repair << !checkSigs; - auto source2 = sinkToSource([&](Sink & sink) { - sink << 1 // == path follows - ; + if (GET_PROTOCOL_MINOR(conn->daemonVersion) >= 23) { + conn.withFramedSink([&](Sink & sink) { copyNAR(source, sink); - sink - << exportMagic - << printStorePath(info.path); - WorkerProto::WriteConn nested { sink, conn->daemonVersion }; - WorkerProto::write(*this, nested, info.references); - sink - << (info.deriver ? printStorePath(*info.deriver) : "") - << 0 // == no legacy signature - << 0 // == no path follows - ; }); - - conn.processStderr(0, source2.get()); - - auto importedPaths = WorkerProto::Serialise::read(*this, *conn); - assert(importedPaths.size() <= 1); - } - - else { - conn->to << WorkerProto::Op::AddToStoreNar - << printStorePath(info.path) - << (info.deriver ? printStorePath(*info.deriver) : "") - << info.narHash.to_string(Base16, false); - WorkerProto::write(*this, *conn, info.references); - conn->to << info.registrationTime << info.narSize - << info.ultimate << info.sigs << renderContentAddress(info.ca) - << repair << !checkSigs; - - if (GET_PROTOCOL_MINOR(conn->daemonVersion) >= 23) { - conn.withFramedSink([&](Sink & sink) { - copyNAR(source, sink); - }); - } else if (GET_PROTOCOL_MINOR(conn->daemonVersion) >= 21) { - conn.processStderr(0, &source); - } else { - copyNAR(source, conn->to); - conn.processStderr(0, nullptr); - } + } else { + conn.processStderr(0, &source); } } @@ -624,7 +594,6 @@ void RemoteStore::buildPaths(const std::vector & drvPaths, BuildMod auto conn(getConnection()); conn->to << WorkerProto::Op::BuildPaths; - assert(GET_PROTOCOL_MINOR(conn->daemonVersion) >= 13); WorkerProto::write(*this, *conn, drvPaths); conn->to << buildMode; conn.processStderr(); @@ -817,25 +786,14 @@ void RemoteStore::queryMissing(const std::vector & targets, StorePathSet & willBuild, StorePathSet & willSubstitute, StorePathSet & unknown, uint64_t & downloadSize, uint64_t & narSize) { - { - auto conn(getConnection()); - if (GET_PROTOCOL_MINOR(conn->daemonVersion) < 19) - // Don't hold the connection handle in the fallback case - // to prevent a deadlock. - goto fallback; - conn->to << WorkerProto::Op::QueryMissing; - WorkerProto::write(*this, *conn, targets); - conn.processStderr(); - willBuild = WorkerProto::Serialise::read(*this, *conn); - willSubstitute = WorkerProto::Serialise::read(*this, *conn); - unknown = WorkerProto::Serialise::read(*this, *conn); - conn->from >> downloadSize >> narSize; - return; - } - - fallback: - return Store::queryMissing(targets, willBuild, willSubstitute, - unknown, downloadSize, narSize); + auto conn(getConnection()); + conn->to << WorkerProto::Op::QueryMissing; + WorkerProto::write(*this, *conn, targets); + conn.processStderr(); + willBuild = WorkerProto::Serialise::read(*this, *conn); + willSubstitute = WorkerProto::Serialise::read(*this, *conn); + unknown = WorkerProto::Serialise::read(*this, *conn); + conn->from >> downloadSize >> narSize; } diff --git a/src/libstore/worker-protocol.cc b/src/libstore/worker-protocol.cc index d4954aaa8..08c5c6b70 100644 --- a/src/libstore/worker-protocol.cc +++ b/src/libstore/worker-protocol.cc @@ -165,11 +165,11 @@ UnkeyedValidPathInfo WorkerProto::Serialise::read(const St if (deriver != "") info.deriver = store.parseStorePath(deriver); info.references = WorkerProto::Serialise::read(store, conn); conn.from >> info.registrationTime >> info.narSize; - if (GET_PROTOCOL_MINOR(conn.version) >= 16) { - conn.from >> info.ultimate; - info.sigs = readStrings(conn.from); - info.ca = ContentAddress::parseOpt(readString(conn.from)); - } + + conn.from >> info.ultimate; + info.sigs = readStrings(conn.from); + info.ca = ContentAddress::parseOpt(readString(conn.from)); + return info; } @@ -180,12 +180,11 @@ void WorkerProto::Serialise::write(const Store & store, Wr << pathInfo.narHash.to_string(Base16, false); WorkerProto::write(store, conn, pathInfo.references); conn.to << pathInfo.registrationTime << pathInfo.narSize; - if (GET_PROTOCOL_MINOR(conn.version) >= 16) { - conn.to - << pathInfo.ultimate - << pathInfo.sigs - << renderContentAddress(pathInfo.ca); - } + + conn.to + << pathInfo.ultimate + << pathInfo.sigs + << renderContentAddress(pathInfo.ca); } }