Fix xor writer failure
When two install op extents generate consecutive XOR extents, xor extent
writer will merge them. But ExtentMap does not support partial look up,
so OTA fails. Add testcases specifically for this case.
Test: th
Bug: 266021213
Change-Id: Ieb5b9a3fd175f16ced2799050f627376bacbfb51
diff --git a/aosp/boot_control_android.h b/aosp/boot_control_android.h
index 018c00c..51923e2 100644
--- a/aosp/boot_control_android.h
+++ b/aosp/boot_control_android.h
@@ -26,7 +26,7 @@
#include <BootControlClient.h>
#include "update_engine/aosp/dynamic_partition_control_android.h"
-#include "update_engine/common/boot_control.h"
+#include "update_engine/common/boot_control_interface.h"
#include "update_engine/common/dynamic_partition_control_interface.h"
namespace chromeos_update_engine {
diff --git a/common/prefs.cc b/common/prefs.cc
index 41a7b24..a070302 100644
--- a/common/prefs.cc
+++ b/common/prefs.cc
@@ -25,7 +25,6 @@
#include <base/strings/string_split.h>
#include <base/strings/string_util.h>
-#include "brillo/file_utils.h"
#include "update_engine/common/utils.h"
using std::string;
diff --git a/payload_consumer/extent_map.h b/payload_consumer/extent_map.h
index a985cb5..a83bf0f 100644
--- a/payload_consumer/extent_map.h
+++ b/payload_consumer/extent_map.h
@@ -56,6 +56,15 @@
const auto it = map_.find(extent);
if (it == map_.end()) {
for (const auto& ext : set_.GetCandidateRange(extent)) {
+ // Sometimes there are operations like
+ // map.AddExtent({0, 5}, 42);
+ // map.Get({2, 1})
+ // If the querying extent is completely covered within the key, we still
+ // consdier this to be a valid query.
+
+ if (ExtentContains(ext, extent)) {
+ return map_.at(ext);
+ }
if (ExtentRanges::ExtentsOverlap(ext, extent)) {
LOG(WARNING) << "Looking up a partially intersecting extent isn't "
"supported by "
diff --git a/payload_consumer/extent_map_unittest.cc b/payload_consumer/extent_map_unittest.cc
index d8137a0..8e79b33 100644
--- a/payload_consumer/extent_map_unittest.cc
+++ b/payload_consumer/extent_map_unittest.cc
@@ -20,7 +20,6 @@
#include "update_engine/payload_consumer/extent_map.h"
#include "update_engine/payload_generator/extent_ranges.h"
-#include "update_engine/payload_generator/extent_utils.h"
namespace chromeos_update_engine {
@@ -41,6 +40,17 @@
ASSERT_TRUE(map_.AddExtent(ExtentForRange(0, 5), 7));
ASSERT_TRUE(map_.AddExtent(ExtentForRange(10, 5), 1));
auto ret = map_.Get(ExtentForRange(1, 2));
+ ASSERT_EQ(ret, 7);
+}
+
+TEST_F(ExtentMapTest, QueryNoMerge) {
+ ASSERT_TRUE(map_.AddExtent(ExtentForRange(0, 5), 7));
+ ASSERT_TRUE(map_.AddExtent(ExtentForRange(5, 5), 1));
+ auto ret = map_.Get(ExtentForRange(1, 2));
+ ASSERT_EQ(ret, 7);
+ ret = map_.Get(ExtentForRange(0, 10));
+ ASSERT_EQ(ret, std::nullopt);
+ ret = map_.Get(ExtentForRange(4, 3));
ASSERT_EQ(ret, std::nullopt);
}
@@ -48,9 +58,9 @@
ASSERT_TRUE(map_.AddExtent(ExtentForRange(0, 5), 7));
ASSERT_TRUE(map_.AddExtent(ExtentForRange(10, 5), 1));
auto ret = map_.Get(ExtentForRange(3, 2));
- ASSERT_EQ(ret, std::nullopt);
+ ASSERT_EQ(ret, 7);
ret = map_.Get(ExtentForRange(4, 1));
- ASSERT_EQ(ret, std::nullopt);
+ ASSERT_EQ(ret, 7);
ret = map_.Get(ExtentForRange(5, 5));
ASSERT_EQ(ret, std::nullopt);
ret = map_.Get(ExtentForRange(5, 6));
diff --git a/payload_consumer/payload_verifier.cc b/payload_consumer/payload_verifier.cc
index 8a3ea65..83ef8c9 100644
--- a/payload_consumer/payload_verifier.cc
+++ b/payload_consumer/payload_verifier.cc
@@ -23,7 +23,6 @@
#include <openssl/pem.h>
#include "update_engine/common/constants.h"
-#include "update_engine/common/hash_calculator.h"
#include "update_engine/common/utils.h"
#include "update_engine/payload_consumer/certificate_parser_interface.h"
#include "update_engine/update_metadata.pb.h"
diff --git a/payload_consumer/xor_extent_writer.cc b/payload_consumer/xor_extent_writer.cc
index 31567f2..1a02a2d 100644
--- a/payload_consumer/xor_extent_writer.cc
+++ b/payload_consumer/xor_extent_writer.cc
@@ -48,21 +48,22 @@
const auto merge_op = merge_op_opt.value();
TEST_AND_RETURN_FALSE(merge_op->has_src_extent());
TEST_AND_RETURN_FALSE(merge_op->has_dst_extent());
- if (merge_op->dst_extent() != xor_ext) {
- LOG(ERROR) << "Each xor extent is expected to correspond to a complete "
- "MergeOp, extent in value: "
- << merge_op->dst_extent() << " extent in key: " << xor_ext;
- return false;
- }
- if (xor_ext.start_block() + xor_ext.num_blocks() >
- extent.start_block() + extent.num_blocks()) {
+ if (!ExtentContains(extent, xor_ext)) {
LOG(ERROR) << "CowXor merge op extent should be completely inside "
"InstallOp's extent. merge op extent: "
<< xor_ext << " InstallOp extent: " << extent;
return false;
}
+ if (!ExtentContains(merge_op->dst_extent(), xor_ext)) {
+ LOG(ERROR) << "CowXor op extent should be completely inside "
+ "xor_map's extent. merge op extent: "
+ << xor_ext << " xor_map extent: " << merge_op->dst_extent();
+ return false;
+ }
const auto src_offset = merge_op->src_offset();
- const auto src_block = merge_op->src_extent().start_block();
+ const auto src_block = merge_op->src_extent().start_block() +
+ xor_ext.start_block() -
+ merge_op->dst_extent().start_block();
xor_block_data.resize(BlockSize() * xor_ext.num_blocks());
ssize_t bytes_read = 0;
TEST_AND_RETURN_FALSE_ERRNO(
diff --git a/payload_consumer/xor_extent_writer_unittest.cc b/payload_consumer/xor_extent_writer_unittest.cc
index 7f35bc2..015c9ab 100644
--- a/payload_consumer/xor_extent_writer_unittest.cc
+++ b/payload_consumer/xor_extent_writer_unittest.cc
@@ -131,4 +131,46 @@
ASSERT_TRUE(writer_.Write(zeros->data(), 9 * kBlockSize));
}
+TEST_F(XorExtentWriterTest, SubsetExtentTest) {
+ constexpr auto COW_XOR = CowMergeOperation::COW_XOR;
+ ON_CALL(cow_writer_, EmitXorBlocks(_, _, _, _, _))
+ .WillByDefault(Return(true));
+
+ const auto op3 = CreateCowMergeOperation(
+ ExtentForRange(12, 4), ExtentForRange(320, 4), COW_XOR, 777);
+ ASSERT_TRUE(xor_map_.AddExtent(op3.dst_extent(), &op3));
+
+ *op_.add_src_extents() = ExtentForRange(12, 3);
+ *op_.add_dst_extents() = ExtentForRange(320, 3);
+ *op_.add_src_extents() = ExtentForRange(20, 3);
+ *op_.add_dst_extents() = ExtentForRange(420, 3);
+ *op_.add_src_extents() = ExtentForRange(15, 1);
+ *op_.add_dst_extents() = ExtentForRange(323, 1);
+ XORExtentWriter writer_{op_, source_fd_, &cow_writer_, xor_map_};
+
+ // OTA op:
+ // [12-14] => [320-322], [20-22] => [420-422], [15-16] => [323-324]
+
+ // merge op:
+ // [12-16] => [321-322]
+
+ // Expected result:
+ // [12-16] should be XOR blocks
+ // [420-422] should be regular replace blocks
+
+ auto zeros = utils::GetReadonlyZeroBlock(kBlockSize * 7);
+ EXPECT_CALL(
+ cow_writer_,
+ EmitRawBlocks(420, zeros->data() + 3 * kBlockSize, kBlockSize * 3))
+ .WillOnce(Return(true));
+
+ EXPECT_CALL(cow_writer_, EmitXorBlocks(320, _, kBlockSize * 3, 12, 777))
+ .WillOnce(Return(true));
+ EXPECT_CALL(cow_writer_, EmitXorBlocks(323, _, kBlockSize, 15, 777))
+ .WillOnce(Return(true));
+
+ ASSERT_TRUE(writer_.Init(op_.dst_extents(), kBlockSize));
+ ASSERT_TRUE(writer_.Write(zeros->data(), zeros->size()));
+}
+
} // namespace chromeos_update_engine
diff --git a/payload_generator/extent_utils.h b/payload_generator/extent_utils.h
index 17995b1..f8d36e7 100644
--- a/payload_generator/extent_utils.h
+++ b/payload_generator/extent_utils.h
@@ -151,6 +151,13 @@
block < extent.start_block() + extent.num_blocks();
}
+// return true iff |big| extent contains |small| extent
+constexpr bool ExtentContains(const Extent& big, const Extent& small) {
+ return big.start_block() <= small.start_block() &&
+ small.start_block() + small.num_blocks() <=
+ big.start_block() + big.num_blocks();
+}
+
} // namespace chromeos_update_engine
#endif // UPDATE_ENGINE_PAYLOAD_GENERATOR_EXTENT_UTILS_H_