Fix false negative NeedResize
BootControlAndroid did not call ResizePartitions when the
sizes were all correct to avoid storing metadata more than
once (after resuming from an update), and potentially writing
to incorrect extents. But, when the update starts, the target
metadata slot may contain metadata that happen to exactly
matches the size requirements, but have extents that maps to
currently running devices.
To do this correctly, DeltaPerformer uses PrefsInterface to
avoid calling into InitPartitionMetadata again when it is
initialized.
Hence, BootControlAndroid::InitPartitionMetadata ALWAYS write
metadata to the target slot. Also removed tests to reflect this.
Test: manual OTA
Test: update_engine_unittests
Bug: 117182932
Change-Id: I488ab369e42d582c94974791fdb988d12e695bc2
diff --git a/boot_control_android.cc b/boot_control_android.cc
index 622646e..61711d2 100644
--- a/boot_control_android.cc
+++ b/boot_control_android.cc
@@ -340,48 +340,6 @@
return true;
}
-// Return false if partition sizes are all correct in metadata slot
-// |target_slot|. If so, no need to resize. |logical_sizes| have format like
-// {vendor: size, ...}, and fail if a partition is not found.
-bool NeedResizePartitions(DynamicPartitionControlInterface* dynamic_control,
- const string& super_device,
- Slot target_slot,
- const string& suffix,
- const PartitionMetadata& logical_sizes) {
- auto target_metadata =
- dynamic_control->LoadMetadataBuilder(super_device, target_slot);
- if (target_metadata == nullptr) {
- LOG(INFO) << "Metadata slot " << BootControlInterface::SlotName(target_slot)
- << " in " << super_device
- << " is corrupted; attempt to recover from source slot.";
- return true;
- }
-
- for (const auto& pair : logical_sizes) {
- Partition* partition = target_metadata->FindPartition(pair.first + suffix);
- if (partition == nullptr) {
- LOG(INFO) << "Cannot find " << pair.first << suffix << " at slot "
- << BootControlInterface::SlotName(target_slot) << " in "
- << super_device << ". Need to resize.";
- return true;
- }
- if (partition->size() != pair.second) {
- LOG(INFO) << super_device << ":"
- << BootControlInterface::SlotName(target_slot) << ":"
- << pair.first << suffix << ": size == " << partition->size()
- << " but requested " << pair.second << ". Need to resize.";
- return true;
- }
- LOG(INFO) << super_device << ":"
- << BootControlInterface::SlotName(target_slot) << ":"
- << pair.first << suffix << ": size == " << partition->size()
- << " as requested.";
- }
- LOG(INFO) << "No need to resize at metadata slot "
- << BootControlInterface::SlotName(target_slot) << " in "
- << super_device;
- return false;
-}
} // namespace
bool BootControlAndroid::InitPartitionMetadata(
@@ -434,21 +392,13 @@
return false;
}
- // Read metadata from target slot to determine if the sizes are correct. Only
- // test logical partitions.
- if (NeedResizePartitions(dynamic_control_.get(),
- super_device,
- target_slot,
- target_suffix,
- logical_sizes)) {
- if (!ResizePartitions(dynamic_control_.get(),
- super_device,
- target_slot,
- target_suffix,
- logical_sizes,
- builder.get())) {
- return false;
- }
+ if (!ResizePartitions(dynamic_control_.get(),
+ super_device,
+ target_slot,
+ target_suffix,
+ logical_sizes,
+ builder.get())) {
+ return false;
}
// Unmap all partitions, and remap partitions if size is non-zero.
diff --git a/boot_control_android_unittest.cc b/boot_control_android_unittest.cc
index d62c3a6..34eabb0 100644
--- a/boot_control_android_unittest.cc
+++ b/boot_control_android_unittest.cc
@@ -298,110 +298,6 @@
}
};
-// Test no resize if no dynamic partitions at all.
-TEST_P(BootControlAndroidTestP, NoResizeIfNoDynamicPartitions) {
- SetMetadata(source(), {});
- SetMetadata(target(), {});
- // Should not need to resize and store metadata
- EXPECT_CALL(dynamicControl(), StoreMetadata(GetSuperDevice(), _, target()))
- .Times(0);
- EXPECT_CALL(dynamicControl(), DeviceExists(Eq(GetDevice("static_a"))))
- .Times(AnyNumber())
- .WillRepeatedly(Return(true));
- EXPECT_CALL(dynamicControl(), DeviceExists(Eq(GetDevice("static_b"))))
- .Times(AnyNumber())
- .WillRepeatedly(Return(true));
-
- EXPECT_TRUE(bootctl_.InitPartitionMetadata(target(), {{"static", 1_GiB}}));
- ExpectDevicesAreMapped({});
-}
-
-// Test no resize if update manifest does not contain any dynamic partitions
-TEST_P(BootControlAndroidTestP, NoResizeIfEmptyMetadata) {
- SetMetadata(source(),
- {{S("system"), 4_GiB},
- {S("vendor"), 100_MiB},
- {T("system"), 3_GiB},
- {T("vendor"), 150_MiB}});
- SetMetadata(target(),
- {{S("system"), 2_GiB},
- {S("vendor"), 1_GiB},
- {T("system"), 3_GiB},
- {T("vendor"), 150_MiB}});
- // Should not need to resize and store metadata
- EXPECT_CALL(dynamicControl(), StoreMetadata(GetSuperDevice(), _, target()))
- .Times(0);
- EXPECT_CALL(dynamicControl(), DeviceExists(Eq(GetDevice("static_a"))))
- .Times(AnyNumber())
- .WillRepeatedly(Return(true));
- EXPECT_CALL(dynamicControl(), DeviceExists(Eq(GetDevice("static_b"))))
- .Times(AnyNumber())
- .WillRepeatedly(Return(true));
-
- EXPECT_TRUE(bootctl_.InitPartitionMetadata(target(), {{"static", 1_GiB}}));
- ExpectDevicesAreMapped({});
-}
-
-// Do not resize if manifest size matches size in target metadata. When resuming
-// from an update, do not redo the resize if not needed.
-TEST_P(BootControlAndroidTestP, NoResizeIfSizeMatchWhenResizing) {
- SetMetadata(source(), {{S("system"), 2_GiB}, {S("vendor"), 1_GiB}});
- SetMetadata(target(),
- {{S("system"), 2_GiB},
- {S("vendor"), 1_GiB},
- {T("system"), 3_GiB},
- {T("vendor"), 1_GiB}});
- // Should not need to resize and store metadata
- EXPECT_CALL(dynamicControl(), StoreMetadata(GetSuperDevice(), _, target()))
- .Times(0);
- ExpectRemap({T("system"), T("vendor")});
-
- EXPECT_TRUE(bootctl_.InitPartitionMetadata(
- target(), {{"system", 3_GiB}, {"vendor", 1_GiB}}));
- ExpectDevicesAreMapped({T("system"), T("vendor")});
-}
-
-// Do not resize if manifest size matches size in target metadata. When resuming
-// from an update, do not redo the resize if not needed.
-TEST_P(BootControlAndroidTestP, NoResizeIfSizeMatchWhenAdding) {
- SetMetadata(source(), {{S("system"), 2_GiB}, {T("system"), 2_GiB}});
- SetMetadata(
- target(),
- {{S("system"), 2_GiB}, {T("system"), 2_GiB}, {T("vendor"), 1_GiB}});
- // Should not need to resize and store metadata
- EXPECT_CALL(dynamicControl(), StoreMetadata(GetSuperDevice(), _, target()))
- .Times(0);
- ExpectRemap({T("system"), T("vendor")});
-
- EXPECT_TRUE(bootctl_.InitPartitionMetadata(
- target(), {{"system", 2_GiB}, {"vendor", 1_GiB}}));
- ExpectDevicesAreMapped({T("system"), T("vendor")});
-}
-
-// Do not resize if manifest size matches size in target metadata. When resuming
-// from an update, do not redo the resize if not needed.
-TEST_P(BootControlAndroidTestP, NoResizeIfSizeMatchWhenDeleting) {
- SetMetadata(source(),
- {{S("system"), 2_GiB},
- {S("vendor"), 1_GiB},
- {T("system"), 2_GiB},
- {T("vendor"), 1_GiB}});
- SetMetadata(target(),
- {{S("system"), 2_GiB},
- {S("vendor"), 1_GiB},
- {T("system"), 2_GiB},
- {T("vendor"), 0}});
- // Should not need to resize and store metadata
- EXPECT_CALL(dynamicControl(), StoreMetadata(GetSuperDevice(), _, target()))
- .Times(0);
- ExpectUnmap({T("system"), T("vendor")});
- ExpectMap({T("system")});
-
- EXPECT_TRUE(bootctl_.InitPartitionMetadata(
- target(), {{"system", 2_GiB}, {"vendor", 0}}));
- ExpectDevicesAreMapped({T("system")});
-}
-
// Test resize case. Grow if target metadata contains a partition with a size
// less than expected.
TEST_P(BootControlAndroidTestP, NeedGrowIfSizeNotMatchWhenResizing) {
diff --git a/common/constants.cc b/common/constants.cc
index 4bca105..3ae7a60 100644
--- a/common/constants.cc
+++ b/common/constants.cc
@@ -37,6 +37,8 @@
const char kPrefsDailyMetricsLastReportedAt[] =
"daily-metrics-last-reported-at";
const char kPrefsDeltaUpdateFailures[] = "delta-update-failures";
+const char kPrefsDynamicPartitionMetadataInitialized[] =
+ "dynamic-partition-metadata-initialized";
const char kPrefsFullPayloadAttemptNumber[] = "full-payload-attempt-number";
const char kPrefsInstallDateDays[] = "install-date-days";
const char kPrefsLastActivePingDay[] = "last-active-ping-day";
diff --git a/common/constants.h b/common/constants.h
index 1057a65..61e5ddd 100644
--- a/common/constants.h
+++ b/common/constants.h
@@ -41,6 +41,7 @@
extern const char kPrefsCurrentUrlIndex[];
extern const char kPrefsDailyMetricsLastReportedAt[];
extern const char kPrefsDeltaUpdateFailures[];
+extern const char kPrefsDynamicPartitionMetadataInitialized[];
extern const char kPrefsFullPayloadAttemptNumber[];
extern const char kPrefsInstallDateDays[];
extern const char kPrefsLastActivePingDay[];
diff --git a/payload_consumer/delta_performer.cc b/payload_consumer/delta_performer.cc
index ca898d5..7dec48f 100644
--- a/payload_consumer/delta_performer.cc
+++ b/payload_consumer/delta_performer.cc
@@ -919,14 +919,7 @@
}
if (install_plan_->target_slot != BootControlInterface::kInvalidSlot) {
- BootControlInterface::PartitionMetadata partition_metadata;
- for (const InstallPlan::Partition& partition : install_plan_->partitions) {
- partition_metadata.emplace(partition.name, partition.target_size);
- }
- if (!boot_control_->InitPartitionMetadata(install_plan_->target_slot,
- partition_metadata)) {
- LOG(ERROR) << "Unable to initialize partition metadata for slot "
- << BootControlInterface::SlotName(install_plan_->target_slot);
+ if (!InitPartitionMetadata()) {
*error = ErrorCode::kInstallDeviceOpenError;
return false;
}
@@ -941,6 +934,33 @@
return true;
}
+bool DeltaPerformer::InitPartitionMetadata() {
+ bool metadata_initialized;
+ if (prefs_->GetBoolean(kPrefsDynamicPartitionMetadataInitialized,
+ &metadata_initialized) &&
+ metadata_initialized) {
+ LOG(INFO) << "Skipping InitPartitionMetadata.";
+ return true;
+ }
+
+ BootControlInterface::PartitionMetadata partition_metadata;
+ for (const InstallPlan::Partition& partition : install_plan_->partitions) {
+ partition_metadata.emplace(partition.name, partition.target_size);
+ }
+
+ if (!boot_control_->InitPartitionMetadata(install_plan_->target_slot,
+ partition_metadata)) {
+ LOG(ERROR) << "Unable to initialize partition metadata for slot "
+ << BootControlInterface::SlotName(install_plan_->target_slot);
+ return false;
+ }
+ TEST_AND_RETURN_FALSE(
+ prefs_->SetBoolean(kPrefsDynamicPartitionMetadataInitialized, true));
+ LOG(INFO) << "InitPartitionMetadata done.";
+
+ return true;
+}
+
bool DeltaPerformer::CanPerformInstallOperation(
const chromeos_update_engine::InstallOperation& operation) {
// If we don't have a data blob we can apply it right away.
@@ -1861,6 +1881,7 @@
prefs->SetInt64(kPrefsResumedUpdateFailures, 0);
prefs->Delete(kPrefsPostInstallSucceeded);
prefs->Delete(kPrefsVerityWritten);
+ prefs->Delete(kPrefsDynamicPartitionMetadataInitialized);
}
return true;
}
diff --git a/payload_consumer/delta_performer.h b/payload_consumer/delta_performer.h
index 38d2c43..cddfef4 100644
--- a/payload_consumer/delta_performer.h
+++ b/payload_consumer/delta_performer.h
@@ -271,6 +271,10 @@
// it up.
bool GetPublicKeyFromResponse(base::FilePath *out_tmp_key);
+ // After install_plan_ is filled with partition names and sizes, initialize
+ // metadata of partitions and map necessary devices before opening devices.
+ bool InitPartitionMetadata();
+
// Update Engine preference store.
PrefsInterface* prefs_;
diff --git a/payload_consumer/delta_performer_integration_test.cc b/payload_consumer/delta_performer_integration_test.cc
index ba5fa18..9368e11 100644
--- a/payload_consumer/delta_performer_integration_test.cc
+++ b/payload_consumer/delta_performer_integration_test.cc
@@ -697,6 +697,8 @@
.WillRepeatedly(Return(true));
EXPECT_CALL(prefs, SetString(kPrefsUpdateStateSignedSHA256Context, _))
.WillRepeatedly(Return(true));
+ EXPECT_CALL(prefs, SetBoolean(kPrefsDynamicPartitionMetadataInitialized, _))
+ .WillRepeatedly(Return(true));
if (op_hash_test == kValidOperationData && signature_test != kSignatureNone) {
EXPECT_CALL(prefs, SetString(kPrefsUpdateStateSignatureBlob, _))
.WillOnce(Return(true));