PM: Shift to signal-based inference of network connection.
Instead of making DBus calls to shill on-demand, and estimating the time
a connection has changed, the RealShillProvider now listens to the
appropriate DBus signal and update its internal state accordingly. Note
that checking for the connection type still requires a DBus call, if the
connection has changed since its type was last checked.
In order to pass all unit tests (including those of PolicyManager and
RealState), there's a substantial portion of DBus mock set up code that
was added. This code will be removed very soon, once we begin injecting
providers into RealState, instead of low-level DBus and/or clock
interfaces.
BUG=chromium:338585
TEST=Unit tests.
Change-Id: Ia7a2f9db18f905f1b7a2cc1234acb31eaa60009e
Reviewed-on: https://chromium-review.googlesource.com/189692
Reviewed-by: Gilad Arnold <garnold@chromium.org>
Commit-Queue: Gilad Arnold <garnold@chromium.org>
Tested-by: Gilad Arnold <garnold@chromium.org>
diff --git a/policy_manager/policy_manager_unittest.cc b/policy_manager/policy_manager_unittest.cc
index fd43045..e2f3218 100644
--- a/policy_manager/policy_manager_unittest.cc
+++ b/policy_manager/policy_manager_unittest.cc
@@ -8,6 +8,8 @@
#include <base/bind.h>
#include <base/memory/scoped_ptr.h>
+// TODO(garnold) Remove once shill DBus constants not needed.
+#include <chromeos/dbus/service_constants.h>
#include <gtest/gtest.h>
#include <gmock/gmock.h>
@@ -17,23 +19,32 @@
#include "update_engine/policy_manager/mock_policy.h"
#include "update_engine/policy_manager/pmtest_utils.h"
#include "update_engine/policy_manager/policy_manager.h"
+// TODO(garnold) Remove once we stop mocking DBus.
#include "update_engine/test_utils.h"
using base::Bind;
using base::Callback;
using chromeos_update_engine::FakeClock;
+using chromeos_update_engine::GValueNewString;
+using chromeos_update_engine::GValueFree;
using chromeos_update_engine::MockDBusWrapper;
using std::pair;
using std::string;
using std::vector;
using testing::NiceMock;
using testing::Return;
+using testing::SetArgPointee;
using testing::StrictMock;
using testing::_;
namespace {
+// TODO(garnold) This whole section gets removed once we mock the shill provider
+// itself in tests.
+
+// Fake dbus-glib objects.
DBusGConnection* const kFakeConnection = reinterpret_cast<DBusGConnection*>(1);
+DBusGProxy* const kFakeManagerProxy = reinterpret_cast<DBusGProxy*>(2);
} // namespace
@@ -42,8 +53,28 @@
class PmPolicyManagerTest : public ::testing::Test {
protected:
virtual void SetUp() {
- EXPECT_CALL(mock_dbus_, BusGet(_, _)).WillOnce(Return(kFakeConnection));
+ // TODO(garnold) Replace this low-level DBus injection with a high-level
+ // mock shill provider.
+ EXPECT_CALL(mock_dbus_, BusGet(_, _))
+ .WillOnce(Return(kFakeConnection));
+ EXPECT_CALL(mock_dbus_, ProxyNewForName(_, _, _, _))
+ .WillOnce(Return(kFakeManagerProxy));
+ EXPECT_CALL(mock_dbus_, ProxyAddSignal_2(_, _, _, _))
+ .WillOnce(Return());
+ EXPECT_CALL(mock_dbus_, ProxyConnectSignal(_, _, _, _, _))
+ .WillOnce(Return());
+ auto properties = g_hash_table_new_full(g_str_hash, g_str_equal, free,
+ GValueFree);
+ g_hash_table_insert(properties, strdup(shill::kDefaultServiceProperty),
+ GValueNewString("/"));
+ EXPECT_CALL(mock_dbus_, ProxyCall_0_1(_, _, _, _))
+ .WillOnce(DoAll(SetArgPointee<3>(g_hash_table_ref(properties)),
+ Return(true)));
+
EXPECT_TRUE(pmut_.Init(&mock_dbus_, &fake_clock_));
+
+ // TODO(garnold) Remove this, too.
+ g_hash_table_unref(properties);
}
NiceMock<MockDBusWrapper> mock_dbus_;
diff --git a/policy_manager/real_shill_provider.cc b/policy_manager/real_shill_provider.cc
index 60a953f..244a059 100644
--- a/policy_manager/real_shill_provider.cc
+++ b/policy_manager/real_shill_provider.cc
@@ -7,6 +7,7 @@
#include <string>
#include <base/logging.h>
+#include <base/stringprintf.h>
#include <chromeos/dbus/service_constants.h>
#include "update_engine/policy_manager/generic_variables.h"
@@ -28,6 +29,8 @@
namespace chromeos_policy_manager {
+// ShillConnector methods.
+
const ShillConnector::ConnStrToType ShillConnector::shill_conn_str_to_type[] = {
{shill::kTypeEthernet, kShillConnTypeEthernet},
{shill::kTypeWifi, kShillConnTypeWifi},
@@ -36,7 +39,21 @@
{shill::kTypeCellular, kShillConnTypeCellular},
};
+ShillConnector::~ShillConnector() {
+ if (!is_init_)
+ return;
+
+ // Detach signal handler, free manager proxy.
+ dbus_->ProxyDisconnectSignal(manager_proxy_, shill::kMonitorPropertyChanged,
+ G_CALLBACK(signal_handler_), signal_data_);
+ dbus_->ProxyUnref(manager_proxy_);
+}
+
bool ShillConnector::Init() {
+ if (is_init_)
+ return true;
+
+ // Obtain a DBus connection.
GError* error = NULL;
connection_ = dbus_->BusGet(DBUS_BUS_SYSTEM, &error);
if (!connection_) {
@@ -44,29 +61,18 @@
<< chromeos_update_engine::utils::GetAndFreeGError(&error);
return false;
}
+
+ // Allocate a shill manager proxy.
manager_proxy_ = GetProxy(shill::kFlimflamServicePath,
shill::kFlimflamManagerInterface);
- return true;
-}
-bool ShillConnector::GetDefaultConnection(bool* is_connected_p,
- string* default_service_path_p) {
- GHashTable* hash_table = NULL;
- if (!GetProperties(manager_proxy_, &hash_table))
- return false;
- GValue* value = reinterpret_cast<GValue*>(
- g_hash_table_lookup(hash_table, shill::kDefaultServiceProperty));
- const char* default_service_path_str = NULL;
- bool success = false;
- if (value && (default_service_path_str = g_value_get_string(value))) {
- success = true;
- *is_connected_p = strcmp(default_service_path_str, "/");
- if (*is_connected_p)
- *default_service_path_p = default_service_path_str;
- }
- g_hash_table_unref(hash_table);
+ // Subscribe to the manager's PropertyChanged signal.
+ dbus_->ProxyAddSignal_2(manager_proxy_, shill::kMonitorPropertyChanged,
+ G_TYPE_STRING, G_TYPE_VALUE);
+ dbus_->ProxyConnectSignal(manager_proxy_, shill::kMonitorPropertyChanged,
+ G_CALLBACK(signal_handler_), signal_data_, NULL);
- return success;
+ return is_init_ = true;
}
bool ShillConnector::GetConnectionType(const string& service_path,
@@ -114,8 +120,6 @@
return kShillConnTypeUnknown;
}
-// Issues a GetProperties call through a given |proxy|, storing the result to
-// |*result_p|. Returns |true| on success.
bool ShillConnector::GetProperties(DBusGProxy* proxy, GHashTable** result_p) {
GError* error = NULL;
if (!dbus_->ProxyCall_0_1(proxy, shill::kGetPropertiesFunction, &error,
@@ -127,82 +131,112 @@
return true;
}
-// A variable returning whether or not we have a network connection.
-class IsConnectedVariable : public Variable<bool> {
- public:
- IsConnectedVariable(const string& name, ShillConnector* connector,
- LastValueTracker<bool>* is_connected_tracker)
- : Variable<bool>(name, kVariableModePoll),
- connector_(connector),
- is_connected_tracker_(is_connected_tracker) {}
-
- protected:
- virtual const bool* GetValue(base::TimeDelta timeout, string* errmsg) {
- bool is_connected;
- string default_service_path;
- if (!connector_->GetDefaultConnection(&is_connected, &default_service_path))
- return NULL;;
-
- return new bool(is_connected_tracker_->Update(is_connected));
- }
-
- private:
- ShillConnector* connector_;
- LastValueTracker<bool>* is_connected_tracker_;
-
- DISALLOW_COPY_AND_ASSIGN(IsConnectedVariable);
-};
// A variable returning the curent connection type.
class ConnTypeVariable : public Variable<ShillConnType> {
public:
ConnTypeVariable(const string& name, ShillConnector* connector,
- LastValueTracker<bool>* is_connected_tracker)
+ RealShillProvider* provider)
: Variable<ShillConnType>(name, kVariableModePoll),
- connector_(connector),
- is_connected_tracker_(is_connected_tracker) {}
+ connector_(connector), provider_(provider) {}
protected:
- virtual const ShillConnType* GetValue(base::TimeDelta timeout,
+ // TODO(garnold) Shift to a non-blocking version, respect the timeout.
+ virtual const ShillConnType* GetValue(base::TimeDelta /* timeout */,
string* errmsg) {
- bool is_connected;
- string default_service_path;
- ShillConnType conn_type;
- if (!(connector_->GetDefaultConnection(&is_connected,
- &default_service_path) &&
- is_connected &&
- connector_->GetConnectionType(default_service_path, &conn_type)))
+ if (!(provider_->is_connected_)) {
+ if (errmsg)
+ *errmsg = "No connection detected";
return NULL;
+ }
- is_connected_tracker_->Update(is_connected);
+ ShillConnType conn_type;
+ if (provider_->is_conn_type_valid_) {
+ conn_type = provider_->conn_type_;
+ } else {
+ if (!connector_->GetConnectionType(provider_->default_service_path_,
+ &conn_type)) {
+ if (errmsg)
+ *errmsg = StringPrintf(
+ "Could not retrieve type of default connection (%s)",
+ provider_->default_service_path_.c_str());
+ return NULL;
+ }
+ provider_->is_conn_type_valid_ = true;
+ }
+
return new ShillConnType(conn_type);
}
private:
+ // The DBus connector.
ShillConnector* connector_;
- LastValueTracker<bool>* is_connected_tracker_;
+
+ // The shill provider object (we need to read/update some internal members).
+ RealShillProvider* provider_;
DISALLOW_COPY_AND_ASSIGN(ConnTypeVariable);
};
-// A real implementation of the ShillProvider.
+
+// RealShillProvider methods.
+
bool RealShillProvider::DoInit() {
// Initialize a DBus connection and obtain the shill manager proxy.
- connector_.reset(new ShillConnector(dbus_));
+ connector_.reset(new ShillConnector(dbus_, HandlePropertyChangedStatic,
+ this));
if (!connector_->Init())
return false;
+ // Read initial connection status.
+ GHashTable* hash_table = NULL;
+ if (!connector_->GetManagerProperties(&hash_table))
+ return false;
+ GValue* value = reinterpret_cast<GValue*>(
+ g_hash_table_lookup(hash_table, shill::kDefaultServiceProperty));
+ bool success = ProcessDefaultService(value);
+ g_hash_table_unref(hash_table);
+ if (!success)
+ return false;
+
// Initialize variables.
set_var_is_connected(
- new IsConnectedVariable("is_connected", connector_.get(),
- &is_connected_tracker_));
+ new CopyVariable<bool>("is_connected", kVariableModePoll, is_connected_));
set_var_conn_type(
- new ConnTypeVariable("conn_type", connector_.get(),
- &is_connected_tracker_));
+ new ConnTypeVariable("conn_type", connector_.get(), this));
set_var_conn_last_changed(
new CopyVariable<base::Time>("conn_last_changed", kVariableModePoll,
conn_last_changed_));
return true;
}
+bool RealShillProvider::ProcessDefaultService(GValue* value) {
+ // Decode the string from the boxed value.
+ const char* default_service_path_str = NULL;
+ if (!(value && (default_service_path_str = g_value_get_string(value))))
+ return false;
+
+ // Update the connection status.
+ is_connected_ = strcmp(default_service_path_str, "/");
+ if (default_service_path_ != default_service_path_str)
+ conn_last_changed_ = clock_->GetWallclockTime();
+ default_service_path_ = default_service_path_str;
+ is_conn_type_valid_ = false;
+ return true;
+}
+
+void RealShillProvider::HandlePropertyChanged(DBusGProxy* proxy,
+ const char* name, GValue* value) {
+ if (!strcmp(name, shill::kDefaultServiceProperty))
+ ProcessDefaultService(value);
+}
+
+void RealShillProvider::HandlePropertyChangedStatic(DBusGProxy* proxy,
+ const char* name,
+ GValue* value,
+ void* data) {
+ auto obj = reinterpret_cast<RealShillProvider*>(data);
+ obj->HandlePropertyChanged(proxy, name, value);
+}
+
} // namespace chromeos_policy_manager
diff --git a/policy_manager/real_shill_provider.h b/policy_manager/real_shill_provider.h
index 80b4802..ba24234 100644
--- a/policy_manager/real_shill_provider.h
+++ b/policy_manager/real_shill_provider.h
@@ -22,55 +22,40 @@
namespace chromeos_policy_manager {
-// A tracker for the last reported value. Whenever Update() is called with a new
-// value, the current time is written to a pointed time object.
-template<typename T>
-class LastValueTracker {
- public:
- LastValueTracker(ClockInterface* clock, T init_val, base::Time* time_p)
- : clock_(clock), last_val_(init_val), time_p_(time_p) {}
-
- const T& Update(const T& curr_val) {
- if (curr_val != last_val_) {
- last_val_ = curr_val;
- *time_p_ = clock_->GetWallclockTime();
- }
- return curr_val;
- }
-
- private:
- ClockInterface* const clock_;
- T last_val_;
- base::Time* time_p_;
-};
-
-// A DBus connector for making shill queries.
+// A DBus connector for making all shill related calls.
class ShillConnector {
public:
- ShillConnector(DBusWrapperInterface* dbus) : dbus_(dbus) {}
+ // Expected type for the PropertyChanged signal handler.
+ typedef void (*PropertyChangedHandler)(DBusGProxy*, const char*, GValue*,
+ void*);
- ~ShillConnector() {
- if (manager_proxy_)
- dbus_->ProxyUnref(manager_proxy_);
- }
+ ShillConnector(DBusWrapperInterface* dbus,
+ PropertyChangedHandler signal_handler, void* signal_data)
+ : dbus_(dbus), signal_handler_(signal_handler),
+ signal_data_(signal_data) {}
+
+ ~ShillConnector();
// Initializes the DBus connector. Returns |true| on success.
bool Init();
- // Obtains the default network connection, storing the connection status to
- // |*is_connected_p| and (if connected) the service path for the default
- // connection in |*default_service_path_p|. Returns |true| on success; |false|
- // on failure, in which case no values are written.
- bool GetDefaultConnection(bool* is_connected_p,
- std::string* default_service_path_p);
-
// Obtains the type of a network connection described by |service_path|,
// storing it to |*conn_type_p|. Returns |true| on success; |false| on
// failure, in which case no value is written.
bool GetConnectionType(const std::string& service_path,
ShillConnType* conn_type_p);
+ // Issues a GetProperties call to shill's manager interface, storing the
+ // result to |*result_p|. Returns |true| on success.
+ bool GetManagerProperties(GHashTable** result_p) {
+ return GetProperties(manager_proxy_, result_p);
+ }
+
private:
+ // Issues a GetProperties call through a given |proxy|, storing the result to
+ // |*result_p|. Returns |true| on success.
+ bool GetProperties(DBusGProxy* proxy, GHashTable** result_p);
+
typedef struct {
const char *str;
ShillConnType type;
@@ -79,21 +64,24 @@
// A mapping from shill connection type strings to enum values.
static const ConnStrToType shill_conn_str_to_type[];
+ // An initialization flag.
+ bool is_init_ = false;
+
// The DBus interface and connection, and a shill manager proxy.
DBusWrapperInterface* dbus_;
DBusGConnection* connection_ = NULL;
DBusGProxy* manager_proxy_ = NULL;
+ // The shill manager signal handler credentials.
+ PropertyChangedHandler signal_handler_ = NULL;
+ void* signal_data_ = NULL;
+
// Return a DBus proxy for a given |path| and |interface| within shill.
DBusGProxy* GetProxy(const char* path, const char* interface);
// Converts a shill connection type string into a symbolic value.
ShillConnType ParseConnType(const char* str);
- // Issues a GetProperties call through a given |proxy|, storing the result to
- // |*result_p|. Returns |true| on success.
- bool GetProperties(DBusGProxy* proxy, GHashTable** result_p);
-
DISALLOW_COPY_AND_ASSIGN(ShillConnector);
};
@@ -101,17 +89,37 @@
class RealShillProvider : public ShillProvider {
public:
RealShillProvider(DBusWrapperInterface* dbus, ClockInterface* clock)
- : conn_last_changed_(clock->GetWallclockTime()),
- dbus_(dbus), clock_(clock),
- is_connected_tracker_(clock, false, &conn_last_changed_) {}
+ : dbus_(dbus), clock_(clock) {}
protected:
virtual bool DoInit();
private:
+ // Process a default connection value, update last change time as needed.
+ bool ProcessDefaultService(GValue* value);
+
+ // A handler for manager PropertyChanged signal, and a static version.
+ void HandlePropertyChanged(DBusGProxy* proxy, const char *name,
+ GValue* value);
+ static void HandlePropertyChangedStatic(DBusGProxy* proxy, const char* name,
+ GValue* value, void* data);
+
+
// The time when the connection type last changed.
base::Time conn_last_changed_;
+ // The current connection status.
+ bool is_connected_;
+
+ // The current default service path, if connected.
+ std::string default_service_path_;
+
+ // The last known type of the default connection.
+ ShillConnType conn_type_ = kShillConnTypeUnknown;
+
+ // Whether the last known connection type is valid.
+ bool is_conn_type_valid_ = false;
+
// A shill DBus connector.
scoped_ptr<ShillConnector> connector_;
@@ -121,9 +129,7 @@
// A clock abstraction (mockable).
ClockInterface* const clock_;
- // Tracker for the latest connection status.
- LastValueTracker<bool> is_connected_tracker_;
-
+ friend class ConnTypeVariable;
DISALLOW_COPY_AND_ASSIGN(RealShillProvider);
};
diff --git a/policy_manager/real_shill_provider_unittest.cc b/policy_manager/real_shill_provider_unittest.cc
index ded5d63..364be1a 100644
--- a/policy_manager/real_shill_provider_unittest.cc
+++ b/policy_manager/real_shill_provider_unittest.cc
@@ -12,15 +12,20 @@
#include "update_engine/mock_dbus_wrapper.h"
#include "update_engine/policy_manager/real_shill_provider.h"
#include "update_engine/policy_manager/pmtest_utils.h"
+#include "update_engine/test_utils.h"
using base::Time;
using base::TimeDelta;
using chromeos_update_engine::FakeClock;
+using chromeos_update_engine::GValueNewString;
+using chromeos_update_engine::GValueFree;
using chromeos_update_engine::MockDBusWrapper;
using std::pair;
using testing::_;
+using testing::Eq;
using testing::NiceMock;
using testing::Return;
+using testing::SaveArg;
using testing::SetArgPointee;
using testing::StrEq;
using testing::StrictMock;
@@ -48,21 +53,6 @@
const char* const kFakeVpnServicePath = "/fake-vpn-service";
const char* const kFakeUnknownServicePath = "/fake-unknown-service";
-// Allocates, initializes and returns a string GValue object.
-GValue* GValueNewString(const char* str) {
- GValue* gval = new GValue();
- g_value_init(gval, G_TYPE_STRING);
- g_value_set_string(gval, str);
- return gval;
-}
-
-// Frees a GValue object and its allocated resources.
-void GValueFree(gpointer arg) {
- auto gval = reinterpret_cast<GValue*>(arg);
- g_value_unset(gval);
- delete gval;
-}
-
} // namespace
namespace chromeos_policy_manager {
@@ -70,23 +60,47 @@
class PmRealShillProviderTest : public ::testing::Test {
protected:
virtual void SetUp() {
- // The provider initializes correctly.
- fake_clock_.SetWallclockTime(init_time_);
- // A DBus connection should only be obtained once.
- EXPECT_CALL(mock_dbus_, BusGet(_, _)).WillOnce(Return(kFakeConnection));
provider_.reset(new RealShillProvider(&mock_dbus_, &fake_clock_));
PMTEST_ASSERT_NOT_NULL(provider_.get());
+ fake_clock_.SetWallclockTime(init_time_);
+ // A DBus connection should only be obtained once.
+ EXPECT_CALL(mock_dbus_, BusGet(_, _)).WillOnce(
+ Return(kFakeConnection));
// A manager proxy should only be obtained once.
EXPECT_CALL(mock_dbus_, ProxyNewForName(
kFakeConnection, StrEq(shill::kFlimflamServiceName),
StrEq(shill::kFlimflamServicePath),
StrEq(shill::kFlimflamManagerInterface)))
.WillOnce(Return(kFakeManagerProxy));
+ // The PropertyChanged signal should be subscribed to.
+ EXPECT_CALL(mock_dbus_, ProxyAddSignal_2(
+ kFakeManagerProxy, StrEq(shill::kMonitorPropertyChanged),
+ G_TYPE_STRING, G_TYPE_VALUE))
+ .WillOnce(Return());
+ EXPECT_CALL(mock_dbus_, ProxyConnectSignal(
+ kFakeManagerProxy, StrEq(shill::kMonitorPropertyChanged),
+ _, _, _))
+ .WillOnce(DoAll(SaveArg<2>(&signal_handler_),
+ SaveArg<3>(&signal_data_),
+ Return()));
+ // Default service should be checked once during initialization.
+ pair<const char*, const char*> manager_pairs[] = {
+ {shill::kDefaultServiceProperty, "/"},
+ };
+ auto manager_properties = MockGetProperties(kFakeManagerProxy, false,
+ 1, manager_pairs);
+ // Check that provider initializes corrrectly.
ASSERT_TRUE(provider_->Init());
+ // Release properties hash table.
+ g_hash_table_unref(manager_properties);
}
virtual void TearDown() {
- // Make sure the manager proxy gets unreffed (once).
+ // Make sure that DBus resources get freed.
+ EXPECT_CALL(mock_dbus_, ProxyDisconnectSignal(
+ kFakeManagerProxy, StrEq(shill::kMonitorPropertyChanged),
+ Eq(signal_handler_), Eq(signal_data_)))
+ .WillOnce(Return());
EXPECT_CALL(mock_dbus_, ProxyUnref(kFakeManagerProxy)).WillOnce(Return());
provider_.reset();
// Verify and clear all expectations.
@@ -133,16 +147,18 @@
DBusGProxy* service_proxy,
const char* shill_type_str,
ShillConnType expected_conn_type) {
- // Mock logic for returning a default service path.
- pair<const char*, const char*> manager_pairs[] = {
- {shill::kDefaultServiceProperty, service_path},
- };
- auto manager_properties = MockGetProperties(
- kFakeManagerProxy, true, arraysize(manager_pairs), manager_pairs);
+ // Send a signal about a new default service.
+ auto callback = reinterpret_cast<ShillConnector::PropertyChangedHandler>(
+ signal_handler_);
+ auto default_service_gval = GValueNewString(service_path);
+ Time conn_change_time = Time::Now();
+ fake_clock_.SetWallclockTime(conn_change_time);
+ callback(kFakeManagerProxy, shill::kDefaultServiceProperty,
+ default_service_gval, signal_data_);
+ fake_clock_.SetWallclockTime(conn_change_time + TimeDelta::FromSeconds(5));
+ GValueFree(default_service_gval);
// Query the connection status, ensure last change time reported correctly.
- const Time change_time = Time::Now();
- fake_clock_.SetWallclockTime(change_time);
scoped_ptr<const bool> is_connected(
provider_->var_is_connected()->GetValue(default_timeout_, NULL));
PMTEST_ASSERT_NOT_NULL(is_connected.get());
@@ -151,7 +167,7 @@
scoped_ptr<const Time> conn_last_changed_1(
provider_->var_conn_last_changed()->GetValue(default_timeout_, NULL));
PMTEST_ASSERT_NOT_NULL(conn_last_changed_1.get());
- EXPECT_EQ(change_time, *conn_last_changed_1);
+ EXPECT_EQ(conn_change_time, *conn_last_changed_1);
// Mock logic for querying the type of the default service.
EXPECT_CALL(mock_dbus_, ProxyNewForName(
@@ -159,17 +175,16 @@
StrEq(service_path),
StrEq(shill::kFlimflamServiceInterface)))
.WillOnce(Return(service_proxy));
- EXPECT_CALL(mock_dbus_, ProxyUnref(service_proxy))
- .WillOnce(Return());
+ EXPECT_CALL(mock_dbus_,
+ ProxyUnref(service_proxy)).WillOnce(Return());
pair<const char*, const char*> service_pairs[] = {
{shill::kTypeProperty, shill_type_str},
};
auto service_properties = MockGetProperties(
- service_proxy, false, arraysize(service_pairs),
- service_pairs);
+ service_proxy, false,
+ arraysize(service_pairs), service_pairs);
- // Query the connection type, ensure last change time reported correctly.
- fake_clock_.SetWallclockTime(change_time + TimeDelta::FromSeconds(5));
+ // Query the connection type, ensure last change time did not change.
scoped_ptr<const ShillConnType> conn_type(
provider_->var_conn_type()->GetValue(default_timeout_, NULL));
PMTEST_ASSERT_NOT_NULL(conn_type.get());
@@ -178,11 +193,10 @@
scoped_ptr<const Time> conn_last_changed_2(
provider_->var_conn_last_changed()->GetValue(default_timeout_, NULL));
PMTEST_ASSERT_NOT_NULL(conn_last_changed_2.get());
- EXPECT_EQ(change_time, *conn_last_changed_2);
+ EXPECT_EQ(*conn_last_changed_1, *conn_last_changed_2);
// Release properties hash tables.
g_hash_table_unref(service_properties);
- g_hash_table_unref(manager_properties);
}
const TimeDelta default_timeout_ = TimeDelta::FromSeconds(1);
@@ -190,21 +204,14 @@
StrictMock<MockDBusWrapper> mock_dbus_;
FakeClock fake_clock_;
scoped_ptr<RealShillProvider> provider_;
+ GCallback signal_handler_;
+ // FIXME void (*signal_handler_)(DBusGProxy*, const char*, GValue*, void*);
+ void* signal_data_;
};
-// Programs the mock DBus interface to indicate no default connection; this is
-// in line with the default values the variables should be initialized with, and
-// so the last changed time should not be updated. Also note that reading the
-// connection type should not induce getting of a per-service proxy, as no
-// default service is reported.
+// Query the connection status, type and time last changed, as they were set
+// during initialization.
TEST_F(PmRealShillProviderTest, ReadDefaultValues) {
- // Mock logic for returning no default connection.
- pair<const char*, const char*> manager_pairs[] = {
- {shill::kDefaultServiceProperty, "/"},
- };
- auto manager_properties = MockGetProperties(
- kFakeManagerProxy, true, arraysize(manager_pairs), manager_pairs);
-
// Query the provider variables.
scoped_ptr<const bool> is_connected(
provider_->var_is_connected()->GetValue(default_timeout_, NULL));
@@ -219,9 +226,6 @@
provider_->var_conn_last_changed()->GetValue(default_timeout_, NULL));
PMTEST_ASSERT_NOT_NULL(conn_last_changed.get());
EXPECT_EQ(init_time_, *conn_last_changed);
-
- // Release properties hash table.
- g_hash_table_unref(manager_properties);
}
// Test that Ethernet connection is identified correctly.
@@ -274,12 +278,18 @@
// Tests that VPN connection is identified correctly.
TEST_F(PmRealShillProviderTest, ReadChangedValuesConnectedViaVpn) {
+ // Send a signal about a new default service.
+ auto callback = reinterpret_cast<ShillConnector::PropertyChangedHandler>(
+ signal_handler_);
+ auto default_service_gval = GValueNewString(kFakeVpnServicePath);
+ Time conn_change_time = Time::Now();
+ fake_clock_.SetWallclockTime(conn_change_time);
+ callback(kFakeManagerProxy, shill::kDefaultServiceProperty,
+ default_service_gval, signal_data_);
+ fake_clock_.SetWallclockTime(conn_change_time + TimeDelta::FromSeconds(5));
+ GValueFree(default_service_gval);
+
// Mock logic for returning a default service path and its type.
- pair<const char*, const char*> manager_pairs[] = {
- {shill::kDefaultServiceProperty, kFakeVpnServicePath},
- };
- auto manager_properties = MockGetProperties(
- kFakeManagerProxy, false, arraysize(manager_pairs), manager_pairs);
EXPECT_CALL(mock_dbus_, ProxyNewForName(
kFakeConnection, StrEq(shill::kFlimflamServiceName),
StrEq(kFakeVpnServicePath), StrEq(shill::kFlimflamServiceInterface)))
@@ -293,7 +303,6 @@
kFakeVpnServiceProxy, false, arraysize(service_pairs), service_pairs);
// Query the connection type, ensure last change time reported correctly.
- fake_clock_.SetWallclockTime(Time::Now());
scoped_ptr<const ShillConnType> conn_type(
provider_->var_conn_type()->GetValue(default_timeout_, NULL));
PMTEST_ASSERT_NOT_NULL(conn_type.get());
@@ -302,11 +311,40 @@
scoped_ptr<const Time> conn_last_changed(
provider_->var_conn_last_changed()->GetValue(default_timeout_, NULL));
PMTEST_ASSERT_NOT_NULL(conn_last_changed.get());
- EXPECT_EQ(fake_clock_.GetWallclockTime(), *conn_last_changed);
+ EXPECT_EQ(conn_change_time, *conn_last_changed);
// Release properties hash tables.
g_hash_table_unref(service_properties);
- g_hash_table_unref(manager_properties);
+}
+
+// Fake two DBus signal prompting about a default connection change, but
+// otherwise give the same service path. Check connection status and the time is
+// was last changed, make sure it is the same time when the first signal was
+// sent (and not the second).
+TEST_F(PmRealShillProviderTest, ReadChangedValuesConnectedTwoSignals) {
+ // Send a default service signal twice, advancing the clock in between.
+ auto callback = reinterpret_cast<ShillConnector::PropertyChangedHandler>(
+ signal_handler_);
+ auto default_service_gval = GValueNewString(kFakeEthernetServicePath);
+ Time conn_change_time = Time::Now();
+ fake_clock_.SetWallclockTime(conn_change_time);
+ callback(kFakeManagerProxy, shill::kDefaultServiceProperty,
+ default_service_gval, signal_data_);
+ fake_clock_.SetWallclockTime(conn_change_time + TimeDelta::FromSeconds(5));
+ callback(kFakeManagerProxy, shill::kDefaultServiceProperty,
+ default_service_gval, signal_data_);
+ GValueFree(default_service_gval);
+
+ // Query the connection status, ensure last change time reported correctly.
+ scoped_ptr<const bool> is_connected(
+ provider_->var_is_connected()->GetValue(default_timeout_, NULL));
+ PMTEST_ASSERT_NOT_NULL(is_connected.get());
+ EXPECT_TRUE(*is_connected);
+
+ scoped_ptr<const Time> conn_last_changed(
+ provider_->var_conn_last_changed()->GetValue(default_timeout_, NULL));
+ PMTEST_ASSERT_NOT_NULL(conn_last_changed.get());
+ EXPECT_EQ(conn_change_time, *conn_last_changed);
}
} // namespace chromeos_policy_manager
diff --git a/policy_manager/real_state_unittest.cc b/policy_manager/real_state_unittest.cc
index 48491d5..13f8db2 100644
--- a/policy_manager/real_state_unittest.cc
+++ b/policy_manager/real_state_unittest.cc
@@ -2,22 +2,34 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
+// TODO(garnold) Remove once shill DBus constants not needed.
+#include <chromeos/dbus/service_constants.h>
#include <gtest/gtest.h>
#include "update_engine/fake_clock.h"
#include "update_engine/mock_dbus_wrapper.h"
#include "update_engine/policy_manager/real_state.h"
#include "update_engine/policy_manager/pmtest_utils.h"
+// TODO(garnold) Remove once we stop mocking DBus.
+#include "update_engine/test_utils.h"
using chromeos_update_engine::FakeClock;
+using chromeos_update_engine::GValueNewString;
+using chromeos_update_engine::GValueFree;
using chromeos_update_engine::MockDBusWrapper;
using testing::_;
using testing::NiceMock;
using testing::Return;
+using testing::SetArgPointee;
namespace {
+// TODO(garnold) This whole section gets removed once we mock the shill provider
+// itself in tests.
+
+// Fake dbus-glib objects.
DBusGConnection* const kFakeConnection = reinterpret_cast<DBusGConnection*>(1);
+DBusGProxy* const kFakeManagerProxy = reinterpret_cast<DBusGProxy*>(2);
} // namespace
@@ -26,9 +38,31 @@
TEST(PmRealStateTest, InitTest) {
NiceMock<MockDBusWrapper> mock_dbus;
FakeClock fake_clock;
- EXPECT_CALL(mock_dbus, BusGet(_, _)).WillOnce(Return(kFakeConnection));
+
+ // TODO(garnold) Replace this low-level DBus injection with a high-level
+ // mock shill provider.
+ EXPECT_CALL(mock_dbus, BusGet(_, _))
+ .WillOnce(Return(kFakeConnection));
+ EXPECT_CALL(mock_dbus, ProxyNewForName(_, _, _, _))
+ .WillOnce(Return(kFakeManagerProxy));
+ EXPECT_CALL(mock_dbus, ProxyAddSignal_2(_, _, _, _))
+ .WillOnce(Return());
+ EXPECT_CALL(mock_dbus, ProxyConnectSignal(_, _, _, _, _))
+ .WillOnce(Return());
+ auto properties = g_hash_table_new_full(g_str_hash, g_str_equal, free,
+ GValueFree);
+ g_hash_table_insert(properties, strdup(shill::kDefaultServiceProperty),
+ GValueNewString("/"));
+ EXPECT_CALL(mock_dbus, ProxyCall_0_1(_, _, _, _))
+ .WillOnce(DoAll(SetArgPointee<3>(g_hash_table_ref(properties)),
+ Return(true)));
+
RealState state(&mock_dbus, &fake_clock);
EXPECT_TRUE(state.Init());
+
+ // TODO(garnold) Remove this, too.
+ g_hash_table_unref(properties);
+
// Check that the providers are being initialized.
PMTEST_ASSERT_NOT_NULL(state.random_provider());
PMTEST_EXPECT_NOT_NULL(state.random_provider()->var_seed());
diff --git a/policy_manager/variable.h b/policy_manager/variable.h
index 2c6a8e3..be3fdb1 100644
--- a/policy_manager/variable.h
+++ b/policy_manager/variable.h
@@ -144,6 +144,7 @@
FRIEND_TEST(PmRealShillProviderTest, ReadDefaultValues);
FRIEND_TEST(PmRealShillProviderTest, ReadChangedValuesConnectedViaEthernet);
FRIEND_TEST(PmRealShillProviderTest, ReadChangedValuesConnectedViaVpn);
+ FRIEND_TEST(PmRealShillProviderTest, ReadChangedValuesConnectedTwoSignals);
Variable(const std::string& name, VariableMode mode)
: BaseVariable(name, mode) {}