shill: implement timeout for DHCP requests
BUG=chromium-os:30689
TEST=new unit tests, manual (see below)
Manual testing:
- Start shill.
- ff_debug +dhcp
- Plug USB-Ethernet into a switch (to get carrier), but without
an upstream connection for the switch. Plug dongle into USB
port.
- Wait 30 seconds.
- Check log file, find "Timed out waiting for DHCP lease on eth0",
and "Service Ethernet state Configuring -> Disconnected".
Change-Id: Ifc27539ec7191b060f615eb9dec61c9fdab07267
Reviewed-on: https://gerrit.chromium.org/gerrit/22302
Commit-Ready: mukesh agrawal <quiche@chromium.org>
Reviewed-by: mukesh agrawal <quiche@chromium.org>
Tested-by: mukesh agrawal <quiche@chromium.org>
diff --git a/device.cc b/device.cc
index 540a538..3d1d010 100644
--- a/device.cc
+++ b/device.cc
@@ -389,8 +389,36 @@
// to the Online state.
StartPortalDetection();
} else {
- // TODO(pstew): This logic gets more complex when multiple IPConfig types
- // are run in parallel (e.g. DHCP and DHCP6)
+ // TODO(pstew): This logic gets yet more complex when multiple
+ // IPConfig types are run in parallel (e.g. DHCP and DHCP6)
+ if (selected_service_ &&
+ selected_service_->static_ip_parameters().ContainsAddress()) {
+ // Consider three cases:
+ //
+ // 1. We're here because DHCP failed while starting up. There
+ // are two subcases:
+ // a. DHCP has failed, and Static IP config has _not yet_
+ // completed. It's fine to do nothing, because we'll
+ // apply the static config shortly.
+ // b. DHCP has failed, and Static IP config has _already_
+ // completed. It's fine to do nothing, because we can
+ // continue to use the static config that's already
+ // been applied.
+ //
+ // 2. We're here because a previously valid DHCP configuration
+ // is no longer valid. There's still a static IP config,
+ // because the condition in the if clause evaluated to true.
+ // Furthermore, the static config includes an IP address for
+ // us to use.
+ //
+ // The current configuration may include some DHCP
+ // parameters, overriden by any static parameters
+ // provided. We continue to use this configuration, because
+ // the only configuration element that is leased to us (IP
+ // address) will be overriden by a static parameter.
+ return;
+ }
+
SetServiceState(Service::kStateDisconnected);
DestroyConnection();
}
diff --git a/device_unittest.cc b/device_unittest.cc
index efeef2e..fb24466 100644
--- a/device_unittest.cc
+++ b/device_unittest.cc
@@ -35,12 +35,14 @@
#include "shill/mock_store.h"
#include "shill/portal_detector.h"
#include "shill/property_store_unittest.h"
+#include "shill/static_ip_parameters.h"
#include "shill/technology.h"
using std::map;
using std::string;
using std::vector;
using ::testing::_;
+using ::testing::AnyNumber;
using ::testing::AtLeast;
using ::testing::Mock;
using ::testing::NiceMock;
@@ -89,6 +91,7 @@
device_info_(control_interface(), NULL, NULL, NULL) {
DHCPProvider::GetInstance()->glib_ = glib();
DHCPProvider::GetInstance()->control_interface_ = control_interface();
+ DHCPProvider::GetInstance()->dispatcher_ = dispatcher();
}
virtual ~DeviceTest() {}
@@ -280,6 +283,21 @@
OnIPConfigUpdated(NULL, false);
}
+TEST_F(DeviceTest, IPConfigUpdatedFailureWithStatic) {
+ scoped_refptr<MockService> service(
+ new StrictMock<MockService>(control_interface(),
+ dispatcher(),
+ metrics(),
+ manager()));
+ SelectService(service);
+ service->static_ip_parameters_.args_.SetString(
+ flimflam::kAddressProperty, "1.1.1.1");
+ service->static_ip_parameters_.args_.SetInt(flimflam::kPrefixlenProperty, 16);
+ EXPECT_CALL(*service.get(), SetState(_)).Times(0);
+ EXPECT_CALL(*service.get(), SetConnection(_)).Times(0);
+ OnIPConfigUpdated(NULL, false);
+}
+
TEST_F(DeviceTest, IPConfigUpdatedSuccess) {
scoped_refptr<MockService> service(
new StrictMock<MockService>(control_interface(),
diff --git a/dhcp_config.cc b/dhcp_config.cc
index 9d87da3..e1f610d 100644
--- a/dhcp_config.cc
+++ b/dhcp_config.cc
@@ -41,6 +41,7 @@
"var/lib/dhcpcd/dhcpcd-%s.lease";
const char DHCPConfig::kDHCPCDPathFormatPID[] =
"var/run/dhcpcd/dhcpcd-%s.pid";
+const int DHCPConfig::kDHCPTimeoutSeconds = 30;
const int DHCPConfig::kMinMTU = 576;
const char DHCPConfig::kReasonBound[] = "BOUND";
const char DHCPConfig::kReasonFail[] = "FAIL";
@@ -67,7 +68,9 @@
arp_gateway_(arp_gateway),
pid_(0),
child_watch_tag_(0),
+ lease_acquisition_timeout_seconds_(kDHCPTimeoutSeconds),
root_("/"),
+ weak_ptr_factory_(this),
dispatcher_(dispatcher),
glib_(glib) {
SLOG(DHCP, 2) << __func__ << ": " << device_name;
@@ -104,6 +107,7 @@
return false;
}
proxy_->Rebind(device_name());
+ StartDHCPTimeout();
return true;
}
@@ -146,6 +150,11 @@
UpdateProperties(properties, true);
}
+void DHCPConfig::UpdateProperties(const Properties &properties, bool success) {
+ StopDHCPTimeout();
+ IPConfig::UpdateProperties(properties, success);
+}
+
bool DHCPConfig::Start() {
SLOG(DHCP, 2) << __func__ << ": " << device_name();
@@ -184,6 +193,7 @@
provider_->BindPID(pid_, this);
CHECK(!child_watch_tag_);
child_watch_tag_ = glib_->ChildWatchAdd(pid_, ChildWatchCallback, this);
+ StartDHCPTimeout();
return true;
}
@@ -208,6 +218,7 @@
if (ret != pid_)
PLOG(ERROR);
}
+ StopDHCPTimeout();
}
bool DHCPConfig::Restart() {
@@ -324,4 +335,22 @@
base::StringPrintf(kDHCPCDPathFormatPID, device_name().c_str())), false);
}
+void DHCPConfig::StartDHCPTimeout() {
+ lease_acquisition_timeout_callback_.Reset(
+ Bind(&DHCPConfig::ProcessDHCPTimeout, weak_ptr_factory_.GetWeakPtr()));
+ dispatcher_->PostDelayedTask(
+ lease_acquisition_timeout_callback_.callback(),
+ lease_acquisition_timeout_seconds_ * 1000);
+}
+
+void DHCPConfig::StopDHCPTimeout() {
+ lease_acquisition_timeout_callback_.Cancel();
+}
+
+void DHCPConfig::ProcessDHCPTimeout() {
+ LOG(ERROR) << "Timed out waiting for DHCP lease on " << device_name() << " "
+ << "(after " << lease_acquisition_timeout_seconds_ << " seconds).";
+ UpdateProperties(IPConfig::Properties(), false);
+}
+
} // namespace shill
diff --git a/dhcp_config.h b/dhcp_config.h
index 429e15e..e66ee98 100644
--- a/dhcp_config.h
+++ b/dhcp_config.h
@@ -5,8 +5,10 @@
#ifndef SHILL_DHCP_CONFIG_
#define SHILL_DHCP_CONFIG_
+#include <base/cancelable_callback.h>
#include <base/file_path.h>
#include <base/memory/scoped_ptr.h>
+#include <base/memory/weak_ptr.h>
#include <dbus-c++/types.h>
#include <glib.h>
#include <gtest/gtest_prod.h> // for FRIEND_TEST
@@ -61,6 +63,10 @@
void ProcessEventSignal(const std::string &reason,
const Configuration &configuration);
+ protected:
+ // Overrides base clase implementation.
+ virtual void UpdateProperties(const Properties &properties, bool success);
+
private:
friend class DHCPConfigTest;
FRIEND_TEST(DHCPConfigTest, GetIPv4AddressString);
@@ -72,14 +78,17 @@
FRIEND_TEST(DHCPConfigTest, ReleaseIP);
FRIEND_TEST(DHCPConfigTest, RenewIP);
FRIEND_TEST(DHCPConfigTest, RequestIP);
+ FRIEND_TEST(DHCPConfigTest, RequestIPTimeout);
FRIEND_TEST(DHCPConfigTest, Restart);
FRIEND_TEST(DHCPConfigTest, RestartNoClient);
FRIEND_TEST(DHCPConfigTest, StartFail);
+ FRIEND_TEST(DHCPConfigTest, StartTimeout);
FRIEND_TEST(DHCPConfigTest, StartWithHostname);
FRIEND_TEST(DHCPConfigTest, StartWithoutArpGateway);
FRIEND_TEST(DHCPConfigTest, StartWithoutHostname);
FRIEND_TEST(DHCPConfigTest, StartWithoutLeaseSuffix);
FRIEND_TEST(DHCPConfigTest, Stop);
+ FRIEND_TEST(DHCPConfigTest, StopDuringRequestIP);
FRIEND_TEST(DHCPProviderTest, CreateConfig);
static const char kConfigurationKeyBroadcastAddress[];
@@ -96,6 +105,7 @@
static const char kDHCPCDPath[];
static const char kDHCPCDPathFormatLease[];
static const char kDHCPCDPathFormatPID[];
+ static const int kDHCPTimeoutSeconds;
static const char kReasonBound[];
static const char kReasonFail[];
@@ -131,6 +141,16 @@
// its GPid, exit watch callback, and state files.
void CleanupClientState();
+ // Initialize a callback that will invoke ProcessDHCPTimeout if we
+ // do not get a lease in a reasonable amount of time.
+ void StartDHCPTimeout();
+ // Cancel callback created by StartDHCPTimeout. One-liner included
+ // for symmetry.
+ void StopDHCPTimeout();
+ // Called if we do not get a DHCP lease in a reasonable amount of time.
+ // Informs upper layers of the failure.
+ void ProcessDHCPTimeout();
+
// Store cached copies of singletons for speed/ease of testing.
ProxyFactory *proxy_factory_;
@@ -158,9 +178,17 @@
// The proxy for communicating with the DHCP client.
scoped_ptr<DHCPProxyInterface> proxy_;
+ // Called if we fail to get a DHCP lease in a timely manner.
+ base::CancelableClosure lease_acquisition_timeout_callback_;
+
+ // Time to wait for a DHCP lease. Represented as field so that it
+ // can be overriden in tests.
+ unsigned int lease_acquisition_timeout_seconds_;
+
// Root file path, used for testing.
FilePath root_;
+ base::WeakPtrFactory<DHCPConfig> weak_ptr_factory_;
EventDispatcher *dispatcher_;
GLib *glib_;
diff --git a/dhcp_config_unittest.cc b/dhcp_config_unittest.cc
index 7cebd32..702f67c 100644
--- a/dhcp_config_unittest.cc
+++ b/dhcp_config_unittest.cc
@@ -325,6 +325,8 @@
bool called_;
};
+void DoNothing() {}
+
} // namespace {}
TEST_F(DHCPConfigTest, ProcessEventSignalFail) {
@@ -334,9 +336,11 @@
UpdateCallbackTest callback_test(DHCPConfig::kReasonFail, config_, false);
config_->RegisterUpdateCallback(
Bind(&UpdateCallbackTest::Callback, Unretained(&callback_test)));
+ config_->lease_acquisition_timeout_callback_.Reset(base::Bind(&DoNothing));
config_->ProcessEventSignal(DHCPConfig::kReasonFail, conf);
EXPECT_TRUE(callback_test.called());
EXPECT_TRUE(config_->properties().address.empty());
+ EXPECT_TRUE(config_->lease_acquisition_timeout_callback_.IsCancelled());
}
TEST_F(DHCPConfigTest, ProcessEventSignalSuccess) {
@@ -353,10 +357,12 @@
UpdateCallbackTest callback_test(message, config_, true);
config_->RegisterUpdateCallback(
Bind(&UpdateCallbackTest::Callback, Unretained(&callback_test)));
+ config_->lease_acquisition_timeout_callback_.Reset(base::Bind(&DoNothing));
config_->ProcessEventSignal(kReasons[r], conf);
EXPECT_TRUE(callback_test.called()) << message;
EXPECT_EQ(base::StringPrintf("%zu.0.0.0", r), config_->properties().address)
<< message;
+ EXPECT_TRUE(config_->lease_acquisition_timeout_callback_.IsCancelled());
}
}
@@ -368,9 +374,11 @@
UpdateCallbackTest callback_test(kReasonUnknown, config_, false);
config_->RegisterUpdateCallback(
Bind(&UpdateCallbackTest::Callback, Unretained(&callback_test)));
+ config_->lease_acquisition_timeout_callback_.Reset(base::Bind(&DoNothing));
config_->ProcessEventSignal(kReasonUnknown, conf);
EXPECT_FALSE(callback_test.called());
EXPECT_TRUE(config_->properties().address.empty());
+ EXPECT_FALSE(config_->lease_acquisition_timeout_callback_.IsCancelled());
}
@@ -383,18 +391,36 @@
}
TEST_F(DHCPConfigTest, RenewIP) {
+ EXPECT_TRUE(config_->lease_acquisition_timeout_callback_.IsCancelled());
config_->pid_ = 456;
EXPECT_CALL(*proxy_, Rebind(kDeviceName)).Times(1);
config_->proxy_.reset(proxy_.release());
EXPECT_TRUE(config_->RenewIP());
+ EXPECT_FALSE(config_->lease_acquisition_timeout_callback_.IsCancelled());
config_->pid_ = 0;
}
TEST_F(DHCPConfigTest, RequestIP) {
+ EXPECT_TRUE(config_->lease_acquisition_timeout_callback_.IsCancelled());
config_->pid_ = 567;
EXPECT_CALL(*proxy_, Rebind(kDeviceName)).Times(1);
config_->proxy_.reset(proxy_.release());
EXPECT_TRUE(config_->RenewIP());
+ EXPECT_FALSE(config_->lease_acquisition_timeout_callback_.IsCancelled());
+ config_->pid_ = 0;
+}
+
+TEST_F(DHCPConfigTest, RequestIPTimeout) {
+ UpdateCallbackTest callback_test(DHCPConfig::kReasonFail, config_, false);
+ config_->RegisterUpdateCallback(
+ Bind(&UpdateCallbackTest::Callback, Unretained(&callback_test)));
+ config_->lease_acquisition_timeout_seconds_ = 0;
+ config_->pid_ = 567;
+ EXPECT_CALL(*proxy_, Rebind(kDeviceName)).Times(1);
+ config_->proxy_.reset(proxy_.release());
+ config_->RenewIP();
+ config_->dispatcher_->DispatchPendingEvents();
+ EXPECT_TRUE(callback_test.called());
config_->pid_ = 0;
}
@@ -449,6 +475,20 @@
StopRunningConfigAndExpect(config, true);
}
+TEST_F(DHCPConfigTest, StartTimeout) {
+ UpdateCallbackTest callback_test(DHCPConfig::kReasonFail, config_, false);
+ config_->RegisterUpdateCallback(
+ Bind(&UpdateCallbackTest::Callback, Unretained(&callback_test)));
+ config_->lease_acquisition_timeout_seconds_ = 0;
+ config_->proxy_.reset(proxy_.release());
+ EXPECT_CALL(*glib(),
+ SpawnAsync(_, IsDHCPCDArgs(true, true, true), _, _, _, _, _, _))
+ .WillOnce(Return(true));
+ config_->Start();
+ config_->dispatcher_->DispatchPendingEvents();
+ EXPECT_TRUE(callback_test.called());
+}
+
TEST_F(DHCPConfigTest, Stop) {
// Ensure no crashes.
const int kPID = 1 << 17; // Ensure unknown positive PID.
@@ -456,6 +496,18 @@
config_->pid_ = kPID;
config_->Stop();
EXPECT_CALL(*glib(), SpawnClosePID(kPID)).Times(1); // Invoked by destructor.
+ EXPECT_TRUE(config_->lease_acquisition_timeout_callback_.IsCancelled());
+}
+
+TEST_F(DHCPConfigTest, StopDuringRequestIP) {
+ config_->pid_ = 567;
+ EXPECT_CALL(*proxy_, Rebind(kDeviceName)).Times(1);
+ config_->proxy_.reset(proxy_.release());
+ EXPECT_TRUE(config_->RenewIP());
+ EXPECT_FALSE(config_->lease_acquisition_timeout_callback_.IsCancelled());
+ config_->pid_ = 0; // Keep Stop from killing a real process.
+ config_->Stop();
+ EXPECT_TRUE(config_->lease_acquisition_timeout_callback_.IsCancelled());
}
TEST_F(DHCPConfigTest, SetProperty) {
diff --git a/ipconfig.h b/ipconfig.h
index b3ad0b0..084e30a 100644
--- a/ipconfig.h
+++ b/ipconfig.h
@@ -102,7 +102,7 @@
protected:
// Updates the IP configuration properties and notifies registered listeners
// about the event. |success| is set to false if the IP configuration failed.
- void UpdateProperties(const Properties &properties, bool success);
+ virtual void UpdateProperties(const Properties &properties, bool success);
// |id_suffix| is appended to the storage id, intended to bind this instance
// to its associated device.
diff --git a/service.h b/service.h
index 612d21a..3730b1b 100644
--- a/service.h
+++ b/service.h
@@ -435,6 +435,7 @@
friend class ServiceAdaptorInterface;
friend class VPNServiceTest;
friend class WiFiServiceTest;
+ FRIEND_TEST(DeviceTest, IPConfigUpdatedFailureWithStatic);
FRIEND_TEST(ServiceTest, ConfigureIgnoredProperty);
FRIEND_TEST(ServiceTest, ConfigureStringProperty);
FRIEND_TEST(ServiceTest, Constructor);
diff --git a/static_ip_parameters.cc b/static_ip_parameters.cc
index 2a26d3f..afe343b 100644
--- a/static_ip_parameters.cc
+++ b/static_ip_parameters.cc
@@ -4,6 +4,8 @@
#include "shill/static_ip_parameters.h"
+#include <string.h>
+
#include <base/logging.h>
#include <base/string_split.h>
#include <chromeos/dbus/service_constants.h>
@@ -208,18 +210,6 @@
}
}
-string StaticIPParameters::GetMappedStringProperty(
- const size_t &index, Error *error) {
- CHECK(index < arraysize(kProperties));
-
- const string &key = kProperties[index].name;
- if (!args_.ContainsString(key)) {
- error->Populate(Error::kNotFound, "Property is not set");
- return string();
- }
- return args_.GetString(key);
-}
-
int32 StaticIPParameters::GetMappedInt32Property(
const size_t &index, Error *error) {
CHECK(index < arraysize(kProperties));
@@ -232,10 +222,16 @@
return args_.GetInt(key);
}
-void StaticIPParameters::SetMappedStringProperty(
- const size_t &index, const string &value, Error *error) {
+string StaticIPParameters::GetMappedStringProperty(
+ const size_t &index, Error *error) {
CHECK(index < arraysize(kProperties));
- args_.SetString(kProperties[index].name, value);
+
+ const string &key = kProperties[index].name;
+ if (!args_.ContainsString(key)) {
+ error->Populate(Error::kNotFound, "Property is not set");
+ return string();
+ }
+ return args_.GetString(key);
}
void StaticIPParameters::SetMappedInt32Property(
@@ -244,4 +240,10 @@
args_.SetInt(kProperties[index].name, value);
}
+void StaticIPParameters::SetMappedStringProperty(
+ const size_t &index, const string &value, Error *error) {
+ CHECK(index < arraysize(kProperties));
+ args_.SetString(kProperties[index].name, value);
+}
+
} // namespace shill
diff --git a/static_ip_parameters.h b/static_ip_parameters.h
index ff6f00e..63bfd8f 100644
--- a/static_ip_parameters.h
+++ b/static_ip_parameters.h
@@ -43,6 +43,7 @@
private:
friend class StaticIPParametersTest;
+ FRIEND_TEST(DeviceTest, IPConfigUpdatedFailureWithStatic);
struct Property {
enum Type {