From: Joel Rosdahl <joel@rosdahl.net>
Date: Tue, 18 Jul 2023 12:06:37 +0000 (+0200)
Subject: refactor: Move Util::unlink_* to util
X-Git-Tag: v4.9~103
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=19f670098cf581ee8c2bd7ec304567dc4f806bc3;p=thirdparty%2Fccache.git

refactor: Move Util::unlink_* to util
---

diff --git a/src/AtomicFile.cpp b/src/AtomicFile.cpp
index 73c5682bb..d68fbd62a 100644
--- a/src/AtomicFile.cpp
+++ b/src/AtomicFile.cpp
@@ -19,7 +19,6 @@
 #include "AtomicFile.hpp"
 
 #include "TemporaryFile.hpp"
-#include "Util.hpp"
 #include "assertions.hpp"
 
 #include <core/exceptions.hpp>
@@ -38,7 +37,7 @@ AtomicFile::~AtomicFile()
   if (m_stream) {
     // commit() was not called so remove the lingering temporary file.
     fclose(m_stream);
-    Util::unlink_tmp(m_tmp_path);
+    util::remove(m_tmp_path);
   }
 }
 
@@ -76,7 +75,7 @@ AtomicFile::commit()
   int retcode = fclose(m_stream);
   m_stream = nullptr;
   if (retcode == EOF) {
-    Util::unlink_tmp(m_tmp_path);
+    util::remove(m_tmp_path);
     throw core::Error(
       FMT("failed to write data to {}: {}", m_path, strerror(errno)));
   }
diff --git a/src/Context.cpp b/src/Context.cpp
index 1aaf996cd..f0a583a67 100644
--- a/src/Context.cpp
+++ b/src/Context.cpp
@@ -90,7 +90,8 @@ Context::unlink_pending_tmp_files_signal_safe()
 {
   for (auto it = m_pending_tmp_files.rbegin(); it != m_pending_tmp_files.rend();
        ++it) {
-    // Don't call Util::unlink_tmp since its log calls aren't signal safe.
+    // Don't call util::remove or std::filesystem::remove since they are not
+    // signal safe.
     unlink(it->c_str());
   }
   // Don't clear m_pending_tmp_files since this method must be signal safe.
@@ -103,7 +104,7 @@ Context::unlink_pending_tmp_files()
 
   for (auto it = m_pending_tmp_files.rbegin(); it != m_pending_tmp_files.rend();
        ++it) {
-    Util::unlink_tmp(*it, Util::UnlinkLog::ignore_failure);
+    util::remove(*it, util::LogFailure::no);
   }
   m_pending_tmp_files.clear();
 }
diff --git a/src/MiniTrace.cpp b/src/MiniTrace.cpp
index db4141d03..7d04b2c5d 100644
--- a/src/MiniTrace.cpp
+++ b/src/MiniTrace.cpp
@@ -20,7 +20,6 @@
 
 #include "ArgsInfo.hpp"
 #include "TemporaryFile.hpp"
-#include "Util.hpp"
 #include "fmtmacros.hpp"
 
 #include <core/wincompat.hpp>
@@ -62,5 +61,5 @@ MiniTrace::~MiniTrace()
   if (!m_args_info.output_obj.empty()) {
     util::copy_file(m_tmp_trace_file, m_args_info.output_obj + ".ccache-trace");
   }
-  Util::unlink_tmp(m_tmp_trace_file);
+  util::remove(m_tmp_trace_file);
 }
diff --git a/src/Util.cpp b/src/Util.cpp
index 021114d85..e8aa61fff 100644
--- a/src/Util.cpp
+++ b/src/Util.cpp
@@ -655,58 +655,4 @@ traverse(const std::string& path, const TraverseVisitor& visitor)
 
 #endif
 
-bool
-unlink_safe(const std::string& path, UnlinkLog unlink_log)
-{
-  int saved_errno = 0;
-
-  // If path is on an NFS share, unlink isn't atomic, so we rename to a temp
-  // file. We don't care if the temp file is trashed, so it's always safe to
-  // unlink it first.
-  const std::string tmp_name =
-    FMT("{}.ccache{}unlink", path, TemporaryFile::tmp_file_infix);
-
-  bool success = true;
-  const auto result = util::rename(path, tmp_name);
-  if (!result) {
-    success = false;
-    saved_errno = errno;
-  }
-  if (success && unlink(tmp_name.c_str()) != 0) {
-    // It's OK if it was unlinked in a race.
-    if (errno != ENOENT && errno != ESTALE) {
-      success = false;
-      saved_errno = errno;
-    }
-  }
-  if (success || unlink_log == UnlinkLog::log_failure) {
-    LOG("Unlink {} via {}", path, tmp_name);
-    if (!success) {
-      LOG("Unlink failed: {}", strerror(saved_errno));
-    }
-  }
-
-  errno = saved_errno;
-  return success;
-}
-
-bool
-unlink_tmp(const std::string& path, UnlinkLog unlink_log)
-{
-  int saved_errno = 0;
-
-  bool success =
-    unlink(path.c_str()) == 0 || (errno == ENOENT || errno == ESTALE);
-  saved_errno = errno;
-  if (success || unlink_log == UnlinkLog::log_failure) {
-    LOG("Unlink {}", path);
-    if (!success) {
-      LOG("Unlink failed: {}", strerror(saved_errno));
-    }
-  }
-
-  errno = saved_errno;
-  return success;
-}
-
 } // namespace Util
diff --git a/src/Util.hpp b/src/Util.hpp
index f47d1d4c8..ed90f57aa 100644
--- a/src/Util.hpp
+++ b/src/Util.hpp
@@ -38,8 +38,6 @@ namespace Util {
 using TraverseVisitor =
   std::function<void(const std::string& path, bool is_dir)>;
 
-enum class UnlinkLog { log_failure, ignore_failure };
-
 // Get base name of path.
 std::string_view base_name(std::string_view path);
 
@@ -150,19 +148,4 @@ void send_to_fd(const Context& ctx, std::string_view text, int fd);
 // Throws core::Error on error.
 void traverse(const std::string& path, const TraverseVisitor& visitor);
 
-// Remove `path` (non-directory), NFS safe. Logs according to `unlink_log`.
-//
-// Returns whether removal was successful. A nonexistent `path` is considered a
-// failure.
-bool unlink_safe(const std::string& path,
-                 UnlinkLog unlink_log = UnlinkLog::log_failure);
-
-// Remove `path` (non-directory), NFS hazardous. Use only for files that will
-// not exist on other systems. Logs according to `unlink_log`.
-//
-// Returns whether removal was successful. A nonexistent `path` is considered
-// successful.
-bool unlink_tmp(const std::string& path,
-                UnlinkLog unlink_log = UnlinkLog::log_failure);
-
 } // namespace Util
diff --git a/src/ccache.cpp b/src/ccache.cpp
index 3112d795e..6f8cc48e8 100644
--- a/src/ccache.cpp
+++ b/src/ccache.cpp
@@ -1069,7 +1069,7 @@ to_cache(Context& ctx,
     // Workaround for Clang bug where it overwrites an existing object file
     // when it's compiling an assembler file, see
     // <https://bugs.llvm.org/show_bug.cgi?id=39782>.
-    Util::unlink_safe(ctx.args_info.output_obj);
+    util::remove_nfs_safe(ctx.args_info.output_obj);
   }
 
   if (ctx.args_info.generating_diagnostics) {
diff --git a/src/core/mainoptions.cpp b/src/core/mainoptions.cpp
index 3ec634f6f..09cff4225 100644
--- a/src/core/mainoptions.cpp
+++ b/src/core/mainoptions.cpp
@@ -375,7 +375,7 @@ trim_dir(const std::string& dir,
       if (final_size <= trim_max_size) {
         break;
       }
-      if (Util::unlink_tmp(file.path())) {
+      if (util::remove(file.path())) {
         ++removed_files;
         final_size -= file.size_on_disk();
       }
diff --git a/src/execute.cpp b/src/execute.cpp
index 44215c66e..d9b679d6a 100644
--- a/src/execute.cpp
+++ b/src/execute.cpp
@@ -218,7 +218,7 @@ win32execute(const char* path,
 
   Finalizer tmp_file_remover([&tmp_file_path] {
     if (!tmp_file_path.empty()) {
-      Util::unlink_tmp(tmp_file_path);
+      util::remove(tmp_file_path);
     }
   });
 
diff --git a/src/storage/local/LocalStorage.cpp b/src/storage/local/LocalStorage.cpp
index f6f7a7558..e4294963c 100644
--- a/src/storage/local/LocalStorage.cpp
+++ b/src/storage/local/LocalStorage.cpp
@@ -219,8 +219,9 @@ delete_file(const std::string& path,
             uint64_t& cache_size,
             uint64_t& files_in_cache)
 {
-  const bool deleted = Util::unlink_safe(path, Util::UnlinkLog::ignore_failure);
-  if (!deleted && errno != ENOENT && errno != ESTALE) {
+  const auto result = util::remove_nfs_safe(path, util::LogFailure::no);
+  if (!result && result.error().value() != ENOENT
+      && result.error().value() != ESTALE) {
     LOG("Failed to unlink {} ({})", path, strerror(errno));
   } else {
     // The counters are intentionally subtracted even if there was no file to
@@ -330,7 +331,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::unlink_tmp(file.path());
+      util::remove(file.path());
       continue;
     }
 
@@ -574,7 +575,7 @@ LocalStorage::remove(const Hash::Digest& key, const core::CacheEntryType type)
     if (!l2_content_lock.acquire()) {
       LOG("Not removing {} due to lock failure", cache_file.path);
     }
-    Util::unlink_safe(cache_file.path);
+    util::remove_nfs_safe(cache_file.path);
   }
 
   LOG("Removed {} from local storage ({})",
@@ -780,7 +781,7 @@ LocalStorage::wipe_all(const ProgressReceiver& progress_receiver)
           l2_progress_receiver(0.5);
 
           for (size_t i = 0; i < files.size(); ++i) {
-            Util::unlink_safe(files[i].path());
+            util::remove_nfs_safe(files[i].path());
             l2_progress_receiver(0.5 + 0.5 * i / files.size());
           }
 
@@ -1417,7 +1418,7 @@ LocalStorage::clean_internal_tempdir()
                    }
                    const auto st = Stat::lstat(path, Stat::OnError::log);
                    if (st && st.mtime() + k_tempdir_cleanup_interval < now) {
-                     Util::unlink_tmp(path);
+                     util::remove(path);
                    }
                  });
 
diff --git a/src/storage/remote/FileStorage.cpp b/src/storage/remote/FileStorage.cpp
index 0d3848eba..57a511917 100644
--- a/src/storage/remote/FileStorage.cpp
+++ b/src/storage/remote/FileStorage.cpp
@@ -175,7 +175,13 @@ FileStorageBackend::put(const Hash::Digest& key,
 nonstd::expected<bool, RemoteStorage::Backend::Failure>
 FileStorageBackend::remove(const Hash::Digest& key)
 {
-  return Util::unlink_safe(get_entry_path(key));
+  auto entry_path = get_entry_path(key);
+  auto result = util::remove_nfs_safe(entry_path);
+  if (!result) {
+    LOG("Failed to remove {}: {}", entry_path, result.error().message());
+    return nonstd::make_unexpected(RemoteStorage::Backend::Failure::error);
+  }
+  return *result;
 }
 
 std::string
diff --git a/src/util/LockFile.cpp b/src/util/LockFile.cpp
index cfa3625d2..fd9571637 100644
--- a/src/util/LockFile.cpp
+++ b/src/util/LockFile.cpp
@@ -162,8 +162,8 @@ LockFile::release()
   if (m_lock_manager) {
     m_lock_manager->deregister_alive_file(m_alive_file);
   }
-  Util::unlink_tmp(m_alive_file);
-  Util::unlink_tmp(m_lock_file);
+  util::remove(m_alive_file);
+  util::remove(m_lock_file);
 #else
   CloseHandle(m_handle);
 #endif
@@ -314,7 +314,7 @@ LockFile::do_acquire(const bool blocking)
           m_lock_file,
           inactive_duration.sec(),
           inactive_duration.nsec_decimal_part() / 1'000'000);
-      if (!Util::unlink_tmp(m_alive_file) || !Util::unlink_tmp(m_lock_file)) {
+      if (!util::remove(m_alive_file) || !util::remove(m_lock_file)) {
         return false;
       }
 
diff --git a/src/util/file.cpp b/src/util/file.cpp
index d7f67d0d6..020499366 100644
--- a/src/util/file.cpp
+++ b/src/util/file.cpp
@@ -28,6 +28,7 @@
 #include <util/Bytes.hpp>
 #include <util/expected.hpp>
 #include <util/file.hpp>
+#include <util/filesystem.hpp>
 
 #ifdef HAVE_UNISTD_H
 #  include <unistd.h>
@@ -59,6 +60,8 @@
 #include <type_traits>
 #include <vector>
 
+namespace fs = util::filesystem;
+
 namespace util {
 
 nonstd::expected<void, std::string>
@@ -372,6 +375,53 @@ read_file_part(const std::string& path, size_t pos, size_t count);
 template nonstd::expected<std::vector<uint8_t>, std::string>
 read_file_part(const std::string& path, size_t pos, size_t count);
 
+nonstd::expected<bool, std::error_code>
+remove(const std::string& path, LogFailure log_failure)
+{
+  auto result = fs::remove(path);
+  if (result || log_failure == LogFailure::yes) {
+    LOG("Removing {}", path);
+    if (!result) {
+      LOG("Removal failed: {}", result.error().message());
+    }
+  }
+  return result;
+}
+
+nonstd::expected<bool, std::error_code>
+remove_nfs_safe(const std::string& path, LogFailure log_failure)
+{
+  // fs::remove isn't atomic if path is on an NFS share, so we rename to a
+  // temporary file. We don't care if the temporary file is trashed, so it's
+  // always safe to remove it first.
+  std::string tmp_name =
+    FMT("{}.ccache{}remove", path, TemporaryFile::tmp_file_infix);
+
+  auto rename_result = util::rename(path, tmp_name);
+  if (!rename_result) {
+    // It's OK if it was removed in a race.
+    if (rename_result.error().value() != ENOENT
+        && rename_result.error().value() != ESTALE
+        && log_failure == LogFailure::yes) {
+      LOG("Removing {} via {}", path, tmp_name);
+      LOG("Renaming {} to {} failed: {}",
+          path,
+          tmp_name,
+          rename_result.error().message());
+    }
+    return nonstd::make_unexpected(rename_result.error());
+  }
+
+  auto remove_result = fs::remove(tmp_name);
+  if (remove_result || log_failure == LogFailure::yes) {
+    LOG("Removing {} via {}", path, tmp_name);
+    if (!remove_result) {
+      LOG("Removal failed: {}", remove_result.error().message());
+    }
+  }
+  return remove_result;
+}
+
 nonstd::expected<void, std::error_code>
 rename(const std::string& oldpath, const std::string& newpath)
 {
diff --git a/src/util/file.hpp b/src/util/file.hpp
index 347f5d91d..e1174c656 100644
--- a/src/util/file.hpp
+++ b/src/util/file.hpp
@@ -36,6 +36,7 @@ namespace util {
 // --- Interface ---
 
 enum class InPlace { yes, no };
+enum class LogFailure { yes, no };
 enum class ViaTmpFile { yes, no };
 
 // Copy a file from `src` to `dest`. If `via_tmp_file` is yes, `src` is copied
@@ -84,6 +85,22 @@ template<typename T>
 nonstd::expected<T, std::string>
 read_file_part(const std::string& path, size_t pos, size_t count);
 
+// Remove `path` (non-directory), NFS hazardous. Use only for files that will
+// not exist on other systems.
+//
+// Returns whether the file was removed. A nonexistent `path` is considered
+// successful.
+nonstd::expected<bool, std::error_code>
+remove(const std::string& path, LogFailure log_failure = LogFailure::yes);
+
+// Remove `path` (non-directory), NFS safe.
+//
+// Returns whether the file was removed. A nonexistent `path` is considered a
+// successful.
+nonstd::expected<bool, std::error_code>
+remove_nfs_safe(const std::string& path,
+                LogFailure log_failure = LogFailure::yes);
+
 // Rename `oldpath` to `newpath` (deleting `newpath`).
 //
 // Note: Mingw-w64's std::filesystem::rename is buggy and doesn't properly