Make update_engine reserve space for decompression via apexd am: b0ab865e4a

Original change: https://android-review.googlesource.com/c/platform/system/update_engine/+/1614853

Change-Id: Iefa9b70759279eab8bb6573df205840c3bec520d

7 files changed, 78 insertions, 148 deletions

diff --git a/aosp/apex_handler_android.cc b/aosp/apex_handler_android.cc
index cdbc9834..38ec410b 100644
--- a/aosp/apex_handler_android.cc
+++ b/aosp/apex_handler_android.cc
@@ -23,32 +23,15 @@
 
 namespace chromeos_update_engine {
 
-// Don't change this path... apexd relies on it.
-constexpr const char* kApexReserveSpaceDir = "/data/apex/ota_reserved";
+namespace {
 
-uint64_t ApexHandlerAndroid::CalculateSize(
-    const std::vector<ApexInfo>& apex_infos) const {
-  return CalculateSize(apex_infos, GetApexService());
-}
-
-uint64_t ApexHandlerAndroid::CalculateSize(
-    const std::vector<ApexInfo>& apex_infos,
-    android::sp<android::apex::IApexService> apex_service) const {
-  // The safest option is to allocate space for every compressed APEX
-  uint64_t size_required_default = 0;
-
-  // We might not need to decompress every APEX. Communicate with apexd to get
-  // accurate requirement.
-  int64_t size_from_apexd;
+android::apex::CompressedApexInfoList CreateCompressedApexInfoList(
+    const std::vector<ApexInfo>& apex_infos) {
   android::apex::CompressedApexInfoList compressed_apex_info_list;
-
   for (const auto& apex_info : apex_infos) {
     if (!apex_info.is_compressed()) {
       continue;
     }
-
-    size_required_default += apex_info.decompressed_size();
-
     android::apex::CompressedApexInfo compressed_apex_info;
     compressed_apex_info.moduleName = apex_info.package_name();
     compressed_apex_info.versionCode = apex_info.version();
@@ -56,33 +39,41 @@ uint64_t ApexHandlerAndroid::CalculateSize(
     compressed_apex_info_list.apexInfos.emplace_back(
         std::move(compressed_apex_info));
   }
-  if (size_required_default == 0 || apex_service == nullptr) {
-    return size_required_default;
+  return compressed_apex_info_list;
+}
+
+}  // namespace
+
+android::base::Result<uint64_t> ApexHandlerAndroid::CalculateSize(
+    const std::vector<ApexInfo>& apex_infos) const {
+  // We might not need to decompress every APEX. Communicate with apexd to get
+  // accurate requirement.
+  auto apex_service = GetApexService();
+  if (apex_service == nullptr) {
+    return android::base::Error() << "Failed to get hold of apexservice";
   }
 
+  auto compressed_apex_info_list = CreateCompressedApexInfoList(apex_infos);
+  int64_t size_from_apexd;
   auto result = apex_service->calculateSizeForCompressedApex(
       compressed_apex_info_list, &size_from_apexd);
   if (!result.isOk()) {
-    return size_required_default;
+    return android::base::Error()
+           << "Failed to get size required from apexservice";
   }
   return size_from_apexd;
 }
 
-bool ApexHandlerAndroid::AllocateSpace(const uint64_t size_required) const {
-  return AllocateSpace(size_required, kApexReserveSpaceDir);
-}
-
-bool ApexHandlerAndroid::AllocateSpace(const uint64_t size_required,
-                                       const std::string& dir_path) const {
-  if (size_required == 0) {
-    return true;
+bool ApexHandlerAndroid::AllocateSpace(
+    const std::vector<ApexInfo>& apex_infos) const {
+  auto apex_service = GetApexService();
+  if (apex_service == nullptr) {
+    return false;
   }
-  base::FilePath path{dir_path};
-  // The filename is not important, it just needs to be under
-  // kApexReserveSpaceDir. We call it "full.tmp" because the current space
-  // estimation is simply adding up all decompressed sizes.
-  path = path.Append("full.tmp");
-  return utils::ReserveStorageSpace(path.value().c_str(), size_required);
+  auto compressed_apex_info_list = CreateCompressedApexInfoList(apex_infos);
+  auto result =
+      apex_service->reserveSpaceForCompressedApex(compressed_apex_info_list);
+  return result.isOk();
 }
 
 android::sp<android::apex::IApexService> ApexHandlerAndroid::GetApexService()
diff --git a/aosp/apex_handler_android.h b/aosp/apex_handler_android.h
index aac1cd9c..00f3a80b 100644
--- a/aosp/apex_handler_android.h
+++ b/aosp/apex_handler_android.h
@@ -30,17 +30,12 @@ namespace chromeos_update_engine {
 
 class ApexHandlerAndroid : virtual public ApexHandlerInterface {
  public:
-  uint64_t CalculateSize(const std::vector<ApexInfo>& apex_infos) const;
-  bool AllocateSpace(const uint64_t size_required) const;
+  android::base::Result<uint64_t> CalculateSize(
+      const std::vector<ApexInfo>& apex_infos) const;
+  bool AllocateSpace(const std::vector<ApexInfo>& apex_infos) const;
 
  private:
-  friend class ApexHandlerAndroidTest;
   android::sp<android::apex::IApexService> GetApexService() const;
-  uint64_t CalculateSize(
-      const std::vector<ApexInfo>& apex_infos,
-      android::sp<android::apex::IApexService> apex_service) const;
-  bool AllocateSpace(const uint64_t size_required,
-                     const std::string& dir_path) const;
 };
 
 }  // namespace chromeos_update_engine
diff --git a/aosp/apex_handler_android_unittest.cc b/aosp/apex_handler_android_unittest.cc
index 3a99f79b..981ae9dd 100644
--- a/aosp/apex_handler_android_unittest.cc
+++ b/aosp/apex_handler_android_unittest.cc
@@ -29,81 +29,45 @@ namespace chromeos_update_engine {
 
 namespace fs = std::filesystem;
 
-class ApexHandlerAndroidTest : public ::testing::Test {
- protected:
-  ApexHandlerAndroidTest() = default;
-
-  android::sp<android::apex::IApexService> GetApexService() const {
-    return apex_handler_.GetApexService();
-  }
-
-  uint64_t CalculateSize(
-      const std::vector<ApexInfo>& apex_infos,
-      android::sp<android::apex::IApexService> apex_service) const {
-    return apex_handler_.CalculateSize(apex_infos, apex_service);
-  }
-
-  bool AllocateSpace(const uint64_t size_required,
-                     const std::string& dir_path) const {
-    return apex_handler_.AllocateSpace(size_required, dir_path);
-  }
-
-  ApexInfo CreateApexInfo(const std::string& package_name,
-                          int version,
-                          bool is_compressed,
-                          int decompressed_size) {
-    ApexInfo result;
-    result.set_package_name(package_name);
-    result.set_version(version);
-    result.set_is_compressed(is_compressed);
-    result.set_decompressed_size(decompressed_size);
-    return std::move(result);
-  }
-
-  ApexHandlerAndroid apex_handler_;
-};
+ApexInfo CreateApexInfo(const std::string& package_name,
+                        int version,
+                        bool is_compressed,
+                        int decompressed_size) {
+  ApexInfo result;
+  result.set_package_name(package_name);
+  result.set_version(version);
+  result.set_is_compressed(is_compressed);
+  result.set_decompressed_size(decompressed_size);
+  return std::move(result);
+}
 
-// TODO(b/172911822): Once apexd has more optimized response for CalculateSize,
-//  improve this test
-TEST_F(ApexHandlerAndroidTest, CalculateSize) {
+TEST(ApexHandlerAndroidTest, CalculateSize) {
+  ApexHandlerAndroid apex_handler;
   std::vector<ApexInfo> apex_infos;
-  ApexInfo compressed_apex_1 = CreateApexInfo("sample1", 1, true, 10);
-  ApexInfo compressed_apex_2 = CreateApexInfo("sample2", 2, true, 20);
+  ApexInfo compressed_apex_1 = CreateApexInfo("sample1", 1, true, 1);
+  ApexInfo compressed_apex_2 = CreateApexInfo("sample2", 2, true, 2);
+  ApexInfo uncompressed_apex = CreateApexInfo("uncompressed", 1, false, 4);
   apex_infos.push_back(compressed_apex_1);
   apex_infos.push_back(compressed_apex_2);
-  auto apex_service = GetApexService();
-  EXPECT_TRUE(apex_service != nullptr) << "Apexservice not found";
-  int required_size = CalculateSize(apex_infos, apex_service);
-  EXPECT_EQ(required_size, 30);
+  apex_infos.push_back(uncompressed_apex);
+  auto result = apex_handler.CalculateSize(apex_infos);
+  ASSERT_TRUE(result.ok());
+  EXPECT_EQ(*result, 3u);
 }
 
-TEST_F(ApexHandlerAndroidTest, AllocateSpace) {
-  // Allocating 0 space should be a no op
-  TemporaryDir td;
-  EXPECT_TRUE(AllocateSpace(0, td.path));
-  EXPECT_TRUE(fs::is_empty(td.path));
-
-  // Allocating non-zero space should create a file with tmp suffix
-  EXPECT_TRUE(AllocateSpace(2 << 20, td.path));
-  EXPECT_FALSE(fs::is_empty(td.path));
-  int num_of_file = 0;
-  for (const auto& entry : fs::directory_iterator(td.path)) {
-    num_of_file++;
-    EXPECT_TRUE(EndsWith(entry.path().string(), ".tmp"));
-    EXPECT_EQ(fs::file_size(entry.path()), 2u << 20);
-  }
-  EXPECT_EQ(num_of_file, 1);
+TEST(ApexHandlerAndroidTest, AllocateSpace) {
+  ApexHandlerAndroid apex_handler;
+  std::vector<ApexInfo> apex_infos;
+  ApexInfo compressed_apex_1 = CreateApexInfo("sample1", 1, true, 1);
+  ApexInfo compressed_apex_2 = CreateApexInfo("sample2", 2, true, 2);
+  ApexInfo uncompressed_apex = CreateApexInfo("uncompressed", 1, false, 4);
+  apex_infos.push_back(compressed_apex_1);
+  apex_infos.push_back(compressed_apex_2);
+  apex_infos.push_back(uncompressed_apex);
+  EXPECT_TRUE(apex_handler.AllocateSpace(apex_infos));
 
-  // AllocateSpace should be safe to call twice
-  EXPECT_TRUE(AllocateSpace(100, td.path));
-  EXPECT_FALSE(fs::is_empty(td.path));
-  num_of_file = 0;
-  for (const auto& entry : fs::directory_iterator(td.path)) {
-    num_of_file++;
-    EXPECT_TRUE(EndsWith(entry.path().string(), ".tmp"));
-    EXPECT_EQ(fs::file_size(entry.path()), 100u);
-  }
-  EXPECT_EQ(num_of_file, 1);
+  // Should be able to pass empty list
+  EXPECT_TRUE(apex_handler.AllocateSpace({}));
 }
 
 }  // namespace chromeos_update_engine
diff --git a/aosp/apex_handler_interface.h b/aosp/apex_handler_interface.h
index c3399b61..b9b6c96f 100644
--- a/aosp/apex_handler_interface.h
+++ b/aosp/apex_handler_interface.h
@@ -19,6 +19,8 @@
 
 #include <vector>
 
+#include <android-base/result.h>
+
 #include "update_engine/update_metadata.pb.h"
 
 namespace chromeos_update_engine {
@@ -26,9 +28,9 @@ namespace chromeos_update_engine {
 class ApexHandlerInterface {
  public:
   virtual ~ApexHandlerInterface() = default;
-  virtual uint64_t CalculateSize(
+  virtual android::base::Result<uint64_t> CalculateSize(
       const std::vector<ApexInfo>& apex_infos) const = 0;
-  virtual bool AllocateSpace(const uint64_t size_required) const = 0;
+  virtual bool AllocateSpace(const std::vector<ApexInfo>& apex_infos) const = 0;
 };
 
 }  // namespace chromeos_update_engine
diff --git a/aosp/update_attempter_android.cc b/aosp/update_attempter_android.cc
index c685855f..1080acba 100644
--- a/aosp/update_attempter_android.cc
+++ b/aosp/update_attempter_android.cc
@@ -321,7 +321,8 @@ bool UpdateAttempterAndroid::ApplyPayload(
     const vector<string>& key_value_pair_headers,
     brillo::ErrorPtr* error) {
   // update_engine state must be checked before modifying payload_fd_ otherwise
-  // already running update will be terminated (existing file descriptor will be closed)
+  // already running update will be terminated (existing file descriptor will be
+  // closed)
   if (status_ == UpdateStatus::UPDATED_NEED_REBOOT) {
     return LogAndSetError(
         error, FROM_HERE, "An update already applied, waiting for reboot");
@@ -983,7 +984,14 @@ uint64_t UpdateAttempterAndroid::AllocateSpaceForPayload(
                                    manifest.apex_info().end());
   uint64_t apex_size_required = 0;
   if (apex_handler_android_ != nullptr) {
-    apex_size_required = apex_handler_android_->CalculateSize(apex_infos);
+    auto result = apex_handler_android_->CalculateSize(apex_infos);
+    if (!result.ok()) {
+      LogAndSetError(error,
+                     FROM_HERE,
+                     "Failed to calculate size required for compressed APEX");
+      return 0;
+    }
+    apex_size_required = *result;
   }
 
   string payload_id = GetPayloadId(headers);
@@ -1006,7 +1014,7 @@ uint64_t UpdateAttempterAndroid::AllocateSpaceForPayload(
   }
 
   if (apex_size_required > 0 && apex_handler_android_ != nullptr &&
-      !apex_handler_android_->AllocateSpace(apex_size_required)) {
+      !apex_handler_android_->AllocateSpace(apex_infos)) {
     LOG(ERROR) << "Insufficient space for apex decompression: "
                << apex_size_required << " bytes";
     return apex_size_required;
diff --git a/common/utils.cc b/common/utils.cc
index f5532ffe..5dbb4453 100644
--- a/common/utils.cc
+++ b/common/utils.cc
@@ -548,31 +548,6 @@ bool SetBlockDeviceReadOnly(const string& device, bool read_only) {
   return true;
 }
 
-bool ReserveStorageSpace(const char* path, uint64_t size) {
-  int fd = HANDLE_EINTR(open(path, O_WRONLY | O_CREAT | O_TRUNC, 0600));
-
-  TEST_AND_RETURN_FALSE_ERRNO(fd >= 0);
-  ScopedFdCloser closer1{&fd};
-  if (ftruncate(fd, size) < 0) {
-    PLOG(WARNING) << "Failed to ftruncate " << path;
-  }
-  // 1MB buffer
-  std::vector<unsigned char> buffer(1 << 20);
-
-  while (size > 0) {
-    uint64_t bytes_to_write = std::min(size, (uint64_t)buffer.size());
-    if (!utils::WriteAll(fd, buffer.data(), bytes_to_write)) {
-      auto off = lseek64(fd, 0, SEEK_CUR);
-      PLOG(ERROR) << "Failed to write 0 to " << path << "offset: " << off
-                  << " size: " << size;
-      unlink(path);
-      return false;
-    }
-    size -= bytes_to_write;
-  }
-  return true;
-}
-
 bool MountFilesystem(const string& device,
                      const string& mountpoint,
                      unsigned long mountflags,  // NOLINT(runtime/int)
diff --git a/common/utils.h b/common/utils.h
index 9a278eb1..5f6e4757 100644
--- a/common/utils.h
+++ b/common/utils.h
@@ -23,7 +23,6 @@
 #include <unistd.h>
 
 #include <algorithm>
-#include <cstdint>
 #include <limits>
 #include <map>
 #include <memory>
@@ -182,10 +181,6 @@ std::string MakePartitionName(const std::string& disk_name, int partition_num);
 // in |read_only|. Return whether the operation succeeded.
 bool SetBlockDeviceReadOnly(const std::string& device, bool read_only);
 
-// Reserve |size| bytes on space on |path| by creating a file at |path| and
-// write 0s into it. Return true iff both creation and writing succeed.
-[[nodiscard]] bool ReserveStorageSpace(const char* path, uint64_t size);
-
 // Synchronously mount or unmount a filesystem. Return true on success.
 // When mounting, it will attempt to mount the device as the passed filesystem
 // type |type|, with the passed |flags| options. If |type| is empty, "ext2",