update_engine: Stop adding unneeded parameters for some operations.
Currently we are adding src_extents, src_length and dst_length for ZERO,
DISCARD and but they don't need it.
Also we are secretly adding dst_length for all REPLACE operations
which they don't need it either.
BUG=chromium:776576
TEST=unittests passed; brillo_update_payload {generate|verify} passed;
Change-Id: I9bf04071a40ef4b9a9d437830f9bdcfb507f18a2
Reviewed-on: https://chromium-review.googlesource.com/729061
Commit-Ready: Amin Hassani <ahassani@chromium.org>
Tested-by: Amin Hassani <ahassani@chromium.org>
Reviewed-by: Ben Chan <benchan@chromium.org>
Reviewed-by: Sen Jiang <senj@chromium.org>
diff --git a/payload_generator/ab_generator.cc b/payload_generator/ab_generator.cc
index 3b0d012..f727c71 100644
--- a/payload_generator/ab_generator.cc
+++ b/payload_generator/ab_generator.cc
@@ -17,6 +17,7 @@
#include "update_engine/payload_generator/ab_generator.h"
#include <algorithm>
+#include <utility>
#include <base/strings/stringprintf.h>
@@ -173,7 +174,6 @@
InstallOperation new_op;
*(new_op.add_dst_extents()) = dst_ext;
uint32_t data_size = dst_ext.num_blocks() * kBlockSize;
- new_op.set_dst_length(data_size);
// If this is a REPLACE, attempt to reuse portions of the existing blob.
if (is_replace) {
new_op.set_type(InstallOperation::REPLACE);
@@ -238,15 +238,9 @@
if (is_delta_op) {
ExtendExtents(last_aop.op.mutable_src_extents(),
curr_aop.op.src_extents());
- if (curr_aop.op.src_length() > 0)
- last_aop.op.set_src_length(last_aop.op.src_length() +
- curr_aop.op.src_length());
}
ExtendExtents(last_aop.op.mutable_dst_extents(),
curr_aop.op.dst_extents());
- if (curr_aop.op.dst_length() > 0)
- last_aop.op.set_dst_length(last_aop.op.dst_length() +
- curr_aop.op.dst_length());
// Set the data length to zero so we know to add the blob later.
if (is_a_replace)
last_aop.op.set_data_length(0);
@@ -276,9 +270,9 @@
BlobFileWriter* blob_file) {
TEST_AND_RETURN_FALSE(IsAReplaceOperation(aop->op.type()));
- brillo::Blob data(aop->op.dst_length());
vector<Extent> dst_extents;
ExtentsToVector(aop->op.dst_extents(), &dst_extents);
+ brillo::Blob data(BlocksInExtents(dst_extents) * kBlockSize);
TEST_AND_RETURN_FALSE(utils::ReadExtents(target_part_path,
dst_extents,
&data,
diff --git a/payload_generator/ab_generator_unittest.cc b/payload_generator/ab_generator_unittest.cc
index ab4b164..25609c7 100644
--- a/payload_generator/ab_generator_unittest.cc
+++ b/payload_generator/ab_generator_unittest.cc
@@ -42,7 +42,9 @@
namespace {
-bool ExtentEquals(const Extent& ext, uint64_t start_block, uint64_t num_blocks) {
+bool ExtentEquals(const Extent& ext,
+ uint64_t start_block,
+ uint64_t num_blocks) {
return ext.start_block() == start_block && ext.num_blocks() == num_blocks;
}
@@ -85,7 +87,6 @@
op_ex1_num_blocks);
*(op.add_dst_extents()) = ExtentForRange(op_ex2_start_block,
op_ex2_num_blocks);
- op.set_dst_length(op_ex1_num_blocks + op_ex2_num_blocks);
brillo::Blob op_data;
op_data.insert(op_data.end(),
@@ -136,7 +137,8 @@
EXPECT_EQ("SplitTestOp:0", result_ops[0].name);
InstallOperation first_op = result_ops[0].op;
EXPECT_EQ(expected_type, first_op.type());
- EXPECT_EQ(op_ex1_size, first_op.dst_length());
+ EXPECT_FALSE(first_op.has_src_length());
+ EXPECT_FALSE(first_op.has_dst_length());
EXPECT_EQ(1, first_op.dst_extents().size());
EXPECT_TRUE(ExtentEquals(first_op.dst_extents(0), op_ex1_start_block,
op_ex1_num_blocks));
@@ -165,7 +167,8 @@
EXPECT_EQ("SplitTestOp:1", result_ops[1].name);
InstallOperation second_op = result_ops[1].op;
EXPECT_EQ(expected_type, second_op.type());
- EXPECT_EQ(op_ex2_size, second_op.dst_length());
+ EXPECT_FALSE(second_op.has_src_length());
+ EXPECT_FALSE(second_op.has_dst_length());
EXPECT_EQ(1, second_op.dst_extents().size());
EXPECT_TRUE(ExtentEquals(second_op.dst_extents(0), op_ex2_start_block,
op_ex2_num_blocks));
@@ -235,7 +238,6 @@
InstallOperation first_op;
first_op.set_type(orig_type);
const size_t first_op_size = first_op_num_blocks * kBlockSize;
- first_op.set_dst_length(first_op_size);
*(first_op.add_dst_extents()) = ExtentForRange(0, first_op_num_blocks);
brillo::Blob first_op_data(part_data.begin(),
part_data.begin() + first_op_size);
@@ -255,8 +257,6 @@
InstallOperation second_op;
second_op.set_type(orig_type);
- const size_t second_op_size = second_op_num_blocks * kBlockSize;
- second_op.set_dst_length(second_op_size);
*(second_op.add_dst_extents()) = ExtentForRange(first_op_num_blocks,
second_op_num_blocks);
brillo::Blob second_op_data(part_data.begin() + first_op_size,
@@ -302,7 +302,7 @@
InstallOperation new_op = aops[0].op;
EXPECT_EQ(expected_op_type, new_op.type());
EXPECT_FALSE(new_op.has_src_length());
- EXPECT_EQ(total_op_num_blocks * kBlockSize, new_op.dst_length());
+ EXPECT_FALSE(new_op.has_dst_length());
EXPECT_EQ(1, new_op.dst_extents().size());
EXPECT_TRUE(ExtentEquals(new_op.dst_extents(0), 0, total_op_num_blocks));
EXPECT_EQ("first,second", aops[0].name);
diff --git a/payload_generator/delta_diff_utils.cc b/payload_generator/delta_diff_utils.cc
index fb72d48..39438dd 100644
--- a/payload_generator/delta_diff_utils.cc
+++ b/payload_generator/delta_diff_utils.cc
@@ -737,26 +737,30 @@
}
}
- size_t removed_bytes = 0;
// Remove identical src/dst block ranges in MOVE operations.
if (operation.type() == InstallOperation::MOVE) {
- removed_bytes = RemoveIdenticalBlockRanges(
+ auto removed_bytes = RemoveIdenticalBlockRanges(
&src_extents, &dst_extents, new_data.size());
+ operation.set_src_length(old_data.size() - removed_bytes);
+ operation.set_dst_length(new_data.size() - removed_bytes);
}
- // Set legacy src_length and dst_length fields.
- operation.set_src_length(old_data.size() - removed_bytes);
- operation.set_dst_length(new_data.size() - removed_bytes);
- // Embed extents in the operation.
- StoreExtents(src_extents, operation.mutable_src_extents());
+ // Set legacy |src_length| and |dst_length| fields for both BSDIFF and
+ // SOURCE_BSDIFF as only these two use these parameters.
+ if (operation.type() == InstallOperation::BSDIFF ||
+ operation.type() == InstallOperation::SOURCE_BSDIFF) {
+ operation.set_src_length(old_data.size());
+ operation.set_dst_length(new_data.size());
+ }
+
+ // Embed extents in the operation. Replace (all variants), zero and discard
+ // operations should not have source extents.
+ if (!IsNoSourceOperation(operation.type())) {
+ StoreExtents(src_extents, operation.mutable_src_extents());
+ }
+ // All operations have dst_extents.
StoreExtents(dst_extents, operation.mutable_dst_extents());
- // Replace operations should not have source extents.
- if (IsAReplaceOperation(operation.type())) {
- operation.clear_src_extents();
- operation.clear_src_length();
- }
-
*out_data = std::move(data_blob);
*out_op = operation;
return true;
@@ -768,6 +772,12 @@
op_type == InstallOperation::REPLACE_XZ);
}
+bool IsNoSourceOperation(InstallOperation_Type op_type) {
+ return (IsAReplaceOperation(op_type) ||
+ op_type == InstallOperation::ZERO ||
+ op_type == InstallOperation::DISCARD);
+}
+
// Returns true if |op| is a no-op operation that doesn't do any useful work
// (e.g., a move operation that copies blocks onto themselves).
bool IsNoopOperation(const InstallOperation& op) {
diff --git a/payload_generator/delta_diff_utils.h b/payload_generator/delta_diff_utils.h
index 2d49459..d21468f 100644
--- a/payload_generator/delta_diff_utils.h
+++ b/payload_generator/delta_diff_utils.h
@@ -112,9 +112,12 @@
brillo::Blob* out_blob,
InstallOperation_Type* out_type);
-// Returns whether op_type is one of the REPLACE full operations.
+// Returns whether |op_type| is one of the REPLACE full operations.
bool IsAReplaceOperation(InstallOperation_Type op_type);
+// Returns true if an operation with type |op_type| has no |src_extents|.
+bool IsNoSourceOperation(InstallOperation_Type op_type);
+
// Returns true if |op| is a no-op operation that doesn't do any useful work
// (e.g., a move operation that copies blocks onto themselves).
bool IsNoopOperation(const InstallOperation& op);
diff --git a/payload_generator/delta_diff_utils_unittest.cc b/payload_generator/delta_diff_utils_unittest.cc
index bb83942..34e33ad 100644
--- a/payload_generator/delta_diff_utils_unittest.cc
+++ b/payload_generator/delta_diff_utils_unittest.cc
@@ -364,7 +364,7 @@
EXPECT_EQ(0, op.src_extents_size());
EXPECT_FALSE(op.has_src_length());
EXPECT_EQ(1, op.dst_extents_size());
- EXPECT_EQ(data_to_test.size(), op.dst_length());
+ EXPECT_FALSE(op.has_dst_length());
EXPECT_EQ(1U, BlocksInExtents(op.dst_extents()));
}
}
diff --git a/update_metadata.proto b/update_metadata.proto
index 1120bd1..f91f673 100644
--- a/update_metadata.proto
+++ b/update_metadata.proto
@@ -178,14 +178,15 @@
// Ordered list of extents that are read from (if any) and written to.
repeated Extent src_extents = 4;
// Byte length of src, equal to the number of blocks in src_extents *
- // block_size. It is used for BSDIFF, because we need to pass that
- // external program the number of bytes to read from the blocks we pass it.
- // This is not used in any other operation.
+ // block_size. It is used for BSDIFF and SOURCE_BSDIFF, because we need to
+ // pass that external program the number of bytes to read from the blocks we
+ // pass it. This is not used in any other operation.
optional uint64 src_length = 5;
repeated Extent dst_extents = 6;
// Byte length of dst, equal to the number of blocks in dst_extents *
- // block_size. Used for BSDIFF, but not in any other operation.
+ // block_size. Used for BSDIFF and SOURCE_BSDIFF, but not in any other
+ // operation.
optional uint64 dst_length = 7;
// Optional SHA 256 hash of the blob associated with this operation.