Merge pull request #14677 from NixOS/restartable-source-no-path

libutil: Get rid of restartableSourceFromFactory, add createAnonymousTempFile

src/libstore/binary-cache-store.cc

@@ -79,8 +79,8 @@ std::optional<std::string> BinaryCacheStore::getNixCacheInfo()
 void BinaryCacheStore::upsertFile(
     const std::string & path, std::string && data, const std::string & mimeType, uint64_t sizeHint)
 {
-    auto source = restartableSourceFromFactory([data = std::move(data)]() { return make_unique<StringSource>(data); });
-    upsertFile(path, *source, mimeType, sizeHint);
+    StringSource source{data};
+    upsertFile(path, source, mimeType, sizeHint);
 }
 
 void BinaryCacheStore::getFile(const std::string & path, Callback<std::optional<std::string>> callback) noexcept
@@ -140,9 +140,7 @@ void BinaryCacheStore::writeNarInfo(ref<NarInfo> narInfo)
 ref<const ValidPathInfo> BinaryCacheStore::addToStoreCommon(
     Source & narSource, RepairFlag repair, CheckSigsFlag checkSigs, std::function<ValidPathInfo(HashResult)> mkInfo)
 {
-    auto [fdTemp, fnTemp] = createTempFile();
-
-    AutoDelete autoDelete(fnTemp);
+    auto fdTemp = createAnonymousTempFile();
 
     auto now1 = std::chrono::steady_clock::now();
 
@@ -272,19 +270,10 @@ ref<const ValidPathInfo> BinaryCacheStore::addToStoreCommon(
 
     /* Atomically write the NAR file. */
     if (repair || !fileExists(narInfo->url)) {
-        auto source = restartableSourceFromFactory([fnTemp]() {
-            struct AutoCloseFDSource : AutoCloseFD, FdSource
-            {
-                AutoCloseFDSource(AutoCloseFD fd)
-                    : AutoCloseFD(std::move(fd))
-                    , FdSource(get())
-                {
-                }
-            };
-            return std::make_unique<AutoCloseFDSource>(toDescriptor(open(fnTemp.c_str(), O_RDONLY)));
-        });
+        FdSource source{fdTemp.get()};
+        source.restart(); /* Seek back to the start of the file. */
         stats.narWrite++;
-        upsertFile(narInfo->url, *source, "application/x-nix-nar", narInfo->fileSize);
+        upsertFile(narInfo->url, source, "application/x-nix-nar", narInfo->fileSize);
     } else
         stats.narWriteAverted++;
 

src/libutil-tests/file-system.cc

@@ -381,4 +381,37 @@ TEST(openFileEnsureBeneathNoSymlinks, works)
 
 #endif
 
+/* ----------------------------------------------------------------------------
+ * createAnonymousTempFile
+ * --------------------------------------------------------------------------*/
+
+TEST(createAnonymousTempFile, works)
+{
+    auto fd = createAnonymousTempFile();
+    writeFull(fd.get(), "test");
+    lseek(fd.get(), 0, SEEK_SET);
+    FdSource source{fd.get()};
+    EXPECT_EQ(source.drain(), "test");
+    lseek(fd.get(), 0, SEEK_END);
+    writeFull(fd.get(), "test");
+    lseek(fd.get(), 0, SEEK_SET);
+    EXPECT_EQ(source.drain(), "testtest");
+}
+
+/* ----------------------------------------------------------------------------
+ * FdSource
+ * --------------------------------------------------------------------------*/
+
+TEST(FdSource, restartWorks)
+{
+    auto fd = createAnonymousTempFile();
+    writeFull(fd.get(), "hello world");
+    lseek(fd.get(), 0, SEEK_SET);
+    FdSource source{fd.get()};
+    EXPECT_EQ(source.drain(), "hello world");
+    source.restart();
+    EXPECT_EQ(source.drain(), "hello world");
+    EXPECT_EQ(source.drain(), "");
+}
+
 } // namespace nix

src/libutil/file-system.cc

@@ -713,11 +713,31 @@ std::filesystem::path createTempDir(const std::filesystem::path & tmpRoot, const
     }
 }
 
+AutoCloseFD createAnonymousTempFile()
+{
+    AutoCloseFD fd;
+#ifdef O_TMPFILE
+    fd = ::open(defaultTempDir().c_str(), O_TMPFILE | O_CLOEXEC | O_RDWR, S_IWUSR | S_IRUSR);
+    if (!fd)
+        throw SysError("creating anonymous temporary file");
+#else
+    auto [fd2, path] = createTempFile("nix-anonymous");
+    if (!fd2)
+        throw SysError("creating temporary file '%s'", path);
+    fd = std::move(fd2);
+#  ifndef _WIN32
+    unlink(requireCString(path)); /* We only care about the file descriptor. */
+#  endif
+#endif
+    return fd;
+}
+
 std::pair<AutoCloseFD, Path> createTempFile(const Path & prefix)
 {
     Path tmpl(defaultTempDir() + "/" + prefix + ".XXXXXX");
     // Strictly speaking, this is UB, but who cares...
     // FIXME: use O_TMPFILE.
+    // FIXME: Windows should use FILE_ATTRIBUTE_TEMPORARY | FILE_FLAG_DELETE_ON_CLOSE
     AutoCloseFD fd = toDescriptor(mkstemp((char *) tmpl.c_str()));
     if (!fd)
         throw SysError("creating temporary file '%s'", tmpl);

src/libutil/include/nix/util/file-system.hh

@@ -341,6 +341,12 @@ typedef std::unique_ptr<DIR, DIRDeleter> AutoCloseDir;
 std::filesystem::path
 createTempDir(const std::filesystem::path & tmpRoot = "", const std::string & prefix = "nix", mode_t mode = 0755);
 
+/**
+ * Create an anonymous readable/writable temporary file, returning a file handle.
+ * On UNIX there resulting file isn't linked to any path on the filesystem.
+ */
+AutoCloseFD createAnonymousTempFile();
+
 /**
  * Create a temporary file, returning a file handle and its path.
  */

src/libutil/include/nix/util/serialise.hh

@@ -105,7 +105,7 @@ struct Source
  * A buffered abstract source. Warning: a BufferedSource should not be
  * used from multiple threads concurrently.
  */
-struct BufferedSource : Source
+struct BufferedSource : virtual Source
 {
     size_t bufSize, bufPosIn, bufPosOut;
     std::unique_ptr<char[]> buffer;
@@ -132,6 +132,14 @@ protected:
     virtual size_t readUnbuffered(char * data, size_t len) = 0;
 };
 
+/**
+ * Source type that can be restarted.
+ */
+struct RestartableSource : virtual Source
+{
+    virtual void restart() = 0;
+};
+
 /**
  * A sink that writes data to a file descriptor.
  */
@@ -174,7 +182,7 @@ private:
 /**
  * A source that reads data from a file descriptor.
  */
-struct FdSource : BufferedSource
+struct FdSource : BufferedSource, RestartableSource
 {
     Descriptor fd;
     size_t read = 0;
@@ -196,6 +204,7 @@ struct FdSource : BufferedSource
     FdSource & operator=(FdSource && s) = default;
 
     bool good() override;
+    void restart() override;
 
     /**
      * Return true if the buffer is not empty after a non-blocking
@@ -230,14 +239,6 @@ struct StringSink : Sink
     void operator()(std::string_view data) override;
 };
 
-/**
- * Source type that can be restarted.
- */
-struct RestartableSource : Source
-{
-    virtual void restart() = 0;
-};
-
 /**
  * A source that reads data from a string.
  */
@@ -316,15 +317,6 @@ public:
     }
 };
 
-/**
- * Create a restartable Source from a factory function.
- *
- * @param factory Factory function that returns a fresh instance of the Source. Gets
- * called for each source restart.
- * @pre factory must return an equivalent source for each invocation.
- */
-std::unique_ptr<RestartableSource> restartableSourceFromFactory(std::function<std::unique_ptr<Source>()> factory);
-
 /**
  * A sink that writes all incoming data to two other sinks.
  */

src/libutil/serialise.cc

@@ -201,6 +201,16 @@ bool FdSource::hasData()
     }
 }
 
+void FdSource::restart()
+{
+    if (!isSeekable)
+        throw Error("can't seek to the start of a file");
+    buffer.reset();
+    read = bufPosOut = bufPosOut = 0;
+    if (lseek(fd, 0, SEEK_SET) == -1)
+        throw SysError("seeking to the start of a file");
+}
+
 void FdSource::skip(size_t len)
 {
     /* Discard data in the buffer. */
@@ -527,41 +537,4 @@ size_t ChainSource::read(char * data, size_t len)
     }
 }
 
-std::unique_ptr<RestartableSource> restartableSourceFromFactory(std::function<std::unique_ptr<Source>()> factory)
-{
-    struct RestartableSourceImpl : RestartableSource
-    {
-        RestartableSourceImpl(decltype(factory) factory_)
-            : factory_(std::move(factory_))
-            , impl(this->factory_())
-        {
-        }
-
-        decltype(factory) factory_;
-        std::unique_ptr<Source> impl = factory_();
-
-        size_t read(char * data, size_t len) override
-        {
-            return impl->read(data, len);
-        }
-
-        bool good() override
-        {
-            return impl->good();
-        }
-
-        void skip(size_t len) override
-        {
-            return impl->skip(len);
-        }
-
-        void restart() override
-        {
-            impl = factory_();
-        }
-    };
-
-    return std::make_unique<RestartableSourceImpl>(std::move(factory));
-}
-
 } // namespace nix