shill: Handle success/failure events from dhcpcd.
BUG=chromium-os:16718
TEST=unit tests
Change-Id: I36d8b737a060f7b295ebab26aa37b4e71022b4f0
Reviewed-on: http://gerrit.chromium.org/gerrit/2880
Tested-by: Darin Petkov <petkov@chromium.org>
Reviewed-by: Chris Masone <cmasone@chromium.org>
diff --git a/dhcp_config.cc b/dhcp_config.cc
index ffe219b..8f5bf63 100644
--- a/dhcp_config.cc
+++ b/dhcp_config.cc
@@ -30,6 +30,12 @@
const char DHCPConfig::kDHCPCDPath[] = "/sbin/dhcpcd";
const char DHCPConfig::kDHCPCDPathFormatLease[] = "var/run/dhcpcd-%s.lease";
const char DHCPConfig::kDHCPCDPathFormatPID[] = "var/run/dhcpcd-%s.pid";
+const char DHCPConfig::kReasonBound[] = "BOUND";
+const char DHCPConfig::kReasonFail[] = "FAIL";
+const char DHCPConfig::kReasonRebind[] = "REBIND";
+const char DHCPConfig::kReasonReboot[] = "REBOOT";
+const char DHCPConfig::kReasonRenew[] = "RENEW";
+
DHCPConfig::DHCPConfig(DHCPProvider *provider,
DeviceConstRefPtr device,
@@ -98,16 +104,24 @@
}
}
-void DHCPConfig::ProcessEventSignal(const std::string &reason,
+void DHCPConfig::ProcessEventSignal(const string &reason,
const Configuration &configuration) {
LOG(INFO) << "Event reason: " << reason;
-
- IPConfig::Properties properties;
- if (!ParseConfiguration(configuration, &properties)) {
- LOG(ERROR) << "Unable to parse the new DHCP configuration -- ignored.";
+ if (reason == kReasonFail) {
+ LOG(ERROR) << "Received failure event from DHCP client.";
+ UpdateProperties(IPConfig::Properties(), false);
return;
}
- UpdateProperties(properties);
+ if (reason != kReasonBound &&
+ reason != kReasonRebind &&
+ reason != kReasonReboot &&
+ reason != kReasonRenew) {
+ LOG(WARNING) << "Event ignored.";
+ return;
+ }
+ IPConfig::Properties properties;
+ CHECK(ParseConfiguration(configuration, &properties));
+ UpdateProperties(properties, true);
}
bool DHCPConfig::Start() {
@@ -232,11 +246,7 @@
VLOG(2) << "pid " << pid << " exit status " << status;
DHCPConfig *config = reinterpret_cast<DHCPConfig *>(data);
config->child_watch_tag_ = 0;
-
CHECK_EQ(pid, config->pid_);
- config->glib_->SpawnClosePID(pid);
- config->pid_ = 0;
-
config->CleanupClientState();
// |config| instance may be destroyed after this call.
@@ -252,6 +262,7 @@
glib_->SpawnClosePID(pid_);
pid_ = 0;
}
+ proxy_.reset();
file_util::Delete(root_.Append(base::StringPrintf(kDHCPCDPathFormatLease,
GetDeviceName().c_str())),
false);
diff --git a/dhcp_config.h b/dhcp_config.h
index 1152d61..16d8442 100644
--- a/dhcp_config.h
+++ b/dhcp_config.h
@@ -27,15 +27,6 @@
public:
typedef std::map<std::string, DBus::Variant> Configuration;
- static const char kConfigurationKeyBroadcastAddress[];
- static const char kConfigurationKeyDNS[];
- static const char kConfigurationKeyDomainName[];
- static const char kConfigurationKeyDomainSearch[];
- static const char kConfigurationKeyIPAddress[];
- static const char kConfigurationKeyMTU[];
- static const char kConfigurationKeyRouters[];
- static const char kConfigurationKeySubnetCIDR[];
-
DHCPConfig(DHCPProvider *provider,
DeviceConstRefPtr device,
GLibInterface *glib);
@@ -58,6 +49,9 @@
friend class DHCPConfigTest;
FRIEND_TEST(DHCPConfigTest, GetIPv4AddressString);
FRIEND_TEST(DHCPConfigTest, ParseConfiguration);
+ FRIEND_TEST(DHCPConfigTest, ProcessEventSignalFail);
+ FRIEND_TEST(DHCPConfigTest, ProcessEventSignalSuccess);
+ FRIEND_TEST(DHCPConfigTest, ProcessEventSignalUnknown);
FRIEND_TEST(DHCPConfigTest, ReleaseIP);
FRIEND_TEST(DHCPConfigTest, RenewIP);
FRIEND_TEST(DHCPConfigTest, RequestIP);
@@ -68,10 +62,25 @@
FRIEND_TEST(DHCPConfigTest, Stop);
FRIEND_TEST(DHCPProviderTest, CreateConfig);
+ static const char kConfigurationKeyBroadcastAddress[];
+ static const char kConfigurationKeyDNS[];
+ static const char kConfigurationKeyDomainName[];
+ static const char kConfigurationKeyDomainSearch[];
+ static const char kConfigurationKeyIPAddress[];
+ static const char kConfigurationKeyMTU[];
+ static const char kConfigurationKeyRouters[];
+ static const char kConfigurationKeySubnetCIDR[];
+
static const char kDHCPCDPath[];
static const char kDHCPCDPathFormatLease[];
static const char kDHCPCDPathFormatPID[];
+ static const char kReasonBound[];
+ static const char kReasonFail[];
+ static const char kReasonRebind[];
+ static const char kReasonReboot[];
+ static const char kReasonRenew[];
+
// Starts dhcpcd, returns true on success and false otherwise.
bool Start();
diff --git a/dhcp_config_unittest.cc b/dhcp_config_unittest.cc
index cb9adfb..ce3ca62 100644
--- a/dhcp_config_unittest.cc
+++ b/dhcp_config_unittest.cc
@@ -113,6 +113,82 @@
EXPECT_EQ(0, config_->pid_);
}
+namespace {
+
+class UpdateCallbackTest {
+ public:
+ UpdateCallbackTest(const string &message,
+ IPConfigRefPtr ipconfig,
+ bool success)
+ : message_(message),
+ ipconfig_(ipconfig),
+ success_(success),
+ called_(false) {}
+
+ void Callback(IPConfigRefPtr ipconfig, bool success) {
+ called_ = true;
+ EXPECT_EQ(ipconfig_.get(), ipconfig.get()) << message_;
+ EXPECT_EQ(success_, success) << message_;
+ }
+
+ bool called() const { return called_; }
+
+ private:
+ const string message_;
+ IPConfigRefPtr ipconfig_;
+ bool success_;
+ bool called_;
+};
+
+} // namespace {}
+
+TEST_F(DHCPConfigTest, ProcessEventSignalFail) {
+ DHCPConfig::Configuration conf;
+ conf[DHCPConfig::kConfigurationKeyIPAddress].writer().append_uint32(
+ 0x01020304);
+ UpdateCallbackTest callback_test(DHCPConfig::kReasonFail, config_, false);
+ config_->RegisterUpdateCallback(
+ NewCallback(&callback_test, &UpdateCallbackTest::Callback));
+ config_->ProcessEventSignal(DHCPConfig::kReasonFail, conf);
+ EXPECT_TRUE(callback_test.called());
+ EXPECT_TRUE(config_->properties().address.empty());
+}
+
+TEST_F(DHCPConfigTest, ProcessEventSignalSuccess) {
+ static const char * const kReasons[] = {
+ DHCPConfig::kReasonBound,
+ DHCPConfig::kReasonRebind,
+ DHCPConfig::kReasonReboot,
+ DHCPConfig::kReasonRenew
+ };
+ for (size_t r = 0; r < arraysize(kReasons); r++) {
+ DHCPConfig::Configuration conf;
+ string message = string(kReasons[r]) + " failed";
+ conf[DHCPConfig::kConfigurationKeyIPAddress].writer().append_uint32(r);
+ UpdateCallbackTest callback_test(message, config_, true);
+ config_->RegisterUpdateCallback(
+ NewCallback(&callback_test, &UpdateCallbackTest::Callback));
+ config_->ProcessEventSignal(kReasons[r], conf);
+ EXPECT_TRUE(callback_test.called()) << message;
+ EXPECT_EQ(base::StringPrintf("%u.0.0.0", r), config_->properties().address)
+ << message;
+ }
+}
+
+TEST_F(DHCPConfigTest, ProcessEventSignalUnknown) {
+ DHCPConfig::Configuration conf;
+ conf[DHCPConfig::kConfigurationKeyIPAddress].writer().append_uint32(
+ 0x01020304);
+ static const char kReasonUnknown[] = "UNKNOWN_REASON";
+ UpdateCallbackTest callback_test(kReasonUnknown, config_, false);
+ config_->RegisterUpdateCallback(
+ NewCallback(&callback_test, &UpdateCallbackTest::Callback));
+ config_->ProcessEventSignal(kReasonUnknown, conf);
+ EXPECT_FALSE(callback_test.called());
+ EXPECT_TRUE(config_->properties().address.empty());
+}
+
+
TEST_F(DHCPConfigTest, ReleaseIP) {
config_->pid_ = 1 << 18; // Ensure unknown positive PID.
EXPECT_CALL(*proxy_, DoRelease(kDeviceName)).Times(1);
diff --git a/dhcp_provider.cc b/dhcp_provider.cc
index bbf73fb..035484e 100644
--- a/dhcp_provider.cc
+++ b/dhcp_provider.cc
@@ -36,7 +36,7 @@
DHCPConfigRefPtr DHCPProvider::GetConfig(int pid) {
VLOG(2) << __func__;
- PIDConfigMap::iterator it = configs_.find(pid);
+ PIDConfigMap::const_iterator it = configs_.find(pid);
if (it == configs_.end()) {
return NULL;
}
diff --git a/ipconfig.cc b/ipconfig.cc
index 1001fe6..04b5638 100644
--- a/ipconfig.cc
+++ b/ipconfig.cc
@@ -36,15 +36,15 @@
return false;
}
-void IPConfig::UpdateProperties(const Properties &properties) {
+void IPConfig::UpdateProperties(const Properties &properties, bool success) {
properties_ = properties;
if (update_callback_.get()) {
- update_callback_->Run(this);
+ update_callback_->Run(this, success);
}
}
void IPConfig::RegisterUpdateCallback(
- Callback1<IPConfigRefPtr>::Type *callback) {
+ Callback2<IPConfigRefPtr, bool>::Type *callback) {
update_callback_.reset(callback);
}
diff --git a/ipconfig.h b/ipconfig.h
index f822b2c..b1815ed 100644
--- a/ipconfig.h
+++ b/ipconfig.h
@@ -46,9 +46,11 @@
// Registers a callback that's executed every time the configuration
// properties change. Takes ownership of |callback|. Pass NULL to remove a
- // callback. The callback's argument is a pointer to this IP configuration
- // instance allowing clients to more easily manage multiple IP configurations.
- void RegisterUpdateCallback(Callback1<IPConfigRefPtr>::Type *callback);
+ // callback. The callback's first argument is a pointer to this IP
+ // configuration instance allowing clients to more easily manage multiple IP
+ // configurations. The callback's second argument is set to false if IP
+ // configuration failed.
+ void RegisterUpdateCallback(Callback2<IPConfigRefPtr, bool>::Type *callback);
const Properties &properties() const { return properties_; }
@@ -61,8 +63,8 @@
protected:
// Updates the IP configuration properties and notifies registered listeners
- // about the event.
- void UpdateProperties(const Properties &properties);
+ // about the event. |success| is set to false if the IP configuration failed.
+ void UpdateProperties(const Properties &properties, bool success);
private:
FRIEND_TEST(IPConfigTest, UpdateCallback);
@@ -70,7 +72,7 @@
DeviceConstRefPtr device_;
Properties properties_;
- scoped_ptr<Callback1<IPConfigRefPtr>::Type> update_callback_;
+ scoped_ptr<Callback2<IPConfigRefPtr, bool>::Type> update_callback_;
DISALLOW_COPY_AND_ASSIGN(IPConfig);
};
diff --git a/ipconfig_unittest.cc b/ipconfig_unittest.cc
index 0ff911a..efb565f 100644
--- a/ipconfig_unittest.cc
+++ b/ipconfig_unittest.cc
@@ -52,7 +52,7 @@
properties.domain_search.push_back("zoo.org");
properties.domain_search.push_back("zoo.com");
properties.mtu = 700;
- ipconfig_->UpdateProperties(properties);
+ ipconfig_->UpdateProperties(properties, true);
EXPECT_EQ("1.2.3.4", ipconfig_->properties().address);
EXPECT_EQ(24, ipconfig_->properties().subnet_cidr);
EXPECT_EQ("11.22.33.44", ipconfig_->properties().broadcast_address);
@@ -67,31 +67,40 @@
EXPECT_EQ(700, ipconfig_->properties().mtu);
}
+namespace {
+
class UpdateCallbackTest {
public:
- UpdateCallbackTest(IPConfigRefPtr ipconfig)
+ UpdateCallbackTest(IPConfigRefPtr ipconfig, bool success)
: ipconfig_(ipconfig),
+ success_(success),
called_(false) {}
- void Callback(IPConfigRefPtr ipconfig) {
+ void Callback(IPConfigRefPtr ipconfig, bool success) {
called_ = true;
- EXPECT_EQ(ipconfig.get(), ipconfig_.get());
+ EXPECT_EQ(ipconfig_.get(), ipconfig.get());
+ EXPECT_EQ(success_, success);
}
bool called() const { return called_; }
private:
IPConfigRefPtr ipconfig_;
+ bool success_;
bool called_;
};
+} // namespace {}
+
TEST_F(IPConfigTest, UpdateCallback) {
- UpdateCallbackTest callback_test(ipconfig_);
- ASSERT_FALSE(callback_test.called());
- ipconfig_->RegisterUpdateCallback(
- NewCallback(&callback_test, &UpdateCallbackTest::Callback));
- ipconfig_->UpdateProperties(IPConfig::Properties());
- EXPECT_TRUE(callback_test.called());
+ for (int success = 0; success < 2; success++) {
+ UpdateCallbackTest callback_test(ipconfig_, success);
+ ASSERT_FALSE(callback_test.called());
+ ipconfig_->RegisterUpdateCallback(
+ NewCallback(&callback_test, &UpdateCallbackTest::Callback));
+ ipconfig_->UpdateProperties(IPConfig::Properties(), success);
+ EXPECT_TRUE(callback_test.called());
+ }
}
} // namespace shill