7 files changed, 98 insertions, 84 deletions

diff --git a/applypatch/applypatch.cpp b/applypatch/applypatch.cpp
index eb0a2a7b5..12e139930 100644
--- a/applypatch/applypatch.cpp
+++ b/applypatch/applypatch.cpp
@@ -23,7 +23,6 @@
 #include <stdlib.h>
 #include <string.h>
 #include <sys/stat.h>
-#include <sys/statfs.h>
 #include <sys/types.h>
 #include <unistd.h>
 
@@ -421,23 +420,6 @@ static size_t FileSink(const unsigned char* data, size_t len, int fd) {
   return done;
 }
 
-size_t FreeSpaceForFile(const std::string& filename) {
-  struct statfs sf;
-  if (statfs(filename.c_str(), &sf) != 0) {
-    PLOG(ERROR) << "Failed to statfs " << filename;
-    return -1;
-  }
-  return sf.f_bsize * sf.f_bavail;
-}
-
-int CacheSizeCheck(size_t bytes) {
-  if (MakeFreeSpaceOnCache(bytes) < 0) {
-    LOG(ERROR) << "Failed to make " << bytes << " bytes available on /cache";
-    return 1;
-  }
-  return 0;
-}
-
 int applypatch(const char* source_filename, const char* target_filename,
                const char* target_sha1_str, size_t /* target_size */,
                const std::vector<std::string>& patch_sha1s,
@@ -572,8 +554,8 @@ static int GenerateTarget(const FileContents& source_file, const std::unique_ptr
 
   CHECK(android::base::StartsWith(target_filename, "EMMC:"));
 
-  // We still write the original source to cache, in case the partition write is interrupted.
-  if (MakeFreeSpaceOnCache(source_file.data.size()) < 0) {
+  // We write the original source to cache, in case the partition write is interrupted.
+  if (!CheckAndFreeSpaceOnCache(source_file.data.size())) {
     LOG(ERROR) << "Not enough free space on /cache";
     return 1;
   }
diff --git a/applypatch/freecache.cpp b/applypatch/freecache.cpp
index dbd4b72b1..e4878655e 100644
--- a/applypatch/freecache.cpp
+++ b/applypatch/freecache.cpp
@@ -14,11 +14,9 @@
  * limitations under the License.
  */
 
-#include <ctype.h>
 #include <dirent.h>
 #include <errno.h>
-#include <error.h>
-#include <libgen.h>
+#include <inttypes.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
@@ -33,6 +31,7 @@
 #include <string>
 
 #include <android-base/file.h>
+#include <android-base/logging.h>
 #include <android-base/parseint.h>
 #include <android-base/stringprintf.h>
 #include <android-base/strings.h>
@@ -43,7 +42,7 @@
 static int EliminateOpenFiles(const std::string& dirname, std::set<std::string>* files) {
   std::unique_ptr<DIR, decltype(&closedir)> d(opendir("/proc"), closedir);
   if (!d) {
-    printf("error opening /proc: %s\n", strerror(errno));
+    PLOG(ERROR) << "Failed to open /proc";
     return -1;
   }
   struct dirent* de;
@@ -57,7 +56,7 @@ static int EliminateOpenFiles(const std::string& dirname, std::set<std::string>*
     struct dirent* fdde;
     std::unique_ptr<DIR, decltype(&closedir)> fdd(opendir(path.c_str()), closedir);
     if (!fdd) {
-      printf("error opening %s: %s\n", path.c_str(), strerror(errno));
+      PLOG(ERROR) << "Failed to open " << path;
       continue;
     }
     while ((fdde = readdir(fdd.get())) != 0) {
@@ -69,7 +68,7 @@ static int EliminateOpenFiles(const std::string& dirname, std::set<std::string>*
         link[count] = '\0';
         if (android::base::StartsWith(link, dirname)) {
           if (files->erase(link) > 0) {
-            printf("%s is open by %s\n", link, de->d_name);
+            LOG(INFO) << link << " is open by " << de->d_name;
           }
         }
       }
@@ -80,15 +79,14 @@ static int EliminateOpenFiles(const std::string& dirname, std::set<std::string>*
 
 static std::vector<std::string> FindExpendableFiles(
     const std::string& dirname, const std::function<bool(const std::string&)>& name_filter) {
-  std::set<std::string> files;
-
   std::unique_ptr<DIR, decltype(&closedir)> d(opendir(dirname.c_str()), closedir);
   if (!d) {
-    printf("error opening %s: %s\n", dirname.c_str(), strerror(errno));
+    PLOG(ERROR) << "Failed to open " << dirname;
     return {};
   }
 
   // Look for regular files in the directory (not in any subdirectories).
+  std::set<std::string> files;
   struct dirent* de;
   while ((de = readdir(d.get())) != 0) {
     std::string path = dirname + "/" + de->d_name;
@@ -110,7 +108,7 @@ static std::vector<std::string> FindExpendableFiles(
     }
   }
 
-  printf("%zu regular files in deletable directory\n", files.size());
+  LOG(INFO) << files.size() << " regular files in deletable directory";
   if (EliminateOpenFiles(dirname, &files) < 0) {
     return {};
   }
@@ -134,38 +132,60 @@ static unsigned int GetLogIndex(const std::string& log_name) {
   return std::numeric_limits<unsigned int>::max();
 }
 
-int MakeFreeSpaceOnCache(size_t bytes_needed) {
+// Returns the amount of free space (in bytes) on the filesystem containing filename, or -1 on
+// error.
+static int64_t FreeSpaceForFile(const std::string& filename) {
+  struct statfs sf;
+  if (statfs(filename.c_str(), &sf) == -1) {
+    PLOG(ERROR) << "Failed to statfs " << filename;
+    return -1;
+  }
+
+  int64_t free_space = static_cast<int64_t>(sf.f_bsize) * sf.f_bavail;
+  if (sf.f_bsize == 0 || free_space / sf.f_bsize != sf.f_bavail) {
+    LOG(ERROR) << "Invalid block size or overflow (sf.f_bsize " << sf.f_bsize << ", sf.f_bavail "
+               << sf.f_bavail << ")";
+    return -1;
+  }
+  return free_space;
+}
+
+bool CheckAndFreeSpaceOnCache(size_t bytes) {
 #ifndef __ANDROID__
-  // TODO (xunchang) implement a heuristic cache size check during host simulation.
-  printf("Skip making (%zu) bytes free space on /cache; program is running on host\n",
-         bytes_needed);
-  return 0;
+  // TODO(xunchang): Implement a heuristic cache size check during host simulation.
+  LOG(WARNING) << "Skipped making (" << bytes
+               << ") bytes free space on /cache; program is running on host";
+  return true;
 #endif
 
-  std::vector<std::string> dirs = { "/cache", Paths::Get().cache_log_directory() };
+  std::vector<std::string> dirs{ "/cache", Paths::Get().cache_log_directory() };
   for (const auto& dirname : dirs) {
-    if (RemoveFilesInDirectory(bytes_needed, dirname, FreeSpaceForFile)) {
-      return 0;
+    if (RemoveFilesInDirectory(bytes, dirname, FreeSpaceForFile)) {
+      return true;
     }
   }
 
-  return -1;
+  return false;
 }
 
 bool RemoveFilesInDirectory(size_t bytes_needed, const std::string& dirname,
-                            const std::function<size_t(const std::string&)>& space_checker) {
+                            const std::function<int64_t(const std::string&)>& space_checker) {
   struct stat st;
-  if (stat(dirname.c_str(), &st) != 0) {
-    error(0, errno, "Unable to free space on %s", dirname.c_str());
+  if (stat(dirname.c_str(), &st) == -1) {
+    PLOG(ERROR) << "Failed to stat " << dirname;
     return false;
   }
   if (!S_ISDIR(st.st_mode)) {
-    printf("%s is not a directory\n", dirname.c_str());
+    LOG(ERROR) << dirname << " is not a directory";
     return false;
   }
 
-  size_t free_now = space_checker(dirname);
-  printf("%zu bytes free on %s (%zu needed)\n", free_now, dirname.c_str(), bytes_needed);
+  int64_t free_now = space_checker(dirname);
+  if (free_now == -1) {
+    LOG(ERROR) << "Failed to check free space for " << dirname;
+    return false;
+  }
+  LOG(INFO) << free_now << " bytes free on " << dirname << " (" << bytes_needed << " needed)";
 
   if (free_now >= bytes_needed) {
     return true;
@@ -200,12 +220,16 @@ bool RemoveFilesInDirectory(size_t bytes_needed, const std::string& dirname,
 
   for (const auto& file : files) {
     if (unlink(file.c_str()) == -1) {
-      error(0, errno, "Failed to delete %s", file.c_str());
+      PLOG(ERROR) << "Failed to delete " << file;
       continue;
     }
 
     free_now = space_checker(dirname);
-    printf("deleted %s; now %zu bytes free\n", file.c_str(), free_now);
+    if (free_now == -1) {
+      LOG(ERROR) << "Failed to check free space for " << dirname;
+      return false;
+    }
+    LOG(INFO) << "Deleted " << file << "; now " << free_now << " bytes free";
     if (free_now >= bytes_needed) {
       return true;
     }
diff --git a/applypatch/include/applypatch/applypatch.h b/applypatch/include/applypatch/applypatch.h
index 92db59c3a..28dba7e68 100644
--- a/applypatch/include/applypatch/applypatch.h
+++ b/applypatch/include/applypatch/applypatch.h
@@ -40,14 +40,6 @@ using SinkFn = std::function<size_t(const unsigned char*, size_t)>;
 
 int ShowLicenses();
 
-// Returns the amount of free space (in bytes) on the filesystem containing filename, or -1 on
-// error. filename must exist.
-size_t FreeSpaceForFile(const std::string& filename);
-
-// Checks whether /cache partition has at least 'bytes'-byte free space. Returns 0 on having
-// sufficient space.
-int CacheSizeCheck(size_t bytes);
-
 // Parses a given string of 40 hex digits into 20-byte array 'digest'. 'str' may contain only the
 // digest or be of the form "<digest>:<anything>". Returns 0 on success, or -1 on any error.
 int ParseSha1(const std::string& str, uint8_t* digest);
@@ -117,10 +109,13 @@ int ApplyImagePatch(const unsigned char* old_data, size_t old_size, const Value&
 
 // freecache.cpp
 
-int MakeFreeSpaceOnCache(size_t bytes_needed);
+// Checks whether /cache partition has at least 'bytes'-byte free space. Returns true immediately
+// if so. Otherwise, it will try to free some space by removing older logs, checks again and
+// returns the checking result.
+bool CheckAndFreeSpaceOnCache(size_t bytes);
 
-// Removes the files in |dirname| until we have at least |bytes_needed| bytes of free space on
-// the partition. The size of the free space is returned by calling |space_checker|.
+// Removes the files in |dirname| until we have at least |bytes_needed| bytes of free space on the
+// partition. |space_checker| should return the size of the free space, or -1 on error.
 bool RemoveFilesInDirectory(size_t bytes_needed, const std::string& dirname,
-                            const std::function<size_t(const std::string&)>& space_checker);
+                            const std::function<int64_t(const std::string&)>& space_checker);
 #endif
diff --git a/tests/unit/applypatch_test.cpp b/tests/unit/applypatch_test.cpp
index 5cc03bc7b..0d4123b5e 100644
--- a/tests/unit/applypatch_test.cpp
+++ b/tests/unit/applypatch_test.cpp
@@ -16,6 +16,7 @@
 
 #include <dirent.h>
 #include <fcntl.h>
+#include <inttypes.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <sys/stat.h>
@@ -29,6 +30,7 @@
 #include <vector>
 
 #include <android-base/file.h>
+#include <android-base/logging.h>
 #include <android-base/stringprintf.h>
 #include <android-base/test_utils.h>
 #include <android-base/unique_fd.h>
@@ -162,27 +164,37 @@ class FreeCacheTest : public ::testing::Test {
     return file_list;
   }
 
-  static void AddFilesToDir(const std::string& dir, const std::vector<std::string>& files) {
+  void AddFilesToDir(const std::string& dir, const std::vector<std::string>& files) {
     std::string zeros(4096, 0);
     for (const auto& file : files) {
-      std::string path = dir + "/" + file;
-      ASSERT_TRUE(android::base::WriteStringToFile(zeros, path));
+      temporary_files_.push_back(dir + "/" + file);
+      ASSERT_TRUE(android::base::WriteStringToFile(zeros, temporary_files_.back()));
     }
   }
 
   void SetUp() override {
     Paths::Get().set_cache_log_directory(mock_log_dir.path);
+    temporary_files_.clear();
+  }
+
+  void TearDown() override {
+    for (const auto& file : temporary_files_) {
+      ASSERT_TRUE(android::base::RemoveFileIfExists(file));
+    }
   }
 
   // A mock method to calculate the free space. It assumes the partition has a total size of 40960
   // bytes and all files are 4096 bytes in size.
-  size_t MockFreeSpaceChecker(const std::string& dirname) {
+  static size_t MockFreeSpaceChecker(const std::string& dirname) {
     std::vector<std::string> files = FindFilesInDir(dirname);
     return PARTITION_SIZE - 4096 * files.size();
   }
 
   TemporaryDir mock_cache;
   TemporaryDir mock_log_dir;
+
+ private:
+  std::vector<std::string> temporary_files_;
 };
 
 TEST_F(FreeCacheTest, FreeCacheSmoke) {
@@ -191,14 +203,22 @@ TEST_F(FreeCacheTest, FreeCacheSmoke) {
   ASSERT_EQ(files, FindFilesInDir(mock_cache.path));
   ASSERT_EQ(4096 * 7, MockFreeSpaceChecker(mock_cache.path));
 
-  ASSERT_TRUE(RemoveFilesInDirectory(4096 * 9, mock_cache.path, [&](const std::string& dir) {
-    return this->MockFreeSpaceChecker(dir);
-  }));
+  ASSERT_TRUE(RemoveFilesInDirectory(4096 * 9, mock_cache.path, MockFreeSpaceChecker));
 
   ASSERT_EQ(std::vector<std::string>{ "file3" }, FindFilesInDir(mock_cache.path));
   ASSERT_EQ(4096 * 9, MockFreeSpaceChecker(mock_cache.path));
 }
 
+TEST_F(FreeCacheTest, FreeCacheFreeSpaceCheckerError) {
+  std::vector<std::string> files{ "file1", "file2", "file3" };
+  AddFilesToDir(mock_cache.path, files);
+  ASSERT_EQ(files, FindFilesInDir(mock_cache.path));
+  ASSERT_EQ(4096 * 7, MockFreeSpaceChecker(mock_cache.path));
+
+  ASSERT_FALSE(
+      RemoveFilesInDirectory(4096 * 9, mock_cache.path, [](const std::string&) { return -1; }));
+}
+
 TEST_F(FreeCacheTest, FreeCacheOpenFile) {
   std::vector<std::string> files = { "file1", "file2" };
   AddFilesToDir(mock_cache.path, files);
@@ -209,9 +229,7 @@ TEST_F(FreeCacheTest, FreeCacheOpenFile) {
   android::base::unique_fd fd(open(file1_path.c_str(), O_RDONLY));
 
   // file1 can't be deleted as it's opened by us.
-  ASSERT_FALSE(RemoveFilesInDirectory(4096 * 10, mock_cache.path, [&](const std::string& dir) {
-    return this->MockFreeSpaceChecker(dir);
-  }));
+  ASSERT_FALSE(RemoveFilesInDirectory(4096 * 10, mock_cache.path, MockFreeSpaceChecker));
 
   ASSERT_EQ(std::vector<std::string>{ "file1" }, FindFilesInDir(mock_cache.path));
 }
@@ -222,9 +240,7 @@ TEST_F(FreeCacheTest, FreeCacheLogsSmoke) {
   AddFilesToDir(mock_log_dir.path, log_files);
   ASSERT_EQ(4096 * 5, MockFreeSpaceChecker(mock_log_dir.path));
 
-  ASSERT_TRUE(RemoveFilesInDirectory(4096 * 8, mock_log_dir.path, [&](const std::string& dir) {
-    return this->MockFreeSpaceChecker(dir);
-  }));
+  ASSERT_TRUE(RemoveFilesInDirectory(4096 * 8, mock_log_dir.path, MockFreeSpaceChecker));
 
   // Logs with a higher index will be deleted first
   std::vector<std::string> expected = { "last_log", "last_log.1" };
@@ -238,9 +254,7 @@ TEST_F(FreeCacheTest, FreeCacheLogsStringComparison) {
   AddFilesToDir(mock_log_dir.path, log_files);
   ASSERT_EQ(4096 * 6, MockFreeSpaceChecker(mock_log_dir.path));
 
-  ASSERT_TRUE(RemoveFilesInDirectory(4096 * 9, mock_log_dir.path, [&](const std::string& dir) {
-    return this->MockFreeSpaceChecker(dir);
-  }));
+  ASSERT_TRUE(RemoveFilesInDirectory(4096 * 9, mock_log_dir.path, MockFreeSpaceChecker));
 
   // Logs with incorrect format will be deleted first; and the last_kmsg with the same index is
   // deleted before last_log.
@@ -255,9 +269,7 @@ TEST_F(FreeCacheTest, FreeCacheLogsOtherFiles) {
   AddFilesToDir(mock_log_dir.path, log_files);
   ASSERT_EQ(4096 * 5, MockFreeSpaceChecker(mock_log_dir.path));
 
-  ASSERT_FALSE(RemoveFilesInDirectory(4096 * 8, mock_log_dir.path, [&](const std::string& dir) {
-    return this->MockFreeSpaceChecker(dir);
-  }));
+  ASSERT_FALSE(RemoveFilesInDirectory(4096 * 8, mock_log_dir.path, MockFreeSpaceChecker));
 
   // Non log files in /cache/recovery won't be deleted.
   std::vector<std::string> expected = { "block.map", "command", "last_install" };
diff --git a/tests/unit/screen_ui_test.cpp b/tests/unit/screen_ui_test.cpp
index 2f4b7b09b..4c0a868f0 100644
--- a/tests/unit/screen_ui_test.cpp
+++ b/tests/unit/screen_ui_test.cpp
@@ -446,8 +446,9 @@ TEST_F(ScreenRecoveryUITest, LoadAnimation_MissingAnimation) {
   RETURN_IF_NO_GRAPHICS;
 
   ASSERT_TRUE(ui_->Init(kTestLocale));
-  TemporaryDir resource_dir;
-  Paths::Get().set_resource_dir(resource_dir.path);
+  // We need a dir that doesn't contain any animation. However, using TemporaryDir will give
+  // leftovers since this is a death test where TemporaryDir::~TemporaryDir() won't be called.
+  Paths::Get().set_resource_dir("/proc/self");
 
   ::testing::FLAGS_gtest_death_test_style = "threadsafe";
   ASSERT_EXIT(ui_->RunLoadAnimation(), ::testing::KilledBySignal(SIGABRT), "");
diff --git a/updater/blockimg.cpp b/updater/blockimg.cpp
index 6a6236b1b..2a2ab19a3 100644
--- a/updater/blockimg.cpp
+++ b/updater/blockimg.cpp
@@ -827,7 +827,7 @@ static int WriteStash(const std::string& base, const std::string& id, int blocks
     return -1;
   }
 
-  if (checkspace && CacheSizeCheck(blocks * BLOCKSIZE) != 0) {
+  if (checkspace && !CheckAndFreeSpaceOnCache(blocks * BLOCKSIZE)) {
     LOG(ERROR) << "not enough space to write stash";
     return -1;
   }
@@ -919,7 +919,7 @@ static int CreateStash(State* state, size_t maxblocks, const std::string& base)
       return -1;
     }
 
-    if (CacheSizeCheck(max_stash_size) != 0) {
+    if (!CheckAndFreeSpaceOnCache(max_stash_size)) {
       ErrorAbort(state, kStashCreationFailure, "not enough space for stash (%zu needed)",
                  max_stash_size);
       return -1;
@@ -951,7 +951,7 @@ static int CreateStash(State* state, size_t maxblocks, const std::string& base)
 
   if (max_stash_size > existing) {
     size_t needed = max_stash_size - existing;
-    if (CacheSizeCheck(needed) != 0) {
+    if (!CheckAndFreeSpaceOnCache(needed)) {
       ErrorAbort(state, kStashCreationFailure, "not enough space for stash (%zu more needed)",
                  needed);
       return -1;
diff --git a/updater/install.cpp b/updater/install.cpp
index ba7bd55b0..f9333459b 100644
--- a/updater/install.cpp
+++ b/updater/install.cpp
@@ -686,7 +686,7 @@ Value* ApplyPatchSpaceFn(const char* name, State* state,
   }
 
   // Skip the cache size check if the update is a retry.
-  if (state->is_retry || CacheSizeCheck(bytes) == 0) {
+  if (state->is_retry || CheckAndFreeSpaceOnCache(bytes)) {
     return StringValue("t");
   }
   return StringValue("");