From: Joel Rosdahl <joel@rosdahl.net>
Date: Sat, 16 May 2020 14:17:44 +0000 (+0200)
Subject: C++-ify unlink wrapper functions
X-Git-Tag: v4.0~442
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=26fd7eab265e5d53082403e28949e7c6b45995b9;p=thirdparty%2Fccache.git

C++-ify unlink wrapper functions

x_unlink(p)   == 0 -> Util::unlink_safe(p)
x_try_unlink  == 0 -> Util::unlink_safe(p, Util::UnlinkLog::ignore_failure)
tmp_unlink(p) == 0 -> Util::unlink_tmp(p)
---

diff --git a/src/AtomicFile.cpp b/src/AtomicFile.cpp
index 4dbc805c9..0cb6cc215 100644
--- a/src/AtomicFile.cpp
+++ b/src/AtomicFile.cpp
@@ -39,7 +39,7 @@ AtomicFile::~AtomicFile()
   if (m_stream) {
     // commit() was not called so remove the lingering temporary file.
     fclose(m_stream);
-    tmp_unlink(m_tmp_path.c_str());
+    Util::unlink_tmp(m_tmp_path);
   }
 }
 
@@ -68,7 +68,7 @@ AtomicFile::commit()
   int result = fclose(m_stream);
   m_stream = nullptr;
   if (result == EOF) {
-    tmp_unlink(m_tmp_path.c_str());
+    Util::unlink_tmp(m_tmp_path);
     throw Error(
       fmt::format("failed to write data to {}: {}", m_path, strerror(errno)));
   }
diff --git a/src/Lockfile.cpp b/src/Lockfile.cpp
index 1ddd16ac4..e1fe22579 100644
--- a/src/Lockfile.cpp
+++ b/src/Lockfile.cpp
@@ -116,7 +116,7 @@ do_acquire_posix(const std::string& lockfile, uint32_t staleness_limit)
     } else {
       // The lock seems to be stale -- break it and try again.
       cc_log("lockfile_acquire: breaking %s", lockfile.c_str());
-      if (tmp_unlink(lockfile.c_str()) != 0) {
+      if (!Util::unlink_tmp(lockfile)) {
         cc_log("Failed to unlink %s: %s", lockfile.c_str(), strerror(errno));
         return false;
       }
@@ -211,7 +211,7 @@ Lockfile::~Lockfile()
   if (acquired()) {
     cc_log("Releasing lock %s", m_lockfile.c_str());
 #ifndef _WIN32
-    if (tmp_unlink(m_lockfile.c_str()) != 0) {
+    if (!Util::unlink_tmp(m_lockfile)) {
       cc_log("Failed to unlink %s: %s", m_lockfile.c_str(), strerror(errno));
     }
 #else
diff --git a/src/Util.cpp b/src/Util.cpp
index 900b5e4c4..104abb007 100644
--- a/src/Util.cpp
+++ b/src/Util.cpp
@@ -21,6 +21,7 @@
 #include "Config.hpp"
 #include "Context.hpp"
 #include "FormatNonstdStringView.hpp"
+#include "logging.hpp"
 
 #ifdef _WIN32
 #  include "win32compat.hpp"
@@ -692,6 +693,57 @@ traverse(const std::string& path, const TraverseVisitor& visitor)
   }
 }
 
+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.
+  std::string tmp_name = path + ".ccache.rm.tmp";
+
+  bool success = true;
+  if (x_rename(path.c_str(), tmp_name.c_str()) != 0) {
+    success = false;
+    saved_errno = errno;
+  } else if (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) {
+    cc_log("Unlink %s via %s", path.c_str(), tmp_name.c_str());
+    if (!success) {
+      cc_log("Unlink failed: %s", 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);
+  if (success || unlink_log == UnlinkLog::log_failure) {
+    cc_log("Unlink %s", path.c_str());
+    if (!success) {
+      cc_log("Unlink failed: %s", strerror(saved_errno));
+    }
+  }
+
+  errno = saved_errno;
+  return success;
+}
+
 void
 wipe_path(const std::string& path)
 {
diff --git a/src/Util.hpp b/src/Util.hpp
index 4902dcd86..44c2306b9 100644
--- a/src/Util.hpp
+++ b/src/Util.hpp
@@ -41,6 +41,8 @@ using SubdirVisitor =
 using TraverseVisitor =
   std::function<void(const std::string& path, bool is_dir)>;
 
+enum class UnlinkLog { log_failure, ignore_failure };
+
 // Get base name of path.
 nonstd::string_view base_name(nonstd::string_view path);
 
@@ -288,6 +290,21 @@ strip_whitespace(const std::string& string);
 // Throws 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 non-existing `path` is considered
+// successful.
+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 non-existing `path` is considered
+// successful.
+bool unlink_tmp(const std::string& path,
+                UnlinkLog unlink_log = UnlinkLog::log_failure);
+
 // Remove `path` (and its contents if it's a directory). A non-existing path is
 // not considered an error.
 //
diff --git a/src/ccache.cpp b/src/ccache.cpp
index 8b3ba4a8a..a3582344d 100644
--- a/src/ccache.cpp
+++ b/src/ccache.cpp
@@ -317,7 +317,7 @@ clean_up_internal_tempdir(const Config& config)
     }
     auto st = Stat::lstat(path, Stat::OnError::log);
     if (st && st.mtime() + k_tempdir_cleanup_interval < now) {
-      tmp_unlink(path.c_str());
+      Util::unlink_tmp(path);
     }
   });
 }
@@ -799,7 +799,7 @@ use_relative_paths_in_depfile(const Context& ctx)
       " usage",
       tmp_file.c_str(),
       e.what());
-    x_unlink(tmp_file.c_str());
+    Util::unlink_safe(tmp_file);
     return;
   }
 
@@ -809,7 +809,7 @@ use_relative_paths_in_depfile(const Context& ctx)
       tmp_file.c_str(),
       output_dep.c_str(),
       strerror(errno));
-    x_unlink(tmp_file.c_str());
+    Util::unlink_safe(tmp_file);
   } else {
     cc_log("Renamed dependency file: %s -> %s",
            tmp_file.c_str(),
@@ -958,7 +958,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>.
-    x_unlink(ctx.args_info.output_obj.c_str());
+    Util::unlink_safe(ctx.args_info.output_obj);
   }
 
   if (ctx.args_info.generating_diagnostics) {
@@ -984,7 +984,8 @@ to_cache(Context& ctx,
     // produced one, intentionally not using x_unlink or tmp_unlink since we're
     // not interested in logging successful deletions or failures due to
     // non-existent .dwo files.
-    if (unlink(ctx.args_info.output_dwo.c_str()) == -1 && errno != ENOENT) {
+    if (unlink(ctx.args_info.output_dwo.c_str()) != 0 && errno != ENOENT
+        && errno != ESTALE) {
       cc_log("Failed to unlink %s: %s",
              ctx.args_info.output_dwo.c_str(),
              strerror(errno));
@@ -1032,8 +1033,8 @@ to_cache(Context& ctx,
   auto st = Stat::stat(tmp_stdout, Stat::OnError::log);
   if (!st) {
     // The stdout file was removed - cleanup in progress? Better bail out.
-    tmp_unlink(tmp_stdout);
-    tmp_unlink(tmp_stderr);
+    Util::unlink_tmp(tmp_stdout);
+    Util::unlink_tmp(tmp_stderr);
     failed(STATS_MISSING);
   }
 
@@ -1041,11 +1042,11 @@ to_cache(Context& ctx,
   // __________Using # distcc servers in pump mode
   if (st.size() != 0 && ctx.guessed_compiler != GuessedCompiler::pump) {
     cc_log("Compiler produced stdout");
-    tmp_unlink(tmp_stdout);
-    tmp_unlink(tmp_stderr);
+    Util::unlink_tmp(tmp_stdout);
+    Util::unlink_tmp(tmp_stderr);
     failed(STATS_STDOUT);
   }
-  tmp_unlink(tmp_stdout);
+  Util::unlink_tmp(tmp_stdout);
 
   // Merge stderr from the preprocessor (if any) and stderr from the real
   // compiler into tmp_stderr.
@@ -1083,7 +1084,7 @@ to_cache(Context& ctx,
     close(fd_cpp_stderr);
     close(fd_real_stderr);
     close(fd_result);
-    tmp_unlink(tmp_stderr2);
+    Util::unlink_tmp(tmp_stderr2);
     free(tmp_stderr2);
   }
 
@@ -1095,12 +1096,12 @@ to_cache(Context& ctx,
       // We can output stderr immediately instead of rerunning the compiler.
       copy_fd(fd, 2);
       close(fd);
-      tmp_unlink(tmp_stderr);
+      Util::unlink_tmp(tmp_stderr);
 
       failed(STATS_STATUS, status);
     }
 
-    tmp_unlink(tmp_stderr);
+    Util::unlink_tmp(tmp_stderr);
     failed(STATS_STATUS);
   }
 
@@ -1188,14 +1189,14 @@ to_cache(Context& ctx,
     // previous ccache versions.
     if (getpid() % 1000 == 0) {
       char* path = format("%s/CACHEDIR.TAG", ctx.config.cache_dir().c_str());
-      x_unlink(path);
+      Util::unlink_safe(path);
       free(path);
     }
   }
 
   // Everything OK.
   send_cached_stderr(tmp_stderr);
-  tmp_unlink(tmp_stderr);
+  Util::unlink_tmp(tmp_stderr);
 
   free(tmp_stderr);
   free(tmp_stdout);
@@ -1897,7 +1898,7 @@ from_cache(Context& ctx, enum fromcache_call_mode mode)
   bool ok = result_get(ctx, ctx.result_path(), result_file_map);
   if (!ok) {
     cc_log("Failed to get result from cache");
-    tmp_unlink(tmp_stderr);
+    Util::unlink_tmp(tmp_stderr);
     free(tmp_stderr);
     return nullopt;
   }
@@ -1906,7 +1907,7 @@ from_cache(Context& ctx, enum fromcache_call_mode mode)
 
   send_cached_stderr(tmp_stderr);
 
-  tmp_unlink(tmp_stderr);
+  Util::unlink_tmp(tmp_stderr);
   free(tmp_stderr);
 
   cc_log("Succeeded getting cached result");
@@ -2377,7 +2378,7 @@ do_cache_compilation(Context& ctx, const char* const* argv)
       cc_log("Hash from manifest doesn't match preprocessor output");
       cc_log("Likely reason: different CCACHE_BASEDIRs used");
       cc_log("Removing manifest as a safety measure");
-      x_unlink(ctx.manifest_path().c_str());
+      Util::unlink_safe(ctx.manifest_path());
 
       put_result_in_manifest = true;
     }
diff --git a/src/cleanup.cpp b/src/cleanup.cpp
index 0001db28b..57e68c8ac 100644
--- a/src/cleanup.cpp
+++ b/src/cleanup.cpp
@@ -21,6 +21,7 @@
 
 #include "CacheFile.hpp"
 #include "Config.hpp"
+#include "Util.hpp"
 #include "logging.hpp"
 #include "stats.hpp"
 
@@ -33,7 +34,7 @@ delete_file(const std::string& path,
             uint64_t* cache_size,
             uint32_t* files_in_cache)
 {
-  bool deleted = x_try_unlink(path.c_str()) == 0;
+  bool deleted = Util::unlink_safe(path, Util::UnlinkLog::ignore_failure);
   if (!deleted && errno != ENOENT && errno != ESTALE) {
     cc_log("Failed to unlink %s (%s)", path.c_str(), strerror(errno));
   } else if (cache_size && files_in_cache) {
@@ -75,7 +76,7 @@ clean_up_dir(const std::string& subdir,
     // Delete any tmp files older than 1 hour right away.
     if (file->lstat().mtime() + 3600 < current_time
         && Util::base_name(file->path()).find(".tmp.") != std::string::npos) {
-      x_unlink(file->path().c_str());
+      Util::unlink_tmp(file->path());
       continue;
     }
 
@@ -174,7 +175,7 @@ wipe_dir(const std::string& subdir,
     subdir, [&](double progress) { progress_receiver(progress / 2); }, files);
 
   for (size_t i = 0; i < files.size(); ++i) {
-    x_unlink(files[i]->path().c_str());
+    Util::unlink_safe(files[i]->path());
     progress_receiver(0.5 + 0.5 * i / files.size());
   }
 
diff --git a/src/execute.cpp b/src/execute.cpp
index 0c25f0dba..f8a89d723 100644
--- a/src/execute.cpp
+++ b/src/execute.cpp
@@ -205,7 +205,7 @@ win32execute(const char* path,
     fclose(fp);
     snprintf(atfile, sizeof(atfile), "\"@%s\"", tmp_file);
     ret = CreateProcess(NULL, atfile, NULL, NULL, 1, 0, NULL, NULL, &si, &pi);
-    tmp_unlink(tmp_file);
+    Util::unlink_tmp(tmp_file);
     free(tmp_file);
   }
   if (!ret) {
diff --git a/src/legacy_util.cpp b/src/legacy_util.cpp
index a088040eb..d4ccf337d 100644
--- a/src/legacy_util.cpp
+++ b/src/legacy_util.cpp
@@ -256,7 +256,7 @@ move_file(const char* src, const char* dest)
 {
   bool ok = copy_file(src, dest, false);
   if (ok) {
-    x_unlink(src);
+    Util::unlink_safe(src);
   }
   return ok;
 }
@@ -709,69 +709,6 @@ x_rename(const char* oldpath, const char* newpath)
 #endif
 }
 
-// Remove path, NFS hazardous. Use only for temporary files that will not exist
-// on other systems. That is, the path should include tmp_string().
-int
-tmp_unlink(const char* path)
-{
-  cc_log("Unlink %s", path);
-  int rc = unlink(path);
-  if (rc) {
-    cc_log("Unlink failed: %s", strerror(errno));
-  }
-  return rc;
-}
-
-static int
-do_x_unlink(const char* path, bool log_failure)
-{
-  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.
-  char* tmp_name = format("%s.ccache.rm.tmp", path);
-
-  int result = 0;
-  if (x_rename(path, tmp_name) == -1) {
-    result = -1;
-    saved_errno = errno;
-    goto out;
-  }
-  if (unlink(tmp_name) == -1) {
-    // If it was released in a race, that's OK.
-    if (errno != ENOENT && errno != ESTALE) {
-      result = -1;
-      saved_errno = errno;
-    }
-  }
-
-out:
-  if (result == 0 || log_failure) {
-    cc_log("Unlink %s via %s", path, tmp_name);
-    if (result != 0 && log_failure) {
-      cc_log("x_unlink failed: %s", strerror(saved_errno));
-    }
-  }
-  free(tmp_name);
-  errno = saved_errno;
-  return result;
-}
-
-// Remove path, NFS safe, log both successes and failures.
-int
-x_unlink(const char* path)
-{
-  return do_x_unlink(path, true);
-}
-
-// Remove path, NFS safe, only log successes.
-int
-x_try_unlink(const char* path)
-{
-  return do_x_unlink(path, false);
-}
-
 // Reads the content of a file. Size hint 0 means no hint. Returns true on
 // success, otherwise false.
 bool
diff --git a/src/legacy_util.hpp b/src/legacy_util.hpp
index a7fb3ae71..95de83673 100644
--- a/src/legacy_util.hpp
+++ b/src/legacy_util.hpp
@@ -56,9 +56,6 @@ bool is_full_path(const char* path);
 void update_mtime(const char* path);
 void x_exit(int status) ATTR_NORETURN;
 int x_rename(const char* oldpath, const char* newpath);
-int tmp_unlink(const char* path);
-int x_unlink(const char* path);
-int x_try_unlink(const char* path);
 bool read_file(const char* path, size_t size_hint, char** data, size_t* size);
 char* read_text_file(const char* path, size_t size_hint);
 char* subst_env_in_string(const char* str, char** errmsg);
diff --git a/src/result.cpp b/src/result.cpp
index fe8c28633..462b90988 100644
--- a/src/result.cpp
+++ b/src/result.cpp
@@ -233,7 +233,7 @@ copy_raw_file(const Context& ctx,
     cc_log("Failed to clone: %s", strerror(errno));
   }
   if (ctx.config.hard_link()) {
-    x_try_unlink(dest.c_str());
+    unlink(dest.c_str());
     cc_log("Hard linking %s to %s", source.c_str(), dest.c_str());
     int ret = link(source.c_str(), dest.c_str());
     if (ret == 0) {
diff --git a/src/stats.cpp b/src/stats.cpp
index bd4972fac..5ebc8e395 100644
--- a/src/stats.cpp
+++ b/src/stats.cpp
@@ -479,7 +479,7 @@ void
 stats_zero(const Config& config)
 {
   char* fname = format("%s/stats", config.cache_dir().c_str());
-  x_unlink(fname);
+  Util::unlink_safe(fname);
   free(fname);
 
   time_t timestamp = time(nullptr);