diff options
-rw-r--r-- | download_action.cc | 26 | ||||
-rw-r--r-- | download_action_unittest.cc | 12 | ||||
-rw-r--r-- | mock_payload_state.h | 6 | ||||
-rw-r--r-- | omaha_request_action.cc | 21 | ||||
-rw-r--r-- | omaha_request_action_unittest.cc | 54 | ||||
-rw-r--r-- | omaha_request_params.h | 45 | ||||
-rw-r--r-- | omaha_response_handler_action.cc | 16 | ||||
-rw-r--r-- | omaha_response_handler_action_unittest.cc | 7 | ||||
-rw-r--r-- | payload_state.cc | 5 | ||||
-rw-r--r-- | payload_state.h | 37 | ||||
-rw-r--r-- | payload_state_interface.h | 17 | ||||
-rw-r--r-- | payload_state_unittest.cc | 6 | ||||
-rw-r--r-- | update_attempter.cc | 18 | ||||
-rw-r--r-- | update_attempter_unittest.cc | 81 |
14 files changed, 192 insertions, 159 deletions
diff --git a/download_action.cc b/download_action.cc index e51549e3..f37828b0 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 @@ void DownloadAction::PerformAction() { } 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 @@ void DownloadAction::PerformAction() { } } } - } - // 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 705d7590..93a72aed 100644 --- a/download_action_unittest.cc +++ b/download_action_unittest.cc @@ -35,6 +35,7 @@ using std::unique_ptr; using std::vector; using testing::AtLeast; using testing::InSequence; +using testing::Return; using testing::_; class DownloadActionTest : public ::testing::Test { }; @@ -476,8 +477,9 @@ class P2PDownloadActionTest : public testing::Test { // |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 @@ TEST_F(P2PDownloadActionTest, IsWrittenTo) { // 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 c3175434..2d40a196 100644 --- a/mock_payload_state.h +++ b/mock_payload_state.h @@ -36,7 +36,9 @@ class MockPayloadState: public PayloadStateInterface { 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 @@ class MockPayloadState: public PayloadStateInterface { 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 fad9ad11..7d7af81a 100644 --- a/omaha_request_action.cc +++ b/omaha_request_action.cc @@ -766,6 +766,8 @@ void OmahaRequestAction::TransferComplete(HttpFetcher *fetcher, 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 @@ void OmahaRequestAction::TransferComplete(HttpFetcher *fetcher, 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 @@ void OmahaRequestAction::TransferComplete(HttpFetcher *fetcher, // 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 @@ void OmahaRequestAction::TransferComplete(HttpFetcher *fetcher, // 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::CompleteProcessing() { 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 @@ bool OmahaRequestAction::ShouldDeferDownload(OmahaResponse* output_object) { // 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 775f074b..4adf9987 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::Ge; using testing::Le; using testing::NiceMock; using testing::Return; +using testing::ReturnPointee; +using testing::SaveArg; using testing::SetArgumentPointee; using testing::_; @@ -116,9 +119,7 @@ class OmahaRequestActionTest : public ::testing::Test { 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 @@ TEST_F(OmahaRequestActionTest, XmlEncodeTest) { 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 @@ TEST_F(OmahaRequestActionTest, FormatDeltaOkayOutputTest) { 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 @@ TEST_F(OmahaRequestActionTest, FormatInteractiveOutputTest) { 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 @@ void OmahaRequestActionTest::P2PTest( 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 @@ void OmahaRequestActionTest::P2PTest( 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(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(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.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 62903223..94db4755 100644 --- a/omaha_request_params.h +++ b/omaha_request_params.h @@ -45,9 +45,7 @@ class OmahaRequestParams { 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 @@ class OmahaRequestParams { 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 @@ class OmahaRequestParams { 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 @@ class OmahaRequestParams { 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 @@ class OmahaRequestParams { 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 4846a000..fcb312a1 100644 --- a/omaha_response_handler_action.cc +++ b/omaha_response_handler_action.cc @@ -64,15 +64,17 @@ void OmahaResponseHandlerAction::PerformAction() { 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 @@ void OmahaResponseHandlerAction::PerformAction() { 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 242cf616..49b5c138 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 @@ TEST_F(OmahaResponseHandlerActionTest, P2PUrlIsUsedAndHashChecksMandatory) { 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 4ce3d673..87965d7f 100644 --- a/payload_state.cc +++ b/payload_state.cc @@ -41,12 +41,12 @@ static const uint32_t kMaxBackoffFuzzMinutes = 12 * 60; 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 @@ bool PayloadState::ShouldBackoffDownload() { "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 8eb541cd..a301cebe 100644 --- a/payload_state.h +++ b/payload_state.h @@ -50,6 +50,10 @@ class PayloadState : public PayloadStateInterface { 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 @@ class PayloadState : public PayloadStateInterface { 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 @@ class PayloadState : public PayloadStateInterface { // 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 @@ class PayloadState : public PayloadStateInterface { // 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 c706e984..1825a40d 100644 --- a/payload_state_interface.h +++ b/payload_state_interface.h @@ -74,11 +74,14 @@ class PayloadStateInterface { 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 @@ class PayloadStateInterface { // 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 cec69f1e..9d732009 100644 --- a/payload_state_unittest.cc +++ b/payload_state_unittest.cc @@ -695,12 +695,12 @@ TEST(PayloadStateTest, NoBackoffForP2PUpdates) { 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 29115402..200493d2 100644 --- a/update_attempter.cc +++ b/update_attempter.cc @@ -352,8 +352,9 @@ void UpdateAttempter::CalculateP2PParams(bool interactive) { } } - 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 UpdateAttempter::CalculateUpdateParams(const string& app_version, 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 @@ bool UpdateAttempter::CalculateUpdateParams(const string& app_version, 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 @@ bool UpdateAttempter::CalculateUpdateParams(const string& app_version, 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 9098180c..6dc921bc 100644 --- a/update_attempter_unittest.cc +++ b/update_attempter_unittest.cc @@ -37,7 +37,9 @@ using testing::InSequence; 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 @@ class UpdateAttempterTest : public ::testing::Test { 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 @@ class UpdateAttempterTest : public ::testing::Test { 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 @@ class UpdateAttempterTest : public ::testing::Test { 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 @@ TEST_F(UpdateAttempterTest, GetErrorCodeForActionTest) { 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 @@ TEST_F(UpdateAttempterTest, MarkDeltaUpdateFailureTest) { 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, ScheduleErrorEventActionNoEventTest) { 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 @@ void UpdateAttempterTest::UpdateTestStart() { 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 @@ void UpdateAttempterTest::RollbackTestStart( 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 @@ TEST_F(UpdateAttempterTest, EnterpriseRollbackTest) { 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 @@ TEST_F(UpdateAttempterTest, P2PStartedAtStartupWhenEnabledAndSharing) { 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 @@ void UpdateAttempterTest::P2PNotEnabledStart() { 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 @@ void UpdateAttempterTest::P2PEnabledStartingFailsStart() { 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 @@ void UpdateAttempterTest::P2PEnabledHousekeepingFailsStart() { 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 @@ void UpdateAttempterTest::P2PEnabledStart() { 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 @@ void UpdateAttempterTest::P2PEnabledInteractiveStart() { 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); } |