update_engine: Pass Action ownership to ActionProcessor
Currently, an object that uses an ActionProcessor for processing one or more
actions has to own the Actions. This is problematic, because if we want to
create an action on the fly and use an ActionProcessor to perform it, we have to
own the Action until it is finished. Furthermore, if someone forget to own the
action, there will be memory leaks because ActionProcessor does not delete the
Action.
This patch passes the ownership of the Actions to the ActionProcessor through
unique pointers. If an object wants to have access to the Action, it can get it
when ActionComplete() is called.
BUG=chromium:807976
TEST=unittests
TEST=cros flash
TEST=precq
Change-Id: I28f7e9fd3425f17cc51b4db4a4abc130a7d6ef8f
Reviewed-on: https://chromium-review.googlesource.com/1065113
Commit-Ready: Amin Hassani <[email protected]>
Tested-by: Amin Hassani <[email protected]>
Reviewed-by: Xiaochu Liu <[email protected]>
diff --git a/common/action_processor.cc b/common/action_processor.cc
index 3549e08..ead99c4 100644
--- a/common/action_processor.cc
+++ b/common/action_processor.cc
@@ -17,6 +17,7 @@
#include "update_engine/common/action_processor.h"
#include <string>
+#include <utility>
#include <base/logging.h>
@@ -24,27 +25,30 @@
#include "update_engine/common/error_code_utils.h"
using std::string;
+using std::unique_ptr;
namespace chromeos_update_engine {
ActionProcessor::~ActionProcessor() {
if (IsRunning())
StopProcessing();
- for (auto action : actions_)
- action->SetProcessor(nullptr);
}
-void ActionProcessor::EnqueueAction(AbstractAction* action) {
- actions_.push_back(action);
+void ActionProcessor::EnqueueAction(unique_ptr<AbstractAction> action) {
action->SetProcessor(this);
+ actions_.push_back(std::move(action));
+}
+
+bool ActionProcessor::IsRunning() const {
+ return current_action_ != nullptr || suspended_;
}
void ActionProcessor::StartProcessing() {
CHECK(!IsRunning());
if (!actions_.empty()) {
- current_action_ = actions_.front();
- LOG(INFO) << "ActionProcessor: starting " << current_action_->Type();
+ current_action_ = std::move(actions_.front());
actions_.pop_front();
+ LOG(INFO) << "ActionProcessor: starting " << current_action_->Type();
current_action_->PerformAction();
}
}
@@ -53,16 +57,13 @@
CHECK(IsRunning());
if (current_action_) {
current_action_->TerminateProcessing();
- current_action_->SetProcessor(nullptr);
}
LOG(INFO) << "ActionProcessor: aborted "
<< (current_action_ ? current_action_->Type() : "")
<< (suspended_ ? " while suspended" : "");
- current_action_ = nullptr;
+ current_action_.reset();
suspended_ = false;
// Delete all the actions before calling the delegate.
- for (auto action : actions_)
- action->SetProcessor(nullptr);
actions_.clear();
if (delegate_)
delegate_->ProcessingStopped(this);
@@ -106,13 +107,12 @@
void ActionProcessor::ActionComplete(AbstractAction* actionptr,
ErrorCode code) {
- CHECK_EQ(actionptr, current_action_);
+ CHECK_EQ(actionptr, current_action_.get());
if (delegate_)
delegate_->ActionCompleted(this, actionptr, code);
string old_type = current_action_->Type();
current_action_->ActionCompleted(code);
- current_action_->SetProcessor(nullptr);
- current_action_ = nullptr;
+ current_action_.reset();
LOG(INFO) << "ActionProcessor: finished "
<< (actions_.empty() ? "last action " : "") << old_type
<< (suspended_ ? " while suspended" : "")
@@ -138,7 +138,7 @@
}
return;
}
- current_action_ = actions_.front();
+ current_action_ = std::move(actions_.front());
actions_.pop_front();
LOG(INFO) << "ActionProcessor: starting " << current_action_->Type();
current_action_->PerformAction();
diff --git a/common/action_processor.h b/common/action_processor.h
index c9c179e..f651b8e 100644
--- a/common/action_processor.h
+++ b/common/action_processor.h
@@ -18,6 +18,8 @@
#define UPDATE_ENGINE_COMMON_ACTION_PROCESSOR_H_
#include <deque>
+#include <memory>
+#include <vector>
#include <base/macros.h>
#include <brillo/errors/error.h>
@@ -69,10 +71,10 @@
// Returns true iff the processing was started but not yet completed nor
// stopped.
- bool IsRunning() const { return current_action_ != nullptr || suspended_; }
+ bool IsRunning() const;
// Adds another Action to the end of the queue.
- virtual void EnqueueAction(AbstractAction* action);
+ virtual void EnqueueAction(std::unique_ptr<AbstractAction> action);
// Sets/gets the current delegate. Set to null to remove a delegate.
ActionProcessorDelegate* delegate() const { return delegate_; }
@@ -81,14 +83,17 @@
}
// Returns a pointer to the current Action that's processing.
- AbstractAction* current_action() const {
- return current_action_;
- }
+ AbstractAction* current_action() const { return current_action_.get(); }
// Called by an action to notify processor that it's done. Caller passes self.
+ // But this call deletes the action if there no other object has a reference
+ // to it, so in that case, the caller should not try to access any of its
+ // member variables after this call.
void ActionComplete(AbstractAction* actionptr, ErrorCode code);
private:
+ FRIEND_TEST(ActionProcessorTest, ChainActionsTest);
+
// Continue processing actions (if any) after the last action terminated with
// the passed error code. If there are no more actions to process, the
// processing will terminate.
@@ -96,10 +101,10 @@
// Actions that have not yet begun processing, in the order in which
// they'll be processed.
- std::deque<AbstractAction*> actions_;
+ std::deque<std::unique_ptr<AbstractAction>> actions_;
// A pointer to the currently processing Action, if any.
- AbstractAction* current_action_{nullptr};
+ std::unique_ptr<AbstractAction> current_action_;
// The ErrorCode reported by an action that was suspended but finished while
// being suspended. This error code is stored here to be reported back to the
diff --git a/common/action_processor_unittest.cc b/common/action_processor_unittest.cc
index 631e42d..b67eca9 100644
--- a/common/action_processor_unittest.cc
+++ b/common/action_processor_unittest.cc
@@ -17,6 +17,7 @@
#include "update_engine/common/action_processor.h"
#include <string>
+#include <utility>
#include <gtest/gtest.h>
@@ -96,7 +97,11 @@
void SetUp() override {
action_processor_.set_delegate(&delegate_);
// Silence Type() calls used for logging.
- EXPECT_CALL(mock_action_, Type()).Times(testing::AnyNumber());
+ mock_action_.reset(new testing::StrictMock<MockAction>());
+ mock_action_ptr_ = mock_action_.get();
+ action_.reset(new ActionProcessorTestAction());
+ action_ptr_ = action_.get();
+ EXPECT_CALL(*mock_action_, Type()).Times(testing::AnyNumber());
}
void TearDown() override {
@@ -110,34 +115,35 @@
MyActionProcessorDelegate delegate_{&action_processor_};
// Common actions used during most tests.
- testing::StrictMock<MockAction> mock_action_;
- ActionProcessorTestAction action_;
+ std::unique_ptr<testing::StrictMock<MockAction>> mock_action_;
+ testing::StrictMock<MockAction>* mock_action_ptr_;
+ std::unique_ptr<ActionProcessorTestAction> action_;
+ ActionProcessorTestAction* action_ptr_;
};
TEST_F(ActionProcessorTest, SimpleTest) {
EXPECT_FALSE(action_processor_.IsRunning());
- action_processor_.EnqueueAction(&action_);
+ action_processor_.EnqueueAction(std::move(action_));
EXPECT_FALSE(action_processor_.IsRunning());
- EXPECT_FALSE(action_.IsRunning());
+ EXPECT_FALSE(action_ptr_->IsRunning());
action_processor_.StartProcessing();
EXPECT_TRUE(action_processor_.IsRunning());
- EXPECT_TRUE(action_.IsRunning());
- EXPECT_EQ(action_processor_.current_action(), &action_);
- action_.CompleteAction();
+ EXPECT_TRUE(action_ptr_->IsRunning());
+ action_ptr_->CompleteAction();
EXPECT_FALSE(action_processor_.IsRunning());
- EXPECT_FALSE(action_.IsRunning());
+ EXPECT_EQ(action_processor_.current_action(), nullptr);
}
TEST_F(ActionProcessorTest, DelegateTest) {
- action_processor_.EnqueueAction(&action_);
+ action_processor_.EnqueueAction(std::move(action_));
action_processor_.StartProcessing();
- action_.CompleteAction();
+ action_ptr_->CompleteAction();
EXPECT_TRUE(delegate_.processing_done_called_);
EXPECT_TRUE(delegate_.action_completed_called_);
}
TEST_F(ActionProcessorTest, StopProcessingTest) {
- action_processor_.EnqueueAction(&action_);
+ action_processor_.EnqueueAction(std::move(action_));
action_processor_.StartProcessing();
action_processor_.StopProcessing();
EXPECT_TRUE(delegate_.processing_stopped_called_);
@@ -150,54 +156,58 @@
// This test doesn't use a delegate since it terminates several actions.
action_processor_.set_delegate(nullptr);
- ActionProcessorTestAction action1, action2;
- action_processor_.EnqueueAction(&action1);
- action_processor_.EnqueueAction(&action2);
+ auto action0 = std::make_unique<ActionProcessorTestAction>();
+ auto action1 = std::make_unique<ActionProcessorTestAction>();
+ auto action2 = std::make_unique<ActionProcessorTestAction>();
+ auto action0_ptr = action0.get();
+ auto action1_ptr = action1.get();
+ auto action2_ptr = action2.get();
+ action_processor_.EnqueueAction(std::move(action0));
+ action_processor_.EnqueueAction(std::move(action1));
+ action_processor_.EnqueueAction(std::move(action2));
+
+ EXPECT_EQ(action_processor_.actions_.size(), 3);
+ EXPECT_EQ(action_processor_.actions_[0].get(), action0_ptr);
+ EXPECT_EQ(action_processor_.actions_[1].get(), action1_ptr);
+ EXPECT_EQ(action_processor_.actions_[2].get(), action2_ptr);
+
action_processor_.StartProcessing();
- EXPECT_EQ(&action1, action_processor_.current_action());
+ EXPECT_EQ(action0_ptr, action_processor_.current_action());
EXPECT_TRUE(action_processor_.IsRunning());
- action1.CompleteAction();
- EXPECT_EQ(&action2, action_processor_.current_action());
+ action0_ptr->CompleteAction();
+ EXPECT_EQ(action1_ptr, action_processor_.current_action());
EXPECT_TRUE(action_processor_.IsRunning());
- action2.CompleteAction();
+ action1_ptr->CompleteAction();
+ EXPECT_EQ(action2_ptr, action_processor_.current_action());
+ EXPECT_TRUE(action_processor_.actions_.empty());
+ EXPECT_TRUE(action_processor_.IsRunning());
+ action2_ptr->CompleteAction();
EXPECT_EQ(nullptr, action_processor_.current_action());
+ EXPECT_TRUE(action_processor_.actions_.empty());
EXPECT_FALSE(action_processor_.IsRunning());
}
-TEST_F(ActionProcessorTest, DtorTest) {
- ActionProcessorTestAction action1, action2;
- {
- ActionProcessor action_processor;
- action_processor.EnqueueAction(&action1);
- action_processor.EnqueueAction(&action2);
- action_processor.StartProcessing();
- }
- EXPECT_EQ(nullptr, action1.processor());
- EXPECT_FALSE(action1.IsRunning());
- EXPECT_EQ(nullptr, action2.processor());
- EXPECT_FALSE(action2.IsRunning());
-}
-
TEST_F(ActionProcessorTest, DefaultDelegateTest) {
- // Just make sure it doesn't crash
- action_processor_.EnqueueAction(&action_);
+ // Just make sure it doesn't crash.
+ action_processor_.EnqueueAction(std::move(action_));
action_processor_.StartProcessing();
- action_.CompleteAction();
+ action_ptr_->CompleteAction();
- action_processor_.EnqueueAction(&action_);
+ action_.reset(new ActionProcessorTestAction());
+ action_processor_.EnqueueAction(std::move(action_));
action_processor_.StartProcessing();
action_processor_.StopProcessing();
}
-// This test suspends and resume the action processor while running one action_.
+// This test suspends and resume the action processor while running one action.
TEST_F(ActionProcessorTest, SuspendResumeTest) {
- action_processor_.EnqueueAction(&mock_action_);
+ action_processor_.EnqueueAction(std::move(mock_action_));
testing::InSequence s;
- EXPECT_CALL(mock_action_, PerformAction());
+ EXPECT_CALL(*mock_action_ptr_, PerformAction());
action_processor_.StartProcessing();
- EXPECT_CALL(mock_action_, SuspendAction());
+ EXPECT_CALL(*mock_action_ptr_, SuspendAction());
action_processor_.SuspendProcessing();
// Suspending the processor twice should not suspend the action twice.
action_processor_.SuspendProcessing();
@@ -205,32 +215,31 @@
// IsRunning should return whether there's is an action doing some work, even
// if it is suspended.
EXPECT_TRUE(action_processor_.IsRunning());
- EXPECT_EQ(&mock_action_, action_processor_.current_action());
+ EXPECT_EQ(mock_action_ptr_, action_processor_.current_action());
- EXPECT_CALL(mock_action_, ResumeAction());
+ EXPECT_CALL(*mock_action_ptr_, ResumeAction());
action_processor_.ResumeProcessing();
// Calling ResumeProcessing twice should not affect the action_.
action_processor_.ResumeProcessing();
-
- action_processor_.ActionComplete(&mock_action_, ErrorCode::kSuccess);
+ action_processor_.ActionComplete(mock_action_ptr_, ErrorCode::kSuccess);
}
// This test suspends an action that presumably doesn't support suspend/resume
// and it finished before being resumed.
TEST_F(ActionProcessorTest, ActionCompletedWhileSuspendedTest) {
- action_processor_.EnqueueAction(&mock_action_);
+ action_processor_.EnqueueAction(std::move(mock_action_));
testing::InSequence s;
- EXPECT_CALL(mock_action_, PerformAction());
+ EXPECT_CALL(*mock_action_ptr_, PerformAction());
action_processor_.StartProcessing();
- EXPECT_CALL(mock_action_, SuspendAction());
+ EXPECT_CALL(*mock_action_ptr_, SuspendAction());
action_processor_.SuspendProcessing();
// Simulate the action completion while suspended. No other call to
// |mock_action_| is expected at this point.
- action_processor_.ActionComplete(&mock_action_, ErrorCode::kSuccess);
+ action_processor_.ActionComplete(mock_action_ptr_, ErrorCode::kSuccess);
// The processing should not be done since the ActionProcessor is suspended
// and the processing is considered to be still running until resumed.
@@ -243,15 +252,15 @@
}
TEST_F(ActionProcessorTest, StoppedWhileSuspendedTest) {
- action_processor_.EnqueueAction(&mock_action_);
+ action_processor_.EnqueueAction(std::move(mock_action_));
testing::InSequence s;
- EXPECT_CALL(mock_action_, PerformAction());
+ EXPECT_CALL(*mock_action_ptr_, PerformAction());
action_processor_.StartProcessing();
- EXPECT_CALL(mock_action_, SuspendAction());
+ EXPECT_CALL(*mock_action_ptr_, SuspendAction());
action_processor_.SuspendProcessing();
- EXPECT_CALL(mock_action_, TerminateProcessing());
+ EXPECT_CALL(*mock_action_ptr_, TerminateProcessing());
action_processor_.StopProcessing();
// Stopping the processing should abort the current execution no matter what.
EXPECT_TRUE(delegate_.processing_stopped_called_);
diff --git a/common/action_unittest.cc b/common/action_unittest.cc
index dcdce17..b2f9ba4 100644
--- a/common/action_unittest.cc
+++ b/common/action_unittest.cc
@@ -16,8 +16,11 @@
#include "update_engine/common/action.h"
-#include <gtest/gtest.h>
#include <string>
+#include <utility>
+
+#include <gtest/gtest.h>
+
#include "update_engine/common/action_processor.h"
using std::string;
@@ -56,21 +59,19 @@
// This test creates two simple Actions and sends a message via an ActionPipe
// from one to the other.
TEST(ActionTest, SimpleTest) {
- ActionTestAction action;
-
- EXPECT_FALSE(action.in_pipe());
- EXPECT_FALSE(action.out_pipe());
- EXPECT_FALSE(action.processor());
- EXPECT_FALSE(action.IsRunning());
+ auto action = std::make_unique<ActionTestAction>();
+ auto action_ptr = action.get();
+ EXPECT_FALSE(action->in_pipe());
+ EXPECT_FALSE(action->out_pipe());
+ EXPECT_FALSE(action->processor());
+ EXPECT_FALSE(action->IsRunning());
ActionProcessor action_processor;
- action_processor.EnqueueAction(&action);
- EXPECT_EQ(&action_processor, action.processor());
-
+ action_processor.EnqueueAction(std::move(action));
+ EXPECT_EQ(&action_processor, action_ptr->processor());
action_processor.StartProcessing();
- EXPECT_TRUE(action.IsRunning());
- action.CompleteAction();
- EXPECT_FALSE(action.IsRunning());
+ EXPECT_TRUE(action_ptr->IsRunning());
+ action_ptr->CompleteAction();
}
} // namespace chromeos_update_engine
diff --git a/common/mock_action_processor.h b/common/mock_action_processor.h
index 04275c1..4c62109 100644
--- a/common/mock_action_processor.h
+++ b/common/mock_action_processor.h
@@ -17,6 +17,10 @@
#ifndef UPDATE_ENGINE_COMMON_MOCK_ACTION_PROCESSOR_H_
#define UPDATE_ENGINE_COMMON_MOCK_ACTION_PROCESSOR_H_
+#include <deque>
+#include <memory>
+#include <utility>
+
#include <gmock/gmock.h>
#include "update_engine/common/action.h"
@@ -27,6 +31,12 @@
public:
MOCK_METHOD0(StartProcessing, void());
MOCK_METHOD1(EnqueueAction, void(AbstractAction* action));
+
+ // This is a legacy workaround described in:
+ // https://github.com/google/googletest/blob/master/googlemock/docs/CookBook.md#legacy-workarounds-for-move-only-types-legacymoveonly
+ void EnqueueAction(std::unique_ptr<AbstractAction> action) override {
+ EnqueueAction(action.get());
+ }
};
} // namespace chromeos_update_engine
diff --git a/common/mock_http_fetcher.cc b/common/mock_http_fetcher.cc
index f1ae72a..9507c9d 100644
--- a/common/mock_http_fetcher.cc
+++ b/common/mock_http_fetcher.cc
@@ -47,71 +47,49 @@
SendData(true);
}
-// Returns false on one condition: If timeout_id_ was already set
-// and it needs to be deleted by the caller. If timeout_id_ is null
-// when this function is called, this function will always return true.
-bool MockHttpFetcher::SendData(bool skip_delivery) {
- if (fail_transfer_) {
+void MockHttpFetcher::SendData(bool skip_delivery) {
+ if (fail_transfer_ || sent_size_ == data_.size()) {
SignalTransferComplete();
- return timeout_id_ != MessageLoop::kTaskIdNull;
- }
-
- CHECK_LT(sent_size_, data_.size());
- if (!skip_delivery) {
- const size_t chunk_size = min(kMockHttpFetcherChunkSize,
- data_.size() - sent_size_);
- CHECK(delegate_);
- delegate_->ReceivedBytes(this, &data_[sent_size_], chunk_size);
- // We may get terminated in the callback.
- if (sent_size_ == data_.size()) {
- LOG(INFO) << "Terminated in the ReceivedBytes callback.";
- return timeout_id_ != MessageLoop::kTaskIdNull;
- }
- sent_size_ += chunk_size;
- CHECK_LE(sent_size_, data_.size());
- if (sent_size_ == data_.size()) {
- // We've sent all the data. Notify of success.
- SignalTransferComplete();
- }
+ return;
}
if (paused_) {
- // If we're paused, we should return true if timeout_id_ is set,
- // since we need the caller to delete it.
- return timeout_id_ != MessageLoop::kTaskIdNull;
+ // If we're paused, we should return so no callback is scheduled.
+ return;
}
- if (timeout_id_ != MessageLoop::kTaskIdNull) {
- // we still need a timeout if there's more data to send
- return sent_size_ < data_.size();
- } else if (sent_size_ < data_.size()) {
- // we don't have a timeout source and we need one
+ // Setup timeout callback even if the transfer is about to be completed in
+ // order to get a call to |TransferComplete|.
+ if (timeout_id_ == MessageLoop::kTaskIdNull) {
timeout_id_ = MessageLoop::current()->PostDelayedTask(
FROM_HERE,
base::Bind(&MockHttpFetcher::TimeoutCallback, base::Unretained(this)),
base::TimeDelta::FromMilliseconds(10));
}
- return true;
+
+ if (!skip_delivery) {
+ const size_t chunk_size =
+ min(kMockHttpFetcherChunkSize, data_.size() - sent_size_);
+ sent_size_ += chunk_size;
+ CHECK(delegate_);
+ delegate_->ReceivedBytes(this, &data_[sent_size_ - chunk_size], chunk_size);
+ }
+ // We may get terminated and deleted right after |ReceivedBytes| call, so we
+ // should not access any class member variable after this call.
}
void MockHttpFetcher::TimeoutCallback() {
CHECK(!paused_);
- if (SendData(false)) {
- // We need to re-schedule the timeout.
- timeout_id_ = MessageLoop::current()->PostDelayedTask(
- FROM_HERE,
- base::Bind(&MockHttpFetcher::TimeoutCallback, base::Unretained(this)),
- base::TimeDelta::FromMilliseconds(10));
- } else {
- timeout_id_ = MessageLoop::kTaskIdNull;
- }
+ timeout_id_ = MessageLoop::kTaskIdNull;
+ CHECK_LE(sent_size_, data_.size());
+ // Same here, we should not access any member variable after this call.
+ SendData(false);
}
// If the transfer is in progress, aborts the transfer early.
// The transfer cannot be resumed.
void MockHttpFetcher::TerminateTransfer() {
LOG(INFO) << "Terminating transfer.";
- sent_size_ = data_.size();
// Kill any timeout, it is ok to call with kTaskIdNull.
MessageLoop::current()->CancelTask(timeout_id_);
timeout_id_ = MessageLoop::kTaskIdNull;
@@ -140,9 +118,7 @@
void MockHttpFetcher::Unpause() {
CHECK(paused_) << "You must pause before unpause.";
paused_ = false;
- if (sent_size_ < data_.size()) {
- SendData(false);
- }
+ SendData(false);
}
void MockHttpFetcher::FailTransfer(int http_response_code) {
diff --git a/common/mock_http_fetcher.h b/common/mock_http_fetcher.h
index 367802e..00f4e2b 100644
--- a/common/mock_http_fetcher.h
+++ b/common/mock_http_fetcher.h
@@ -112,13 +112,10 @@
}
private:
- // Sends data to the delegate and sets up a timeout callback if needed.
- // There must be a delegate and there must be data to send. If there is
- // already a timeout callback, and it should be deleted by the caller,
- // this will return false; otherwise true is returned.
- // If skip_delivery is true, no bytes will be delivered, but the callbacks
- // still be set if needed.
- bool SendData(bool skip_delivery);
+ // Sends data to the delegate and sets up a timeout callback if needed. There
+ // must be a delegate. If |skip_delivery| is true, no bytes will be delivered,
+ // but the callbacks still be set if needed.
+ void SendData(bool skip_delivery);
// Callback for when our message loop timeout expires.
void TimeoutCallback();
diff --git a/omaha_request_action.h b/omaha_request_action.h
index 7f526ec..c21bb37 100644
--- a/omaha_request_action.h
+++ b/omaha_request_action.h
@@ -177,6 +177,7 @@
private:
friend class OmahaRequestActionTest;
+ friend class OmahaRequestActionTestProcessorDelegate;
FRIEND_TEST(OmahaRequestActionTest, GetInstallDateWhenNoPrefsNorOOBE);
FRIEND_TEST(OmahaRequestActionTest,
GetInstallDateWhenOOBECompletedWithInvalidDate);
diff --git a/omaha_request_action_unittest.cc b/omaha_request_action_unittest.cc
index 21aecb2..63bb170 100644
--- a/omaha_request_action_unittest.cc
+++ b/omaha_request_action_unittest.cc
@@ -243,6 +243,55 @@
namespace chromeos_update_engine {
+class OmahaRequestActionTestProcessorDelegate : public ActionProcessorDelegate {
+ public:
+ OmahaRequestActionTestProcessorDelegate()
+ : expected_code_(ErrorCode::kSuccess),
+ interactive_(false),
+ test_http_fetcher_headers_(false) {}
+ ~OmahaRequestActionTestProcessorDelegate() override = default;
+
+ void ProcessingDone(const ActionProcessor* processor,
+ ErrorCode code) override {
+ brillo::MessageLoop::current()->BreakLoop();
+ }
+
+ void ActionCompleted(ActionProcessor* processor,
+ AbstractAction* action,
+ ErrorCode code) override {
+ // Make sure actions always succeed.
+ if (action->Type() == OmahaRequestAction::StaticType()) {
+ EXPECT_EQ(expected_code_, code);
+ // Check that the headers were set in the fetcher during the action. Note
+ // that we set this request as "interactive".
+ auto fetcher = static_cast<const MockHttpFetcher*>(
+ static_cast<OmahaRequestAction*>(action)->http_fetcher_.get());
+
+ if (test_http_fetcher_headers_) {
+ EXPECT_EQ(interactive_ ? "fg" : "bg",
+ fetcher->GetHeader("X-Goog-Update-Interactivity"));
+ EXPECT_EQ(kTestAppId, fetcher->GetHeader("X-Goog-Update-AppId"));
+ EXPECT_NE("", fetcher->GetHeader("X-Goog-Update-Updater"));
+ }
+ post_data_ = fetcher->post_data();
+ } else if (action->Type() ==
+ ObjectCollectorAction<OmahaResponse>::StaticType()) {
+ EXPECT_EQ(ErrorCode::kSuccess, code);
+ auto collector_action =
+ static_cast<ObjectCollectorAction<OmahaResponse>*>(action);
+ omaha_response_.reset(new OmahaResponse(collector_action->object()));
+ EXPECT_TRUE(omaha_response_);
+ } else {
+ EXPECT_EQ(ErrorCode::kSuccess, code);
+ }
+ }
+ ErrorCode expected_code_;
+ brillo::Blob post_data_;
+ bool interactive_;
+ bool test_http_fetcher_headers_;
+ std::unique_ptr<OmahaResponse> omaha_response_;
+};
+
class OmahaRequestActionTest : public ::testing::Test {
protected:
void SetUp() override {
@@ -312,6 +361,10 @@
bool is_policy_loaded,
OmahaResponse* out_response);
+ void TestEvent(OmahaEvent* event,
+ const string& http_response,
+ brillo::Blob* out_post_data);
+
// Runs and checks a ping test. |ping_only| indicates whether it should send
// only a ping or also an updatecheck.
void PingTest(bool ping_only);
@@ -339,69 +392,10 @@
OmahaRequestParams request_params_{&fake_system_state_};
FakePrefs fake_prefs_;
-};
-namespace {
-class OmahaRequestActionTestProcessorDelegate : public ActionProcessorDelegate {
- public:
- OmahaRequestActionTestProcessorDelegate()
- : expected_code_(ErrorCode::kSuccess) {}
- ~OmahaRequestActionTestProcessorDelegate() override {
- }
- void ProcessingDone(const ActionProcessor* processor,
- ErrorCode code) override {
- brillo::MessageLoop::current()->BreakLoop();
- }
+ OmahaRequestActionTestProcessorDelegate delegate_;
- void ActionCompleted(ActionProcessor* processor,
- AbstractAction* action,
- ErrorCode code) override {
- // make sure actions always succeed
- if (action->Type() == OmahaRequestAction::StaticType())
- EXPECT_EQ(expected_code_, code);
- else
- EXPECT_EQ(ErrorCode::kSuccess, code);
- }
- ErrorCode expected_code_;
-};
-} // namespace
-
-class OutputObjectCollectorAction;
-
-template<>
-class ActionTraits<OutputObjectCollectorAction> {
- public:
- // Does not take an object for input
- typedef OmahaResponse InputObjectType;
- // On success, puts the output path on output
- typedef NoneType OutputObjectType;
-};
-
-class OutputObjectCollectorAction : public Action<OutputObjectCollectorAction> {
- public:
- OutputObjectCollectorAction() : has_input_object_(false) {}
- void PerformAction() {
- // copy input object
- has_input_object_ = HasInputObject();
- if (has_input_object_)
- omaha_response_ = GetInputObject();
- processor_->ActionComplete(this, ErrorCode::kSuccess);
- }
- // Should never be called
- void TerminateProcessing() {
- CHECK(false);
- }
- // Debugging/logging
- static string StaticType() {
- return "OutputObjectCollectorAction";
- }
- string Type() const { return StaticType(); }
- using InputObjectType =
- ActionTraits<OutputObjectCollectorAction>::InputObjectType;
- using OutputObjectType =
- ActionTraits<OutputObjectCollectorAction>::OutputObjectType;
- bool has_input_object_;
- OmahaResponse omaha_response_;
+ bool test_http_fetcher_headers_{false};
};
bool OmahaRequestActionTest::TestUpdateCheck(
@@ -419,9 +413,8 @@
brillo::Blob* out_post_data) {
brillo::FakeMessageLoop loop(nullptr);
loop.SetAsCurrent();
- MockHttpFetcher* fetcher = new MockHttpFetcher(http_response.data(),
- http_response.size(),
- nullptr);
+ auto fetcher = std::make_unique<MockHttpFetcher>(
+ http_response.data(), http_response.size(), nullptr);
if (fail_http_response_code >= 0) {
fetcher->FailTransfer(fail_http_response_code);
}
@@ -429,10 +422,9 @@
// are not using the default request_params_.
EXPECT_EQ(&request_params_, fake_system_state_.request_params());
- OmahaRequestAction action(&fake_system_state_,
- nullptr,
- base::WrapUnique(fetcher),
- ping_only);
+ auto omaha_request_action = std::make_unique<OmahaRequestAction>(
+ &fake_system_state_, nullptr, std::move(fetcher), ping_only);
+
auto mock_policy_provider =
std::make_unique<NiceMock<policy::MockPolicyProvider>>();
EXPECT_CALL(*mock_policy_provider, IsConsumerDevice())
@@ -449,18 +441,19 @@
EXPECT_CALL(*mock_policy_provider, GetDevicePolicy())
.WillRepeatedly(ReturnRef(device_policy));
+ omaha_request_action->policy_provider_ = std::move(mock_policy_provider);
- action.policy_provider_ = std::move(mock_policy_provider);
- OmahaRequestActionTestProcessorDelegate delegate;
- delegate.expected_code_ = expected_code;
-
+ delegate_.expected_code_ = expected_code;
+ delegate_.interactive_ = request_params_.interactive();
+ delegate_.test_http_fetcher_headers_ = test_http_fetcher_headers_;
ActionProcessor processor;
- processor.set_delegate(&delegate);
- processor.EnqueueAction(&action);
+ processor.set_delegate(&delegate_);
- OutputObjectCollectorAction collector_action;
- BondActions(&action, &collector_action);
- processor.EnqueueAction(&collector_action);
+ auto collector_action =
+ std::make_unique<ObjectCollectorAction<OmahaResponse>>();
+ BondActions(omaha_request_action.get(), collector_action.get());
+ processor.EnqueueAction(std::move(omaha_request_action));
+ processor.EnqueueAction(std::move(collector_action));
EXPECT_CALL(*fake_system_state_.mock_metrics_reporter(),
ReportUpdateCheckMetrics(_, _, _, _))
@@ -478,11 +471,11 @@
base::Unretained(&processor)));
loop.Run();
EXPECT_FALSE(loop.PendingTasks());
- if (collector_action.has_input_object_ && out_response)
- *out_response = collector_action.omaha_response_;
+ if (delegate_.omaha_response_ && out_response)
+ *out_response = *delegate_.omaha_response_;
if (out_post_data)
- *out_post_data = fetcher->post_data();
- return collector_action.has_input_object_;
+ *out_post_data = delegate_.post_data_;
+ return delegate_.omaha_response_ != nullptr;
}
bool OmahaRequestActionTest::TestUpdateCheck(
@@ -529,26 +522,24 @@
ASSERT_TRUE(out_response->update_exists);
}
-// Tests Event requests -- they should always succeed. |out_post_data|
-// may be null; if non-null, the post-data received by the mock
-// HttpFetcher is returned.
-void TestEvent(OmahaEvent* event,
- const string& http_response,
- brillo::Blob* out_post_data) {
+// Tests Event requests -- they should always succeed. |out_post_data| may be
+// null; if non-null, the post-data received by the mock HttpFetcher is
+// returned.
+void OmahaRequestActionTest::TestEvent(OmahaEvent* event,
+ const string& http_response,
+ brillo::Blob* out_post_data) {
brillo::FakeMessageLoop loop(nullptr);
loop.SetAsCurrent();
- MockHttpFetcher* fetcher = new MockHttpFetcher(http_response.data(),
- http_response.size(),
- nullptr);
- FakeSystemState fake_system_state;
- OmahaRequestAction action(&fake_system_state,
- event,
- base::WrapUnique(fetcher),
- false);
- OmahaRequestActionTestProcessorDelegate delegate;
+
+ auto action = std::make_unique<OmahaRequestAction>(
+ &fake_system_state_,
+ event,
+ std::make_unique<MockHttpFetcher>(
+ http_response.data(), http_response.size(), nullptr),
+ false);
ActionProcessor processor;
- processor.set_delegate(&delegate);
- processor.EnqueueAction(&action);
+ processor.set_delegate(&delegate_);
+ processor.EnqueueAction(std::move(action));
loop.PostTask(base::Bind(
[](ActionProcessor* processor) { processor->StartProcessing(); },
@@ -557,7 +548,7 @@
EXPECT_FALSE(loop.PendingTasks());
if (out_post_data)
- *out_post_data = fetcher->post_data();
+ *out_post_data = delegate_.post_data_;
}
TEST_F(OmahaRequestActionTest, RejectEntities) {
@@ -841,31 +832,36 @@
EXPECT_TRUE(response.powerwash_required);
}
-TEST_F(OmahaRequestActionTest, ExtraHeadersSentTest) {
- const string http_response = "<?xml invalid response";
+TEST_F(OmahaRequestActionTest, ExtraHeadersSentInteractiveTest) {
+ OmahaResponse response;
request_params_.set_interactive(true);
+ test_http_fetcher_headers_ = true;
+ ASSERT_FALSE(TestUpdateCheck("invalid xml>",
+ -1,
+ false, // ping_only
+ ErrorCode::kOmahaRequestXMLParseError,
+ metrics::CheckResult::kParsingError,
+ metrics::CheckReaction::kUnset,
+ metrics::DownloadErrorCode::kUnset,
+ &response,
+ nullptr));
+ EXPECT_FALSE(response.update_exists);
+}
- brillo::FakeMessageLoop loop(nullptr);
- loop.SetAsCurrent();
-
- MockHttpFetcher* fetcher =
- new MockHttpFetcher(http_response.data(), http_response.size(), nullptr);
- OmahaRequestAction action(&fake_system_state_, nullptr,
- base::WrapUnique(fetcher), false);
- ActionProcessor processor;
- processor.EnqueueAction(&action);
-
- loop.PostTask(base::Bind(
- [](ActionProcessor* processor) { processor->StartProcessing(); },
- base::Unretained(&processor)));
- loop.Run();
- EXPECT_FALSE(loop.PendingTasks());
-
- // Check that the headers were set in the fetcher during the action. Note that
- // we set this request as "interactive".
- EXPECT_EQ("fg", fetcher->GetHeader("X-Goog-Update-Interactivity"));
- EXPECT_EQ(kTestAppId, fetcher->GetHeader("X-Goog-Update-AppId"));
- EXPECT_NE("", fetcher->GetHeader("X-Goog-Update-Updater"));
+TEST_F(OmahaRequestActionTest, ExtraHeadersSentNoInteractiveTest) {
+ OmahaResponse response;
+ request_params_.set_interactive(false);
+ test_http_fetcher_headers_ = true;
+ ASSERT_FALSE(TestUpdateCheck("invalid xml>",
+ -1,
+ false, // ping_only
+ ErrorCode::kOmahaRequestXMLParseError,
+ metrics::CheckResult::kParsingError,
+ metrics::CheckReaction::kUnset,
+ metrics::DownloadErrorCode::kUnset,
+ &response,
+ nullptr));
+ EXPECT_FALSE(response.update_exists);
}
TEST_F(OmahaRequestActionTest, ValidUpdateBlockedByConnection) {
@@ -1483,17 +1479,15 @@
brillo::FakeMessageLoop loop(nullptr);
loop.SetAsCurrent();
- OmahaRequestAction action(
+ auto action = std::make_unique<OmahaRequestAction>(
&fake_system_state_,
nullptr,
- std::make_unique<MockHttpFetcher>(http_response.data(),
- http_response.size(),
- nullptr),
+ std::make_unique<MockHttpFetcher>(
+ http_response.data(), http_response.size(), nullptr),
false);
- OmahaRequestActionTestProcessorDelegate delegate;
ActionProcessor processor;
- processor.set_delegate(&delegate);
- processor.EnqueueAction(&action);
+ processor.set_delegate(&delegate_);
+ processor.EnqueueAction(std::move(action));
loop.PostTask(base::Bind(
[](ActionProcessor* processor) { processor->StartProcessing(); },
@@ -1652,17 +1646,16 @@
loop.SetAsCurrent();
string http_response("doesn't matter");
- OmahaRequestAction action(
+ auto action = std::make_unique<OmahaRequestAction>(
&fake_system_state_,
nullptr,
- std::make_unique<MockHttpFetcher>(http_response.data(),
- http_response.size(),
- nullptr),
+ std::make_unique<MockHttpFetcher>(
+ http_response.data(), http_response.size(), nullptr),
false);
TerminateEarlyTestProcessorDelegate delegate;
ActionProcessor processor;
processor.set_delegate(&delegate);
- processor.EnqueueAction(&action);
+ processor.EnqueueAction(std::move(action));
loop.PostTask(base::Bind(&TerminateTransferTestStarter, &processor));
loop.Run();
diff --git a/omaha_response_handler_action.cc b/omaha_response_handler_action.cc
index 3a8439d..7e6da5d 100644
--- a/omaha_response_handler_action.cc
+++ b/omaha_response_handler_action.cc
@@ -44,14 +44,8 @@
OmahaResponseHandlerAction::OmahaResponseHandlerAction(
SystemState* system_state)
- : OmahaResponseHandlerAction(system_state,
- constants::kOmahaResponseDeadlineFile) {}
-
-OmahaResponseHandlerAction::OmahaResponseHandlerAction(
- SystemState* system_state, const string& deadline_file)
: system_state_(system_state),
- key_path_(constants::kUpdatePayloadPublicKeyPath),
- deadline_file_(deadline_file) {}
+ deadline_file_(constants::kOmahaResponseDeadlineFile) {}
void OmahaResponseHandlerAction::PerformAction() {
CHECK(HasInputObject());
diff --git a/omaha_response_handler_action.h b/omaha_response_handler_action.h
index 0a001b8..344fc1d 100644
--- a/omaha_response_handler_action.h
+++ b/omaha_response_handler_action.h
@@ -59,7 +59,6 @@
// Debugging/logging
static std::string StaticType() { return "OmahaResponseHandlerAction"; }
std::string Type() const override { return StaticType(); }
- void set_key_path(const std::string& path) { key_path_ = path; }
private:
// Returns true if payload hash checks are mandatory based on the state
@@ -72,18 +71,11 @@
// The install plan, if we have an update.
InstallPlan install_plan_;
- // Public key path to use for payload verification.
- std::string key_path_;
-
// File used for communication deadline to Chrome.
- const std::string deadline_file_;
-
- // Special ctor + friend declarations for testing purposes.
- OmahaResponseHandlerAction(SystemState* system_state,
- const std::string& deadline_file);
+ std::string deadline_file_;
friend class OmahaResponseHandlerActionTest;
-
+ friend class OmahaResponseHandlerActionProcessorDelegate;
FRIEND_TEST(UpdateAttempterTest, CreatePendingErrorEventResumedTest);
FRIEND_TEST(UpdateAttempterTest, RollbackMetricsNotRollbackFailure);
FRIEND_TEST(UpdateAttempterTest, RollbackMetricsNotRollbackSuccess);
diff --git a/omaha_response_handler_action_unittest.cc b/omaha_response_handler_action_unittest.cc
index c598c89..bdec86f 100644
--- a/omaha_response_handler_action_unittest.cc
+++ b/omaha_response_handler_action_unittest.cc
@@ -18,6 +18,7 @@
#include <memory>
#include <string>
+#include <utility>
#include <base/files/file_util.h>
#include <base/files/scoped_temp_dir.h>
@@ -46,6 +47,35 @@
namespace chromeos_update_engine {
+class OmahaResponseHandlerActionProcessorDelegate
+ : public ActionProcessorDelegate {
+ public:
+ OmahaResponseHandlerActionProcessorDelegate()
+ : code_(ErrorCode::kError), code_set_(false) {}
+ void ActionCompleted(ActionProcessor* processor,
+ AbstractAction* action,
+ ErrorCode code) {
+ if (action->Type() == OmahaResponseHandlerAction::StaticType()) {
+ auto response_handler_action =
+ static_cast<OmahaResponseHandlerAction*>(action);
+ code_ = code;
+ code_set_ = true;
+ response_handler_action_install_plan_.reset(
+ new InstallPlan(response_handler_action->install_plan_));
+ } else if (action->Type() ==
+ ObjectCollectorAction<InstallPlan>::StaticType()) {
+ auto collector_action =
+ static_cast<ObjectCollectorAction<InstallPlan>*>(action);
+ collector_action_install_plan_.reset(
+ new InstallPlan(collector_action->object()));
+ }
+ }
+ ErrorCode code_;
+ bool code_set_;
+ std::unique_ptr<InstallPlan> collector_action_install_plan_;
+ std::unique_ptr<InstallPlan> response_handler_action_install_plan_;
+};
+
class OmahaResponseHandlerActionTest : public ::testing::Test {
protected:
void SetUp() override {
@@ -66,9 +96,9 @@
const string& deadline_file,
InstallPlan* out);
- // Pointer to the Action, valid after |DoTest|, released when the test is
- // finished.
- std::unique_ptr<OmahaResponseHandlerAction> action_;
+ // Delegate passed to the ActionProcessor.
+ OmahaResponseHandlerActionProcessorDelegate delegate_;
+
// Captures the action's result code, for tests that need to directly verify
// it in non-success cases.
ErrorCode action_result_code_;
@@ -78,23 +108,6 @@
const brillo::Blob expected_hash_ = {0x48, 0x61, 0x73, 0x68, 0x2b};
};
-class OmahaResponseHandlerActionProcessorDelegate
- : public ActionProcessorDelegate {
- public:
- OmahaResponseHandlerActionProcessorDelegate()
- : code_(ErrorCode::kError), code_set_(false) {}
- void ActionCompleted(ActionProcessor* processor,
- AbstractAction* action,
- ErrorCode code) {
- if (action->Type() == OmahaResponseHandlerAction::StaticType()) {
- code_ = code;
- code_set_ = true;
- }
- }
- ErrorCode code_;
- bool code_set_;
-};
-
namespace {
const char* const kLongName =
"very_long_name_and_no_slashes-very_long_name_and_no_slashes"
@@ -115,11 +128,10 @@
brillo::FakeMessageLoop loop(nullptr);
loop.SetAsCurrent();
ActionProcessor processor;
- OmahaResponseHandlerActionProcessorDelegate delegate;
- processor.set_delegate(&delegate);
+ processor.set_delegate(&delegate_);
- ObjectFeederAction<OmahaResponse> feeder_action;
- feeder_action.set_obj(in);
+ auto feeder_action = std::make_unique<ObjectFeederAction<OmahaResponse>>();
+ feeder_action->set_obj(in);
if (in.update_exists && in.version != kBadVersion) {
string expected_hash;
for (const auto& package : in.packages)
@@ -138,24 +150,29 @@
EXPECT_CALL(*(fake_system_state_.mock_payload_state()), GetCurrentUrl())
.WillRepeatedly(Return(current_url));
- action_.reset(new OmahaResponseHandlerAction(
- &fake_system_state_,
- (test_deadline_file.empty() ? constants::kOmahaResponseDeadlineFile
- : test_deadline_file)));
- BondActions(&feeder_action, action_.get());
- ObjectCollectorAction<InstallPlan> collector_action;
- BondActions(action_.get(), &collector_action);
- processor.EnqueueAction(&feeder_action);
- processor.EnqueueAction(action_.get());
- processor.EnqueueAction(&collector_action);
+ auto response_handler_action =
+ std::make_unique<OmahaResponseHandlerAction>(&fake_system_state_);
+ if (!test_deadline_file.empty())
+ response_handler_action->deadline_file_ = test_deadline_file;
+
+ auto collector_action =
+ std::make_unique<ObjectCollectorAction<InstallPlan>>();
+
+ BondActions(feeder_action.get(), response_handler_action.get());
+ BondActions(response_handler_action.get(), collector_action.get());
+ processor.EnqueueAction(std::move(feeder_action));
+ processor.EnqueueAction(std::move(response_handler_action));
+ processor.EnqueueAction(std::move(collector_action));
processor.StartProcessing();
EXPECT_TRUE(!processor.IsRunning())
<< "Update test to handle non-async actions";
- if (out)
- *out = collector_action.object();
- EXPECT_TRUE(delegate.code_set_);
- action_result_code_ = delegate.code_;
- return delegate.code_ == ErrorCode::kSuccess;
+
+ if (out && delegate_.collector_action_install_plan_)
+ *out = *delegate_.collector_action_install_plan_;
+
+ EXPECT_TRUE(delegate_.code_set_);
+ action_result_code_ = delegate_.code_;
+ return delegate_.code_ == ErrorCode::kSuccess;
}
TEST_F(OmahaResponseHandlerActionTest, SimpleTest) {
@@ -645,9 +662,8 @@
EXPECT_EQ(ErrorCode::kOmahaUpdateDeferredPerPolicy, action_result_code_);
// Verify that DoTest() didn't set the output install plan.
EXPECT_EQ("", install_plan.version);
- // Copy the underlying InstallPlan from the Action (like a real Delegate).
- install_plan = action_->install_plan();
// Now verify the InstallPlan that was generated.
+ install_plan = *delegate_.response_handler_action_install_plan_;
EXPECT_EQ(in.packages[0].payload_urls[0], install_plan.download_url);
EXPECT_EQ(expected_hash_, install_plan.payloads[0].hash);
EXPECT_EQ(1U, install_plan.target_slot);
diff --git a/payload_consumer/download_action_unittest.cc b/payload_consumer/download_action_unittest.cc
index a3ad5b9..6d2a9f0 100644
--- a/payload_consumer/download_action_unittest.cc
+++ b/payload_consumer/download_action_unittest.cc
@@ -65,9 +65,8 @@
class DownloadActionTestProcessorDelegate : public ActionProcessorDelegate {
public:
- explicit DownloadActionTestProcessorDelegate(ErrorCode expected_code)
- : processing_done_called_(false),
- expected_code_(expected_code) {}
+ DownloadActionTestProcessorDelegate()
+ : processing_done_called_(false), expected_code_(ErrorCode::kSuccess) {}
~DownloadActionTestProcessorDelegate() override {
EXPECT_TRUE(processing_done_called_);
}
@@ -91,6 +90,7 @@
const string type = action->Type();
if (type == DownloadAction::StaticType()) {
EXPECT_EQ(expected_code_, code);
+ p2p_file_id_ = static_cast<DownloadAction*>(action)->p2p_file_id();
} else {
EXPECT_EQ(ErrorCode::kSuccess, code);
}
@@ -100,6 +100,7 @@
brillo::Blob expected_data_;
bool processing_done_called_;
ErrorCode expected_code_;
+ string p2p_file_id_;
};
class TestDirectFileWriter : public DirectFileWriter {
@@ -155,25 +156,26 @@
install_plan.source_slot, true);
fake_system_state.fake_boot_control()->SetSlotBootable(
install_plan.target_slot, true);
- ObjectFeederAction<InstallPlan> feeder_action;
- feeder_action.set_obj(install_plan);
+ auto feeder_action = std::make_unique<ObjectFeederAction<InstallPlan>>();
+ feeder_action->set_obj(install_plan);
MockPrefs prefs;
MockHttpFetcher* http_fetcher = new MockHttpFetcher(data.data(),
data.size(),
nullptr);
// takes ownership of passed in HttpFetcher
- DownloadAction download_action(&prefs,
- fake_system_state.boot_control(),
- fake_system_state.hardware(),
- &fake_system_state,
- http_fetcher,
- false /* interactive */);
- download_action.SetTestFileWriter(&writer);
- BondActions(&feeder_action, &download_action);
+ auto download_action =
+ std::make_unique<DownloadAction>(&prefs,
+ fake_system_state.boot_control(),
+ fake_system_state.hardware(),
+ &fake_system_state,
+ http_fetcher,
+ false /* interactive */);
+ download_action->SetTestFileWriter(&writer);
+ BondActions(feeder_action.get(), download_action.get());
MockDownloadActionDelegate download_delegate;
if (use_download_delegate) {
InSequence s;
- download_action.set_delegate(&download_delegate);
+ download_action->set_delegate(&download_delegate);
if (data.size() > kMockHttpFetcherChunkSize)
EXPECT_CALL(download_delegate,
BytesReceived(_, kMockHttpFetcherChunkSize, _));
@@ -181,16 +183,15 @@
EXPECT_CALL(download_delegate, DownloadComplete())
.Times(fail_write == 0 ? 1 : 0);
}
- ErrorCode expected_code = ErrorCode::kSuccess;
- if (fail_write > 0)
- expected_code = ErrorCode::kDownloadWriteError;
- DownloadActionTestProcessorDelegate delegate(expected_code);
+ DownloadActionTestProcessorDelegate delegate;
+ delegate.expected_code_ =
+ (fail_write > 0) ? ErrorCode::kDownloadWriteError : ErrorCode::kSuccess;
delegate.expected_data_ = brillo::Blob(data.begin() + 1, data.end());
delegate.path_ = output_temp_file.path();
ActionProcessor processor;
processor.set_delegate(&delegate);
- processor.EnqueueAction(&feeder_action);
- processor.EnqueueAction(&download_action);
+ processor.EnqueueAction(std::move(feeder_action));
+ processor.EnqueueAction(std::move(download_action));
loop.PostTask(FROM_HERE,
base::Bind(&StartProcessorInRunLoop, &processor, http_fetcher));
@@ -272,24 +273,25 @@
{.size = size, .type = InstallPayloadType::kFull});
total_expected_download_size += size;
}
- ObjectFeederAction<InstallPlan> feeder_action;
- feeder_action.set_obj(install_plan);
+ auto feeder_action = std::make_unique<ObjectFeederAction<InstallPlan>>();
+ feeder_action->set_obj(install_plan);
MockPrefs prefs;
MockHttpFetcher* http_fetcher = new MockHttpFetcher(
payload_datas[0].data(), payload_datas[0].size(), nullptr);
// takes ownership of passed in HttpFetcher
- DownloadAction download_action(&prefs,
- fake_system_state.boot_control(),
- fake_system_state.hardware(),
- &fake_system_state,
- http_fetcher,
- false /* interactive */);
- download_action.SetTestFileWriter(&mock_file_writer);
- BondActions(&feeder_action, &download_action);
+ auto download_action =
+ std::make_unique<DownloadAction>(&prefs,
+ fake_system_state.boot_control(),
+ fake_system_state.hardware(),
+ &fake_system_state,
+ http_fetcher,
+ false /* interactive */);
+ download_action->SetTestFileWriter(&mock_file_writer);
+ BondActions(feeder_action.get(), download_action.get());
MockDownloadActionDelegate download_delegate;
{
InSequence s;
- download_action.set_delegate(&download_delegate);
+ download_action->set_delegate(&download_delegate);
// these are hand-computed based on the payloads specified above
EXPECT_CALL(download_delegate,
BytesReceived(kMockHttpFetcherChunkSize,
@@ -321,8 +323,8 @@
total_expected_download_size));
}
ActionProcessor processor;
- processor.EnqueueAction(&feeder_action);
- processor.EnqueueAction(&download_action);
+ processor.EnqueueAction(std::move(feeder_action));
+ processor.EnqueueAction(std::move(download_action));
loop.PostTask(
FROM_HERE,
@@ -361,31 +363,31 @@
EXPECT_EQ(0, writer.Open(temp_file.path().c_str(), O_WRONLY | O_CREAT, 0));
// takes ownership of passed in HttpFetcher
- ObjectFeederAction<InstallPlan> feeder_action;
+ auto feeder_action = std::make_unique<ObjectFeederAction<InstallPlan>>();
InstallPlan install_plan;
install_plan.payloads.resize(1);
- feeder_action.set_obj(install_plan);
+ feeder_action->set_obj(install_plan);
FakeSystemState fake_system_state_;
MockPrefs prefs;
- DownloadAction download_action(
+ auto download_action = std::make_unique<DownloadAction>(
&prefs,
fake_system_state_.boot_control(),
fake_system_state_.hardware(),
&fake_system_state_,
new MockHttpFetcher(data.data(), data.size(), nullptr),
false /* interactive */);
- download_action.SetTestFileWriter(&writer);
+ download_action->SetTestFileWriter(&writer);
MockDownloadActionDelegate download_delegate;
if (use_download_delegate) {
- download_action.set_delegate(&download_delegate);
+ download_action->set_delegate(&download_delegate);
EXPECT_CALL(download_delegate, BytesReceived(_, _, _)).Times(0);
}
TerminateEarlyTestProcessorDelegate delegate;
ActionProcessor processor;
processor.set_delegate(&delegate);
- processor.EnqueueAction(&feeder_action);
- processor.EnqueueAction(&download_action);
- BondActions(&feeder_action, &download_action);
+ BondActions(feeder_action.get(), download_action.get());
+ processor.EnqueueAction(std::move(feeder_action));
+ processor.EnqueueAction(std::move(download_action));
loop.PostTask(FROM_HERE,
base::Bind(&TerminateEarlyTestStarter, &processor));
@@ -423,22 +425,21 @@
// This is a simple Action class for testing.
class DownloadActionTestAction : public Action<DownloadActionTestAction> {
public:
- DownloadActionTestAction() : did_run_(false) {}
+ DownloadActionTestAction() = default;
typedef InstallPlan InputObjectType;
typedef InstallPlan OutputObjectType;
ActionPipe<InstallPlan>* in_pipe() { return in_pipe_.get(); }
ActionPipe<InstallPlan>* out_pipe() { return out_pipe_.get(); }
ActionProcessor* processor() { return processor_; }
void PerformAction() {
- did_run_ = true;
ASSERT_TRUE(HasInputObject());
EXPECT_TRUE(expected_input_object_ == GetInputObject());
ASSERT_TRUE(processor());
processor()->ActionComplete(this, ErrorCode::kSuccess);
}
- string Type() const { return "DownloadActionTestAction"; }
+ static std::string StaticType() { return "DownloadActionTestAction"; }
+ string Type() const { return StaticType(); }
InstallPlan expected_input_object_;
- bool did_run_;
};
namespace {
@@ -447,9 +448,19 @@
// only by the test PassObjectOutTest.
class PassObjectOutTestProcessorDelegate : public ActionProcessorDelegate {
public:
- void ProcessingDone(const ActionProcessor* processor, ErrorCode code) {
+ void ProcessingDone(const ActionProcessor* processor,
+ ErrorCode code) override {
brillo::MessageLoop::current()->BreakLoop();
}
+ void ActionCompleted(ActionProcessor* processor,
+ AbstractAction* action,
+ ErrorCode code) override {
+ if (action->Type() == DownloadActionTestAction::StaticType()) {
+ did_test_action_run_ = true;
+ }
+ }
+
+ bool did_test_action_run_ = false;
};
} // namespace
@@ -466,29 +477,30 @@
install_plan.payloads.push_back({.size = 1});
EXPECT_TRUE(
HashCalculator::RawHashOfData({'x'}, &install_plan.payloads[0].hash));
- ObjectFeederAction<InstallPlan> feeder_action;
- feeder_action.set_obj(install_plan);
+ auto feeder_action = std::make_unique<ObjectFeederAction<InstallPlan>>();
+ feeder_action->set_obj(install_plan);
MockPrefs prefs;
FakeSystemState fake_system_state_;
- DownloadAction download_action(&prefs,
- fake_system_state_.boot_control(),
- fake_system_state_.hardware(),
- &fake_system_state_,
- new MockHttpFetcher("x", 1, nullptr),
- false /* interactive */);
- download_action.SetTestFileWriter(&writer);
+ auto download_action =
+ std::make_unique<DownloadAction>(&prefs,
+ fake_system_state_.boot_control(),
+ fake_system_state_.hardware(),
+ &fake_system_state_,
+ new MockHttpFetcher("x", 1, nullptr),
+ false /* interactive */);
+ download_action->SetTestFileWriter(&writer);
- DownloadActionTestAction test_action;
- test_action.expected_input_object_ = install_plan;
- BondActions(&feeder_action, &download_action);
- BondActions(&download_action, &test_action);
+ auto test_action = std::make_unique<DownloadActionTestAction>();
+ test_action->expected_input_object_ = install_plan;
+ BondActions(feeder_action.get(), download_action.get());
+ BondActions(download_action.get(), test_action.get());
ActionProcessor processor;
PassObjectOutTestProcessorDelegate delegate;
processor.set_delegate(&delegate);
- processor.EnqueueAction(&feeder_action);
- processor.EnqueueAction(&download_action);
- processor.EnqueueAction(&test_action);
+ processor.EnqueueAction(std::move(feeder_action));
+ processor.EnqueueAction(std::move(download_action));
+ processor.EnqueueAction(std::move(test_action));
loop.PostTask(
FROM_HERE,
@@ -498,7 +510,7 @@
loop.Run();
EXPECT_FALSE(loop.PendingTasks());
- EXPECT_EQ(true, test_action.did_run_);
+ EXPECT_EQ(true, delegate.did_test_action_run_);
}
// Test fixture for P2P tests.
@@ -553,43 +565,44 @@
install_plan.payloads.push_back(
{.size = data_.length(),
.hash = {'1', '2', '3', '4', 'h', 'a', 's', 'h'}});
- ObjectFeederAction<InstallPlan> feeder_action;
- feeder_action.set_obj(install_plan);
+ auto feeder_action = std::make_unique<ObjectFeederAction<InstallPlan>>();
+ feeder_action->set_obj(install_plan);
MockPrefs prefs;
- http_fetcher_ = new MockHttpFetcher(data_.c_str(),
- data_.length(),
- nullptr);
// Note that DownloadAction takes ownership of the passed in HttpFetcher.
- download_action_.reset(new DownloadAction(&prefs,
- fake_system_state_.boot_control(),
- fake_system_state_.hardware(),
- &fake_system_state_,
- http_fetcher_,
- false /* interactive */));
- download_action_->SetTestFileWriter(&writer);
- BondActions(&feeder_action, download_action_.get());
- DownloadActionTestProcessorDelegate delegate(ErrorCode::kSuccess);
- delegate.expected_data_ = brillo::Blob(data_.begin() + start_at_offset_,
- data_.end());
- delegate.path_ = output_temp_file.path();
- processor_.set_delegate(&delegate);
- processor_.EnqueueAction(&feeder_action);
- processor_.EnqueueAction(download_action_.get());
+ auto download_action = std::make_unique<DownloadAction>(
+ &prefs,
+ fake_system_state_.boot_control(),
+ fake_system_state_.hardware(),
+ &fake_system_state_,
+ new MockHttpFetcher(data_.c_str(), data_.length(), nullptr),
+ false /* interactive */);
+ auto http_fetcher = download_action->http_fetcher();
+ download_action->SetTestFileWriter(&writer);
+ BondActions(feeder_action.get(), download_action.get());
+ delegate_.expected_data_ =
+ brillo::Blob(data_.begin() + start_at_offset_, data_.end());
+ delegate_.path_ = output_temp_file.path();
+ processor_.set_delegate(&delegate_);
+ processor_.EnqueueAction(std::move(feeder_action));
+ processor_.EnqueueAction(std::move(download_action));
- loop_.PostTask(FROM_HERE, base::Bind(
- &P2PDownloadActionTest::StartProcessorInRunLoopForP2P,
- base::Unretained(this)));
+ loop_.PostTask(
+ FROM_HERE,
+ base::Bind(
+ [](P2PDownloadActionTest* action_test, HttpFetcher* http_fetcher) {
+ action_test->processor_.StartProcessing();
+ http_fetcher->SetOffset(action_test->start_at_offset_);
+ },
+ base::Unretained(this),
+ base::Unretained(http_fetcher)));
loop_.Run();
}
// Mainloop used to make StartDownload() synchronous.
brillo::FakeMessageLoop loop_{nullptr};
- // The DownloadAction instance under test.
- unique_ptr<DownloadAction> download_action_;
-
- // The HttpFetcher used in the test.
- MockHttpFetcher* http_fetcher_;
+ // Delegate that is passed to the ActionProcessor.
+ DownloadActionTestProcessorDelegate delegate_;
// The P2PManager used in the test.
unique_ptr<P2PManager> p2p_manager_;
@@ -604,12 +617,6 @@
string data_;
private:
- // Callback used in StartDownload() method.
- void StartProcessorInRunLoopForP2P() {
- processor_.StartProcessing();
- download_action_->http_fetcher()->SetOffset(start_at_offset_);
- }
-
// The requested starting offset passed to SetupDownload().
off_t start_at_offset_;
@@ -627,7 +634,7 @@
StartDownload(true); // use_p2p_to_share
// Check the p2p file and its content matches what was sent.
- string file_id = download_action_->p2p_file_id();
+ string file_id = delegate_.p2p_file_id_;
EXPECT_NE("", file_id);
EXPECT_EQ(static_cast<int>(data_.length()),
p2p_manager_->FileGetSize(file_id));
@@ -651,7 +658,7 @@
// DownloadAction should convey that the file is not being shared.
// and that we don't have any p2p files.
- EXPECT_EQ(download_action_->p2p_file_id(), "");
+ EXPECT_EQ(delegate_.p2p_file_id_, "");
EXPECT_EQ(p2p_manager_->CountSharedFiles(), 0);
}
@@ -679,7 +686,7 @@
// DownloadAction should convey the same file_id and the file should
// have the expected size.
- EXPECT_EQ(download_action_->p2p_file_id(), file_id);
+ EXPECT_EQ(delegate_.p2p_file_id_, file_id);
EXPECT_EQ(static_cast<ssize_t>(data_.length()),
p2p_manager_->FileGetSize(file_id));
EXPECT_EQ(static_cast<ssize_t>(data_.length()),
diff --git a/payload_consumer/filesystem_verifier_action_unittest.cc b/payload_consumer/filesystem_verifier_action_unittest.cc
index 2e1d95d..db8b664 100644
--- a/payload_consumer/filesystem_verifier_action_unittest.cc
+++ b/payload_consumer/filesystem_verifier_action_unittest.cc
@@ -18,8 +18,10 @@
#include <fcntl.h>
+#include <memory>
#include <set>
#include <string>
+#include <utility>
#include <vector>
#include <base/bind.h>
@@ -90,27 +92,24 @@
if (action->Type() == FilesystemVerifierAction::StaticType()) {
ran_ = true;
code_ = code;
+ } else if (action->Type() ==
+ ObjectCollectorAction<InstallPlan>::StaticType()) {
+ auto collector_action =
+ static_cast<ObjectCollectorAction<InstallPlan>*>(action);
+ install_plan_.reset(new InstallPlan(collector_action->object()));
}
}
bool ran() const { return ran_; }
ErrorCode code() const { return code_; }
+ std::unique_ptr<InstallPlan> install_plan_;
+
private:
FilesystemVerifierAction* action_;
bool ran_;
ErrorCode code_;
};
-void StartProcessorInRunLoop(ActionProcessor* processor,
- FilesystemVerifierAction* filesystem_copier_action,
- bool terminate_early) {
- processor->StartProcessing();
- if (terminate_early) {
- EXPECT_NE(nullptr, filesystem_copier_action);
- processor->StopProcessing();
- }
-}
-
bool FilesystemVerifierActionTest::DoTest(bool terminate_early,
bool hash_fail) {
string a_loop_file;
@@ -165,27 +164,32 @@
}
install_plan.partitions = {part};
+ auto feeder_action = std::make_unique<ObjectFeederAction<InstallPlan>>();
+ feeder_action->set_obj(install_plan);
+ auto copier_action = std::make_unique<FilesystemVerifierAction>();
+ auto collector_action =
+ std::make_unique<ObjectCollectorAction<InstallPlan>>();
+
+ BondActions(feeder_action.get(), copier_action.get());
+ BondActions(copier_action.get(), collector_action.get());
+
ActionProcessor processor;
-
- ObjectFeederAction<InstallPlan> feeder_action;
- FilesystemVerifierAction copier_action;
- ObjectCollectorAction<InstallPlan> collector_action;
-
- BondActions(&feeder_action, &copier_action);
- BondActions(&copier_action, &collector_action);
-
- FilesystemVerifierActionTestDelegate delegate(&copier_action);
+ FilesystemVerifierActionTestDelegate delegate(copier_action.get());
processor.set_delegate(&delegate);
- processor.EnqueueAction(&feeder_action);
- processor.EnqueueAction(&copier_action);
- processor.EnqueueAction(&collector_action);
+ processor.EnqueueAction(std::move(feeder_action));
+ processor.EnqueueAction(std::move(copier_action));
+ processor.EnqueueAction(std::move(collector_action));
- feeder_action.set_obj(install_plan);
-
- loop_.PostTask(FROM_HERE, base::Bind(&StartProcessorInRunLoop,
- &processor,
- &copier_action,
- terminate_early));
+ loop_.PostTask(FROM_HERE,
+ base::Bind(
+ [](ActionProcessor* processor, bool terminate_early) {
+ processor->StartProcessing();
+ if (terminate_early) {
+ processor->StopProcessing();
+ }
+ },
+ base::Unretained(&processor),
+ terminate_early));
loop_.Run();
if (!terminate_early) {
@@ -214,7 +218,7 @@
EXPECT_TRUE(is_a_file_reading_eq);
success = success && is_a_file_reading_eq;
- bool is_install_plan_eq = (collector_action.object() == install_plan);
+ bool is_install_plan_eq = (*delegate.install_plan_ == install_plan);
EXPECT_TRUE(is_install_plan_eq);
success = success && is_install_plan_eq;
return success;
@@ -240,13 +244,14 @@
processor.set_delegate(&delegate);
- FilesystemVerifierAction copier_action;
- ObjectCollectorAction<InstallPlan> collector_action;
+ auto copier_action = std::make_unique<FilesystemVerifierAction>();
+ auto collector_action =
+ std::make_unique<ObjectCollectorAction<InstallPlan>>();
- BondActions(&copier_action, &collector_action);
+ BondActions(copier_action.get(), collector_action.get());
- processor.EnqueueAction(&copier_action);
- processor.EnqueueAction(&collector_action);
+ processor.EnqueueAction(std::move(copier_action));
+ processor.EnqueueAction(std::move(collector_action));
processor.StartProcessing();
EXPECT_FALSE(processor.IsRunning());
EXPECT_TRUE(delegate.ran_);
@@ -259,7 +264,6 @@
processor.set_delegate(&delegate);
- ObjectFeederAction<InstallPlan> feeder_action;
InstallPlan install_plan;
InstallPlan::Partition part;
part.name = "nope";
@@ -267,15 +271,18 @@
part.target_path = "/no/such/file";
install_plan.partitions = {part};
- feeder_action.set_obj(install_plan);
- FilesystemVerifierAction verifier_action;
- ObjectCollectorAction<InstallPlan> collector_action;
+ auto feeder_action = std::make_unique<ObjectFeederAction<InstallPlan>>();
+ auto verifier_action = std::make_unique<FilesystemVerifierAction>();
+ auto collector_action =
+ std::make_unique<ObjectCollectorAction<InstallPlan>>();
- BondActions(&verifier_action, &collector_action);
+ feeder_action->set_obj(install_plan);
- processor.EnqueueAction(&feeder_action);
- processor.EnqueueAction(&verifier_action);
- processor.EnqueueAction(&collector_action);
+ BondActions(verifier_action.get(), collector_action.get());
+
+ processor.EnqueueAction(std::move(feeder_action));
+ processor.EnqueueAction(std::move(verifier_action));
+ processor.EnqueueAction(std::move(collector_action));
processor.StartProcessing();
EXPECT_FALSE(processor.IsRunning());
EXPECT_TRUE(delegate.ran_);
diff --git a/payload_consumer/postinstall_runner_action_unittest.cc b/payload_consumer/postinstall_runner_action_unittest.cc
index b7f0fdc..3c606c1 100644
--- a/payload_consumer/postinstall_runner_action_unittest.cc
+++ b/payload_consumer/postinstall_runner_action_unittest.cc
@@ -22,6 +22,7 @@
#include <memory>
#include <string>
+#include <utility>
#include <base/bind.h>
#include <base/files/file_util.h>
@@ -172,7 +173,7 @@
bool is_rollback) {
ActionProcessor processor;
processor_ = &processor;
- ObjectFeederAction<InstallPlan> feeder_action;
+ auto feeder_action = std::make_unique<ObjectFeederAction<InstallPlan>>();
InstallPlan::Partition part;
part.name = "part";
part.target_path = device_path;
@@ -183,16 +184,18 @@
install_plan.download_url = "http://127.0.0.1:8080/update";
install_plan.powerwash_required = powerwash_required;
install_plan.is_rollback = is_rollback;
- feeder_action.set_obj(install_plan);
- PostinstallRunnerAction runner_action(&fake_boot_control_, &fake_hardware_);
- postinstall_action_ = &runner_action;
- runner_action.set_delegate(setup_action_delegate_);
- BondActions(&feeder_action, &runner_action);
- ObjectCollectorAction<InstallPlan> collector_action;
- BondActions(&runner_action, &collector_action);
- processor.EnqueueAction(&feeder_action);
- processor.EnqueueAction(&runner_action);
- processor.EnqueueAction(&collector_action);
+ feeder_action->set_obj(install_plan);
+ auto runner_action = std::make_unique<PostinstallRunnerAction>(
+ &fake_boot_control_, &fake_hardware_);
+ postinstall_action_ = runner_action.get();
+ runner_action->set_delegate(setup_action_delegate_);
+ BondActions(feeder_action.get(), runner_action.get());
+ auto collector_action =
+ std::make_unique<ObjectCollectorAction<InstallPlan>>();
+ BondActions(runner_action.get(), collector_action.get());
+ processor.EnqueueAction(std::move(feeder_action));
+ processor.EnqueueAction(std::move(runner_action));
+ processor.EnqueueAction(std::move(collector_action));
processor.set_delegate(&processor_delegate_);
loop_.PostTask(
diff --git a/update_attempter.cc b/update_attempter.cc
index 82acfe8..4f45a2a 100644
--- a/update_attempter.cc
+++ b/update_attempter.cc
@@ -582,98 +582,77 @@
omaha_request_params_->waiting_period());
}
-void UpdateAttempter::BuildPostInstallActions(
- InstallPlanAction* previous_action) {
- shared_ptr<PostinstallRunnerAction> postinstall_runner_action(
- new PostinstallRunnerAction(system_state_->boot_control(),
- system_state_->hardware()));
- postinstall_runner_action->set_delegate(this);
- actions_.push_back(shared_ptr<AbstractAction>(postinstall_runner_action));
- BondActions(previous_action,
- postinstall_runner_action.get());
-}
-
void UpdateAttempter::BuildUpdateActions(bool interactive) {
CHECK(!processor_->IsRunning());
processor_->set_delegate(this);
// Actions:
- std::unique_ptr<LibcurlHttpFetcher> update_check_fetcher(
- new LibcurlHttpFetcher(GetProxyResolver(), system_state_->hardware()));
+ auto update_check_fetcher = std::make_unique<LibcurlHttpFetcher>(
+ GetProxyResolver(), system_state_->hardware());
update_check_fetcher->set_server_to_check(ServerToCheck::kUpdate);
// Try harder to connect to the network, esp when not interactive.
// See comment in libcurl_http_fetcher.cc.
update_check_fetcher->set_no_network_max_retries(interactive ? 1 : 3);
- shared_ptr<OmahaRequestAction> update_check_action(
- new OmahaRequestAction(system_state_,
- nullptr,
- std::move(update_check_fetcher),
- false));
- shared_ptr<OmahaResponseHandlerAction> response_handler_action(
- new OmahaResponseHandlerAction(system_state_));
-
- shared_ptr<OmahaRequestAction> download_started_action(new OmahaRequestAction(
+ auto update_check_action = std::make_unique<OmahaRequestAction>(
+ system_state_, nullptr, std::move(update_check_fetcher), false);
+ auto response_handler_action =
+ std::make_unique<OmahaResponseHandlerAction>(system_state_);
+ auto download_started_action = std::make_unique<OmahaRequestAction>(
system_state_,
new OmahaEvent(OmahaEvent::kTypeUpdateDownloadStarted),
std::make_unique<LibcurlHttpFetcher>(GetProxyResolver(),
system_state_->hardware()),
- false));
+ false);
LibcurlHttpFetcher* download_fetcher =
new LibcurlHttpFetcher(GetProxyResolver(), system_state_->hardware());
download_fetcher->set_server_to_check(ServerToCheck::kDownload);
if (interactive)
download_fetcher->set_max_retry_count(kDownloadMaxRetryCountInteractive);
- shared_ptr<DownloadAction> download_action(
- new DownloadAction(prefs_,
- system_state_->boot_control(),
- system_state_->hardware(),
- system_state_,
- download_fetcher, // passes ownership
- interactive));
- shared_ptr<OmahaRequestAction> download_finished_action(
- new OmahaRequestAction(
- system_state_,
- new OmahaEvent(OmahaEvent::kTypeUpdateDownloadFinished),
- std::make_unique<LibcurlHttpFetcher>(GetProxyResolver(),
- system_state_->hardware()),
- false));
- shared_ptr<FilesystemVerifierAction> filesystem_verifier_action(
- new FilesystemVerifierAction());
- shared_ptr<OmahaRequestAction> update_complete_action(
- new OmahaRequestAction(system_state_,
- new OmahaEvent(OmahaEvent::kTypeUpdateComplete),
- std::make_unique<LibcurlHttpFetcher>(
- GetProxyResolver(), system_state_->hardware()),
- false));
-
+ auto download_action =
+ std::make_unique<DownloadAction>(prefs_,
+ system_state_->boot_control(),
+ system_state_->hardware(),
+ system_state_,
+ download_fetcher, // passes ownership
+ interactive);
download_action->set_delegate(this);
- response_handler_action_ = response_handler_action;
- download_action_ = download_action;
- actions_.push_back(shared_ptr<AbstractAction>(update_check_action));
- actions_.push_back(shared_ptr<AbstractAction>(response_handler_action));
- actions_.push_back(shared_ptr<AbstractAction>(download_started_action));
- actions_.push_back(shared_ptr<AbstractAction>(download_action));
- actions_.push_back(shared_ptr<AbstractAction>(download_finished_action));
- actions_.push_back(shared_ptr<AbstractAction>(filesystem_verifier_action));
+ auto download_finished_action = std::make_unique<OmahaRequestAction>(
+ system_state_,
+ new OmahaEvent(OmahaEvent::kTypeUpdateDownloadFinished),
+ std::make_unique<LibcurlHttpFetcher>(GetProxyResolver(),
+ system_state_->hardware()),
+ false);
+ auto filesystem_verifier_action =
+ std::make_unique<FilesystemVerifierAction>();
+ auto update_complete_action = std::make_unique<OmahaRequestAction>(
+ system_state_,
+ new OmahaEvent(OmahaEvent::kTypeUpdateComplete),
+ std::make_unique<LibcurlHttpFetcher>(GetProxyResolver(),
+ system_state_->hardware()),
+ false);
+
+ auto postinstall_runner_action = std::make_unique<PostinstallRunnerAction>(
+ system_state_->boot_control(), system_state_->hardware());
+ postinstall_runner_action->set_delegate(this);
// Bond them together. We have to use the leaf-types when calling
// BondActions().
- BondActions(update_check_action.get(),
- response_handler_action.get());
- BondActions(response_handler_action.get(),
- download_action.get());
- BondActions(download_action.get(),
- filesystem_verifier_action.get());
- BuildPostInstallActions(filesystem_verifier_action.get());
+ BondActions(update_check_action.get(), response_handler_action.get());
+ BondActions(response_handler_action.get(), download_action.get());
+ BondActions(download_action.get(), filesystem_verifier_action.get());
+ BondActions(filesystem_verifier_action.get(),
+ postinstall_runner_action.get());
- actions_.push_back(shared_ptr<AbstractAction>(update_complete_action));
-
- // Enqueue the actions
- for (const shared_ptr<AbstractAction>& action : actions_) {
- processor_->EnqueueAction(action.get());
- }
+ processor_->EnqueueAction(std::move(update_check_action));
+ processor_->EnqueueAction(std::move(response_handler_action));
+ processor_->EnqueueAction(std::move(download_started_action));
+ processor_->EnqueueAction(std::move(download_action));
+ processor_->EnqueueAction(std::move(download_finished_action));
+ processor_->EnqueueAction(std::move(filesystem_verifier_action));
+ processor_->EnqueueAction(std::move(postinstall_runner_action));
+ processor_->EnqueueAction(std::move(update_complete_action));
}
bool UpdateAttempter::Rollback(bool powerwash) {
@@ -704,28 +683,25 @@
}
LOG(INFO) << "Setting rollback options.";
- InstallPlan install_plan;
-
- install_plan.target_slot = GetRollbackSlot();
- install_plan.source_slot = system_state_->boot_control()->GetCurrentSlot();
+ install_plan_.reset(new InstallPlan());
+ install_plan_->target_slot = GetRollbackSlot();
+ install_plan_->source_slot = system_state_->boot_control()->GetCurrentSlot();
TEST_AND_RETURN_FALSE(
- install_plan.LoadPartitionsFromSlots(system_state_->boot_control()));
- install_plan.powerwash_required = powerwash;
+ install_plan_->LoadPartitionsFromSlots(system_state_->boot_control()));
+ install_plan_->powerwash_required = powerwash;
LOG(INFO) << "Using this install plan:";
- install_plan.Dump();
+ install_plan_->Dump();
- shared_ptr<InstallPlanAction> install_plan_action(
- new InstallPlanAction(install_plan));
- actions_.push_back(shared_ptr<AbstractAction>(install_plan_action));
-
- BuildPostInstallActions(install_plan_action.get());
-
- // Enqueue the actions
- for (const shared_ptr<AbstractAction>& action : actions_) {
- processor_->EnqueueAction(action.get());
- }
+ auto install_plan_action =
+ std::make_unique<InstallPlanAction>(*install_plan_);
+ auto postinstall_runner_action = std::make_unique<PostinstallRunnerAction>(
+ system_state_->boot_control(), system_state_->hardware());
+ postinstall_runner_action->set_delegate(this);
+ BondActions(install_plan_action.get(), postinstall_runner_action.get());
+ processor_->EnqueueAction(std::move(install_plan_action));
+ processor_->EnqueueAction(std::move(postinstall_runner_action));
// Update the payload state for Rollback.
system_state_->payload_state()->Rollback();
@@ -939,7 +915,6 @@
void UpdateAttempter::ProcessingDone(const ActionProcessor* processor,
ErrorCode code) {
LOG(INFO) << "Processing Done.";
- actions_.clear();
// Reset cpu shares back to normal.
cpu_limiter_.StopLimiter();
@@ -989,15 +964,12 @@
ScheduleUpdates();
LOG(INFO) << "Update successfully applied, waiting to reboot.";
- // This pointer is null during rollback operations, and the stats
- // don't make much sense then anyway.
- if (response_handler_action_) {
- const InstallPlan& install_plan =
- response_handler_action_->install_plan();
-
+ // |install_plan_| is null during rollback operations, and the stats don't
+ // make much sense then anyway.
+ if (install_plan_) {
// Generate an unique payload identifier.
string target_version_uid;
- for (const auto& payload : install_plan.payloads) {
+ for (const auto& payload : install_plan_->payloads) {
target_version_uid +=
brillo::data_encoding::Base64Encode(payload.hash) + ":" +
payload.metadata_signature + ":";
@@ -1005,10 +977,10 @@
// If we just downloaded a rollback image, we should preserve this fact
// over the following powerwash.
- if (install_plan.is_rollback) {
+ if (install_plan_->is_rollback) {
system_state_->payload_state()->SetRollbackHappened(true);
system_state_->metrics_reporter()->ReportEnterpriseRollbackMetrics(
- /*success=*/true, install_plan.version);
+ /*success=*/true, install_plan_->version);
}
// Expect to reboot into the new version to send the proper metric during
@@ -1019,8 +991,7 @@
// If we just finished a rollback, then we expect to have no Omaha
// response. Otherwise, it's an error.
if (system_state_->payload_state()->GetRollbackVersion().empty()) {
- LOG(ERROR) << "Can't send metrics because expected "
- "response_handler_action_ missing.";
+ LOG(ERROR) << "Can't send metrics because there was no Omaha response";
}
}
return;
@@ -1040,7 +1011,6 @@
download_progress_ = 0.0;
SetStatusAndNotify(UpdateStatus::IDLE);
ScheduleUpdates();
- actions_.clear();
error_event_.reset(nullptr);
}
@@ -1100,13 +1070,15 @@
// callback is invoked. This avoids notifying the user that a download
// has started in cases when the server and the client are unable to
// initiate the download.
- CHECK(action == response_handler_action_.get());
- auto plan = response_handler_action_->install_plan();
+ auto omaha_response_handler_action =
+ static_cast<OmahaResponseHandlerAction*>(action);
+ install_plan_.reset(
+ new InstallPlan(omaha_response_handler_action->install_plan()));
UpdateLastCheckedTime();
- new_version_ = plan.version;
- new_system_version_ = plan.system_version;
+ new_version_ = install_plan_->version;
+ new_system_version_ = install_plan_->system_version;
new_payload_size_ = 0;
- for (const auto& payload : plan.payloads)
+ for (const auto& payload : install_plan_->payloads)
new_payload_size_ += payload.size;
cpu_limiter_.StartLimiter();
SetStatusAndNotify(UpdateStatus::UPDATE_AVAILABLE);
@@ -1300,8 +1272,7 @@
if (!system_state_->hardware()->IsNormalBootMode())
flags |= static_cast<uint32_t>(ErrorCode::kDevModeFlag);
- if (response_handler_action_.get() &&
- response_handler_action_->install_plan().is_resume)
+ if (install_plan_ && install_plan_->is_resume)
flags |= static_cast<uint32_t>(ErrorCode::kResumedFlag);
if (!system_state_->hardware()->IsOfficialBuild())
@@ -1380,24 +1351,20 @@
system_state_->payload_state()->UpdateFailed(error_event_->error_code);
// Send metrics if it was a rollback.
- if (response_handler_action_) {
- const InstallPlan& install_plan = response_handler_action_->install_plan();
- if (install_plan.is_rollback) {
- system_state_->metrics_reporter()->ReportEnterpriseRollbackMetrics(
- /*success=*/false, install_plan.version);
- }
+ if (install_plan_ && install_plan_->is_rollback) {
+ system_state_->metrics_reporter()->ReportEnterpriseRollbackMetrics(
+ /*success=*/false, install_plan_->version);
}
// Send it to Omaha.
LOG(INFO) << "Reporting the error event";
- shared_ptr<OmahaRequestAction> error_event_action(
- new OmahaRequestAction(system_state_,
- error_event_.release(), // Pass ownership.
- std::make_unique<LibcurlHttpFetcher>(
- GetProxyResolver(), system_state_->hardware()),
- false));
- actions_.push_back(shared_ptr<AbstractAction>(error_event_action));
- processor_->EnqueueAction(error_event_action.get());
+ auto error_event_action = std::make_unique<OmahaRequestAction>(
+ system_state_,
+ error_event_.release(), // Pass ownership.
+ std::make_unique<LibcurlHttpFetcher>(GetProxyResolver(),
+ system_state_->hardware()),
+ false);
+ processor_->EnqueueAction(std::move(error_event_action));
SetStatusAndNotify(UpdateStatus::REPORTING_ERROR_EVENT);
processor_->StartProcessing();
return true;
@@ -1435,15 +1402,14 @@
void UpdateAttempter::PingOmaha() {
if (!processor_->IsRunning()) {
- shared_ptr<OmahaRequestAction> ping_action(new OmahaRequestAction(
+ auto ping_action = std::make_unique<OmahaRequestAction>(
system_state_,
nullptr,
std::make_unique<LibcurlHttpFetcher>(GetProxyResolver(),
system_state_->hardware()),
- true));
- actions_.push_back(shared_ptr<OmahaRequestAction>(ping_action));
+ true);
processor_->set_delegate(nullptr);
- processor_->EnqueueAction(ping_action.get());
+ processor_->EnqueueAction(std::move(ping_action));
// Call StartProcessing() synchronously here to avoid any race conditions
// caused by multiple outstanding ping Omaha requests. If we call
// StartProcessing() asynchronously, the device can be suspended before we
diff --git a/update_attempter.h b/update_attempter.h
index 44255b0..324f549 100644
--- a/update_attempter.h
+++ b/update_attempter.h
@@ -362,12 +362,6 @@
// scatter factor value specified from policy.
void GenerateNewWaitingPeriod();
- // Helper method of Update() and Rollback() to construct the sequence of
- // actions to be performed for the postinstall.
- // |previous_action| is the previous action to get
- // bonded with the install_plan that gets passed to postinstall.
- void BuildPostInstallActions(InstallPlanAction* previous_action);
-
// Helper method of Update() to construct the sequence of actions to
// be performed for an update check. Please refer to
// Update() method for the meaning of the parameters.
@@ -421,7 +415,6 @@
// set back in the middle of an update.
base::TimeTicks last_notify_time_;
- std::vector<std::shared_ptr<AbstractAction>> actions_;
std::unique_ptr<ActionProcessor> processor_;
// External state of the system outside the update_engine process
@@ -434,11 +427,8 @@
// The list of services observing changes in the updater.
std::set<ServiceObserverInterface*> service_observers_;
- // Pointer to the OmahaResponseHandlerAction in the actions_ vector.
- std::shared_ptr<OmahaResponseHandlerAction> response_handler_action_;
-
- // Pointer to the DownloadAction in the actions_ vector.
- std::shared_ptr<DownloadAction> download_action_;
+ // The install plan.
+ std::unique_ptr<InstallPlan> install_plan_;
// Pointer to the preferences store interface. This is just a cached
// copy of system_state->prefs() because it's used in many methods and
diff --git a/update_attempter_unittest.cc b/update_attempter_unittest.cc
index eceb02b..4af5f8b 100644
--- a/update_attempter_unittest.cc
+++ b/update_attempter_unittest.cc
@@ -62,6 +62,7 @@
using testing::InSequence;
using testing::Ne;
using testing::NiceMock;
+using testing::Pointee;
using testing::Property;
using testing::Return;
using testing::ReturnPointee;
@@ -426,8 +427,8 @@
TEST_F(UpdateAttempterTest, ScheduleErrorEventActionTest) {
EXPECT_CALL(*processor_,
- EnqueueAction(Property(&AbstractAction::Type,
- OmahaRequestAction::StaticType())));
+ EnqueueAction(Pointee(Property(
+ &AbstractAction::Type, OmahaRequestAction::StaticType()))));
EXPECT_CALL(*processor_, StartProcessing());
ErrorCode err = ErrorCode::kError;
EXPECT_CALL(*fake_system_state_.mock_payload_state(), UpdateFailed(err));
@@ -474,8 +475,8 @@
InSequence s;
for (size_t i = 0; i < arraysize(kUpdateActionTypes); ++i) {
EXPECT_CALL(*processor_,
- EnqueueAction(Property(&AbstractAction::Type,
- kUpdateActionTypes[i])));
+ EnqueueAction(Pointee(
+ Property(&AbstractAction::Type, kUpdateActionTypes[i]))));
}
EXPECT_CALL(*processor_, StartProcessing());
}
@@ -489,17 +490,6 @@
void UpdateAttempterTest::UpdateTestVerify() {
EXPECT_EQ(0, attempter_.http_response_code());
EXPECT_EQ(&attempter_, processor_->delegate());
- EXPECT_EQ(arraysize(kUpdateActionTypes), attempter_.actions_.size());
- for (size_t i = 0; i < arraysize(kUpdateActionTypes); ++i) {
- EXPECT_EQ(kUpdateActionTypes[i], attempter_.actions_[i]->Type());
- }
- EXPECT_EQ(attempter_.response_handler_action_.get(),
- attempter_.actions_[1].get());
- AbstractAction* action_3 = attempter_.actions_[3].get();
- ASSERT_NE(nullptr, action_3);
- ASSERT_EQ(DownloadAction::StaticType(), action_3->Type());
- DownloadAction* download_action = static_cast<DownloadAction*>(action_3);
- EXPECT_EQ(&attempter_, download_action->delegate());
EXPECT_EQ(UpdateStatus::CHECKING_FOR_UPDATE, attempter_.status());
loop_.BreakLoop();
}
@@ -545,8 +535,8 @@
InSequence s;
for (size_t i = 0; i < arraysize(kRollbackActionTypes); ++i) {
EXPECT_CALL(*processor_,
- EnqueueAction(Property(&AbstractAction::Type,
- kRollbackActionTypes[i])));
+ EnqueueAction(Pointee(Property(&AbstractAction::Type,
+ kRollbackActionTypes[i]))));
}
EXPECT_CALL(*processor_, StartProcessing());
@@ -563,19 +553,9 @@
void UpdateAttempterTest::RollbackTestVerify() {
// Verifies the actions that were enqueued.
EXPECT_EQ(&attempter_, processor_->delegate());
- EXPECT_EQ(arraysize(kRollbackActionTypes), attempter_.actions_.size());
- for (size_t i = 0; i < arraysize(kRollbackActionTypes); ++i) {
- EXPECT_EQ(kRollbackActionTypes[i], attempter_.actions_[i]->Type());
- }
EXPECT_EQ(UpdateStatus::ATTEMPTING_ROLLBACK, attempter_.status());
- AbstractAction* action_0 = attempter_.actions_[0].get();
- ASSERT_NE(nullptr, action_0);
- ASSERT_EQ(InstallPlanAction::StaticType(), action_0->Type());
- InstallPlanAction* install_plan_action =
- static_cast<InstallPlanAction*>(action_0);
- InstallPlan* install_plan = install_plan_action->install_plan();
- EXPECT_EQ(0U, install_plan->partitions.size());
- EXPECT_EQ(install_plan->powerwash_required, true);
+ EXPECT_EQ(0U, attempter_.install_plan_->partitions.size());
+ EXPECT_EQ(attempter_.install_plan_->powerwash_required, true);
loop_.BreakLoop();
}
@@ -610,8 +590,8 @@
void UpdateAttempterTest::PingOmahaTestStart() {
EXPECT_CALL(*processor_,
- EnqueueAction(Property(&AbstractAction::Type,
- OmahaRequestAction::StaticType())));
+ EnqueueAction(Pointee(Property(
+ &AbstractAction::Type, OmahaRequestAction::StaticType()))));
EXPECT_CALL(*processor_, StartProcessing());
attempter_.PingOmaha();
ScheduleQuitMainLoop();
@@ -645,10 +625,8 @@
}
TEST_F(UpdateAttempterTest, CreatePendingErrorEventResumedTest) {
- OmahaResponseHandlerAction *response_action =
- new OmahaResponseHandlerAction(&fake_system_state_);
- response_action->install_plan_.is_resume = true;
- attempter_.response_handler_action_.reset(response_action);
+ attempter_.install_plan_.reset(new InstallPlan);
+ attempter_.install_plan_->is_resume = true;
MockAction action;
const ErrorCode kCode = ErrorCode::kInstallDeviceOpenError;
attempter_.CreatePendingErrorEvent(&action, kCode);
@@ -1097,13 +1075,11 @@
TEST_F(UpdateAttempterTest, UpdateDeferredByPolicyTest) {
// Construct an OmahaResponseHandlerAction that has processed an InstallPlan,
// but the update is being deferred by the Policy.
- OmahaResponseHandlerAction* response_action =
- new OmahaResponseHandlerAction(&fake_system_state_);
+ auto response_action = new OmahaResponseHandlerAction(&fake_system_state_);
response_action->install_plan_.version = "a.b.c.d";
response_action->install_plan_.system_version = "b.c.d.e";
response_action->install_plan_.payloads.push_back(
{.size = 1234ULL, .type = InstallPayloadType::kFull});
- attempter_.response_handler_action_.reset(response_action);
// Inform the UpdateAttempter that the OmahaResponseHandlerAction has
// completed, with the deferred-update error code.
attempter_.ActionCompleted(
@@ -1112,10 +1088,11 @@
UpdateEngineStatus status;
attempter_.GetStatus(&status);
EXPECT_EQ(UpdateStatus::UPDATE_AVAILABLE, status.status);
- EXPECT_EQ(response_action->install_plan_.version, status.new_version);
- EXPECT_EQ(response_action->install_plan_.system_version,
+ EXPECT_TRUE(attempter_.install_plan_);
+ EXPECT_EQ(attempter_.install_plan_->version, status.new_version);
+ EXPECT_EQ(attempter_.install_plan_->system_version,
status.new_system_version);
- EXPECT_EQ(response_action->install_plan_.payloads[0].size,
+ EXPECT_EQ(attempter_.install_plan_->payloads[0].size,
status.new_size_bytes);
}
// An "error" event should have been created to tell Omaha that the update is
@@ -1247,10 +1224,8 @@
}
TEST_F(UpdateAttempterTest, SetRollbackHappenedRollback) {
- OmahaResponseHandlerAction* response_action =
- new OmahaResponseHandlerAction(&fake_system_state_);
- response_action->install_plan_.is_rollback = true;
- attempter_.response_handler_action_.reset(response_action);
+ attempter_.install_plan_.reset(new InstallPlan);
+ attempter_.install_plan_->is_rollback = true;
EXPECT_CALL(*fake_system_state_.mock_payload_state(),
SetRollbackHappened(true))
@@ -1259,10 +1234,8 @@
}
TEST_F(UpdateAttempterTest, SetRollbackHappenedNotRollback) {
- OmahaResponseHandlerAction* response_action =
- new OmahaResponseHandlerAction(&fake_system_state_);
- response_action->install_plan_.is_rollback = false;
- attempter_.response_handler_action_.reset(response_action);
+ attempter_.install_plan_.reset(new InstallPlan);
+ attempter_.install_plan_->is_rollback = false;
EXPECT_CALL(*fake_system_state_.mock_payload_state(),
SetRollbackHappened(true))
@@ -1271,11 +1244,9 @@
}
TEST_F(UpdateAttempterTest, RollbackMetricsRollbackSuccess) {
- OmahaResponseHandlerAction* response_action =
- new OmahaResponseHandlerAction(&fake_system_state_);
- response_action->install_plan_.is_rollback = true;
- response_action->install_plan_.version = kRollbackVersion;
- attempter_.response_handler_action_.reset(response_action);
+ attempter_.install_plan_.reset(new InstallPlan);
+ attempter_.install_plan_->is_rollback = true;
+ attempter_.install_plan_->version = kRollbackVersion;
EXPECT_CALL(*fake_system_state_.mock_metrics_reporter(),
ReportEnterpriseRollbackMetrics(true, kRollbackVersion))
@@ -1284,11 +1255,9 @@
}
TEST_F(UpdateAttempterTest, RollbackMetricsNotRollbackSuccess) {
- OmahaResponseHandlerAction* response_action =
- new OmahaResponseHandlerAction(&fake_system_state_);
- response_action->install_plan_.is_rollback = false;
- response_action->install_plan_.version = kRollbackVersion;
- attempter_.response_handler_action_.reset(response_action);
+ attempter_.install_plan_.reset(new InstallPlan);
+ attempter_.install_plan_->is_rollback = false;
+ attempter_.install_plan_->version = kRollbackVersion;
EXPECT_CALL(*fake_system_state_.mock_metrics_reporter(),
ReportEnterpriseRollbackMetrics(_, _))
@@ -1297,11 +1266,9 @@
}
TEST_F(UpdateAttempterTest, RollbackMetricsRollbackFailure) {
- OmahaResponseHandlerAction* response_action =
- new OmahaResponseHandlerAction(&fake_system_state_);
- response_action->install_plan_.is_rollback = true;
- response_action->install_plan_.version = kRollbackVersion;
- attempter_.response_handler_action_.reset(response_action);
+ attempter_.install_plan_.reset(new InstallPlan);
+ attempter_.install_plan_->is_rollback = true;
+ attempter_.install_plan_->version = kRollbackVersion;
EXPECT_CALL(*fake_system_state_.mock_metrics_reporter(),
ReportEnterpriseRollbackMetrics(false, kRollbackVersion))
@@ -1312,11 +1279,9 @@
}
TEST_F(UpdateAttempterTest, RollbackMetricsNotRollbackFailure) {
- OmahaResponseHandlerAction* response_action =
- new OmahaResponseHandlerAction(&fake_system_state_);
- response_action->install_plan_.is_rollback = false;
- response_action->install_plan_.version = kRollbackVersion;
- attempter_.response_handler_action_.reset(response_action);
+ attempter_.install_plan_.reset(new InstallPlan);
+ attempter_.install_plan_->is_rollback = false;
+ attempter_.install_plan_->version = kRollbackVersion;
EXPECT_CALL(*fake_system_state_.mock_metrics_reporter(),
ReportEnterpriseRollbackMetrics(_, _))
