PolicyManager: Move the global variables to Provider members.
Per the Google C++ Style Guide, the global variables can't be
instances of a class and that includes smart pointers like
scoped_ptr. The alternative is to use plain pointers such as a
Variable<T>*. The disadvantage of this approach is that the memory
pointed by this global variable is managed by a singleton class
(a FooProvider) on production code and a different class during
testing (a FakeFooProvider). Also, these classes, fake and real
implementation, aren't forced to implement the same set of variables
since there's no common interface. Finally, the
construction/destruction of these variables also needs to handle the
global pointer, nulifying it after deletion.
This patch moves the global variable to a member of a base class that
hides the initialization of these variables to a subclass. This patch
includes a subclass with the real implementation.
BUG=chromium:338591
TEST=unittest passes.
Change-Id: I0aa43c51d72c65d97be280fd58307d085bb8577b
Reviewed-on: https://chromium-review.googlesource.com/184774
Tested-by: Alex Deymo <deymo@chromium.org>
Reviewed-by: Gilad Arnold <garnold@chromium.org>
Commit-Queue: Alex Deymo <deymo@chromium.org>
diff --git a/policy_manager/all_variables.h b/policy_manager/all_variables.h
deleted file mode 100644
index fb3401e..0000000
--- a/policy_manager/all_variables.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_ALL_VARIABLES_H
-#define CHROMEOS_PLATFORM_UPDATE_ENGINE_POLICY_MANAGER_ALL_VARIABLES_H
-
-// List of globally available variables exposed by the different providers.
-//
-// 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"
-
-#endif // CHROMEOS_PLATFORM_UPDATE_ENGINE_POLICY_MANAGER_ALL_VARIABLES_H
diff --git a/policy_manager/provider_utils.h b/policy_manager/provider_utils.h
deleted file mode 100644
index 5284ab2..0000000
--- a/policy_manager/provider_utils.h
+++ /dev/null
@@ -1,45 +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_PROVIDER_UTILS_H
-#define CHROMEOS_PLATFORM_UPDATE_ENGINE_POLICY_MANAGER_PROVIDER_UTILS_H
-
-// Convenience macro for declaring policy variable closers.
-//
-// TODO(garnold) If/when we switch to C++11, this can already initialize the
-// closer with the variable's address and save us further work in the ctor.
-//
-// TODO(garnold) This is likely unnecessary once we introduce a non-template
-// variable base class.
-#define DECLARE_VAR_CLOSER(closer_name, var_name) \
- ScopedPtrVarCloser<typeof(var_name)> closer_name
-
-namespace chromeos_policy_manager {
-
-// Scoped closer for a pointer variable. It is initialized with a reference to
-// a pointer variable. Upon destruction, it will destruct the object pointed to
-// by the variable and nullify the variable. This template can be easily
-// instantiated via 'typeof' of the variable that is being scoped:
-//
-// ScopedPtrVarCloser<typeof(foo)> foo_closer(&foo);
-//
-// where 'foo' is pointer variable of some type.
-template<typename T>
-class ScopedPtrVarCloser {
- public:
- ScopedPtrVarCloser(T* ptr_var_p) : ptr_var_p_(ptr_var_p) {}
- ~ScopedPtrVarCloser() {
- if (ptr_var_p_) {
- delete *ptr_var_p_;
- *ptr_var_p_ = NULL;
- }
- }
-
- private:
- T* ptr_var_p_;
-};
-
-} // namespace chromeos_policy_manager
-
-#endif // CHROMEOS_PLATFORM_UPDATE_ENGINE_POLICY_MANAGER_PROVIDER_UTILS_H
diff --git a/policy_manager/random_provider.h b/policy_manager/random_provider.h
index 7eaa8f6..60d7b2c 100644
--- a/policy_manager/random_provider.h
+++ b/policy_manager/random_provider.h
@@ -5,23 +5,29 @@
#ifndef CHROMEOS_PLATFORM_UPDATE_ENGINE_POLICY_MANAGER_RANDOM_PROVIDER_H
#define CHROMEOS_PLATFORM_UPDATE_ENGINE_POLICY_MANAGER_RANDOM_PROVIDER_H
+#include <base/memory/scoped_ptr.h>
+
#include "policy_manager/provider.h"
-#include "policy_manager/provider_utils.h"
-#include "policy_manager/random_vars.h"
+#include "policy_manager/variable.h"
namespace chromeos_policy_manager {
// Provider of random values.
class RandomProvider : public Provider {
public:
- RandomProvider() : seed_closer_(&var_random_seed) {}
+ // Return a random number every time it is requested. Note that values
+ // returned by the variables are cached by the EvaluationContext, so the
+ // returned value will be the same during the same policy request. If more
+ // random values are needed use a PRNG seeded with this value.
+ Variable<uint64_t>* seed() const { return seed_.get(); }
protected:
- virtual bool DoInit();
+ RandomProvider() {}
+
+ // The seed() scoped variable.
+ scoped_ptr<Variable<uint64_t> > seed_;
private:
- DECLARE_VAR_CLOSER(seed_closer_, var_random_seed);
-
DISALLOW_COPY_AND_ASSIGN(RandomProvider);
};
diff --git a/policy_manager/random_provider_unittest.cc b/policy_manager/random_provider_unittest.cc
deleted file mode 100644
index b0a6a0a..0000000
--- a/policy_manager/random_provider_unittest.cc
+++ /dev/null
@@ -1,67 +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.
-
-#include <string>
-
-#include <base/memory/scoped_ptr.h>
-#include <gtest/gtest.h>
-
-#include "policy_manager/random_provider.h"
-#include "policy_manager/random_vars.h"
-#include "policy_manager/pmtest_utils.h"
-
-using base::TimeDelta;
-using std::string;
-
-namespace chromeos_policy_manager {
-
-class PmRandomProviderTest : public ::testing::Test {
- protected:
- virtual void SetUp() {
- default_timeout_ = TimeDelta::FromSeconds(1);
-
- // All variables are initially null.
- PMTEST_ASSERT_NULL(var_random_seed);
-
- // The provider initializes correctly.
- provider_ = new RandomProvider();
- PMTEST_ASSERT_NOT_NULL(provider_);
- ASSERT_TRUE(provider_->Init());
-
- // The provider initializes all variables with valid objects.
- PMTEST_EXPECT_NOT_NULL(var_random_seed);
- }
-
- virtual void TearDown() {
- delete provider_;
- provider_ = NULL;
- PMTEST_EXPECT_NULL(var_random_seed);
- }
-
- TimeDelta default_timeout_;
-
- private:
- RandomProvider* provider_;
-};
-
-
-TEST_F(PmRandomProviderTest, GetRandomValues) {
- // Should not return the same random seed repeatedly.
- scoped_ptr<const uint64_t> value(
- var_random_seed->GetValue(default_timeout_, NULL));
- PMTEST_ASSERT_NOT_NULL(value.get());
-
- // Test that at least the returned values are different. This test fails,
- // by design, once every 2^320 runs.
- bool is_same_value = true;
- for (int i = 0; i < 5; i++) {
- scoped_ptr<const uint64_t> other_value(
- var_random_seed->GetValue(default_timeout_, NULL));
- PMTEST_ASSERT_NOT_NULL(other_value.get());
- is_same_value = is_same_value && *other_value == *value;
- }
- EXPECT_FALSE(is_same_value);
-}
-
-} // namespace chromeos_policy_manager
diff --git a/policy_manager/random_vars.cc b/policy_manager/random_vars.cc
deleted file mode 100644
index 95e4eea..0000000
--- a/policy_manager/random_vars.cc
+++ /dev/null
@@ -1,11 +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.
-
-#include "policy_manager/random_vars.h"
-
-namespace chromeos_policy_manager {
-
-Variable<uint64_t>* var_random_seed = NULL;
-
-} // namespace chromeos_policy_manager
diff --git a/policy_manager/random_vars.h b/policy_manager/random_vars.h
deleted file mode 100644
index 7c2d92d..0000000
--- a/policy_manager/random_vars.h
+++ /dev/null
@@ -1,20 +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_RANDOM_VARS_H
-#define CHROMEOS_PLATFORM_UPDATE_ENGINE_POLICY_MANAGER_RANDOM_VARS_H
-
-#include "policy_manager/variable.h"
-
-namespace chromeos_policy_manager {
-
-// Return a random number every time it is requested. Note that values returned
-// by the variables are cached by the EvaluationContext, so the returned value
-// will be the same during the same policy request. If more random values are
-// needed use a PRNG seeded with this value.
-extern Variable<uint64_t>* var_random_seed;
-
-} // namespace chromeos_policy_manager
-
-#endif // CHROMEOS_PLATFORM_UPDATE_ENGINE_POLICY_MANAGER_RANDOM_VARS_H
diff --git a/policy_manager/random_provider.cc b/policy_manager/real_random_provider.cc
similarity index 90%
rename from policy_manager/random_provider.cc
rename to policy_manager/real_random_provider.cc
index 431e77b..28213fa 100644
--- a/policy_manager/random_provider.cc
+++ b/policy_manager/real_random_provider.cc
@@ -9,7 +9,7 @@
#include <base/file_util.h>
#include <base/stringprintf.h>
-#include "policy_manager/random_provider.h"
+#include "policy_manager/real_random_provider.h"
#include "policy_manager/variable.h"
using std::string;
@@ -60,14 +60,11 @@
DISALLOW_COPY_AND_ASSIGN(RandomSeedVariable);
};
-
-// RandomProvider implementation.
-
-bool RandomProvider::DoInit(void) {
+bool RealRandomProvider::DoInit(void) {
FILE* fp = fopen(kRandomDevice, "r");
if (!fp)
return false;
- var_random_seed = new RandomSeedVariable("random_seed", fp);
+ seed_.reset(new RandomSeedVariable("random_seed", fp));
return true;
}
diff --git a/policy_manager/real_random_provider.h b/policy_manager/real_random_provider.h
new file mode 100644
index 0000000..e909d38
--- /dev/null
+++ b/policy_manager/real_random_provider.h
@@ -0,0 +1,27 @@
+// 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_REAL_RANDOM_PROVIDER_H
+#define CHROMEOS_PLATFORM_UPDATE_ENGINE_POLICY_MANAGER_REAL_RANDOM_PROVIDER_H
+
+#include "policy_manager/random_provider.h"
+
+namespace chromeos_policy_manager {
+
+// RandomProvider implementation class.
+class RealRandomProvider : public RandomProvider {
+ public:
+ RealRandomProvider() {}
+
+ protected:
+ // Initializes all the variables.
+ virtual bool DoInit(void);
+
+ private:
+ DISALLOW_COPY_AND_ASSIGN(RealRandomProvider);
+};
+
+} // namespace chromeos_policy_manager
+
+#endif // CHROMEOS_PLATFORM_UPDATE_ENGINE_POLICY_MANAGER_REAL_RANDOM_PROVIDER_H
diff --git a/policy_manager/real_random_provider_unittest.cc b/policy_manager/real_random_provider_unittest.cc
new file mode 100644
index 0000000..f04ca7c
--- /dev/null
+++ b/policy_manager/real_random_provider_unittest.cc
@@ -0,0 +1,61 @@
+// 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.
+
+#include <base/memory/scoped_ptr.h>
+#include <base/time.h>
+#include <gtest/gtest.h>
+
+#include "policy_manager/pmtest_utils.h"
+#include "policy_manager/real_random_provider.h"
+
+using base::TimeDelta;
+
+namespace chromeos_policy_manager {
+
+class PmRealRandomProviderTest : public ::testing::Test {
+ protected:
+ virtual void SetUp() {
+ default_timeout_ = TimeDelta::FromSeconds(1);
+
+ // The provider initializes correctly.
+ random_ = new RealRandomProvider();
+ ASSERT_TRUE(random_ != NULL);
+ ASSERT_TRUE(random_->Init());
+
+ (random_->seed());
+ }
+
+ virtual void TearDown() {
+ delete random_;
+ random_ = NULL;
+ }
+
+ TimeDelta default_timeout_;
+ RealRandomProvider* random_;
+};
+
+TEST_F(PmRealRandomProviderTest, InitFinalize) {
+ // The provider initializes all variables with valid objects.
+ PMTEST_EXPECT_NOT_NULL(random_->seed());
+}
+
+TEST_F(PmRealRandomProviderTest, GetRandomValues) {
+ // Should not return the same random seed repeatedly.
+ scoped_ptr<const uint64_t> value(
+ random_->seed()->GetValue(default_timeout_, NULL));
+ PMTEST_ASSERT_NOT_NULL(value.get());
+
+ // Test that at least the returned values are different. This test fails,
+ // by design, once every 2^320 runs.
+ bool is_same_value = true;
+ for (int i = 0; i < 5; i++) {
+ scoped_ptr<const uint64_t> other_value(
+ random_->seed()->GetValue(default_timeout_, NULL));
+ PMTEST_ASSERT_NOT_NULL(other_value.get());
+ is_same_value = is_same_value && *other_value == *value;
+ }
+ EXPECT_FALSE(is_same_value);
+}
+
+} // namespace chromeos_policy_manager
diff --git a/policy_manager/variable.h b/policy_manager/variable.h
index 9b933d7..7dcb5b9 100644
--- a/policy_manager/variable.h
+++ b/policy_manager/variable.h
@@ -39,8 +39,8 @@
virtual ~Variable() {}
protected:
- friend class PmRandomProviderTest;
- FRIEND_TEST(PmRandomProviderTest, GetRandomValues);
+ friend class PmRealRandomProviderTest;
+ FRIEND_TEST(PmRealRandomProviderTest, 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