AU/PM: Some refactoring
* Introduced a Provider base class, used for defining interface and
shared logic (e.g. initialization semantics).
* Eliminated the Finalize() method in providers; release of resources is
done in destructors (safer, less boilerplate).
* Revised CamelCase capitalization: PMFooTest -> PmFooTest, plus various
cosmetics.
BUG=None
TEST=Builds and passes unit tests.
Change-Id: Ib959dfd2522e00928d735202b1448c9436cbb00b
Reviewed-on: https://chromium-review.googlesource.com/184352
Tested-by: Gilad Arnold <garnold@chromium.org>
Reviewed-by: Alex Deymo <deymo@chromium.org>
Commit-Queue: Gilad Arnold <garnold@chromium.org>
diff --git a/policy_manager/all_variables.h b/policy_manager/all_variables.h
index be77698..fb3401e 100644
--- a/policy_manager/all_variables.h
+++ b/policy_manager/all_variables.h
@@ -7,11 +7,14 @@
// List of globally available variables exposed by the different providers.
//
-// Each state provider should implement a header file with the suffix "_vars.h"
-// with all the defined variable pointers, such as:
-// Variable<MyType>* var_something;
-// This file includes all the different provider's header files with these
-// definitions.
+// Each state provider must implement a header file with the suffix "_vars.h",
+// which declares all the variables owned by this provider declared as extern
+// global pointers.
+//
+// extern Variable<SomeType>* var_providername_variablename;
+//
+// This file is just an aggregate of all variable declarations
+// from the different providers.
#include "policy_manager/random_vars.h"
diff --git a/policy_manager/generic_variables-inl.h b/policy_manager/generic_variables-inl.h
deleted file mode 100644
index f44a287..0000000
--- a/policy_manager/generic_variables-inl.h
+++ /dev/null
@@ -1,21 +0,0 @@
-// Copyright (c) 2014 The Chromium OS Authors. All rights reserved.
-// Use of this source code is governed by a BSD-style license that can be
-// found in the LICENSE file.
-
-#ifndef CHROMEOS_PLATFORM_UPDATE_ENGINE_POLICY_MANAGER_GENERIC_VARIABLES_INL_H
-#define CHROMEOS_PLATFORM_UPDATE_ENGINE_POLICY_MANAGER_GENERIC_VARIABLES_INL_H
-
-namespace chromeos_policy_manager {
-
-template<typename T>
-CopyVariable<T>::CopyVariable(const T& ref) : ref_(ref) {}
-
-template<typename T>
-const T* CopyVariable<T>::GetValue(base::TimeDelta /* timeout */,
- std::string* /* errmsg */) {
- return new T(ref_);
-};
-
-} // namespace chromeos_policy_manager
-
-#endif // CHROMEOS_PLATFORM_UPDATE_ENGINE_POLICY_MANAGER_GENERIC_VARIABLES_INL_H
diff --git a/policy_manager/generic_variables.h b/policy_manager/generic_variables.h
index 321c89a..e78bbbd 100644
--- a/policy_manager/generic_variables.h
+++ b/policy_manager/generic_variables.h
@@ -2,7 +2,7 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
-// Generic and provider independent Variable subclasses. These variables can be
+// Generic and provider-independent Variable subclasses. These variables can be
// used by any state provider to implement simple variables to avoid repeat the
// same common code on different state providers.
@@ -19,24 +19,17 @@
// create copies of the provided object using the copy constructor of that
// class.
//
-// For example, a state provider exposing a private method as a variable could
-// be implemented in this way:
+// For example, a state provider exposing a private member as a variable can
+// implement this as follows:
//
-// * On something_vars.h:
-// Variable<MyType>* var_something;
-//
-// * On something_provider:
// class SomethingProvider {
// public:
// SomethingProvider(...) {
-// var_something = new CopyVariable<MyType>(priv_object_);
+// var_something_foo = new CopyVariable<MyType>(foo_);
// }
-// ~SomethingProvider() {
-// delete var_something;
-// var_something = NULL;
-// }
+// ...
// private:
-// MyType priv_object_;
+// MyType foo_;
// };
template<typename T>
class CopyVariable : public Variable<T> {
@@ -44,17 +37,20 @@
// Creates the variable returning copies of the passed |obj| reference. The
// reference to this object is kept and it should be available whenever the
// GetValue() method is called.
- CopyVariable(const T& obj);
+ CopyVariable(const T& ref) : ref_(ref) {};
virtual ~CopyVariable() {}
protected:
- friend class PMCopyVariableTest;
- FRIEND_TEST(PMCopyVariableTest, SimpleTest);
- FRIEND_TEST(PMCopyVariableTest, UseCopyConstructorTest);
+ friend class PmCopyVariableTest;
+ FRIEND_TEST(PmCopyVariableTest, SimpleTest);
+ FRIEND_TEST(PmCopyVariableTest, UseCopyConstructorTest);
// Variable override.
- virtual const T* GetValue(base::TimeDelta timeout, std::string* errmsg);
+ virtual const T* GetValue(base::TimeDelta /* timeout */,
+ std::string* /* errmsg */) {
+ return new T(ref_);
+ }
private:
// Reference to the object to be copied by GetValue().
@@ -63,7 +59,4 @@
} // namespace chromeos_policy_manager
-// Include implementation on header files for templates.
-#include "policy_manager/generic_variables-inl.h"
-
#endif // CHROMEOS_PLATFORM_UPDATE_ENGINE_POLICY_MANAGER_GENERIC_VARIABLES_H
diff --git a/policy_manager/generic_variables_unittest.cc b/policy_manager/generic_variables_unittest.cc
index 6758e23..14b40d3 100644
--- a/policy_manager/generic_variables_unittest.cc
+++ b/policy_manager/generic_variables_unittest.cc
@@ -10,7 +10,7 @@
namespace chromeos_policy_manager {
-TEST(PMCopyVariableTest, SimpleTest) {
+TEST(PmCopyVariableTest, SimpleTest) {
int obj_int = 5;
CopyVariable<int> var(obj_int);
@@ -45,7 +45,7 @@
bool copied_;
};
-TEST(PMCopyVariableTest, UseCopyConstructorTest) {
+TEST(PmCopyVariableTest, UseCopyConstructorTest) {
ConstructorTestClass obj;
ASSERT_FALSE(obj.copied_);
diff --git a/policy_manager/provider.h b/policy_manager/provider.h
new file mode 100644
index 0000000..0081c0d
--- /dev/null
+++ b/policy_manager/provider.h
@@ -0,0 +1,35 @@
+// Copyright (c) 2014 The Chromium OS Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CHROMEOS_PLATFORM_UPDATE_ENGINE_PM_PROVIDER_H
+#define CHROMEOS_PLATFORM_UPDATE_ENGINE_PM_PROVIDER_H
+
+namespace chromeos_policy_manager {
+
+// Abstract base class for a policy provider.
+class Provider {
+ public:
+ Provider() : is_initialized_(false) {}
+ virtual ~Provider() {}
+
+ // Initializes the provider at most once. Returns true on success.
+ bool Init() {
+ return is_initialized_ || (is_initialized_ = DoInit());
+ }
+
+ protected:
+ // Performs the actual initialization. To be implemented by concrete
+ // subclasses.
+ virtual bool DoInit() = 0;
+
+ private:
+ // Whether the provider was already initialized.
+ bool is_initialized_;
+
+ DISALLOW_COPY_AND_ASSIGN(Provider);
+};
+
+} // namespace chromeos_policy_manager
+
+#endif // CHROMEOS_PLATFORM_UPDATE_ENGINE_PM_PROVIDER_H
diff --git a/policy_manager/random_provider.cc b/policy_manager/random_provider.cc
index f2373b0..70dc31b 100644
--- a/policy_manager/random_provider.cc
+++ b/policy_manager/random_provider.cc
@@ -62,11 +62,14 @@
// RandomProvider implementation.
-bool RandomProvider::Init(void) {
- // Check for double Init()
- if (var_random_seed != NULL)
- return false;
+RandomProvider::~RandomProvider(void) {
+ if (var_random_seed) {
+ delete var_random_seed;
+ var_random_seed = NULL;
+ }
+}
+bool RandomProvider::DoInit(void) {
FILE* fp = fopen(kRandomDevice, "r");
if (!fp)
return false;
@@ -74,11 +77,4 @@
return true;
}
-void RandomProvider::Finalize(void) {
- if (var_random_seed) {
- delete var_random_seed;
- var_random_seed = NULL;
- }
-}
-
} // namespace chromeos_policy_manager
diff --git a/policy_manager/random_provider.h b/policy_manager/random_provider.h
index 7c7bae9..a610786 100644
--- a/policy_manager/random_provider.h
+++ b/policy_manager/random_provider.h
@@ -5,21 +5,18 @@
#ifndef CHROMEOS_PLATFORM_UPDATE_ENGINE_PM_RANDOM_PROVIDER_H
#define CHROMEOS_PLATFORM_UPDATE_ENGINE_PM_RANDOM_PROVIDER_H
-#include "base/basictypes.h"
+#include "policy_manager/provider.h"
namespace chromeos_policy_manager {
// Provider of random values.
-class RandomProvider {
+class RandomProvider : public Provider {
public:
RandomProvider() {}
+ virtual ~RandomProvider();
- // Initialize the provider variables and internal state. Returns whether it
- // succeeded.
- bool Init();
-
- // Destroy all the provider variables in a best-effor approach.
- void Finalize();
+ protected:
+ virtual bool DoInit();
private:
DISALLOW_COPY_AND_ASSIGN(RandomProvider);
diff --git a/policy_manager/random_provider_unittest.cc b/policy_manager/random_provider_unittest.cc
index 82ee5cc..af11b6e 100644
--- a/policy_manager/random_provider_unittest.cc
+++ b/policy_manager/random_provider_unittest.cc
@@ -14,44 +14,47 @@
namespace chromeos_policy_manager {
-class PMRandomProviderTest : public ::testing::Test {
+class PmRandomProviderTest : public ::testing::Test {
protected:
virtual void SetUp() {
ASSERT_TRUE(var_random_seed == NULL);
- EXPECT_TRUE(provider_.Init());
+ provider_ = new RandomProvider();
+ ASSERT_TRUE(provider_);
+ EXPECT_TRUE(provider_->Init());
}
virtual void TearDown() {
- provider_.Finalize();
+ delete provider_;
+ provider_ = NULL;
ASSERT_TRUE(var_random_seed == NULL);
}
private:
- RandomProvider provider_;
+ RandomProvider* provider_;
};
-TEST_F(PMRandomProviderTest, InitFinalize) {
+TEST_F(PmRandomProviderTest, InitFinalize) {
// The provider should initialize the variable with a valid object.
ASSERT_TRUE(var_random_seed != NULL);
}
-TEST_F(PMRandomProviderTest, GetRandomValues) {
+TEST_F(PmRandomProviderTest, GetRandomValues) {
string errmsg;
scoped_ptr<const uint64> value(
var_random_seed->GetValue(TimeDelta::FromSeconds(1.), &errmsg));
ASSERT_TRUE(value != NULL);
- bool returns_allways_the_same_value = true;
+ bool always_returns_the_same_value = true;
// Test that at least the returned values are different. This test fails,
// by design, once every 2^320 runs.
for (int i = 0; i < 5; i++) {
scoped_ptr<const uint64> other_value(
var_random_seed->GetValue(TimeDelta::FromSeconds(1.), &errmsg));
ASSERT_TRUE(other_value != NULL);
- returns_allways_the_same_value = returns_allways_the_same_value &&
+ always_returns_the_same_value = always_returns_the_same_value &&
*other_value == *value;
}
- EXPECT_FALSE(returns_allways_the_same_value);
+ EXPECT_FALSE(always_returns_the_same_value);
}
} // namespace chromeos_policy_manager
diff --git a/policy_manager/random_vars.h b/policy_manager/random_vars.h
index c1563c5..d983ca1 100644
--- a/policy_manager/random_vars.h
+++ b/policy_manager/random_vars.h
@@ -6,6 +6,7 @@
#define CHROMEOS_PLATFORM_UPDATE_ENGINE_PM_RANDOM_VARS_H
#include "base/basictypes.h"
+
#include "policy_manager/variable.h"
namespace chromeos_policy_manager {
diff --git a/policy_manager/variable.h b/policy_manager/variable.h
index 6acd31f..79953ed 100644
--- a/policy_manager/variable.h
+++ b/policy_manager/variable.h
@@ -19,8 +19,8 @@
virtual ~Variable() {}
protected:
- friend class PMRandomProviderTest;
- FRIEND_TEST(PMRandomProviderTest, GetRandomValues);
+ friend class PmRandomProviderTest;
+ FRIEND_TEST(PmRandomProviderTest, GetRandomValues);
// Gets the current value of the variable. The current value is copied to a
// new object and returned. The caller of this method owns the object and