update_engine: Move P2P enabled pref handling into P2PManager.
This makes the P2PManager own the manipulation of the P2P enabled
setting, which positions us better toward moving the decision about
whether P2P is enabled from P2PManager and into the UpdateManager. Also
increases encapsulation and makes it a bit more resilient.
BUG=chromium:425233
TEST=Unit tests.
Change-Id: Ic91872df84920ae80c5ef973aee99cc46cac264c
Reviewed-on: https://chromium-review.googlesource.com/225681
Tested-by: Gilad Arnold <garnold@chromium.org>
Reviewed-by: Gaurav Shah <gauravsh@chromium.org>
Commit-Queue: Gilad Arnold <garnold@chromium.org>
diff --git a/dbus_service.cc b/dbus_service.cc
index e7b877d..ccd175f 100644
--- a/dbus_service.cc
+++ b/dbus_service.cc
@@ -9,6 +9,7 @@
#include <base/logging.h>
#include <base/strings/stringprintf.h>
+#include <chromeos/strings/string_utils.h>
#include <policy/device_policy.h>
#include "update_engine/clock_interface.h"
@@ -21,6 +22,8 @@
#include "update_engine/update_attempter.h"
#include "update_engine/utils.h"
+using base::StringPrintf;
+using chromeos::string_utils::ToString;
using std::set;
using std::string;
using chromeos_update_engine::AttemptUpdateFlags;
@@ -352,25 +355,18 @@
UpdateEngineService* self,
gboolean enabled,
GError **error) {
- chromeos_update_engine::PrefsInterface* prefs = self->system_state_->prefs();
chromeos_update_engine::P2PManager* p2p_manager =
self->system_state_->p2p_manager();
- bool p2p_was_enabled = p2p_manager && p2p_manager->IsP2PEnabled();
-
- if (!prefs->SetBoolean(chromeos_update_engine::kPrefsP2PEnabled, enabled)) {
+ if (!(p2p_manager &&
+ p2p_manager->SetP2PEnabledPref(enabled))) {
log_and_set_response_error(
error, UPDATE_ENGINE_SERVICE_ERROR_FAILED,
- string("Error setting the update over cellular to ") +
- (enabled ? "true." : "false."));
+ StringPrintf("Error setting the update via p2p permission to %s.",
+ ToString(enabled).c_str()));
return FALSE;
}
- // If P2P is being effectively disabled (IsP2PEnabled() reports the change)
- // then we need to shutdown the service.
- if (p2p_was_enabled && !p2p_manager->IsP2PEnabled())
- p2p_manager->EnsureP2PNotRunning();
-
return TRUE;
}
diff --git a/fake_p2p_manager.h b/fake_p2p_manager.h
index a00b91f..9ec0812 100644
--- a/fake_p2p_manager.h
+++ b/fake_p2p_manager.h
@@ -19,7 +19,8 @@
ensure_p2p_running_result_(false),
ensure_p2p_not_running_result_(false),
perform_housekeeping_result_(false),
- count_shared_files_result_(0) {}
+ count_shared_files_result_(0),
+ set_p2p_enabled_pref_result_(true) {}
virtual ~FakeP2PManager() {}
@@ -79,6 +80,10 @@
return count_shared_files_result_;
}
+ bool SetP2PEnabledPref(bool /* enabled */) override {
+ return set_p2p_enabled_pref_result_;
+ }
+
// Methods for controlling what the fake returns and how it acts.
void SetP2PEnabled(bool is_p2p_enabled) {
is_p2p_enabled_ = is_p2p_enabled;
@@ -100,6 +105,10 @@
count_shared_files_result_ = count_shared_files_result;
}
+ void SetSetP2PEnabledPrefResult(bool set_p2p_enabled_pref_result) {
+ set_p2p_enabled_pref_result_ = set_p2p_enabled_pref_result;
+ }
+
void SetLookupUrlForFileResult(const std::string& url) {
lookup_url_for_file_result_ = url;
}
@@ -110,6 +119,7 @@
bool ensure_p2p_not_running_result_;
bool perform_housekeeping_result_;
int count_shared_files_result_;
+ bool set_p2p_enabled_pref_result_;
std::string lookup_url_for_file_result_;
DISALLOW_COPY_AND_ASSIGN(FakeP2PManager);
diff --git a/mock_p2p_manager.h b/mock_p2p_manager.h
index a0ed920..64c9c82 100644
--- a/mock_p2p_manager.h
+++ b/mock_p2p_manager.h
@@ -58,6 +58,9 @@
ON_CALL(*this, CountSharedFiles())
.WillByDefault(testing::Invoke(&fake_,
&FakeP2PManager::CountSharedFiles));
+ ON_CALL(*this, SetP2PEnabledPref(testing::_))
+ .WillByDefault(testing::Invoke(&fake_,
+ &FakeP2PManager::SetP2PEnabledPref));
}
virtual ~MockP2PManager() {}
@@ -79,6 +82,7 @@
MOCK_METHOD2(FileGetVisible, bool(const std::string&, bool*));
MOCK_METHOD1(FileMakeVisible, bool(const std::string&));
MOCK_METHOD0(CountSharedFiles, int());
+ MOCK_METHOD1(SetP2PEnabledPref, bool(bool));
// Returns a reference to the underlying FakeP2PManager.
FakeP2PManager& fake() {
diff --git a/p2p_manager.cc b/p2p_manager.cc
index b1383e2..9f0db53 100644
--- a/p2p_manager.cc
+++ b/p2p_manager.cc
@@ -118,6 +118,7 @@
bool *out_result);
virtual bool FileMakeVisible(const string& file_id);
virtual int CountSharedFiles();
+ bool SetP2PEnabledPref(bool enabled) override;
private:
// Enumeration for specifying visibility.
@@ -161,6 +162,9 @@
// The string ".tmp".
static const char kTmpExtension[];
+ // Whether P2P service may be running; initially, we assume it may be.
+ bool may_be_running_ = true;
+
DISALLOW_COPY_AND_ASSIGN(P2PManagerImpl);
};
@@ -236,6 +240,8 @@
GError *error = nullptr;
gint exit_status = 0;
+ may_be_running_ = true; // Unless successful, we must be conservative.
+
vector<string> args = configuration_->GetInitctlArgs(should_be_running);
unique_ptr<gchar*, GLibStrvFreeDeleter> argv(
utils::StringVectorToGStrv(args));
@@ -260,29 +266,21 @@
return false;
}
- // If initctl(8) exits normally with exit status 0 ("success"), it
- // meant that it did what we requested.
- if (WEXITSTATUS(exit_status) == 0) {
- return true;
- }
-
- // Otherwise, screenscape stderr from initctl(8). Ugh, yes, this is
- // ugly but since the program lacks verbs/actions such as
- //
- // ensure-started (or start-or-return-success-if-already-started)
- // ensure-stopped (or stop-or-return-success-if-not-running)
- //
- // this is what we have to do.
- //
+ // If initctl(8) does not exit normally (exit status other than zero), ensure
+ // that the error message is not benign by scanning stderr; this is a
+ // necessity because initctl does not offer actions such as "start if not
+ // running" or "stop if running".
// TODO(zeuthen,chromium:277051): Avoid doing this.
- const gchar *expected_error_message = should_be_running ?
- "initctl: Job is already running: p2p\n" :
- "initctl: Unknown instance \n";
- if (g_strcmp0(standard_error, expected_error_message) == 0) {
- return true;
+ if (WEXITSTATUS(exit_status) != 0) {
+ const gchar *expected_error_message = should_be_running ?
+ "initctl: Job is already running: p2p\n" :
+ "initctl: Unknown instance \n";
+ if (g_strcmp0(standard_error, expected_error_message) != 0)
+ return false;
}
- return false;
+ may_be_running_ = should_be_running; // Successful after all.
+ return true;
}
bool P2PManagerImpl::EnsureP2PRunning() {
@@ -763,6 +761,17 @@
return num_files;
}
+bool P2PManagerImpl::SetP2PEnabledPref(bool enabled) {
+ if (!prefs_->SetBoolean(chromeos_update_engine::kPrefsP2PEnabled, enabled))
+ return false;
+
+ // If P2P should not be running, make sure it isn't.
+ if (may_be_running_ && !IsP2PEnabled())
+ EnsureP2PNotRunning();
+
+ return true;
+}
+
P2PManager* P2PManager::Construct(Configuration *configuration,
PrefsInterface *prefs,
const string& file_extension,
diff --git a/p2p_manager.h b/p2p_manager.h
index b2cb999..ddd0fda 100644
--- a/p2p_manager.h
+++ b/p2p_manager.h
@@ -149,6 +149,12 @@
// occurred.
virtual int CountSharedFiles() = 0;
+ // Updates the preference setting for enabling P2P. If P2P is disabled as a
+ // result, attempts to ensure that the service is not running. Returns true if
+ // the setting was updated successfully (even through stopping the service may
+ // have failed).
+ virtual bool SetP2PEnabledPref(bool enabled) = 0;
+
// Creates a suitable P2PManager instance and initializes the object
// so it's ready for use. The |file_extension| parameter is used to
// identify your application, use e.g. "cros_au". If