update_engine: Move DLC metadata ownership to update_engine
Add dbus messages so dlcservice can let update_engine know when a DLC is
installed or uninstalled.
BUG=chromium:912666
TEST=unittests, install and uninstall DLCs on DUT.
Cq-Depend: chromium:2112994,chromium:2113254
Change-Id: I35374504afcdaf96b099e343cabe072fc18f1022
Reviewed-on: https://chromium-review.googlesource.com/c/aosp/platform/system/update_engine/+/2113134
Reviewed-by: Amin Hassani <ahassani@chromium.org>
Reviewed-by: Jae Hoon Kim <kimjae@chromium.org>
Tested-by: Andrew Lassalle <andrewlassalle@chromium.org>
Commit-Queue: Andrew Lassalle <andrewlassalle@chromium.org>
diff --git a/UpdateEngine.conf b/UpdateEngine.conf
index ab776e3..f9a66dc 100644
--- a/UpdateEngine.conf
+++ b/UpdateEngine.conf
@@ -98,5 +98,8 @@
<allow send_destination="org.chromium.UpdateEngine"
send_interface="org.chromium.UpdateEngineInterface"
send_member="AttemptInstall"/>
+ <allow send_destination="org.chromium.UpdateEngine"
+ send_interface="org.chromium.UpdateEngineInterface"
+ send_member="SetDlcActiveValue"/>
</policy>
</busconfig>
diff --git a/client_library/client_dbus.cc b/client_library/client_dbus.cc
index 18b155b..f16b759 100644
--- a/client_library/client_dbus.cc
+++ b/client_library/client_dbus.cc
@@ -98,6 +98,11 @@
nullptr /* brillo::ErrorPtr* */);
}
+bool DBusUpdateEngineClient::SetDlcActiveValue(bool is_active,
+ const std::string& dlc_id) {
+ return proxy_->SetDlcActiveValue(is_active, dlc_id, /*error=*/nullptr);
+}
+
bool DBusUpdateEngineClient::GetStatus(UpdateEngineStatus* out_status) const {
StatusResult status;
if (!proxy_->GetStatusAdvanced(&status, nullptr)) {
diff --git a/client_library/client_dbus.h b/client_library/client_dbus.h
index 6d7784a..a032d21 100644
--- a/client_library/client_dbus.h
+++ b/client_library/client_dbus.h
@@ -46,6 +46,8 @@
bool AttemptInstall(const std::string& omaha_url,
const std::vector<std::string>& dlc_module_ids) override;
+ bool SetDlcActiveValue(bool is_active, const std::string& dlc_id) override;
+
bool GetStatus(UpdateEngineStatus* out_status) const override;
bool SetCohortHint(const std::string& cohort_hint) override;
diff --git a/client_library/include/update_engine/client.h b/client_library/include/update_engine/client.h
index d13359b..9bda0b9 100644
--- a/client_library/include/update_engine/client.h
+++ b/client_library/include/update_engine/client.h
@@ -63,6 +63,11 @@
// Same as above but return the entire struct instead.
virtual bool GetStatus(UpdateEngineStatus* out_status) const = 0;
+ // Sets the DLC as active or inactive. When set to active, the ping metadata
+ // for the DLC is updated accordingly. When set to inactive, the metadata
+ // for the DLC is deleted.
+ virtual bool SetDlcActiveValue(bool is_active, const std::string& dlc_id) = 0;
+
// Getter and setter for the cohort hint.
virtual bool SetCohortHint(const std::string& cohort_hint) = 0;
virtual bool GetCohortHint(std::string* cohort_hint) const = 0;
diff --git a/common/constants.cc b/common/constants.cc
index 58cf1b3..793ce97 100644
--- a/common/constants.cc
+++ b/common/constants.cc
@@ -18,7 +18,7 @@
namespace chromeos_update_engine {
-// Keep this in sync with the one in dlcservice.
+// TODO(andrewlassalle): Move this to the prefs directory.
const char kDlcMetadataRootpath[] = "/var/lib/dlc/";
const char kPowerwashSafePrefsSubDirectory[] = "update_engine/prefs";
diff --git a/common_service.cc b/common_service.cc
index a99d10c..b94e734 100644
--- a/common_service.cc
+++ b/common_service.cc
@@ -143,6 +143,17 @@
return true;
}
+bool UpdateEngineService::SetDlcActiveValue(brillo::ErrorPtr* error,
+ bool is_active,
+ const string& dlc_id) {
+ if (!system_state_->update_attempter()->SetDlcActiveValue(is_active,
+ dlc_id)) {
+ LogAndSetError(error, FROM_HERE, "SetDlcActiveValue failed.");
+ return false;
+ }
+ return true;
+}
+
bool UpdateEngineService::GetStatus(ErrorPtr* error,
UpdateEngineStatus* out_status) {
if (!system_state_->update_attempter()->GetStatus(out_status)) {
diff --git a/common_service.h b/common_service.h
index 3349244..a74c46b 100644
--- a/common_service.h
+++ b/common_service.h
@@ -70,6 +70,13 @@
// update. This is used for development only.
bool ResetStatus(brillo::ErrorPtr* error);
+ // Sets the DLC as active or inactive. When set to active, the ping metadata
+ // for the DLC is updated accordingly. When set to inactive, the metadata
+ // for the DLC is deleted.
+ bool SetDlcActiveValue(brillo::ErrorPtr* error,
+ bool is_active,
+ const std::string& dlc_id);
+
// Returns the current status of the Update Engine. If an update is in
// progress, the number of operations, size to download and overall progress
// is reported.
diff --git a/common_service_unittest.cc b/common_service_unittest.cc
index 00c4357..3dc8a22 100644
--- a/common_service_unittest.cc
+++ b/common_service_unittest.cc
@@ -100,6 +100,20 @@
EXPECT_FALSE(common_service_.AttemptInstall(&error_, "", {}));
}
+TEST_F(UpdateEngineServiceTest, SetDlcActiveValue) {
+ EXPECT_CALL(*mock_update_attempter_, SetDlcActiveValue(_, _))
+ .WillOnce(Return(true));
+
+ EXPECT_TRUE(common_service_.SetDlcActiveValue(&error_, true, "dlc0"));
+}
+
+TEST_F(UpdateEngineServiceTest, SetDlcActiveValueReturnsFalse) {
+ EXPECT_CALL(*mock_update_attempter_, SetDlcActiveValue(_, _))
+ .WillOnce(Return(false));
+
+ EXPECT_FALSE(common_service_.SetDlcActiveValue(&error_, true, "dlc0"));
+}
+
// SetChannel is allowed when there's no device policy (the device is not
// enterprise enrolled).
TEST_F(UpdateEngineServiceTest, SetChannelWithNoPolicy) {
diff --git a/dbus_bindings/org.chromium.UpdateEngineInterface.dbus-xml b/dbus_bindings/org.chromium.UpdateEngineInterface.dbus-xml
index afa34d7..51457e5 100644
--- a/dbus_bindings/org.chromium.UpdateEngineInterface.dbus-xml
+++ b/dbus_bindings/org.chromium.UpdateEngineInterface.dbus-xml
@@ -51,6 +51,18 @@
</method>
<method name="ResetStatus">
</method>
+ <method name="SetDlcActiveValue">
+ <arg type="b" name="is_active" direction="in">
+ <tp:docstring>
+ If the DLC is being set to active or inactive.
+ </tp:docstring>
+ </arg>
+ <arg type="s" name="dlc_id" direction="in">
+ <tp:docstring>
+ The ID of the DLC module that will be set to active/inactive.
+ </tp:docstring>
+ </arg>
+ </method>
<method name="GetStatusAdvanced">
<arg type="ay" name="status" direction="out">
<tp:docstring>
diff --git a/dbus_service.cc b/dbus_service.cc
index b1cc298..cd71488 100644
--- a/dbus_service.cc
+++ b/dbus_service.cc
@@ -110,6 +110,12 @@
return common_->ResetStatus(error);
}
+bool DBusUpdateEngineService::SetDlcActiveValue(brillo::ErrorPtr* error,
+ bool is_active,
+ const string& dlc_id) {
+ return common_->SetDlcActiveValue(error, is_active, dlc_id);
+}
+
bool DBusUpdateEngineService::GetStatusAdvanced(ErrorPtr* error,
StatusResult* out_status) {
UpdateEngineStatus status;
diff --git a/dbus_service.h b/dbus_service.h
index 28ba268..86f5b93 100644
--- a/dbus_service.h
+++ b/dbus_service.h
@@ -64,6 +64,13 @@
// update. This is used for development only.
bool ResetStatus(brillo::ErrorPtr* error) override;
+ // Sets the DLC as active or inactive. When set to active, the ping metadata
+ // for the DLC is updated accordingly. When set to inactive, the metadata
+ // for the DLC is deleted.
+ bool SetDlcActiveValue(brillo::ErrorPtr* error,
+ bool is_active,
+ const std::string& dlc_id) override;
+
// Similar to Above, but returns a protobuffer instead. In the future it will
// have more features and is easily extendable.
bool GetStatusAdvanced(brillo::ErrorPtr* error,
diff --git a/mock_update_attempter.h b/mock_update_attempter.h
index c39fb62..9d966d7 100644
--- a/mock_update_attempter.h
+++ b/mock_update_attempter.h
@@ -58,6 +58,8 @@
bool(const std::vector<std::string>& dlc_module_ids,
const std::string& omaha_url));
+ MOCK_METHOD2(SetDlcActiveValue, bool(bool, const std::string&));
+
MOCK_METHOD0(RefreshDevicePolicy, void(void));
MOCK_CONST_METHOD0(consecutive_failed_update_checks, unsigned int(void));
diff --git a/omaha_request_action.cc b/omaha_request_action.cc
index 58e7f47..8890c7c 100644
--- a/omaha_request_action.cc
+++ b/omaha_request_action.cc
@@ -414,7 +414,6 @@
return num_days;
}
-// static
void OmahaRequestAction::StorePingReply(
const OmahaParserData& parser_data) const {
for (const auto& app : parser_data.apps) {
@@ -430,24 +429,15 @@
base::FilePath metadata_path =
base::FilePath(params_->dlc_prefs_root()).Append(dlc_params.name);
- if (!base::PathExists(metadata_path)) {
- LOG(ERROR) << "Metadata path (" << metadata_path.value() << ") "
- << "doesn't exist.";
- // Skip this DLC if the metadata directory is missing.
- continue;
- }
Prefs prefs;
- if (!prefs.Init(metadata_path)) {
+ if (!base::CreateDirectory(metadata_path) || !prefs.Init(metadata_path)) {
LOG(ERROR) << "Failed to initialize the preferences path:"
<< metadata_path.value() << ".";
continue;
}
// Reset the active metadata value to |kPingInactiveValue|.
- // Only write into this file if the file exists, otherwise the file will be
- // created with different owner/permissions.
- if (prefs.Exists(kPrefsPingActive) &&
- !prefs.SetInt64(kPrefsPingActive, kPingInactiveValue))
+ if (!prefs.SetInt64(kPrefsPingActive, kPingInactiveValue))
LOG(ERROR) << "Failed to set the value of ping metadata '"
<< kPrefsPingActive << "'.";
diff --git a/update_attempter.cc b/update_attempter.cc
index 8e32091..f5885c9 100644
--- a/update_attempter.cc
+++ b/update_attempter.cc
@@ -22,11 +22,13 @@
#include <map>
#include <memory>
#include <string>
+#include <unordered_set>
#include <utility>
#include <vector>
#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>
@@ -656,6 +658,47 @@
}
}
+bool UpdateAttempter::SetDlcActiveValue(bool is_active, const string& dlc_id) {
+ if (dlc_id.empty()) {
+ LOG(ERROR) << "Empty DLC ID passed.";
+ return false;
+ }
+ 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);
+ 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)) {
+ 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 true;
+}
+
int64_t UpdateAttempter::GetPingMetadata(
const PrefsInterface& prefs, const std::string& metadata_name) const {
// The first time a ping is sent, the metadata files containing the values
@@ -681,6 +724,23 @@
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_module_ids_.begin(),
+ dlc_module_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;
for (auto dlc_id : dlc_module_ids_) {
OmahaRequestParams::AppParams dlc_params{
@@ -691,11 +751,9 @@
// 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 =
- base::FilePath(omaha_request_params_->dlc_prefs_root())
- .Append(dlc_id);
+ base::FilePath metadata_path = metadata_root_path.Append(dlc_id);
Prefs prefs;
- if (!prefs.Init(metadata_path)) {
+ if (!base::CreateDirectory(metadata_path) || !prefs.Init(metadata_path)) {
LOG(ERROR) << "Failed to initialize the preferences path:"
<< metadata_path.value() << ".";
} else {
diff --git a/update_attempter.h b/update_attempter.h
index 91e072a..c364de3 100644
--- a/update_attempter.h
+++ b/update_attempter.h
@@ -162,6 +162,9 @@
// UPDATED_NEED_REBOOT. Returns true on success, false otherwise.
bool RebootIfNeeded();
+ // Sets the DLC as active or inactive. See common_service.h
+ virtual bool SetDlcActiveValue(bool is_active, const std::string& dlc_id);
+
// DownloadActionDelegate methods:
void BytesReceived(uint64_t bytes_progressed,
uint64_t bytes_received,
@@ -253,6 +256,7 @@
FRIEND_TEST(UpdateAttempterTest, CalculateDlcParamsNoPrefFilesTest);
FRIEND_TEST(UpdateAttempterTest, CalculateDlcParamsNonParseableValuesTest);
FRIEND_TEST(UpdateAttempterTest, CalculateDlcParamsValidValuesTest);
+ FRIEND_TEST(UpdateAttempterTest, CalculateDlcParamsRemoveStaleMetadata);
FRIEND_TEST(UpdateAttempterTest, ChangeToDownloadingOnReceivedBytesTest);
FRIEND_TEST(UpdateAttempterTest, CheckForInstallNotIdleFails);
FRIEND_TEST(UpdateAttempterTest, CheckForUpdateAUDlcTest);
diff --git a/update_attempter_unittest.cc b/update_attempter_unittest.cc
index d468f56..d65a556 100644
--- a/update_attempter_unittest.cc
+++ b/update_attempter_unittest.cc
@@ -2401,4 +2401,60 @@
EXPECT_EQ(78, dlc_app_params.ping_date_last_active);
EXPECT_EQ(99, dlc_app_params.ping_date_last_rollcall);
}
+
+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_, GetInstalled(_))
+ .WillOnce(
+ DoAll(SetArgPointee<0>(std::vector<string>({dlc_id})), Return(true)));
+
+ attempter_.is_install_ = false;
+ attempter_.CalculateDlcParams();
+
+ EXPECT_TRUE(base::PathExists(metadata_path_dlc0));
+ EXPECT_FALSE(base::PathExists(metadata_path_dlc_stale));
+}
+
+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);
+ 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));
+ 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));
+ attempter_.SetDlcActiveValue(false, dlc_id);
+ EXPECT_FALSE(base::PathExists(metadata_path_dlc0));
+}
+
} // namespace chromeos_update_engine