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 NixOS/hydra#1164
NixOS · May 27, 2024 · 3e9c373 · 3e9c373
1 parent 1796444
commit 3e9c373
Show file tree

Hide file tree

Showing 7 changed files with 58 additions and 55 deletions.
diff --git 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<Pool<Connection>>(
         std::max(1, (int) maxConnections),
         [this]() { return openConnection(); },
         [](const ref<Connection> & 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
@@ -33,8 +33,6 @@ struct LegacySSHStore : public virtual LegacySSHStoreConfig, public virtual Stor
 
     struct Connection;
 
-    std::string host;
-
     ref<Pool<Connection>> 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<Connection> openConnection();
 
     std::string getUri() override;

diff --git a/src/libstore/ssh-store-config.cc b/src/libstore/ssh-store-config.cc
@@ -1,10 +1,11 @@
 #include <regex>
 
 #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,22 @@ 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
@@ -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<Path> 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
@@ -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
@@ -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
@@ -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
     {