shill: Create an IPAddress store in Manager for ConnectionHealthChecker
(1) Added class IPAddressStore
(2) Added an IPAddressStore object to Manager
(3) ConnectionHealthChecker uses this object so that the store persists
across Device resets.
BUG=chromium:232883
TEST=(1)build and run unit-tests.
(2)Check that DHCP renewal works -- connect the device to the same
wifi network twice and verify wifi is connected.
Change-Id: Ia5ee314db46c3de2e037f59c1f99a248c80e4ef9
Reviewed-on: https://gerrit.chromium.org/gerrit/49487
Reviewed-by: Prathmesh Prabhu <pprabhu@chromium.org>
Tested-by: Prathmesh Prabhu <pprabhu@chromium.org>
Commit-Queue: Prathmesh Prabhu <pprabhu@chromium.org>
diff --git a/Makefile b/Makefile
index 1c51d4c..89a8b9a 100644
--- a/Makefile
+++ b/Makefile
@@ -249,6 +249,7 @@
http_request.o \
http_url.o \
ip_address.o \
+ ip_address_store.o \
ipconfig.o \
ipconfig_dbus_adaptor.o \
key_file_store.o \
@@ -347,10 +348,10 @@
crypto_provider_unittest.o \
crypto_rot47_unittest.o \
crypto_util_proxy_unittest.o \
- connection_unittest.o \
connection_health_checker_unittest.o \
connection_info_reader_unittest.o \
connection_info_unittest.o \
+ connection_unittest.o \
dbus_adaptor_unittest.o \
dbus_manager_unittest.o \
dbus_properties_unittest.o \
@@ -375,6 +376,7 @@
http_request_unittest.o \
http_url_unittest.o \
ip_address_unittest.o \
+ ip_address_store_unittest.o \
ipconfig_unittest.o \
key_file_store_unittest.o \
key_value_store_unittest.o \
@@ -412,6 +414,7 @@
mock_event_dispatcher.o \
mock_glib.o \
mock_http_request.o \
+ mock_ip_address_store.o \
mock_ipconfig.o \
mock_link_monitor.o \
mock_log.o \
diff --git a/connection_health_checker.cc b/connection_health_checker.cc
index c43d9e4..750747a 100644
--- a/connection_health_checker.cc
+++ b/connection_health_checker.cc
@@ -22,6 +22,7 @@
#include "shill/error.h"
#include "shill/http_url.h"
#include "shill/ip_address.h"
+#include "shill/ip_address_store.h"
#include "shill/logging.h"
#include "shill/sockets.h"
#include "shill/socket_info.h"
@@ -35,6 +36,13 @@
namespace shill {
// static
+const char *ConnectionHealthChecker::kDefaultRemoteIPPool[] = {
+ "74.125.224.47",
+ "74.125.224.79",
+ "74.125.224.111",
+ "74.125.224.143"
+};
+// static
const int ConnectionHealthChecker::kDNSTimeoutMilliseconds = 5000;
// static
const int ConnectionHealthChecker::kInvalidSocket = -1;
@@ -56,9 +64,11 @@
ConnectionHealthChecker::ConnectionHealthChecker(
ConnectionRefPtr connection,
EventDispatcher *dispatcher,
+ IPAddressStore *remote_ips,
const base::Callback<void(Result)> &result_callback)
: connection_(connection),
dispatcher_(dispatcher),
+ remote_ips_(remote_ips),
result_callback_(result_callback),
socket_(new Sockets()),
weak_ptr_factory_(this),
@@ -80,7 +90,14 @@
health_check_in_progress_(false),
num_connection_failures_(0),
num_congested_queue_detected_(0),
- num_successful_sends_(0) {}
+ num_successful_sends_(0) {
+ for (size_t i = 0; i < arraysize(kDefaultRemoteIPPool); ++i) {
+ const char *ip_string = kDefaultRemoteIPPool[i];
+ IPAddress ip(IPAddress::kFamilyIPv4);
+ ip.SetAddressFromString(ip_string);
+ remote_ips_->AddUnique(ip);
+ }
+}
ConnectionHealthChecker::~ConnectionHealthChecker() {
Stop();
@@ -91,7 +108,7 @@
}
void ConnectionHealthChecker::AddRemoteIP(IPAddress ip) {
- remote_ips_.push_back(ip);
+ remote_ips_->AddUnique(ip);
}
void ConnectionHealthChecker::AddRemoteURL(const string &url_string) {
@@ -141,7 +158,7 @@
num_congested_queue_detected_ = 0;
num_successful_sends_ = 0;
- if (remote_ips_.empty()) {
+ if (remote_ips_->Empty()) {
// Nothing to try.
Stop();
SLOG(Connection, 2) << __func__ << ": Not enough IPs.";
@@ -154,7 +171,7 @@
}
void ConnectionHealthChecker::Stop() {
- if (tcp_connection_ != NULL)
+ if (tcp_connection_.get() != NULL)
tcp_connection_->Stop();
verify_sent_data_callback_.Cancel();
ClearSocketDescriptor();
@@ -165,6 +182,20 @@
num_tx_queue_polling_attempts_ = 0;
}
+void ConnectionHealthChecker::SetConnection(ConnectionRefPtr connection) {
+ SLOG(Connection, 3) << __func__;
+ connection_ = connection;
+ tcp_connection_.reset(new AsyncConnection(connection_->interface_name(),
+ dispatcher_,
+ socket_.get(),
+ connection_complete_callback_));
+ dns_clients_.clear();
+ bool restart = health_check_in_progress();
+ Stop();
+ if (restart)
+ Start();
+}
+
const char *ConnectionHealthChecker::ResultToString(
ConnectionHealthChecker::Result result) {
switch(result) {
@@ -192,11 +223,7 @@
<< error.message();
return;
}
- // Insert ip into the list of cached IP addresses, if not already present.
- for (IPAddresses::size_type i = 0; i < remote_ips_.size(); ++i)
- if (remote_ips_[i].Equals(ip))
- return;
- remote_ips_.push_back(ip);
+ remote_ips_->AddUnique(ip);
}
void ConnectionHealthChecker::GarbageCollectDNSClients() {
@@ -235,8 +262,7 @@
// This guards against
// (1) Repeated failed attempts for the same IP at start-up everytime.
// (2) All users attempting to connect to the same IP.
- int next_ip_index = rand() % remote_ips_.size();
- const IPAddress &ip = remote_ips_[next_ip_index];
+ IPAddress ip = remote_ips_->GetRandomIP();
SLOG(Connection, 3) << __func__ << ": Starting connection at "
<< ip.ToString();
if (!tcp_connection_->Start(ip, kRemotePort)) {
@@ -249,7 +275,9 @@
void ConnectionHealthChecker::OnConnectionComplete(bool success, int sock_fd) {
if (!success) {
SLOG(Connection, 2) << __func__
- << ": AsyncConnection connection attempt failed.";
+ << ": AsyncConnection connection attempt failed "
+ << "with error: "
+ << tcp_connection_->error();
++num_connection_failures_;
NextHealthCheckSample();
return;
diff --git a/connection_health_checker.h b/connection_health_checker.h
index 6bc870e..3efef07 100644
--- a/connection_health_checker.h
+++ b/connection_health_checker.h
@@ -27,6 +27,7 @@
class Error;
class EventDispatcher;
class IPAddress;
+class IPAddressStore;
class SocketInfoReader;
// The ConnectionHealthChecker class implements the facilities to test
@@ -39,7 +40,6 @@
// -(3)- Connectivity OK (TCP connection established, is healthy)
class ConnectionHealthChecker {
public:
- typedef std::vector<IPAddress> IPAddresses;
enum Result {
// There was some problem in the setup of ConnctionHealthChecker.
@@ -64,6 +64,7 @@
ConnectionHealthChecker(ConnectionRefPtr connection,
EventDispatcher *dispatcher,
+ IPAddressStore *remote_ips,
const base::Callback<void(Result)> &result_callback);
virtual ~ConnectionHealthChecker();
@@ -75,6 +76,11 @@
// to attempt the TCP connection with.
virtual void AddRemoteIP(IPAddress ip);
+ // Change the associated Connection on the Device.
+ // This will restart any ongoing health check. Any ongoing DNS query will be
+ // dropped (not restarted).
+ virtual void SetConnection(ConnectionRefPtr connection);
+
// Start a connection health check. The health check involves one or more
// attempts at establishing and using a TCP connection. |result_callback_| is
// called with the final result of the check. |result_callback_| will always
@@ -93,10 +99,9 @@
static const char *ResultToString(Result result);
// Accessors.
- const IPAddresses &remote_ips() const { return remote_ips_; }
+ const IPAddressStore *remote_ips() const { return remote_ips_; }
virtual bool health_check_in_progress() const;
-
protected:
// For unit-tests.
void set_dispatcher(EventDispatcher *dispatcher) {
@@ -127,14 +132,20 @@
old_transmit_queue_value_ = val;
}
Result health_check_result() const { return health_check_result_; }
+ AsyncConnection *tcp_connection() const { return tcp_connection_.get(); }
+ Connection *connection() const { return connection_.get(); }
+
private:
friend class ConnectionHealthCheckerTest;
FRIEND_TEST(ConnectionHealthCheckerTest, GarbageCollectDNSClients);
FRIEND_TEST(ConnectionHealthCheckerTest, GetSocketInfo);
FRIEND_TEST(ConnectionHealthCheckerTest, NextHealthCheckSample);
FRIEND_TEST(ConnectionHealthCheckerTest, OnConnectionComplete);
+ FRIEND_TEST(ConnectionHealthCheckerTest, SetConnection);
FRIEND_TEST(ConnectionHealthCheckerTest, VerifySentData);
+ // List of static IPs for connection health check.
+ static const char *kDefaultRemoteIPPool[];
// Time to wait for DNS server.
static const int kDNSTimeoutMilliseconds;
static const int kInvalidSocket;
@@ -152,12 +163,11 @@
static const int kMinSuccessfulSendAttempts;
// Number of DNS queries to be spawned when a new remote URL is added.
static const int kNumDNSQueries;
+ static const uint16 kRemotePort;
// Time to wait before testing successful data transfer / disconnect after
// request is made on the device.
static const int kTCPStateUpdateWaitMilliseconds;
- static const uint16 kRemotePort;
-
// Callback for DnsClient
void GetDNSResult(const Error &error, const IPAddress &ip);
void GarbageCollectDNSClients();
@@ -179,15 +189,13 @@
// The connection on which the health check is being run.
ConnectionRefPtr connection_;
EventDispatcher *dispatcher_;
-
- // The client function to call to report the result.
+ // Set of IPs to create TCP connection with for the health check.
+ IPAddressStore *remote_ips_;
base::Callback<void(Result)> result_callback_;
scoped_ptr<Sockets> socket_;
base::WeakPtrFactory<ConnectionHealthChecker> weak_ptr_factory_;
- // Set of IPs to create TCP connection with for the health check.
- IPAddresses remote_ips_;
// Callback passed to |tcp_connection_| to report an established TCP
// connection.
const base::Callback<void(bool, int)> connection_complete_callback_;
diff --git a/connection_health_checker_unittest.cc b/connection_health_checker_unittest.cc
index 18c102c..12f33b5 100644
--- a/connection_health_checker_unittest.cc
+++ b/connection_health_checker_unittest.cc
@@ -22,6 +22,7 @@
#include "shill/mock_dns_client_factory.h"
#include "shill/mock_device_info.h"
#include "shill/mock_event_dispatcher.h"
+#include "shill/mock_ip_address_store.h"
#include "shill/mock_sockets.h"
#include "shill/mock_socket_info_reader.h"
@@ -31,6 +32,7 @@
using base::Unretained;
using std::string;
using std::vector;
+using ::testing::AtLeast;
using ::testing::DoAll;
using ::testing::Gt;
using ::testing::Invoke;
@@ -122,13 +124,16 @@
.WillRepeatedly(ReturnRef(interface_name_));
ON_CALL(*connection_.get(), dns_servers())
.WillByDefault(ReturnRef(dns_servers_));
+ // ConnectionHealthChecker constructor should add some IPs
+ EXPECT_CALL(remote_ips_, AddUnique(_)).Times(AtLeast(1));
health_checker_.reset(
new ConnectionHealthChecker(
connection_,
&dispatcher_,
+ &remote_ips_,
Bind(&ConnectionHealthCheckerTest::ResultCallbackTarget,
Unretained(this))));
-
+ Mock::VerifyAndClearExpectations(&remote_ips_);
socket_ = new StrictMock<MockSockets>();
tcp_connection_ = new StrictMock<MockAsyncConnection>();
socket_info_reader_ = new StrictMock<MockSocketInfoReader>();
@@ -140,8 +145,7 @@
}
void TearDown() {
- EXPECT_CALL(*tcp_connection_, Stop())
- .Times(1);
+ ExpectStop();
}
// Accessors for private data in ConnectionHealthChecker.
@@ -154,9 +158,6 @@
ScopedVector<DNSClient> &dns_clients() {
return health_checker_->dns_clients_;
}
- ConnectionHealthChecker::IPAddresses &remote_ips() {
- return health_checker_->remote_ips_;
- }
int NumDNSQueries() {
return ConnectionHealthChecker::kNumDNSQueries;
}
@@ -238,6 +239,7 @@
EXPECT_FALSE(tcp_connection_ == NULL);
EXPECT_FALSE(health_checker_->health_check_in_progress_);
}
+
// Setup ConnectionHealthChecker::GetSocketInfo to return sock_info.
// This only works if GetSocketInfo is called with kProxyFD.
// If no matching sock_info is provided (Does not belong to proxy socket),
@@ -256,6 +258,9 @@
}
void ExpectSuccessfulStart() {
+ EXPECT_CALL(remote_ips_, Empty()).WillRepeatedly(Return(false));
+ EXPECT_CALL(remote_ips_, GetRandomIP())
+ .WillRepeatedly(Return(StringToIPv4Address(kProxyIPAddressRemote)));
EXPECT_CALL(
*tcp_connection_,
Start(IsSameIPAddress(StringToIPv4Address(kProxyIPAddressRemote)),
@@ -265,7 +270,6 @@
}
void ExpectRetry() {
EXPECT_CALL(*socket_, Close(kProxyFD))
- .Times(1)
.InSequence(seq_);
EXPECT_CALL(
*tcp_connection_,
@@ -275,15 +279,14 @@
.WillOnce(Return(true));
}
void ExpectStop() {
- EXPECT_CALL(*tcp_connection_, Stop())
- .InSequence(seq_);
+ if (tcp_connection_)
+ EXPECT_CALL(*tcp_connection_, Stop())
+ .InSequence(seq_);
}
void ExpectCleanUp() {
EXPECT_CALL(*socket_, Close(kProxyFD))
- .Times(1)
.InSequence(seq_);
EXPECT_CALL(*tcp_connection_, Stop())
- .Times(1)
.InSequence(seq_);
}
@@ -302,6 +305,7 @@
scoped_refptr<NiceMock<MockConnection> > connection_;
EventDispatcher dispatcher_;
+ MockIPAddressStore remote_ips_;
StrictMock<MockSockets> *socket_;
StrictMock<MockSocketInfoReader> *socket_info_reader_;
StrictMock<MockAsyncConnection> *tcp_connection_;
@@ -316,17 +320,27 @@
ExpectReset();
}
-TEST_F(ConnectionHealthCheckerTest, AddRemoteIP) {
- EXPECT_EQ(0, remote_ips().size());
+TEST_F(ConnectionHealthCheckerTest, SetConnection) {
+ scoped_refptr<NiceMock<MockConnection> > new_connection
+ = new NiceMock<MockConnection>(&device_info_);
+ // If a health check was in progress when SetConnection is called, verify
+ // that it restarts with the new connection.
+ ExpectSuccessfulStart();
+ health_checker_->Start();
+ VerifyAndClearAllExpectations();
- IPAddress ip = StringToIPv4Address(kIPAddress_0_0_0_0);
- health_checker_->AddRemoteIP(ip);
- EXPECT_EQ(1, remote_ips().size());
- EXPECT_TRUE(remote_ips().rbegin()->Equals(ip));
+ EXPECT_CALL(remote_ips_, Empty()).WillRepeatedly(Return(true));
+ EXPECT_CALL(*new_connection.get(), interface_name())
+ .WillRepeatedly(ReturnRef(interface_name_));
+ EXPECT_CALL(*this,
+ ResultCallbackTarget(ConnectionHealthChecker::kResultUnknown));
+ health_checker_->SetConnection(new_connection);
+ EXPECT_NE(tcp_connection_, health_checker_->tcp_connection());
+ EXPECT_EQ(new_connection.get(), health_checker_->connection());
- health_checker_->AddRemoteIP(
- StringToIPv4Address(kIPAddress_0_0_0_0));
- EXPECT_EQ(2, remote_ips().size());
+ // health_checker_ has reset tcp_connection_ to a new object.
+ // Since it owned tcp_connection_, the object has been destroyed.
+ tcp_connection_ = NULL;
}
TEST_F(ConnectionHealthCheckerTest, GarbageCollectDNSClients) {
@@ -369,7 +383,6 @@
MockDNSClientFactory *dns_client_factory
= MockDNSClientFactory::GetInstance();
- ConnectionHealthChecker::IPAddresses::size_type num_old_ips;
vector<MockDNSClient *> dns_client_buffer;
// All DNS queries fail.
@@ -385,10 +398,10 @@
.InSequence(seq_)
.WillOnce(Return(dns_client_buffer[i]));
}
- num_old_ips = remote_ips().size();
+ EXPECT_CALL(remote_ips_, AddUnique(_)).Times(0);
health_checker_->AddRemoteURL(kProxyURLRemote);
- EXPECT_EQ(num_old_ips, remote_ips().size());
Mock::VerifyAndClearExpectations(dns_client_factory);
+ Mock::VerifyAndClearExpectations(&remote_ips_);
dns_client_buffer.clear();
dns_clients().clear();
@@ -405,15 +418,14 @@
.InSequence(seq_)
.WillOnce(Return(dns_client_buffer[i]));
}
- num_old_ips = remote_ips().size();
+ EXPECT_CALL(remote_ips_, AddUnique(_));
health_checker_->AddRemoteURL(kProxyURLRemote);
for(int i = 0; i < NumDNSQueries() - 1; ++i) {
InvokeGetDNSResultFailure();
}
InvokeGetDNSResultSuccess(remote_ip);
- EXPECT_EQ(num_old_ips + 1, remote_ips().size());
- EXPECT_TRUE(remote_ip.Equals(*remote_ips().rbegin()));
Mock::VerifyAndClearExpectations(dns_client_factory);
+ Mock::VerifyAndClearExpectations(&remote_ips_);
dns_client_buffer.clear();
dns_clients().clear();
@@ -430,15 +442,15 @@
.InSequence(seq_)
.WillOnce(Return(dns_client_buffer[i]));
}
- remote_ips().clear();
- num_old_ips = remote_ips().size();
+ EXPECT_CALL(remote_ips_, AddUnique(IsSameIPAddress(remote_ip))).Times(4);
+ EXPECT_CALL(remote_ips_, AddUnique(IsSameIPAddress(remote_ip_2)));
health_checker_->AddRemoteURL(kProxyURLRemote);
for (int i = 0; i < NumDNSQueries() - 1; ++i) {
InvokeGetDNSResultSuccess(remote_ip);
}
InvokeGetDNSResultSuccess(remote_ip_2);
- EXPECT_EQ(num_old_ips + 2, remote_ips().size());
Mock::VerifyAndClearExpectations(dns_client_factory);
+ Mock::VerifyAndClearExpectations(&remote_ips_);
dns_client_buffer.clear();
dns_clients().clear();
}
@@ -448,14 +460,12 @@
vector<SocketInfo> info_list;
// GetSockName fails.
- LOG(INFO) << "ConnectionHealthCheckerTest::GetSocketInfo::1";
EXPECT_CALL(*socket_, GetSockName(_,_,_))
.WillOnce(Return(-1));
EXPECT_FALSE(health_checker_->GetSocketInfo(kProxyFD, &sock_info));
Mock::VerifyAndClearExpectations(socket_);
// GetSockName returns IPv6.
- LOG(INFO) << "ConnectionHealthCheckerTest::GetSocketInfo::2";
EXPECT_CALL(*socket_, GetSockName(_,_,_))
.WillOnce(
Invoke(this,
@@ -464,7 +474,6 @@
Mock::VerifyAndClearExpectations(socket_);
// LoadTcpSocketInfo fails.
- LOG(INFO) << "ConnectionHealthCheckerTest::GetSocketInfo::3";
EXPECT_CALL(*socket_, GetSockName(kProxyFD, _, _))
.WillOnce(Invoke(this, &ConnectionHealthCheckerTest::GetSockName));
EXPECT_CALL(*socket_info_reader_, LoadTcpSocketInfo(_))
@@ -474,7 +483,6 @@
Mock::VerifyAndClearExpectations(socket_info_reader_);
// LoadTcpSocketInfo returns empty list.
- LOG(INFO) << "ConnectionHealthCheckerTest::GetSocketInfo::4";
info_list.clear();
EXPECT_CALL(*socket_, GetSockName(kProxyFD, _, _))
.WillOnce(Invoke(this, &ConnectionHealthCheckerTest::GetSockName));
@@ -486,7 +494,6 @@
Mock::VerifyAndClearExpectations(socket_info_reader_);
// LoadTcpSocketInfo returns a list without our socket.
- LOG(INFO) << "ConnectionHealthCheckerTest::GetSocketInfo::5";
info_list.clear();
info_list.push_back(CreateSocketInfoOther());
EXPECT_CALL(*socket_, GetSockName(kProxyFD, _, _))
@@ -499,7 +506,6 @@
Mock::VerifyAndClearExpectations(socket_info_reader_);
// LoadTcpSocketInfo returns a list with only our socket.
- LOG(INFO) << "ConnectionHealthCheckerTest::GetSocketInfo::6";
info_list.clear();
info_list.push_back(
CreateSocketInfoProxy(SocketInfo::kConnectionStateUnknown));
@@ -515,7 +521,6 @@
Mock::VerifyAndClearExpectations(socket_info_reader_);
// LoadTcpSocketInfo returns a list with two sockets, including ours.
- LOG(INFO) << "ConnectionHealthCheckerTest::GetSocketInfo::7";
info_list.clear();
info_list.push_back(CreateSocketInfoOther());
info_list.push_back(
@@ -531,7 +536,6 @@
Mock::VerifyAndClearExpectations(socket_);
Mock::VerifyAndClearExpectations(socket_info_reader_);
- LOG(INFO) << "ConnectionHealthCheckerTest::GetSocketInfo::8";
info_list.clear();
info_list.push_back(
CreateSocketInfoProxy(SocketInfo::kConnectionStateUnknown));
@@ -549,7 +553,9 @@
}
TEST_F(ConnectionHealthCheckerTest, NextHealthCheckSample) {
- health_checker_->AddRemoteIP(StringToIPv4Address(kProxyIPAddressRemote));
+ IPAddress ip = StringToIPv4Address(kProxyIPAddressRemote);
+ ON_CALL(remote_ips_, GetRandomIP())
+ .WillByDefault(Return(ip));
health_checker_->set_num_connection_failures(MaxFailedConnectionAttempts());
ExpectStop();
@@ -700,7 +706,9 @@
TEST_F(ConnectionHealthCheckerTest, StartStartSkipsSecond) {
EXPECT_CALL(*tcp_connection_, Start(_,_))
.WillOnce(Return(true));
- health_checker_->AddRemoteIP(StringToIPv4Address(kProxyIPAddressRemote));
+ EXPECT_CALL(remote_ips_, Empty()).WillRepeatedly(Return(false));
+ EXPECT_CALL(remote_ips_, GetRandomIP())
+ .WillOnce(Return(StringToIPv4Address(kProxyIPAddressRemote)));
health_checker_->Start();
health_checker_->Start();
}
@@ -711,11 +719,12 @@
TEST_F(ConnectionHealthCheckerTest, StartStopNoCallback) {
EXPECT_CALL(*tcp_connection_, Start(_,_))
.WillOnce(Return(true));
- EXPECT_CALL(*tcp_connection_, Stop())
- .Times(1);
+ EXPECT_CALL(*tcp_connection_, Stop());
EXPECT_CALL(*this, ResultCallbackTarget(_))
.Times(0);
- health_checker_->AddRemoteIP(StringToIPv4Address(kProxyIPAddressRemote));
+ EXPECT_CALL(remote_ips_, Empty()).WillRepeatedly(Return(false));
+ EXPECT_CALL(remote_ips_, GetRandomIP())
+ .WillOnce(Return(StringToIPv4Address(kProxyIPAddressRemote)));
health_checker_->Start();
health_checker_->Stop();
}
@@ -724,31 +733,30 @@
// Flow: Start()
// Expectation: call |result_callback| with kResultUnknown
TEST_F(ConnectionHealthCheckerTest, StartImmediateFailure) {
- EXPECT_CALL(*tcp_connection_, Stop())
- .Times(1);
+ EXPECT_CALL(remote_ips_, Empty()).WillOnce(Return(true));
+ EXPECT_CALL(*tcp_connection_, Stop());
EXPECT_CALL(*this, ResultCallbackTarget(
- ConnectionHealthChecker::kResultUnknown))
- .Times(1);
+ ConnectionHealthChecker::kResultUnknown));
health_checker_->Start();
Mock::VerifyAndClearExpectations(this);
+ Mock::VerifyAndClearExpectations(&remote_ips_);
Mock::VerifyAndClearExpectations(tcp_connection_);
- health_checker_->AddRemoteIP(StringToIPv4Address(kProxyIPAddressRemote));
+ EXPECT_CALL(remote_ips_, Empty()).WillRepeatedly(Return(false));
+ EXPECT_CALL(remote_ips_, GetRandomIP())
+ .WillRepeatedly(Return(StringToIPv4Address(kProxyIPAddressRemote)));
EXPECT_CALL(*tcp_connection_,
Start(IsSameIPAddress(StringToIPv4Address(kProxyIPAddressRemote)),
kProxyPortRemote))
.Times(MaxFailedConnectionAttempts())
.WillRepeatedly(Return(false));
- EXPECT_CALL(*tcp_connection_, Stop())
- .Times(1);
+ EXPECT_CALL(*tcp_connection_, Stop());
EXPECT_CALL(*this, ResultCallbackTarget(
- ConnectionHealthChecker::kResultConnectionFailure))
- .Times(1);
+ ConnectionHealthChecker::kResultConnectionFailure));
health_checker_->Start();
dispatcher_.DispatchPendingEvents();
Mock::VerifyAndClearExpectations(this);
Mock::VerifyAndClearExpectations(tcp_connection_);
}
-
} // namespace shill
diff --git a/device.cc b/device.cc
index 7bb92e3..5a21384 100644
--- a/device.cc
+++ b/device.cc
@@ -71,10 +71,10 @@
// static
const char Device::kIPFlagReversePathFilterLooseMode[] = "2";
// static
-const char Device::kStoragePowered[] = "Powered";
-// static
const char Device::kStorageIPConfigs[] = "IPConfigs";
// static
+const char Device::kStoragePowered[] = "Powered";
+// static
const char Device::kStorageReceiveByteCount[] = "ReceiveByteCount";
// static
const char Device::kStorageTransmitByteCount[] = "TransmitByteCount";
@@ -166,13 +166,6 @@
&Device::GetTransmitByteCountProperty);
}
- // TODO(armansito): Remove this hack once shill has support for caching DNS
- // results.
- health_check_ip_pool_.push_back("74.125.224.47");
- health_check_ip_pool_.push_back("74.125.224.79");
- health_check_ip_pool_.push_back("74.125.224.111");
- health_check_ip_pool_.push_back("74.125.224.143");
-
LOG(INFO) << "Device created: " << link_name_
<< " index " << interface_index_;
}
@@ -492,6 +485,7 @@
}
StartLinkMonitor();
StartTrafficMonitor();
+ SetupConnectionHealthChecker();
} else {
// TODO(pstew): This logic gets yet more complex when multiple
// IPConfig types are run in parallel (e.g. DHCP and DHCP6)
@@ -546,26 +540,6 @@
link_name_,
technology_,
manager_->device_info());
- set_health_checker(
- new ConnectionHealthChecker(
- connection_,
- dispatcher_,
- Bind(&Device::OnConnectionHealthCheckerResult,
- weak_ptr_factory_.GetWeakPtr())));
- InitializeHealthCheckIps();
- }
-}
-
-void Device::InitializeHealthCheckIps() {
- IPAddress ip(IPAddress::kFamilyIPv4);
- for (size_t i = 0; i < health_check_ip_pool_.size(); ++i) {
- // TODO(armansito): DNS resolution fails when we run out-of-credit
- // and shill currently doesn't cache resolved IPs. The hardcoded IP
- // here corresponds to gstatic.google.com/generate_204, however I do
- // not know if it will remain static. We should implement some sort of
- // caching after portal detection for this to work.
- ip.SetAddressFromString(health_check_ip_pool_[i]);
- health_checker_->AddRemoteIP(ip);
}
}
@@ -655,6 +629,23 @@
manager_->UpdateDevice(this);
}
+void Device::SetupConnectionHealthChecker() {
+ DCHECK(connection_);
+ if (!health_checker_.get()) {
+ health_checker_.reset(new ConnectionHealthChecker(
+ connection_,
+ dispatcher_,
+ manager_->health_checker_remote_ips(),
+ Bind(&Device::OnConnectionHealthCheckerResult,
+ weak_ptr_factory_.GetWeakPtr())));
+ } else {
+ health_checker_->SetConnection(connection_);
+ }
+ // Add URL in either case because a connection reset could have dropped past
+ // DNS queries.
+ health_checker_->AddRemoteURL(manager_->GetPortalCheckURL());
+}
+
void Device::RequestConnectionHealthCheck() {
if (!health_checker_.get()) {
SLOG(Device, 2) << "No health checker exists, cannot request "
@@ -806,10 +797,6 @@
traffic_monitor_.reset(traffic_monitor);
}
-void Device::set_health_checker(ConnectionHealthChecker *health_checker) {
- health_checker_.reset(health_checker);
-}
-
bool Device::StartTrafficMonitor() {
SLOG(Device, 2) << __func__;
if (!traffic_monitor_enabled_) {
diff --git a/device.h b/device.h
index c173152..4ef7b80 100644
--- a/device.h
+++ b/device.h
@@ -193,6 +193,10 @@
DeviceAdaptorInterface *adaptor() const { return adaptor_.get(); }
+ void set_health_checker(ConnectionHealthChecker *health_checker) {
+ health_checker_.reset(health_checker);
+ }
+
// Suspend event handler. Called by Manager before the system
// suspends. For this callback to be useful, the device must specify
// a suspend delay. Otherwise, there is no guarantee that the device
@@ -221,9 +225,9 @@
FRIEND_TEST(CellularTest, ModemStateChangeDisable);
FRIEND_TEST(CellularTest, UseNoArpGateway);
FRIEND_TEST(ConnectionTest, OnRouteQueryResponse);
- FRIEND_TEST(DeviceHealthCheckerTest, CreateConnectionCreatesHealthChecker);
- FRIEND_TEST(DeviceHealthCheckerTest, InitializeHealthCheckIps);
+ FRIEND_TEST(DeviceHealthCheckerTest, HealthCheckerPersistsAcrossDeviceReset);
FRIEND_TEST(DeviceHealthCheckerTest, RequestConnectionHealthCheck);
+ FRIEND_TEST(DeviceHealthCheckerTest, SetupHealthChecker);
FRIEND_TEST(DeviceTest, AcquireIPConfig);
FRIEND_TEST(DeviceTest, DestroyIPConfig);
FRIEND_TEST(DeviceTest, DestroyIPConfigNULL);
@@ -335,8 +339,7 @@
// Avoids showing a failure mole in the UI.
void SetServiceFailureSilent(Service::ConnectFailure failure_state);
- // Initializes the health checker's internal collection of remote IPs to try.
- void InitializeHealthCheckIps();
+ virtual void SetupConnectionHealthChecker();
// Checks the network connectivity status by creating a TCP connection, and
// optionally sending a small amout of data.
@@ -424,9 +427,6 @@
traffic_monitor_enabled_ = traffic_monitor_enabled;
}
- // Ownership of |health_checker_| is taken.
- void set_health_checker(ConnectionHealthChecker *health_checker);
-
private:
friend class CellularCapabilityTest;
friend class CellularTest;
@@ -447,8 +447,8 @@
static const char kIPFlagReversePathFilter[];
static const char kIPFlagReversePathFilterEnabled[];
static const char kIPFlagReversePathFilterLooseMode[];
- static const char kStoragePowered[];
static const char kStorageIPConfigs[];
+ static const char kStoragePowered[];
static const char kStorageReceiveByteCount[];
static const char kStorageTransmitByteCount[];
@@ -552,10 +552,6 @@
DHCPProvider *dhcp_provider_;
RTNLHandler *rtnl_handler_;
- // TODO(armansito): List of static IPs for connection health check.
- // These should be removed once shill has a DNS result caching mechanism.
- std::vector<std::string> health_check_ip_pool_;
-
DISALLOW_COPY_AND_ASSIGN(Device);
};
diff --git a/device_unittest.cc b/device_unittest.cc
index 80cbef4..c053f4e 100644
--- a/device_unittest.cc
+++ b/device_unittest.cc
@@ -33,6 +33,7 @@
#include "shill/mock_dhcp_config.h"
#include "shill/mock_dhcp_provider.h"
#include "shill/mock_glib.h"
+#include "shill/mock_ip_address_store.h"
#include "shill/mock_ipconfig.h"
#include "shill/mock_link_monitor.h"
#include "shill/mock_manager.h"
@@ -1085,6 +1086,7 @@
public:
DeviceHealthCheckerTest()
: connection_(new StrictMock<MockConnection>(&device_info_)),
+ ip_address_store_(new MockIPAddressStore()),
weak_ptr_factory_(this) {}
protected:
@@ -1102,11 +1104,13 @@
.Times(AnyNumber());
EXPECT_CALL(*connection_.get(), dns_servers())
.Times(AnyNumber());
+ EXPECT_CALL(*ip_address_store_.get(), AddUnique(_)).Times(AnyNumber());
mock_health_checker_.reset(
new MockConnectionHealthChecker(
connection_,
NULL,
+ ip_address_store_.get(),
Bind(&DeviceHealthCheckerTest::OnConnectionHealthCheckerResult,
weak_ptr_factory_.GetWeakPtr())));
}
@@ -1116,32 +1120,20 @@
EXPECT_TRUE(device_->health_checker_.get());
}
+ void SetMockConnection() {
+ device_->connection_ = connection_;
+ }
+
void OnConnectionHealthCheckerResult(
ConnectionHealthChecker::Result result) {
}
scoped_refptr<MockConnection> connection_;
scoped_ptr<MockConnectionHealthChecker> mock_health_checker_;
+ scoped_ptr<MockIPAddressStore> ip_address_store_;
base::WeakPtrFactory<DeviceHealthCheckerTest> weak_ptr_factory_;
};
-TEST_F(DeviceHealthCheckerTest, CreateConnectionCreatesHealthChecker) {
- EXPECT_FALSE(device_->connection_.get());
- EXPECT_FALSE(device_->health_checker_.get());
- device_->CreateConnection();
- EXPECT_TRUE(device_->connection_.get());
- EXPECT_TRUE(device_->health_checker_.get());
-}
-
-TEST_F(DeviceHealthCheckerTest, InitializeHealthCheckIps) {
- EXPECT_FALSE(device_->health_checker_.get());
- MockConnectionHealthChecker *health_checker = mock_health_checker_.get();
- SetMockHealthChecker();
- EXPECT_CALL(*health_checker, AddRemoteIP(_)).Times(
- device_->health_check_ip_pool_.size());
- device_->InitializeHealthCheckIps();
-}
-
TEST_F(DeviceHealthCheckerTest, RequestConnectionHealthCheck) {
EXPECT_FALSE(device_->health_checker_.get());
MockConnectionHealthChecker *health_checker = mock_health_checker_.get();
diff --git a/ip_address_store.cc b/ip_address_store.cc
new file mode 100644
index 0000000..c0064e5
--- /dev/null
+++ b/ip_address_store.cc
@@ -0,0 +1,55 @@
+// Copyright (c) 2013 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 "shill/ip_address_store.h"
+
+#include <iterator>
+
+#include <stdlib.h>
+#include <time.h>
+
+using std::advance;
+
+namespace shill {
+
+// This is a less than comparison so that IPAddress can be stored in a set.
+// We do not care about a semantically meaningful comparison. This is
+// deterministic, and that's all that matters.
+bool IPAddressLTIgnorePrefix::operator () (const IPAddress &lhs,
+ const IPAddress &rhs) const {
+ return lhs.ToString() < rhs.ToString();
+}
+
+IPAddressStore::IPAddressStore() {
+ srand(time(NULL));
+}
+
+IPAddressStore::~IPAddressStore() {}
+
+void IPAddressStore::AddUnique(const IPAddress &ip) {
+ ip_addresses_.insert(ip);
+}
+
+void IPAddressStore::Clear() {
+ ip_addresses_.clear();
+}
+
+size_t IPAddressStore::Count() const {
+ return ip_addresses_.size();
+}
+
+bool IPAddressStore::Empty() const {
+ return ip_addresses_.empty();
+}
+
+IPAddress IPAddressStore::GetRandomIP() {
+ if (ip_addresses_.empty())
+ return IPAddress(IPAddress::kFamilyUnknown);
+ int index = rand() % ip_addresses_.size();
+ IPAddresses::const_iterator cit = ip_addresses_.begin();
+ advance(cit, index);
+ return *cit;
+}
+
+} // namespace shill
diff --git a/ip_address_store.h b/ip_address_store.h
new file mode 100644
index 0000000..fd3ee04
--- /dev/null
+++ b/ip_address_store.h
@@ -0,0 +1,52 @@
+// Copyright (c) 2013 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 SHILL_IP_ADDRESS_STORE_H_
+#define SHILL_IP_ADDRESS_STORE_H_
+
+#include <set>
+
+#include "shill/ip_address.h"
+
+namespace shill {
+
+struct IPAddressLTIgnorePrefix {
+ bool operator () (const IPAddress &lhs, const IPAddress &rhs) const;
+};
+
+// Stores a set of IP addresses used by ConnectionHealthChecker to check
+// connectivity when there is a chance that the service has run out-of-credits.
+// The IP addresses are populated (using DNS queries) opportunistically and
+// must be persistent so that they can be used in an out-of-credit scenario
+// (when DNS queries would also fail).
+// To make the store persistent across Device resets (e.g. suspend-resume), it
+// is owned by the Manager.
+// Currently, this is a thin wrapper around an STL container.
+class IPAddressStore {
+ public:
+ typedef std::set<IPAddress, IPAddressLTIgnorePrefix> IPAddresses;
+
+ IPAddressStore();
+ virtual ~IPAddressStore();
+
+ // Add a new IP address if it does not already exist.
+ virtual void AddUnique(const IPAddress &ip);
+ virtual void Clear();
+ virtual size_t Count() const;
+ virtual bool Empty() const;
+
+ virtual IPAddress GetRandomIP();
+
+ protected:
+ friend class IPAddressStoreTest;
+
+ private:
+ IPAddresses ip_addresses_;
+
+ DISALLOW_COPY_AND_ASSIGN(IPAddressStore);
+};
+
+} // namespace shill
+
+#endif // SHILL_IP_ADDRESS_STORE_H_
diff --git a/ip_address_store_unittest.cc b/ip_address_store_unittest.cc
new file mode 100644
index 0000000..e10f3b7
--- /dev/null
+++ b/ip_address_store_unittest.cc
@@ -0,0 +1,72 @@
+// Copyright (c) 2013 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 "shill/ip_address_store.h"
+
+#include <string>
+
+#include <gtest/gtest.h>
+
+using ::std::string;
+using ::std::vector;
+using ::testing::Test;
+
+namespace shill {
+
+namespace {
+const char kInterfaceName[] = "int0";
+const char kIPAddress_0_0_0_0[] = "0.0.0.0";
+const char kIPAddress_8_8_8_8[] = "8.8.8.8";
+const char kIPAddress_7_7_7_7[] = "7.7.7.7";
+} // namespace {}
+
+class IPAddressStoreTest : public Test {
+ public:
+ IPAddressStoreTest() {}
+
+ protected:
+ IPAddress StringToIPv4Address(const string &address_string) {
+ IPAddress ip_address(IPAddress::kFamilyIPv4);
+ EXPECT_TRUE(ip_address.SetAddressFromString(address_string));
+ return ip_address;
+ }
+
+ void PopulateIPAddressStore() {
+ ip_address_store_.AddUnique(StringToIPv4Address(kIPAddress_0_0_0_0));
+ ip_address_store_.AddUnique(StringToIPv4Address(kIPAddress_8_8_8_8));
+ ip_address_store_.AddUnique(StringToIPv4Address(kIPAddress_7_7_7_7));
+ }
+
+ bool Contains(const IPAddress& ip) {
+ return ip_address_store_.ip_addresses_.find(ip) !=
+ ip_address_store_.ip_addresses_.end();
+ }
+
+ IPAddressStore ip_address_store_;
+};
+
+TEST_F(IPAddressStoreTest, AddUnique) {
+ IPAddress ip_0_0_0_0 = StringToIPv4Address(kIPAddress_0_0_0_0);
+ IPAddress ip_8_8_8_8 = StringToIPv4Address(kIPAddress_8_8_8_8);
+ IPAddress ip_7_7_7_7 = StringToIPv4Address(kIPAddress_7_7_7_7);
+
+ EXPECT_EQ(0, ip_address_store_.Count());
+ ip_address_store_.AddUnique(ip_0_0_0_0);
+ EXPECT_TRUE(Contains(ip_0_0_0_0));
+ EXPECT_EQ(1, ip_address_store_.Count());
+ ip_address_store_.AddUnique(ip_8_8_8_8);
+ EXPECT_TRUE(Contains(ip_8_8_8_8));
+ EXPECT_EQ(2, ip_address_store_.Count());
+ ip_address_store_.AddUnique(ip_8_8_8_8);
+ EXPECT_TRUE(Contains(ip_0_0_0_0));
+ EXPECT_TRUE(Contains(ip_8_8_8_8));
+ EXPECT_EQ(2, ip_address_store_.Count());
+ ip_address_store_.AddUnique(ip_0_0_0_0);
+ EXPECT_EQ(2, ip_address_store_.Count());
+ ip_address_store_.AddUnique(ip_7_7_7_7);
+ EXPECT_TRUE(Contains(ip_7_7_7_7));
+ EXPECT_EQ(3, ip_address_store_.Count());
+}
+
+} // namespace shill
diff --git a/manager.cc b/manager.cc
index d5389e3..09d18c7 100644
--- a/manager.cc
+++ b/manager.cc
@@ -35,6 +35,7 @@
#include "shill/event_dispatcher.h"
#include "shill/geolocation_info.h"
#include "shill/hook_table.h"
+#include "shill/ip_address_store.h"
#include "shill/key_file_store.h"
#include "shill/logging.h"
#include "shill/profile.h"
@@ -114,7 +115,8 @@
suspend_delay_registered_(false),
suspend_delay_id_(0),
default_service_callback_tag_(0),
- crypto_util_proxy_(new CryptoUtilProxy(dispatcher, glib)) {
+ crypto_util_proxy_(new CryptoUtilProxy(dispatcher, glib)),
+ health_checker_remote_ips_(new IPAddressStore()) {
HelpRegisterDerivedString(flimflam::kActiveProfileProperty,
&Manager::GetActiveProfileRpcIdentifier,
NULL);
diff --git a/manager.h b/manager.h
index 43f124e..7175444 100644
--- a/manager.h
+++ b/manager.h
@@ -41,6 +41,7 @@
class Error;
class EthernetEapProvider;
class EventDispatcher;
+class IPAddressStore;
class ManagerAdaptorInterface;
class Resolver;
class StoreInterface;
@@ -261,6 +262,9 @@
GLib *glib() const { return glib_; }
virtual const base::FilePath &run_path() const { return run_path_; }
const base::FilePath &storage_path() const { return storage_path_; }
+ IPAddressStore * health_checker_remote_ips() const {
+ return health_checker_remote_ips_.get();
+ }
bool GetArpGateway() const { return props_.arp_gateway; }
const std::string &GetHostName() const { return props_.host_name; }
@@ -557,6 +561,11 @@
// Delegate to handle destination verification operations for the manager.
scoped_ptr<CryptoUtilProxy> crypto_util_proxy_;
+
+ // Stores IP addresses of some remote hosts that accept port 80 TCP
+ // connections. ConnectionHealthChecker uses these IPs.
+ // The store resides in Manager so that it persists across Device reset.
+ scoped_ptr<IPAddressStore> health_checker_remote_ips_;
};
} // namespace shill
diff --git a/mock_connection_health_checker.cc b/mock_connection_health_checker.cc
index cda845f..66b2a5c 100644
--- a/mock_connection_health_checker.cc
+++ b/mock_connection_health_checker.cc
@@ -14,8 +14,12 @@
MockConnectionHealthChecker::MockConnectionHealthChecker(
ConnectionRefPtr connection,
EventDispatcher *dispatcher,
+ IPAddressStore *remote_ips,
const Callback<void(Result)> &result_callback)
- : ConnectionHealthChecker(connection, dispatcher, result_callback) {}
+ : ConnectionHealthChecker(connection,
+ dispatcher,
+ remote_ips,
+ result_callback) {}
MockConnectionHealthChecker::~MockConnectionHealthChecker() {}
diff --git a/mock_connection_health_checker.h b/mock_connection_health_checker.h
index 03d71a1..08d31b2 100644
--- a/mock_connection_health_checker.h
+++ b/mock_connection_health_checker.h
@@ -16,6 +16,7 @@
MockConnectionHealthChecker(
ConnectionRefPtr connection,
EventDispatcher *dispatcher,
+ IPAddressStore *remote_ips,
const base::Callback<void(Result)> &result_callback);
virtual ~MockConnectionHealthChecker();
diff --git a/mock_ip_address_store.cc b/mock_ip_address_store.cc
new file mode 100644
index 0000000..3051229
--- /dev/null
+++ b/mock_ip_address_store.cc
@@ -0,0 +1,12 @@
+// Copyright (c) 2012 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 "shill/mock_ip_address_store.h"
+
+namespace shill {
+
+MockIPAddressStore::MockIPAddressStore() {}
+MockIPAddressStore::~MockIPAddressStore() {}
+
+} // namespace shill
diff --git a/mock_ip_address_store.h b/mock_ip_address_store.h
new file mode 100644
index 0000000..1e3640c
--- /dev/null
+++ b/mock_ip_address_store.h
@@ -0,0 +1,32 @@
+// Copyright (c) 2012 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 SHILL_MOCK_IP_ADDRESS_STORE_H_
+#define SHILL_MOCK_IP_ADDRESS_STORE_H_
+
+#include <base/basictypes.h>
+#include <gmock/gmock.h>
+
+#include "shill/ip_address_store.h"
+
+namespace shill {
+
+class MockIPAddressStore : public IPAddressStore {
+ public:
+ MockIPAddressStore();
+ virtual ~MockIPAddressStore();
+
+ MOCK_METHOD1(AddUnique, void(const IPAddress &ip));
+ MOCK_METHOD0(Clear, void());
+ MOCK_CONST_METHOD0(Count, size_t());
+ MOCK_CONST_METHOD0(Empty, bool());
+ MOCK_METHOD0(GetRandomIP, IPAddress());
+
+ private:
+ DISALLOW_COPY_AND_ASSIGN(MockIPAddressStore);
+};
+
+} // namespace shill
+
+#endif // SHILL_MOCK_IP_ADDRESS_STORE_H_
diff --git a/wifi_unittest.cc b/wifi_unittest.cc
index 5f57331..5bbc9f9 100644
--- a/wifi_unittest.cc
+++ b/wifi_unittest.cc
@@ -80,6 +80,7 @@
using ::testing::NiceMock;
using ::testing::NotNull;
using ::testing::Return;
+using ::testing::ReturnRef;
using ::testing::SaveArg;
using ::testing::SetArgumentPointee;
using ::testing::StrEq;
@@ -1682,6 +1683,8 @@
Mock::VerifyAndClearExpectations(GetSupplicantInterfaceProxy());
EXPECT_CALL(*GetSupplicantInterfaceProxy(), EnableHighBitrates()).Times(1);
EXPECT_CALL(*manager(), device_info()).WillOnce(Return(device_info()));
+ string portal_url;
+ EXPECT_CALL(*manager(), GetPortalCheckURL()).WillOnce(ReturnRef(portal_url));
ReportIPConfigComplete();
// Similarly, rekeying events after we have an IP don't trigger L3
// configuration. However, we treat all transitions to completed as potential