Refactor unix domain socket store config

src/libstore/uds-remote-store.cc

@@ -2,10 +2,8 @@
 #include "unix-domain-socket.hh"
 #include "worker-protocol.hh"
 
-#include <cstring>
 #include <sys/types.h>
 #include <sys/stat.h>
-#include <errno.h>
 #include <fcntl.h>
 #include <unistd.h>
 
@@ -19,6 +17,24 @@
 
 namespace nix {
 
+UDSRemoteStoreConfig::UDSRemoteStoreConfig(
+    std::string_view scheme,
+    std::string_view authority,
+    const Params & params)
+    : StoreConfig(params)
+    , LocalFSStoreConfig(params)
+    , RemoteStoreConfig(params)
+{
+    if (scheme != UDSRemoteStoreConfig::scheme) {
+        throw UsageError("Scheme must be 'unix'");
+    }
+
+    if (!authority.empty()) {
+        path.emplace(authority);
+    }
+}
+
+
 std::string UDSRemoteStoreConfig::doc()
 {
     return
@@ -27,34 +43,40 @@ std::string UDSRemoteStoreConfig::doc()
 }
 
 
+// A bit gross that we now pass empty string but this is knowing that
+// empty string will later default to the same nixDaemonSocketFile. Why
+// don't we just wire it all through? I believe there are cases where it
+// will live reload so we want to continue to account for that.
 UDSRemoteStore::UDSRemoteStore(const Params & params)
+    : UDSRemoteStore(scheme, "", params)
+{}
+
+
+UDSRemoteStore::UDSRemoteStore(std::string_view scheme, std::string_view authority, const Params & params)
     : StoreConfig(params)
     , LocalFSStoreConfig(params)
     , RemoteStoreConfig(params)
-    , UDSRemoteStoreConfig(params)
+    , UDSRemoteStoreConfig(scheme, authority, params)
     , Store(params)
     , LocalFSStore(params)
     , RemoteStore(params)
 {
 }
 
 
-UDSRemoteStore::UDSRemoteStore(
-    std::string_view scheme,
-    PathView socket_path,
-    const Params & params)
-    : UDSRemoteStore(params)
+std::string UDSRemoteStore::getPathOrDefault() const
 {
-    if (!socket_path.empty())
-        path.emplace(socket_path);
+    return path.value_or(settings.nixDaemonSocketFile);
 }
 
-
 std::string UDSRemoteStore::getUri()
 {
     if (path) {
-        return std::string("unix://") + *path;
+        return std::string(scheme) + "://" + *path;
     } else {
+        // FIXME: Not clear why we return daemon here and not default to
+        // settings.nixDaemonSocketFile
+        //
         // unix:// with no path also works. Change what we return?
         return "daemon";
     }
@@ -74,7 +96,7 @@ ref<RemoteStore::Connection> UDSRemoteStore::openConnection()
     /* Connect to a daemon that does the privileged work for us. */
     conn->fd = createUnixDomainSocket();
 
-    nix::connect(toSocket(conn->fd.get()), path ? *path : settings.nixDaemonSocketFile);
+    nix::connect(toSocket(conn->fd.get()), getPathOrDefault());
 
     conn->from.fd = conn->fd.get();
     conn->to.fd = conn->fd.get();

src/libstore/uds-remote-store.hh

@@ -9,16 +9,34 @@ namespace nix {
 
 struct UDSRemoteStoreConfig : virtual LocalFSStoreConfig, virtual RemoteStoreConfig
 {
-    UDSRemoteStoreConfig(const Params & params)
-        : StoreConfig(params)
-        , LocalFSStoreConfig(params)
-        , RemoteStoreConfig(params)
-    {
-    }
+    // TODO(fzakaria): Delete this constructor once moved over to the factory pattern
+    // outlined in https://github.com/NixOS/nix/issues/10766
+    using LocalFSStoreConfig::LocalFSStoreConfig;
+    using RemoteStoreConfig::RemoteStoreConfig;
+
+    /**
+     * @param authority is the socket path.
+     */
+    UDSRemoteStoreConfig(
+        std::string_view scheme,
+        std::string_view authority,
+        const Params & params);
 
     const std::string name() override { return "Local Daemon Store"; }
 
     std::string doc() override;
+
+    /**
+     * The path to the unix domain socket.
+     *
+     * The default *could be* settings.nixDaemonSocketFile but that
+     * won't pick up live changes unfortunately. This optional handling is instead
+     * handled on opening of the connection.
+     */
+    std::optional<std::string> path;
+
+protected:
+    static constexpr char const * scheme = "unix";
 };
 
 class UDSRemoteStore : public virtual UDSRemoteStoreConfig
@@ -27,16 +45,23 @@ class UDSRemoteStore : public virtual UDSRemoteStoreConfig
 {
 public:
 
+    /**
+     * @deprecated This is the old API to construct the store.
+    */
     UDSRemoteStore(const Params & params);
+
+    /**
+     * @param authority is the socket path.
+     */
     UDSRemoteStore(
         std::string_view scheme,
-        PathView path,
+        std::string_view authority,
         const Params & params);
 
     std::string getUri() override;
 
     static std::set<std::string> uriSchemes()
-    { return {"unix"}; }
+    { return {scheme}; }
 
     ref<SourceAccessor> getFSAccessor(bool requireValidPath = true) override
     { return LocalFSStore::getFSAccessor(requireValidPath); }
@@ -63,7 +88,8 @@ private:
     };
 
     ref<RemoteStore::Connection> openConnection() override;
-    std::optional<std::string> path;
+
+    std::string getPathOrDefault() const;
 };
 
 }

tests/unit/libstore/meson.build

@@ -71,6 +71,7 @@ sources = files(
   'ssh-store.cc',
   'store-reference.cc',
   'worker-protocol.cc',
+  'uds-remote-store.cc',
 )
 
 include_dirs = [include_directories('.')]

tests/unit/libstore/uds-remote-store.cc

@@ -0,0 +1,19 @@
+#include <gtest/gtest.h>
+
+#include "uds-remote-store.hh"
+
+namespace nix {
+
+TEST(UDSRemoteStore, constructConfig)
+{
+    UDSRemoteStoreConfig config{"unix", "/tmp/socket", {}};
+
+    EXPECT_EQ(*config.path, "/tmp/socket");
+}
+
+TEST(UDSRemoteStore, constructConfigWrongScheme)
+{
+    EXPECT_THROW(UDSRemoteStoreConfig("http", "/tmp/socket", {}), UsageError);
+}
+
+} // namespace nix