Refactor extent writer to take filedescriptor in constructor

Functions which receive an instance of extent writer need to manually
pass fd to ExtentWriter via Init() call, which breaks separation of
concerns. It makes it hard for us to decouple InstallOp execution from
writing of data, as the execution unit must be aware of which fd to pass
to extent writer. In addition, many extents writer, such as snapshot
extent writer, simply ignores the fd parameter, which is a indication of
poor code structure.

To address the above issue, we pass FileDescriptorPtr via constructor if
needed. This way, whoever is "executing" InstallOps don't need to care
about where the output data is going, and whoever's writing the data
would be responsible for initializing an ExtentWriter.

Test: th
Change-Id: I6d1eabde085eefd55da9ecc0352d4a16ae458698

16 files changed, 40 insertions, 62 deletions

diff --git a/payload_consumer/bzip_extent_writer.cc b/payload_consumer/bzip_extent_writer.cc
index 0c25c71f..26fdc5f4 100644
--- a/payload_consumer/bzip_extent_writer.cc
+++ b/payload_consumer/bzip_extent_writer.cc
@@ -29,8 +29,7 @@ BzipExtentWriter::~BzipExtentWriter() {
   TEST_AND_RETURN(input_buffer_.empty());
 }
 
-bool BzipExtentWriter::Init(FileDescriptorPtr fd,
-                            const RepeatedPtrField<Extent>& extents,
+bool BzipExtentWriter::Init(const RepeatedPtrField<Extent>& extents,
                             uint32_t block_size) {
   // Init bzip2 stream
   int rc = BZ2_bzDecompressInit(&stream_,
@@ -39,7 +38,7 @@ bool BzipExtentWriter::Init(FileDescriptorPtr fd,
 
   TEST_AND_RETURN_FALSE(rc == BZ_OK);
 
-  return next_->Init(fd, extents, block_size);
+  return next_->Init(extents, block_size);
 }
 
 bool BzipExtentWriter::Write(const void* bytes, size_t count) {
diff --git a/payload_consumer/bzip_extent_writer.h b/payload_consumer/bzip_extent_writer.h
index ec181a78..38c041a8 100644
--- a/payload_consumer/bzip_extent_writer.h
+++ b/payload_consumer/bzip_extent_writer.h
@@ -40,8 +40,7 @@ class BzipExtentWriter : public ExtentWriter {
   }
   ~BzipExtentWriter() override;
 
-  bool Init(FileDescriptorPtr fd,
-            const google::protobuf::RepeatedPtrField<Extent>& extents,
+  bool Init(const google::protobuf::RepeatedPtrField<Extent>& extents,
             uint32_t block_size) override;
   bool Write(const void* bytes, size_t count) override;
 
diff --git a/payload_consumer/bzip_extent_writer_unittest.cc b/payload_consumer/bzip_extent_writer_unittest.cc
index b5870401..c93545ab 100644
--- a/payload_consumer/bzip_extent_writer_unittest.cc
+++ b/payload_consumer/bzip_extent_writer_unittest.cc
@@ -29,7 +29,6 @@
 #include "update_engine/common/utils.h"
 #include "update_engine/payload_generator/extent_ranges.h"
 
-using google::protobuf::RepeatedPtrField;
 using std::min;
 using std::string;
 using std::vector;
@@ -64,9 +63,8 @@ TEST_F(BzipExtentWriterTest, SimpleTest) {
       0x22, 0x9c, 0x28, 0x48, 0x66, 0x61, 0xb8, 0xea, 0x00,
   };
 
-  BzipExtentWriter bzip_writer(std::make_unique<DirectExtentWriter>());
-  EXPECT_TRUE(
-      bzip_writer.Init(fd_, {extents.begin(), extents.end()}, kBlockSize));
+  BzipExtentWriter bzip_writer(std::make_unique<DirectExtentWriter>(fd_));
+  EXPECT_TRUE(bzip_writer.Init({extents.begin(), extents.end()}, kBlockSize));
   EXPECT_TRUE(bzip_writer.Write(test, sizeof(test)));
 
   brillo::Blob buf;
@@ -97,9 +95,8 @@ TEST_F(BzipExtentWriterTest, ChunkedTest) {
 
   vector<Extent> extents = {ExtentForBytes(kBlockSize, 0, kDecompressedLength)};
 
-  BzipExtentWriter bzip_writer(std::make_unique<DirectExtentWriter>());
-  EXPECT_TRUE(
-      bzip_writer.Init(fd_, {extents.begin(), extents.end()}, kBlockSize));
+  BzipExtentWriter bzip_writer(std::make_unique<DirectExtentWriter>(fd_));
+  EXPECT_TRUE(bzip_writer.Init({extents.begin(), extents.end()}, kBlockSize));
 
   brillo::Blob original_compressed_data = compressed_data;
   for (brillo::Blob::size_type i = 0; i < compressed_data.size();
diff --git a/payload_consumer/extent_writer.h b/payload_consumer/extent_writer.h
index 9e53561e..8b1b5326 100644
--- a/payload_consumer/extent_writer.h
+++ b/payload_consumer/extent_writer.h
@@ -38,8 +38,7 @@ class ExtentWriter {
   virtual ~ExtentWriter() = default;
 
   // Returns true on success.
-  virtual bool Init(FileDescriptorPtr fd,
-                    const google::protobuf::RepeatedPtrField<Extent>& extents,
+  virtual bool Init(const google::protobuf::RepeatedPtrField<Extent>& extents,
                     uint32_t block_size) = 0;
 
   // Returns true on success.
@@ -51,13 +50,11 @@ class ExtentWriter {
 
 class DirectExtentWriter : public ExtentWriter {
  public:
-  DirectExtentWriter() = default;
+  explicit DirectExtentWriter(FileDescriptorPtr fd) : fd_(fd) {}
   ~DirectExtentWriter() override = default;
 
-  bool Init(FileDescriptorPtr fd,
-            const google::protobuf::RepeatedPtrField<Extent>& extents,
+  bool Init(const google::protobuf::RepeatedPtrField<Extent>& extents,
             uint32_t block_size) override {
-    fd_ = fd;
     block_size_ = block_size;
     extents_ = extents;
     cur_extent_ = extents_.begin();
diff --git a/payload_consumer/extent_writer_unittest.cc b/payload_consumer/extent_writer_unittest.cc
index afebb1a9..5c67d3e8 100644
--- a/payload_consumer/extent_writer_unittest.cc
+++ b/payload_consumer/extent_writer_unittest.cc
@@ -65,9 +65,8 @@ class ExtentWriterTest : public ::testing::Test {
 TEST_F(ExtentWriterTest, SimpleTest) {
   vector<Extent> extents = {ExtentForRange(1, 1)};
   const string bytes = "1234";
-  DirectExtentWriter direct_writer;
-  EXPECT_TRUE(
-      direct_writer.Init(fd_, {extents.begin(), extents.end()}, kBlockSize));
+  DirectExtentWriter direct_writer{fd_};
+  EXPECT_TRUE(direct_writer.Init({extents.begin(), extents.end()}, kBlockSize));
   EXPECT_TRUE(direct_writer.Write(bytes.data(), bytes.size()));
 
   EXPECT_EQ(static_cast<off_t>(kBlockSize + bytes.size()),
@@ -84,9 +83,8 @@ TEST_F(ExtentWriterTest, SimpleTest) {
 
 TEST_F(ExtentWriterTest, ZeroLengthTest) {
   vector<Extent> extents = {ExtentForRange(1, 1)};
-  DirectExtentWriter direct_writer;
-  EXPECT_TRUE(
-      direct_writer.Init(fd_, {extents.begin(), extents.end()}, kBlockSize));
+  DirectExtentWriter direct_writer{fd_};
+  EXPECT_TRUE(direct_writer.Init({extents.begin(), extents.end()}, kBlockSize));
   EXPECT_TRUE(direct_writer.Write(nullptr, 0));
 }
 
@@ -109,9 +107,8 @@ void ExtentWriterTest::WriteAlignedExtents(size_t chunk_size,
   brillo::Blob data(kBlockSize * 3);
   test_utils::FillWithData(&data);
 
-  DirectExtentWriter direct_writer;
-  EXPECT_TRUE(
-      direct_writer.Init(fd_, {extents.begin(), extents.end()}, kBlockSize));
+  DirectExtentWriter direct_writer{fd_};
+  EXPECT_TRUE(direct_writer.Init({extents.begin(), extents.end()}, kBlockSize));
 
   size_t bytes_written = 0;
   while (bytes_written < data.size()) {
@@ -150,9 +147,8 @@ TEST_F(ExtentWriterTest, SparseFileTest) {
   brillo::Blob data(17);
   test_utils::FillWithData(&data);
 
-  DirectExtentWriter direct_writer;
-  EXPECT_TRUE(
-      direct_writer.Init(fd_, {extents.begin(), extents.end()}, kBlockSize));
+  DirectExtentWriter direct_writer{fd_};
+  EXPECT_TRUE(direct_writer.Init({extents.begin(), extents.end()}, kBlockSize));
 
   size_t bytes_written = 0;
   while (bytes_written < (block_count * kBlockSize)) {
diff --git a/payload_consumer/fake_extent_writer.h b/payload_consumer/fake_extent_writer.h
index 7b2b7ac3..680b1b3b 100644
--- a/payload_consumer/fake_extent_writer.h
+++ b/payload_consumer/fake_extent_writer.h
@@ -33,8 +33,7 @@ class FakeExtentWriter : public ExtentWriter {
   ~FakeExtentWriter() override = default;
 
   // ExtentWriter overrides.
-  bool Init(FileDescriptorPtr /* fd */,
-            const google::protobuf::RepeatedPtrField<Extent>& /* extents */,
+  bool Init(const google::protobuf::RepeatedPtrField<Extent>& /* extents */,
             uint32_t /* block_size */) override {
     init_called_ = true;
     return true;
diff --git a/payload_consumer/file_descriptor_utils.cc b/payload_consumer/file_descriptor_utils.cc
index 846cbd72..9a6a601e 100644
--- a/payload_consumer/file_descriptor_utils.cc
+++ b/payload_consumer/file_descriptor_utils.cc
@@ -82,8 +82,8 @@ bool CopyAndHashExtents(FileDescriptorPtr source,
                         const RepeatedPtrField<Extent>& tgt_extents,
                         uint64_t block_size,
                         brillo::Blob* hash_out) {
-  DirectExtentWriter writer;
-  TEST_AND_RETURN_FALSE(writer.Init(target, tgt_extents, block_size));
+  DirectExtentWriter writer{target};
+  TEST_AND_RETURN_FALSE(writer.Init(tgt_extents, block_size));
   TEST_AND_RETURN_FALSE(utils::BlocksInExtents(src_extents) ==
                         utils::BlocksInExtents(tgt_extents));
   TEST_AND_RETURN_FALSE(
diff --git a/payload_consumer/partition_writer.cc b/payload_consumer/partition_writer.cc
index 6f06dd2f..6f98ba36 100644
--- a/payload_consumer/partition_writer.cc
+++ b/payload_consumer/partition_writer.cc
@@ -326,8 +326,7 @@ bool PartitionWriter::PerformReplaceOperation(const InstallOperation& operation,
     writer.reset(new XzExtentWriter(std::move(writer)));
   }
 
-  TEST_AND_RETURN_FALSE(
-      writer->Init(target_fd_, operation.dst_extents(), block_size_));
+  TEST_AND_RETURN_FALSE(writer->Init(operation.dst_extents(), block_size_));
   TEST_AND_RETURN_FALSE(writer->Write(data, operation.data_length()));
 
   return true;
@@ -377,7 +376,7 @@ bool PartitionWriter::PerformSourceCopyOperation(
   const auto& partition_control = dynamic_control_;
 
   InstallOperation buf;
-  bool should_optimize = partition_control->OptimizeOperation(
+  const bool should_optimize = partition_control->OptimizeOperation(
       partition.partition_name(), operation, &buf);
   const InstallOperation& optimized = should_optimize ? buf : operation;
 
@@ -493,8 +492,7 @@ bool PartitionWriter::PerformSourceBsdiffOperation(
       utils::BlocksInExtents(operation.src_extents()) * block_size_);
 
   auto writer = CreateBaseExtentWriter();
-  TEST_AND_RETURN_FALSE(
-      writer->Init(target_fd_, operation.dst_extents(), block_size_));
+  TEST_AND_RETURN_FALSE(writer->Init(operation.dst_extents(), block_size_));
   auto dst_file = std::make_unique<BsdiffExtentFile>(
       std::move(writer),
       utils::BlocksInExtents(operation.dst_extents()) * block_size_);
@@ -522,8 +520,7 @@ bool PartitionWriter::PerformPuffDiffOperation(
       utils::BlocksInExtents(operation.src_extents()) * block_size_));
 
   auto writer = CreateBaseExtentWriter();
-  TEST_AND_RETURN_FALSE(
-      writer->Init(target_fd_, operation.dst_extents(), block_size_));
+  TEST_AND_RETURN_FALSE(writer->Init(operation.dst_extents(), block_size_));
   puffin::UniqueStreamPtr dst_stream(new PuffinExtentStream(
       std::move(writer),
       utils::BlocksInExtents(operation.dst_extents()) * block_size_));
@@ -658,7 +655,7 @@ void PartitionWriter::CheckpointUpdateProgress(size_t next_op_index) {
 }
 
 std::unique_ptr<ExtentWriter> PartitionWriter::CreateBaseExtentWriter() {
-  return std::make_unique<DirectExtentWriter>();
+  return std::make_unique<DirectExtentWriter>(target_fd_);
 }
 
 }  // namespace chromeos_update_engine
diff --git a/payload_consumer/partition_writer_unittest.cc b/payload_consumer/partition_writer_unittest.cc
index 91e5e26e..263f338e 100644
--- a/payload_consumer/partition_writer_unittest.cc
+++ b/payload_consumer/partition_writer_unittest.cc
@@ -82,10 +82,10 @@ class PartitionWriterTest : public testing::Test {
   brillo::Blob PerformSourceCopyOp(const InstallOperation& op,
                                    const brillo::Blob blob_data) {
     ScopedTempFile source_partition("Blob-XXXXXX");
-    DirectExtentWriter extent_writer;
     FileDescriptorPtr fd(new EintrSafeFileDescriptor());
+    DirectExtentWriter extent_writer{fd};
     EXPECT_TRUE(fd->Open(source_partition.path().c_str(), O_RDWR));
-    EXPECT_TRUE(extent_writer.Init(fd, op.src_extents(), kBlockSize));
+    EXPECT_TRUE(extent_writer.Init(op.src_extents(), kBlockSize));
     EXPECT_TRUE(extent_writer.Write(blob_data.data(), blob_data.size()));
 
     ScopedTempFile target_partition("Blob-XXXXXX");
diff --git a/payload_consumer/snapshot_extent_writer.cc b/payload_consumer/snapshot_extent_writer.cc
index c9e6f312..242e7260 100644
--- a/payload_consumer/snapshot_extent_writer.cc
+++ b/payload_consumer/snapshot_extent_writer.cc
@@ -36,7 +36,6 @@ SnapshotExtentWriter::~SnapshotExtentWriter() {
 }
 
 bool SnapshotExtentWriter::Init(
-    FileDescriptorPtr /*fd*/,
     const google::protobuf::RepeatedPtrField<Extent>& extents,
     uint32_t block_size) {
   extents_ = extents;
diff --git a/payload_consumer/snapshot_extent_writer.h b/payload_consumer/snapshot_extent_writer.h
index 6d9fe7d6..c3c64cda 100644
--- a/payload_consumer/snapshot_extent_writer.h
+++ b/payload_consumer/snapshot_extent_writer.h
@@ -29,8 +29,7 @@ class SnapshotExtentWriter : public chromeos_update_engine::ExtentWriter {
   explicit SnapshotExtentWriter(android::snapshot::ICowWriter* cow_writer);
   ~SnapshotExtentWriter();
   // Returns true on success.
-  bool Init(FileDescriptorPtr fd,
-            const google::protobuf::RepeatedPtrField<Extent>& extents,
+  bool Init(const google::protobuf::RepeatedPtrField<Extent>& extents,
             uint32_t block_size) override;
   // Returns true on success.
   // This will construct a COW_REPLACE operation and forward it to CowWriter. It
diff --git a/payload_consumer/snapshot_extent_writer_unittest.cc b/payload_consumer/snapshot_extent_writer_unittest.cc
index 0e22482b..22010433 100644
--- a/payload_consumer/snapshot_extent_writer_unittest.cc
+++ b/payload_consumer/snapshot_extent_writer_unittest.cc
@@ -107,7 +107,7 @@ void AddExtent(google::protobuf::RepeatedPtrField<Extent>* extents,
 TEST_F(SnapshotExtentWriterTest, BufferWrites) {
   google::protobuf::RepeatedPtrField<Extent> extents;
   AddExtent(&extents, 123, 1);
-  writer_.Init(nullptr, extents, kBlockSize);
+  writer_.Init(extents, kBlockSize);
 
   std::vector<uint8_t> buf(kBlockSize, 0);
   buf[123] = 231;
@@ -130,7 +130,7 @@ TEST_F(SnapshotExtentWriterTest, NonBufferedWrites) {
   google::protobuf::RepeatedPtrField<Extent> extents;
   AddExtent(&extents, 123, 1);
   AddExtent(&extents, 125, 1);
-  writer_.Init(nullptr, extents, kBlockSize);
+  writer_.Init(extents, kBlockSize);
 
   std::vector<uint8_t> buf(kBlockSize * 2, 0);
   buf[123] = 231;
@@ -153,7 +153,7 @@ TEST_F(SnapshotExtentWriterTest, WriteAcrossBlockBoundary) {
   google::protobuf::RepeatedPtrField<Extent> extents;
   AddExtent(&extents, 123, 1);
   AddExtent(&extents, 125, 2);
-  writer_.Init(nullptr, extents, kBlockSize);
+  writer_.Init(extents, kBlockSize);
 
   std::vector<uint8_t> buf(kBlockSize * 3);
   std::memset(buf.data(), 0, buf.size());
diff --git a/payload_consumer/xz_extent_writer.cc b/payload_consumer/xz_extent_writer.cc
index a5b939db..a6483519 100644
--- a/payload_consumer/xz_extent_writer.cc
+++ b/payload_consumer/xz_extent_writer.cc
@@ -57,12 +57,11 @@ XzExtentWriter::~XzExtentWriter() {
   TEST_AND_RETURN(input_buffer_.empty());
 }
 
-bool XzExtentWriter::Init(FileDescriptorPtr fd,
-                          const RepeatedPtrField<Extent>& extents,
+bool XzExtentWriter::Init(const RepeatedPtrField<Extent>& extents,
                           uint32_t block_size) {
   stream_ = xz_dec_init(XZ_DYNALLOC, kXzMaxDictSize);
   TEST_AND_RETURN_FALSE(stream_ != nullptr);
-  return underlying_writer_->Init(fd, extents, block_size);
+  return underlying_writer_->Init(extents, block_size);
 }
 
 bool XzExtentWriter::Write(const void* bytes, size_t count) {
diff --git a/payload_consumer/xz_extent_writer.h b/payload_consumer/xz_extent_writer.h
index e022274e..70338f20 100644
--- a/payload_consumer/xz_extent_writer.h
+++ b/payload_consumer/xz_extent_writer.h
@@ -39,8 +39,7 @@ class XzExtentWriter : public ExtentWriter {
       : underlying_writer_(std::move(underlying_writer)) {}
   ~XzExtentWriter() override;
 
-  bool Init(FileDescriptorPtr fd,
-            const google::protobuf::RepeatedPtrField<Extent>& extents,
+  bool Init(const google::protobuf::RepeatedPtrField<Extent>& extents,
             uint32_t block_size) override;
   bool Write(const void* bytes, size_t count) override;
 
diff --git a/payload_consumer/xz_extent_writer_unittest.cc b/payload_consumer/xz_extent_writer_unittest.cc
index 34980a9a..5269dbc9 100644
--- a/payload_consumer/xz_extent_writer_unittest.cc
+++ b/payload_consumer/xz_extent_writer_unittest.cc
@@ -87,7 +87,7 @@ class XzExtentWriterTest : public ::testing::Test {
   }
 
   void WriteAll(const brillo::Blob& compressed) {
-    EXPECT_TRUE(xz_writer_->Init(fd_, {}, 1024));
+    EXPECT_TRUE(xz_writer_->Init({}, 1024));
     EXPECT_TRUE(xz_writer_->Write(compressed.data(), compressed.size()));
 
     EXPECT_TRUE(fake_extent_writer_->InitCalled());
@@ -130,7 +130,7 @@ TEST_F(XzExtentWriterTest, CompressedDataBiggerThanTheBuffer) {
 }
 
 TEST_F(XzExtentWriterTest, GarbageDataRejected) {
-  EXPECT_TRUE(xz_writer_->Init(fd_, {}, 1024));
+  EXPECT_TRUE(xz_writer_->Init({}, 1024));
   // The sample_data_ is an uncompressed string.
   EXPECT_FALSE(xz_writer_->Write(sample_data_.data(), sample_data_.size()));
 }
@@ -138,7 +138,7 @@ TEST_F(XzExtentWriterTest, GarbageDataRejected) {
 TEST_F(XzExtentWriterTest, PartialDataIsKept) {
   brillo::Blob compressed(std::begin(kCompressed30KiBofA),
                           std::end(kCompressed30KiBofA));
-  EXPECT_TRUE(xz_writer_->Init(fd_, {}, 1024));
+  EXPECT_TRUE(xz_writer_->Init({}, 1024));
   for (uint8_t byte : compressed) {
     EXPECT_TRUE(xz_writer_->Write(&byte, 1));
   }
diff --git a/payload_generator/zip_unittest.cc b/payload_generator/zip_unittest.cc
index e357b156..10e899b0 100644
--- a/payload_generator/zip_unittest.cc
+++ b/payload_generator/zip_unittest.cc
@@ -33,7 +33,6 @@
 using chromeos_update_engine::test_utils::kRandomString;
 using google::protobuf::RepeatedPtrField;
 using std::string;
-using std::vector;
 
 namespace chromeos_update_engine {
 
@@ -50,8 +49,7 @@ class MemoryExtentWriter : public ExtentWriter {
   }
   ~MemoryExtentWriter() override = default;
 
-  bool Init(FileDescriptorPtr fd,
-            const RepeatedPtrField<Extent>& extents,
+  bool Init(const RepeatedPtrField<Extent>& extents,
             uint32_t block_size) override {
     return true;
   }
@@ -72,7 +70,7 @@ bool DecompressWithWriter(const brillo::Blob& in, brillo::Blob* out) {
   std::unique_ptr<ExtentWriter> writer(
       new W(std::make_unique<MemoryExtentWriter>(out)));
   // Init() parameters are ignored by the testing MemoryExtentWriter.
-  bool ok = writer->Init(nullptr, {}, 1);
+  bool ok = writer->Init({}, 1);
   ok = writer->Write(in.data(), in.size()) && ok;
   return ok;
 }