PM: RealShillProvider starts even when shill is not available.
The RealShillProvider used to query shill for the default connection
status during its initialization, failing the latter if this query did
not go through. However, we would like this provider to initialize
properly even if shill is not responding (e.g. hasn't started yet). This
change relies on the assumption that, when shill comes up and detects
a connection, it will signal the detected properties via DBus and so
there's no need to reattempt querying.
That said, the RealShillProvider's variables need to be aware of the
possibility that their underlying connection values are not available,
and respond accordingly to GetValue queries. In doing so, we extend the
generic CopyVariable class to also accept a flag (Boolean), indicating
whether the underlying value object is set and available for copying.
BUG=chromium:356949
TEST=Unit tests.
Change-Id: Ibe29b6cd1fb8dce2e227418c721dbc47a75763be
Reviewed-on: https://chromium-review.googlesource.com/193748
Tested-by: Gilad Arnold <garnold@chromium.org>
Reviewed-by: Alex Vakulenko <avakulenko@chromium.org>
Reviewed-by: Alex Deymo <deymo@chromium.org>
Commit-Queue: Gilad Arnold <garnold@chromium.org>
diff --git a/policy_manager/fake_shill_provider.h b/policy_manager/fake_shill_provider.h
index 8919d16..d3e3697 100644
--- a/policy_manager/fake_shill_provider.h
+++ b/policy_manager/fake_shill_provider.h
@@ -15,18 +15,27 @@
public:
FakeShillProvider() {}
- protected:
- virtual bool DoInit() {
- set_var_is_connected(
- new FakeVariable<bool>("is_connected", kVariableModePoll));
- set_var_conn_type(
- new FakeVariable<ConnectionType>("conn_type", kVariableModePoll));
- set_var_conn_last_changed(
- new FakeVariable<base::Time>("conn_last_changed", kVariableModePoll));
- return true;
+ virtual inline FakeVariable<bool>* var_is_connected() override {
+ return &var_is_connected_;
}
+ virtual inline FakeVariable<ConnectionType>* var_conn_type() override {
+ return &var_conn_type_;
+ }
+
+ virtual inline FakeVariable<base::Time>* var_conn_last_changed() override {
+ return &var_conn_last_changed_;
+ }
+
+ protected:
+ virtual bool DoInit() { return true; }
+
private:
+ FakeVariable<bool> var_is_connected_{"is_connected", kVariableModePoll};
+ FakeVariable<ConnectionType> var_conn_type_{"conn_type", kVariableModePoll};
+ FakeVariable<base::Time> var_conn_last_changed_{"conn_last_changed",
+ kVariableModePoll};
+
DISALLOW_COPY_AND_ASSIGN(FakeShillProvider);
};
diff --git a/policy_manager/real_shill_provider.cc b/policy_manager/real_shill_provider.cc
index 90973e4..46d8958 100644
--- a/policy_manager/real_shill_provider.cc
+++ b/policy_manager/real_shill_provider.cc
@@ -17,6 +17,8 @@
namespace {
+const char* kConnInfoNotAvailErrMsg = "Connection information not available";
+
// Looks up a key in a hash table and returns the string inside of the returned
// GValue.
const char* GetStrProperty(GHashTable* hash_table, const char* key) {
@@ -144,6 +146,12 @@
// TODO(garnold) Shift to a non-blocking version, respect the timeout.
virtual const ConnectionType* GetValue(base::TimeDelta /* timeout */,
string* errmsg) {
+ if (!(provider_->is_conn_status_init_)) {
+ if (errmsg)
+ *errmsg = kConnInfoNotAvailErrMsg;
+ return NULL;
+ }
+
if (!(provider_->is_connected_)) {
if (errmsg)
*errmsg = "No connection detected";
@@ -188,25 +196,27 @@
if (!connector_->Init())
return false;
- // Read initial connection status.
+ // Attempt to read initial connection status. Even if this fails because shill
+ // is not responding (e.g. it is down) we'll be notified via "PropertyChanged"
+ // signal as soon as it comes up, so this is not a critical step.
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;
+ if (connector_->GetManagerProperties(&hash_table)) {
+ GValue* value = reinterpret_cast<GValue*>(
+ g_hash_table_lookup(hash_table, shill::kDefaultServiceProperty));
+ ProcessDefaultService(value);
+ g_hash_table_unref(hash_table);
+ }
// Initialize variables.
- set_var_is_connected(
- new CopyVariable<bool>("is_connected", kVariableModePoll, is_connected_));
- set_var_conn_type(
+ var_is_connected_.reset(
+ new CopyVariable<bool>("is_connected", kVariableModePoll, is_connected_,
+ &is_conn_status_init_, kConnInfoNotAvailErrMsg));
+ var_conn_type_.reset(
new ConnTypeVariable("conn_type", connector_.get(), this));
- set_var_conn_last_changed(
+ var_conn_last_changed_.reset(
new CopyVariable<base::Time>("conn_last_changed", kVariableModePoll,
- conn_last_changed_));
+ conn_last_changed_, &is_conn_status_init_,
+ kConnInfoNotAvailErrMsg));
return true;
}
@@ -222,6 +232,9 @@
conn_last_changed_ = clock_->GetWallclockTime();
default_service_path_ = default_service_path_str;
is_conn_type_valid_ = false;
+
+ // Mark the connection status as initialized.
+ is_conn_status_init_ = true;
return true;
}
diff --git a/policy_manager/real_shill_provider.h b/policy_manager/real_shill_provider.h
index ffb6b0c..a0619e2 100644
--- a/policy_manager/real_shill_provider.h
+++ b/policy_manager/real_shill_provider.h
@@ -91,8 +91,20 @@
RealShillProvider(DBusWrapperInterface* dbus, ClockInterface* clock)
: dbus_(dbus), clock_(clock) {}
+ virtual inline Variable<bool>* var_is_connected() override {
+ return var_is_connected_.get();
+ }
+
+ virtual inline Variable<ConnectionType>* var_conn_type() override {
+ return var_conn_type_.get();
+ }
+
+ virtual inline Variable<base::Time>* var_conn_last_changed() override {
+ return var_conn_last_changed_.get();
+ }
+
protected:
- virtual bool DoInit();
+ virtual bool DoInit() override;
private:
// Process a default connection value, update last change time as needed.
@@ -105,6 +117,9 @@
GValue* value, void* data);
+ // Whether the connection status has been properly initialized.
+ bool is_conn_status_init_ = false;
+
// The time when the connection type last changed.
base::Time conn_last_changed_;
@@ -129,6 +144,11 @@
// A clock abstraction (mockable).
ClockInterface* const clock_;
+ // Pointers to all variable objects.
+ scoped_ptr<Variable<bool>> var_is_connected_;
+ scoped_ptr<Variable<ConnectionType>> var_conn_type_;
+ scoped_ptr<Variable<base::Time>> var_conn_last_changed_;
+
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 2612a96..417fc16 100644
--- a/policy_manager/real_shill_provider_unittest.cc
+++ b/policy_manager/real_shill_provider_unittest.cc
@@ -60,18 +60,39 @@
class PmRealShillProviderTest : public ::testing::Test {
protected:
virtual void SetUp() {
+ // By default, initialize the provider so that it gets an initial connection
+ // status from shill. This simulates the common case where shill is
+ // available and respoding during RealShillProvider initialization.
+ Init(true);
+ }
+
+ virtual void TearDown() {
+ Shutdown();
+ }
+
+ // Initialize the RealShillProvider under test. If |do_init_conn_status| is
+ // true, configure mocks to respond to the initial connection status check
+ // with shill. Otherwise, the initial check will fail.
+ void Init(bool do_init_conn_status) {
+ // Properly shutdown a previously initialized provider.
+ if (provider_.get())
+ Shutdown();
+
provider_.reset(new RealShillProvider(&mock_dbus_, &fake_clock_));
PMTEST_ASSERT_NOT_NULL(provider_.get());
fake_clock_.SetWallclockTime(InitTime());
+
// 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),
@@ -83,19 +104,32 @@
.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);
+
+ // Mock a response to an initial connection check (optional).
+ GHashTable* manager_properties = nullptr;
+ if (do_init_conn_status) {
+ pair<const char*, const char*> manager_pairs[] = {
+ {shill::kDefaultServiceProperty, "/"},
+ };
+ manager_properties = SetupGetPropertiesOkay(
+ kFakeManagerProxy, arraysize(manager_pairs), manager_pairs);
+ } else {
+ SetupGetPropertiesFail(kFakeManagerProxy);
+ }
+
// Check that provider initializes corrrectly.
ASSERT_TRUE(provider_->Init());
- // Release properties hash table.
- g_hash_table_unref(manager_properties);
+
+ // Release properties hash table (if provided).
+ if (manager_properties)
+ g_hash_table_unref(manager_properties);
+
+ // Verify and clear all expectations.
+ testing::Mock::VerifyAndClear(&mock_dbus_);
}
- virtual void TearDown() {
+ // Deletes the RealShillProvider under test.
+ void Shutdown() {
// Make sure that DBus resources get freed.
EXPECT_CALL(mock_dbus_, ProxyDisconnectSignal(
kFakeManagerProxy, StrEq(shill::kMonitorPropertyChanged),
@@ -103,6 +137,7 @@
.WillOnce(Return());
EXPECT_CALL(mock_dbus_, ProxyUnref(kFakeManagerProxy)).WillOnce(Return());
provider_.reset();
+
// Verify and clear all expectations.
testing::Mock::VerifyAndClear(&mock_dbus_);
}
@@ -125,46 +160,49 @@
return InitTime() + TimeDelta::FromSeconds(10);
}
- // Sets up a mock "GetProperties" call on |proxy| that returns a hash table
- // containing |num_entries| entries formed by subsequent key/value pairs. Keys
- // and values are plain C strings (const char*). Mock will be expected to call
- // exactly once, unless |allow_multiple_calls| is true, in which case it's
- // allowed to be called multiple times. Returns a pointer to a newly allocated
- // hash table, which should be unreffed with g_hash_table_unref() when done.
- GHashTable* MockGetProperties(DBusGProxy* proxy, bool allow_multiple_calls,
- size_t num_entries,
- pair<const char*, const char*>* key_val_pairs) {
+ // Sets up a successful mock "GetProperties" call on |proxy|, writing a hash
+ // table containing |num_entries| entries formed by key/value pairs from
+ // |key_val_pairs| and returning true. Keys and values are plain C strings
+ // (const char*). The proxy call is expected to be made exactly once. Returns
+ // a pointer to a newly allocated hash table, which should be unreffed with
+ // g_hash_table_unref() when done.
+ GHashTable* SetupGetPropertiesOkay(
+ DBusGProxy* proxy, size_t num_entries,
+ pair<const char*, const char*>* key_val_pairs) {
// Allocate and populate the hash table.
- auto properties = g_hash_table_new_full(g_str_hash, g_str_equal, free,
- GValueFree);
+ GHashTable* properties = g_hash_table_new_full(g_str_hash, g_str_equal,
+ free, GValueFree);
for (size_t i = 0; i < num_entries; i++) {
g_hash_table_insert(properties, strdup(key_val_pairs[i].first),
GValueNewString(key_val_pairs[i].second));
}
- // Set mock expectation and actions.
- auto action = DoAll(SetArgPointee<3>(g_hash_table_ref(properties)),
- Return(true));
- if (allow_multiple_calls) {
- EXPECT_CALL(mock_dbus_, ProxyCall_0_1(
- proxy, StrEq(shill::kGetPropertiesFunction), _, _))
- .WillRepeatedly(action);
- } else {
- EXPECT_CALL(mock_dbus_, ProxyCall_0_1(
- proxy, StrEq(shill::kGetPropertiesFunction), _, _))
- .WillOnce(action);
- }
+ // Set mock expectations.
+ EXPECT_CALL(mock_dbus_,
+ ProxyCall_0_1(proxy, StrEq(shill::kGetPropertiesFunction),
+ _, _))
+ .WillOnce(DoAll(SetArgPointee<3>(g_hash_table_ref(properties)),
+ Return(true)));
return properties;
}
+ // Sets up a failing mock "GetProperties" call on |proxy|, returning false.
+ // The proxy call is expected to be made exactly once.
+ void SetupGetPropertiesFail(DBusGProxy* proxy) {
+ EXPECT_CALL(mock_dbus_,
+ ProxyCall_0_1(proxy, StrEq(shill::kGetPropertiesFunction),
+ _, _))
+ .WillOnce(Return(false));
+ }
+
// Programs the mock DBus interface to return an non-VPN service and ensures
// that the shill provider reads it correctly, including updating the last
// changed time on each of the queries.
- void SetConnectionAndTestType(const char* service_path,
- DBusGProxy* service_proxy,
- const char* shill_type_str,
- ConnectionType expected_conn_type) {
+ void SetupConnectionAndTestType(const char* service_path,
+ DBusGProxy* service_proxy,
+ const char* shill_type_str,
+ ConnectionType expected_conn_type) {
// Send a signal about a new default service.
auto callback = reinterpret_cast<ShillConnector::PropertyChangedHandler>(
signal_handler_);
@@ -198,9 +236,8 @@
pair<const char*, const char*> service_pairs[] = {
{shill::kTypeProperty, shill_type_str},
};
- auto service_properties = MockGetProperties(
- service_proxy, false,
- arraysize(service_pairs), service_pairs);
+ auto service_properties = SetupGetPropertiesOkay(
+ service_proxy, arraysize(service_pairs), service_pairs);
// Query the connection type, ensure last change time did not change.
scoped_ptr<const ConnectionType> conn_type(
@@ -226,8 +263,8 @@
};
// Query the connection status, type and time last changed, as they were set
-// during initialization.
-TEST_F(PmRealShillProviderTest, ReadDefaultValues) {
+// during initialization (no signals).
+TEST_F(PmRealShillProviderTest, ReadBaseValues) {
// Query the provider variables.
scoped_ptr<const bool> is_connected(
provider_->var_is_connected()->GetValue(default_timeout_, NULL));
@@ -246,50 +283,51 @@
// Test that Ethernet connection is identified correctly.
TEST_F(PmRealShillProviderTest, ReadChangedValuesConnectedViaEthernet) {
- SetConnectionAndTestType(kFakeEthernetServicePath,
- kFakeEthernetServiceProxy,
- shill::kTypeEthernet,
- ConnectionType::kEthernet);
+ SetupConnectionAndTestType(kFakeEthernetServicePath,
+ kFakeEthernetServiceProxy,
+ shill::kTypeEthernet,
+ ConnectionType::kEthernet);
}
// Test that Wifi connection is identified correctly.
TEST_F(PmRealShillProviderTest, ReadChangedValuesConnectedViaWifi) {
- SetConnectionAndTestType(kFakeWifiServicePath,
- kFakeWifiServiceProxy,
- shill::kTypeWifi,
- ConnectionType::kWifi);
+ SetupConnectionAndTestType(kFakeWifiServicePath,
+ kFakeWifiServiceProxy,
+ shill::kTypeWifi,
+ ConnectionType::kWifi);
}
// Test that Wimax connection is identified correctly.
TEST_F(PmRealShillProviderTest, ReadChangedValuesConnectedViaWimax) {
- SetConnectionAndTestType(kFakeWimaxServicePath,
- kFakeWimaxServiceProxy,
- shill::kTypeWimax,
- ConnectionType::kWimax);
+ SetupConnectionAndTestType(kFakeWimaxServicePath,
+ kFakeWimaxServiceProxy,
+ shill::kTypeWimax,
+ ConnectionType::kWimax);
}
// Test that Bluetooth connection is identified correctly.
TEST_F(PmRealShillProviderTest, ReadChangedValuesConnectedViaBluetooth) {
- SetConnectionAndTestType(kFakeBluetoothServicePath,
- kFakeBluetoothServiceProxy,
- shill::kTypeBluetooth,
- ConnectionType::kBluetooth);
+ SetupConnectionAndTestType(kFakeBluetoothServicePath,
+ kFakeBluetoothServiceProxy,
+ shill::kTypeBluetooth,
+ ConnectionType::kBluetooth);
}
// Test that Cellular connection is identified correctly.
TEST_F(PmRealShillProviderTest, ReadChangedValuesConnectedViaCellular) {
- SetConnectionAndTestType(kFakeCellularServicePath,
- kFakeCellularServiceProxy,
- shill::kTypeCellular,
- ConnectionType::kCellular);
+ SetupConnectionAndTestType(kFakeCellularServicePath,
+ kFakeCellularServiceProxy,
+ shill::kTypeCellular,
+ ConnectionType::kCellular);
}
// Test that an unknown connection is identified as such.
-TEST_F(PmRealShillProviderTest, ReadChangedValuesConnectedViaUnknown) {
- SetConnectionAndTestType(kFakeUnknownServicePath,
- kFakeUnknownServiceProxy,
- "FooConnectionType",
- ConnectionType::kUnknown);
+TEST_F(PmRealShillProviderTest,
+ ReadChangedValuesConnectedViaUnknown) {
+ SetupConnectionAndTestType(kFakeUnknownServicePath,
+ kFakeUnknownServiceProxy,
+ "FooConnectionType",
+ ConnectionType::kUnknown);
}
// Tests that VPN connection is identified correctly.
@@ -315,8 +353,9 @@
{shill::kTypeProperty, shill::kTypeVPN},
{shill::kPhysicalTechnologyProperty, shill::kTypeWifi},
};
- auto service_properties = MockGetProperties(
- kFakeVpnServiceProxy, false, arraysize(service_pairs), service_pairs);
+ auto service_properties = SetupGetPropertiesOkay(kFakeVpnServiceProxy,
+ arraysize(service_pairs),
+ service_pairs);
// Query the connection type, ensure last change time reported correctly.
scoped_ptr<const ConnectionType> conn_type(
@@ -363,4 +402,30 @@
EXPECT_EQ(conn_change_time, *conn_last_changed);
}
+// Make sure that the provider initializes correctly even if shill is not
+// responding, that variables can be obtained, and that they all return a null
+// value (indicating that the underlying values were not set).
+TEST_F(PmRealShillProviderTest, NoInitConnStatusReadBaseValues) {
+ // Re-initialize the provider, no initial connection status response.
+ Init(false);
+ PMTEST_ASSERT_NULL(provider_->var_is_connected()->GetValue(
+ default_timeout_, NULL));
+ PMTEST_ASSERT_NULL(provider_->var_conn_type()->GetValue(
+ default_timeout_, NULL));
+ PMTEST_ASSERT_NULL(provider_->var_conn_last_changed()->GetValue(
+ default_timeout_, NULL));
+}
+
+// Test that, once a signal is received, the connection status and other info
+// can be read correctly.
+TEST_F(PmRealShillProviderTest,
+ NoInitConnStatusReadChangedValuesConnectedViaEthernet) {
+ // Re-initialize the provider, no initial connection status response.
+ Init(false);
+ SetupConnectionAndTestType(kFakeEthernetServicePath,
+ kFakeEthernetServiceProxy,
+ shill::kTypeEthernet,
+ ConnectionType::kEthernet);
+}
+
} // namespace chromeos_policy_manager
diff --git a/policy_manager/shill_provider.h b/policy_manager/shill_provider.h
index 4fbfedd..7b87903 100644
--- a/policy_manager/shill_provider.h
+++ b/policy_manager/shill_provider.h
@@ -26,42 +26,17 @@
class ShillProvider : public Provider {
public:
// Returns whether we currently have network connectivity.
- Variable<bool>* var_is_connected() const {
- return var_is_connected_.get();
- }
+ virtual Variable<bool>* var_is_connected() = 0;
// Returns the current network connection type. Unknown if not connected.
- Variable<ConnectionType>* var_conn_type() const {
- return var_conn_type_.get();
- }
+ virtual Variable<ConnectionType>* var_conn_type() = 0;
// Returns the time when network connection last changed; initialized to
// current time.
- Variable<base::Time>* var_conn_last_changed() const {
- return var_conn_last_changed_.get();
- }
+ virtual Variable<base::Time>* var_conn_last_changed() = 0;
protected:
ShillProvider() {}
-
- void set_var_is_connected(Variable<bool>* var_is_connected) {
- var_is_connected_.reset(var_is_connected);
- }
-
- void set_var_conn_type(Variable<ConnectionType>* var_conn_type) {
- var_conn_type_.reset(var_conn_type);
- }
-
- void set_var_conn_last_changed(Variable<base::Time>* var_conn_last_changed) {
- var_conn_last_changed_.reset(var_conn_last_changed);
- }
-
- private:
- scoped_ptr<Variable<bool>> var_is_connected_;
- scoped_ptr<Variable<ConnectionType>> var_conn_type_;
- scoped_ptr<Variable<base::Time>> var_conn_last_changed_;
-
- DISALLOW_COPY_AND_ASSIGN(ShillProvider);
};
} // namespace chromeos_policy_manager
diff --git a/policy_manager/variable.h b/policy_manager/variable.h
index da8eee9..2ef0fe1 100644
--- a/policy_manager/variable.h
+++ b/policy_manager/variable.h
@@ -170,10 +170,10 @@
friend class PmRealRandomProviderTest;
FRIEND_TEST(PmRealRandomProviderTest, GetRandomValues);
friend class PmRealShillProviderTest;
- FRIEND_TEST(PmRealShillProviderTest, ReadDefaultValues);
- FRIEND_TEST(PmRealShillProviderTest, ReadChangedValuesConnectedViaEthernet);
+ FRIEND_TEST(PmRealShillProviderTest, ReadBaseValues);
FRIEND_TEST(PmRealShillProviderTest, ReadChangedValuesConnectedViaVpn);
FRIEND_TEST(PmRealShillProviderTest, ReadChangedValuesConnectedTwoSignals);
+ FRIEND_TEST(PmRealShillProviderTest, NoInitConnStatusReadBaseValues);
friend class PmRealTimeProviderTest;
FRIEND_TEST(PmRealTimeProviderTest, CurrDateValid);
FRIEND_TEST(PmRealTimeProviderTest, CurrHourValid);