diff options
author | Andrew <andrewlassalle@chromium.org> | 2020-04-07 15:43:07 -0700 |
---|---|---|
committer | Commit Bot <commit-bot@chromium.org> | 2020-05-05 21:05:43 +0000 |
commit | 065d78d6963ca13a38ad305bf751b09ec929cf51 (patch) | |
tree | 1f526b5f8e363bf08e9abfe43f48b508006a0815 | |
parent | ebea33916754d5522ce6489a910b990b119b7174 (diff) |
update_engine: Change DLC metadata path
Change the location of the DLC metadata from /var/lib/dlc to
/var/lib/update_engine/dlc_prefs/ to make update_engine the owner of
metadata.
BUG=chromium:912666
TEST=cros_workon_make update_engine --test
TEST=install and uninstall DLCs on DUT. Check new prefs path.
Change-Id: I75f5506eee1abc834ad89a7cf363f42e384b695b
Reviewed-on: https://chromium-review.googlesource.com/c/aosp/platform/system/update_engine/+/2140007
Tested-by: Andrew Lassalle <andrewlassalle@chromium.org>
Commit-Queue: Amin Hassani <ahassani@chromium.org>
Reviewed-by: Jae Hoon Kim <kimjae@chromium.org>
Reviewed-by: Amin Hassani <ahassani@chromium.org>
-rw-r--r-- | common/constants.cc | 3 | ||||
-rw-r--r-- | common/constants.h | 5 | ||||
-rw-r--r-- | common/prefs.cc | 33 | ||||
-rw-r--r-- | common/prefs_interface.h | 5 | ||||
-rw-r--r-- | common/prefs_unittest.cc | 69 | ||||
-rw-r--r-- | omaha_request_action.cc | 36 | ||||
-rw-r--r-- | omaha_request_action_unittest.cc | 137 | ||||
-rw-r--r-- | omaha_request_params.cc | 1 | ||||
-rw-r--r-- | omaha_request_params.h | 6 | ||||
-rw-r--r-- | update_attempter.cc | 120 | ||||
-rw-r--r-- | update_attempter.h | 10 | ||||
-rw-r--r-- | update_attempter_unittest.cc | 155 |
12 files changed, 321 insertions, 259 deletions
diff --git a/common/constants.cc b/common/constants.cc index 793ce97c..25aa9a8a 100644 --- a/common/constants.cc +++ b/common/constants.cc @@ -18,8 +18,7 @@ namespace chromeos_update_engine { -// TODO(andrewlassalle): Move this to the prefs directory. -const char kDlcMetadataRootpath[] = "/var/lib/dlc/"; +const char kDlcPrefsSubDir[] = "dlc"; const char kPowerwashSafePrefsSubDirectory[] = "update_engine/prefs"; diff --git a/common/constants.h b/common/constants.h index 44b20b0f..67519bdd 100644 --- a/common/constants.h +++ b/common/constants.h @@ -19,9 +19,8 @@ namespace chromeos_update_engine { -// The root path of all DLC modules metadata. -// Keep this in sync with the one in dlcservice. -extern const char kDlcMetadataRootpath[]; +// The root path of all DLC metadata. +extern const char kDlcPrefsSubDir[]; // Directory for AU prefs that are preserved across powerwash. extern const char kPowerwashSafePrefsSubDirectory[]; diff --git a/common/prefs.cc b/common/prefs.cc index 6d86a504..6a330378 100644 --- a/common/prefs.cc +++ b/common/prefs.cc @@ -18,9 +18,11 @@ #include <algorithm> +#include <base/files/file_enumerator.h> #include <base/files/file_util.h> #include <base/logging.h> #include <base/strings/string_number_conversions.h> +#include <base/strings/string_split.h> #include <base/strings/string_util.h> #include "update_engine/common/utils.h" @@ -29,6 +31,8 @@ using std::string; namespace chromeos_update_engine { +const char kKeySeparator = '/'; + bool PrefsBase::GetString(const string& key, string* value) const { return storage_->GetKey(key, value); } @@ -104,6 +108,13 @@ void PrefsBase::RemoveObserver(const string& key, ObserverInterface* observer) { observers_for_key.erase(observer_it); } +string PrefsInterface::CreateSubKey(const string& name_space, + const string& sub_pref, + const string& key) { + return base::JoinString({name_space, sub_pref, key}, + string(1, kKeySeparator)); +} + // Prefs bool Prefs::Init(const base::FilePath& prefs_dir) { @@ -112,6 +123,24 @@ bool Prefs::Init(const base::FilePath& prefs_dir) { bool Prefs::FileStorage::Init(const base::FilePath& prefs_dir) { prefs_dir_ = prefs_dir; + // Delete empty directories. Ignore errors when deleting empty directories. + base::FileEnumerator namespace_enum( + prefs_dir_, false /* recursive */, base::FileEnumerator::DIRECTORIES); + for (base::FilePath namespace_path = namespace_enum.Next(); + !namespace_path.empty(); + namespace_path = namespace_enum.Next()) { + base::FileEnumerator sub_pref_enum(namespace_path, + false /* recursive */, + base::FileEnumerator::DIRECTORIES); + for (base::FilePath sub_pref_path = sub_pref_enum.Next(); + !sub_pref_path.empty(); + sub_pref_path = sub_pref_enum.Next()) { + if (base::IsDirectoryEmpty(sub_pref_path)) + base::DeleteFile(sub_pref_path, false); + } + if (base::IsDirectoryEmpty(namespace_path)) + base::DeleteFile(namespace_path, false); + } return true; } @@ -146,7 +175,7 @@ bool Prefs::FileStorage::KeyExists(const string& key) const { bool Prefs::FileStorage::DeleteKey(const string& key) { base::FilePath filename; TEST_AND_RETURN_FALSE(GetFileNameForKey(key, &filename)); - TEST_AND_RETURN_FALSE(base::DeleteFile(filename, false)); + TEST_AND_RETURN_FALSE(base::DeleteFile(filename, true)); return true; } @@ -157,7 +186,7 @@ bool Prefs::FileStorage::GetFileNameForKey(const string& key, for (size_t i = 0; i < key.size(); ++i) { char c = key.at(i); TEST_AND_RETURN_FALSE(base::IsAsciiAlpha(c) || base::IsAsciiDigit(c) || - c == '_' || c == '-'); + c == '_' || c == '-' || c == kKeySeparator); } *filename = prefs_dir_.Append(key); return true; diff --git a/common/prefs_interface.h b/common/prefs_interface.h index 03ae3ecd..b5596974 100644 --- a/common/prefs_interface.h +++ b/common/prefs_interface.h @@ -79,6 +79,11 @@ class PrefsInterface { // this key. Calling with non-existent keys does nothing. virtual bool Delete(const std::string& key) = 0; + // Creates a key which is part of a sub preference. + static std::string CreateSubKey(const std::string& name_space, + const std::string& sub_pref, + const std::string& key); + // Add an observer to watch whenever the given |key| is modified. The // OnPrefSet() and OnPrefDelete() methods will be called whenever any of the // Set*() methods or the Delete() method are called on the given key, diff --git a/common/prefs_unittest.cc b/common/prefs_unittest.cc index 3f293199..f226949c 100644 --- a/common/prefs_unittest.cc +++ b/common/prefs_unittest.cc @@ -31,6 +31,7 @@ using std::string; using testing::_; +using testing::ElementsAre; using testing::Eq; namespace { @@ -59,6 +60,21 @@ class PrefsTest : public ::testing::Test { Prefs prefs_; }; +TEST(Prefs, Init) { + Prefs prefs; + const string name_space = "ns"; + const string sub_pref = "sp"; + + base::ScopedTempDir temp_dir; + ASSERT_TRUE(temp_dir.CreateUniqueTempDir()); + base::FilePath namespace_path = temp_dir.GetPath().Append(name_space); + + EXPECT_TRUE(base::CreateDirectory(namespace_path.Append(sub_pref))); + EXPECT_TRUE(base::PathExists(namespace_path.Append(sub_pref))); + ASSERT_TRUE(prefs.Init(temp_dir.GetPath())); + EXPECT_FALSE(base::PathExists(namespace_path)); +} + TEST_F(PrefsTest, GetFileNameForKey) { const char kAllvalidCharsKey[] = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz_-"; @@ -77,6 +93,18 @@ TEST_F(PrefsTest, GetFileNameForKeyEmpty) { EXPECT_FALSE(prefs_.file_storage_.GetFileNameForKey("", &path)); } +TEST_F(PrefsTest, CreateSubKey) { + const string name_space = "ns"; + const string sub_pref1 = "sp1"; + const string sub_pref2 = "sp2"; + const string sub_key = "sk"; + + EXPECT_EQ(PrefsInterface::CreateSubKey(name_space, sub_pref1, sub_key), + "ns/sp1/sk"); + EXPECT_EQ(PrefsInterface::CreateSubKey(name_space, sub_pref2, sub_key), + "ns/sp2/sk"); +} + TEST_F(PrefsTest, GetString) { const string test_data = "test data"; ASSERT_TRUE(SetValue(kKey, test_data)); @@ -279,6 +307,29 @@ TEST_F(PrefsTest, DeleteWorks) { EXPECT_FALSE(prefs_.Exists(kKey)); } +TEST_F(PrefsTest, SetDeleteSubKey) { + const string name_space = "ns"; + const string sub_pref = "sp"; + const string sub_key1 = "sk1"; + const string sub_key2 = "sk2"; + auto key1 = prefs_.CreateSubKey(name_space, sub_pref, sub_key1); + auto key2 = prefs_.CreateSubKey(name_space, sub_pref, sub_key2); + base::FilePath sub_pref_path = prefs_dir_.Append(name_space).Append(sub_pref); + + ASSERT_TRUE(prefs_.SetInt64(key1, 0)); + ASSERT_TRUE(prefs_.SetInt64(key2, 0)); + EXPECT_TRUE(base::PathExists(sub_pref_path.Append(sub_key1))); + EXPECT_TRUE(base::PathExists(sub_pref_path.Append(sub_key2))); + + ASSERT_TRUE(prefs_.Delete(key1)); + EXPECT_FALSE(base::PathExists(sub_pref_path.Append(sub_key1))); + EXPECT_TRUE(base::PathExists(sub_pref_path.Append(sub_key2))); + ASSERT_TRUE(prefs_.Delete(key2)); + EXPECT_FALSE(base::PathExists(sub_pref_path.Append(sub_key2))); + prefs_.Init(prefs_dir_); + EXPECT_FALSE(base::PathExists(prefs_dir_.Append(name_space))); +} + class MockPrefsObserver : public PrefsInterface::ObserverInterface { public: MOCK_METHOD1(OnPrefSet, void(const string&)); @@ -299,6 +350,19 @@ TEST_F(PrefsTest, ObserversCalled) { prefs_.Delete(kKey); testing::Mock::VerifyAndClearExpectations(&mock_obserser); + auto key1 = prefs_.CreateSubKey("ns", "sp1", "key1"); + prefs_.AddObserver(key1, &mock_obserser); + + EXPECT_CALL(mock_obserser, OnPrefSet(key1)); + EXPECT_CALL(mock_obserser, OnPrefDeleted(_)).Times(0); + prefs_.SetString(key1, "value"); + testing::Mock::VerifyAndClearExpectations(&mock_obserser); + + EXPECT_CALL(mock_obserser, OnPrefSet(_)).Times(0); + EXPECT_CALL(mock_obserser, OnPrefDeleted(Eq(key1))); + prefs_.Delete(key1); + testing::Mock::VerifyAndClearExpectations(&mock_obserser); + prefs_.RemoveObserver(kKey, &mock_obserser); } @@ -359,6 +423,11 @@ TEST_F(MemoryPrefsTest, BasicTest) { EXPECT_TRUE(prefs_.Delete(kKey)); EXPECT_FALSE(prefs_.Exists(kKey)); EXPECT_TRUE(prefs_.Delete(kKey)); + + auto key = prefs_.CreateSubKey("ns", "sp", "sk"); + ASSERT_TRUE(prefs_.SetInt64(key, 0)); + EXPECT_TRUE(prefs_.Exists(key)); + EXPECT_TRUE(prefs_.Delete(kKey)); } } // namespace chromeos_update_engine diff --git a/omaha_request_action.cc b/omaha_request_action.cc index 81abb3e9..c9b8aa04 100644 --- a/omaha_request_action.cc +++ b/omaha_request_action.cc @@ -413,37 +413,33 @@ void OmahaRequestAction::StorePingReply( continue; const OmahaRequestParams::AppParams& dlc_params = it->second; - + const string& dlc_id = dlc_params.name; // Skip if the ping for this DLC was not sent. if (!dlc_params.send_ping) continue; - base::FilePath metadata_path = - base::FilePath(params_->dlc_prefs_root()).Append(dlc_params.name); - - Prefs prefs; - if (!base::CreateDirectory(metadata_path) || !prefs.Init(metadata_path)) { - LOG(ERROR) << "Failed to initialize the preferences path:" - << metadata_path.value() << "."; - continue; - } + PrefsInterface* prefs = system_state_->prefs(); // Reset the active metadata value to |kPingInactiveValue|. - if (!prefs.SetInt64(kPrefsPingActive, kPingInactiveValue)) - LOG(ERROR) << "Failed to set the value of ping metadata '" - << kPrefsPingActive << "'."; - - if (!prefs.SetString(kPrefsPingLastRollcall, - parser_data.daystart_elapsed_days)) + auto active_key = + prefs->CreateSubKey(kDlcPrefsSubDir, dlc_id, kPrefsPingActive); + if (!prefs->SetInt64(active_key, kPingInactiveValue)) + LOG(ERROR) << "Failed to set the value of ping metadata '" << active_key + << "'."; + + auto last_rollcall_key = + prefs->CreateSubKey(kDlcPrefsSubDir, dlc_id, kPrefsPingLastRollcall); + if (!prefs->SetString(last_rollcall_key, parser_data.daystart_elapsed_days)) LOG(ERROR) << "Failed to set the value of ping metadata '" - << kPrefsPingLastRollcall << "'."; + << last_rollcall_key << "'."; if (dlc_params.ping_active) { // Write the value of elapsed_days into |kPrefsPingLastActive| only if // the previous ping was an active one. - if (!prefs.SetString(kPrefsPingLastActive, - parser_data.daystart_elapsed_days)) + auto last_active_key = + prefs->CreateSubKey(kDlcPrefsSubDir, dlc_id, kPrefsPingLastActive); + if (!prefs->SetString(last_active_key, parser_data.daystart_elapsed_days)) LOG(ERROR) << "Failed to set the value of ping metadata '" - << kPrefsPingLastActive << "'."; + << last_active_key << "'."; } } } diff --git a/omaha_request_action_unittest.cc b/omaha_request_action_unittest.cc index 2528f7b7..e1f5ef90 100644 --- a/omaha_request_action_unittest.cc +++ b/omaha_request_action_unittest.cc @@ -429,12 +429,6 @@ class OmahaRequestActionTest : public ::testing::Test { bool expected_allow_p2p_for_sharing, const string& expected_p2p_url); - // Helper function used to test the Ping request. - // Create the test directory and setup the Omaha response. - void SetUpStorePingReply(const string& dlc_id, - base::FilePath* metadata_path_dlc, - base::ScopedTempDir* tempdir); - FakeSystemState fake_system_state_; FakeUpdateResponse fake_update_response_; // Used by all tests. @@ -453,6 +447,36 @@ class OmahaRequestActionTest : public ::testing::Test { string post_str; }; +class OmahaRequestActionDlcPingTest : public OmahaRequestActionTest { + protected: + void SetUp() override { + OmahaRequestActionTest::SetUp(); + dlc_id_ = "dlc0"; + active_key_ = PrefsInterface::CreateSubKey( + kDlcPrefsSubDir, dlc_id_, kPrefsPingActive); + last_active_key_ = PrefsInterface::CreateSubKey( + kDlcPrefsSubDir, dlc_id_, kPrefsPingLastActive); + last_rollcall_key_ = PrefsInterface::CreateSubKey( + kDlcPrefsSubDir, dlc_id_, kPrefsPingLastRollcall); + + tuc_params_.http_response = + "<?xml version=\"1.0\" encoding=\"UTF-8\"?><response " + "protocol=\"3.0\"><daystart elapsed_days=\"4763\" " + "elapsed_seconds=\"36540\"/><app appid=\"test-app-id\" status=\"ok\">\"" + "<updatecheck status=\"noupdate\"/></app><app " + "appid=\"test-app-id_dlc0\" " + "status=\"ok\"><ping status=\"ok\"/><updatecheck status=\"noupdate\"/>" + "</app></response>"; + tuc_params_.expected_check_result = + metrics::CheckResult::kNoUpdateAvailable; + tuc_params_.expected_check_reaction = metrics::CheckReaction::kUnset; + } + + std::string dlc_id_; + std::string active_key_; + std::string last_active_key_; + std::string last_rollcall_key_; +}; bool OmahaRequestActionTest::TestUpdateCheck() { brillo::FakeMessageLoop loop(nullptr); loop.SetAsCurrent(); @@ -2904,106 +2928,67 @@ TEST_F(OmahaRequestActionTest, PersistEolBadDateTest) { EXPECT_EQ(kEolDateInvalid, StringToEolDate(eol_date)); } -void OmahaRequestActionTest::SetUpStorePingReply( - const string& dlc_id, - base::FilePath* metadata_path_dlc, - base::ScopedTempDir* tempdir) { - // Create a uniquely named test directory. - ASSERT_TRUE(tempdir->CreateUniqueTempDir()); - request_params_.set_root(tempdir->GetPath().value()); - *metadata_path_dlc = - base::FilePath(request_params_.dlc_prefs_root()).Append(dlc_id); - ASSERT_TRUE(base::CreateDirectory(*metadata_path_dlc)); - - tuc_params_.http_response = - "<?xml version=\"1.0\" encoding=\"UTF-8\"?><response " - "protocol=\"3.0\"><daystart elapsed_days=\"4763\" " - "elapsed_seconds=\"36540\"/><app appid=\"test-app-id\" status=\"ok\">\"" - "<updatecheck status=\"noupdate\"/></app><app appid=\"test-app-id_dlc0\" " - "status=\"ok\"><ping status=\"ok\"/><updatecheck status=\"noupdate\"/>" - "</app></response>"; - tuc_params_.expected_check_result = metrics::CheckResult::kNoUpdateAvailable; - tuc_params_.expected_check_reaction = metrics::CheckReaction::kUnset; -} - -TEST_F(OmahaRequestActionTest, StorePingReplyNoPing) { - string dlc_id = "dlc0"; - base::FilePath metadata_path_dlc0; - base::ScopedTempDir tempdir; - SetUpStorePingReply(dlc_id, &metadata_path_dlc0, &tempdir); - int64_t temp_int; - Prefs prefs; - ASSERT_TRUE(prefs.Init(metadata_path_dlc0)); - - OmahaRequestParams::AppParams app_param = {.name = dlc_id}; +TEST_F(OmahaRequestActionDlcPingTest, StorePingReplyNoPing) { + OmahaRequestParams::AppParams app_param = {.name = dlc_id_}; request_params_.set_dlc_apps_params( - {{request_params_.GetDlcAppId(dlc_id), app_param}}); + {{request_params_.GetDlcAppId(dlc_id_), app_param}}); ASSERT_TRUE(TestUpdateCheck()); + + int64_t temp_int; // If there was no ping, the metadata files shouldn't exist yet. - EXPECT_FALSE(prefs.GetInt64(kPrefsPingActive, &temp_int)); - EXPECT_FALSE(prefs.GetInt64(kPrefsPingLastActive, &temp_int)); - EXPECT_FALSE(prefs.GetInt64(kPrefsPingLastRollcall, &temp_int)); + EXPECT_FALSE(fake_prefs_.GetInt64(active_key_, &temp_int)); + EXPECT_FALSE(fake_prefs_.GetInt64(last_active_key_, &temp_int)); + EXPECT_FALSE(fake_prefs_.GetInt64(last_rollcall_key_, &temp_int)); } -TEST_F(OmahaRequestActionTest, StorePingReplyActiveTest) { - string dlc_id = "dlc0"; - base::FilePath metadata_path_dlc0; - base::ScopedTempDir tempdir; - SetUpStorePingReply(dlc_id, &metadata_path_dlc0, &tempdir); - int64_t temp_int; - Prefs prefs; - ASSERT_TRUE(prefs.Init(metadata_path_dlc0)); +TEST_F(OmahaRequestActionDlcPingTest, StorePingReplyActiveTest) { // Create Active value - prefs.SetInt64(kPrefsPingActive, 0); + fake_prefs_.SetInt64(active_key_, 0); OmahaRequestParams::AppParams app_param = { .active_counting_type = OmahaRequestParams::kDateBased, - .name = dlc_id, + .name = dlc_id_, .ping_active = 1, .send_ping = true}; request_params_.set_dlc_apps_params( - {{request_params_.GetDlcAppId(dlc_id), app_param}}); + {{request_params_.GetDlcAppId(dlc_id_), app_param}}); + int64_t temp_int; + string temp_str; ASSERT_TRUE(TestUpdateCheck()); - EXPECT_TRUE(prefs.GetInt64(kPrefsPingActive, &temp_int)); + EXPECT_TRUE(fake_prefs_.GetInt64(active_key_, &temp_int)); EXPECT_EQ(temp_int, kPingInactiveValue); - EXPECT_TRUE(prefs.GetInt64(kPrefsPingLastActive, &temp_int)); - EXPECT_EQ(temp_int, 4763); - EXPECT_TRUE(prefs.GetInt64(kPrefsPingLastRollcall, &temp_int)); - EXPECT_EQ(temp_int, 4763); + EXPECT_TRUE(fake_prefs_.GetString(last_active_key_, &temp_str)); + EXPECT_EQ(temp_str, "4763"); + EXPECT_TRUE(fake_prefs_.GetString(last_rollcall_key_, &temp_str)); + EXPECT_EQ(temp_str, "4763"); } -TEST_F(OmahaRequestActionTest, StorePingReplyInactiveTest) { - string dlc_id = "dlc0"; - base::FilePath metadata_path_dlc0; - base::ScopedTempDir tempdir; - SetUpStorePingReply(dlc_id, &metadata_path_dlc0, &tempdir); - int64_t temp_int; - Prefs prefs; - ASSERT_TRUE(prefs.Init(metadata_path_dlc0)); +TEST_F(OmahaRequestActionDlcPingTest, StorePingReplyInactiveTest) { // Create Active value - prefs.SetInt64(kPrefsPingActive, 0); + fake_prefs_.SetInt64(active_key_, 0); OmahaRequestParams::AppParams app_param = { .active_counting_type = OmahaRequestParams::kDateBased, - .name = dlc_id, + .name = dlc_id_, .ping_active = 0, .send_ping = true}; request_params_.set_dlc_apps_params( - {{request_params_.GetDlcAppId(dlc_id), app_param}}); + {{request_params_.GetDlcAppId(dlc_id_), app_param}}); // Set the previous active value to an older value than 4763. - prefs.SetInt64(kPrefsPingLastActive, 555); + fake_prefs_.SetString(last_active_key_, "555"); + int64_t temp_int; ASSERT_TRUE(TestUpdateCheck()); - ASSERT_TRUE(prefs.Init(metadata_path_dlc0)); - EXPECT_TRUE(prefs.GetInt64(kPrefsPingActive, &temp_int)); + EXPECT_TRUE(fake_prefs_.GetInt64(active_key_, &temp_int)); EXPECT_EQ(temp_int, kPingInactiveValue); - EXPECT_TRUE(prefs.GetInt64(kPrefsPingLastActive, &temp_int)); - EXPECT_EQ(temp_int, 555); - EXPECT_TRUE(prefs.GetInt64(kPrefsPingLastRollcall, &temp_int)); - EXPECT_EQ(temp_int, 4763); + string temp_str; + EXPECT_TRUE(fake_prefs_.GetString(last_active_key_, &temp_str)); + EXPECT_EQ(temp_str, "555"); + EXPECT_TRUE(fake_prefs_.GetString(last_rollcall_key_, &temp_str)); + EXPECT_EQ(temp_str, "4763"); } } // namespace chromeos_update_engine diff --git a/omaha_request_params.cc b/omaha_request_params.cc index 52675980..d4b8d649 100644 --- a/omaha_request_params.cc +++ b/omaha_request_params.cc @@ -214,7 +214,6 @@ bool OmahaRequestParams::IsValidChannel(const string& channel, void OmahaRequestParams::set_root(const string& root) { root_ = root; test::SetImagePropertiesRootPrefix(root_.c_str()); - dlc_prefs_root_ = root + kDlcMetadataRootpath; } int OmahaRequestParams::GetChannelIndex(const string& channel) const { diff --git a/omaha_request_params.h b/omaha_request_params.h index b33d0b16..d29ce70f 100644 --- a/omaha_request_params.h +++ b/omaha_request_params.h @@ -58,7 +58,6 @@ class OmahaRequestParams { update_check_count_wait_enabled_(false), min_update_checks_needed_(kDefaultMinUpdateChecks), max_update_checks_allowed_(kDefaultMaxUpdateChecks), - dlc_prefs_root_(kDlcMetadataRootpath), is_install_(false) {} virtual ~OmahaRequestParams(); @@ -222,8 +221,6 @@ class OmahaRequestParams { return autoupdate_token_; } - inline std::string dlc_prefs_root() const { return dlc_prefs_root_; } - // Returns the App ID corresponding to the current value of the // download channel. virtual std::string GetAppId() const; @@ -410,9 +407,6 @@ class OmahaRequestParams { // When reading files, prepend root_ to the paths. Useful for testing. std::string root_; - // The metadata/prefs root path for DLCs. - std::string dlc_prefs_root_; - // A list of DLC modules to install. A mapping from DLC App ID to |AppParams|. std::map<std::string, AppParams> dlc_apps_params_; diff --git a/update_attempter.cc b/update_attempter.cc index ae7f71eb..0ead18ae 100644 --- a/update_attempter.cc +++ b/update_attempter.cc @@ -28,7 +28,6 @@ #include <base/bind.h> #include <base/compiler_specific.h> -#include <base/files/file_enumerator.h> #include <base/files/file_util.h> #include <base/logging.h> #include <base/rand_util.h> @@ -85,6 +84,7 @@ using chromeos_update_manager::EvalStatus; using chromeos_update_manager::Policy; using chromeos_update_manager::StagingCase; using chromeos_update_manager::UpdateCheckParams; +using std::map; using std::string; using std::vector; using update_engine::UpdateAttemptFlags; @@ -658,6 +658,22 @@ void UpdateAttempter::CalculateStagingParams(bool interactive) { } } +bool UpdateAttempter::ResetDlcPrefs(const string& dlc_id) { + vector<string> failures; + PrefsInterface* prefs = system_state_->prefs(); + for (auto& sub_key : + {kPrefsPingActive, kPrefsPingLastActive, kPrefsPingLastRollcall}) { + auto key = prefs->CreateSubKey(kDlcPrefsSubDir, dlc_id, sub_key); + if (!prefs->Delete(key)) + failures.emplace_back(sub_key); + } + if (failures.size() != 0) + PLOG(ERROR) << "Failed to delete prefs (" << base::JoinString(failures, ",") + << " for DLC (" << dlc_id << ")."; + + return failures.size() == 0; +} + bool UpdateAttempter::SetDlcActiveValue(bool is_active, const string& dlc_id) { if (dlc_id.empty()) { LOG(ERROR) << "Empty DLC ID passed."; @@ -665,50 +681,30 @@ bool UpdateAttempter::SetDlcActiveValue(bool is_active, const string& dlc_id) { } LOG(INFO) << "Set DLC (" << dlc_id << ") to " << (is_active ? "Active" : "Inactive"); - // TODO(andrewlassalle): Should dlc_prefs_root be in systemstate instead of - // omaha_request_params_? - base::FilePath metadata_path = - base::FilePath(omaha_request_params_->dlc_prefs_root()).Append(dlc_id); + PrefsInterface* prefs = system_state_->prefs(); if (is_active) { - base::File::Error error; - if (!base::CreateDirectoryAndGetError(metadata_path, &error)) { - PLOG(ERROR) << "Failed to create metadata directory for DLC (" << dlc_id - << "). Error:" << error; - return false; - } - - Prefs prefs; - if (!prefs.Init(metadata_path)) { - LOG(ERROR) << "Failed to initialize the preferences path:" - << metadata_path.value() << "."; - return false; - } - - if (!prefs.SetInt64(kPrefsPingActive, kPingActiveValue)) { + auto ping_active_key = + prefs->CreateSubKey(kDlcPrefsSubDir, dlc_id, kPrefsPingActive); + if (!prefs->SetInt64(ping_active_key, kPingActiveValue)) { LOG(ERROR) << "Failed to set the value of ping metadata '" << kPrefsPingActive << "'."; return false; } } else { - if (!base::DeleteFile(metadata_path, true)) { - PLOG(ERROR) << "Failed to delete metadata directory(" - << metadata_path.value() << ") for DLC (" << dlc_id << ")."; - return false; - } + return ResetDlcPrefs(dlc_id); } return true; } -int64_t UpdateAttempter::GetPingMetadata( - const PrefsInterface& prefs, const std::string& metadata_name) const { +int64_t UpdateAttempter::GetPingMetadata(const string& metadata_key) const { // The first time a ping is sent, the metadata files containing the values // sent back by the server still don't exist. A value of -1 is used to // indicate this. - if (!prefs.Exists(metadata_name)) + if (!system_state_->prefs()->Exists(metadata_key)) return kPingNeverPinged; int64_t value; - if (prefs.GetInt64(metadata_name, &value)) + if (system_state_->prefs()->GetInt64(metadata_key, &value)) return value; // Return -2 when the file exists and there is a problem reading from it, or @@ -724,49 +720,41 @@ void UpdateAttempter::CalculateDlcParams() { LOG(INFO) << "Failed to retrieve DLC module IDs from dlcservice. Check the " "state of dlcservice, will not update DLC modules."; } - base::FilePath metadata_root_path = - base::FilePath(omaha_request_params_->dlc_prefs_root()); - // Cleanup any leftover metadata for DLCs which don't exist. - base::FileEnumerator dir_enum(metadata_root_path, - false /* recursive */, - base::FileEnumerator::DIRECTORIES); - std::unordered_set<string> dlc_ids(dlc_ids_.begin(), dlc_ids_.end()); - for (base::FilePath name = dir_enum.Next(); !name.empty(); - name = dir_enum.Next()) { - string id = name.BaseName().value(); - if (dlc_ids.find(id) == dlc_ids.end()) { - LOG(INFO) << "Deleting stale metadata for DLC:" << id; - if (!base::DeleteFile(name, true)) - PLOG(WARNING) << "Failed to delete DLC prefs path:" << name.value(); - } - } - std::map<std::string, OmahaRequestParams::AppParams> dlc_apps_params; + PrefsInterface* prefs = system_state_->prefs(); + map<string, OmahaRequestParams::AppParams> dlc_apps_params; for (const auto& dlc_id : dlc_ids_) { OmahaRequestParams::AppParams dlc_params{ .active_counting_type = OmahaRequestParams::kDateBased, .name = dlc_id, .send_ping = false}; - // Only send the ping when the request is to update DLCs. When installing - // DLCs, we don't want to send the ping yet, since the DLCs might fail to - // install or might not really be active yet. - if (!is_install_) { - base::FilePath metadata_path = metadata_root_path.Append(dlc_id); - Prefs prefs; - if (!base::CreateDirectory(metadata_path) || !prefs.Init(metadata_path)) { - LOG(ERROR) << "Failed to initialize the preferences path:" - << metadata_path.value() << "."; - } else { - dlc_params.ping_active = kPingActiveValue; - if (!prefs.GetInt64(kPrefsPingActive, &dlc_params.ping_active) || - dlc_params.ping_active != kPingActiveValue) { - dlc_params.ping_active = kPingInactiveValue; - } - dlc_params.ping_date_last_active = - GetPingMetadata(prefs, kPrefsPingLastActive); - dlc_params.ping_date_last_rollcall = - GetPingMetadata(prefs, kPrefsPingLastRollcall); - dlc_params.send_ping = true; + if (is_install_) { + // In some cases, |SetDlcActiveValue| might fail to reset the DLC prefs + // when a DLC is uninstalled. To avoid having stale values from that + // scenario, we reset the metadata values on a new install request. + // Ignore failure to delete stale prefs. + ResetDlcPrefs(dlc_id); + SetDlcActiveValue(true, dlc_id); + } else { + // Only send the ping when the request is to update DLCs. When installing + // DLCs, we don't want to send the ping yet, since the DLCs might fail to + // install or might not really be active yet. + dlc_params.ping_active = kPingActiveValue; + auto ping_active_key = + prefs->CreateSubKey(kDlcPrefsSubDir, dlc_id, kPrefsPingActive); + if (!prefs->GetInt64(ping_active_key, &dlc_params.ping_active) || + dlc_params.ping_active != kPingActiveValue) { + dlc_params.ping_active = kPingInactiveValue; } + auto ping_last_active_key = + prefs->CreateSubKey(kDlcPrefsSubDir, dlc_id, kPrefsPingLastActive); + dlc_params.ping_date_last_active = GetPingMetadata(ping_last_active_key); + + auto ping_last_rollcall_key = + prefs->CreateSubKey(kDlcPrefsSubDir, dlc_id, kPrefsPingLastRollcall); + dlc_params.ping_date_last_rollcall = + GetPingMetadata(ping_last_rollcall_key); + + dlc_params.send_ping = true; } dlc_apps_params[omaha_request_params_->GetDlcAppId(dlc_id)] = dlc_params; } diff --git a/update_attempter.h b/update_attempter.h index 9e481792..e270b598 100644 --- a/update_attempter.h +++ b/update_attempter.h @@ -439,13 +439,15 @@ class UpdateAttempter : public ActionProcessorDelegate, // Resets interactivity and forced update flags. void ResetInteractivityFlags(); - // Get the integer values from the metadata directory set in |prefs| for - // |kPrefsPingLastActive| or |kPrefsPingLastRollcall|. + // Resets all the DLC prefs. + bool ResetDlcPrefs(const std::string& dlc_id); + + // Get the integer values from the DLC metadata for |kPrefsPingLastActive| + // or |kPrefsPingLastRollcall|. // The value is equal to -2 when the value cannot be read or is not numeric. // The value is equal to -1 the first time it is being sent, which is // when the metadata file doesn't exist. - int64_t GetPingMetadata(const PrefsInterface& prefs, - const std::string& metadata_name) const; + int64_t GetPingMetadata(const std::string& metadata_key) const; // Calculates the update parameters for DLCs. Sets the |dlc_ids_| // parameter on the |omaha_request_params_| object. diff --git a/update_attempter_unittest.cc b/update_attempter_unittest.cc index 56665ad3..5a6a23e9 100644 --- a/update_attempter_unittest.cc +++ b/update_attempter_unittest.cc @@ -72,6 +72,7 @@ using std::unique_ptr; using std::unordered_set; using std::vector; using testing::_; +using testing::Contains; using testing::DoAll; using testing::ElementsAre; using testing::Field; @@ -2349,16 +2350,9 @@ TEST_F(UpdateAttempterTest, FailedEolTest) { } TEST_F(UpdateAttempterTest, CalculateDlcParamsInstallTest) { - // Create a uniquely named test directory. - base::ScopedTempDir tempdir; - ASSERT_TRUE(tempdir.CreateUniqueTempDir()); - fake_system_state_.request_params()->set_root(tempdir.GetPath().value()); string dlc_id = "dlc0"; - base::FilePath metadata_path_dlc0 = - base::FilePath(fake_system_state_.request_params()->dlc_prefs_root()) - .Append(dlc_id); - - ASSERT_TRUE(base::CreateDirectory(metadata_path_dlc0)); + FakePrefs fake_prefs; + fake_system_state_.set_prefs(&fake_prefs); attempter_.is_install_ = true; attempter_.dlc_ids_ = {dlc_id}; attempter_.CalculateDlcParams(); @@ -2371,22 +2365,18 @@ TEST_F(UpdateAttempterTest, CalculateDlcParamsInstallTest) { EXPECT_EQ(false, dlc_app_params.send_ping); // When the DLC gets installed, a ping is not sent, therefore we don't store // the values sent by Omaha. - EXPECT_FALSE( - base::PathExists(metadata_path_dlc0.Append(kPrefsPingLastActive))); - EXPECT_FALSE( - base::PathExists(metadata_path_dlc0.Append(kPrefsPingLastRollcall))); + auto last_active_key = PrefsInterface::CreateSubKey( + kDlcPrefsSubDir, dlc_id, kPrefsPingLastActive); + EXPECT_FALSE(fake_system_state_.prefs()->Exists(last_active_key)); + auto last_rollcall_key = PrefsInterface::CreateSubKey( + kDlcPrefsSubDir, dlc_id, kPrefsPingLastRollcall); + EXPECT_FALSE(fake_system_state_.prefs()->Exists(last_rollcall_key)); } TEST_F(UpdateAttempterTest, CalculateDlcParamsNoPrefFilesTest) { - // Create a uniquely named test directory. - base::ScopedTempDir tempdir; - ASSERT_TRUE(tempdir.CreateUniqueTempDir()); - fake_system_state_.request_params()->set_root(tempdir.GetPath().value()); string dlc_id = "dlc0"; - base::FilePath metadata_path_dlc0 = - base::FilePath(fake_system_state_.request_params()->dlc_prefs_root()) - .Append(dlc_id); - ASSERT_TRUE(base::CreateDirectory(metadata_path_dlc0)); + FakePrefs fake_prefs; + fake_system_state_.set_prefs(&fake_prefs); EXPECT_CALL(mock_dlcservice_, GetDlcsToUpdate(_)) .WillOnce( DoAll(SetArgPointee<0>(std::vector<string>({dlc_id})), Return(true))); @@ -2407,23 +2397,23 @@ TEST_F(UpdateAttempterTest, CalculateDlcParamsNoPrefFilesTest) { } TEST_F(UpdateAttempterTest, CalculateDlcParamsNonParseableValuesTest) { - // Create a uniquely named test directory. - base::ScopedTempDir tempdir; - ASSERT_TRUE(tempdir.CreateUniqueTempDir()); - fake_system_state_.request_params()->set_root(tempdir.GetPath().value()); string dlc_id = "dlc0"; - base::FilePath metadata_path_dlc0 = - base::FilePath(fake_system_state_.request_params()->dlc_prefs_root()) - .Append(dlc_id); - ASSERT_TRUE(base::CreateDirectory(metadata_path_dlc0)); + MemoryPrefs prefs; + fake_system_state_.set_prefs(&prefs); EXPECT_CALL(mock_dlcservice_, GetDlcsToUpdate(_)) .WillOnce( DoAll(SetArgPointee<0>(std::vector<string>({dlc_id})), Return(true))); // Write non numeric values in the metadata files. - base::WriteFile(metadata_path_dlc0.Append(kPrefsPingActive), "z2yz", 4); - base::WriteFile(metadata_path_dlc0.Append(kPrefsPingLastActive), "z2yz", 4); - base::WriteFile(metadata_path_dlc0.Append(kPrefsPingLastRollcall), "z2yz", 4); + auto active_key = + PrefsInterface::CreateSubKey(kDlcPrefsSubDir, dlc_id, kPrefsPingActive); + auto last_active_key = PrefsInterface::CreateSubKey( + kDlcPrefsSubDir, dlc_id, kPrefsPingLastActive); + auto last_rollcall_key = PrefsInterface::CreateSubKey( + kDlcPrefsSubDir, dlc_id, kPrefsPingLastRollcall); + fake_system_state_.prefs()->SetString(active_key, "z2yz"); + fake_system_state_.prefs()->SetString(last_active_key, "z2yz"); + fake_system_state_.prefs()->SetString(last_rollcall_key, "z2yz"); attempter_.is_install_ = false; attempter_.CalculateDlcParams(); @@ -2440,23 +2430,24 @@ TEST_F(UpdateAttempterTest, CalculateDlcParamsNonParseableValuesTest) { } TEST_F(UpdateAttempterTest, CalculateDlcParamsValidValuesTest) { - // Create a uniquely named test directory. - base::ScopedTempDir tempdir; - ASSERT_TRUE(tempdir.CreateUniqueTempDir()); - fake_system_state_.request_params()->set_root(tempdir.GetPath().value()); string dlc_id = "dlc0"; - base::FilePath metadata_path_dlc0 = - base::FilePath(fake_system_state_.request_params()->dlc_prefs_root()) - .Append(dlc_id); - ASSERT_TRUE(base::CreateDirectory(metadata_path_dlc0)); + MemoryPrefs fake_prefs; + fake_system_state_.set_prefs(&fake_prefs); EXPECT_CALL(mock_dlcservice_, GetDlcsToUpdate(_)) .WillOnce( DoAll(SetArgPointee<0>(std::vector<string>({dlc_id})), Return(true))); // Write numeric values in the metadata files. - base::WriteFile(metadata_path_dlc0.Append(kPrefsPingActive), "1", 1); - base::WriteFile(metadata_path_dlc0.Append(kPrefsPingLastActive), "78", 2); - base::WriteFile(metadata_path_dlc0.Append(kPrefsPingLastRollcall), "99", 2); + auto active_key = + PrefsInterface::CreateSubKey(kDlcPrefsSubDir, dlc_id, kPrefsPingActive); + auto last_active_key = PrefsInterface::CreateSubKey( + kDlcPrefsSubDir, dlc_id, kPrefsPingLastActive); + auto last_rollcall_key = PrefsInterface::CreateSubKey( + kDlcPrefsSubDir, dlc_id, kPrefsPingLastRollcall); + + fake_system_state_.prefs()->SetInt64(active_key, 1); + fake_system_state_.prefs()->SetInt64(last_active_key, 78); + fake_system_state_.prefs()->SetInt64(last_rollcall_key, 99); attempter_.is_install_ = false; attempter_.CalculateDlcParams(); @@ -2473,58 +2464,64 @@ TEST_F(UpdateAttempterTest, CalculateDlcParamsValidValuesTest) { } TEST_F(UpdateAttempterTest, CalculateDlcParamsRemoveStaleMetadata) { - base::ScopedTempDir tempdir; - ASSERT_TRUE(tempdir.CreateUniqueTempDir()); - fake_system_state_.request_params()->set_root(tempdir.GetPath().value()); string dlc_id = "dlc0"; - base::FilePath metadata_path_dlc0 = - base::FilePath(fake_system_state_.request_params()->dlc_prefs_root()) - .Append(dlc_id); - ASSERT_TRUE(base::CreateDirectory(metadata_path_dlc0)); - base::FilePath metadata_path_dlc_stale = - base::FilePath(fake_system_state_.request_params()->dlc_prefs_root()) - .Append("stale"); - ASSERT_TRUE(base::CreateDirectory(metadata_path_dlc_stale)); - EXPECT_CALL(mock_dlcservice_, GetDlcsToUpdate(_)) - .WillOnce( - DoAll(SetArgPointee<0>(std::vector<string>({dlc_id})), Return(true))); + FakePrefs fake_prefs; + fake_system_state_.set_prefs(&fake_prefs); + auto active_key = + PrefsInterface::CreateSubKey(kDlcPrefsSubDir, dlc_id, kPrefsPingActive); + auto last_active_key = PrefsInterface::CreateSubKey( + kDlcPrefsSubDir, dlc_id, kPrefsPingLastActive); + auto last_rollcall_key = PrefsInterface::CreateSubKey( + kDlcPrefsSubDir, dlc_id, kPrefsPingLastRollcall); + fake_system_state_.prefs()->SetInt64(active_key, kPingInactiveValue); + fake_system_state_.prefs()->SetInt64(last_active_key, 0); + fake_system_state_.prefs()->SetInt64(last_rollcall_key, 0); + EXPECT_TRUE(fake_system_state_.prefs()->Exists(active_key)); + EXPECT_TRUE(fake_system_state_.prefs()->Exists(last_active_key)); + EXPECT_TRUE(fake_system_state_.prefs()->Exists(last_rollcall_key)); - attempter_.is_install_ = false; + attempter_.dlc_ids_ = {dlc_id}; + attempter_.is_install_ = true; attempter_.CalculateDlcParams(); - EXPECT_TRUE(base::PathExists(metadata_path_dlc0)); - EXPECT_FALSE(base::PathExists(metadata_path_dlc_stale)); + EXPECT_FALSE(fake_system_state_.prefs()->Exists(last_active_key)); + EXPECT_FALSE(fake_system_state_.prefs()->Exists(last_rollcall_key)); + // Active key is set on install. + EXPECT_TRUE(fake_system_state_.prefs()->Exists(active_key)); + int64_t temp_int; + EXPECT_TRUE(fake_system_state_.prefs()->GetInt64(active_key, &temp_int)); + EXPECT_EQ(temp_int, kPingActiveValue); } TEST_F(UpdateAttempterTest, SetDlcActiveValue) { - base::ScopedTempDir tempdir; - ASSERT_TRUE(tempdir.CreateUniqueTempDir()); - fake_system_state_.request_params()->set_root(tempdir.GetPath().value()); string dlc_id = "dlc0"; - base::FilePath metadata_path_dlc0 = - base::FilePath(fake_system_state_.request_params()->dlc_prefs_root()) - .Append(dlc_id); + FakePrefs fake_prefs; + fake_system_state_.set_prefs(&fake_prefs); attempter_.SetDlcActiveValue(true, dlc_id); - Prefs prefs; - ASSERT_TRUE(base::PathExists(metadata_path_dlc0)); - ASSERT_TRUE(prefs.Init(metadata_path_dlc0)); int64_t temp_int; - EXPECT_TRUE(prefs.GetInt64(kPrefsPingActive, &temp_int)); + auto active_key = + PrefsInterface::CreateSubKey(kDlcPrefsSubDir, dlc_id, kPrefsPingActive); + EXPECT_TRUE(fake_system_state_.prefs()->Exists(active_key)); + EXPECT_TRUE(fake_system_state_.prefs()->GetInt64(active_key, &temp_int)); EXPECT_EQ(temp_int, kPingActiveValue); } TEST_F(UpdateAttempterTest, SetDlcInactive) { - base::ScopedTempDir tempdir; - ASSERT_TRUE(tempdir.CreateUniqueTempDir()); - fake_system_state_.request_params()->set_root(tempdir.GetPath().value()); string dlc_id = "dlc0"; - base::FilePath metadata_path_dlc0 = - base::FilePath(fake_system_state_.request_params()->dlc_prefs_root()) - .Append(dlc_id); - base::CreateDirectory(metadata_path_dlc0); - EXPECT_TRUE(base::PathExists(metadata_path_dlc0)); + MemoryPrefs fake_prefs; + fake_system_state_.set_prefs(&fake_prefs); + auto sub_keys = { + kPrefsPingActive, kPrefsPingLastActive, kPrefsPingLastRollcall}; + for (auto& sub_key : sub_keys) { + auto key = PrefsInterface::CreateSubKey(kDlcPrefsSubDir, dlc_id, sub_key); + fake_system_state_.prefs()->SetInt64(key, 1); + EXPECT_TRUE(fake_system_state_.prefs()->Exists(key)); + } attempter_.SetDlcActiveValue(false, dlc_id); - EXPECT_FALSE(base::PathExists(metadata_path_dlc0)); + for (auto& sub_key : sub_keys) { + auto key = PrefsInterface::CreateSubKey(kDlcPrefsSubDir, dlc_id, sub_key); + EXPECT_FALSE(fake_system_state_.prefs()->Exists(key)); + } } TEST_F(UpdateAttempterTest, GetSuccessfulDlcIds) { |