Remove ZeroPadExtentWriter.
We don't generate replace operations with trailing zero removed, so
we can remove ZeroPadExtentWriter and ExtentWriter::End().
Bug: 78792859
Test: update_engine_unittests
Test: brillo_update_payload verify
Change-Id: I3c36d80a9c4475fda0b32c86c6503ab8b179b88f
diff --git a/payload_consumer/bzip_extent_writer.cc b/payload_consumer/bzip_extent_writer.cc
index 7828589..8926047 100644
--- a/payload_consumer/bzip_extent_writer.cc
+++ b/payload_consumer/bzip_extent_writer.cc
@@ -26,6 +26,7 @@
BzipExtentWriter::~BzipExtentWriter() {
TEST_AND_RETURN(BZ2_bzDecompressEnd(&stream_) == BZ_OK);
+ TEST_AND_RETURN(input_buffer_.empty());
}
bool BzipExtentWriter::Init(FileDescriptorPtr fd,
@@ -86,9 +87,4 @@
return true;
}
-bool BzipExtentWriter::EndImpl() {
- TEST_AND_RETURN_FALSE(input_buffer_.empty());
- return next_->End();
-}
-
} // namespace chromeos_update_engine
diff --git a/payload_consumer/bzip_extent_writer.h b/payload_consumer/bzip_extent_writer.h
index 710727f..023db75 100644
--- a/payload_consumer/bzip_extent_writer.h
+++ b/payload_consumer/bzip_extent_writer.h
@@ -44,7 +44,6 @@
const google::protobuf::RepeatedPtrField<Extent>& extents,
uint32_t block_size) override;
bool Write(const void* bytes, size_t count) override;
- bool EndImpl() override;
private:
std::unique_ptr<ExtentWriter> next_; // The underlying ExtentWriter.
diff --git a/payload_consumer/bzip_extent_writer_unittest.cc b/payload_consumer/bzip_extent_writer_unittest.cc
index 4426876..c121e11 100644
--- a/payload_consumer/bzip_extent_writer_unittest.cc
+++ b/payload_consumer/bzip_extent_writer_unittest.cc
@@ -49,8 +49,6 @@
void TearDown() override {
fd_->Close();
}
- void WriteAlignedExtents(size_t chunk_size, size_t first_chunk_size);
- void TestZeroPad(bool aligned_size);
FileDescriptorPtr fd_;
test_utils::ScopedTempFile temp_file_{"BzipExtentWriterTest-file.XXXXXX"};
@@ -72,7 +70,6 @@
EXPECT_TRUE(
bzip_writer.Init(fd_, {extents.begin(), extents.end()}, kBlockSize));
EXPECT_TRUE(bzip_writer.Write(test, sizeof(test)));
- EXPECT_TRUE(bzip_writer.End());
brillo::Blob buf;
EXPECT_TRUE(utils::ReadFile(temp_file_.path(), &buf));
@@ -112,7 +109,6 @@
size_t this_chunk_size = min(kChunkSize, compressed_data.size() - i);
EXPECT_TRUE(bzip_writer.Write(&compressed_data[i], this_chunk_size));
}
- EXPECT_TRUE(bzip_writer.End());
// Check that the const input has not been clobbered.
test_utils::ExpectVectorsEq(original_compressed_data, compressed_data);
diff --git a/payload_consumer/delta_performer.cc b/payload_consumer/delta_performer.cc
index 7a19374..3cce4d2 100644
--- a/payload_consumer/delta_performer.cc
+++ b/payload_consumer/delta_performer.cc
@@ -1019,8 +1019,7 @@
}
// Setup the ExtentWriter stack based on the operation type.
- std::unique_ptr<ExtentWriter> writer = std::make_unique<ZeroPadExtentWriter>(
- std::make_unique<DirectExtentWriter>());
+ std::unique_ptr<ExtentWriter> writer = std::make_unique<DirectExtentWriter>();
if (operation.type() == InstallOperation::REPLACE_BZ) {
writer.reset(new BzipExtentWriter(std::move(writer)));
@@ -1031,7 +1030,6 @@
TEST_AND_RETURN_FALSE(
writer->Init(target_fd_, operation.dst_extents(), block_size_));
TEST_AND_RETURN_FALSE(writer->Write(buffer_.data(), operation.data_length()));
- TEST_AND_RETURN_FALSE(writer->End());
// Update buffer
DiscardBuffer(true, buffer_.size());
@@ -1391,12 +1389,7 @@
return true;
}
- bool Close() override {
- if (writer_ != nullptr) {
- TEST_AND_RETURN_FALSE(writer_->End());
- }
- return true;
- }
+ bool Close() override { return true; }
bool GetSize(uint64_t* size) override {
*size = size_;
@@ -1510,12 +1503,7 @@
return true;
}
- bool Close() override {
- if (!is_read_) {
- TEST_AND_RETURN_FALSE(writer_->End());
- }
- return true;
- }
+ bool Close() override { return true; }
private:
PuffinExtentStream(std::unique_ptr<ExtentReader> reader,
diff --git a/payload_consumer/delta_performer_unittest.cc b/payload_consumer/delta_performer_unittest.cc
index b0520e7..3cddee4 100644
--- a/payload_consumer/delta_performer_unittest.cc
+++ b/payload_consumer/delta_performer_unittest.cc
@@ -81,13 +81,16 @@
};
// Compressed data without checksum, generated with:
-// echo -n a | xz -9 --check=none | hexdump -v -e '" " 12/1 "0x%02x, " "\n"'
+// echo -n "a$(head -c 4095 /dev/zero)" | xz -9 --check=none |
+// hexdump -v -e '" " 12/1 "0x%02x, " "\n"'
const uint8_t kXzCompressedData[] = {
0xfd, 0x37, 0x7a, 0x58, 0x5a, 0x00, 0x00, 0x00, 0xff, 0x12, 0xd9, 0x41,
0x02, 0x00, 0x21, 0x01, 0x1c, 0x00, 0x00, 0x00, 0x10, 0xcf, 0x58, 0xcc,
- 0x01, 0x00, 0x00, 0x61, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01, 0x11, 0x01,
- 0xad, 0xa6, 0x58, 0x04, 0x06, 0x72, 0x9e, 0x7a, 0x01, 0x00, 0x00, 0x00,
- 0x00, 0x00, 0x59, 0x5a,
+ 0xe0, 0x0f, 0xff, 0x00, 0x1b, 0x5d, 0x00, 0x30, 0x80, 0x33, 0xff, 0xdf,
+ 0xff, 0x51, 0xd6, 0xaf, 0x90, 0x1c, 0x1b, 0x4c, 0xaa, 0x3d, 0x7b, 0x28,
+ 0xe4, 0x7a, 0x74, 0xbc, 0xe5, 0xa7, 0x33, 0x4e, 0xcf, 0x00, 0x00, 0x00,
+ 0x00, 0x01, 0x2f, 0x80, 0x20, 0x00, 0x00, 0x00, 0x92, 0x7c, 0x7b, 0x24,
+ 0xa8, 0x00, 0x0a, 0xfc, 0x02, 0x00, 0x00, 0x00, 0x00, 0x00, 0x59, 0x5a,
};
const uint8_t src_deflates[] = {
@@ -508,8 +511,8 @@
TEST_F(DeltaPerformerTest, ReplaceXzOperationTest) {
brillo::Blob xz_data(std::begin(kXzCompressedData),
std::end(kXzCompressedData));
- // The compressed xz data contains only a single "a", but the operation should
- // pad the rest of the two blocks with zeros.
+ // The compressed xz data contains a single "a" and padded with zero for the
+ // rest of the block.
brillo::Blob expected_data = brillo::Blob(4096, 0);
expected_data[0] = 'a';
diff --git a/payload_consumer/extent_writer.h b/payload_consumer/extent_writer.h
index 2c15861..9e53561 100644
--- a/payload_consumer/extent_writer.h
+++ b/payload_consumer/extent_writer.h
@@ -35,9 +35,7 @@
class ExtentWriter {
public:
ExtentWriter() = default;
- virtual ~ExtentWriter() {
- LOG_IF(ERROR, !end_called_) << "End() not called on ExtentWriter.";
- }
+ virtual ~ExtentWriter() = default;
// Returns true on success.
virtual bool Init(FileDescriptorPtr fd,
@@ -46,16 +44,6 @@
// Returns true on success.
virtual bool Write(const void* bytes, size_t count) = 0;
-
- // Should be called when all writing is complete. Returns true on success.
- // The fd is not closed. Caller is responsible for closing it.
- bool End() {
- end_called_ = true;
- return EndImpl();
- }
- virtual bool EndImpl() = 0;
- private:
- bool end_called_{false};
};
// DirectExtentWriter is probably the simplest ExtentWriter implementation.
@@ -76,7 +64,6 @@
return true;
}
bool Write(const void* bytes, size_t count) override;
- bool EndImpl() override { return true; }
private:
FileDescriptorPtr fd_{nullptr};
@@ -89,48 +76,6 @@
google::protobuf::RepeatedPtrField<Extent>::iterator cur_extent_;
};
-// Takes an underlying ExtentWriter to which all operations are delegated.
-// When End() is called, ZeroPadExtentWriter ensures that the total number
-// of bytes written is a multiple of block_size_. If not, it writes zeros
-// to pad as needed.
-
-class ZeroPadExtentWriter : public ExtentWriter {
- public:
- explicit ZeroPadExtentWriter(
- std::unique_ptr<ExtentWriter> underlying_extent_writer)
- : underlying_extent_writer_(std::move(underlying_extent_writer)) {}
- ~ZeroPadExtentWriter() override = default;
-
- bool Init(FileDescriptorPtr fd,
- const google::protobuf::RepeatedPtrField<Extent>& extents,
- uint32_t block_size) override {
- block_size_ = block_size;
- return underlying_extent_writer_->Init(fd, extents, block_size);
- }
- bool Write(const void* bytes, size_t count) override {
- if (underlying_extent_writer_->Write(bytes, count)) {
- bytes_written_mod_block_size_ += count;
- bytes_written_mod_block_size_ %= block_size_;
- return true;
- }
- return false;
- }
- bool EndImpl() override {
- if (bytes_written_mod_block_size_) {
- const size_t write_size = block_size_ - bytes_written_mod_block_size_;
- brillo::Blob zeros(write_size, 0);
- TEST_AND_RETURN_FALSE(underlying_extent_writer_->Write(zeros.data(),
- write_size));
- }
- return underlying_extent_writer_->End();
- }
-
- private:
- std::unique_ptr<ExtentWriter> underlying_extent_writer_;
- size_t block_size_{0};
- size_t bytes_written_mod_block_size_{0};
-};
-
} // namespace chromeos_update_engine
#endif // UPDATE_ENGINE_PAYLOAD_CONSUMER_EXTENT_WRITER_H_
diff --git a/payload_consumer/extent_writer_unittest.cc b/payload_consumer/extent_writer_unittest.cc
index 48b27cb..580c4a6 100644
--- a/payload_consumer/extent_writer_unittest.cc
+++ b/payload_consumer/extent_writer_unittest.cc
@@ -59,7 +59,6 @@
// resultant file should look like and ensure that the extent writer
// wrote the file correctly.
void WriteAlignedExtents(size_t chunk_size, size_t first_chunk_size);
- void TestZeroPad(bool aligned_size);
FileDescriptorPtr fd_;
test_utils::ScopedTempFile temp_file_{"ExtentWriterTest-file.XXXXXX"};
@@ -72,7 +71,6 @@
EXPECT_TRUE(
direct_writer.Init(fd_, {extents.begin(), extents.end()}, kBlockSize));
EXPECT_TRUE(direct_writer.Write(bytes.data(), bytes.size()));
- EXPECT_TRUE(direct_writer.End());
EXPECT_EQ(static_cast<off_t>(kBlockSize + bytes.size()),
utils::FileSize(temp_file_.path()));
@@ -92,7 +90,6 @@
EXPECT_TRUE(
direct_writer.Init(fd_, {extents.begin(), extents.end()}, kBlockSize));
EXPECT_TRUE(direct_writer.Write(nullptr, 0));
- EXPECT_TRUE(direct_writer.End());
}
TEST_F(ExtentWriterTest, OverflowExtentTest) {
@@ -127,7 +124,6 @@
EXPECT_TRUE(direct_writer.Write(&data[bytes_written], bytes_to_write));
bytes_written += bytes_to_write;
}
- EXPECT_TRUE(direct_writer.End());
EXPECT_EQ(static_cast<off_t>(data.size()),
utils::FileSize(temp_file_.path()));
@@ -146,50 +142,6 @@
ExpectVectorsEq(expected_file, result_file);
}
-TEST_F(ExtentWriterTest, ZeroPadNullTest) {
- TestZeroPad(true);
-}
-
-TEST_F(ExtentWriterTest, ZeroPadFillTest) {
- TestZeroPad(false);
-}
-
-void ExtentWriterTest::TestZeroPad(bool aligned_size) {
- vector<Extent> extents = {ExtentForRange(1, 1), ExtentForRange(0, 1)};
- brillo::Blob data(kBlockSize * 2);
- test_utils::FillWithData(&data);
-
- ZeroPadExtentWriter zero_pad_writer(std::make_unique<DirectExtentWriter>());
-
- EXPECT_TRUE(
- zero_pad_writer.Init(fd_, {extents.begin(), extents.end()}, kBlockSize));
- size_t bytes_to_write = data.size();
- const size_t missing_bytes = (aligned_size ? 0 : 9);
- bytes_to_write -= missing_bytes;
- fd_->Seek(kBlockSize - missing_bytes, SEEK_SET);
- EXPECT_EQ(3, fd_->Write("xxx", 3));
- ASSERT_TRUE(zero_pad_writer.Write(data.data(), bytes_to_write));
- EXPECT_TRUE(zero_pad_writer.End());
-
- EXPECT_EQ(static_cast<off_t>(data.size()),
- utils::FileSize(temp_file_.path()));
-
- brillo::Blob result_file;
- EXPECT_TRUE(utils::ReadFile(temp_file_.path(), &result_file));
-
- brillo::Blob expected_file;
- expected_file.insert(expected_file.end(),
- data.begin() + kBlockSize,
- data.begin() + kBlockSize * 2);
- expected_file.insert(expected_file.end(),
- data.begin(), data.begin() + kBlockSize);
- if (missing_bytes) {
- memset(&expected_file[kBlockSize - missing_bytes], 0, missing_bytes);
- }
-
- ExpectVectorsEq(expected_file, result_file);
-}
-
TEST_F(ExtentWriterTest, SparseFileTest) {
vector<Extent> extents = {ExtentForRange(1, 1),
ExtentForRange(kSparseHole, 2),
@@ -211,7 +163,6 @@
EXPECT_TRUE(direct_writer.Write(data.data(), bytes_to_write));
bytes_written += bytes_to_write;
}
- EXPECT_TRUE(direct_writer.End());
// check file size, then data inside
ASSERT_EQ(static_cast<off_t>(2 * kBlockSize),
diff --git a/payload_consumer/fake_extent_writer.h b/payload_consumer/fake_extent_writer.h
index 4418a9e..7b2b7ac 100644
--- a/payload_consumer/fake_extent_writer.h
+++ b/payload_consumer/fake_extent_writer.h
@@ -40,26 +40,20 @@
return true;
};
bool Write(const void* bytes, size_t count) override {
- if (!init_called_ || end_called_)
+ if (!init_called_)
return false;
written_data_.insert(written_data_.end(),
reinterpret_cast<const uint8_t*>(bytes),
reinterpret_cast<const uint8_t*>(bytes) + count);
return true;
}
- bool EndImpl() override {
- end_called_ = true;
- return true;
- }
// Fake methods.
bool InitCalled() { return init_called_; }
- bool EndCalled() { return end_called_; }
brillo::Blob WrittenData() { return written_data_; }
private:
bool init_called_{false};
- bool end_called_{false};
brillo::Blob written_data_;
DISALLOW_COPY_AND_ASSIGN(FakeExtentWriter);
diff --git a/payload_consumer/file_descriptor_utils.cc b/payload_consumer/file_descriptor_utils.cc
index ebfb977..846cbd7 100644
--- a/payload_consumer/file_descriptor_utils.cc
+++ b/payload_consumer/file_descriptor_utils.cc
@@ -88,7 +88,6 @@
utils::BlocksInExtents(tgt_extents));
TEST_AND_RETURN_FALSE(
CommonHashExtents(source, src_extents, &writer, block_size, hash_out));
- TEST_AND_RETURN_FALSE(writer.End());
return true;
}
diff --git a/payload_consumer/xz_extent_writer.cc b/payload_consumer/xz_extent_writer.cc
index 343ed80..835dcf7 100644
--- a/payload_consumer/xz_extent_writer.cc
+++ b/payload_consumer/xz_extent_writer.cc
@@ -52,6 +52,7 @@
XzExtentWriter::~XzExtentWriter() {
xz_dec_end(stream_);
+ TEST_AND_RETURN(input_buffer_.empty());
}
bool XzExtentWriter::Init(FileDescriptorPtr fd,
@@ -110,9 +111,4 @@
return true;
}
-bool XzExtentWriter::EndImpl() {
- TEST_AND_RETURN_FALSE(input_buffer_.empty());
- return underlying_writer_->End();
-}
-
} // namespace chromeos_update_engine
diff --git a/payload_consumer/xz_extent_writer.h b/payload_consumer/xz_extent_writer.h
index 5e50256..e022274 100644
--- a/payload_consumer/xz_extent_writer.h
+++ b/payload_consumer/xz_extent_writer.h
@@ -43,7 +43,6 @@
const google::protobuf::RepeatedPtrField<Extent>& extents,
uint32_t block_size) override;
bool Write(const void* bytes, size_t count) override;
- bool EndImpl() override;
private:
// The underlying ExtentWriter.
diff --git a/payload_consumer/xz_extent_writer_unittest.cc b/payload_consumer/xz_extent_writer_unittest.cc
index c8bcdf9..76a53a4 100644
--- a/payload_consumer/xz_extent_writer_unittest.cc
+++ b/payload_consumer/xz_extent_writer_unittest.cc
@@ -89,10 +89,8 @@
void WriteAll(const brillo::Blob& compressed) {
EXPECT_TRUE(xz_writer_->Init(fd_, {}, 1024));
EXPECT_TRUE(xz_writer_->Write(compressed.data(), compressed.size()));
- EXPECT_TRUE(xz_writer_->End());
EXPECT_TRUE(fake_extent_writer_->InitCalled());
- EXPECT_TRUE(fake_extent_writer_->EndCalled());
}
// Owned by |xz_writer_|. This object is invalidated after |xz_writer_| is
@@ -109,7 +107,6 @@
TEST_F(XzExtentWriterTest, CreateAndDestroy) {
// Test that no Init() or End() called doesn't crash the program.
EXPECT_FALSE(fake_extent_writer_->InitCalled());
- EXPECT_FALSE(fake_extent_writer_->EndCalled());
}
TEST_F(XzExtentWriterTest, CompressedSampleData) {
@@ -137,9 +134,6 @@
EXPECT_TRUE(xz_writer_->Init(fd_, {}, 1024));
// The sample_data_ is an uncompressed string.
EXPECT_FALSE(xz_writer_->Write(sample_data_.data(), sample_data_.size()));
- EXPECT_TRUE(xz_writer_->End());
-
- EXPECT_TRUE(fake_extent_writer_->EndCalled());
}
TEST_F(XzExtentWriterTest, PartialDataIsKept) {
@@ -149,7 +143,6 @@
for (uint8_t byte : compressed) {
EXPECT_TRUE(xz_writer_->Write(&byte, 1));
}
- EXPECT_TRUE(xz_writer_->End());
// The sample_data_ is an uncompressed string.
brillo::Blob expected_data(30 * 1024, 'a');