Skip to content

Commit 4d22ca2

Browse files
zhangxp1998Treehugger Robot
zhangxp1998
authored and
Treehugger Robot
committed
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
1 parent 0c71550 commit 4d22ca2

16 files changed

+40
-62
lines changed

‎payload_consumer/bzip_extent_writer.cc

+2-3
Original file line numberDiff line numberDiff line change
@@ -29,8 +29,7 @@ BzipExtentWriter::~BzipExtentWriter() {
2929
TEST_AND_RETURN(input_buffer_.empty());
3030
}
3131

32-
bool BzipExtentWriter::Init(FileDescriptorPtr fd,
33-
const RepeatedPtrField<Extent>& extents,
32+
bool BzipExtentWriter::Init(const RepeatedPtrField<Extent>& extents,
3433
uint32_t block_size) {
3534
// Init bzip2 stream
3635
int rc = BZ2_bzDecompressInit(&stream_,
@@ -39,7 +38,7 @@ bool BzipExtentWriter::Init(FileDescriptorPtr fd,
3938

4039
TEST_AND_RETURN_FALSE(rc == BZ_OK);
4140

42-
return next_->Init(fd, extents, block_size);
41+
return next_->Init(extents, block_size);
4342
}
4443

4544
bool BzipExtentWriter::Write(const void* bytes, size_t count) {

‎payload_consumer/bzip_extent_writer.h

+1-2
Original file line numberDiff line numberDiff line change
@@ -40,8 +40,7 @@ class BzipExtentWriter : public ExtentWriter {
4040
}
4141
~BzipExtentWriter() override;
4242

43-
bool Init(FileDescriptorPtr fd,
44-
const google::protobuf::RepeatedPtrField<Extent>& extents,
43+
bool Init(const google::protobuf::RepeatedPtrField<Extent>& extents,
4544
uint32_t block_size) override;
4645
bool Write(const void* bytes, size_t count) override;
4746

‎payload_consumer/bzip_extent_writer_unittest.cc

+4-7
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,6 @@
2929
#include "update_engine/common/utils.h"
3030
#include "update_engine/payload_generator/extent_ranges.h"
3131

32-
using google::protobuf::RepeatedPtrField;
3332
using std::min;
3433
using std::string;
3534
using std::vector;
@@ -64,9 +63,8 @@ TEST_F(BzipExtentWriterTest, SimpleTest) {
6463
0x22, 0x9c, 0x28, 0x48, 0x66, 0x61, 0xb8, 0xea, 0x00,
6564
};
6665

67-
BzipExtentWriter bzip_writer(std::make_unique<DirectExtentWriter>());
68-
EXPECT_TRUE(
69-
bzip_writer.Init(fd_, {extents.begin(), extents.end()}, kBlockSize));
66+
BzipExtentWriter bzip_writer(std::make_unique<DirectExtentWriter>(fd_));
67+
EXPECT_TRUE(bzip_writer.Init({extents.begin(), extents.end()}, kBlockSize));
7068
EXPECT_TRUE(bzip_writer.Write(test, sizeof(test)));
7169

7270
brillo::Blob buf;
@@ -97,9 +95,8 @@ TEST_F(BzipExtentWriterTest, ChunkedTest) {
9795

9896
vector<Extent> extents = {ExtentForBytes(kBlockSize, 0, kDecompressedLength)};
9997

100-
BzipExtentWriter bzip_writer(std::make_unique<DirectExtentWriter>());
101-
EXPECT_TRUE(
102-
bzip_writer.Init(fd_, {extents.begin(), extents.end()}, kBlockSize));
98+
BzipExtentWriter bzip_writer(std::make_unique<DirectExtentWriter>(fd_));
99+
EXPECT_TRUE(bzip_writer.Init({extents.begin(), extents.end()}, kBlockSize));
103100

104101
brillo::Blob original_compressed_data = compressed_data;
105102
for (brillo::Blob::size_type i = 0; i < compressed_data.size();

‎payload_consumer/extent_writer.h

+3-6
Original file line numberDiff line numberDiff line change
@@ -38,8 +38,7 @@ class ExtentWriter {
3838
virtual ~ExtentWriter() = default;
3939

4040
// Returns true on success.
41-
virtual bool Init(FileDescriptorPtr fd,
42-
const google::protobuf::RepeatedPtrField<Extent>& extents,
41+
virtual bool Init(const google::protobuf::RepeatedPtrField<Extent>& extents,
4342
uint32_t block_size) = 0;
4443

4544
// Returns true on success.
@@ -51,13 +50,11 @@ class ExtentWriter {
5150

5251
class DirectExtentWriter : public ExtentWriter {
5352
public:
54-
DirectExtentWriter() = default;
53+
explicit DirectExtentWriter(FileDescriptorPtr fd) : fd_(fd) {}
5554
~DirectExtentWriter() override = default;
5655

57-
bool Init(FileDescriptorPtr fd,
58-
const google::protobuf::RepeatedPtrField<Extent>& extents,
56+
bool Init(const google::protobuf::RepeatedPtrField<Extent>& extents,
5957
uint32_t block_size) override {
60-
fd_ = fd;
6158
block_size_ = block_size;
6259
extents_ = extents;
6360
cur_extent_ = extents_.begin();

‎payload_consumer/extent_writer_unittest.cc

+8-12
Original file line numberDiff line numberDiff line change
@@ -65,9 +65,8 @@ class ExtentWriterTest : public ::testing::Test {
6565
TEST_F(ExtentWriterTest, SimpleTest) {
6666
vector<Extent> extents = {ExtentForRange(1, 1)};
6767
const string bytes = "1234";
68-
DirectExtentWriter direct_writer;
69-
EXPECT_TRUE(
70-
direct_writer.Init(fd_, {extents.begin(), extents.end()}, kBlockSize));
68+
DirectExtentWriter direct_writer{fd_};
69+
EXPECT_TRUE(direct_writer.Init({extents.begin(), extents.end()}, kBlockSize));
7170
EXPECT_TRUE(direct_writer.Write(bytes.data(), bytes.size()));
7271

7372
EXPECT_EQ(static_cast<off_t>(kBlockSize + bytes.size()),
@@ -84,9 +83,8 @@ TEST_F(ExtentWriterTest, SimpleTest) {
8483

8584
TEST_F(ExtentWriterTest, ZeroLengthTest) {
8685
vector<Extent> extents = {ExtentForRange(1, 1)};
87-
DirectExtentWriter direct_writer;
88-
EXPECT_TRUE(
89-
direct_writer.Init(fd_, {extents.begin(), extents.end()}, kBlockSize));
86+
DirectExtentWriter direct_writer{fd_};
87+
EXPECT_TRUE(direct_writer.Init({extents.begin(), extents.end()}, kBlockSize));
9088
EXPECT_TRUE(direct_writer.Write(nullptr, 0));
9189
}
9290

@@ -109,9 +107,8 @@ void ExtentWriterTest::WriteAlignedExtents(size_t chunk_size,
109107
brillo::Blob data(kBlockSize * 3);
110108
test_utils::FillWithData(&data);
111109

112-
DirectExtentWriter direct_writer;
113-
EXPECT_TRUE(
114-
direct_writer.Init(fd_, {extents.begin(), extents.end()}, kBlockSize));
110+
DirectExtentWriter direct_writer{fd_};
111+
EXPECT_TRUE(direct_writer.Init({extents.begin(), extents.end()}, kBlockSize));
115112

116113
size_t bytes_written = 0;
117114
while (bytes_written < data.size()) {
@@ -150,9 +147,8 @@ TEST_F(ExtentWriterTest, SparseFileTest) {
150147
brillo::Blob data(17);
151148
test_utils::FillWithData(&data);
152149

153-
DirectExtentWriter direct_writer;
154-
EXPECT_TRUE(
155-
direct_writer.Init(fd_, {extents.begin(), extents.end()}, kBlockSize));
150+
DirectExtentWriter direct_writer{fd_};
151+
EXPECT_TRUE(direct_writer.Init({extents.begin(), extents.end()}, kBlockSize));
156152

157153
size_t bytes_written = 0;
158154
while (bytes_written < (block_count * kBlockSize)) {

‎payload_consumer/fake_extent_writer.h

+1-2
Original file line numberDiff line numberDiff line change
@@ -33,8 +33,7 @@ class FakeExtentWriter : public ExtentWriter {
3333
~FakeExtentWriter() override = default;
3434

3535
// ExtentWriter overrides.
36-
bool Init(FileDescriptorPtr /* fd */,
37-
const google::protobuf::RepeatedPtrField<Extent>& /* extents */,
36+
bool Init(const google::protobuf::RepeatedPtrField<Extent>& /* extents */,
3837
uint32_t /* block_size */) override {
3938
init_called_ = true;
4039
return true;

‎payload_consumer/file_descriptor_utils.cc

+2-2
Original file line numberDiff line numberDiff line change
@@ -82,8 +82,8 @@ bool CopyAndHashExtents(FileDescriptorPtr source,
8282
const RepeatedPtrField<Extent>& tgt_extents,
8383
uint64_t block_size,
8484
brillo::Blob* hash_out) {
85-
DirectExtentWriter writer;
86-
TEST_AND_RETURN_FALSE(writer.Init(target, tgt_extents, block_size));
85+
DirectExtentWriter writer{target};
86+
TEST_AND_RETURN_FALSE(writer.Init(tgt_extents, block_size));
8787
TEST_AND_RETURN_FALSE(utils::BlocksInExtents(src_extents) ==
8888
utils::BlocksInExtents(tgt_extents));
8989
TEST_AND_RETURN_FALSE(

‎payload_consumer/partition_writer.cc

+5-8
Original file line numberDiff line numberDiff line change
@@ -326,8 +326,7 @@ bool PartitionWriter::PerformReplaceOperation(const InstallOperation& operation,
326326
writer.reset(new XzExtentWriter(std::move(writer)));
327327
}
328328

329-
TEST_AND_RETURN_FALSE(
330-
writer->Init(target_fd_, operation.dst_extents(), block_size_));
329+
TEST_AND_RETURN_FALSE(writer->Init(operation.dst_extents(), block_size_));
331330
TEST_AND_RETURN_FALSE(writer->Write(data, operation.data_length()));
332331

333332
return true;
@@ -377,7 +376,7 @@ bool PartitionWriter::PerformSourceCopyOperation(
377376
const auto& partition_control = dynamic_control_;
378377

379378
InstallOperation buf;
380-
bool should_optimize = partition_control->OptimizeOperation(
379+
const bool should_optimize = partition_control->OptimizeOperation(
381380
partition.partition_name(), operation, &buf);
382381
const InstallOperation& optimized = should_optimize ? buf : operation;
383382

@@ -493,8 +492,7 @@ bool PartitionWriter::PerformSourceBsdiffOperation(
493492
utils::BlocksInExtents(operation.src_extents()) * block_size_);
494493

495494
auto writer = CreateBaseExtentWriter();
496-
TEST_AND_RETURN_FALSE(
497-
writer->Init(target_fd_, operation.dst_extents(), block_size_));
495+
TEST_AND_RETURN_FALSE(writer->Init(operation.dst_extents(), block_size_));
498496
auto dst_file = std::make_unique<BsdiffExtentFile>(
499497
std::move(writer),
500498
utils::BlocksInExtents(operation.dst_extents()) * block_size_);
@@ -522,8 +520,7 @@ bool PartitionWriter::PerformPuffDiffOperation(
522520
utils::BlocksInExtents(operation.src_extents()) * block_size_));
523521

524522
auto writer = CreateBaseExtentWriter();
525-
TEST_AND_RETURN_FALSE(
526-
writer->Init(target_fd_, operation.dst_extents(), block_size_));
523+
TEST_AND_RETURN_FALSE(writer->Init(operation.dst_extents(), block_size_));
527524
puffin::UniqueStreamPtr dst_stream(new PuffinExtentStream(
528525
std::move(writer),
529526
utils::BlocksInExtents(operation.dst_extents()) * block_size_));
@@ -658,7 +655,7 @@ void PartitionWriter::CheckpointUpdateProgress(size_t next_op_index) {
658655
}
659656

660657
std::unique_ptr<ExtentWriter> PartitionWriter::CreateBaseExtentWriter() {
661-
return std::make_unique<DirectExtentWriter>();
658+
return std::make_unique<DirectExtentWriter>(target_fd_);
662659
}
663660

664661
} // namespace chromeos_update_engine

‎payload_consumer/partition_writer_unittest.cc

+2-2
Original file line numberDiff line numberDiff line change
@@ -82,10 +82,10 @@ class PartitionWriterTest : public testing::Test {
8282
brillo::Blob PerformSourceCopyOp(const InstallOperation& op,
8383
const brillo::Blob blob_data) {
8484
ScopedTempFile source_partition("Blob-XXXXXX");
85-
DirectExtentWriter extent_writer;
8685
FileDescriptorPtr fd(new EintrSafeFileDescriptor());
86+
DirectExtentWriter extent_writer{fd};
8787
EXPECT_TRUE(fd->Open(source_partition.path().c_str(), O_RDWR));
88-
EXPECT_TRUE(extent_writer.Init(fd, op.src_extents(), kBlockSize));
88+
EXPECT_TRUE(extent_writer.Init(op.src_extents(), kBlockSize));
8989
EXPECT_TRUE(extent_writer.Write(blob_data.data(), blob_data.size()));
9090

9191
ScopedTempFile target_partition("Blob-XXXXXX");

‎payload_consumer/snapshot_extent_writer.cc

-1
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,6 @@ SnapshotExtentWriter::~SnapshotExtentWriter() {
3636
}
3737

3838
bool SnapshotExtentWriter::Init(
39-
FileDescriptorPtr /*fd*/,
4039
const google::protobuf::RepeatedPtrField<Extent>& extents,
4140
uint32_t block_size) {
4241
extents_ = extents;

‎payload_consumer/snapshot_extent_writer.h

+1-2
Original file line numberDiff line numberDiff line change
@@ -29,8 +29,7 @@ class SnapshotExtentWriter : public chromeos_update_engine::ExtentWriter {
2929
explicit SnapshotExtentWriter(android::snapshot::ICowWriter* cow_writer);
3030
~SnapshotExtentWriter();
3131
// Returns true on success.
32-
bool Init(FileDescriptorPtr fd,
33-
const google::protobuf::RepeatedPtrField<Extent>& extents,
32+
bool Init(const google::protobuf::RepeatedPtrField<Extent>& extents,
3433
uint32_t block_size) override;
3534
// Returns true on success.
3635
// This will construct a COW_REPLACE operation and forward it to CowWriter. It

‎payload_consumer/snapshot_extent_writer_unittest.cc

+3-3
Original file line numberDiff line numberDiff line change
@@ -107,7 +107,7 @@ void AddExtent(google::protobuf::RepeatedPtrField<Extent>* extents,
107107
TEST_F(SnapshotExtentWriterTest, BufferWrites) {
108108
google::protobuf::RepeatedPtrField<Extent> extents;
109109
AddExtent(&extents, 123, 1);
110-
writer_.Init(nullptr, extents, kBlockSize);
110+
writer_.Init(extents, kBlockSize);
111111

112112
std::vector<uint8_t> buf(kBlockSize, 0);
113113
buf[123] = 231;
@@ -130,7 +130,7 @@ TEST_F(SnapshotExtentWriterTest, NonBufferedWrites) {
130130
google::protobuf::RepeatedPtrField<Extent> extents;
131131
AddExtent(&extents, 123, 1);
132132
AddExtent(&extents, 125, 1);
133-
writer_.Init(nullptr, extents, kBlockSize);
133+
writer_.Init(extents, kBlockSize);
134134

135135
std::vector<uint8_t> buf(kBlockSize * 2, 0);
136136
buf[123] = 231;
@@ -153,7 +153,7 @@ TEST_F(SnapshotExtentWriterTest, WriteAcrossBlockBoundary) {
153153
google::protobuf::RepeatedPtrField<Extent> extents;
154154
AddExtent(&extents, 123, 1);
155155
AddExtent(&extents, 125, 2);
156-
writer_.Init(nullptr, extents, kBlockSize);
156+
writer_.Init(extents, kBlockSize);
157157

158158
std::vector<uint8_t> buf(kBlockSize * 3);
159159
std::memset(buf.data(), 0, buf.size());

‎payload_consumer/xz_extent_writer.cc

+2-3
Original file line numberDiff line numberDiff line change
@@ -57,12 +57,11 @@ XzExtentWriter::~XzExtentWriter() {
5757
TEST_AND_RETURN(input_buffer_.empty());
5858
}
5959

60-
bool XzExtentWriter::Init(FileDescriptorPtr fd,
61-
const RepeatedPtrField<Extent>& extents,
60+
bool XzExtentWriter::Init(const RepeatedPtrField<Extent>& extents,
6261
uint32_t block_size) {
6362
stream_ = xz_dec_init(XZ_DYNALLOC, kXzMaxDictSize);
6463
TEST_AND_RETURN_FALSE(stream_ != nullptr);
65-
return underlying_writer_->Init(fd, extents, block_size);
64+
return underlying_writer_->Init(extents, block_size);
6665
}
6766

6867
bool XzExtentWriter::Write(const void* bytes, size_t count) {

‎payload_consumer/xz_extent_writer.h

+1-2
Original file line numberDiff line numberDiff line change
@@ -39,8 +39,7 @@ class XzExtentWriter : public ExtentWriter {
3939
: underlying_writer_(std::move(underlying_writer)) {}
4040
~XzExtentWriter() override;
4141

42-
bool Init(FileDescriptorPtr fd,
43-
const google::protobuf::RepeatedPtrField<Extent>& extents,
42+
bool Init(const google::protobuf::RepeatedPtrField<Extent>& extents,
4443
uint32_t block_size) override;
4544
bool Write(const void* bytes, size_t count) override;
4645

‎payload_consumer/xz_extent_writer_unittest.cc

+3-3
Original file line numberDiff line numberDiff line change
@@ -87,7 +87,7 @@ class XzExtentWriterTest : public ::testing::Test {
8787
}
8888

8989
void WriteAll(const brillo::Blob& compressed) {
90-
EXPECT_TRUE(xz_writer_->Init(fd_, {}, 1024));
90+
EXPECT_TRUE(xz_writer_->Init({}, 1024));
9191
EXPECT_TRUE(xz_writer_->Write(compressed.data(), compressed.size()));
9292

9393
EXPECT_TRUE(fake_extent_writer_->InitCalled());
@@ -130,15 +130,15 @@ TEST_F(XzExtentWriterTest, CompressedDataBiggerThanTheBuffer) {
130130
}
131131

132132
TEST_F(XzExtentWriterTest, GarbageDataRejected) {
133-
EXPECT_TRUE(xz_writer_->Init(fd_, {}, 1024));
133+
EXPECT_TRUE(xz_writer_->Init({}, 1024));
134134
// The sample_data_ is an uncompressed string.
135135
EXPECT_FALSE(xz_writer_->Write(sample_data_.data(), sample_data_.size()));
136136
}
137137

138138
TEST_F(XzExtentWriterTest, PartialDataIsKept) {
139139
brillo::Blob compressed(std::begin(kCompressed30KiBofA),
140140
std::end(kCompressed30KiBofA));
141-
EXPECT_TRUE(xz_writer_->Init(fd_, {}, 1024));
141+
EXPECT_TRUE(xz_writer_->Init({}, 1024));
142142
for (uint8_t byte : compressed) {
143143
EXPECT_TRUE(xz_writer_->Write(&byte, 1));
144144
}

‎payload_generator/zip_unittest.cc

+2-4
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,6 @@
3333
using chromeos_update_engine::test_utils::kRandomString;
3434
using google::protobuf::RepeatedPtrField;
3535
using std::string;
36-
using std::vector;
3736

3837
namespace chromeos_update_engine {
3938

@@ -50,8 +49,7 @@ class MemoryExtentWriter : public ExtentWriter {
5049
}
5150
~MemoryExtentWriter() override = default;
5251

53-
bool Init(FileDescriptorPtr fd,
54-
const RepeatedPtrField<Extent>& extents,
52+
bool Init(const RepeatedPtrField<Extent>& extents,
5553
uint32_t block_size) override {
5654
return true;
5755
}
@@ -72,7 +70,7 @@ bool DecompressWithWriter(const brillo::Blob& in, brillo::Blob* out) {
7270
std::unique_ptr<ExtentWriter> writer(
7371
new W(std::make_unique<MemoryExtentWriter>(out)));
7472
// Init() parameters are ignored by the testing MemoryExtentWriter.
75-
bool ok = writer->Init(nullptr, {}, 1);
73+
bool ok = writer->Init({}, 1);
7674
ok = writer->Write(in.data(), in.size()) && ok;
7775
return ok;
7876
}

0 commit comments

Comments
 (0)