Improve atomiticy of update checkpointing

Current check point works by writing different prefs to different files
under a pending directory, and rename the pending directory to actual pref
directory afterwards to achieve atomicity. It has two pitfalls:

1. Before the rename() call, existing prefs dir must be rm -rf'ed , this
   deletion process isn't atomic. If device rebooted during rm -rf, we
   will end up with a partially deleted old pref.

2. fsync() on the parent directory is needed after rename()

This CL addresses both issues. For #1, we rename() the old pref dir to a
tmp dir first, and then rm -rf the tmp dir. Upon device restart, if the
current prefs dir is empty, we can simply rename() the pending directory
to actual pref directory.

Test: th
Bug: 295252766
Change-Id: Ic671a18245986c579b51d7443c3e8c10e206c448

3 files changed, 37 insertions, 4 deletions

diff --git a/common/prefs.cc b/common/prefs.cc
index 57d4dcdd..af4d3186 100644
--- a/common/prefs.cc
+++ b/common/prefs.cc
@@ -20,6 +20,7 @@
 #include <filesystem>
 #include <unistd.h>
 
+#include <android-base/file.h>
 #include <base/files/file_enumerator.h>
 #include <base/files/file_util.h>
 #include <base/logging.h>
@@ -27,7 +28,6 @@
 #include <base/strings/string_split.h>
 #include <base/strings/string_util.h>
 
-#include "update_engine/common/platform_constants.h"
 #include "update_engine/common/utils.h"
 
 using std::string;
@@ -74,7 +74,16 @@ bool PrefsBase::GetInt64(const std::string_view key, int64_t* value) const {
   if (!GetString(key, &str_value))
     return false;
   base::TrimWhitespaceASCII(str_value, base::TRIM_ALL, &str_value);
-  TEST_AND_RETURN_FALSE(base::StringToInt64(str_value, value));
+  if (str_value.empty()) {
+    LOG(ERROR) << "When reading pref " << key
+               << ", got an empty value after trim";
+    return false;
+  }
+  if (!base::StringToInt64(str_value, value)) {
+    LOG(ERROR) << "When reading pref " << key << ", failed to convert value "
+               << str_value << " to integer";
+    return false;
+  }
   return true;
 }
 
@@ -206,18 +215,30 @@ bool Prefs::FileStorage::DeleteTemporaryPrefs() {
 }
 
 bool Prefs::FileStorage::SwapPrefs() {
-  if (std::filesystem::exists(prefs_dir_.value())) {
-    std::filesystem::remove_all(prefs_dir_.value());
+  if (!utils::DeleteDirectory(prefs_dir_.value().c_str())) {
+    LOG(ERROR) << "Failed to remove prefs dir " << prefs_dir_;
+    return false;
   }
   if (rename(GetTemporaryDir().c_str(), prefs_dir_.value().c_str()) != 0) {
     LOG(ERROR) << "Error replacing prefs with prefs_tmp" << strerror(errno);
     return false;
   }
+  if (!utils::FsyncDirectory(
+          android::base::Dirname(prefs_dir_.value()).c_str())) {
+    PLOG(ERROR) << "Failed to fsync prefs parent dir after swapping prefs";
+  }
   return true;
 }
 
 bool Prefs::FileStorage::Init(const base::FilePath& prefs_dir) {
   prefs_dir_ = prefs_dir;
+  if (!std::filesystem::exists(prefs_dir_.value())) {
+    LOG(INFO) << "Prefs dir does not exist, possibly due to an interrupted "
+                 "transaction.";
+    if (std::filesystem::exists(GetTemporaryDir())) {
+      SwapPrefs();
+    }
+  }
 
   // Delete empty directories. Ignore errors when deleting empty directories.
   DeleteEmptyDirectories(prefs_dir_);
diff --git a/common/utils.cc b/common/utils.cc
index 15ee05c5..f0c045f2 100644
--- a/common/utils.cc
+++ b/common/utils.cc
@@ -412,6 +412,17 @@ bool SendFile(int out_fd, int in_fd, size_t count) {
   return true;
 }
 
+bool DeleteDirectory(const char* dirname) {
+  const std::string tmpdir = std::string(dirname) + "_deleted";
+  std::filesystem::remove_all(tmpdir);
+  if (rename(dirname, tmpdir.c_str()) != 0) {
+    PLOG(ERROR) << "Failed to rename " << dirname << " to " << tmpdir;
+    return false;
+  }
+  std::filesystem::remove_all(tmpdir);
+  return true;
+}
+
 bool FsyncDirectory(const char* dirname) {
   android::base::unique_fd fd(
       TEMP_FAILURE_RETRY(open(dirname, O_RDONLY | O_CLOEXEC)));
diff --git a/common/utils.h b/common/utils.h
index 0c8c13f2..6bb89f1e 100644
--- a/common/utils.h
+++ b/common/utils.h
@@ -162,6 +162,7 @@ off_t FileSize(int fd);
 bool SendFile(int out_fd, int in_fd, size_t count);
 
 bool FsyncDirectory(const char* dirname);
+bool DeleteDirectory(const char* dirname);
 bool WriteStringToFileAtomic(const std::string& path, std::string_view content);
 
 // Returns true if the file exists for sure. Returns false if it doesn't exist,