Use Result types in image_aggregator.cc

Databean · Databean · commit 064238e3a6c3 · 2025-07-30T00:40:02.000Z
This allows it to report errors without aborting, and reported errors
include stack trace information rather than only the line number of the
failing assertion.

image_aggregator at one time had a non-Cuttlefish dependency, which
was a reason to not use the Result type throughout. The dependent code
has since stopped using image_aggregator, and the image_aggregator code
has moved to a new repository (GitHub).

Bug: b/434998699
diff --git a/base/cvd/cuttlefish/host/commands/assemble_cvd/disk_builder.cpp b/base/cvd/cuttlefish/host/commands/assemble_cvd/disk_builder.cpp
@@ -211,14 +211,13 @@ Result<bool> DiskBuilder::BuildCompositeDiskIfNecessary() {
   if (vm_manager_ == VmmMode::kCrosvm) {
     CF_EXPECT(!header_path_.empty(), "No header path");
     CF_EXPECT(!footer_path_.empty(), "No footer path");
-    CreateCompositeDisk(partitions_, AbsolutePath(header_path_),
-                        AbsolutePath(footer_path_),
-                        AbsolutePath(composite_disk_path_),
-                        read_only_);
+    CF_EXPECT(CreateCompositeDisk(
+        partitions_, AbsolutePath(header_path_), AbsolutePath(footer_path_),
+        AbsolutePath(composite_disk_path_), read_only_));
   } else {
     // If this doesn't fit into the disk, it will fail while aggregating. The
     // aggregator doesn't maintain any sparse attributes.
-    AggregateImage(partitions_, AbsolutePath(composite_disk_path_));
+    CF_EXPECT(AggregateImage(partitions_, AbsolutePath(composite_disk_path_)));
   }
 
   using android::base::WriteStringToFile;
diff --git a/base/cvd/cuttlefish/host/libs/image_aggregator/image_aggregator.cc b/base/cvd/cuttlefish/host/libs/image_aggregator/image_aggregator.cc
@@ -141,60 +141,52 @@ struct PartitionInfo {
  * disk file, as the imag eflashing process also can handle Android-Sparse
  * images.
  */
-std::uint64_t ExpandedStorageSize(const std::string& file_path) {
+Result<uint64_t> ExpandedStorageSize(const std::string& file_path) {
   android::base::unique_fd fd(open(file_path.c_str(), O_RDONLY));
-  CHECK(fd.get() >= 0) << "Could not open \"" << file_path << "\""
-                       << strerror(errno);
+  CF_EXPECTF(fd.get() >= 0, "Could not open '{}': {}", file_path,
+             strerror(errno));
 
   std::uint64_t file_size = FileSize(file_path);
 
   // Try to read the disk in a nicely-aligned block size unless the whole file
   // is smaller.
   constexpr uint64_t MAGIC_BLOCK_SIZE = 4096;
   std::string magic(std::min(file_size, MAGIC_BLOCK_SIZE), '\0');
-  if (!android::base::ReadFully(fd, magic.data(), magic.size())) {
-    PLOG(FATAL) << "Fail to read: " << file_path;
-    return 0;
-  }
-  CHECK(lseek(fd, 0, SEEK_SET) != -1)
-      << "Fail to seek(\"" << file_path << "\")" << strerror(errno);
+
+  CF_EXPECTF(android::base::ReadFully(fd, magic.data(), magic.size()),
+             "Failed to read '{}': {}'", file_path, strerror(errno));
+
+  CF_EXPECTF(lseek(fd, 0, SEEK_SET) != -1, "Failed to lseek('{}'): {}",
+             file_path, strerror(errno));
 
   // Composite disk image
   if (android::base::StartsWith(magic, CDISK_MAGIC)) {
     // seek to the beginning of proto message
-    CHECK(lseek(fd, CDISK_MAGIC.size(), SEEK_SET) != -1)
-        << "Fail to seek(\"" << file_path << "\")" << strerror(errno);
+    CF_EXPECTF(lseek(fd, CDISK_MAGIC.size(), SEEK_SET) != -1,
+               "Failed to lseek('{}'): {}", file_path, strerror(errno));
+
     std::string message;
-    if (!android::base::ReadFdToString(fd, &message)) {
-      PLOG(FATAL) << "Fail to read(cdisk): " << file_path;
-      return 0;
-    }
+    CF_EXPECTF(android::base::ReadFdToString(fd, &message),
+               "Failed to read '{}': {}", file_path, strerror(errno));
+
     CompositeDisk cdisk;
-    if (!cdisk.ParseFromString(message)) {
-      PLOG(FATAL) << "Fail to parse(cdisk): " << file_path;
-      return 0;
-    }
+    CF_EXPECTF(cdisk.ParseFromString(message), "Failed to parse '{}': {}",
+               file_path, strerror(errno));
+
     return cdisk.length();
   }
 
   // Qcow2 image
   if (android::base::StartsWith(magic, Qcow2Image::MagicString())) {
-    Result<Qcow2Image> image = Qcow2Image::OpenExisting(file_path);
-    CHECK(image.ok()) << image.error().FormatForEnv();
-
-    Result<uint64_t> size = image->VirtualSizeBytes();
-    CHECK(size.ok()) << size.error().FormatForEnv();
-    return *size;
+    Qcow2Image image = CF_EXPECT(Qcow2Image::OpenExisting(file_path));
+    return CF_EXPECT(image.VirtualSizeBytes());
   }
 
   // Android-Sparse
   if (android::base::StartsWith(magic, AndroidSparseImage::MagicString())) {
-    Result<AndroidSparseImage> image = AndroidSparseImage::OpenExisting(file_path);
-    CHECK(image.ok()) << image.error().FormatForEnv();
-
-    Result<uint64_t> size = image->VirtualSizeBytes();
-    CHECK(size.ok()) << size.error().FormatForEnv();
-    return *size;
+    AndroidSparseImage image =
+        CF_EXPECT(AndroidSparseImage::OpenExisting(file_path));
+    return CF_EXPECT(image.VirtualSizeBytes());
   }
 
   // raw image file
@@ -240,7 +232,7 @@ class CompositeDiskBuilder {
   std::uint64_t next_disk_offset_ = sizeof(GptBeginning);
   bool read_only_ = true;
 
-  static const std::uint8_t* GetPartitionGUID(ImagePartition source) {
+  static Result<const std::uint8_t*> GetPartitionGUID(ImagePartition source) {
     // Due to some endianness mismatch in e2fsprogs GUID vs GPT, the GUIDs are
     // rearranged to make the right GUIDs appear in gdisk
     switch (source.type) {
@@ -256,25 +248,26 @@ class CompositeDiskBuilder {
             0xba, 0x4b, 0x0,  0xa0, 0xc9, 0x3e, 0xc9, 0x3b};
         return kEfiSystemPartitionGuid;
       default:
-        LOG(FATAL) << "Unknown partition type: " << (int) source.type;
+        return CF_ERR("Unknown partition type: " << (int)source.type);
     }
   }
 
 public:
   CompositeDiskBuilder(bool read_only) : read_only_(read_only) {}
 
-  void AppendPartition(ImagePartition source) {
-    uint64_t size = ExpandedStorageSize(source.image_file_path);
+  Result<void> AppendPartition(ImagePartition source) {
+    uint64_t size = CF_EXPECT(ExpandedStorageSize(source.image_file_path));
     auto aligned_size = AlignToPartitionSize(size);
-    CHECK(size == aligned_size || read_only_)
-        << "read-write partition " << source.label
-        << " is not aligned to the size of " << (1 << PARTITION_SIZE_SHIFT);
+    CF_EXPECTF(size == aligned_size || read_only_,
+               "read-write partition '{}' is not aligned to the size of '{}'",
+               source.label, (1 << PARTITION_SIZE_SHIFT));
     partitions_.push_back(PartitionInfo{
         .source = source,
         .size = size,
         .offset = next_disk_offset_,
     });
     next_disk_offset_ = next_disk_offset_ + aligned_size;
+    return {};
   }
 
   std::uint64_t DiskSize() const {
@@ -286,8 +279,8 @@ class CompositeDiskBuilder {
    * and `footer_file` will be populated with the contents of `Beginning()` and
    * `End()`.
    */
-  CompositeDisk MakeCompositeDiskSpec(const std::string& header_file,
-                                      const std::string& footer_file) const {
+  Result<CompositeDisk> MakeCompositeDiskSpec(
+      const std::string& header_file, const std::string& footer_file) const {
     CompositeDisk disk;
     disk.set_version(2);
     disk.set_length(DiskSize());
@@ -303,8 +296,9 @@ class CompositeDiskBuilder {
       component->set_read_write_capability(
           read_only_ ? ReadWriteCapability::READ_ONLY
                      : ReadWriteCapability::READ_WRITE);
-      uint64_t size = ExpandedStorageSize(partition.source.image_file_path);
-      CHECK(partition.size == size);
+      uint64_t size =
+          CF_EXPECT(ExpandedStorageSize(partition.source.image_file_path));
+      CF_EXPECT_EQ(partition.size, size);
       // When partition's aligned size differs from its (unaligned) size
       // reading the disk within the guest os would fail due to the gap.
       // Putting any disk bigger than 4K can fill this gap.
@@ -333,11 +327,8 @@ class CompositeDiskBuilder {
    * This method is not deterministic: some data is generated such as the disk
    * uuids.
    */
-  GptBeginning Beginning() const {
-    if (partitions_.size() > GPT_NUM_PARTITIONS) {
-      LOG(FATAL) << "Too many partitions: " << partitions_.size();
-      return {};
-    }
+  Result<GptBeginning> Beginning() const {
+    CF_EXPECT_LE(partitions_.size(), GPT_NUM_PARTITIONS, "Too many partitions");
     GptBeginning gpt = {
         .protective_mbr = ProtectiveMbr(DiskSize()),
         .header =
@@ -363,10 +354,9 @@ class CompositeDiskBuilder {
               (partition.offset + partition.AlignedSize()) / kSectorSize - 1,
       };
       SetRandomUuid(gpt.entries[i].unique_partition_guid);
-      const std::uint8_t* const type_guid = GetPartitionGUID(partition.source);
-      if (type_guid == nullptr) {
-        LOG(FATAL) << "Could not recognize partition guid";
-      }
+      const std::uint8_t* const type_guid =
+          CF_EXPECT(GetPartitionGUID(partition.source));
+      CF_EXPECT(type_guid != nullptr, "Could not recognize partition guid");
       memcpy(gpt.entries[i].partition_type_guid, type_guid, 16);
       std::u16string wide_name(partitions_[i].source.label.begin(),
                               partitions_[i].source.label.end());
@@ -399,29 +389,24 @@ class CompositeDiskBuilder {
   }
 };
 
-bool WriteBeginning(SharedFD out, const GptBeginning& beginning) {
+Result<void> WriteBeginning(SharedFD out, const GptBeginning& beginning) {
   std::string begin_str((const char*) &beginning, sizeof(GptBeginning));
-  if (WriteAll(out, begin_str) != begin_str.size()) {
-    LOG(ERROR) << "Could not write GPT beginning: " << out->StrError();
-    return false;
-  }
-  return true;
+  CF_EXPECT_EQ(WriteAll(out, begin_str), begin_str.size(),
+               "Could not write GPT beginning: " << out->StrError());
+  return {};
 }
 
-bool WriteEnd(SharedFD out, const GptEnd& end) {
+Result<void> WriteEnd(SharedFD out, const GptEnd& end) {
   auto disk_size = (end.footer.current_lba + 1) * kSectorSize;
   auto footer_start = (end.footer.last_usable_lba + 1) * kSectorSize;
   auto padding = disk_size - footer_start - sizeof(GptEnd);
   std::string padding_str(padding, '\0');
-  if (WriteAll(out, padding_str) != padding_str.size()) {
-    LOG(ERROR) << "Could not write GPT end padding: " << out->StrError();
-    return false;
-  }
-  if (WriteAllBinary(out, &end) != sizeof(end)) {
-    LOG(ERROR) << "Could not write GPT end contents: " << out->StrError();
-    return false;
-  }
-  return true;
+
+  CF_EXPECT_EQ(WriteAll(out, padding_str), padding_str.size(),
+               "Could not write GPT end padding: " << out->StrError());
+  CF_EXPECT_EQ(WriteAllBinary(out, &end), sizeof(end),
+               "Could not write GPT end contents: " << out->StrError());
+  return {};
 }
 
 /**
@@ -436,13 +421,11 @@ bool WriteEnd(SharedFD out, const GptEnd& end) {
  * crosvm has read-only support for Android-Sparse files, but QEMU does not
  * support them.
  */
-void DeAndroidSparse(const std::vector<ImagePartition>& partitions) {
+Result<void> DeAndroidSparse(const std::vector<ImagePartition>& partitions) {
   for (const auto& partition : partitions) {
-    Result<void> res = ForceRawImage(partition.image_file_path);
-    if (!res.ok()) {
-      LOG(FATAL) << "Desparse failed: " << res.error().FormatForEnv();
-    }
+    CF_EXPECT(ForceRawImage(partition.image_file_path));
   }
+  return {};
 }
 
 } // namespace
@@ -451,69 +434,80 @@ uint64_t AlignToPartitionSize(uint64_t size) {
   return AlignToPowerOf2(size, PARTITION_SIZE_SHIFT);
 }
 
-void AggregateImage(const std::vector<ImagePartition>& partitions,
-                    const std::string& output_path) {
-  DeAndroidSparse(partitions);
+Result<void> AggregateImage(const std::vector<ImagePartition>& partitions,
+                            const std::string& output_path) {
+  CF_EXPECT(DeAndroidSparse(partitions));
+
   CompositeDiskBuilder builder(false);
   for (auto& partition : partitions) {
     builder.AppendPartition(partition);
   }
-  auto output = SharedFD::Creat(output_path, 0600);
-  auto beginning = builder.Beginning();
-  if (!WriteBeginning(output, beginning)) {
-    LOG(FATAL) << "Could not write GPT beginning to \"" << output_path
-               << "\": " << output->StrError();
-  }
+
+  SharedFD output = SharedFD::Creat(output_path, 0600);
+  CF_EXPECTF(output->IsOpen(), "{}", output->StrError());
+
+  GptBeginning beginning = CF_EXPECT(builder.Beginning());
+  CF_EXPECTF(WriteBeginning(output, beginning),
+             "Could not write GPT beginning to '{}': {}", output_path,
+             output->StrError());
+
   for (auto& disk : partitions) {
-    auto disk_fd = SharedFD::Open(disk.image_file_path, O_RDONLY);
+    SharedFD disk_fd = SharedFD::Open(disk.image_file_path, O_RDONLY);
+    CF_EXPECTF(disk_fd->IsOpen(), "{}", disk_fd->StrError());
+
     auto file_size = FileSize(disk.image_file_path);
-    if (!output->CopyFrom(*disk_fd, file_size)) {
-      LOG(FATAL) << "Could not copy from \"" << disk.image_file_path
-                 << "\" to \"" << output_path << "\": " << output->StrError();
-    }
+    CF_EXPECTF(output->CopyFrom(*disk_fd, file_size),
+               "Could not copy from '{}' to '{}': {}", disk.image_file_path,
+               output_path, output->StrError());
     // Handle disk images that are not aligned to PARTITION_SIZE_SHIFT
     std::uint64_t padding = AlignToPartitionSize(file_size) - file_size;
     std::string padding_str;
     padding_str.resize(padding, '\0');
-    if (WriteAll(output, padding_str) != padding_str.size()) {
-      LOG(FATAL) << "Could not write partition padding to \"" << output_path
-                 << "\": " << output->StrError();
-    }
-  }
-  if (!WriteEnd(output, builder.End(beginning))) {
-    LOG(FATAL) << "Could not write GPT end to \"" << output_path
-               << "\": " << output->StrError();
+
+    CF_EXPECTF(WriteAll(output, padding_str) != padding_str.size(),
+               "Could not write partition padding to '{}': {}", output_path,
+               output->StrError());
   }
+  CF_EXPECTF(WriteEnd(output, builder.End(beginning)),
+             "Could not write GPT end to '{}': {}", output_path,
+             output->StrError());
+  return {};
 };
 
-void CreateCompositeDisk(std::vector<ImagePartition> partitions,
-                         const std::string& header_file,
-                         const std::string& footer_file,
-                         const std::string& output_composite_path,
-                         bool read_only) {
-  DeAndroidSparse(partitions);
+Result<void> CreateCompositeDisk(std::vector<ImagePartition> partitions,
+                                 const std::string& header_file,
+                                 const std::string& footer_file,
+                                 const std::string& output_composite_path,
+                                 bool read_only) {
+  CF_EXPECT(DeAndroidSparse(partitions));
 
   CompositeDiskBuilder builder(read_only);
   for (auto& partition : partitions) {
     builder.AppendPartition(partition);
   }
-  auto header = SharedFD::Creat(header_file, 0600);
-  auto beginning = builder.Beginning();
-  if (!WriteBeginning(header, beginning)) {
-    LOG(FATAL) << "Could not write GPT beginning to \"" << header_file
-               << "\": " << header->StrError();
-  }
-  auto footer = SharedFD::Creat(footer_file, 0600);
-  if (!WriteEnd(footer, builder.End(beginning))) {
-    LOG(FATAL) << "Could not write GPT end to \"" << footer_file
-               << "\": " << footer->StrError();
-  }
-  auto composite_proto = builder.MakeCompositeDiskSpec(header_file, footer_file);
+  SharedFD header = SharedFD::Creat(header_file, 0600);
+  CF_EXPECTF(header->IsOpen(), "{}", header->StrError());
+
+  GptBeginning beginning = CF_EXPECT(builder.Beginning());
+  CF_EXPECTF(WriteBeginning(header, beginning),
+             "Could not write GPT beginning to '{}': {}", header_file,
+             header->StrError());
+
+  SharedFD footer = SharedFD::Creat(footer_file, 0600);
+  CF_EXPECTF(footer->IsOpen(), "{}", footer->StrError());
+
+  CF_EXPECTF(WriteEnd(footer, builder.End(beginning)),
+             "Could not write GPT end to '{}': {}", footer_file,
+             footer->StrError());
+  CompositeDisk composite_proto =
+      CF_EXPECT(builder.MakeCompositeDiskSpec(header_file, footer_file));
   std::ofstream composite(output_composite_path.c_str(),
                           std::ios::binary | std::ios::trunc);
   composite << CDISK_MAGIC;
   composite_proto.SerializeToOstream(&composite);
   composite.flush();
+
+  return {};
 }
 
 } // namespace cuttlefish
diff --git a/base/cvd/cuttlefish/host/libs/image_aggregator/image_aggregator.h b/base/cvd/cuttlefish/host/libs/image_aggregator/image_aggregator.h
