PolicyManager: Remove unnecessary Init() methods.
The PolicyManager::Init() method always return true as it can't
really fail. This patch removes it and simplifies the interface
on the FakePolicyManager to make it easier to use it on Policy
unit testing exposing the FakeState there.
BUG=chromium:358269
TEST=Build and unittests.
Change-Id: Ib27dd41a483b10f164810e18585a8e4b4cb92f5a
Reviewed-on: https://chromium-review.googlesource.com/196968
Tested-by: Alex Deymo <deymo@chromium.org>
Reviewed-by: Gilad Arnold <garnold@chromium.org>
Commit-Queue: Alex Deymo <deymo@chromium.org>
diff --git a/fake_system_state.cc b/fake_system_state.cc
index 8c73aa1..6777838 100644
--- a/fake_system_state.cc
+++ b/fake_system_state.cc
@@ -32,7 +32,6 @@
fake_system_rebooted_(false) {
mock_payload_state_.Initialize(this);
mock_update_attempter_.Init();
- fake_policy_manager_.Init(new FakeState());
}
} // namespace chromeos_update_engine
diff --git a/policy_manager/fake_policy_manager.h b/policy_manager/fake_policy_manager.h
index 2b6ea3a..83c575c 100644
--- a/policy_manager/fake_policy_manager.h
+++ b/policy_manager/fake_policy_manager.h
@@ -8,17 +8,25 @@
#include "update_engine/policy_manager/policy_manager.h"
#include "update_engine/policy_manager/default_policy.h"
+#include "update_engine/policy_manager/fake_state.h"
namespace chromeos_policy_manager {
class FakePolicyManager : public PolicyManager {
public:
explicit FakePolicyManager(chromeos_update_engine::ClockInterface* clock)
- : PolicyManager(clock) {
+ : PolicyManager(clock, new FakeState()) {
// The FakePolicyManager uses a DefaultPolicy.
set_policy(new DefaultPolicy());
}
+ // PolicyManager overrides.
+ using PolicyManager::set_policy;
+
+ FakeState* state() {
+ return reinterpret_cast<FakeState*>(PolicyManager::state());
+ }
+
private:
DISALLOW_COPY_AND_ASSIGN(FakePolicyManager);
};
diff --git a/policy_manager/policy_manager.cc b/policy_manager/policy_manager.cc
index c8e7d4e..71f0290 100644
--- a/policy_manager/policy_manager.cc
+++ b/policy_manager/policy_manager.cc
@@ -11,20 +11,12 @@
namespace chromeos_policy_manager {
-template <typename T>
-bool InitProvider(scoped_ptr<T>* handle_ptr, T* provider) {
- handle_ptr->reset(provider);
- return handle_ptr->get() && (*handle_ptr)->Init();
-}
-
-bool PolicyManager::Init(State* state) {
+PolicyManager::PolicyManager(chromeos_update_engine::ClockInterface* clock,
+ State* state)
+ : state_(state), clock_(clock) {
// TODO(deymo): Make it possible to replace this policy with a different
// implementation with a build-time flag.
policy_.reset(new ChromeOSPolicy());
-
- state_.reset(state);
-
- return true;
}
} // namespace chromeos_policy_manager
diff --git a/policy_manager/policy_manager.h b/policy_manager/policy_manager.h
index 75e6082..99ca26b 100644
--- a/policy_manager/policy_manager.h
+++ b/policy_manager/policy_manager.h
@@ -19,13 +19,12 @@
// The main Policy Manager singleton class.
class PolicyManager {
public:
- explicit PolicyManager(chromeos_update_engine::ClockInterface* clock)
- : clock_(clock) {}
+ // Creates the PolicyManager instance, assuming ownership on the provided
+ // |state|.
+ PolicyManager(chromeos_update_engine::ClockInterface* clock,
+ State* state);
- // Initializes the PolicyManager instance, assuming ownership on the provided
- // |state|, which is assumed to be pre-initialized. Returns whether the
- // initialization succeeded.
- bool Init(State* state);
+ virtual ~PolicyManager() {}
// PolicyRequest() evaluates the given policy with the provided arguments and
// returns the result. The |policy_method| is the pointer-to-method of the
@@ -76,6 +75,9 @@
policy_.reset(policy);
}
+ // State getter used for testing.
+ State* state() { return state_.get(); }
+
private:
FRIEND_TEST(PmPolicyManagerTest, PolicyRequestCallsPolicy);
FRIEND_TEST(PmPolicyManagerTest, PolicyRequestCallsDefaultOnError);
diff --git a/policy_manager/policy_manager_unittest.cc b/policy_manager/policy_manager_unittest.cc
index 9b14a00..b1f5562 100644
--- a/policy_manager/policy_manager_unittest.cc
+++ b/policy_manager/policy_manager_unittest.cc
@@ -8,6 +8,7 @@
#include <vector>
#include <base/bind.h>
+#include <base/memory/scoped_ptr.h>
#include <gmock/gmock.h>
#include <gtest/gtest.h>
@@ -32,18 +33,15 @@
namespace chromeos_policy_manager {
class PmPolicyManagerTest : public ::testing::Test {
- public:
- PmPolicyManagerTest() : pmut_(&fake_clock_) {}
-
protected:
virtual void SetUp() {
fake_state_ = new FakeState();
- EXPECT_TRUE(pmut_.Init(fake_state_));
+ pmut_.reset(new PolicyManager(&fake_clock_, fake_state_));
}
- FakeState* fake_state_;
+ FakeState* fake_state_; // Owned by the pmut_.
FakeClock fake_clock_;
- PolicyManager pmut_;
+ scoped_ptr<PolicyManager> pmut_;
};
// The FailingPolicy implements a single method and make it always fail. This
@@ -79,38 +77,42 @@
TEST_F(PmPolicyManagerTest, PolicyRequestCall) {
bool result;
- EvalStatus status = pmut_.PolicyRequest(&Policy::UpdateCheckAllowed, &result);
+ EvalStatus status = pmut_->PolicyRequest(
+ &Policy::UpdateCheckAllowed, &result);
EXPECT_EQ(status, EvalStatus::kSucceeded);
}
TEST_F(PmPolicyManagerTest, PolicyRequestCallsPolicy) {
StrictMock<MockPolicy>* policy = new StrictMock<MockPolicy>();
- pmut_.policy_.reset(policy);
+ pmut_->set_policy(policy);
bool result;
// Tests that the method is called on the policy_ instance.
EXPECT_CALL(*policy, UpdateCheckAllowed(_, _, _, _))
.WillOnce(Return(EvalStatus::kSucceeded));
- EvalStatus status = pmut_.PolicyRequest(&Policy::UpdateCheckAllowed, &result);
+ EvalStatus status = pmut_->PolicyRequest(
+ &Policy::UpdateCheckAllowed, &result);
EXPECT_EQ(status, EvalStatus::kSucceeded);
}
TEST_F(PmPolicyManagerTest, PolicyRequestCallsDefaultOnError) {
- pmut_.policy_.reset(new FailingPolicy());
+ pmut_->set_policy(new FailingPolicy());
// Tests that the DefaultPolicy instance is called when the method fails,
// which will set this as true.
bool result = false;
- EvalStatus status = pmut_.PolicyRequest(&Policy::UpdateCheckAllowed, &result);
+ EvalStatus status = pmut_->PolicyRequest(
+ &Policy::UpdateCheckAllowed, &result);
EXPECT_EQ(status, EvalStatus::kSucceeded);
EXPECT_TRUE(result);
}
TEST_F(PmPolicyManagerTest, PolicyRequestDoesntBlock) {
- pmut_.policy_.reset(new LazyPolicy());
+ pmut_->set_policy(new LazyPolicy());
bool result;
- EvalStatus status = pmut_.PolicyRequest(&Policy::UpdateCheckAllowed, &result);
+ EvalStatus status = pmut_->PolicyRequest(
+ &Policy::UpdateCheckAllowed, &result);
EXPECT_EQ(status, EvalStatus::kAskMeAgainLater);
}
@@ -119,13 +121,13 @@
// call on a policy that returns AskMeAgainLater the first time and one that
// succeeds the first time, we ensure that the passed callback is called from
// the main loop in both cases even when we could evaluate it right now.
- pmut_.policy_.reset(new FailingPolicy());
+ pmut_->set_policy(new FailingPolicy());
vector<pair<EvalStatus, bool>> calls;
Callback<void(EvalStatus, const bool& result)> callback =
Bind(AccumulateCallsCallback<bool>, &calls);
- pmut_.AsyncPolicyRequest(callback, &Policy::UpdateCheckAllowed);
+ pmut_->AsyncPolicyRequest(callback, &Policy::UpdateCheckAllowed);
// The callback should wait until we run the main loop for it to be executed.
EXPECT_EQ(0, calls.size());
chromeos_update_engine::RunGMainLoopMaxIterations(100);
diff --git a/real_system_state.cc b/real_system_state.cc
index 9384228..7fca0ca 100644
--- a/real_system_state.cc
+++ b/real_system_state.cc
@@ -17,7 +17,6 @@
connection_manager_(this),
update_attempter_(this, &dbus_),
request_params_(this),
- policy_manager_(&clock_),
system_rebooted_(false) {}
bool RealSystemState::Initialize(bool enable_gpio) {
@@ -45,11 +44,15 @@
kMaxP2PFilesToKeep));
// Initialize the PolicyManager using the default State Factory.
- if (!policy_manager_.Init(chromeos_policy_manager::DefaultStateFactory(
- &policy_provider_, &dbus_, this))) {
+ chromeos_policy_manager::State* pm_state =
+ chromeos_policy_manager::DefaultStateFactory(
+ &policy_provider_, &dbus_, this);
+ if (!pm_state) {
LOG(ERROR) << "Failed to initialize the policy manager.";
return false;
}
+ policy_manager_.reset(
+ new chromeos_policy_manager::PolicyManager(&clock_, pm_state));
if (!payload_state_.Initialize(this)) {
LOG(ERROR) << "Failed to initialize the payload state object.";
diff --git a/real_system_state.h b/real_system_state.h
index 32d972b..aac7efc 100644
--- a/real_system_state.h
+++ b/real_system_state.h
@@ -90,7 +90,7 @@
virtual inline chromeos_policy_manager::PolicyManager* policy_manager()
override {
- return &policy_manager_;
+ return policy_manager_.get();
}
virtual inline bool system_rebooted() override {
@@ -142,7 +142,7 @@
scoped_ptr<P2PManager> p2p_manager_;
- chromeos_policy_manager::PolicyManager policy_manager_;
+ scoped_ptr<chromeos_policy_manager::PolicyManager> policy_manager_;
policy::PolicyProvider policy_provider_;