shill: Almost complete support for terminating dhcpcd.
BUG=chromium-os:16365,chromium-os:16013
TEST=unit tests
Change-Id: I00a46e8364fc4de3cc48ef72c1b0a9a88e95e6a0
Reviewed-on: http://gerrit.chromium.org/gerrit/2435
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 13d9325..49bc8ff 100644
--- a/dhcp_config.cc
+++ b/dhcp_config.cc
@@ -6,7 +6,9 @@
#include <arpa/inet.h>
+#include <base/file_util.h>
#include <base/logging.h>
+#include <base/stringprintf.h>
#include "shill/dhcpcd_proxy.h"
#include "shill/dhcp_provider.h"
@@ -26,7 +28,8 @@
const char DHCPConfig::kConfigurationKeyRouters[] = "Routers";
const char DHCPConfig::kConfigurationKeySubnetCIDR[] = "SubnetCIDR";
const char DHCPConfig::kDHCPCDPath[] = "/sbin/dhcpcd";
-
+const char DHCPConfig::kDHCPCDPathFormatLease[] = "var/run/dhcpcd-%s.lease";
+const char DHCPConfig::kDHCPCDPathFormatPID[] = "var/run/dhcpcd-%s.pid";
DHCPConfig::DHCPConfig(DHCPProvider *provider,
DeviceConstRefPtr device,
@@ -34,15 +37,32 @@
: IPConfig(device),
provider_(provider),
pid_(0),
+ child_watch_tag_(0),
+ root_("/"),
glib_(glib) {
VLOG(2) << __func__ << ": " << GetDeviceName();
}
DHCPConfig::~DHCPConfig() {
VLOG(2) << __func__ << ": " << GetDeviceName();
+
+ // Don't leave behind dhcpcd running.
+ Stop();
+
+ // Somehow we got destroyed before the client died, most likely at exit. Make
+ // sure we don't get any callbacks to the destroyed instance.
+ if (child_watch_tag_) {
+ glib_->SourceRemove(child_watch_tag_);
+ child_watch_tag_ = 0;
+ }
+ if (pid_) {
+ glib_->SpawnClosePID(pid_);
+ pid_ = 0;
+ }
+ CleanupClientState();
}
-bool DHCPConfig::Request() {
+bool DHCPConfig::RequestIP() {
VLOG(2) << __func__ << ": " << GetDeviceName();
if (!pid_) {
return Start();
@@ -52,10 +72,10 @@
<< "Unable to acquire destination address before receiving request.";
return false;
}
- return Renew();
+ return RenewIP();
}
-bool DHCPConfig::Renew() {
+bool DHCPConfig::RenewIP() {
VLOG(2) << __func__ << ": " << GetDeviceName();
if (!pid_ || !proxy_.get()) {
return false;
@@ -64,6 +84,11 @@
return true;
}
+bool DHCPConfig::ReleaseIP() {
+ VLOG(2) << __func__ << ": " << GetDeviceName();
+ // TODO(petkov): Implement D-Bus calls to Release and stop the process.
+}
+
void DHCPConfig::InitProxy(DBus::Connection *connection, const char *service) {
if (!proxy_.get()) {
proxy_.reset(new DHCPCDProxy(connection, service));
@@ -108,10 +133,17 @@
pid_ = pid;
LOG(INFO) << "Spawned " << kDHCPCDPath << " with pid: " << pid_;
provider_->BindPID(pid_, this);
- // TODO(petkov): Add an exit watch to cleanup w/ g_spawn_close_pid.
+ child_watch_tag_ = glib_->ChildWatchAdd(pid, ChildWatchCallback, this);
return true;
}
+void DHCPConfig::Stop() {
+ if (pid_) {
+ VLOG(2) << "Terminating " << pid_;
+ PLOG_IF(ERROR, kill(pid_, SIGTERM) < 0);
+ }
+}
+
string DHCPConfig::GetIPv4AddressString(unsigned int address) {
char str[INET_ADDRSTRLEN];
if (inet_ntop(AF_INET, &address, str, arraysize(str))) {
@@ -178,4 +210,28 @@
return true;
}
+void DHCPConfig::ChildWatchCallback(GPid pid, gint status, gpointer data) {
+ 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.
+ config->provider_->UnbindPID(pid);
+}
+
+void DHCPConfig::CleanupClientState() {
+ file_util::Delete(root_.Append(base::StringPrintf(kDHCPCDPathFormatLease,
+ GetDeviceName().c_str())),
+ false);
+ file_util::Delete(root_.Append(base::StringPrintf(kDHCPCDPathFormatPID,
+ GetDeviceName().c_str())),
+ false);
+}
+
} // namespace shill
diff --git a/dhcp_config.h b/dhcp_config.h
index 0e25873..6d4ca2b 100644
--- a/dhcp_config.h
+++ b/dhcp_config.h
@@ -5,7 +5,9 @@
#ifndef SHILL_DHCP_CONFIG_
#define SHILL_DHCP_CONFIG_
+#include <base/file_path.h>
#include <base/memory/scoped_ptr.h>
+#include <glib.h>
#include <gtest/gtest_prod.h> // for FRIEND_TEST
#include <dbus-c++/connection.h>
@@ -40,8 +42,9 @@
virtual ~DHCPConfig();
// Inherited from IPConfig.
- virtual bool Request();
- virtual bool Renew();
+ virtual bool RequestIP();
+ virtual bool RenewIP();
+ virtual bool ReleaseIP();
// If |proxy_| is not initialized already, sets it to a new D-Bus proxy to
// |service|.
@@ -54,13 +57,19 @@
private:
FRIEND_TEST(DHCPConfigTest, GetIPv4AddressString);
FRIEND_TEST(DHCPConfigTest, ParseConfiguration);
- FRIEND_TEST(DHCPConfigTest, Start);
+ FRIEND_TEST(DHCPConfigTest, StartFail);
+ FRIEND_TEST(DHCPConfigTest, StartSuccess);
static const char kDHCPCDPath[];
+ static const char kDHCPCDPathFormatLease[];
+ static const char kDHCPCDPathFormatPID[];
// Starts dhcpcd, returns true on success and false otherwise.
bool Start();
+ // Stops dhcpcd if running.
+ void Stop();
+
// Parses |configuration| into |properties|. Returns true on success, and
// false otherwise.
bool ParseConfiguration(const Configuration& configuration,
@@ -70,15 +79,26 @@
// empty string on failure.
std::string GetIPv4AddressString(unsigned int address);
+ // Called when the dhcpcd client process exits.
+ static void ChildWatchCallback(GPid pid, gint status, gpointer data);
+
+ void CleanupClientState();
+
DHCPProvider *provider_;
// The PID of the spawned DHCP client. May be 0 if no client has been spawned
// yet or the client has died.
- unsigned int pid_;
+ int pid_;
+
+ // Child exit watch callback source tag.
+ unsigned int child_watch_tag_;
// The proxy for communicating with the DHCP client.
scoped_ptr<DHCPProxyInterface> proxy_;
+ // Root file path, used for testing.
+ FilePath root_;
+
GLibInterface *glib_;
DISALLOW_COPY_AND_ASSIGN(DHCPConfig);
diff --git a/dhcp_config_unittest.cc b/dhcp_config_unittest.cc
index 1c2b498..c25ee3d 100644
--- a/dhcp_config_unittest.cc
+++ b/dhcp_config_unittest.cc
@@ -2,6 +2,10 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
+#include <base/file_util.h>
+#include <base/memory/scoped_temp_dir.h>
+#include <base/stringprintf.h>
+
#include "shill/dhcp_config.h"
#include "shill/dhcp_provider.h"
#include "shill/mock_control.h"
@@ -17,10 +21,18 @@
namespace shill {
+namespace {
+const char kDeviceName[] = "testdevicename";
+} // namespace {}
+
class DHCPConfigTest : public Test {
public:
DHCPConfigTest()
- : device_(new MockDevice(&control_interface_, NULL, NULL, "testname", 0)),
+ : device_(new MockDevice(&control_interface_,
+ NULL,
+ NULL,
+ kDeviceName,
+ 0)),
config_(new DHCPConfig(DHCPProvider::GetInstance(), device_, &glib_)) {}
protected:
@@ -88,20 +100,43 @@
EXPECT_EQ(600, properties.mtu);
}
-TEST_F(DHCPConfigTest, Start) {
+TEST_F(DHCPConfigTest, StartFail) {
EXPECT_CALL(glib_, SpawnAsync(_, _, _, _, _, _, _, _))
.WillOnce(Return(false));
+ EXPECT_CALL(glib_, ChildWatchAdd(_, _, _)).Times(0);
EXPECT_FALSE(config_->Start());
EXPECT_EQ(0, config_->pid_);
+}
- const unsigned int kPID = 1234;
+TEST_F(DHCPConfigTest, StartSuccess) {
+ const int kPID = 123456;
+ const unsigned int kTag = 55;
EXPECT_CALL(glib_, SpawnAsync(_, _, _, _, _, _, _, _))
.WillOnce(DoAll(SetArgumentPointee<6>(kPID), Return(true)));
+ EXPECT_CALL(glib_, ChildWatchAdd(kPID, _, _)).WillOnce(Return(kTag));
EXPECT_TRUE(config_->Start());
EXPECT_EQ(kPID, config_->pid_);
DHCPProvider *provider = DHCPProvider::GetInstance();
ASSERT_TRUE(provider->configs_.find(kPID) != provider->configs_.end());
- EXPECT_EQ(config_.get(), provider->configs_.find(1234)->second.get());
+ EXPECT_EQ(config_.get(), provider->configs_.find(kPID)->second.get());
+
+ ScopedTempDir temp_dir;
+ config_->root_ = temp_dir.path();
+ FilePath varrun = temp_dir.path().Append("var/run");
+ EXPECT_TRUE(file_util::CreateDirectory(varrun));
+ FilePath pid_file =
+ varrun.Append(base::StringPrintf("dhcpcd-%s.pid", kDeviceName));
+ FilePath lease_file =
+ varrun.Append(base::StringPrintf("dhcpcd-%s.lease", kDeviceName));
+ EXPECT_EQ(0, file_util::WriteFile(pid_file, "", 0));
+ EXPECT_EQ(0, file_util::WriteFile(lease_file, "", 0));
+ ASSERT_TRUE(file_util::PathExists(pid_file));
+ ASSERT_TRUE(file_util::PathExists(lease_file));
+
+ EXPECT_CALL(glib_, SpawnClosePID(kPID)).Times(1);
+ DHCPConfig::ChildWatchCallback(kPID, 0, config_.get());
+ EXPECT_FALSE(file_util::PathExists(pid_file));
+ EXPECT_FALSE(file_util::PathExists(lease_file));
}
} // namespace shill
diff --git a/dhcp_provider.cc b/dhcp_provider.cc
index c506a77..163b1ca 100644
--- a/dhcp_provider.cc
+++ b/dhcp_provider.cc
@@ -43,12 +43,12 @@
return it->second;
}
-void DHCPProvider::BindPID(unsigned int pid, DHCPConfigRefPtr config) {
+void DHCPProvider::BindPID(int pid, DHCPConfigRefPtr config) {
VLOG(2) << __func__ << " pid: " << pid;
configs_[pid] = config;
}
-void DHCPProvider::UnbindPID(unsigned int pid) {
+void DHCPProvider::UnbindPID(int pid) {
VLOG(2) << __func__ << " pid: " << pid;
configs_.erase(pid);
}
diff --git a/dhcp_provider.h b/dhcp_provider.h
index 28e07e4..ca7014b 100644
--- a/dhcp_provider.h
+++ b/dhcp_provider.h
@@ -47,18 +47,18 @@
// Binds a |pid| to a DHCP |config|. When a DHCP config spawns a new DHCP
// client, it binds itself to that client's |pid|.
- void BindPID(unsigned int pid, DHCPConfigRefPtr config);
+ void BindPID(int pid, DHCPConfigRefPtr config);
// Unbinds a |pid|. This method is used by a DHCP config to signal the
// provider that the DHCP client has been terminated. This may result in
// destruction of the DHCP config instance if its reference count goes to 0.
- void UnbindPID(unsigned int pid);
+ void UnbindPID(int pid);
private:
friend struct DefaultSingletonTraits<DHCPProvider>;
- FRIEND_TEST(DHCPConfigTest, Start);
+ FRIEND_TEST(DHCPConfigTest, StartSuccess);
- typedef std::map<unsigned int, DHCPConfigRefPtr> PIDConfigMap;
+ typedef std::map<int, DHCPConfigRefPtr> PIDConfigMap;
// Private to ensure that this behaves as a singleton.
DHCPProvider();
diff --git a/glib.cc b/glib.cc
index dc33470..bf1cda7 100644
--- a/glib.cc
+++ b/glib.cc
@@ -6,6 +6,16 @@
namespace shill {
+guint GLib::ChildWatchAdd(GPid pid,
+ GChildWatchFunc function,
+ gpointer data) {
+ return g_child_watch_add(pid, function, data);
+}
+
+gboolean GLib::SourceRemove(guint tag) {
+ return g_source_remove(tag);
+}
+
gboolean GLib::SpawnAsync(const gchar *working_directory,
gchar **argv,
gchar **envp,
@@ -24,4 +34,8 @@
error);
}
+void GLib::SpawnClosePID(GPid pid) {
+ g_spawn_close_pid(pid);
+}
+
} // namespace shill
diff --git a/glib.h b/glib.h
index 274d3a2..8aa4da3 100644
--- a/glib.h
+++ b/glib.h
@@ -12,6 +12,10 @@
// A concrete implementation of the GLib interface.
class GLib : public GLibInterface {
public:
+ virtual guint ChildWatchAdd(GPid pid,
+ GChildWatchFunc function,
+ gpointer data);
+ virtual gboolean SourceRemove(guint tag);
virtual gboolean SpawnAsync(const gchar *working_directory,
gchar **argv,
gchar **envp,
@@ -20,6 +24,7 @@
gpointer user_data,
GPid *child_pid,
GError **error);
+ virtual void SpawnClosePID(GPid pid);
};
} // namespace shill
diff --git a/glib_interface.h b/glib_interface.h
index 37b3405..9017d1d 100644
--- a/glib_interface.h
+++ b/glib_interface.h
@@ -14,6 +14,12 @@
public:
virtual ~GLibInterface() {}
+ // g_child_watch_add
+ virtual guint ChildWatchAdd(GPid pid,
+ GChildWatchFunc function,
+ gpointer data) = 0;
+ // g_source_remove
+ virtual gboolean SourceRemove(guint tag) = 0;
// g_spawn_async
virtual gboolean SpawnAsync(const gchar *working_directory,
gchar **argv,
@@ -23,6 +29,8 @@
gpointer user_data,
GPid *child_pid,
GError **error) = 0;
+ // g_spawn_close_pid
+ virtual void SpawnClosePID(GPid pid) = 0;
};
} // namespace shill
diff --git a/ipconfig.cc b/ipconfig.cc
index 12c20f0..1001fe6 100644
--- a/ipconfig.cc
+++ b/ipconfig.cc
@@ -24,11 +24,15 @@
return device()->UniqueName();
}
-bool IPConfig::Request() {
+bool IPConfig::RequestIP() {
return false;
}
-bool IPConfig::Renew() {
+bool IPConfig::RenewIP() {
+ return false;
+}
+
+bool IPConfig::ReleaseIP() {
return false;
}
diff --git a/ipconfig.h b/ipconfig.h
index 5e2ae32..f822b2c 100644
--- a/ipconfig.h
+++ b/ipconfig.h
@@ -52,11 +52,12 @@
const Properties &properties() const { return properties_; }
- // Request or renew IP configuration. Return true on success, false
+ // Request, renew and release IP configuration. Return true on success, false
// otherwise. The default implementation always returns false indicating a
// failure.
- virtual bool Request();
- virtual bool Renew();
+ virtual bool RequestIP();
+ virtual bool RenewIP();
+ virtual bool ReleaseIP();
protected:
// Updates the IP configuration properties and notifies registered listeners
diff --git a/ipconfig_unittest.cc b/ipconfig_unittest.cc
index d5e9d35..0ff911a 100644
--- a/ipconfig_unittest.cc
+++ b/ipconfig_unittest.cc
@@ -28,12 +28,16 @@
EXPECT_EQ("testname", ipconfig_->GetDeviceName());
}
-TEST_F(IPConfigTest, Request) {
- EXPECT_FALSE(ipconfig_->Request());
+TEST_F(IPConfigTest, RequestIP) {
+ EXPECT_FALSE(ipconfig_->RequestIP());
}
-TEST_F(IPConfigTest, Renew) {
- EXPECT_FALSE(ipconfig_->Renew());
+TEST_F(IPConfigTest, RenewIP) {
+ EXPECT_FALSE(ipconfig_->RenewIP());
+}
+
+TEST_F(IPConfigTest, ReleaseIP) {
+ EXPECT_FALSE(ipconfig_->ReleaseIP());
}
TEST_F(IPConfigTest, UpdateProperties) {
diff --git a/mock_glib.h b/mock_glib.h
index 073217f..814db0b 100644
--- a/mock_glib.h
+++ b/mock_glib.h
@@ -13,6 +13,10 @@
class MockGLib : public GLibInterface {
public:
+ MOCK_METHOD3(ChildWatchAdd, guint(GPid pid,
+ GChildWatchFunc function,
+ gpointer data));
+ MOCK_METHOD1(SourceRemove, gboolean(guint tag));
MOCK_METHOD8(SpawnAsync, gboolean(const gchar *working_directory,
gchar **argv,
gchar **envp,
@@ -21,6 +25,7 @@
gpointer user_data,
GPid *child_pid,
GError **error));
+ MOCK_METHOD1(SpawnClosePID, void(GPid pid));
};
} // namespace shill