RemoveFile returns Result and doesn't log

Logging from a library function is not ideal. This change also moves
all logging of failure to remove files to the call sites.

Author: jemoreira

base/cvd/cuttlefish/common/libs/utils/files.cpp

@@ -490,14 +490,13 @@ Result<std::string> RenameFile(const std::string& current_filepath,
   return target_filepath;
 }
 
-bool RemoveFile(const std::string& file) {
+Result<void> RemoveFile(const std::string& file) {
   LOG(DEBUG) << "Removing file " << file;
-  if (remove(file.c_str()) == 0) {
-    return true;
+  if (remove(file.c_str()) != 0) {
+    return CF_ERRF("Failed to remove file '{}' : {}", file,
+                   StrError(errno));
   }
-  LOG(ERROR) << "Failed to remove file " << file << " : "
-             << std::strerror(errno);
-  return false;
+  return {};
 }
 
 std::string ReadFile(const std::string& file) {

base/cvd/cuttlefish/common/libs/utils/files.h

@@ -66,7 +66,7 @@ Result<bool> IsDirectoryEmpty(const std::string& path);
 Result<void> RecursivelyRemoveDirectory(const std::string& path);
 bool Copy(const std::string& from, const std::string& to);
 off_t FileSize(const std::string& path);
-bool RemoveFile(const std::string& file);
+Result<void> RemoveFile(const std::string& file);
 Result<std::string> RenameFile(const std::string& current_filepath,
                                const std::string& target_filepath);
 std::string ReadFile(const std::string& file);

base/cvd/cuttlefish/host/commands/assemble_cvd/boot_config.cc

@@ -203,7 +203,9 @@ Result<void> PrepareBootEnvImage(
               "Unable to delete the old env image");
     LOG(DEBUG) << "Updated bootloader environment image.";
   } else {
-    RemoveFile(tmp_boot_env_image_path);
+    if (Result<void> res = RemoveFile(tmp_boot_env_image_path); !res.ok()) {
+      LOG(ERROR) << res.error().FormatForEnv();
+    }
   }
 
   return {};

base/cvd/cuttlefish/host/commands/assemble_cvd/boot_image_utils.cc

@@ -103,7 +103,9 @@ bool DeleteTmpFileIfNotChanged(const std::string& tmp_file, const std::string& c
     LOG(DEBUG) << "Updated " << current_file;
   } else {
     LOG(DEBUG) << "Didn't update " << current_file;
-    RemoveFile(tmp_file);
+    if (Result<void> res = RemoveFile(tmp_file); !res.ok()) {
+      LOG(ERROR) << res.error().FormatForEnv();
+    }
   }
 
   return true;

base/cvd/cuttlefish/host/commands/cvd/instances/instance_manager.cpp

@@ -189,10 +189,9 @@ Result<bool> InstanceManager::RemoveInstanceGroup(LocalInstanceGroup group) {
     if (instance.id() == 0) {
       continue;
     }
-    auto remove_res = lock_manager_.RemoveLockFile(instance.id());
-    if (!remove_res.ok()) {
+    if (auto res = lock_manager_.RemoveLockFile(instance.id()); !res.ok()) {
       LOG(ERROR) << "Failed to remove instance id lock: "
-                 << remove_res.error().FormatForEnv();
+                 << res.error().FormatForEnv();
     }
   }
   CF_EXPECT(RemoveGroupDirectory(group));
@@ -257,8 +256,14 @@ Result<void> InstanceManager::Clear() {
                    << res.error().FormatForEnv();
       }
     }
-    RemoveFile(group.HomeDir() + "/cuttlefish_runtime");
-    RemoveFile(group.HomeDir() + config_json_name);
+    std::string runtime_link = group.HomeDir() + "/cuttlefish_runtime";
+    if (Result<void> res = RemoveFile(runtime_link);!res.ok()) {
+      LOG(ERROR) << res.error().FormatForEnv();
+    }
+    std::string config_link = group.HomeDir() + config_json_name;
+    if (Result<void> res = RemoveFile(config_link);!res.ok()) {
+      LOG(ERROR) << res.error().FormatForEnv();
+    }
     RemoveGroupDirectory(group);
   }
   std::cerr << "Stopped all known instances\n";

base/cvd/cuttlefish/host/commands/cvd/instances/stop.cpp

@@ -153,11 +153,12 @@ Result<void> DeleteLockFile(const GroupProcInfo& group_info) {
     lock_file_path_stream << lock_file_prefix << id << ".lock";
     auto lock_file_path = lock_file_path_stream.str();
     if (FileExists(lock_file_path) && !DirectoryExists(lock_file_path)) {
-      if (RemoveFile(lock_file_path)) {
+      if (Result<void> res = RemoveFile(lock_file_path); res.ok()) {
         LOG(DEBUG) << "Reset the lock file: " << lock_file_path;
       } else {
         all_success = false;
-        LOG(ERROR) << "Failed to remove the lock file: " << lock_file_path;
+        LOG(ERROR) << "Failed to remove the lock file '" << lock_file_path
+                   << "': " << res.error().FormatForEnv();
       }
     }
   }
@@ -210,8 +211,8 @@ Result<void> DeleteAllOwnedInstanceLocks() {
                    << "' because it's not owned by current user";
       continue;
     }
-    if (!RemoveFile(lock_file_path)) {
-      LOG(ERROR) << "Failed to remove '" << lock_file_path << "'";
+    if (Result<void> res = RemoveFile(lock_file_path); !res.ok()) {
+      LOG(ERROR) << res.error().FormatForEnv();
     }
   }
   return {};

base/cvd/cuttlefish/host/commands/host_bugreport/main.cc

@@ -213,8 +213,9 @@ Result<void> CvdHostBugreportMain(int argc, char** argv) {
 
   LogError(WritableZip::Finalize(std::move(archive)));
 
-  if (!RemoveFile(log_filename)) {
-    LOG(INFO) << "Failed to remove host bug report log file: " << log_filename;
+  if (Result<void> res = RemoveFile(log_filename); !res.ok()) {
+    LOG(INFO) << "Failed to remove host bug report log file '" << log_filename
+              << "': " << res.error().FormatForEnv();
   }
 
   return {};

base/cvd/cuttlefish/host/commands/mkenvimage_slim/mkenvimage_slim.cc

@@ -79,7 +79,9 @@ Result<int> MkenvimageSlimMain(int argc, char** argv) {
     return CF_ERR("Couldn't open the output file " + FLAGS_output_path);
   } else if (FLAGS_env_size !=
              WriteAll(output_fd, (char*)env_buffer.data(), FLAGS_env_size)) {
-    RemoveFile(FLAGS_output_path);
+    if (Result<void> res = RemoveFile(FLAGS_output_path); !res.ok()) {
+      LOG(ERROR) << res.error().FormatForEnv();
+    }
     return CF_ERR("Couldn't complete write to " + FLAGS_output_path);
   }
 

base/cvd/cuttlefish/host/commands/run_cvd/server_loop_impl.cpp

@@ -415,7 +415,7 @@ void ServerLoopImpl::RestartRunCvd(int notification_fd) {
   // undesired behavior. Always try to delete the file "restore" if a restart is
   // requested.
   if (IsRestoring(config_)) {
-    CHECK(RemoveFile(config_.AssemblyPath("restore")));
+    CHECK(RemoveFile(config_.AssemblyPath("restore")).ok());
   }
   auto config_path = config_.AssemblyPath("cuttlefish_config.json");
   auto followup_stdin = SharedFD::MemfdCreate("pseudo_stdin");

base/cvd/cuttlefish/host/libs/config/data_image.cpp

@@ -217,7 +217,9 @@ Result<void> InitializeDataImage(
       LOG(DEBUG) << instance.data_image() << " exists. Not creating it.";
       return {};
     case DataImageAction::kCreateBlankImage: {
-      RemoveFile(instance.new_data_image());
+      if (Result<void> res = RemoveFile(instance.new_data_image()); !res.ok()) {
+        LOG(ERROR) << res.error().FormatForEnv();
+      }
       CF_EXPECT(instance.blank_data_image_mb() != 0,
                 "Expected `-blank_data_image_mb` to be set for "
                 "image creation.");