Cancel the current download if user chooses a different channel.
In my earlier CL, to keep the implementation simple, we disallowed changing
a channel until the previous change completed in its entirety. Given that
the UI is not going to be updated for M27, such a restriction turned out
to be very confusing when playing around with channel changing. So, we
decided to implement a simple form of canceling the download if the
user selected a different channel while we're downloading the bits. This
implementation can easily be extended to support a general form of cancel
in the future, if required.
This CL also adds validation of libchromeos API calls when interpreting
the policy values. It also cleans up some bogus error messages that were
logged earlier when we abort a download.
BUG=chromium:222617
TEST=All scenarios pass on ZGB. Unit Tests pass.
Change-Id: I7cd691fe461d9ce47314299f6e2598944650ee33
Reviewed-on: https://gerrit.chromium.org/gerrit/46095
Commit-Queue: Jay Srinivasan <jaysri@chromium.org>
Reviewed-by: Jay Srinivasan <jaysri@chromium.org>
Tested-by: Jay Srinivasan <jaysri@chromium.org>
diff --git a/update_attempter.cc b/update_attempter.cc
index c287fe3..8ebe267 100644
--- a/update_attempter.cc
+++ b/update_attempter.cc
@@ -184,6 +184,24 @@
UpdateBootFlags();
}
+void UpdateAttempter::RefreshDevicePolicy() {
+ // Lazy initialize the policy provider, or reload the latest policy data.
+ if (!policy_provider_.get())
+ policy_provider_.reset(new policy::PolicyProvider());
+ policy_provider_->Reload();
+
+ const policy::DevicePolicy* device_policy = NULL;
+ if (policy_provider_->device_policy_is_loaded())
+ device_policy = &policy_provider_->GetDevicePolicy();
+
+ if (device_policy)
+ LOG(INFO) << "Device policies/settings present";
+ else
+ LOG(INFO) << "No device policies/settings present.";
+
+ system_state_->set_device_policy(device_policy);
+}
+
bool UpdateAttempter::CalculateUpdateParams(const string& app_version,
const string& omaha_url,
bool obey_proxies,
@@ -193,31 +211,20 @@
// Set the test mode flag for the current update attempt.
is_test_mode_ = is_test_mode;
-
- // Lazy initialize the policy provider, or reload the latest policy data.
- if (!policy_provider_.get())
- policy_provider_.reset(new policy::PolicyProvider());
- policy_provider_->Reload();
-
- if (policy_provider_->device_policy_is_loaded()) {
- LOG(INFO) << "Device policies/settings present";
-
- const policy::DevicePolicy& device_policy =
- policy_provider_->GetDevicePolicy();
-
+ RefreshDevicePolicy();
+ const policy::DevicePolicy* device_policy = system_state_->device_policy();
+ if (device_policy) {
bool update_disabled = false;
- device_policy.GetUpdateDisabled(&update_disabled);
- omaha_request_params_->set_update_disabled(update_disabled);
+ if (device_policy->GetUpdateDisabled(&update_disabled))
+ omaha_request_params_->set_update_disabled(update_disabled);
string target_version_prefix;
- device_policy.GetTargetVersionPrefix(&target_version_prefix);
- omaha_request_params_->set_target_version_prefix(target_version_prefix);
-
- system_state_->set_device_policy(&device_policy);
+ if (device_policy->GetTargetVersionPrefix(&target_version_prefix))
+ omaha_request_params_->set_target_version_prefix(target_version_prefix);
set<string> allowed_types;
string allowed_types_str;
- if (device_policy.GetAllowedConnectionTypesForUpdate(&allowed_types)) {
+ if (device_policy->GetAllowedConnectionTypesForUpdate(&allowed_types)) {
set<string>::const_iterator iter;
for (iter = allowed_types.begin(); iter != allowed_types.end(); ++iter)
allowed_types_str += *iter + " ";
@@ -225,9 +232,6 @@
LOG(INFO) << "Networks over which updates are allowed per policy : "
<< (allowed_types_str.empty() ? "all" : allowed_types_str);
- } else {
- LOG(INFO) << "No device policies/settings present.";
- system_state_->set_device_policy(NULL);
}
CalculateScatteringParams(interactive);
@@ -242,31 +246,37 @@
if (!omaha_request_params_->Init(app_version,
omaha_url_to_use,
interactive)) {
- LOG(ERROR) << "Unable to initialize Omaha request device params.";
+ LOG(ERROR) << "Unable to initialize Omaha request params.";
return false;
}
// Set the target channel iff ReleaseChannelDelegated policy is set to
// false and a non-empty ReleaseChannel policy is present. If delegated
// is true, we'll ignore ReleaseChannel policy value.
- if (system_state_->device_policy()) {
+ if (device_policy) {
bool delegated = false;
- system_state_->device_policy()->GetReleaseChannelDelegated(&delegated);
- if (delegated) {
+ if (device_policy->GetReleaseChannelDelegated(&delegated) && delegated) {
LOG(INFO) << "Channel settings are delegated to user by policy. "
"Ignoring ReleaseChannel policy value";
}
else {
LOG(INFO) << "Channel settings are not delegated to the user by policy";
string target_channel;
- system_state_->device_policy()->GetReleaseChannel(&target_channel);
- if (target_channel.empty()) {
- LOG(INFO) << "No ReleaseChannel specified in policy";
- } else {
+ if (device_policy->GetReleaseChannel(&target_channel) &&
+ !target_channel.empty()) {
// Pass in false for powerwash_allowed until we add it to the policy
// protobuf.
LOG(INFO) << "Setting target channel from ReleaseChannel policy value";
omaha_request_params_->SetTargetChannel(target_channel, false);
+
+ // Since this is the beginning of a new attempt, update the download
+ // channel. The download channel won't be updated until the next
+ // attempt, even if target channel changes meanwhile, so that how we'll
+ // know if we should cancel the current download attempt if there's
+ // such a change in target channel.
+ omaha_request_params_->UpdateDownloadChannel();
+ } else {
+ LOG(INFO) << "No ReleaseChannel specified in policy";
}
}
}
@@ -756,14 +766,18 @@
return true;
case UPDATE_STATUS_UPDATED_NEED_REBOOT: {
+ bool ret_value = true;
status_ = UPDATE_STATUS_IDLE;
LOG(INFO) << "Reset Successful";
- // also remove the reboot marker so that if the machine is rebooted
+ // Remove the reboot marker so that if the machine is rebooted
// after resetting to idle state, it doesn't go back to
// UPDATE_STATUS_UPDATED_NEED_REBOOT state.
const FilePath kUpdateCompletedMarkerPath(kUpdateCompletedMarker);
- return file_util::Delete(kUpdateCompletedMarkerPath, false);
+ if (!file_util::Delete(kUpdateCompletedMarkerPath, false))
+ ret_value = false;
+
+ return ret_value;
}
default:
@@ -856,6 +870,22 @@
return flags;
}
+bool UpdateAttempter::ShouldCancel(ActionExitCode* cancel_reason) {
+ // Check if the channel we're attempting to update to is the same as the
+ // target channel currently chosen by the user.
+ OmahaRequestParams* params = system_state_->request_params();
+ if (params->download_channel() != params->target_channel()) {
+ LOG(ERROR) << "Aborting download as target channel: "
+ << params->target_channel()
+ << " is different from the download channel: "
+ << params->download_channel();
+ *cancel_reason = kActionCodeUpdateCanceledByChannelChange;
+ return true;
+ }
+
+ return false;
+}
+
void UpdateAttempter::SetStatusAndNotify(UpdateStatus status,
UpdateNotice notice) {
status_ = status;
@@ -1115,5 +1145,4 @@
prefs_->Delete(kPrefsUpdateCheckCount);
return false;
}
-
} // namespace chromeos_update_engine