From: Joel Rosdahl <joel@rosdahl.net>
Date: Fri, 4 Aug 2023 06:47:40 +0000 (+0200)
Subject: refactor: Move TemporaryFile to util
X-Git-Tag: v4.9~69
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=1da50f6f99c29637af4042d0f6697a5caf59d5e9;p=thirdparty%2Fccache.git

refactor: Move TemporaryFile to util
---

diff --git a/src/AtomicFile.cpp b/src/AtomicFile.cpp
index a3a877a83..ee64863f4 100644
--- a/src/AtomicFile.cpp
+++ b/src/AtomicFile.cpp
@@ -18,11 +18,12 @@
 
 #include "AtomicFile.hpp"
 
-#include "TemporaryFile.hpp"
 #include "assertions.hpp"
 
 #include <core/exceptions.hpp>
 #include <fmtmacros.hpp>
+#include <util/TemporaryFile.hpp>
+#include <util/expected.hpp>
 #include <util/file.hpp>
 #include <util/filesystem.hpp>
 
@@ -30,7 +31,8 @@ namespace fs = util::filesystem;
 
 AtomicFile::AtomicFile(const std::string& path, Mode mode) : m_path(path)
 {
-  TemporaryFile tmp_file(path);
+  auto tmp_file =
+    util::value_or_throw<core::Fatal>(util::TemporaryFile::create(path));
   m_stream = fdopen(tmp_file.fd.release(), mode == Mode::binary ? "w+b" : "w+");
   m_tmp_path = std::move(tmp_file.path);
 }
diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt
index a2dc35d60..a5df950ce 100644
--- a/src/CMakeLists.txt
+++ b/src/CMakeLists.txt
@@ -9,7 +9,6 @@ set(
   Hash.cpp
   Logging.cpp
   ProgressBar.cpp
-  TemporaryFile.cpp
   ThreadPool.cpp
   Util.cpp
   argprocessing.cpp
diff --git a/src/InodeCache.cpp b/src/InodeCache.cpp
index aabc583e8..8fd1be75b 100644
--- a/src/InodeCache.cpp
+++ b/src/InodeCache.cpp
@@ -22,11 +22,11 @@
 #include "Finalizer.hpp"
 #include "Hash.hpp"
 #include "Logging.hpp"
-#include "TemporaryFile.hpp"
 #include "Util.hpp"
 #include "fmtmacros.hpp"
 
 #include <util/DirEntry.hpp>
+#include <util/TemporaryFile.hpp>
 #include <util/conversion.hpp>
 #include <util/file.hpp>
 
@@ -345,15 +345,19 @@ InodeCache::create_new_file(const std::string& filename)
 {
   // Create the new file to a temporary name to prevent other processes from
   // mapping it before it is fully initialized.
-  TemporaryFile tmp_file(filename);
+  auto tmp_file = util::TemporaryFile::create(filename);
+  if (!tmp_file) {
+    LOG("Failed to created inode cache file: {}", tmp_file.error());
+    return false;
+  }
 
-  Finalizer temp_file_remover([&] { unlink(tmp_file.path.c_str()); });
+  Finalizer temp_file_remover([&] { unlink(tmp_file->path.c_str()); });
 
-  if (!fd_is_on_known_to_work_file_system(*tmp_file.fd)) {
+  if (!fd_is_on_known_to_work_file_system(*tmp_file->fd)) {
     return false;
   }
 
-  if (auto result = util::fallocate(*tmp_file.fd, sizeof(SharedRegion));
+  if (auto result = util::fallocate(*tmp_file->fd, sizeof(SharedRegion));
       !result) {
     LOG("Failed to allocate file space for inode cache: {}", result.error());
     return false;
@@ -363,7 +367,7 @@ InodeCache::create_new_file(const std::string& filename)
                                          sizeof(SharedRegion),
                                          PROT_READ | PROT_WRITE,
                                          MAP_SHARED,
-                                         *tmp_file.fd,
+                                         *tmp_file->fd,
                                          0));
   if (sr == MMAP_FAILED) {
     LOG("Failed to mmap new inode cache: {}", strerror(errno));
@@ -378,14 +382,14 @@ InodeCache::create_new_file(const std::string& filename)
   }
 
   munmap(sr, sizeof(SharedRegion));
-  tmp_file.fd.close();
+  tmp_file->fd.close();
 
   // link() will fail silently if a file with the same name already exists.
   // This will be the case if two processes try to create a new file
   // simultaneously. Thus close the current file handle and reopen a new one,
   // which will make us use the first created file even if we didn't win the
   // race.
-  if (link(tmp_file.path.c_str(), filename.c_str()) != 0) {
+  if (link(tmp_file->path.c_str(), filename.c_str()) != 0) {
     LOG("Failed to link new inode cache: {}", strerror(errno));
     return false;
   }
diff --git a/src/MiniTrace.cpp b/src/MiniTrace.cpp
index ca65a90ab..ec3914cf7 100644
--- a/src/MiniTrace.cpp
+++ b/src/MiniTrace.cpp
@@ -19,10 +19,12 @@
 #include "MiniTrace.hpp"
 
 #include "ArgsInfo.hpp"
-#include "TemporaryFile.hpp"
 #include "fmtmacros.hpp"
 
+#include <core/exceptions.hpp>
+#include <util/TemporaryFile.hpp>
 #include <util/TimePoint.hpp>
+#include <util/expected.hpp>
 #include <util/file.hpp>
 #include <util/filesystem.hpp>
 #include <util/wincompat.hpp>
@@ -41,7 +43,8 @@ MiniTrace::MiniTrace(const ArgsInfo& args_info)
   if (!tmp_dir) {
     tmp_dir = "/tmp";
   }
-  TemporaryFile tmp_file(*tmp_dir / "ccache-trace");
+  auto tmp_file = util::value_or_throw<core::Fatal>(
+    util::TemporaryFile::create(*tmp_dir / "ccache-trace"));
   m_tmp_trace_file = tmp_file.path;
 
   mtr_init(m_tmp_trace_file.c_str());
diff --git a/src/Util.cpp b/src/Util.cpp
index cdd83c031..d48eec21a 100644
--- a/src/Util.cpp
+++ b/src/Util.cpp
@@ -22,7 +22,6 @@
 #include "Context.hpp"
 #include "Fd.hpp"
 #include "Logging.hpp"
-#include "TemporaryFile.hpp"
 #include "Win32Util.hpp"
 
 #include <Config.hpp>
diff --git a/src/ccache.cpp b/src/ccache.cpp
index 24511f995..2b820fd53 100644
--- a/src/ccache.cpp
+++ b/src/ccache.cpp
@@ -30,7 +30,6 @@
 #include "Logging.hpp"
 #include "MiniTrace.hpp"
 #include "SignalHandler.hpp"
-#include "TemporaryFile.hpp"
 #include "Util.hpp"
 #include "Win32Util.hpp"
 #include "argprocessing.hpp"
@@ -53,6 +52,7 @@
 #include <core/mainoptions.hpp>
 #include <core/types.hpp>
 #include <storage/Storage.hpp>
+#include <util/TemporaryFile.hpp>
 #include <util/UmaskScope.hpp>
 #include <util/environment.hpp>
 #include <util/expected.hpp>
@@ -717,8 +717,9 @@ get_tmp_fd(Context& ctx,
            const bool capture_output)
 {
   if (capture_output) {
-    TemporaryFile tmp_stdout(
-      FMT("{}/{}", ctx.config.temporary_dir(), description));
+    auto tmp_stdout =
+      util::value_or_throw<core::Fatal>(util::TemporaryFile::create(
+        FMT("{}/{}", ctx.config.temporary_dir(), description)));
     ctx.register_pending_tmp_file(tmp_stdout.path.string());
     return {std::move(tmp_stdout.fd), std::move(tmp_stdout.path)};
   } else {
@@ -1230,8 +1231,10 @@ get_result_key_from_cpp(Context& ctx, Args& args, Hash& hash)
 
     // preprocessed_path needs the proper cpp_extension for the compiler to do
     // its thing correctly.
-    TemporaryFile tmp_stdout(FMT("{}/cpp_stdout", ctx.config.temporary_dir()),
-                             FMT(".{}", ctx.config.cpp_extension()));
+    auto tmp_stdout =
+      util::value_or_throw<core::Fatal>(util::TemporaryFile::create(
+        FMT("{}/cpp_stdout", ctx.config.temporary_dir()),
+        FMT(".{}", ctx.config.cpp_extension())));
     preprocessed_path = tmp_stdout.path.string();
     tmp_stdout.fd.close(); // We're only using the path.
     ctx.register_pending_tmp_file(preprocessed_path);
diff --git a/src/core/mainoptions.cpp b/src/core/mainoptions.cpp
index 312a17906..b7f67a985 100644
--- a/src/core/mainoptions.cpp
+++ b/src/core/mainoptions.cpp
@@ -25,7 +25,6 @@
 #include <InodeCache.hpp>
 #include <Logging.hpp>
 #include <ProgressBar.hpp>
-#include <TemporaryFile.hpp>
 #include <ThreadPool.hpp>
 #include <Util.hpp>
 #include <assertions.hpp>
@@ -42,6 +41,7 @@
 #include <fmtmacros.hpp>
 #include <storage/Storage.hpp>
 #include <storage/local/LocalStorage.hpp>
+#include <util/TemporaryFile.hpp>
 #include <util/TextTable.hpp>
 #include <util/UmaskScope.hpp>
 #include <util/XXH3_128.hpp>
@@ -304,7 +304,7 @@ trim_dir(const std::string& dir,
 
   util::throw_on_error<core::Error>(util::traverse_directory(
     dir, [&](const std::string& path, const bool is_dir) {
-      if (is_dir || TemporaryFile::is_tmp_file(path)) {
+      if (is_dir || util::TemporaryFile::is_tmp_file(path)) {
         return;
       }
       DirEntry entry(path);
diff --git a/src/execute.cpp b/src/execute.cpp
index 78092a73c..37dba2c2f 100644
--- a/src/execute.cpp
+++ b/src/execute.cpp
@@ -24,7 +24,6 @@
 #include "Fd.hpp"
 #include "Logging.hpp"
 #include "SignalHandler.hpp"
-#include "TemporaryFile.hpp"
 #include "Util.hpp"
 #include "Win32Util.hpp"
 
@@ -32,6 +31,8 @@
 #include <core/exceptions.hpp>
 #include <fmtmacros.hpp>
 #include <util/DirEntry.hpp>
+#include <util/TemporaryFile.hpp>
+#include <util/expected.hpp>
 #include <util/file.hpp>
 #include <util/filesystem.hpp>
 #include <util/path.hpp>
@@ -226,7 +227,8 @@ win32execute(const char* path,
   });
 
   if (args.length() > 8192) {
-    TemporaryFile tmp_file(FMT("{}/cmd_args", temp_dir));
+    auto tmp_file = util::value_or_throw<core::Fatal>(
+      util::TemporaryFile::create(FMT("{}/cmd_args", temp_dir)));
     args = Win32Util::argv_to_string(argv + 1, sh, true);
     util::write_fd(*tmp_file.fd, args.data(), args.length());
     args = FMT(R"("{}" "@{}")", full_path, tmp_file.path);
diff --git a/src/storage/Storage.cpp b/src/storage/Storage.cpp
index f1521fa2d..a37a07c58 100644
--- a/src/storage/Storage.cpp
+++ b/src/storage/Storage.cpp
@@ -21,7 +21,6 @@
 #include <Config.hpp>
 #include <Logging.hpp>
 #include <MiniTrace.hpp>
-#include <TemporaryFile.hpp>
 #include <Util.hpp>
 #include <assertions.hpp>
 #include <core/CacheEntry.hpp>
diff --git a/src/storage/local/LocalStorage.cpp b/src/storage/local/LocalStorage.cpp
index 55640f93d..f0cd9b36a 100644
--- a/src/storage/local/LocalStorage.cpp
+++ b/src/storage/local/LocalStorage.cpp
@@ -24,7 +24,6 @@
 #include <File.hpp>
 #include <Logging.hpp>
 #include <MiniTrace.hpp>
-#include <TemporaryFile.hpp>
 #include <ThreadPool.hpp>
 #include <Util.hpp>
 #include <assertions.hpp>
@@ -36,6 +35,7 @@
 #include <core/exceptions.hpp>
 #include <fmtmacros.hpp>
 #include <util/Duration.hpp>
+#include <util/TemporaryFile.hpp>
 #include <util/TextTable.hpp>
 #include <util/expected.hpp>
 #include <util/file.hpp>
@@ -252,9 +252,10 @@ clone_file(const std::string& src, const std::string& dest, bool via_tmp_file)
   Fd dest_fd;
   std::string tmp_file;
   if (via_tmp_file) {
-    TemporaryFile temp_file(dest);
+    auto temp_file =
+      util::value_or_throw<core::Fatal>(util::TemporaryFile::create(dest));
     dest_fd = std::move(temp_file.fd);
-    tmp_file = temp_file.path;
+    tmp_file = std::move(temp_file.path);
   } else {
     dest_fd =
       Fd(open(dest.c_str(), O_WRONLY | O_CREAT | O_TRUNC | O_BINARY, 0666));
@@ -332,7 +333,7 @@ clean_dir(
 
     // Delete any tmp files older than 1 hour right away.
     if (file.mtime() + util::Duration(3600) < current_time
-        && TemporaryFile::is_tmp_file(file.path())) {
+        && util::TemporaryFile::is_tmp_file(file.path())) {
       util::remove(file.path().string());
       continue;
     }
@@ -893,7 +894,7 @@ LocalStorage::recompress(const std::optional<int8_t> level,
                   incompressible_size += file.size_on_disk();
                 }
               });
-            } else if (!TemporaryFile::is_tmp_file(file.path())) {
+            } else if (!util::TemporaryFile::is_tmp_file(file.path())) {
               incompressible_size += file.size_on_disk();
             }
 
diff --git a/src/util/CMakeLists.txt b/src/util/CMakeLists.txt
index e9d4042fe..adfc7709f 100644
--- a/src/util/CMakeLists.txt
+++ b/src/util/CMakeLists.txt
@@ -4,6 +4,7 @@ set(
   DirEntry.cpp
   LockFile.cpp
   LongLivedLockFileManager.cpp
+  TemporaryFile.cpp
   TextTable.cpp
   TimePoint.cpp
   Tokenizer.cpp
diff --git a/src/TemporaryFile.cpp b/src/util/TemporaryFile.cpp
similarity index 67%
rename from src/TemporaryFile.cpp
rename to src/util/TemporaryFile.cpp
index c933fd2e9..04115e8db 100644
--- a/src/TemporaryFile.cpp
+++ b/src/util/TemporaryFile.cpp
@@ -18,10 +18,6 @@
 
 #include "TemporaryFile.hpp"
 
-#include "Util.hpp"
-
-#include <core/common.hpp>
-#include <core/exceptions.hpp>
 #include <fmtmacros.hpp>
 #include <util/file.hpp>
 #include <util/filesystem.hpp>
@@ -34,19 +30,29 @@
 #endif
 
 #ifdef _WIN32
-#  include "third_party/win32/mktemp.h"
+#  include <third_party/win32/mktemp.h>
 #endif
 
 namespace fs = util::filesystem;
 
-TemporaryFile::TemporaryFile(const fs::path& path_prefix,
-                             std::string_view suffix)
+namespace util {
+
+TemporaryFile::TemporaryFile(Fd&& fd_, const fs::path& path_)
+  : fd(std::move(fd_)),
+    path(path_)
+{
+}
+
+tl::expected<TemporaryFile, std::string>
+TemporaryFile::create(const fs::path& path_prefix, std::string_view suffix)
 {
   if (path_prefix.has_parent_path()) {
-    core::ensure_dir_exists(path_prefix.parent_path());
+    if (auto ret = fs::create_directories(path_prefix.parent_path()); !ret) {
+      return tl::unexpected(ret.error().message());
+    }
   }
   std::string path_template =
-    FMT("{}{}XXXXXX{}", path_prefix, tmp_file_infix, suffix);
+    FMT("{}{}XXXXXX{}", path_prefix, TemporaryFile::tmp_file_infix, suffix);
 #ifdef _WIN32
   // MSVC lacks mkstemps() and Mingw-w64's implementation[1] is problematic, as
   // it can reuse the names of recently-deleted files unless the caller
@@ -54,20 +60,22 @@ TemporaryFile::TemporaryFile(const fs::path& path_prefix,
 
   // [1]: <https://github.com/Alexpux/mingw-w64/blob/
   // d0d7f784833bbb0b2d279310ddc6afb52fe47a46/mingw-w64-crt/misc/mkstemp.c>
-  fd = Fd(bsd_mkstemps(&path_template[0], suffix.length()));
+  Fd fd(bsd_mkstemps(&path_template[0], suffix.length()));
 #else
-  fd = Fd(mkstemps(&path_template[0], suffix.length()));
+  Fd fd(mkstemps(&path_template[0], suffix.length()));
 #endif
   if (!fd) {
-    throw core::Fatal(
-      FMT("Failed to create temporary file for {}: {}", path, strerror(errno)));
+    return tl::unexpected(FMT("failed to create temporary file for {}: {}",
+                              path_template,
+                              strerror(errno)));
   }
-  path = path_template;
 
   util::set_cloexec_flag(*fd);
 #ifndef _WIN32
   fchmod(*fd, 0666 & ~util::get_umask());
 #endif
+
+  return TemporaryFile(std::move(fd), path_template);
 }
 
 bool
@@ -75,3 +83,5 @@ TemporaryFile::is_tmp_file(const fs::path& path)
 {
   return path.filename().string().find(tmp_file_infix) != std::string::npos;
 }
+
+} // namespace util
diff --git a/src/TemporaryFile.hpp b/src/util/TemporaryFile.hpp
similarity index 79%
rename from src/TemporaryFile.hpp
rename to src/util/TemporaryFile.hpp
index 089d92c90..f7d36ba00 100644
--- a/src/TemporaryFile.hpp
+++ b/src/util/TemporaryFile.hpp
@@ -20,10 +20,14 @@
 
 #include <Fd.hpp>
 
+#include <third_party/tl/expected.hpp>
+
 #include <filesystem>
 #include <string>
 #include <string_view>
 
+namespace util {
+
 // This class represents a unique temporary file created by mkstemp. The file is
 // not deleted by the destructor.
 class TemporaryFile
@@ -31,21 +35,29 @@ class TemporaryFile
 public:
   static constexpr char tmp_file_infix[] = ".tmp.";
 
-  // `path_prefix` is the base path. The resulting filename will be this path
-  // plus a unique string plus `suffix`. If `path_prefix` refers to a
-  // nonexistent directory the directory will be created if possible.
-  TemporaryFile(const std::filesystem::path& path_prefix,
-                std::string_view suffix = ".tmp");
+  TemporaryFile() = delete;
 
   TemporaryFile(TemporaryFile&& other) noexcept = default;
 
   TemporaryFile& operator=(TemporaryFile&& other) noexcept = default;
 
+  // `path_prefix` is the base path. The resulting filename will be this path
+  // plus a unique string plus `suffix`. If `path_prefix` refers to a
+  // nonexistent directory the directory will be created if possible.
+  static tl::expected<TemporaryFile, std::string>
+  create(const std::filesystem::path& path_prefix,
+         std::string_view suffix = ".tmp");
+
   static bool is_tmp_file(const std::filesystem::path& path);
 
-  // The resulting open file descriptor in read/write mode. Unset on error.
+  // The resulting open file descriptor in read/write mode.
   Fd fd;
 
-  // The actual filename. Empty on error.
+  // The actual filename.
   std::filesystem::path path;
+
+private:
+  TemporaryFile(Fd&& fd_, const std::filesystem::path& path_);
 };
+
+} // namespace util
diff --git a/src/util/file.cpp b/src/util/file.cpp
index ec4037889..220c7b819 100644
--- a/src/util/file.cpp
+++ b/src/util/file.cpp
@@ -21,11 +21,11 @@
 #include <Fd.hpp>
 #include <Finalizer.hpp>
 #include <Logging.hpp>
-#include <TemporaryFile.hpp>
 #include <Win32Util.hpp>
 #include <fmtmacros.hpp>
 #include <util/Bytes.hpp>
 #include <util/DirEntry.hpp>
+#include <util/TemporaryFile.hpp>
 #include <util/expected.hpp>
 #include <util/file.hpp>
 #include <util/filesystem.hpp>
@@ -81,9 +81,12 @@ copy_file(const fs::path& src, const fs::path& dest, ViaTmpFile via_tmp_file)
   Fd dest_fd;
   fs::path tmp_file;
   if (via_tmp_file == ViaTmpFile::yes) {
-    TemporaryFile temp_file(dest);
-    dest_fd = std::move(temp_file.fd);
-    tmp_file = temp_file.path;
+    auto temp_file = TemporaryFile::create(dest);
+    if (!temp_file) {
+      return tl::unexpected(temp_file.error());
+    }
+    dest_fd = std::move(temp_file->fd);
+    tmp_file = std::move(temp_file->path);
   } else {
     dest_fd = Fd(open(
       dest.string().c_str(), O_WRONLY | O_CREAT | O_TRUNC | O_BINARY, 0666));