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/SConstruct b/SConstruct
index 6594f24..85a11d1 100644
--- a/SConstruct
+++ b/SConstruct
@@ -246,6 +246,7 @@
chrome_browser_proxy_resolver.cc
chrome_proxy_resolver.cc
connection_manager.cc
+ constants.cc
cycle_breaker.cc
dbus_service.cc
delta_diff_generator.cc
diff --git a/action_processor.h b/action_processor.h
index 7d2e9f8..38d63ae 100644
--- a/action_processor.h
+++ b/action_processor.h
@@ -65,6 +65,7 @@
kActionCodeDownloadMetadataSignatureMissingError = 39,
kActionCodeOmahaUpdateDeferredForBackoff = 40,
kActionCodePostinstallPowerwashError = 41,
+ kActionCodeUpdateCanceledByChannelChange = 42,
// Note: When adding new error codes, please remember to add the
// error into one of the buckets in PayloadState::UpdateFailed method so
diff --git a/constants.cc b/constants.cc
new file mode 100644
index 0000000..4b87a58
--- /dev/null
+++ b/constants.cc
@@ -0,0 +1,13 @@
+// Copyright (c) 2013 The Chromium OS Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "update_engine/constants.h"
+
+namespace chromeos_update_engine {
+
+const char kPowerwashMarkerFile[] =
+ "/mnt/stateful_partition/factory_install_reset";
+
+const char kPowerwashCommand[] = "safe fast\n";
+}
diff --git a/constants.h b/constants.h
new file mode 100644
index 0000000..d6fd736
--- /dev/null
+++ b/constants.h
@@ -0,0 +1,19 @@
+// Copyright (c) 2013 The Chromium OS Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CHROMEOS_PLATFORM_UPDATE_ENGINE_CONSTANTS_H
+#define CHROMEOS_PLATFORM_UPDATE_ENGINE_CONSTANTS_H
+
+namespace chromeos_update_engine {
+
+// The name of the marker file used to trigger powerwash when post-install
+// completes successfully so that the device is powerwashed on next reboot.
+extern const char kPowerwashMarkerFile[];
+
+// The contents of the powerwash marker file.
+extern const char kPowerwashCommand[];
+
+} // namespace chromeos_update_engine
+
+#endif // CHROMEOS_PLATFORM_UPDATE_ENGINE_CONSTANTS_H
diff --git a/dbus_service.cc b/dbus_service.cc
index b4cc7c8..97aae35 100644
--- a/dbus_service.cc
+++ b/dbus_service.cc
@@ -7,6 +7,7 @@
#include <string>
#include <base/logging.h>
+#include <policy/device_policy.h>
#include "update_engine/marshal.glibmarshal.h"
#include "update_engine/omaha_request_params.h"
@@ -145,7 +146,17 @@
gchar* track,
GError **error) {
// track == target channel.
- return update_engine_service_set_channel(self, track, false, error);
+ // TODO(jaysri): Remove this method once chromium:219292 is fixed.
+ // Since UI won't be ready for now, preserve the existing
+ // behavior for set_track by calling SetTargetChannel directly without the
+ // policy checks instead of calling update_engine_service_set_channel.
+ LOG(INFO) << "Setting destination track to: " << track;
+ if (!self->system_state_->request_params()->SetTargetChannel(track, false)) {
+ *error = NULL;
+ return FALSE;
+ }
+
+ return TRUE;
}
gboolean update_engine_service_get_track(UpdateEngineService* self,
@@ -162,18 +173,16 @@
if (!target_channel)
return FALSE;
- if (!self->system_state_->device_policy()) {
+ const policy::DevicePolicy* device_policy =
+ self->system_state_->device_policy();
+ if (!device_policy) {
LOG(INFO) << "Cannot set target channel until device policy/settings are "
"known";
return FALSE;
}
bool delegated = false;
- self->system_state_->device_policy()->GetReleaseChannelDelegated(&delegated);
- if (!delegated) {
- // Note: This message will appear in UE logs with the current UI code
- // because UI hasn't been modified to call this method only if
- // delegated is set to true. chromium-os:219292 tracks this work item.
+ if (!(device_policy->GetReleaseChannelDelegated(&delegated) && delegated)) {
LOG(INFO) << "Cannot set target channel explicitly when channel "
"policy/settings is not delegated";
return FALSE;
diff --git a/delta_performer.cc b/delta_performer.cc
index c299d79..b0c8c17 100644
--- a/delta_performer.cc
+++ b/delta_performer.cc
@@ -27,6 +27,7 @@
#include "update_engine/prefs_interface.h"
#include "update_engine/subprocess.h"
#include "update_engine/terminator.h"
+#include "update_engine/update_attempter.h"
using std::min;
using std::string;
@@ -262,10 +263,9 @@
fd_ = -2; // Set to invalid so that calls to Open() will fail.
path_ = "";
if (!buffer_.empty()) {
- LOG(ERROR) << "Called Close() while buffer not empty!";
- if (err >= 0) {
+ LOG(INFO) << "Discarding " << buffer_.size() << " unused downloaded bytes";
+ if (err >= 0)
err = 1;
- }
}
return -err;
}
@@ -457,6 +457,12 @@
}
while (next_operation_num_ < num_total_operations_) {
+ // Check if we should cancel the current attempt for any reason.
+ // In this case, *error will have already been populated with the reason
+ // why we're cancelling.
+ if (system_state_->update_attempter()->ShouldCancel(error))
+ return false;
+
const bool is_kernel_partition =
(next_operation_num_ >= num_rootfs_operations_);
const DeltaArchiveManifest_InstallOperation &op =
diff --git a/download_action.cc b/download_action.cc
index 8aa9e35..1ee3a8e 100644
--- a/download_action.cc
+++ b/download_action.cc
@@ -77,7 +77,7 @@
void DownloadAction::TerminateProcessing() {
if (writer_) {
- LOG_IF(WARNING, writer_->Close() != 0) << "Error closing the writer.";
+ writer_->Close();
writer_ = NULL;
}
if (delegate_) {
diff --git a/omaha_request_action.cc b/omaha_request_action.cc
index 14ec8f6..6391208 100644
--- a/omaha_request_action.cc
+++ b/omaha_request_action.cc
@@ -113,7 +113,7 @@
// Returns an XML that goes into the body of the <app> element of the Omaha
// request based on the given parameters.
string GetAppBody(const OmahaEvent* event,
- const OmahaRequestParams& params,
+ OmahaRequestParams* params,
bool ping_only,
int ping_active_days,
int ping_roll_call_days,
@@ -130,7 +130,7 @@
app_body += StringPrintf(
" <updatecheck targetversionprefix=\"%s\""
"></updatecheck>\n",
- XmlEncode(params.target_version_prefix()).c_str());
+ XmlEncode(params->target_version_prefix()).c_str());
// If this is the first update check after a reboot following a previous
// update, generate an event containing the previous version number. If
@@ -171,7 +171,7 @@
// Returns an XML that corresponds to the entire <app> node of the Omaha
// request based on the given parameters.
string GetAppXml(const OmahaEvent* event,
- const OmahaRequestParams& params,
+ OmahaRequestParams* params,
bool ping_only,
int ping_active_days,
int ping_roll_call_days,
@@ -183,34 +183,35 @@
// If we are upgrading to a more stable channel and we are allowed to do
// powerwash, then pass 0.0.0.0 as the version. This is needed to get the
// highest-versioned payload on the destination channel.
- if (params.to_more_stable_channel() && params.is_powerwash_allowed()) {
+ if (params->to_more_stable_channel() && params->is_powerwash_allowed()) {
LOG(INFO) << "Passing OS version as 0.0.0.0 as we are set to powerwash "
<< "on downgrading to the version in the more stable channel";
app_versions = "version=\"0.0.0.0\" from_version=\"" +
- XmlEncode(params.app_version()) + "\" ";
+ XmlEncode(params->app_version()) + "\" ";
} else {
- app_versions = "version=\"" + XmlEncode(params.app_version()) + "\" ";
+ app_versions = "version=\"" + XmlEncode(params->app_version()) + "\" ";
}
- string app_channels = "track=\"" + XmlEncode(params.target_channel()) + "\" ";
- if (params.current_channel() != params.target_channel())
+ string download_channel = params->download_channel();
+ string app_channels = "track=\"" + XmlEncode(download_channel) + "\" ";
+ if (params->current_channel() != download_channel)
app_channels +=
- "from_track=\"" + XmlEncode(params.current_channel()) + "\" ";
+ "from_track=\"" + XmlEncode(params->current_channel()) + "\" ";
- string delta_okay_str = params.delta_okay() ? "true" : "false";
+ string delta_okay_str = params->delta_okay() ? "true" : "false";
// Use the default app_id only if we're asking for an update on the
// canary-channel. Otherwise, use the board's app_id.
string request_app_id =
- (params.target_channel() == "canary-channel" ?
- params.app_id() : params.board_app_id());
+ (download_channel == "canary-channel" ?
+ params->app_id() : params->board_app_id());
string app_xml =
" <app appid=\"" + XmlEncode(request_app_id) + "\" " +
app_versions +
app_channels +
- "lang=\"" + XmlEncode(params.app_lang()) + "\" " +
- "board=\"" + XmlEncode(params.os_board()) + "\" " +
- "hardware_class=\"" + XmlEncode(params.hwid()) + "\" " +
+ "lang=\"" + XmlEncode(params->app_lang()) + "\" " +
+ "board=\"" + XmlEncode(params->os_board()) + "\" " +
+ "hardware_class=\"" + XmlEncode(params->hwid()) + "\" " +
"delta_okay=\"" + delta_okay_str + "\" "
">\n" +
app_body +
@@ -221,11 +222,11 @@
// Returns an XML that corresponds to the entire <os> node of the Omaha
// request based on the given parameters.
-string GetOsXml(const OmahaRequestParams& params) {
+string GetOsXml(OmahaRequestParams* params) {
string os_xml =
- " <os version=\"" + XmlEncode(params.os_version()) + "\" " +
- "platform=\"" + XmlEncode(params.os_platform()) + "\" " +
- "sp=\"" + XmlEncode(params.os_sp()) + "\">"
+ " <os version=\"" + XmlEncode(params->os_version()) + "\" " +
+ "platform=\"" + XmlEncode(params->os_platform()) + "\" " +
+ "sp=\"" + XmlEncode(params->os_sp()) + "\">"
"</os>\n";
return os_xml;
}
@@ -233,7 +234,7 @@
// Returns an XML that corresponds to the entire Omaha request based on the
// given parameters.
string GetRequestXml(const OmahaEvent* event,
- const OmahaRequestParams& params,
+ OmahaRequestParams* params,
bool ping_only,
int ping_active_days,
int ping_roll_call_days,
@@ -243,7 +244,7 @@
ping_roll_call_days, system_state);
string install_source = StringPrintf("installsource=\"%s\" ",
- (params.interactive() ? "ondemandupdate" : "scheduler"));
+ (params->interactive() ? "ondemandupdate" : "scheduler"));
string request_xml =
"<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n"
@@ -334,7 +335,7 @@
return;
}
string request_post(GetRequestXml(event_.get(),
- *params_,
+ params_,
ping_only_,
ping_active_days_,
ping_roll_call_days_,
diff --git a/omaha_request_params.cc b/omaha_request_params.cc
index 53c0975..bd94b2d 100644
--- a/omaha_request_params.cc
+++ b/omaha_request_params.cc
@@ -50,6 +50,7 @@
bool OmahaRequestParams::Init(const std::string& in_app_version,
const std::string& in_update_url,
bool in_interactive) {
+ LOG(INFO) << "Initializing parameters for this update attempt";
InitFromLsbValue();
bool stateful_override = !ShouldLockDown();
os_platform_ = OmahaRequestParams::kOsPlatform;
@@ -104,34 +105,11 @@
bool OmahaRequestParams::SetTargetChannel(const std::string& new_target_channel,
bool is_powerwash_allowed) {
LOG(INFO) << "SetTargetChannel called with " << new_target_channel
- << ". Is Powerwash Allowed = "
- << utils::ToString(is_powerwash_allowed);
-
- // Ignore duplicate calls so we can make the method succeed and be
- // idempotent so as not to surface unnecessary errors to the UI.
- if (new_target_channel == target_channel_ &&
- is_powerwash_allowed == is_powerwash_allowed_) {
- if (new_target_channel == current_channel_) {
- // Return true to make such calls no-op and idempotent.
- LOG(INFO) << "SetTargetChannel: Already on " << current_channel_;
- return true;
- }
-
- LOG(INFO) << "SetTargetChannel: Target channel has already been set";
- return true;
- }
-
- // See if there's a channel change already in progress. If so, don't honor
- // a new channel change until the existing request is fulfilled.
- if (current_channel_ != target_channel_) {
- // Avoid dealing with multiple pending channels as they cause a lot of
- // edge cases that's not worth adding the complexity for.
- LOG(ERROR) << "Cannot change to " << new_target_channel
- << " now as we're currently in " << current_channel_
- << " and the request to change to " << target_channel_
- << " is pending";
- return false;
- }
+ << ", Is Powerwash Allowed = "
+ << utils::ToString(is_powerwash_allowed)
+ << ". Current channel = " << current_channel_
+ << ", existing target channel = " << target_channel_
+ << ", download channel = " << download_channel_;
if (current_channel_ == "canary-channel") {
// TODO(jaysri): chromium-os:39751: We don't have the UI warnings yet. So,
@@ -171,7 +149,7 @@
void OmahaRequestParams::SetTargetChannelFromLsbValue() {
string target_channel_new_value = GetLsbValue(
kUpdateChannelKey,
- "",
+ current_channel_,
&chromeos_update_engine::OmahaRequestParams::IsValidChannel,
true); // stateful_override
@@ -185,7 +163,7 @@
void OmahaRequestParams::SetCurrentChannelFromLsbValue() {
string current_channel_new_value = GetLsbValue(
kUpdateChannelKey,
- "",
+ current_channel_,
NULL, // No need to validate the read-only rootfs channel.
false); // stateful_override is false so we get the current channel.
@@ -211,10 +189,18 @@
}
}
+void OmahaRequestParams::UpdateDownloadChannel() {
+ if (download_channel_ != target_channel_) {
+ download_channel_ = target_channel_;
+ LOG(INFO) << "Download channel for this attempt = " << download_channel_;
+ }
+}
+
void OmahaRequestParams::InitFromLsbValue() {
- SetTargetChannelFromLsbValue();
SetCurrentChannelFromLsbValue();
+ SetTargetChannelFromLsbValue();
SetIsPowerwashAllowedFromLsbValue();
+ UpdateDownloadChannel();
}
string OmahaRequestParams::GetLsbValue(const string& key,
@@ -286,9 +272,9 @@
bool OmahaRequestParams::to_more_stable_channel() const {
int current_channel_index = GetChannelIndex(current_channel_);
- int target_channel_index = GetChannelIndex(target_channel_);
+ int download_channel_index = GetChannelIndex(download_channel_);
- return target_channel_index > current_channel_index;
+ return download_channel_index > current_channel_index;
}
} // namespace chromeos_update_engine
diff --git a/omaha_request_params.h b/omaha_request_params.h
index 3420b41..ada90a8 100644
--- a/omaha_request_params.h
+++ b/omaha_request_params.h
@@ -105,6 +105,7 @@
inline std::string current_channel() const { return current_channel_; }
inline std::string target_channel() const { return target_channel_; }
+ inline std::string download_channel() const { return download_channel_; }
// Can client accept a delta ?
inline void set_delta_okay(bool ok) { delta_okay_ = ok; }
@@ -193,6 +194,14 @@
// all such cases.
bool SetTargetChannel(const std::string& channel, bool is_powerwash_allowed);
+ // Updates the download channel for this particular attempt from the current
+ // value of target channel. This method takes a "snapshot" of the current
+ // value of target channel and uses it for all subsequent Omaha requests for
+ // this attempt (i.e. initial request as well as download progress/error
+ // event requests). The snapshot will be updated only when either this method
+ // or Init is called again.
+ void UpdateDownloadChannel();
+
bool is_powerwash_allowed() const { return is_powerwash_allowed_; }
// For unit-tests.
@@ -266,10 +275,26 @@
std::string app_version_;
std::string app_lang_;
- // Current channel and target channel. Usually there are same, except when
- // there's a pending channel change.
+ // The three channel values we deal with.
+ // Current channel: is always the channel from /etc/lsb-release. It never
+ // changes. It's just read in during initialization.
std::string current_channel_;
+
+ // Target channel: It starts off with the value of current channel. But if
+ // the user changes the channel, then it'll have a different value. If the
+ // user changes multiple times, target channel will always contain the most
+ // recent change and is updated immediately to the user-selected value even
+ // if we're in the middle of a download (as opposed to download channel
+ // which gets updated only at the start of next download)
std::string target_channel_;
+
+ // The channel from which we're downloading the payload. This should normally
+ // be the same as target channel. But if the user made another channel change
+ // we started the download, then they'd be different, in which case, we'd
+ // detect elsewhere that the target channel has been changed and cancel the
+ // current download attempt.
+ std::string download_channel_;
+
std::string hwid_; // Hardware Qualification ID of the client
bool delta_okay_; // If this client can accept a delta
bool interactive_; // Whether this is a user-initiated update check
diff --git a/omaha_request_params_unittest.cc b/omaha_request_params_unittest.cc
index 3c76cd2..7d81ba7 100644
--- a/omaha_request_params_unittest.cc
+++ b/omaha_request_params_unittest.cc
@@ -467,9 +467,29 @@
EXPECT_EQ("beta-channel", params_.target_channel());
// When set to a valid value while a change is already pending, it should
- // fail.
+ // succeed.
params_.Init("", "", false);
- EXPECT_FALSE(params_.SetTargetChannel("stable-channel", true));
+ EXPECT_TRUE(params_.SetTargetChannel("stable-channel", true));
+ EXPECT_EQ("stable-channel", params_.target_channel());
+
+ // Set a different channel in stateful LSB release.
+ ASSERT_TRUE(WriteFileString(
+ kTestDir + utils::kStatefulPartition + "/etc/lsb-release",
+ "CHROMEOS_RELEASE_TRACK=stable-channel\n"
+ "CHROMEOS_IS_POWERWASH_ALLOWED=true\n"));
+
+ // When set to a valid value while a change is already pending, it should
+ // succeed.
+ params_.Init("", "", false);
+ EXPECT_TRUE(params_.SetTargetChannel("beta-channel", true));
+ // The target channel should reflect the change, but the download channel
+ // should continue to retain the old value ...
+ EXPECT_EQ("beta-channel", params_.target_channel());
+ EXPECT_EQ("stable-channel", params_.download_channel());
+
+ // ... until we update the download channel explicitly.
+ params_.UpdateDownloadChannel();
+ EXPECT_EQ("beta-channel", params_.download_channel());
EXPECT_EQ("beta-channel", params_.target_channel());
}
diff --git a/omaha_response_handler_action.cc b/omaha_response_handler_action.cc
index d705549..9c3c0db 100644
--- a/omaha_response_handler_action.cc
+++ b/omaha_response_handler_action.cc
@@ -67,10 +67,10 @@
install_plan_.kernel_install_path =
utils::BootKernelDevice(install_plan_.install_path);
- if (system_state_->request_params()->to_more_stable_channel() &&
- system_state_->request_params()->is_powerwash_allowed()) {
+ OmahaRequestParams* params = system_state_->request_params();
+ if (params->to_more_stable_channel() && params->is_powerwash_allowed())
install_plan_.powerwash_required = true;
- }
+
TEST_AND_RETURN(HasOutputPipe());
if (HasOutputPipe())
diff --git a/payload_state.cc b/payload_state.cc
index 1225947..f5b8f36 100644
--- a/payload_state.cc
+++ b/payload_state.cc
@@ -172,6 +172,7 @@
case kActionCodeOmahaUpdateDeferredPerPolicy:
case kActionCodeOmahaUpdateDeferredForBackoff:
case kActionCodePostinstallPowerwashError:
+ case kActionCodeUpdateCanceledByChannelChange:
LOG(INFO) << "Not incrementing URL index or failure count for this error";
break;
diff --git a/payload_state_unittest.cc b/payload_state_unittest.cc
index dfbbbaa..e647a8c 100644
--- a/payload_state_unittest.cc
+++ b/payload_state_unittest.cc
@@ -58,7 +58,7 @@
class PayloadStateTest : public ::testing::Test { };
TEST(PayloadStateTest, DidYouAddANewActionExitCode) {
- if (kActionCodeUmaReportedMax != 42) {
+ if (kActionCodeUmaReportedMax != 43) {
LOG(ERROR) << "The following failure is intentional. If you added a new "
<< "ActionExitCode enum value, make sure to add it to the "
<< "PayloadState::UpdateFailed method and then update this test "
diff --git a/postinstall_runner_action.cc b/postinstall_runner_action.cc
index 1baa9ec..934b461 100644
--- a/postinstall_runner_action.cc
+++ b/postinstall_runner_action.cc
@@ -3,9 +3,12 @@
// found in the LICENSE file.
#include "update_engine/postinstall_runner_action.h"
+
#include <sys/mount.h>
#include <stdlib.h>
#include <vector>
+
+#include "update_engine/constants.h"
#include "update_engine/subprocess.h"
#include "update_engine/utils.h"
@@ -16,9 +19,6 @@
namespace {
const char kPostinstallScript[] = "/postinst";
-const char kPowerwashMarkerFile[] =
- "/mnt/stateful_partition/factory_install_reset";
-const char kPowerwashCommand[] = "safe fast\n";
}
void PostinstallRunnerAction::PerformAction() {
@@ -55,6 +55,15 @@
temp_dir_remover.set_should_remove(false);
completer.set_should_complete(false);
+ if (install_plan.powerwash_required) {
+ if (utils::CreatePowerwashMarkerFile()) {
+ powerwash_marker_created_ = true;
+ } else {
+ completer.set_code(kActionCodePostinstallPowerwashError);
+ return;
+ }
+ }
+
// Runs the postinstall script asynchronously to free up the main loop while
// it's running.
vector<string> command;
@@ -70,6 +79,11 @@
ScopedTempUnmounter temp_unmounter(temp_rootfs_dir_);
if (return_code != 0) {
LOG(ERROR) << "Postinst command failed with code: " << return_code;
+
+ // Undo any changes done to trigger Powerwash using clobber-state.
+ if (powerwash_marker_created_)
+ utils::DeletePowerwashMarkerFile();
+
if (return_code == 3) {
// This special return code means that we tried to update firmware,
// but couldn't because we booted from FW B, and we need to reboot
@@ -83,18 +97,6 @@
CHECK(HasInputObject());
const InstallPlan install_plan = GetInputObject();
- if (install_plan.powerwash_required) {
- if (utils::WriteFile(kPowerwashMarkerFile,
- kPowerwashCommand,
- strlen(kPowerwashCommand))) {
- LOG(INFO) << "Configured clobber-state to do powerwash on next reboot";
- } else {
- LOG(ERROR) << "Error in configuring clobber-state to do powerwash";
- completer.set_code(kActionCodePostinstallPowerwashError);
- return;
- }
- }
-
if (HasOutputPipe())
SetOutputObject(install_plan);
diff --git a/postinstall_runner_action.h b/postinstall_runner_action.h
index 9a2ebcb..6979975 100644
--- a/postinstall_runner_action.h
+++ b/postinstall_runner_action.h
@@ -29,7 +29,9 @@
class PostinstallRunnerAction : public Action<PostinstallRunnerAction> {
public:
- PostinstallRunnerAction() {}
+ PostinstallRunnerAction()
+ : powerwash_marker_created_(false) {}
+
typedef ActionTraits<PostinstallRunnerAction>::InputObjectType
InputObjectType;
typedef ActionTraits<PostinstallRunnerAction>::OutputObjectType
@@ -52,6 +54,10 @@
std::string temp_rootfs_dir_;
+ // True if Powerwash Marker was created before invoking post-install script.
+ // False otherwise. Used for cleaning up if post-install fails.
+ bool powerwash_marker_created_;
+
DISALLOW_COPY_AND_ASSIGN(PostinstallRunnerAction);
};
diff --git a/postinstall_runner_action_unittest.cc b/postinstall_runner_action_unittest.cc
index 65f378e..81f4420 100644
--- a/postinstall_runner_action_unittest.cc
+++ b/postinstall_runner_action_unittest.cc
@@ -2,6 +2,8 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
+#include "update_engine/postinstall_runner_action.h"
+
#include <sys/stat.h>
#include <sys/types.h>
#include <unistd.h>
@@ -9,11 +11,12 @@
#include <string>
#include <vector>
+#include <base/file_util.h>
#include <base/string_util.h>
#include <base/stringprintf.h>
#include <gtest/gtest.h>
-#include "update_engine/postinstall_runner_action.h"
+#include "update_engine/constants.h"
#include "update_engine/test_utils.h"
#include "update_engine/utils.h"
@@ -32,7 +35,9 @@
class PostinstallRunnerActionTest : public ::testing::Test {
public:
- void DoTest(bool do_losetup, int err_code);
+ // DoTest with various combinations of do_losetup, err_code and
+ // powerwash_required.
+ void DoTest(bool do_losetup, int err_code, bool powerwash_required);
};
class PostinstActionProcessorDelegate : public ActionProcessorDelegate {
@@ -61,27 +66,37 @@
TEST_F(PostinstallRunnerActionTest, RunAsRootSimpleTest) {
ASSERT_EQ(0, getuid());
- DoTest(true, 0);
+ DoTest(true, 0, false);
+}
+
+TEST_F(PostinstallRunnerActionTest, RunAsRootPowerwashRequiredTest) {
+ ASSERT_EQ(0, getuid());
+ DoTest(true, 0, true);
}
TEST_F(PostinstallRunnerActionTest, RunAsRootCantMountTest) {
ASSERT_EQ(0, getuid());
- DoTest(false, 0);
+ DoTest(false, 0, true);
}
TEST_F(PostinstallRunnerActionTest, RunAsRootErrScriptTest) {
ASSERT_EQ(0, getuid());
- DoTest(true, 1);
+ DoTest(true, 1, false);
}
TEST_F(PostinstallRunnerActionTest, RunAsRootFirmwareBErrScriptTest) {
ASSERT_EQ(0, getuid());
- DoTest(true, 3);
+ DoTest(true, 3, false);
}
-void PostinstallRunnerActionTest::DoTest(bool do_losetup, int err_code) {
+void PostinstallRunnerActionTest::DoTest(
+ bool do_losetup,
+ int err_code,
+ bool powerwash_required) {
ASSERT_EQ(0, getuid()) << "Run me as root. Ideally don't run other tests "
<< "as root, tho.";
+ // True if the post-install action is expected to succeed.
+ bool should_succeed = do_losetup && !err_code;
const string mountpoint(string(utils::kStatefulPartition) +
"/au_destination");
@@ -136,6 +151,7 @@
ObjectFeederAction<InstallPlan> feeder_action;
InstallPlan install_plan;
install_plan.install_path = dev;
+ install_plan.powerwash_required = powerwash_required;
feeder_action.set_obj(install_plan);
PostinstallRunnerAction runner_action;
BondActions(&feeder_action, &runner_action);
@@ -155,18 +171,27 @@
ASSERT_FALSE(processor.IsRunning());
EXPECT_TRUE(delegate.code_set_);
- EXPECT_EQ(do_losetup && !err_code, delegate.code_ == kActionCodeSuccess);
- EXPECT_EQ(do_losetup && !err_code,
- !collector_action.object().install_path.empty());
- if (do_losetup && !err_code) {
+ EXPECT_EQ(should_succeed, delegate.code_ == kActionCodeSuccess);
+ EXPECT_EQ(should_succeed, !collector_action.object().install_path.empty());
+ if (should_succeed)
EXPECT_TRUE(install_plan == collector_action.object());
+
+ const FilePath kPowerwashMarkerPath(kPowerwashMarkerFile);
+ string actual_cmd;
+ if (should_succeed && powerwash_required) {
+ EXPECT_TRUE(file_util::ReadFileToString(kPowerwashMarkerPath, &actual_cmd));
+ EXPECT_EQ(kPowerwashCommand, actual_cmd);
+ } else {
+ EXPECT_FALSE(
+ file_util::ReadFileToString(kPowerwashMarkerPath, &actual_cmd));
}
+
if (err_code == 2)
EXPECT_EQ(kActionCodePostinstallBootedFromFirmwareB, delegate.code_);
struct stat stbuf;
int rc = lstat((string(cwd) + "/postinst_called").c_str(), &stbuf);
- if (do_losetup && !err_code)
+ if (should_succeed)
ASSERT_EQ(0, rc);
else
ASSERT_LT(rc, 0);
@@ -176,6 +201,7 @@
}
ASSERT_EQ(0, System(string("rm -f ") + cwd + "/postinst_called"));
ASSERT_EQ(0, System(string("rm -f ") + cwd + "/image.dat"));
+ utils::DeletePowerwashMarkerFile();
}
// Death tests don't seem to be working on Hardy
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
diff --git a/update_attempter.h b/update_attempter.h
index 23b45c0..bcd730f 100644
--- a/update_attempter.h
+++ b/update_attempter.h
@@ -157,6 +157,12 @@
// parameters used in the current update attempt.
uint32_t GetErrorCodeFlags();
+ // Returns true if we should cancel the current download attempt based on the
+ // current state of the system, in which case |cancel_reason| indicates the
+ // reason for the cancellation. False otherwise, in which case
+ // |cancel_reason| is untouched.
+ bool ShouldCancel(ActionExitCode* cancel_reason);
+
private:
// Update server URL for automated lab test.
static const char* const kTestUpdateUrl;
@@ -238,6 +244,12 @@
// update has been applied.
void PingOmaha();
+ // Reloads the device policy from libchromeos. Note: This method doesn't
+ // cause a real-time policy fetch from the policy server. It just reloads the
+ // latest value that libchromeos has cached. libchromeos fetches the policies
+ // from the server asynchronously at its own frequency.
+ void RefreshDevicePolicy();
+
// Helper method of Update() to calculate the update-related parameters
// from various sources and set the appropriate state. Please refer to
// Update() method for the meaning of the parametes.
diff --git a/utils.cc b/utils.cc
index 9337cc7..756ef8a 100644
--- a/utils.cc
+++ b/utils.cc
@@ -31,6 +31,7 @@
#include <google/protobuf/stubs/common.h>
#include <rootdev/rootdev.h>
+#include "update_engine/constants.h"
#include "update_engine/file_writer.h"
#include "update_engine/omaha_request_params.h"
#include "update_engine/subprocess.h"
@@ -52,7 +53,6 @@
// one second.
const int kUnmountMaxNumOfRetries = 5;
const int kUnmountRetryIntervalInMicroseconds = 200 * 1000; // 200 ms
-
} // namespace
namespace utils {
@@ -873,6 +873,8 @@
return "kActionCodeOmahaUpdateDeferredForBackoff";
case kActionCodePostinstallPowerwashError:
return "kActionCodePostinstallPowerwashError";
+ case kActionCodeUpdateCanceledByChannelChange:
+ return "kActionCodeUpdateCanceledByChannelChange";
case kActionCodeUmaReportedMax:
return "kActionCodeUmaReportedMax";
case kActionCodeOmahaRequestHTTPResponseBase:
@@ -894,6 +896,34 @@
return "Unknown error: " + base::UintToString(static_cast<unsigned>(code));
}
+bool CreatePowerwashMarkerFile() {
+ bool result = utils::WriteFile(kPowerwashMarkerFile,
+ kPowerwashCommand,
+ strlen(kPowerwashCommand));
+ if (result)
+ LOG(INFO) << "Created " << kPowerwashMarkerFile
+ << " to powerwash on next reboot";
+ else
+ PLOG(ERROR) << "Error in creating powerwash marker file: "
+ << kPowerwashMarkerFile;
+
+ return result;
+}
+
+bool DeletePowerwashMarkerFile() {
+ const FilePath kPowerwashMarkerPath(kPowerwashMarkerFile);
+ bool result = file_util::Delete(kPowerwashMarkerPath, false);
+
+ if (result)
+ LOG(INFO) << "Successfully deleted the powerwash marker file : "
+ << kPowerwashMarkerFile;
+ else
+ PLOG(ERROR) << "Could not delete the powerwash marker file : "
+ << kPowerwashMarkerFile;
+
+ return result;
+}
+
} // namespace utils
} // namespace chromeos_update_engine
diff --git a/utils.h b/utils.h
index badaa5f..a5a5a1e 100644
--- a/utils.h
+++ b/utils.h
@@ -300,6 +300,14 @@
// error codes or the bit flags) for logging purposes.
std::string CodeToString(ActionExitCode code);
+// Creates the powerwash marker file with the appropriate commands in it.
+// Returns true if successfully created. False otherwise.
+bool CreatePowerwashMarkerFile();
+
+// Deletes the marker file used to trigger Powerwash using clobber-state.
+// Returns true if successfully deleted. False otherwise.
+bool DeletePowerwashMarkerFile();
+
} // namespace utils