update_engine: Relocate inference and storage of P2P related properties.
This change moves the inference of P2P related properties from
OmahaRequestAction to OmahaResponseHandlerAction, and their storage from
OmahaRequestParams to PayloadState. This is needed in order for the
UpdateCanStart policy to be able to decide P2P properties, which only
happens after the Omaha response is received and processed, and prior to
applying the update. Further, P2P properties do not affect the Omaha
request, and so there's no reason for them to reside in
OmahaRequestParams nor decided as early as OmahaRequestAction.
Additional cleanup includes swapping expected/actual arguments to EXPECT
macros where appropriate, and removing redundant .Times(1) expectation
qualifiers.
BUG=chromium:384087
TEST=Unit tests.
Change-Id: I6d5b4b44745d5dab7e350bdf019dbf804bf196a1
Reviewed-on: https://chromium-review.googlesource.com/223618
Reviewed-by: Gilad Arnold <garnold@chromium.org>
Commit-Queue: Gilad Arnold <garnold@chromium.org>
Tested-by: Gilad Arnold <garnold@chromium.org>
diff --git a/download_action.cc b/download_action.cc
index e51549e..f37828b 100644
--- a/download_action.cc
+++ b/download_action.cc
@@ -15,6 +15,7 @@
#include "update_engine/action_pipe.h"
#include "update_engine/omaha_request_params.h"
#include "update_engine/p2p_manager.h"
+#include "update_engine/payload_state_interface.h"
#include "update_engine/subprocess.h"
#include "update_engine/utils.h"
@@ -187,9 +188,10 @@
}
if (system_state_ != nullptr) {
+ const PayloadStateInterface* payload_state = system_state_->payload_state();
string file_id = utils::CalculateP2PFileId(install_plan_.payload_hash,
install_plan_.payload_size);
- if (system_state_->request_params()->use_p2p_for_sharing()) {
+ if (payload_state->GetUsingP2PForSharing()) {
// If we're sharing the update, store the file_id to convey
// that we should write to the file.
p2p_file_id_ = file_id;
@@ -210,19 +212,17 @@
}
}
}
- }
- // Tweak timeouts on the HTTP fetcher if we're downloading from a
- // local peer.
- if (system_state_ != nullptr &&
- system_state_->request_params()->use_p2p_for_downloading() &&
- system_state_->request_params()->p2p_url() ==
- install_plan_.download_url) {
- LOG(INFO) << "Tweaking HTTP fetcher since we're downloading via p2p";
- http_fetcher_->set_low_speed_limit(kDownloadP2PLowSpeedLimitBps,
- kDownloadP2PLowSpeedTimeSeconds);
- http_fetcher_->set_max_retry_count(kDownloadP2PMaxRetryCount);
- http_fetcher_->set_connect_timeout(kDownloadP2PConnectTimeoutSeconds);
+ // Tweak timeouts on the HTTP fetcher if we're downloading from a
+ // local peer.
+ if (payload_state->GetUsingP2PForDownloading() &&
+ payload_state->GetP2PUrl() == install_plan_.download_url) {
+ LOG(INFO) << "Tweaking HTTP fetcher since we're downloading via p2p";
+ http_fetcher_->set_low_speed_limit(kDownloadP2PLowSpeedLimitBps,
+ kDownloadP2PLowSpeedTimeSeconds);
+ http_fetcher_->set_max_retry_count(kDownloadP2PMaxRetryCount);
+ http_fetcher_->set_connect_timeout(kDownloadP2PConnectTimeoutSeconds);
+ }
}
http_fetcher_->BeginTransfer(install_plan_.download_url);
diff --git a/download_action_unittest.cc b/download_action_unittest.cc
index 705d759..93a72ae 100644
--- a/download_action_unittest.cc
+++ b/download_action_unittest.cc
@@ -35,6 +35,7 @@
using std::vector;
using testing::AtLeast;
using testing::InSequence;
+using testing::Return;
using testing::_;
class DownloadActionTest : public ::testing::Test { };
@@ -476,8 +477,9 @@
// |use_p2p_to_share| parameter is used to indicate whether the
// payload should be shared via p2p.
void StartDownload(bool use_p2p_to_share) {
- fake_system_state_.request_params()->set_use_p2p_for_sharing(
- use_p2p_to_share);
+ EXPECT_CALL(*fake_system_state_.mock_payload_state(),
+ GetUsingP2PForSharing())
+ .WillRepeatedly(Return(use_p2p_to_share));
ScopedTempFile output_temp_file;
TestDirectFileWriter writer;
@@ -562,9 +564,9 @@
// Check the p2p file and its content matches what was sent.
string file_id = download_action_->p2p_file_id();
- EXPECT_NE(file_id, "");
- EXPECT_EQ(p2p_manager_->FileGetSize(file_id), data_.length());
- EXPECT_EQ(p2p_manager_->FileGetExpectedSize(file_id), data_.length());
+ EXPECT_NE("", file_id);
+ EXPECT_EQ(data_.length(), p2p_manager_->FileGetSize(file_id));
+ EXPECT_EQ(data_.length(), p2p_manager_->FileGetExpectedSize(file_id));
string p2p_file_contents;
EXPECT_TRUE(ReadFileToString(p2p_manager_->FileGetPath(file_id),
&p2p_file_contents));
diff --git a/mock_payload_state.h b/mock_payload_state.h
index c317543..2d40a19 100644
--- a/mock_payload_state.h
+++ b/mock_payload_state.h
@@ -36,7 +36,9 @@
MOCK_METHOD0(P2PNewAttempt, void());
MOCK_METHOD0(P2PAttemptAllowed, bool());
MOCK_METHOD1(SetUsingP2PForDownloading, void(bool value));
+ MOCK_METHOD1(SetUsingP2PForSharing, void(bool value));
MOCK_METHOD1(SetScatteringWaitPeriod, void(base::TimeDelta));
+ MOCK_METHOD1(SetP2PUrl, void(const std::string&));
// Getters.
MOCK_METHOD0(GetResponseSignature, std::string());
@@ -55,8 +57,10 @@
MOCK_METHOD0(GetRollbackVersion, std::string());
MOCK_METHOD0(GetP2PNumAttempts, int());
MOCK_METHOD0(GetP2PFirstAttemptTimestamp, base::Time());
- MOCK_METHOD0(GetUsingP2PForDownloading, bool());
+ MOCK_CONST_METHOD0(GetUsingP2PForDownloading, bool());
+ MOCK_CONST_METHOD0(GetUsingP2PForSharing, bool());
MOCK_METHOD0(GetScatteringWaitPeriod, base::TimeDelta());
+ MOCK_CONST_METHOD0(GetP2PUrl, std::string());
};
} // namespace chromeos_update_engine
diff --git a/omaha_request_action.cc b/omaha_request_action.cc
index fad9ad1..7d7af81 100644
--- a/omaha_request_action.cc
+++ b/omaha_request_action.cc
@@ -766,6 +766,8 @@
string current_response(response_buffer_.begin(), response_buffer_.end());
LOG(INFO) << "Omaha request response: " << current_response;
+ PayloadStateInterface* const payload_state = system_state_->payload_state();
+
// Events are best effort transactions -- assume they always succeed.
if (IsEvent()) {
CHECK(!HasOutputPipe()) << "No output pipe allowed for event requests.";
@@ -840,12 +842,12 @@
if (output_object.disable_p2p_for_downloading) {
LOG(INFO) << "Forcibly disabling use of p2p for downloading as "
<< "requested by Omaha.";
- params_->set_use_p2p_for_downloading(false);
+ payload_state->SetUsingP2PForDownloading(false);
}
if (output_object.disable_p2p_for_sharing) {
LOG(INFO) << "Forcibly disabling use of p2p for sharing as "
<< "requested by Omaha.";
- params_->set_use_p2p_for_sharing(false);
+ payload_state->SetUsingP2PForSharing(false);
}
// Update the payload state with the current response. The payload state
@@ -854,17 +856,16 @@
// as possible in this method so that if a new release gets pushed and then
// got pulled back due to some issues, we don't want to clear our internal
// state unnecessarily.
- PayloadStateInterface* payload_state = system_state_->payload_state();
payload_state->SetResponse(output_object);
// It could be we've already exceeded the deadline for when p2p is
// allowed or that we've tried too many times with p2p. Check that.
- if (params_->use_p2p_for_downloading()) {
+ if (payload_state->GetUsingP2PForDownloading()) {
payload_state->P2PNewAttempt();
if (!payload_state->P2PAttemptAllowed()) {
LOG(INFO) << "Forcibly disabling use of p2p for downloading because "
<< "of previous failures when using p2p.";
- params_->set_use_p2p_for_downloading(false);
+ payload_state->SetUsingP2PForDownloading(false);
}
}
@@ -877,7 +878,7 @@
// attention to wall-clock-based waiting if the URL is indeed
// available via p2p. Therefore, check if the file is available via
// p2p before deferring...
- if (params_->use_p2p_for_downloading()) {
+ if (payload_state->GetUsingP2PForDownloading()) {
LookupPayloadViaP2P(output_object);
} else {
CompleteProcessing();
@@ -909,11 +910,11 @@
void OmahaRequestAction::OnLookupPayloadViaP2PCompleted(const string& url) {
LOG(INFO) << "Lookup complete, p2p-client returned URL '" << url << "'";
if (!url.empty()) {
- params_->set_p2p_url(url);
+ system_state_->payload_state()->SetP2PUrl(url);
} else {
LOG(INFO) << "Forcibly disabling use of p2p for downloading "
<< "because no suitable peer could be found.";
- params_->set_use_p2p_for_downloading(false);
+ system_state_->payload_state()->SetUsingP2PForDownloading(false);
}
CompleteProcessing();
}
@@ -971,7 +972,9 @@
// defer the download. This is because the download will always
// happen from a peer on the LAN and we've been waiting in line for
// our turn.
- if (params_->use_p2p_for_downloading() && !params_->p2p_url().empty()) {
+ const PayloadStateInterface* payload_state = system_state_->payload_state();
+ if (payload_state->GetUsingP2PForDownloading() &&
+ !payload_state->GetP2PUrl().empty()) {
LOG(INFO) << "Download not deferred because download "
<< "will happen from a local peer (via p2p).";
return false;
diff --git a/omaha_request_action_unittest.cc b/omaha_request_action_unittest.cc
index 775f074..4adf998 100644
--- a/omaha_request_action_unittest.cc
+++ b/omaha_request_action_unittest.cc
@@ -19,6 +19,7 @@
#include "update_engine/fake_prefs.h"
#include "update_engine/mock_connection_manager.h"
#include "update_engine/mock_http_fetcher.h"
+#include "update_engine/mock_payload_state.h"
#include "update_engine/omaha_hash_calculator.h"
#include "update_engine/omaha_request_action.h"
#include "update_engine/omaha_request_params.h"
@@ -37,6 +38,8 @@
using testing::Le;
using testing::NiceMock;
using testing::Return;
+using testing::ReturnPointee;
+using testing::SaveArg;
using testing::SetArgumentPointee;
using testing::_;
@@ -116,9 +119,7 @@
false, // delta okay
false, // interactive
"http://url",
- "", // target_version_prefix
- false, // use_p2p_for_downloading
- false}; // use_p2p_for_sharing
+ ""}; // target_version_prefix
FakePrefs fake_prefs_;
};
@@ -1049,9 +1050,7 @@
false, // delta okay
false, // interactive
"http://url",
- "", // target_version_prefix
- false, // use_p2p_for_downloading
- false); // use_p2p_for_sharing
+ ""); // target_version_prefix
OmahaResponse response;
ASSERT_FALSE(
TestUpdateCheck(¶ms,
@@ -1247,9 +1246,7 @@
delta_okay,
false, // interactive
"http://url",
- "", // target_version_prefix
- false, // use_p2p_for_downloading
- false); // use_p2p_for_sharing
+ ""); // target_version_prefix
ASSERT_FALSE(TestUpdateCheck(¶ms,
"invalid xml>",
-1,
@@ -1290,9 +1287,7 @@
true, // delta_okay
interactive,
"http://url",
- "", // target_version_prefix
- false, // use_p2p_for_downloading
- false); // use_p2p_for_sharing
+ ""); // target_version_prefix
ASSERT_FALSE(TestUpdateCheck(¶ms,
"invalid xml>",
-1,
@@ -1924,13 +1919,25 @@
const string& expected_p2p_url) {
OmahaResponse response;
OmahaRequestParams request_params = request_params_;
- request_params.set_use_p2p_for_downloading(initial_allow_p2p_for_downloading);
- request_params.set_use_p2p_for_sharing(initial_allow_p2p_for_sharing);
+ bool actual_allow_p2p_for_downloading = initial_allow_p2p_for_downloading;
+ bool actual_allow_p2p_for_sharing = initial_allow_p2p_for_sharing;
+ string actual_p2p_url;
MockPayloadState mock_payload_state;
fake_system_state_.set_payload_state(&mock_payload_state);
EXPECT_CALL(mock_payload_state, P2PAttemptAllowed())
.WillRepeatedly(Return(payload_state_allow_p2p_attempt));
+ EXPECT_CALL(mock_payload_state, GetUsingP2PForDownloading())
+ .WillRepeatedly(ReturnPointee(&actual_allow_p2p_for_downloading));
+ EXPECT_CALL(mock_payload_state, GetUsingP2PForSharing())
+ .WillRepeatedly(ReturnPointee(&actual_allow_p2p_for_sharing));
+ EXPECT_CALL(mock_payload_state, SetUsingP2PForDownloading(_))
+ .WillRepeatedly(SaveArg<0>(&actual_allow_p2p_for_downloading));
+ EXPECT_CALL(mock_payload_state, SetUsingP2PForSharing(_))
+ .WillRepeatedly(SaveArg<0>(&actual_allow_p2p_for_sharing));
+ EXPECT_CALL(mock_payload_state, SetP2PUrl(_))
+ .WillRepeatedly(SaveArg<0>(&actual_p2p_url));
+
MockP2PManager mock_p2p_manager;
fake_system_state_.set_p2p_manager(&mock_p2p_manager);
mock_p2p_manager.fake().SetLookupUrlForFileResult(p2p_client_result_url);
@@ -1965,18 +1972,15 @@
nullptr));
EXPECT_TRUE(response.update_exists);
- EXPECT_EQ(response.disable_p2p_for_downloading,
- omaha_disable_p2p_for_downloading);
- EXPECT_EQ(response.disable_p2p_for_sharing,
- omaha_disable_p2p_for_sharing);
+ EXPECT_EQ(omaha_disable_p2p_for_downloading,
+ response.disable_p2p_for_downloading);
+ EXPECT_EQ(omaha_disable_p2p_for_sharing,
+ response.disable_p2p_for_sharing);
- EXPECT_EQ(request_params.use_p2p_for_downloading(),
- expected_allow_p2p_for_downloading);
-
- EXPECT_EQ(request_params.use_p2p_for_sharing(),
- expected_allow_p2p_for_sharing);
-
- EXPECT_EQ(request_params.p2p_url(), expected_p2p_url);
+ EXPECT_EQ(expected_allow_p2p_for_downloading,
+ actual_allow_p2p_for_downloading);
+ EXPECT_EQ(expected_allow_p2p_for_sharing, actual_allow_p2p_for_sharing);
+ EXPECT_EQ(expected_p2p_url, actual_p2p_url);
}
TEST_F(OmahaRequestActionTest, P2PWithPeer) {
diff --git a/omaha_request_params.h b/omaha_request_params.h
index 6290322..94db475 100644
--- a/omaha_request_params.h
+++ b/omaha_request_params.h
@@ -45,9 +45,7 @@
max_update_checks_allowed_(kDefaultMaxUpdateChecks),
is_powerwash_allowed_(false),
force_lock_down_(false),
- forced_lock_down_(false),
- use_p2p_for_downloading_(false),
- use_p2p_for_sharing_(false) {
+ forced_lock_down_(false) {
InitFromLsbValue();
}
@@ -66,9 +64,7 @@
bool in_delta_okay,
bool in_interactive,
const std::string& in_update_url,
- const std::string& in_target_version_prefix,
- bool in_use_p2p_for_downloading,
- bool in_use_p2p_for_sharing)
+ const std::string& in_target_version_prefix)
: system_state_(system_state),
os_platform_(in_os_platform),
os_version_(in_os_version),
@@ -93,9 +89,7 @@
max_update_checks_allowed_(kDefaultMaxUpdateChecks),
is_powerwash_allowed_(false),
force_lock_down_(false),
- forced_lock_down_(false),
- use_p2p_for_downloading_(in_use_p2p_for_downloading),
- use_p2p_for_sharing_(in_use_p2p_for_sharing) {}
+ forced_lock_down_(false) {}
// Setters and getters for the various properties.
inline std::string os_platform() const { return os_platform_; }
@@ -171,27 +165,6 @@
return max_update_checks_allowed_;
}
- inline void set_use_p2p_for_downloading(bool value) {
- use_p2p_for_downloading_ = value;
- }
- inline bool use_p2p_for_downloading() const {
- return use_p2p_for_downloading_;
- }
-
- inline void set_use_p2p_for_sharing(bool value) {
- use_p2p_for_sharing_ = value;
- }
- inline bool use_p2p_for_sharing() const {
- return use_p2p_for_sharing_;
- }
-
- inline void set_p2p_url(const std::string& value) {
- p2p_url_ = value;
- }
- inline std::string p2p_url() const {
- return p2p_url_;
- }
-
// True if we're trying to update to a more stable channel.
// i.e. index(target_channel) > index(current_channel).
virtual bool to_more_stable_channel() const;
@@ -377,18 +350,6 @@
bool force_lock_down_;
bool forced_lock_down_;
- // True if we may use p2p to download. This is based on owner
- // preferences and policy.
- bool use_p2p_for_downloading_;
-
- // True if we may use p2p to share. This is based on owner
- // preferences and policy.
- bool use_p2p_for_sharing_;
-
- // An URL to a local peer serving the requested payload or "" if no
- // such peer is available.
- std::string p2p_url_;
-
// TODO(jaysri): Uncomment this after fixing unit tests, as part of
// chromium-os:39752
// DISALLOW_COPY_AND_ASSIGN(OmahaRequestParams);
diff --git a/omaha_response_handler_action.cc b/omaha_response_handler_action.cc
index 4846a00..fcb312a 100644
--- a/omaha_response_handler_action.cc
+++ b/omaha_response_handler_action.cc
@@ -64,15 +64,17 @@
install_plan_.download_url = current_url;
install_plan_.version = response.version;
- OmahaRequestParams* params = system_state_->request_params();
+ OmahaRequestParams* const params = system_state_->request_params();
+ PayloadStateInterface* const payload_state = system_state_->payload_state();
// If we're using p2p to download and there is a local peer, use it.
- if (params->use_p2p_for_downloading() && !params->p2p_url().empty()) {
+ if (payload_state->GetUsingP2PForDownloading() &&
+ !payload_state->GetP2PUrl().empty()) {
LOG(INFO) << "Replacing URL " << install_plan_.download_url
- << " with local URL " << params->p2p_url()
+ << " with local URL " << payload_state->GetP2PUrl()
<< " since p2p is enabled.";
- install_plan_.download_url = params->p2p_url();
- system_state_->payload_state()->SetUsingP2PForDownloading(true);
+ install_plan_.download_url = payload_state->GetP2PUrl();
+ payload_state->SetUsingP2PForDownloading(true);
}
// Fill up the other properties based on the response.
@@ -85,9 +87,9 @@
install_plan_.is_resume =
DeltaPerformer::CanResumeUpdate(system_state_->prefs(), response.hash);
if (install_plan_.is_resume) {
- system_state_->payload_state()->UpdateResumed();
+ payload_state->UpdateResumed();
} else {
- system_state_->payload_state()->UpdateRestarted();
+ payload_state->UpdateRestarted();
LOG_IF(WARNING, !DeltaPerformer::ResetUpdateProgress(
system_state_->prefs(), false))
<< "Unable to reset the update progress.";
diff --git a/omaha_response_handler_action_unittest.cc b/omaha_response_handler_action_unittest.cc
index 242cf61..49b5c13 100644
--- a/omaha_response_handler_action_unittest.cc
+++ b/omaha_response_handler_action_unittest.cc
@@ -8,6 +8,7 @@
#include "update_engine/constants.h"
#include "update_engine/fake_system_state.h"
+#include "update_engine/mock_payload_state.h"
#include "update_engine/omaha_response_handler_action.h"
#include "update_engine/test_utils.h"
#include "update_engine/utils.h"
@@ -349,8 +350,10 @@
SetUsingP2PForDownloading(true));
string p2p_url = "http://9.8.7.6/p2p";
- params.set_p2p_url(p2p_url);
- params.set_use_p2p_for_downloading(true);
+ EXPECT_CALL(*fake_system_state.mock_payload_state(), GetP2PUrl())
+ .WillRepeatedly(Return(p2p_url));
+ EXPECT_CALL(*fake_system_state.mock_payload_state(),
+ GetUsingP2PForDownloading()).WillRepeatedly(Return(true));
InstallPlan install_plan;
EXPECT_TRUE(DoTestCommon(&fake_system_state, in, "/dev/sda5", "",
diff --git a/payload_state.cc b/payload_state.cc
index 4ce3d67..87965d7 100644
--- a/payload_state.cc
+++ b/payload_state.cc
@@ -41,12 +41,12 @@
PayloadState::PayloadState()
: prefs_(nullptr),
using_p2p_for_downloading_(false),
+ p2p_num_attempts_(0),
payload_attempt_number_(0),
full_payload_attempt_number_(0),
url_index_(0),
url_failure_count_(0),
url_switch_count_(0),
- p2p_num_attempts_(0),
attempt_num_bytes_downloaded_(0),
attempt_connection_type_(metrics::ConnectionType::kUnknown),
attempt_type_(AttemptType::kUpdate) {
@@ -357,8 +357,7 @@
"Can proceed with the download";
return false;
}
- if (system_state_->request_params()->use_p2p_for_downloading() &&
- !system_state_->request_params()->p2p_url().empty()) {
+ if (GetUsingP2PForDownloading() && !GetP2PUrl().empty()) {
LOG(INFO) << "Payload backoff logic is disabled because download "
<< "will happen from local peer (via p2p).";
return false;
diff --git a/payload_state.h b/payload_state.h
index 8eb541c..a301ceb 100644
--- a/payload_state.h
+++ b/payload_state.h
@@ -50,6 +50,10 @@
virtual void ExpectRebootInNewVersion(const std::string& target_version_uid);
virtual void SetUsingP2PForDownloading(bool value);
+ void SetUsingP2PForSharing(bool value) override {
+ using_p2p_for_sharing_ = value;
+ }
+
virtual inline std::string GetResponseSignature() {
return response_signature_;
}
@@ -109,16 +113,28 @@
virtual void P2PNewAttempt();
virtual bool P2PAttemptAllowed();
- virtual bool GetUsingP2PForDownloading() {
+ bool GetUsingP2PForDownloading() const override {
return using_p2p_for_downloading_;
}
+ bool GetUsingP2PForSharing() const override {
+ return using_p2p_for_sharing_;
+ }
+
base::TimeDelta GetScatteringWaitPeriod() override {
return scattering_wait_period_;
}
void SetScatteringWaitPeriod(base::TimeDelta wait_period) override;
+ void SetP2PUrl(const std::string& url) override {
+ p2p_url_ = url;
+ }
+
+ std::string GetP2PUrl() const override {
+ return p2p_url_;
+ }
+
private:
enum class AttemptType {
kUpdate,
@@ -410,9 +426,18 @@
// This is the current response object from Omaha.
OmahaResponse response_;
- // Whether p2p is being used for downloading as set with the
- // SetUsingP2PForDownloading() method.
+ // Whether P2P is being used for downloading and sharing.
bool using_p2p_for_downloading_;
+ bool using_p2p_for_sharing_;
+
+ // Stores the P2P download URL, if one is used.
+ std::string p2p_url_;
+
+ // The cached value of |kPrefsP2PFirstAttemptTimestamp|.
+ base::Time p2p_first_attempt_timestamp_;
+
+ // The cached value of |kPrefsP2PNumAttempts|.
+ int p2p_num_attempts_;
// This stores a "signature" of the current response. The signature here
// refers to a subset of the current response from Omaha. Each update to
@@ -517,12 +542,6 @@
// reboot.
std::string rollback_version_;
- // The cached value of |kPrefsP2PFirstAttemptTimestamp|.
- base::Time p2p_first_attempt_timestamp_;
-
- // The cached value of |kPrefsP2PNumAttempts|.
- int p2p_num_attempts_;
-
// The number of bytes downloaded per attempt.
int64_t attempt_num_bytes_downloaded_;
diff --git a/payload_state_interface.h b/payload_state_interface.h
index c706e98..1825a40 100644
--- a/payload_state_interface.h
+++ b/payload_state_interface.h
@@ -74,11 +74,14 @@
virtual void ExpectRebootInNewVersion(
const std::string& target_version_uid) = 0;
- // Sets whether p2p is being used to download the update payload. This
- // is used to keep track download sources being used and should be called
+ // Sets whether P2P is being used to download the update payload. This
+ // is used to keep track of download sources being used and should be called
// before the transfer begins.
virtual void SetUsingP2PForDownloading(bool value) = 0;
+ // Sets whether P2P is being used for sharing the update payloads.
+ virtual void SetUsingP2PForSharing(bool value) = 0;
+
// Returns true if we should backoff the current download attempt.
// False otherwise.
virtual bool ShouldBackoffDownload() = 0;
@@ -161,14 +164,20 @@
// as when the first attempt was.
virtual bool P2PAttemptAllowed() = 0;
- // Gets the value previously set with SetUsingP2PForDownloading().
- virtual bool GetUsingP2PForDownloading() = 0;
+ // Gets the values previously set with SetUsingP2PForDownloading() and
+ // SetUsingP2PForSharing().
+ virtual bool GetUsingP2PForDownloading() const = 0;
+ virtual bool GetUsingP2PForSharing() const = 0;
// Returns the current (persisted) scattering wallclock-based wait period.
virtual base::TimeDelta GetScatteringWaitPeriod() = 0;
// Sets and persists the scattering wallclock-based wait period.
virtual void SetScatteringWaitPeriod(base::TimeDelta wait_period) = 0;
+
+ // Sets/gets the P2P download URL, if one is to be used.
+ virtual void SetP2PUrl(const std::string& url) = 0;
+ virtual std::string GetP2PUrl() const = 0;
};
} // namespace chromeos_update_engine
diff --git a/payload_state_unittest.cc b/payload_state_unittest.cc
index cec69f1..9d73200 100644
--- a/payload_state_unittest.cc
+++ b/payload_state_unittest.cc
@@ -695,12 +695,12 @@
EXPECT_EQ(1, payload_state.GetPayloadAttemptNumber());
EXPECT_EQ(1, payload_state.GetFullPayloadAttemptNumber());
// Set p2p url.
- params.set_use_p2p_for_downloading(true);
- params.set_p2p_url("http://mypeer:52909/path/to/file");
+ payload_state.SetUsingP2PForDownloading(true);
+ payload_state.SetP2PUrl("http://mypeer:52909/path/to/file");
// Should not backoff for p2p updates.
EXPECT_FALSE(payload_state.ShouldBackoffDownload());
- params.set_p2p_url("");
+ payload_state.SetP2PUrl("");
// No actual p2p update if no url is provided.
EXPECT_TRUE(payload_state.ShouldBackoffDownload());
}
diff --git a/update_attempter.cc b/update_attempter.cc
index 2911540..200493d 100644
--- a/update_attempter.cc
+++ b/update_attempter.cc
@@ -352,8 +352,9 @@
}
}
- omaha_request_params_->set_use_p2p_for_downloading(use_p2p_for_downloading);
- omaha_request_params_->set_use_p2p_for_sharing(use_p2p_for_sharing);
+ PayloadStateInterface* const payload_state = system_state_->payload_state();
+ payload_state->SetUsingP2PForDownloading(use_p2p_for_downloading);
+ payload_state->SetUsingP2PForSharing(use_p2p_for_sharing);
}
bool UpdateAttempter::CalculateUpdateParams(const string& app_version,
@@ -363,6 +364,7 @@
bool obey_proxies,
bool interactive) {
http_response_code_ = 0;
+ PayloadStateInterface* const payload_state = system_state_->payload_state();
// Refresh the policy before computing all the update parameters.
RefreshDevicePolicy();
@@ -374,15 +376,15 @@
CalculateScatteringParams(interactive);
CalculateP2PParams(interactive);
- if (omaha_request_params_->use_p2p_for_downloading() ||
- omaha_request_params_->use_p2p_for_sharing()) {
+ if (payload_state->GetUsingP2PForDownloading() ||
+ payload_state->GetUsingP2PForSharing()) {
// OK, p2p is to be used - start it and perform housekeeping.
if (!StartP2PAndPerformHousekeeping()) {
// If this fails, disable p2p for this attempt
LOG(INFO) << "Forcibly disabling use of p2p since starting p2p or "
<< "performing housekeeping failed.";
- omaha_request_params_->set_use_p2p_for_downloading(false);
- omaha_request_params_->set_use_p2p_for_sharing(false);
+ payload_state->SetUsingP2PForDownloading(false);
+ payload_state->SetUsingP2PForSharing(false);
}
}
@@ -423,9 +425,9 @@
omaha_request_params_->waiting_period().InSeconds());
LOG(INFO) << "Use p2p For Downloading = "
- << omaha_request_params_->use_p2p_for_downloading()
+ << payload_state->GetUsingP2PForDownloading()
<< ", Use p2p For Sharing = "
- << omaha_request_params_->use_p2p_for_sharing();
+ << payload_state->GetUsingP2PForSharing();
obeying_proxies_ = true;
if (obey_proxies || proxy_manual_checks_ == 0) {
diff --git a/update_attempter_unittest.cc b/update_attempter_unittest.cc
index 9098180..6dc921b 100644
--- a/update_attempter_unittest.cc
+++ b/update_attempter_unittest.cc
@@ -37,7 +37,9 @@
using testing::Ne;
using testing::NiceMock;
using testing::Property;
+using testing::SaveArg;
using testing::Return;
+using testing::ReturnPointee;
using testing::SetArgumentPointee;
using testing::_;
@@ -118,6 +120,22 @@
processor_ = new NiceMock<ActionProcessorMock>();
attempter_.processor_.reset(processor_); // Transfers ownership.
prefs_ = fake_system_state_.mock_prefs();
+
+ // Set up store/load semantics of P2P properties via the mock PayloadState.
+ actual_using_p2p_for_downloading_ = false;
+ EXPECT_CALL(*fake_system_state_.mock_payload_state(),
+ SetUsingP2PForDownloading(_))
+ .WillRepeatedly(SaveArg<0>(&actual_using_p2p_for_downloading_));
+ EXPECT_CALL(*fake_system_state_.mock_payload_state(),
+ GetUsingP2PForDownloading())
+ .WillRepeatedly(ReturnPointee(&actual_using_p2p_for_downloading_));
+ actual_using_p2p_for_sharing_ = false;
+ EXPECT_CALL(*fake_system_state_.mock_payload_state(),
+ SetUsingP2PForSharing(_))
+ .WillRepeatedly(SaveArg<0>(&actual_using_p2p_for_sharing_));
+ EXPECT_CALL(*fake_system_state_.mock_payload_state(),
+ GetUsingP2PForDownloading())
+ .WillRepeatedly(ReturnPointee(&actual_using_p2p_for_sharing_));
}
virtual void TearDown() {
@@ -169,6 +187,13 @@
void P2PEnabledHousekeepingFailsStart();
static gboolean StaticP2PEnabledHousekeepingFails(gpointer data);
+ bool actual_using_p2p_for_downloading() {
+ return actual_using_p2p_for_downloading_;
+ }
+ bool actual_using_p2p_for_sharing() {
+ return actual_using_p2p_for_sharing_;
+ }
+
FakeSystemState fake_system_state_;
NiceMock<MockDBusWrapper> dbus_;
UpdateAttempterUnderTest attempter_;
@@ -178,6 +203,9 @@
GMainLoop* loop_;
string test_dir_;
+
+ bool actual_using_p2p_for_downloading_;
+ bool actual_using_p2p_for_sharing_;
};
TEST_F(UpdateAttempterTest, ActionCompletedDownloadTest) {
@@ -257,7 +285,7 @@
GetErrorCodeForAction(&postinstall_runner_action,
ErrorCode::kError));
ActionMock action_mock;
- EXPECT_CALL(action_mock, Type()).Times(1).WillOnce(Return("ActionMock"));
+ EXPECT_CALL(action_mock, Type()).WillOnce(Return("ActionMock"));
EXPECT_EQ(ErrorCode::kError,
GetErrorCodeForAction(&action_mock, ErrorCode::kError));
}
@@ -296,10 +324,9 @@
EXPECT_CALL(*prefs_, SetInt64(Ne(kPrefsDeltaUpdateFailures), _))
.WillRepeatedly(Return(true));
EXPECT_CALL(*prefs_, SetInt64(kPrefsDeltaUpdateFailures, 1)).Times(2);
- EXPECT_CALL(*prefs_, SetInt64(kPrefsDeltaUpdateFailures, 2)).Times(1);
+ EXPECT_CALL(*prefs_, SetInt64(kPrefsDeltaUpdateFailures, 2));
EXPECT_CALL(*prefs_, SetInt64(kPrefsDeltaUpdateFailures,
- UpdateAttempter::kMaxDeltaUpdateFailures + 1))
- .Times(1);
+ UpdateAttempter::kMaxDeltaUpdateFailures + 1));
for (int i = 0; i < 4; i ++)
attempter_.MarkDeltaUpdateFailure();
}
@@ -323,9 +350,8 @@
TEST_F(UpdateAttempterTest, ScheduleErrorEventActionTest) {
EXPECT_CALL(*processor_,
EnqueueAction(Property(&AbstractAction::Type,
- OmahaRequestAction::StaticType())))
- .Times(1);
- EXPECT_CALL(*processor_, StartProcessing()).Times(1);
+ OmahaRequestAction::StaticType())));
+ EXPECT_CALL(*processor_, StartProcessing());
ErrorCode err = ErrorCode::kError;
EXPECT_CALL(*fake_system_state_.mock_payload_state(), UpdateFailed(err));
attempter_.error_event_.reset(new OmahaEvent(OmahaEvent::kTypeUpdateComplete,
@@ -443,9 +469,9 @@
for (size_t i = 0; i < arraysize(kUpdateActionTypes); ++i) {
EXPECT_CALL(*processor_,
EnqueueAction(Property(&AbstractAction::Type,
- kUpdateActionTypes[i]))).Times(1);
+ kUpdateActionTypes[i])));
}
- EXPECT_CALL(*processor_, StartProcessing()).Times(1);
+ EXPECT_CALL(*processor_, StartProcessing());
}
attempter_.Update("", "", "", "", false, false);
@@ -511,9 +537,9 @@
for (size_t i = 0; i < arraysize(kRollbackActionTypes); ++i) {
EXPECT_CALL(*processor_,
EnqueueAction(Property(&AbstractAction::Type,
- kRollbackActionTypes[i]))).Times(1);
+ kRollbackActionTypes[i])));
}
- EXPECT_CALL(*processor_, StartProcessing()).Times(1);
+ EXPECT_CALL(*processor_, StartProcessing());
EXPECT_TRUE(attempter_.Rollback(true));
g_idle_add(&StaticRollbackTestVerify, this);
@@ -577,9 +603,8 @@
void UpdateAttempterTest::PingOmahaTestStart() {
EXPECT_CALL(*processor_,
EnqueueAction(Property(&AbstractAction::Type,
- OmahaRequestAction::StaticType())))
- .Times(1);
- EXPECT_CALL(*processor_, StartProcessing()).Times(1);
+ OmahaRequestAction::StaticType())));
+ EXPECT_CALL(*processor_, StartProcessing());
attempter_.PingOmaha();
g_idle_add(&StaticQuitMainLoop, this);
}
@@ -652,7 +677,7 @@
fake_system_state_.set_p2p_manager(&mock_p2p_manager);
mock_p2p_manager.fake().SetP2PEnabled(true);
mock_p2p_manager.fake().SetCountSharedFilesResult(1);
- EXPECT_CALL(mock_p2p_manager, EnsureP2PRunning()).Times(1);
+ EXPECT_CALL(mock_p2p_manager, EnsureP2PRunning());
attempter_.UpdateEngineStarted();
}
@@ -676,8 +701,8 @@
mock_p2p_manager.fake().SetP2PEnabled(false);
EXPECT_CALL(mock_p2p_manager, PerformHousekeeping()).Times(0);
attempter_.Update("", "", "", "", false, false);
- EXPECT_FALSE(attempter_.omaha_request_params_->use_p2p_for_downloading());
- EXPECT_FALSE(attempter_.omaha_request_params_->use_p2p_for_sharing());
+ EXPECT_FALSE(actual_using_p2p_for_downloading());
+ EXPECT_FALSE(actual_using_p2p_for_sharing());
g_idle_add(&StaticQuitMainLoop, this);
}
@@ -704,8 +729,8 @@
mock_p2p_manager.fake().SetPerformHousekeepingResult(false);
EXPECT_CALL(mock_p2p_manager, PerformHousekeeping()).Times(0);
attempter_.Update("", "", "", "", false, false);
- EXPECT_FALSE(attempter_.omaha_request_params_->use_p2p_for_downloading());
- EXPECT_FALSE(attempter_.omaha_request_params_->use_p2p_for_sharing());
+ EXPECT_FALSE(actual_using_p2p_for_downloading());
+ EXPECT_FALSE(actual_using_p2p_for_sharing());
g_idle_add(&StaticQuitMainLoop, this);
}
@@ -730,10 +755,10 @@
mock_p2p_manager.fake().SetP2PEnabled(true);
mock_p2p_manager.fake().SetEnsureP2PRunningResult(true);
mock_p2p_manager.fake().SetPerformHousekeepingResult(false);
- EXPECT_CALL(mock_p2p_manager, PerformHousekeeping()).Times(1);
+ EXPECT_CALL(mock_p2p_manager, PerformHousekeeping());
attempter_.Update("", "", "", "", false, false);
- EXPECT_FALSE(attempter_.omaha_request_params_->use_p2p_for_downloading());
- EXPECT_FALSE(attempter_.omaha_request_params_->use_p2p_for_sharing());
+ EXPECT_FALSE(actual_using_p2p_for_downloading());
+ EXPECT_FALSE(actual_using_p2p_for_sharing());
g_idle_add(&StaticQuitMainLoop, this);
}
@@ -757,10 +782,10 @@
mock_p2p_manager.fake().SetP2PEnabled(true);
mock_p2p_manager.fake().SetEnsureP2PRunningResult(true);
mock_p2p_manager.fake().SetPerformHousekeepingResult(true);
- EXPECT_CALL(mock_p2p_manager, PerformHousekeeping()).Times(1);
+ EXPECT_CALL(mock_p2p_manager, PerformHousekeeping());
attempter_.Update("", "", "", "", false, false);
- EXPECT_TRUE(attempter_.omaha_request_params_->use_p2p_for_downloading());
- EXPECT_TRUE(attempter_.omaha_request_params_->use_p2p_for_sharing());
+ EXPECT_TRUE(actual_using_p2p_for_downloading());
+ EXPECT_TRUE(actual_using_p2p_for_sharing());
g_idle_add(&StaticQuitMainLoop, this);
}
@@ -785,10 +810,10 @@
mock_p2p_manager.fake().SetP2PEnabled(true);
mock_p2p_manager.fake().SetEnsureP2PRunningResult(true);
mock_p2p_manager.fake().SetPerformHousekeepingResult(true);
- EXPECT_CALL(mock_p2p_manager, PerformHousekeeping()).Times(1);
+ EXPECT_CALL(mock_p2p_manager, PerformHousekeeping());
attempter_.Update("", "", "", "", false, true /* interactive */);
- EXPECT_FALSE(attempter_.omaha_request_params_->use_p2p_for_downloading());
- EXPECT_TRUE(attempter_.omaha_request_params_->use_p2p_for_sharing());
+ EXPECT_FALSE(actual_using_p2p_for_downloading());
+ EXPECT_TRUE(actual_using_p2p_for_sharing());
g_idle_add(&StaticQuitMainLoop, this);
}