Replace is_full_update boolean with a payload_state enum.
The "is_full_update" flag in the InstallPlan is required to decide
whether we should run a FilesystemVerification step before start
downloading the payload (for delta payloads) or not (for full payloads).
This step is done before start downloading the payload and not after
downloading the metadata to avoid long delays in the connection which
would then drop and require a retry.
Since the not so recent inclusion of the source_data_hash field in the
delta operations, the source data is verified on each operation, so the
install plan field and the pre-download FilesystemVerification is not
needed anymore.
To help deprecate this process, which is not included in the non-Brillo
version, this patch changes the is_full_update field to a payload_state
enum with a third "unknown" state that will be changed to delta or full
once the payload metadata is parsed.
Bug: 25631949
TEST=unittests updated.
TEST=Pushed a delta update to edison-eng and a non-Brillo target.
Change-Id: I17d8bf58990d8465bb8487adc66601f1c1dfca6d
diff --git a/payload_consumer/delta_performer.cc b/payload_consumer/delta_performer.cc
index 09304e4..cb3cdb5 100644
--- a/payload_consumer/delta_performer.cc
+++ b/payload_consumer/delta_performer.cc
@@ -291,7 +291,7 @@
const PartitionUpdate& partition = partitions_[current_partition_];
// Open source fds if we have a delta payload with minor version >= 2.
- if (!install_plan_->is_full_update &&
+ if (install_plan_->payload_type == InstallPayloadType::kDelta &&
GetMinorVersion() != kInPlaceMinorPayloadVersion) {
source_path_ = install_plan_->partitions[current_partition_].source_path;
int err;
@@ -374,9 +374,9 @@
if (manifest_.has_minor_version()) {
return manifest_.minor_version();
} else {
- return (install_plan_->is_full_update ?
- kFullPayloadMinorVersion :
- kSupportedMinorPayloadVersion);
+ return install_plan_->payload_type == InstallPayloadType::kDelta
+ ? kSupportedMinorPayloadVersion
+ : kFullPayloadMinorVersion;
}
}
@@ -1362,29 +1362,34 @@
ErrorCode DeltaPerformer::ValidateManifest() {
// Perform assorted checks to sanity check the manifest, make sure it
// matches data from other sources, and that it is a supported version.
- //
- // TODO(garnold) in general, the presence of an old partition hash should be
- // the sole indicator for a delta update, as we would generally like update
- // payloads to be self contained and not assume an Omaha response to tell us
- // that. However, since this requires some massive reengineering of the update
- // flow (making filesystem copying happen conditionally only *after*
- // downloading and parsing of the update manifest) we'll put it off for now.
- // See chromium-os:7597 for further discussion.
- if (install_plan_->is_full_update) {
- if (manifest_.has_old_kernel_info() || manifest_.has_old_rootfs_info()) {
- LOG(ERROR) << "Purported full payload contains old partition "
- "hash(es), aborting update";
- return ErrorCode::kPayloadMismatchedType;
- }
- for (const PartitionUpdate& partition : manifest_.partitions()) {
- if (partition.has_old_partition_info()) {
- LOG(ERROR) << "Purported full payload contains old partition "
- "hash(es), aborting update";
- return ErrorCode::kPayloadMismatchedType;
- }
- }
+ bool has_old_fields =
+ (manifest_.has_old_kernel_info() || manifest_.has_old_rootfs_info());
+ for (const PartitionUpdate& partition : manifest_.partitions()) {
+ has_old_fields = has_old_fields || partition.has_old_partition_info();
+ }
+ // The presence of an old partition hash is the sole indicator for a delta
+ // update.
+ InstallPayloadType actual_payload_type =
+ has_old_fields ? InstallPayloadType::kDelta : InstallPayloadType::kFull;
+
+ if (install_plan_->payload_type == InstallPayloadType::kUnknown) {
+ LOG(INFO) << "Detected a '"
+ << InstallPayloadTypeToString(actual_payload_type)
+ << "' payload.";
+ install_plan_->payload_type = actual_payload_type;
+ } else if (install_plan_->payload_type != actual_payload_type) {
+ LOG(ERROR) << "InstallPlan expected a '"
+ << InstallPayloadTypeToString(install_plan_->payload_type)
+ << "' payload but the downloaded manifest contains a '"
+ << InstallPayloadTypeToString(actual_payload_type)
+ << "' payload.";
+ return ErrorCode::kPayloadMismatchedType;
+ }
+
+ // Check that the minor version is compatible.
+ if (actual_payload_type == InstallPayloadType::kFull) {
if (manifest_.minor_version() != kFullPayloadMinorVersion) {
LOG(ERROR) << "Manifest contains minor version "
<< manifest_.minor_version()
diff --git a/payload_consumer/delta_performer_integration_test.cc b/payload_consumer/delta_performer_integration_test.cc
index 65d685d..9b7854b 100644
--- a/payload_consumer/delta_performer_integration_test.cc
+++ b/payload_consumer/delta_performer_integration_test.cc
@@ -715,7 +715,9 @@
InstallPlan* install_plan = &state->install_plan;
install_plan->hash_checks_mandatory = hash_checks_mandatory;
install_plan->metadata_size = state->metadata_size;
- install_plan->is_full_update = full_kernel && full_rootfs;
+ install_plan->payload_type = (full_kernel && full_rootfs)
+ ? InstallPayloadType::kFull
+ : InstallPayloadType::kDelta;
install_plan->source_slot = 0;
install_plan->target_slot = 1;
diff --git a/payload_consumer/delta_performer_unittest.cc b/payload_consumer/delta_performer_unittest.cc
index fa904f2..dabe138 100644
--- a/payload_consumer/delta_performer_unittest.cc
+++ b/payload_consumer/delta_performer_unittest.cc
@@ -97,10 +97,9 @@
// Test helper placed where it can easily be friended from DeltaPerformer.
void RunManifestValidation(const DeltaArchiveManifest& manifest,
uint64_t major_version,
- bool full_payload,
+ InstallPayloadType payload_type,
ErrorCode expected) {
- // The install plan is for Full or Delta.
- install_plan_.is_full_update = full_payload;
+ install_plan_.payload_type = payload_type;
// The Manifest we are validating.
performer_.manifest_.CopyFrom(manifest);
@@ -137,6 +136,12 @@
EXPECT_TRUE(payload.Init(config));
PartitionConfig old_part(kLegacyPartitionNameRoot);
+ if (minor_version != kFullPayloadMinorVersion) {
+ // When generating a delta payload we need to include the old partition
+ // information to mark it as a delta payload.
+ old_part.path = "/dev/null";
+ old_part.size = 0;
+ }
PartitionConfig new_part(kLegacyPartitionNameRoot);
new_part.path = "/dev/zero";
new_part.size = 1234;
@@ -323,7 +328,7 @@
};
TEST_F(DeltaPerformerTest, FullPayloadWriteTest) {
- install_plan_.is_full_update = true;
+ install_plan_.payload_type = InstallPayloadType::kFull;
brillo::Blob expected_data = brillo::Blob(std::begin(kRandomString),
std::end(kRandomString));
expected_data.resize(4096); // block size
@@ -342,7 +347,7 @@
}
TEST_F(DeltaPerformerTest, ShouldCancelTest) {
- install_plan_.is_full_update = true;
+ install_plan_.payload_type = InstallPayloadType::kFull;
brillo::Blob expected_data = brillo::Blob(std::begin(kRandomString),
std::end(kRandomString));
expected_data.resize(4096); // block size
@@ -522,7 +527,9 @@
manifest.mutable_new_rootfs_info();
manifest.set_minor_version(kFullPayloadMinorVersion);
- RunManifestValidation(manifest, kChromeOSMajorPayloadVersion, true,
+ RunManifestValidation(manifest,
+ kChromeOSMajorPayloadVersion,
+ InstallPayloadType::kFull,
ErrorCode::kSuccess);
}
@@ -535,7 +542,9 @@
manifest.mutable_new_rootfs_info();
manifest.set_minor_version(DeltaPerformer::kSupportedMinorPayloadVersion);
- RunManifestValidation(manifest, kChromeOSMajorPayloadVersion, false,
+ RunManifestValidation(manifest,
+ kChromeOSMajorPayloadVersion,
+ InstallPayloadType::kDelta,
ErrorCode::kSuccess);
}
@@ -543,16 +552,23 @@
// The Manifest we are validating.
DeltaArchiveManifest manifest;
- RunManifestValidation(manifest, DeltaPerformer::kSupportedMajorPayloadVersion,
- true, ErrorCode::kSuccess);
+ RunManifestValidation(manifest,
+ DeltaPerformer::kSupportedMajorPayloadVersion,
+ InstallPayloadType::kFull,
+ ErrorCode::kSuccess);
}
TEST_F(DeltaPerformerTest, ValidateManifestDeltaUnsetMinorVersion) {
// The Manifest we are validating.
DeltaArchiveManifest manifest;
+ // Add an empty old_rootfs_info() to trick the DeltaPerformer into think that
+ // this is a delta payload manifest with a missing minor version.
+ manifest.mutable_old_rootfs_info();
- RunManifestValidation(manifest, DeltaPerformer::kSupportedMajorPayloadVersion,
- false, ErrorCode::kUnsupportedMinorPayloadVersion);
+ RunManifestValidation(manifest,
+ DeltaPerformer::kSupportedMajorPayloadVersion,
+ InstallPayloadType::kDelta,
+ ErrorCode::kUnsupportedMinorPayloadVersion);
}
TEST_F(DeltaPerformerTest, ValidateManifestFullOldKernelTest) {
@@ -563,7 +579,9 @@
manifest.mutable_new_rootfs_info();
manifest.set_minor_version(DeltaPerformer::kSupportedMinorPayloadVersion);
- RunManifestValidation(manifest, kChromeOSMajorPayloadVersion, true,
+ RunManifestValidation(manifest,
+ kChromeOSMajorPayloadVersion,
+ InstallPayloadType::kFull,
ErrorCode::kPayloadMismatchedType);
}
@@ -575,7 +593,9 @@
manifest.mutable_new_rootfs_info();
manifest.set_minor_version(DeltaPerformer::kSupportedMinorPayloadVersion);
- RunManifestValidation(manifest, kChromeOSMajorPayloadVersion, true,
+ RunManifestValidation(manifest,
+ kChromeOSMajorPayloadVersion,
+ InstallPayloadType::kFull,
ErrorCode::kPayloadMismatchedType);
}
@@ -587,7 +607,9 @@
partition->mutable_new_partition_info();
manifest.set_minor_version(DeltaPerformer::kSupportedMinorPayloadVersion);
- RunManifestValidation(manifest, kBrilloMajorPayloadVersion, true,
+ RunManifestValidation(manifest,
+ kBrilloMajorPayloadVersion,
+ InstallPayloadType::kFull,
ErrorCode::kPayloadMismatchedType);
}
@@ -598,9 +620,13 @@
// Generate a bad version number.
manifest.set_minor_version(DeltaPerformer::kSupportedMinorPayloadVersion +
10000);
+ // Mark the manifest as a delta payload by setting old_rootfs_info.
+ manifest.mutable_old_rootfs_info();
- RunManifestValidation(manifest, DeltaPerformer::kSupportedMajorPayloadVersion,
- false, ErrorCode::kUnsupportedMinorPayloadVersion);
+ RunManifestValidation(manifest,
+ DeltaPerformer::kSupportedMajorPayloadVersion,
+ InstallPayloadType::kDelta,
+ ErrorCode::kUnsupportedMinorPayloadVersion);
}
TEST_F(DeltaPerformerTest, BrilloMetadataSignatureSizeTest) {
diff --git a/payload_consumer/download_action_unittest.cc b/payload_consumer/download_action_unittest.cc
index 60cc6f2..979776f 100644
--- a/payload_consumer/download_action_unittest.cc
+++ b/payload_consumer/download_action_unittest.cc
@@ -143,14 +143,10 @@
// We pull off the first byte from data and seek past it.
string hash = HashCalculator::HashOfBytes(&data[1], data.size() - 1);
uint64_t size = data.size();
- InstallPlan install_plan(false,
- false,
- "",
- size,
- hash,
- 0,
- "",
- "");
+ InstallPlan install_plan;
+ install_plan.payload_type = InstallPayloadType::kDelta;
+ install_plan.payload_size = size;
+ install_plan.payload_hash = hash;
install_plan.source_slot = 0;
install_plan.target_slot = 1;
// We mark both slots as bootable. Only the target slot should be unbootable
@@ -278,7 +274,7 @@
// takes ownership of passed in HttpFetcher
ObjectFeederAction<InstallPlan> feeder_action;
- InstallPlan install_plan(false, false, "", 0, "", 0, "", "");
+ InstallPlan install_plan;
feeder_action.set_obj(install_plan);
FakeSystemState fake_system_state_;
MockPrefs prefs;
@@ -375,14 +371,9 @@
EXPECT_EQ(0, writer.Open("/dev/null", O_WRONLY | O_CREAT, 0));
// takes ownership of passed in HttpFetcher
- InstallPlan install_plan(false,
- false,
- "",
- 1,
- HashCalculator::HashOfString("x"),
- 0,
- "",
- "");
+ InstallPlan install_plan;
+ install_plan.payload_size = 1;
+ install_plan.payload_hash = HashCalculator::HashOfString("x");
ObjectFeederAction<InstallPlan> feeder_action;
feeder_action.set_obj(install_plan);
MockPrefs prefs;
@@ -463,14 +454,9 @@
EXPECT_EQ(0, writer.Open(output_temp_file.GetPath().c_str(),
O_WRONLY | O_CREAT,
0));
- InstallPlan install_plan(false,
- false,
- "",
- data_.length(),
- "1234hash",
- 0,
- "",
- "");
+ InstallPlan install_plan;
+ install_plan.payload_size = data_.length();
+ install_plan.payload_hash = "1234hash";
ObjectFeederAction<InstallPlan> feeder_action;
feeder_action.set_obj(install_plan);
MockPrefs prefs;
diff --git a/payload_consumer/filesystem_verifier_action.cc b/payload_consumer/filesystem_verifier_action.cc
index 8530d37..759b455 100644
--- a/payload_consumer/filesystem_verifier_action.cc
+++ b/payload_consumer/filesystem_verifier_action.cc
@@ -59,7 +59,8 @@
// For delta updates (major version 1) we need to populate the source
// partition hash if not pre-populated.
- if (!install_plan_.is_full_update && install_plan_.partitions.empty() &&
+ if (install_plan_.payload_type == InstallPayloadType::kDelta &&
+ install_plan_.partitions.empty() &&
verifier_mode_ == VerifierMode::kComputeSourceHash &&
DeltaPerformer::kSupportedMinorPayloadVersion <
kOpSrcHashMinorPayloadVersion) {
diff --git a/payload_consumer/filesystem_verifier_action_unittest.cc b/payload_consumer/filesystem_verifier_action_unittest.cc
index 0b6232b..10daaa8 100644
--- a/payload_consumer/filesystem_verifier_action_unittest.cc
+++ b/payload_consumer/filesystem_verifier_action_unittest.cc
@@ -285,14 +285,7 @@
processor.set_delegate(&delegate);
ObjectFeederAction<InstallPlan> feeder_action;
- InstallPlan install_plan(false,
- false,
- "",
- 0,
- "",
- 0,
- "",
- "");
+ InstallPlan install_plan;
InstallPlan::Partition part;
part.name = "nope";
part.source_path = "/no/such/file";
diff --git a/payload_consumer/install_plan.cc b/payload_consumer/install_plan.cc
index a2a1b7b..572ff41 100644
--- a/payload_consumer/install_plan.cc
+++ b/payload_consumer/install_plan.cc
@@ -27,29 +27,21 @@
namespace chromeos_update_engine {
-InstallPlan::InstallPlan(bool is_resume,
- bool is_full_update,
- const string& url,
- uint64_t payload_size,
- const string& payload_hash,
- uint64_t metadata_size,
- const string& metadata_signature,
- const string& public_key_rsa)
- : is_resume(is_resume),
- is_full_update(is_full_update),
- download_url(url),
- payload_size(payload_size),
- payload_hash(payload_hash),
- metadata_size(metadata_size),
- metadata_signature(metadata_signature),
- hash_checks_mandatory(false),
- powerwash_required(false),
- public_key_rsa(public_key_rsa) {}
-
+string InstallPayloadTypeToString(InstallPayloadType type) {
+ switch (type) {
+ case InstallPayloadType::kUnknown:
+ return "unknown";
+ case InstallPayloadType::kFull:
+ return "full";
+ case InstallPayloadType::kDelta:
+ return "delta";
+ }
+ return "invalid type";
+}
bool InstallPlan::operator==(const InstallPlan& that) const {
return ((is_resume == that.is_resume) &&
- (is_full_update == that.is_full_update) &&
+ (payload_type == that.payload_type) &&
(download_url == that.download_url) &&
(payload_size == that.payload_size) &&
(payload_hash == that.payload_hash) &&
@@ -74,7 +66,7 @@
LOG(INFO) << "InstallPlan: "
<< (is_resume ? "resume" : "new_update")
- << ", payload type: " << (is_full_update ? "full" : "delta")
+ << ", payload type: " << InstallPayloadTypeToString(payload_type)
<< ", source_slot: " << BootControlInterface::SlotName(source_slot)
<< ", target_slot: " << BootControlInterface::SlotName(target_slot)
<< ", url: " << download_url
@@ -85,8 +77,7 @@
<< partitions_str
<< ", hash_checks_mandatory: " << utils::ToString(
hash_checks_mandatory)
- << ", powerwash_required: " << utils::ToString(
- powerwash_required);
+ << ", powerwash_required: " << utils::ToString(powerwash_required);
}
bool InstallPlan::LoadPartitionsFromSlots(BootControlInterface* boot_control) {
diff --git a/payload_consumer/install_plan.h b/payload_consumer/install_plan.h
index e69462d..d2f15fa 100644
--- a/payload_consumer/install_plan.h
+++ b/payload_consumer/install_plan.h
@@ -30,17 +30,15 @@
// parts of the update system about the install that should happen.
namespace chromeos_update_engine {
-struct InstallPlan {
- InstallPlan(bool is_resume,
- bool is_full_update,
- const std::string& url,
- uint64_t payload_size,
- const std::string& payload_hash,
- uint64_t metadata_size,
- const std::string& metadata_signature,
- const std::string& public_key_rsa);
+enum class InstallPayloadType {
+ kUnknown,
+ kFull,
+ kDelta,
+};
- // Default constructor.
+std::string InstallPayloadTypeToString(InstallPayloadType type);
+
+struct InstallPlan {
InstallPlan() = default;
bool operator==(const InstallPlan& that) const;
@@ -54,7 +52,7 @@
bool LoadPartitionsFromSlots(BootControlInterface* boot_control);
bool is_resume{false};
- bool is_full_update{false};
+ InstallPayloadType payload_type{InstallPayloadType::kUnknown};
std::string download_url; // url to download from
std::string version; // version we are installing.
