Refactor NewFile::CommitAllOrAbandon.

This refactoring creates a more generic function to back
NewFile::CommitAllOrAbandon, which can be used for moving any files.

The generic function will be used for moving Pre-reboot Dexopt staged
files.

Bug: 311377497
Test: m test-art-host-gtest-art_artd_tests
Change-Id: If17392557229d857743011942d0a666e925a0448

3 files changed, 104 insertions, 26 deletions

diff --git a/artd/file_utils.cc b/artd/file_utils.cc
index f3558534ba..1415efbdb3 100644
--- a/artd/file_utils.cc
+++ b/artd/file_utils.cc
@@ -26,6 +26,7 @@
 #include <string>
 #include <string_view>
 #include <system_error>
+#include <unordered_set>
 #include <utility>
 
 #include "aidl/com/android/server/art/FsPermission.h"
@@ -46,7 +47,7 @@ using ::aidl::com::android::server::art::FsPermission;
 using ::android::base::make_scope_guard;
 using ::android::base::Result;
 
-void UnlinkIfExists(const std::string& path) {
+void UnlinkIfExists(std::string_view path) {
   std::error_code ec;
   std::filesystem::remove(path, ec);
   if (ec) {
@@ -85,7 +86,6 @@ Result<void> NewFile::CommitOrAbandon() {
         "Failed to move new file '{}' to path '{}': {}", temp_path_, final_path_, ec.message());
   }
   cleanup.Disable();
-  committed_ = true;
   return {};
 }
 
@@ -122,15 +122,35 @@ void NewFile::Unlink() {
 
 Result<void> NewFile::CommitAllOrAbandon(const std::vector<NewFile*>& files_to_commit,
                                          const std::vector<std::string_view>& files_to_remove) {
+  std::vector<std::pair<std::string_view, std::string_view>> files_to_move;
+
+  auto cleanup = make_scope_guard([&]() {
+    for (NewFile* file : files_to_commit) {
+      file->Unlink();
+    }
+  });
+  for (NewFile* file : files_to_commit) {
+    OR_RETURN(file->Keep());
+    files_to_move.emplace_back(file->TempPath(), file->FinalPath());
+  }
+  cleanup.Disable();
+
+  return MoveAllOrAbandon(files_to_move, files_to_remove);
+}
+
+Result<void> MoveAllOrAbandon(
+    const std::vector<std::pair<std::string_view, std::string_view>>& files_to_move,
+    const std::vector<std::string_view>& files_to_remove) {
   std::vector<std::pair<std::string_view, std::string>> moved_files;
+  std::unordered_set<std::string_view> committed_files;
 
   auto cleanup = make_scope_guard([&]() {
-    // Clean up new files.
-    for (NewFile* new_file : files_to_commit) {
-      if (new_file->committed_) {
-        UnlinkIfExists(new_file->FinalPath());
+    // Clean up `files_to_move`.
+    for (const auto& [src_path, dst_path] : files_to_move) {
+      if (committed_files.find(dst_path) != committed_files.end()) {
+        UnlinkIfExists(dst_path);
       } else {
-        new_file->Cleanup();
+        UnlinkIfExists(src_path);
       }
     }
 
@@ -151,9 +171,9 @@ Result<void> NewFile::CommitAllOrAbandon(const std::vector<NewFile*>& files_to_c
 
   // Move old files to temporary locations.
   std::vector<std::string_view> all_files_to_remove;
-  all_files_to_remove.reserve(files_to_commit.size() + files_to_remove.size());
-  for (NewFile* file : files_to_commit) {
-    all_files_to_remove.push_back(file->FinalPath());
+  all_files_to_remove.reserve(files_to_move.size() + files_to_remove.size());
+  for (const auto& [src_path, dst_path] : files_to_move) {
+    all_files_to_remove.push_back(dst_path);
   }
   all_files_to_remove.insert(
       all_files_to_remove.end(), files_to_remove.begin(), files_to_remove.end());
@@ -168,7 +188,7 @@ Result<void> NewFile::CommitAllOrAbandon(const std::vector<NewFile*>& files_to_c
       return ErrnoErrorf("Old file '{}' is a directory", original_path);
     }
     if (std::filesystem::exists(status)) {
-      std::string temp_path = BuildTempPath(original_path, "XXXXXX");
+      std::string temp_path = NewFile::BuildTempPath(original_path, "XXXXXX");
       int fd = mkstemps(temp_path.data(), /*suffixlen=*/4);
       if (fd < 0) {
         return ErrnoErrorf("Failed to create temporary path for old file '{}'", original_path);
@@ -188,9 +208,14 @@ Result<void> NewFile::CommitAllOrAbandon(const std::vector<NewFile*>& files_to_c
     }
   }
 
-  // Commit new files.
-  for (NewFile* file : files_to_commit) {
-    OR_RETURN(file->CommitOrAbandon());
+  // Move `files_to_move`.
+  for (const auto& [src_path, dst_path] : files_to_move) {
+    std::error_code ec;
+    std::filesystem::rename(src_path, dst_path, ec);
+    if (ec) {
+      return Errorf("Failed to move file from '{}' to '{}': {}", src_path, dst_path, ec.message());
+    }
+    committed_files.insert(dst_path);
   }
 
   cleanup.Disable();
diff --git a/artd/file_utils.h b/artd/file_utils.h
index b5fd170676..a97f52c42a 100644
--- a/artd/file_utils.h
+++ b/artd/file_utils.h
@@ -113,7 +113,6 @@ class NewFile {
   std::string temp_path_;
   std::string temp_id_;
   aidl::com::android::server::art::FsPermission fs_permission_;
-  bool committed_ = false;
 };
 
 // Opens a file for reading.
@@ -129,6 +128,25 @@ mode_t DirFsPermissionToMode(const aidl::com::android::server::art::FsPermission
 android::base::Result<void> Chown(
     const std::string& path, const aidl::com::android::server::art::FsPermission& fs_permission);
 
+// Moves every file in `files_to_move` from a given location to another, replacing the existing file
+// at the destination if it exists, and removes files in `files_to_remove` in addition. Or abandons
+// all files in `files_to_move` and restores old files at best effort if any error occurs.
+//
+// This function does not accept duplicate paths. Passing duplicate paths to this function leads to
+// undefined behavior.
+//
+// Note: This function is NOT thread-safe. It is intended to be used in single-threaded code or in
+// cases where some race condition is acceptable.
+//
+// Usage:
+//
+// Move file at `path_1` to `path_2`, move file at `path_3` to `file_4`, and remove the file at
+// "path_5":
+//   MoveAllOrAbandon({{"path_1", "path_2"}, {"path_3", "path_4}}, {"path_5"});
+android::base::Result<void> MoveAllOrAbandon(
+    const std::vector<std::pair<std::string_view, std::string_view>>& files_to_move,
+    const std::vector<std::string_view>& files_to_remove = {});
+
 }  // namespace artd
 }  // namespace art
 
diff --git a/artd/file_utils_test.cc b/artd/file_utils_test.cc
index 8f79d5dce9..62a7e39fe9 100644
--- a/artd/file_utils_test.cc
+++ b/artd/file_utils_test.cc
@@ -57,19 +57,19 @@ void CheckContent(const std::string& path, const std::string& expected_content)
   EXPECT_EQ(actual_content, expected_content);
 }
 
-// A file that will always fail on `Commit`.
-class UncommittableFile : public NewFile {
+// A file that will always fail on `Keep`.
+class UnkeepableFile : public NewFile {
  public:
-  static Result<std::unique_ptr<UncommittableFile>> Create(const std::string& path,
-                                                           const FsPermission& fs_permission) {
+  static Result<std::unique_ptr<UnkeepableFile>> Create(const std::string& path,
+                                                        const FsPermission& fs_permission) {
     std::unique_ptr<NewFile> new_file = OR_RETURN(NewFile::Create(path, fs_permission));
-    return std::unique_ptr<UncommittableFile>(new UncommittableFile(std::move(*new_file)));
+    return std::unique_ptr<UnkeepableFile>(new UnkeepableFile(std::move(*new_file)));
   }
 
-  Result<void> Keep() override { return Error() << "Uncommittable file"; }
+  Result<void> Keep() override { return Error() << "Unkeepable file"; }
 
  private:
-  explicit UncommittableFile(NewFile&& other) : NewFile(std::move(other)) {}
+  explicit UnkeepableFile(NewFile&& other) : NewFile(std::move(other)) {}
 };
 
 class FileUtilsTest : public CommonArtTest {
@@ -247,7 +247,7 @@ TEST_F(FileUtilsTest, NewFileCommitAllReplacesMoreOldFiles) {
   EXPECT_FALSE(std::filesystem::exists(new_file_2->TempPath()));
 }
 
-TEST_F(FileUtilsTest, NewFileCommitAllFailedToCommit) {
+TEST_F(FileUtilsTest, NewFileCommitAllFailedToKeep) {
   std::string file_1_path = scratch_dir_->GetPath() + "/file_1";
   std::string file_2_path = scratch_dir_->GetPath() + "/file_2";
   std::string file_3_path = scratch_dir_->GetPath() + "/file_3";
@@ -257,15 +257,15 @@ TEST_F(FileUtilsTest, NewFileCommitAllFailedToCommit) {
   ASSERT_TRUE(WriteStringToFile("old_file_3", file_3_path));  // Extra file.
 
   std::unique_ptr<NewFile> new_file_1 = OR_FATAL(NewFile::Create(file_1_path, fs_permission_));
-  // Uncommittable file.
+  // Unkeepable file.
   std::unique_ptr<NewFile> new_file_2 =
-      OR_FATAL(UncommittableFile::Create(file_2_path, fs_permission_));
+      OR_FATAL(UnkeepableFile::Create(file_2_path, fs_permission_));
 
   ASSERT_TRUE(WriteStringToFd("new_file_1", new_file_1->Fd()));
   ASSERT_TRUE(WriteStringToFd("new_file_2", new_file_2->Fd()));
 
   EXPECT_THAT(NewFile::CommitAllOrAbandon({new_file_1.get(), new_file_2.get()}, {file_3_path}),
-              HasError(WithMessage("Uncommittable file")));
+              HasError(WithMessage("Unkeepable file")));
 
   // Old files are fine.
   CheckContent(file_1_path, "old_file_1");
@@ -277,6 +277,41 @@ TEST_F(FileUtilsTest, NewFileCommitAllFailedToCommit) {
   EXPECT_FALSE(std::filesystem::exists(new_file_2->TempPath()));
 }
 
+TEST_F(FileUtilsTest, NewFileCommitAllFailedToCommit) {
+  std::string dir_1_path = scratch_dir_->GetPath() + "/dir_1";
+  std::string dir_2_path = scratch_dir_->GetPath() + "/dir_2";
+
+  ASSERT_TRUE(std::filesystem::create_directory(dir_1_path));
+  ASSERT_TRUE(std::filesystem::create_directory(dir_2_path));
+
+  std::string file_1_path = dir_1_path + "/file_1";
+  std::string file_2_path = dir_2_path + "/file_2";
+
+  std::unique_ptr<NewFile> new_file_1 = OR_FATAL(NewFile::Create(file_1_path, fs_permission_));
+  std::unique_ptr<NewFile> new_file_2 = OR_FATAL(NewFile::Create(file_2_path, fs_permission_));
+
+  ASSERT_TRUE(WriteStringToFd("new_file_1", new_file_1->Fd()));
+  ASSERT_TRUE(WriteStringToFd("new_file_2", new_file_2->Fd()));
+
+  {
+    // Make `new_file_2` fail to commit.
+    auto scoped_inaccessible = ScopedInaccessible(dir_2_path);
+    std::filesystem::permissions(
+        dir_2_path, std::filesystem::perms::owner_exec, std::filesystem::perm_options::add);
+    auto scoped_unroot = ScopedUnroot();
+
+    EXPECT_THAT(NewFile::CommitAllOrAbandon({new_file_1.get(), new_file_2.get()}),
+                HasError(WithMessage(ContainsRegex("Failed to move file .*file_2.*"))));
+  }
+
+  // Files are abandoned at best effort. File 1 is abandoned, but file 2 cannot be abandoned due to
+  // permission denied.
+  EXPECT_FALSE(std::filesystem::exists(new_file_1->TempPath()));
+  EXPECT_FALSE(std::filesystem::exists(new_file_1->FinalPath()));
+  EXPECT_TRUE(std::filesystem::exists(new_file_2->TempPath()));
+  EXPECT_FALSE(std::filesystem::exists(new_file_2->FinalPath()));
+}
+
 TEST_F(FileUtilsTest, NewFileCommitAllFailedToMoveOldFile) {
   std::string file_1_path = scratch_dir_->GetPath() + "/file_1";
   std::string file_2_path = scratch_dir_->GetPath() + "/file_2";