Fix rollback crash while previous rollback is in progress.
The CHECK in Rollback is wrong. We should not be crashing the update_engine
just because we have a previous rollback in progress. This CL changes the
CHECK to a if/else and cleans up the Rollback() logic to be easier to follow
and removes a redundant check for partitions (since CanRollback already
covers this problem).
This CL also cleans up a couple rollback-related unittests.
BUG=chromium:356975
TEST=unittests + on device
Change-Id: Iee8de65eabcddd1dbe6c6413e33a15bf75302260
Reviewed-on: https://chromium-review.googlesource.com/192909
Tested-by: Chris Sosa <sosa@chromium.org>
Reviewed-by: Alex Vakulenko <avakulenko@chromium.org>
Commit-Queue: Chris Sosa <sosa@chromium.org>
diff --git a/dbus_service.cc b/dbus_service.cc
index 03bc602..230e6d5 100644
--- a/dbus_service.cc
+++ b/dbus_service.cc
@@ -182,7 +182,7 @@
GError **error) {
LOG(INFO) << "Attempting rollback to non-active partitions.";
- if (!self->system_state_->update_attempter()->Rollback(powerwash, NULL)) {
+ if (!self->system_state_->update_attempter()->Rollback(powerwash)) {
// TODO(dgarrett): Give a more specific error code/reason.
log_and_set_response_error(error,
UPDATE_ENGINE_SERVICE_ERROR_FAILED,
@@ -198,7 +198,7 @@
GError **error)
{
bool can_rollback = self->system_state_->update_attempter()->CanRollback();
- LOG(INFO) << "Checking for a rollback partition. Result: " << can_rollback;
+ LOG(INFO) << "Checking to see if we can rollback . Result: " << can_rollback;
*out_can_rollback = can_rollback;
return TRUE;
}
diff --git a/fake_hardware.h b/fake_hardware.h
index 161f51a..5aa6a3d 100644
--- a/fake_hardware.h
+++ b/fake_hardware.h
@@ -17,7 +17,7 @@
FakeHardware()
: kernel_device_("/dev/sdz4"),
boot_device_("/dev/sdz5"),
- bootable_devices_({"/dev/sdz4", "/dev/sdz5"}),
+ kernel_devices_({"/dev/sdz2", "/dev/sdz4"}),
is_official_build_(true),
is_normal_boot_mode_(true),
hardware_class_("Fake HWID BLAH-1234"),
@@ -32,7 +32,7 @@
virtual std::string BootDevice() const override { return boot_device_; }
virtual std::vector<std::string> GetKernelDevices() const override {
- return bootable_devices_;
+ return kernel_devices_;
}
virtual bool IsKernelBootable(const std::string& kernel_device,
@@ -91,7 +91,7 @@
private:
std::string kernel_device_;
std::string boot_device_;
- std::vector<std::string> bootable_devices_;
+ std::vector<std::string> kernel_devices_;
std::map<std::string, bool> is_bootable_;
bool is_official_build_;
bool is_normal_boot_mode_;
diff --git a/update_attempter.cc b/update_attempter.cc
index 237e43c..f3f6d4e 100644
--- a/update_attempter.cc
+++ b/update_attempter.cc
@@ -709,13 +709,26 @@
}
}
-bool UpdateAttempter::Rollback(bool powerwash, string *install_path) {
- CHECK(!processor_->IsRunning());
- processor_->set_delegate(this);
+bool UpdateAttempter::Rollback(bool powerwash) {
+ if (!CanRollback()) {
+ return false;
+ }
- // TODO(sosa): crbug.com/252539 -- refactor channel into system_state and
- // check for != stable-channel here.
- RefreshDevicePolicy();
+ // Extra check for enterprise-enrolled devices since they don't support
+ // powerwash.
+ if (powerwash) {
+ // Enterprise-enrolled devices have an empty owner in their device policy.
+ string owner;
+ RefreshDevicePolicy();
+ const policy::DevicePolicy* device_policy = system_state_->device_policy();
+ if (device_policy && (!device_policy->GetOwner(&owner) || owner.empty())) {
+ LOG(ERROR) << "Enterprise device detected. "
+ << "Cannot perform a powerwash for enterprise devices.";
+ return false;
+ }
+ }
+
+ processor_->set_delegate(this);
// Initialize the default request params.
if (!omaha_request_params_->Init("", "", true)) {
@@ -725,45 +738,14 @@
LOG(INFO) << "Setting rollback options.";
InstallPlan install_plan;
- if (install_path == NULL) {
- TEST_AND_RETURN_FALSE(utils::GetInstallDev(
- system_state_->hardware()->BootDevice(),
- &install_plan.install_path));
- }
- else {
- install_plan.install_path = *install_path;
- }
+
+ TEST_AND_RETURN_FALSE(utils::GetInstallDev(
+ system_state_->hardware()->BootDevice(),
+ &install_plan.install_path));
install_plan.kernel_install_path =
utils::KernelDeviceOfBootDevice(install_plan.install_path);
-
- // Check to see if the kernel we want to rollback too is bootable.
- LOG(INFO) << "Validating there is something to rollback too at: "
- << install_plan.kernel_install_path;
- bool rollback_bootable;
- if (!system_state_->hardware()->IsKernelBootable(
- install_plan.kernel_install_path,
- &rollback_bootable)) {
- LOG(ERROR) << "Unable to read GPT kernel flags.";
- return false;
- }
-
- if (!rollback_bootable) {
- LOG(ERROR) << "There is no valid OS to rollback too.";
- return false;
- }
-
install_plan.powerwash_required = powerwash;
- if (powerwash) {
- // Enterprise-enrolled devices have an empty owner in their device policy.
- string owner;
- const policy::DevicePolicy* device_policy = system_state_->device_policy();
- if (device_policy && (!device_policy->GetOwner(&owner) || owner.empty())) {
- LOG(ERROR) << "Enterprise device detected. "
- << "Cannot perform a powerwash for enterprise devices.";
- return false;
- }
- }
LOG(INFO) << "Using this install plan:";
install_plan.Dump();
@@ -795,7 +777,9 @@
}
bool UpdateAttempter::CanRollback() const {
- return !GetRollbackPartition().empty();
+ // We can only rollback if the update_engine isn't busy and we have a valid
+ // rollback partition.
+ return (status_ == UPDATE_STATUS_IDLE && !GetRollbackPartition().empty());
}
std::string UpdateAttempter::GetRollbackPartition() const {
diff --git a/update_attempter.h b/update_attempter.h
index d64771b..dfde4ff 100644
--- a/update_attempter.h
+++ b/update_attempter.h
@@ -145,12 +145,10 @@
// This is the internal entry point for going through a rollback. This will
// attempt to run the postinstall on the non-active partition and set it as
// the partition to boot from. If |powerwash| is True, perform a powerwash
- // as part of rollback. If |install_path| is set, use this value to determine
- // the partitions to roll back to (used in testing). Returns True on success.
- bool Rollback(bool powerwash, std::string* install_path);
+ // as part of rollback. Returns True on success.
+ bool Rollback(bool powerwash);
- // This is the internal entry point for checking if a valid rollback
- // partition exists.
+ // This is the internal entry point for checking if we can rollback.
bool CanRollback() const;
// This is the internal entry point for getting a rollback partition name,
diff --git a/update_attempter_unittest.cc b/update_attempter_unittest.cc
index c67185c..b623aec 100644
--- a/update_attempter_unittest.cc
+++ b/update_attempter_unittest.cc
@@ -96,7 +96,6 @@
void UpdateTestStart();
void UpdateTestVerify();
void RollbackTestStart(bool enterprise_rollback,
- bool stable_channel,
bool valid_slot);
void RollbackTestVerify();
static gboolean StaticUpdateTestStart(gpointer data);
@@ -104,7 +103,6 @@
static gboolean StaticRollbackTestStart(gpointer data);
static gboolean StaticInvalidSlotRollbackTestStart(gpointer data);
static gboolean StaticEnterpriseRollbackTestStart(gpointer data);
- static gboolean StaticStableChannelRollbackTestStart(gpointer data);
static gboolean StaticRollbackTestVerify(gpointer data);
void PingOmahaTestStart();
@@ -335,27 +333,20 @@
gboolean UpdateAttempterTest::StaticRollbackTestStart(gpointer data) {
reinterpret_cast<UpdateAttempterTest*>(data)->RollbackTestStart(
- false, false, true);
+ false, true);
return FALSE;
}
gboolean UpdateAttempterTest::StaticInvalidSlotRollbackTestStart(
gpointer data) {
reinterpret_cast<UpdateAttempterTest*>(data)->RollbackTestStart(
- false, false, false);
+ false, false);
return FALSE;
}
gboolean UpdateAttempterTest::StaticEnterpriseRollbackTestStart(gpointer data) {
reinterpret_cast<UpdateAttempterTest*>(data)->RollbackTestStart(
- true, false, true);
- return FALSE;
-}
-
-gboolean UpdateAttempterTest::StaticStableChannelRollbackTestStart(
- gpointer data) {
- reinterpret_cast<UpdateAttempterTest*>(data)->RollbackTestStart(
- false, true, true);
+ true, true);
return FALSE;
}
@@ -467,7 +458,7 @@
}
void UpdateAttempterTest::RollbackTestStart(
- bool enterprise_rollback, bool stable_channel, bool valid_slot) {
+ bool enterprise_rollback, bool valid_slot) {
// Create a device policy so that we can change settings.
policy::MockDevicePolicy* device_policy = new policy::MockDevicePolicy();
attempter_.policy_provider_.reset(new policy::PolicyProvider(device_policy));
@@ -477,13 +468,13 @@
Return(device_policy));
if (!valid_slot) {
- string rollback_kernel = "/dev/sda2";
+ // References bootable kernels in fake_hardware.h
+ string rollback_kernel = "/dev/sdz2";
LOG(INFO) << "Test Mark Unbootable: " << rollback_kernel;
mock_system_state_.get_fake_hardware()->MarkKernelUnbootable(
rollback_kernel);
}
- string install_path = "/dev/sda3";
bool is_rollback_allowed = false;
// We only allow rollback on devices that are not enterprise enrolled and
@@ -492,12 +483,6 @@
is_rollback_allowed = true;
}
- // Set up the policy for the test given our args.
- if (stable_channel) {
- attempter_.omaha_request_params_->set_current_channel(
- string("stable-channel"));
- }
-
if (enterprise_rollback) {
// We return an empty owner as this is an enterprise.
EXPECT_CALL(*device_policy, GetOwner(_)).WillRepeatedly(
@@ -519,10 +504,10 @@
}
EXPECT_CALL(*processor_, StartProcessing()).Times(1);
- EXPECT_TRUE(attempter_.Rollback(true, &install_path));
+ EXPECT_TRUE(attempter_.Rollback(true));
g_idle_add(&StaticRollbackTestVerify, this);
} else {
- EXPECT_FALSE(attempter_.Rollback(true, &install_path));
+ EXPECT_FALSE(attempter_.Rollback(true));
g_main_loop_quit(loop_);
}
}
@@ -538,8 +523,10 @@
InstallPlanAction* install_plan_action =
dynamic_cast<InstallPlanAction*>(attempter_.actions_[0].get());
InstallPlan* install_plan = install_plan_action->install_plan();
- EXPECT_EQ(install_plan->install_path, string("/dev/sda3"));
- EXPECT_EQ(install_plan->kernel_install_path, string("/dev/sda2"));
+ // Matches fake_hardware.h -> rollback should move from kernel/boot device
+ // pair to other pair.
+ EXPECT_EQ(install_plan->install_path, string("/dev/sdz3"));
+ EXPECT_EQ(install_plan->kernel_install_path, string("/dev/sdz2"));
EXPECT_EQ(install_plan->powerwash_required, true);
g_main_loop_quit(loop_);
}
@@ -568,14 +555,6 @@
loop_ = NULL;
}
-TEST_F(UpdateAttempterTest, StableChannelRollbackTest) {
- loop_ = g_main_loop_new(g_main_context_default(), FALSE);
- g_idle_add(&StaticStableChannelRollbackTestStart, this);
- g_main_loop_run(loop_);
- g_main_loop_unref(loop_);
- loop_ = NULL;
-}
-
TEST_F(UpdateAttempterTest, EnterpriseRollbackTest) {
loop_ = g_main_loop_new(g_main_context_default(), FALSE);
g_idle_add(&StaticEnterpriseRollbackTestStart, this);