Transfer memory ownership when stacking ExtentWriters.
The ExtentWriter implementations are meant to be stacked in a way that
the underlying extent writer can't be reused. Calling End() on the top
of the stack will call End() on every ExtentWriter.
This patch enforces this design decision in the interface transfering
the ownership of the ExtentWriter when stacking them using a unique_ptr
instead of a plain pointer.
Bug: 23604708
Test: Updated unittests.
Change-Id: Ie3b5b9cbb3058359c487a783f6fb3c0ac65bde00
diff --git a/bzip_extent_writer.h b/bzip_extent_writer.h
index d854dd2..6941c66 100644
--- a/bzip_extent_writer.h
+++ b/bzip_extent_writer.h
@@ -18,6 +18,7 @@
#define UPDATE_ENGINE_BZIP_EXTENT_WRITER_H_
#include <bzlib.h>
+#include <memory>
#include <vector>
#include <chromeos/secure_blob.h>
@@ -33,19 +34,20 @@
class BzipExtentWriter : public ExtentWriter {
public:
- explicit BzipExtentWriter(ExtentWriter* next) : next_(next) {
+ explicit BzipExtentWriter(std::unique_ptr<ExtentWriter> next)
+ : next_(std::move(next)) {
memset(&stream_, 0, sizeof(stream_));
}
- ~BzipExtentWriter() {}
+ ~BzipExtentWriter() override = default;
bool Init(FileDescriptorPtr fd,
const std::vector<Extent>& extents,
- uint32_t block_size);
- bool Write(const void* bytes, size_t count);
- bool EndImpl();
+ uint32_t block_size) override;
+ bool Write(const void* bytes, size_t count) override;
+ bool EndImpl() override;
private:
- ExtentWriter* const next_; // The underlying ExtentWriter.
+ std::unique_ptr<ExtentWriter> next_; // The underlying ExtentWriter.
bz_stream stream_; // the libbz2 stream
chromeos::Blob input_buffer_;
};
diff --git a/bzip_extent_writer_unittest.cc b/bzip_extent_writer_unittest.cc
index 14e821a..986be9f 100644
--- a/bzip_extent_writer_unittest.cc
+++ b/bzip_extent_writer_unittest.cc
@@ -20,10 +20,14 @@
#include <sys/stat.h>
#include <sys/types.h>
#include <unistd.h>
+
#include <algorithm>
#include <string>
#include <vector>
+
+#include <chromeos/make_unique_ptr.h>
#include <gtest/gtest.h>
+
#include "update_engine/test_utils.h"
#include "update_engine/utils.h"
@@ -74,8 +78,8 @@
0x22, 0x9c, 0x28, 0x48, 0x66, 0x61, 0xb8, 0xea, 0x00,
};
- DirectExtentWriter direct_writer;
- BzipExtentWriter bzip_writer(&direct_writer);
+ BzipExtentWriter bzip_writer(
+ chromeos::make_unique_ptr(new DirectExtentWriter()));
EXPECT_TRUE(bzip_writer.Init(fd_, extents, kBlockSize));
EXPECT_TRUE(bzip_writer.Write(test, sizeof(test)));
EXPECT_TRUE(bzip_writer.End());
@@ -114,8 +118,8 @@
chromeos::Blob compressed_data;
EXPECT_TRUE(utils::ReadFile(compressed_path, &compressed_data));
- DirectExtentWriter direct_writer;
- BzipExtentWriter bzip_writer(&direct_writer);
+ BzipExtentWriter bzip_writer(
+ chromeos::make_unique_ptr(new DirectExtentWriter()));
EXPECT_TRUE(bzip_writer.Init(fd_, extents, kBlockSize));
chromeos::Blob original_compressed_data = compressed_data;
diff --git a/delta_performer.cc b/delta_performer.cc
index 194f934..2d4517f 100644
--- a/delta_performer.cc
+++ b/delta_performer.cc
@@ -30,6 +30,7 @@
#include <base/strings/string_util.h>
#include <base/strings/stringprintf.h>
#include <chromeos/data_encoding.h>
+#include <chromeos/make_unique_ptr.h>
#include <google/protobuf/repeated_field.h>
#include "update_engine/bzip_extent_writer.h"
@@ -50,7 +51,6 @@
using google::protobuf::RepeatedPtrField;
using std::min;
using std::string;
-using std::unique_ptr;
using std::vector;
namespace chromeos_update_engine {
@@ -660,21 +660,13 @@
// Extract the signature message if it's in this operation.
ExtractSignatureMessage(operation);
- DirectExtentWriter direct_writer;
- ZeroPadExtentWriter zero_pad_writer(&direct_writer);
- unique_ptr<BzipExtentWriter> bzip_writer;
+ // Setup the ExtentWriter stack based on the operation type.
+ std::unique_ptr<ExtentWriter> writer =
+ chromeos::make_unique_ptr(new ZeroPadExtentWriter(
+ chromeos::make_unique_ptr(new DirectExtentWriter())));
- // Since bzip decompression is optional, we have a variable writer that will
- // point to one of the ExtentWriter objects above.
- ExtentWriter* writer = nullptr;
- if (operation.type() == InstallOperation::REPLACE) {
- writer = &zero_pad_writer;
- } else if (operation.type() == InstallOperation::REPLACE_BZ) {
- bzip_writer.reset(new BzipExtentWriter(&zero_pad_writer));
- writer = bzip_writer.get();
- } else {
- NOTREACHED();
- }
+ if (operation.type() == InstallOperation::REPLACE_BZ)
+ writer.reset(new BzipExtentWriter(std::move(writer)));
// Create a vector of extents to pass to the ExtentWriter.
vector<Extent> extents;
diff --git a/extent_writer.h b/extent_writer.h
index 1744781..99b2c2f 100644
--- a/extent_writer.h
+++ b/extent_writer.h
@@ -33,7 +33,7 @@
class ExtentWriter {
public:
- ExtentWriter() : end_called_(false) {}
+ ExtentWriter() = default;
virtual ~ExtentWriter() {
LOG_IF(ERROR, !end_called_) << "End() not called on ExtentWriter.";
}
@@ -54,7 +54,7 @@
}
virtual bool EndImpl() = 0;
private:
- bool end_called_;
+ bool end_called_{false};
};
// DirectExtentWriter is probably the simplest ExtentWriter implementation.
@@ -62,35 +62,29 @@
class DirectExtentWriter : public ExtentWriter {
public:
- DirectExtentWriter()
- : fd_(nullptr),
- block_size_(0),
- extent_bytes_written_(0),
- next_extent_index_(0) {}
- ~DirectExtentWriter() {}
+ DirectExtentWriter() = default;
+ ~DirectExtentWriter() override = default;
bool Init(FileDescriptorPtr fd,
const std::vector<Extent>& extents,
- uint32_t block_size) {
+ uint32_t block_size) override {
fd_ = fd;
block_size_ = block_size;
extents_ = extents;
return true;
}
- bool Write(const void* bytes, size_t count);
- bool EndImpl() {
- return true;
- }
+ bool Write(const void* bytes, size_t count) override;
+ bool EndImpl() override { return true; }
private:
- FileDescriptorPtr fd_;
+ FileDescriptorPtr fd_{nullptr};
- size_t block_size_;
+ size_t block_size_{0};
// Bytes written into next_extent_index_ thus far
- uint64_t extent_bytes_written_;
+ uint64_t extent_bytes_written_{0};
std::vector<Extent> extents_;
// The next call to write should correspond to extents_[next_extent_index_]
- std::vector<Extent>::size_type next_extent_index_;
+ std::vector<Extent>::size_type next_extent_index_{0};
};
// Takes an underlying ExtentWriter to which all operations are delegated.
@@ -100,19 +94,18 @@
class ZeroPadExtentWriter : public ExtentWriter {
public:
- explicit ZeroPadExtentWriter(ExtentWriter* underlying_extent_writer)
- : underlying_extent_writer_(underlying_extent_writer),
- block_size_(0),
- bytes_written_mod_block_size_(0) {}
- ~ZeroPadExtentWriter() {}
+ 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 std::vector<Extent>& extents,
- uint32_t block_size) {
+ 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) {
+ 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_;
@@ -120,7 +113,7 @@
}
return false;
}
- bool EndImpl() {
+ bool EndImpl() override {
if (bytes_written_mod_block_size_) {
const size_t write_size = block_size_ - bytes_written_mod_block_size_;
chromeos::Blob zeros(write_size, 0);
@@ -131,9 +124,9 @@
}
private:
- ExtentWriter* underlying_extent_writer_; // The underlying ExtentWriter.
- size_t block_size_;
- size_t bytes_written_mod_block_size_;
+ std::unique_ptr<ExtentWriter> underlying_extent_writer_;
+ size_t block_size_{0};
+ size_t bytes_written_mod_block_size_{0};
};
} // namespace chromeos_update_engine
diff --git a/extent_writer_unittest.cc b/extent_writer_unittest.cc
index 9da8adb..0750ebc 100644
--- a/extent_writer_unittest.cc
+++ b/extent_writer_unittest.cc
@@ -25,6 +25,7 @@
#include <string>
#include <vector>
+#include <chromeos/make_unique_ptr.h>
#include <chromeos/secure_blob.h>
#include <gtest/gtest.h>
@@ -189,8 +190,8 @@
chromeos::Blob data(kBlockSize * 2);
test_utils::FillWithData(&data);
- DirectExtentWriter direct_writer;
- ZeroPadExtentWriter zero_pad_writer(&direct_writer);
+ ZeroPadExtentWriter zero_pad_writer(
+ chromeos::make_unique_ptr(new DirectExtentWriter()));
EXPECT_TRUE(zero_pad_writer.Init(fd_, extents, kBlockSize));
size_t bytes_to_write = data.size();