From 8b4e3c1c0a3d19f12b1d0fcc685792777f00653e Mon Sep 17 00:00:00 2001 From: John Ericson Date: Thu, 23 May 2024 12:14:53 -0400 Subject: [PATCH] Create `CommonSSHStoreConfig::createSSHMaster` By moving `host` to the config, we can do a lot further cleanups and dedups. This anticipates a world where we always go `StoreReference` -> `*StoreConfig` -> `Store*` rather than skipping the middle step too. Progress on #10766 Progress on https://github.com/NixOS/hydra/issues/1164 --- src/libstore/legacy-ssh-store.cc | 19 +++-------------- src/libstore/legacy-ssh-store.hh | 9 -------- src/libstore/ssh-store-config.cc | 25 +++++++++++++++++++++- src/libstore/ssh-store-config.hh | 22 +++++++++++++------ src/libstore/ssh-store.cc | 36 ++++++++++++++------------------ src/libstore/ssh.cc | 2 +- src/libstore/ssh.hh | 4 ++-- 7 files changed, 62 insertions(+), 55 deletions(-) diff --git a/src/libstore/legacy-ssh-store.cc b/src/libstore/legacy-ssh-store.cc index c75d50ade89e..9db5ce5325eb 100644 --- a/src/libstore/legacy-ssh-store.cc +++ b/src/libstore/legacy-ssh-store.cc @@ -32,32 +32,19 @@ LegacySSHStore::LegacySSHStore( std::string_view scheme, std::string_view host, const Params & params) - : LegacySSHStore{scheme, LegacySSHStoreConfig::extractConnStr(scheme, host), params} -{ -} - -LegacySSHStore::LegacySSHStore( - std::string_view scheme, - std::string host, - const Params & params) : StoreConfig(params) - , CommonSSHStoreConfig(params) + , CommonSSHStoreConfig(scheme, host, params) , LegacySSHStoreConfig(params) , Store(params) - , host(host) , connections(make_ref>( std::max(1, (int) maxConnections), [this]() { return openConnection(); }, [](const ref & r) { return r->good; } )) - , master( - host, - sshKey.get(), - sshPublicHostKey.get(), + , master(createSSHMaster( // Use SSH master only if using more than 1 connection. connections->capacity() > 1, - compress, - logFD) + logFD)) { } diff --git a/src/libstore/legacy-ssh-store.hh b/src/libstore/legacy-ssh-store.hh index 81872996a4c6..6c3c9508034b 100644 --- a/src/libstore/legacy-ssh-store.hh +++ b/src/libstore/legacy-ssh-store.hh @@ -33,8 +33,6 @@ struct LegacySSHStore : public virtual LegacySSHStoreConfig, public virtual Stor struct Connection; - std::string host; - ref> connections; SSHMaster master; @@ -46,13 +44,6 @@ struct LegacySSHStore : public virtual LegacySSHStoreConfig, public virtual Stor std::string_view host, const Params & params); -private: - LegacySSHStore( - std::string_view scheme, - std::string host, - const Params & params); -public: - ref openConnection(); std::string getUri() override; diff --git a/src/libstore/ssh-store-config.cc b/src/libstore/ssh-store-config.cc index 742354cce6ad..782155a087be 100644 --- a/src/libstore/ssh-store-config.cc +++ b/src/libstore/ssh-store-config.cc @@ -1,10 +1,11 @@ #include #include "ssh-store-config.hh" +#include "ssh.hh" namespace nix { -std::string CommonSSHStoreConfig::extractConnStr(std::string_view scheme, std::string_view _connStr) +static std::string extractConnStr(std::string_view scheme, std::string_view _connStr) { if (_connStr.empty()) throw UsageError("`%s` store requires a valid SSH host as the authority part in Store URI", scheme); @@ -21,4 +22,26 @@ std::string CommonSSHStoreConfig::extractConnStr(std::string_view scheme, std::s return connStr; } +CommonSSHStoreConfig::CommonSSHStoreConfig( + std::string_view scheme, + std::string_view host, + const Params & params) + : StoreConfig(params) + , host(extractConnStr(scheme, host)) +{ +} + +SSHMaster CommonSSHStoreConfig::createSSHMaster( + bool useMaster, Descriptor logFD) +{ + return { + host, + sshKey.get(), + sshPublicHostKey.get(), + useMaster, + compress, + logFD, + }; +} + } diff --git a/src/libstore/ssh-store-config.hh b/src/libstore/ssh-store-config.hh index 09f9f23a7825..5deb6f4c9e99 100644 --- a/src/libstore/ssh-store-config.hh +++ b/src/libstore/ssh-store-config.hh @@ -5,10 +5,14 @@ namespace nix { +class SSHMaster; + struct CommonSSHStoreConfig : virtual StoreConfig { using StoreConfig::StoreConfig; + CommonSSHStoreConfig(std::string_view scheme, std::string_view host, const Params & params); + const Setting sshKey{this, "", "ssh-key", "Path to the SSH private key used to authenticate to the remote machine."}; @@ -30,9 +34,7 @@ struct CommonSSHStoreConfig : virtual StoreConfig * RFC2732, but also pure addresses. The latter one is needed here to * connect to a remote store via SSH (it's possible to do e.g. `ssh root@::1`). * - * This function now ensures that a usable connection string is available: - * - * - If the store to be opened is not an SSH store, nothing will be done. + * When initialized, the following adjustments are made: * * - If the URL looks like `root@[::1]` (which is allowed by the URL parser and probably * needed to pass further flags), it @@ -44,9 +46,17 @@ struct CommonSSHStoreConfig : virtual StoreConfig * * Will throw an error if `connStr` is empty too. */ - static std::string extractConnStr( - std::string_view scheme, - std::string_view connStr); + std::string host; + + /** + * Small wrapper around `SSHMaster::SSHMaster` that gets most + * arguments from this configuration. + * + * See that constructor for details on the remaining two arguments. + */ + SSHMaster createSSHMaster( + bool useMaster, + Descriptor logFD = INVALID_DESCRIPTOR); }; } diff --git a/src/libstore/ssh-store.cc b/src/libstore/ssh-store.cc index 246abaac2c6a..7ad934b738b7 100644 --- a/src/libstore/ssh-store.cc +++ b/src/libstore/ssh-store.cc @@ -32,34 +32,21 @@ struct SSHStoreConfig : virtual RemoteStoreConfig, virtual CommonSSHStoreConfig class SSHStore : public virtual SSHStoreConfig, public virtual RemoteStore { +public: + SSHStore( std::string_view scheme, - std::string host, + std::string_view host, const Params & params) : StoreConfig(params) , RemoteStoreConfig(params) - , CommonSSHStoreConfig(params) + , CommonSSHStoreConfig(scheme, host, params) , SSHStoreConfig(params) , Store(params) , RemoteStore(params) - , host(host) - , master( - host, - sshKey.get(), - sshPublicHostKey.get(), + , master(createSSHMaster( // Use SSH master only if using more than 1 connection. - connections->capacity() > 1, - compress) - { - } - -public: - - SSHStore( - std::string_view scheme, - std::string_view host, - const Params & params) - : SSHStore{scheme, SSHStoreConfig::extractConnStr(scheme, host), params} + connections->capacity() > 1)) { } @@ -119,6 +106,15 @@ struct MountedSSHStoreConfig : virtual SSHStoreConfig, virtual LocalFSStoreConfi { } + MountedSSHStoreConfig(std::string_view scheme, std::string_view host, StringMap params) + : StoreConfig(params) + , RemoteStoreConfig(params) + , CommonSSHStoreConfig(scheme, host, params) + , SSHStoreConfig(params) + , LocalFSStoreConfig(params) + { + } + const std::string name() override { return "Experimental SSH Store with filesystem mounted"; } std::string doc() override @@ -158,7 +154,7 @@ class MountedSSHStore : public virtual MountedSSHStoreConfig, public virtual SSH const Params & params) : StoreConfig(params) , RemoteStoreConfig(params) - , CommonSSHStoreConfig(params) + , CommonSSHStoreConfig(scheme, host, params) , SSHStoreConfig(params) , LocalFSStoreConfig(params) , MountedSSHStoreConfig(params) diff --git a/src/libstore/ssh.cc b/src/libstore/ssh.cc index 5eb63fa67a5c..e5d623adf3ac 100644 --- a/src/libstore/ssh.cc +++ b/src/libstore/ssh.cc @@ -10,7 +10,7 @@ SSHMaster::SSHMaster( std::string_view host, std::string_view keyFile, std::string_view sshPublicHostKey, - bool useMaster, bool compress, int logFD) + bool useMaster, bool compress, Descriptor logFD) : host(host) , fakeSSH(host == "localhost") , keyFile(keyFile) diff --git a/src/libstore/ssh.hh b/src/libstore/ssh.hh index be0a3228770e..19b30e8838fb 100644 --- a/src/libstore/ssh.hh +++ b/src/libstore/ssh.hh @@ -17,7 +17,7 @@ private: const std::string sshPublicHostKey; const bool useMaster; const bool compress; - const int logFD; + const Descriptor logFD; struct State { @@ -43,7 +43,7 @@ public: std::string_view host, std::string_view keyFile, std::string_view sshPublicHostKey, - bool useMaster, bool compress, int logFD = -1); + bool useMaster, bool compress, Descriptor logFD = INVALID_DESCRIPTOR); struct Connection {