update_engine: Block force update + install when not idle
Currently |CheckForUpdate()| and |CheckForInstall()| sets the values of
|is_install_| to false and true respectively. The change of the value
|is_install_| at the initial entry to these functions will cause the
checks and actions taken within |ProcessingDone()| to behave as it's not
intended to. This unwanted behavior occurs because |ProcessingDone()|
depends on |UpdateAttempter|'s member variable |is_install_|. Until
|ProcessingDone()| is called, the |is_install_| value needs to remain
the same.
BUG=chromium:982929
TEST=FEATURES="test" emerge-$BOARD update_engine
Change-Id: Iddb55f1b46e66b9bc94b63e10e9f393f3cb89e1c
diff --git a/client_library/include/update_engine/update_status.h b/client_library/include/update_engine/update_status.h
index 4b86df3..edf90b4 100644
--- a/client_library/include/update_engine/update_status.h
+++ b/client_library/include/update_engine/update_status.h
@@ -23,10 +23,12 @@
namespace update_engine {
-// ATTENTION: When adding a new enum value here, always append at the end and
-// make sure to make proper adjustments in UpdateAttempter:ActionCompleted(). If
-// any enum memeber is deprecated, the assigned value of other members should
-// not change. See b/62842358.
+// ATTENTION:
+// When adding a new enum value:
+// - always append at the end with proper adjustments in |ActionCompleted()|.
+// - always update |kNonIdleUpdateStatues| in update_attempter_unittest.cc.
+// When deprecating an old enum value:
+// - other enum values should not change their old values. See b/62842358.
enum class UpdateStatus {
IDLE = 0,
CHECKING_FOR_UPDATE = 1,
diff --git a/update_attempter.cc b/update_attempter.cc
index cf588e5..9573c43 100644
--- a/update_attempter.cc
+++ b/update_attempter.cc
@@ -830,19 +830,17 @@
bool UpdateAttempter::CheckForUpdate(const string& app_version,
const string& omaha_url,
UpdateAttemptFlags flags) {
- dlc_module_ids_.clear();
- is_install_ = false;
- bool interactive = !(flags & UpdateAttemptFlags::kFlagNonInteractive);
-
- if (interactive && status_ != UpdateStatus::IDLE) {
- // An update check is either in-progress, or an update has completed and the
- // system is in UPDATED_NEED_REBOOT. Either way, don't do an interactive
- // update at this time.
- LOG(INFO) << "Refusing to do an interactive update with an update already "
- "in progress";
+ if (status_ != UpdateStatus::IDLE) {
+ LOG(INFO) << "Refusing to do an update as there is an "
+ << (is_install_ ? "install" : "update")
+ << " already in progress.";
return false;
}
+ bool interactive = !(flags & UpdateAttemptFlags::kFlagNonInteractive);
+ dlc_module_ids_.clear();
+ is_install_ = false;
+
LOG(INFO) << "Forced update check requested.";
forced_app_version_.clear();
forced_omaha_url_.clear();
@@ -888,6 +886,13 @@
bool UpdateAttempter::CheckForInstall(const vector<string>& dlc_module_ids,
const string& omaha_url) {
+ if (status_ != UpdateStatus::IDLE) {
+ LOG(INFO) << "Refusing to do an install as there is an "
+ << (is_install_ ? "install" : "update")
+ << " already in progress.";
+ return false;
+ }
+
dlc_module_ids_ = dlc_module_ids;
is_install_ = true;
forced_omaha_url_.clear();
diff --git a/update_attempter.h b/update_attempter.h
index b0654d8..3b580ad 100644
--- a/update_attempter.h
+++ b/update_attempter.h
@@ -250,6 +250,7 @@
FRIEND_TEST(UpdateAttempterTest, BootTimeInUpdateMarkerFile);
FRIEND_TEST(UpdateAttempterTest, BroadcastCompleteDownloadTest);
FRIEND_TEST(UpdateAttempterTest, ChangeToDownloadingOnReceivedBytesTest);
+ FRIEND_TEST(UpdateAttempterTest, CheckForInstallNotIdleFails);
FRIEND_TEST(UpdateAttempterTest, CheckForUpdateAUDlcTest);
FRIEND_TEST(UpdateAttempterTest, CreatePendingErrorEventTest);
FRIEND_TEST(UpdateAttempterTest, CreatePendingErrorEventResumedTest);
diff --git a/update_attempter_unittest.cc b/update_attempter_unittest.cc
index b7d0997..b69527d 100644
--- a/update_attempter_unittest.cc
+++ b/update_attempter_unittest.cc
@@ -88,6 +88,19 @@
namespace {
+const UpdateStatus kNonIdleUpdateStatuses[] = {
+ UpdateStatus::CHECKING_FOR_UPDATE,
+ UpdateStatus::UPDATE_AVAILABLE,
+ UpdateStatus::DOWNLOADING,
+ UpdateStatus::VERIFYING,
+ UpdateStatus::FINALIZING,
+ UpdateStatus::UPDATED_NEED_REBOOT,
+ UpdateStatus::REPORTING_ERROR_EVENT,
+ UpdateStatus::ATTEMPTING_ROLLBACK,
+ UpdateStatus::DISABLED,
+ UpdateStatus::NEED_PERMISSION_TO_UPDATE,
+};
+
struct CheckForUpdateTestParams {
// Setups + Inputs:
UpdateStatus status = UpdateStatus::IDLE;
@@ -1389,50 +1402,29 @@
EXPECT_FALSE(attempter_.IsAnyUpdateSourceAllowed());
}
-TEST_F(UpdateAttempterTest, CheckForUpdateInteractiveNotIdleFails) {
- // GIVEN an update is in progress.
- cfu_params_.status = UpdateStatus::CHECKING_FOR_UPDATE;
- // GIVEN a interactive update.
+// TODO(kimjae): Follow testing pattern with params for |CheckForInstall()|.
+// When adding, remove older tests related to |CheckForInstall()|.
+TEST_F(UpdateAttempterTest, CheckForInstallNotIdleFails) {
+ for (const auto status : kNonIdleUpdateStatuses) {
+ // GIVEN a non-idle status.
+ attempter_.status_ = status;
- // THEN |ScheduleUpdates()| should not be called.
- cfu_params_.should_schedule_updates_be_called = false;
- // THEN result should indicate failure.
- cfu_params_.expected_result = false;
-
- TestCheckForUpdate();
+ EXPECT_FALSE(attempter_.CheckForInstall({}, ""));
+ }
}
-// TODO(b/137217982): Currently, since the logic is to flow through, the app
-// version and omaha url are cleared.
-TEST_F(UpdateAttempterTest,
- CheckForUpdateNonInteractiveNotIdleOfficialBuildSucceeds) {
- // GIVEN an update is in progress.
- cfu_params_.status = UpdateStatus::CHECKING_FOR_UPDATE;
- // GIVEN a non interactive update.
- cfu_params_.flags = UpdateAttemptFlags::kFlagNonInteractive;
+TEST_F(UpdateAttempterTest, CheckForUpdateNotIdleFails) {
+ for (const auto status : kNonIdleUpdateStatuses) {
+ // GIVEN a non-idle status.
+ cfu_params_.status = status;
- // THEN we except forced app version + forced omaha url to be cleared.
+ // THEN |ScheduleUpdates()| should not be called.
+ cfu_params_.should_schedule_updates_be_called = false;
+ // THEN result should indicate failure.
+ cfu_params_.expected_result = false;
- TestCheckForUpdate();
-}
-
-// TODO(b/137217982): Currently, since the logic is to flow through, the app
-// version and omaha url are set based on inputs.
-TEST_F(UpdateAttempterTest,
- CheckForUpdateNonInteractiveNotIdleUnofficialBuildSucceeds) {
- // GIVEN an update is in progress.
- cfu_params_.status = UpdateStatus::CHECKING_FOR_UPDATE;
- // GIVEN a non interactive update.
- cfu_params_.flags = UpdateAttemptFlags::kFlagNonInteractive;
- // GIVEN a non offical build with dev features enabled.
- cfu_params_.is_official_build = false;
- cfu_params_.are_dev_features_enabled = true;
-
- // THEN the forced app version + forced omaha url changes based on input.
- cfu_params_.expected_forced_app_version = cfu_params_.app_version;
- cfu_params_.expected_forced_omaha_url = cfu_params_.omaha_url;
-
- TestCheckForUpdate();
+ TestCheckForUpdate();
+ }
}
TEST_F(UpdateAttempterTest, CheckForUpdateOfficalBuildClearsSource) {
@@ -1444,7 +1436,7 @@
}
TEST_F(UpdateAttempterTest, CheckForUpdateUnofficialBuildChangesSource) {
- // GIVEN a non offical build with dev features enabled.
+ // GIVEN a nonofficial build with dev features enabled.
cfu_params_.is_official_build = false;
cfu_params_.are_dev_features_enabled = true;
@@ -1469,7 +1461,7 @@
TEST_F(UpdateAttempterTest, CheckForUpdateUnofficialBuildScheduledAUTest) {
// GIVEN a scheduled autest omaha url.
cfu_params_.omaha_url = "autest-scheduled";
- // GIVEN a non offical build with dev features enabled.
+ // GIVEN a nonofficial build with dev features enabled.
cfu_params_.is_official_build = false;
cfu_params_.are_dev_features_enabled = true;
@@ -1495,7 +1487,7 @@
TEST_F(UpdateAttempterTest, CheckForUpdateUnofficialBuildAUTest) {
// GIVEN a autest omha url.
cfu_params_.omaha_url = "autest";
- // GIVEN a non offical build with dev features enabled.
+ // GIVEN a nonofficial build with dev features enabled.
cfu_params_.is_official_build = false;
cfu_params_.are_dev_features_enabled = true;
@@ -1511,7 +1503,7 @@
CheckForUpdateNonInteractiveOfficialBuildScheduledAUTest) {
// GIVEN a scheduled autest omaha url.
cfu_params_.omaha_url = "autest-scheduled";
- // GIVEN a non interactive update.
+ // GIVEN a noninteractive update.
cfu_params_.flags = UpdateAttemptFlags::kFlagNonInteractive;
// THEN forced app version is cleared.
@@ -1525,9 +1517,9 @@
CheckForUpdateNonInteractiveUnofficialBuildScheduledAUTest) {
// GIVEN a scheduled autest omaha url.
cfu_params_.omaha_url = "autest-scheduled";
- // GIVEN a non interactive update.
+ // GIVEN a noninteractive update.
cfu_params_.flags = UpdateAttemptFlags::kFlagNonInteractive;
- // GIVEN a non offical build with dev features enabled.
+ // GIVEN a nonofficial build with dev features enabled.
cfu_params_.is_official_build = false;
cfu_params_.are_dev_features_enabled = true;
@@ -1542,7 +1534,7 @@
TEST_F(UpdateAttempterTest, CheckForUpdateNonInteractiveOfficialBuildAUTest) {
// GIVEN a autest omaha url.
cfu_params_.omaha_url = "autest";
- // GIVEN a non interactive update.
+ // GIVEN a noninteractive update.
cfu_params_.flags = UpdateAttemptFlags::kFlagNonInteractive;
// THEN forced app version is cleared.
@@ -1555,9 +1547,9 @@
TEST_F(UpdateAttempterTest, CheckForUpdateNonInteractiveUnofficialBuildAUTest) {
// GIVEN a autest omaha url.
cfu_params_.omaha_url = "autest";
- // GIVEN a non interactive update.
+ // GIVEN a noninteractive update.
cfu_params_.flags = UpdateAttemptFlags::kFlagNonInteractive;
- // GIVEN a non offical build with dev features enabled.
+ // GIVEN a nonofficial build with dev features enabled.
cfu_params_.is_official_build = false;
cfu_params_.are_dev_features_enabled = true;
@@ -1582,7 +1574,7 @@
}
TEST_F(UpdateAttempterTest, CheckForUpdateMissingForcedCallback2) {
- // GIVEN a non offical build with dev features enabled.
+ // GIVEN a nonofficial build with dev features enabled.
cfu_params_.is_official_build = false;
cfu_params_.are_dev_features_enabled = true;
// GIVEN forced callback is not set.