shill: Implement DHCPConfig::ReleaseIP
Also, add some unit tests, and some cleanup.
BUG=chromium-os:16365,chromium-os:16013
TEST=unit tests
Change-Id: I896bce08c6f177c9d5f6c5772c9208e8223c39df
Reviewed-on: http://gerrit.chromium.org/gerrit/2486
Reviewed-by: Chris Masone <cmasone@chromium.org>
Reviewed-by: Darin Petkov <petkov@chromium.org>
Tested-by: Darin Petkov <petkov@chromium.org>
diff --git a/Makefile b/Makefile
index 730fc00..df9da75 100644
--- a/Makefile
+++ b/Makefile
@@ -76,6 +76,7 @@
dbus_adaptor_unittest.o \
device_info_unittest.o \
dhcp_config_unittest.o \
+ dhcp_provider_unittest.o \
ipconfig_unittest.o \
manager_unittest.o \
mock_control.o \
diff --git a/dhcp_config.cc b/dhcp_config.cc
index 49bc8ff..ffe219b 100644
--- a/dhcp_config.cc
+++ b/dhcp_config.cc
@@ -49,16 +49,7 @@
// 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;
- }
+ // Make sure we don't get any callbacks to the destroyed instance.
CleanupClientState();
}
@@ -68,16 +59,19 @@
return Start();
}
if (!proxy_.get()) {
- LOG(ERROR)
- << "Unable to acquire destination address before receiving request.";
- return false;
+ LOG(ERROR) << "Unable to request IP before acquiring destination.";
+ return Restart();
}
return RenewIP();
}
bool DHCPConfig::RenewIP() {
VLOG(2) << __func__ << ": " << GetDeviceName();
- if (!pid_ || !proxy_.get()) {
+ if (!pid_) {
+ return false;
+ }
+ if (!proxy_.get()) {
+ LOG(ERROR) << "Unable to renew IP before acquiring destination.";
return false;
}
proxy_->DoRebind(GetDeviceName());
@@ -86,7 +80,16 @@
bool DHCPConfig::ReleaseIP() {
VLOG(2) << __func__ << ": " << GetDeviceName();
- // TODO(petkov): Implement D-Bus calls to Release and stop the process.
+ if (!pid_) {
+ return true;
+ }
+ if (!proxy_.get()) {
+ LOG(ERROR) << "Unable to release IP before acquiring destination.";
+ return false;
+ }
+ proxy_->DoRelease(GetDeviceName());
+ Stop();
+ return true;
}
void DHCPConfig::InitProxy(DBus::Connection *connection, const char *service) {
@@ -110,30 +113,30 @@
bool DHCPConfig::Start() {
VLOG(2) << __func__ << ": " << GetDeviceName();
- char *argv[4], *envp[1];
- argv[0] = const_cast<char *>(kDHCPCDPath);
- argv[1] = const_cast<char *>("-B"); // foreground
- argv[2] = const_cast<char *>(GetDeviceName().c_str());
- argv[3] = NULL;
+ char *argv[4] = {
+ const_cast<char *>(kDHCPCDPath),
+ const_cast<char *>("-B"), // foreground
+ const_cast<char *>(GetDeviceName().c_str()),
+ NULL
+ };
+ char *envp[1] = { NULL };
- envp[0] = NULL;
-
- GPid pid = 0;
+ CHECK(!pid_);
if (!glib_->SpawnAsync(NULL,
argv,
envp,
G_SPAWN_DO_NOT_REAP_CHILD,
NULL,
NULL,
- &pid,
+ &pid_,
NULL)) {
LOG(ERROR) << "Unable to spawn " << kDHCPCDPath;
return false;
}
- pid_ = pid;
LOG(INFO) << "Spawned " << kDHCPCDPath << " with pid: " << pid_;
provider_->BindPID(pid_, this);
- child_watch_tag_ = glib_->ChildWatchAdd(pid, ChildWatchCallback, this);
+ CHECK(!child_watch_tag_);
+ child_watch_tag_ = glib_->ChildWatchAdd(pid_, ChildWatchCallback, this);
return true;
}
@@ -144,6 +147,21 @@
}
}
+bool DHCPConfig::Restart() {
+ // Check to ensure that this instance doesn't get destroyed in the middle of
+ // this call. If stopping a running client while there's only one reference to
+ // this instance, we will end up destroying it when the PID is unbound from
+ // the Provider. Since the Provider doesn't invoke Restart, this would mean
+ // that Restart was erroneously executed through a bare reference.
+ CHECK(!pid_ || !HasOneRef());
+ Stop();
+ if (pid_) {
+ provider_->UnbindPID(pid_);
+ }
+ CleanupClientState();
+ return Start();
+}
+
string DHCPConfig::GetIPv4AddressString(unsigned int address) {
char str[INET_ADDRSTRLEN];
if (inet_ntop(AF_INET, &address, str, arraysize(str))) {
@@ -226,6 +244,14 @@
}
void DHCPConfig::CleanupClientState() {
+ if (child_watch_tag_) {
+ glib_->SourceRemove(child_watch_tag_);
+ child_watch_tag_ = 0;
+ }
+ if (pid_) {
+ glib_->SpawnClosePID(pid_);
+ pid_ = 0;
+ }
file_util::Delete(root_.Append(base::StringPrintf(kDHCPCDPathFormatLease,
GetDeviceName().c_str())),
false);
diff --git a/dhcp_config.h b/dhcp_config.h
index 6d4ca2b..1152d61 100644
--- a/dhcp_config.h
+++ b/dhcp_config.h
@@ -7,9 +7,9 @@
#include <base/file_path.h>
#include <base/memory/scoped_ptr.h>
+#include <dbus-c++/connection.h>
#include <glib.h>
#include <gtest/gtest_prod.h> // for FRIEND_TEST
-#include <dbus-c++/connection.h>
#include "shill/device.h"
#include "shill/ipconfig.h"
@@ -55,10 +55,18 @@
const Configuration &configuration);
private:
+ friend class DHCPConfigTest;
FRIEND_TEST(DHCPConfigTest, GetIPv4AddressString);
FRIEND_TEST(DHCPConfigTest, ParseConfiguration);
+ FRIEND_TEST(DHCPConfigTest, ReleaseIP);
+ FRIEND_TEST(DHCPConfigTest, RenewIP);
+ FRIEND_TEST(DHCPConfigTest, RequestIP);
+ FRIEND_TEST(DHCPConfigTest, Restart);
+ FRIEND_TEST(DHCPConfigTest, RestartNoClient);
FRIEND_TEST(DHCPConfigTest, StartFail);
FRIEND_TEST(DHCPConfigTest, StartSuccess);
+ FRIEND_TEST(DHCPConfigTest, Stop);
+ FRIEND_TEST(DHCPProviderTest, CreateConfig);
static const char kDHCPCDPath[];
static const char kDHCPCDPathFormatLease[];
@@ -70,6 +78,10 @@
// Stops dhcpcd if running.
void Stop();
+ // Stops dhcpcd if already running and then starts it. Returns true on success
+ // and false otherwise.
+ bool Restart();
+
// Parses |configuration| into |properties|. Returns true on success, and
// false otherwise.
bool ParseConfiguration(const Configuration& configuration,
@@ -82,6 +94,8 @@
// Called when the dhcpcd client process exits.
static void ChildWatchCallback(GPid pid, gint status, gpointer data);
+ // Cleans up remaining state from a running client, if any, including freeing
+ // its GPid, exit watch callback, and state files.
void CleanupClientState();
DHCPProvider *provider_;
diff --git a/dhcp_config_unittest.cc b/dhcp_config_unittest.cc
index c25ee3d..cb9adfb 100644
--- a/dhcp_config_unittest.cc
+++ b/dhcp_config_unittest.cc
@@ -10,6 +10,7 @@
#include "shill/dhcp_provider.h"
#include "shill/mock_control.h"
#include "shill/mock_device.h"
+#include "shill/mock_dhcp_proxy.h"
#include "shill/mock_glib.h"
using std::string;
@@ -33,12 +34,16 @@
NULL,
kDeviceName,
0)),
- config_(new DHCPConfig(DHCPProvider::GetInstance(), device_, &glib_)) {}
+ proxy_(new MockDHCPProxy()),
+ config_(new DHCPConfig(DHCPProvider::GetInstance(), device_, &glib_)) {
+ config_->proxy_.reset(proxy_); // pass ownership
+ }
protected:
MockGLib glib_;
MockControl control_interface_;
scoped_refptr<MockDevice> device_;
+ MockDHCPProxy * const proxy_;
DHCPConfigRefPtr config_;
};
@@ -108,6 +113,66 @@
EXPECT_EQ(0, config_->pid_);
}
+TEST_F(DHCPConfigTest, ReleaseIP) {
+ config_->pid_ = 1 << 18; // Ensure unknown positive PID.
+ EXPECT_CALL(*proxy_, DoRelease(kDeviceName)).Times(1);
+ EXPECT_TRUE(config_->ReleaseIP());
+ config_->pid_ = 0;
+}
+
+TEST_F(DHCPConfigTest, RenewIP) {
+ config_->pid_ = 456;
+ EXPECT_CALL(*proxy_, DoRebind(kDeviceName)).Times(1);
+ EXPECT_TRUE(config_->RenewIP());
+ config_->pid_ = 0;
+}
+
+TEST_F(DHCPConfigTest, RequestIP) {
+ config_->pid_ = 567;
+ EXPECT_CALL(*proxy_, DoRebind(kDeviceName)).Times(1);
+ EXPECT_TRUE(config_->RenewIP());
+ config_->pid_ = 0;
+}
+
+TEST_F(DHCPConfigTest, Restart) {
+ const int kPID1 = 1 << 17; // Ensure unknown positive PID.
+ const int kPID2 = 987;
+ const unsigned int kTag1 = 11;
+ const unsigned int kTag2 = 22;
+ config_->pid_ = kPID1;
+ config_->child_watch_tag_ = kTag1;
+ DHCPProvider::GetInstance()->BindPID(kPID1, config_);
+ EXPECT_CALL(glib_, SourceRemove(kTag1)).WillOnce(Return(true));
+ EXPECT_CALL(glib_, SpawnClosePID(kPID1)).Times(1);
+ EXPECT_CALL(glib_, SpawnAsync(_, _, _, _, _, _, _, _))
+ .WillOnce(DoAll(SetArgumentPointee<6>(kPID2), Return(true)));
+ EXPECT_CALL(glib_, ChildWatchAdd(kPID2, _, _)).WillOnce(Return(kTag2));
+ EXPECT_TRUE(config_->Restart());
+ EXPECT_EQ(kPID2, config_->pid_);
+ EXPECT_EQ(config_.get(), DHCPProvider::GetInstance()->GetConfig(kPID2).get());
+ EXPECT_EQ(kTag2, config_->child_watch_tag_);
+ DHCPProvider::GetInstance()->UnbindPID(kPID2);
+ config_->pid_ = 0;
+ config_->child_watch_tag_ = 0;
+}
+
+TEST_F(DHCPConfigTest, RestartNoClient) {
+ const int kPID = 777;
+ const unsigned int kTag = 66;
+ EXPECT_CALL(glib_, SourceRemove(_)).Times(0);
+ EXPECT_CALL(glib_, SpawnClosePID(_)).Times(0);
+ EXPECT_CALL(glib_, SpawnAsync(_, _, _, _, _, _, _, _))
+ .WillOnce(DoAll(SetArgumentPointee<6>(kPID), Return(true)));
+ EXPECT_CALL(glib_, ChildWatchAdd(kPID, _, _)).WillOnce(Return(kTag));
+ EXPECT_TRUE(config_->Restart());
+ EXPECT_EQ(kPID, config_->pid_);
+ EXPECT_EQ(config_.get(), DHCPProvider::GetInstance()->GetConfig(kPID).get());
+ EXPECT_EQ(kTag, config_->child_watch_tag_);
+ DHCPProvider::GetInstance()->UnbindPID(kPID);
+ config_->pid_ = 0;
+ config_->child_watch_tag_ = 0;
+}
+
TEST_F(DHCPConfigTest, StartSuccess) {
const int kPID = 123456;
const unsigned int kTag = 55;
@@ -116,9 +181,8 @@
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(kPID)->second.get());
+ EXPECT_EQ(config_.get(), DHCPProvider::GetInstance()->GetConfig(kPID).get());
+ EXPECT_EQ(kTag, config_->child_watch_tag_);
ScopedTempDir temp_dir;
config_->root_ = temp_dir.path();
@@ -135,8 +199,18 @@
EXPECT_CALL(glib_, SpawnClosePID(kPID)).Times(1);
DHCPConfig::ChildWatchCallback(kPID, 0, config_.get());
+ EXPECT_EQ(NULL, DHCPProvider::GetInstance()->GetConfig(kPID).get());
EXPECT_FALSE(file_util::PathExists(pid_file));
EXPECT_FALSE(file_util::PathExists(lease_file));
}
+TEST_F(DHCPConfigTest, Stop) {
+ // Ensure no crashes.
+ const int kPID = 1 << 17; // Ensure unknown positive PID.
+ config_->Stop();
+ config_->pid_ = kPID;
+ config_->Stop();
+ EXPECT_CALL(glib_, SpawnClosePID(kPID)).Times(1); // Invoked by destuctor.
+}
+
} // namespace shill
diff --git a/dhcp_provider.cc b/dhcp_provider.cc
index 163b1ca..bbf73fb 100644
--- a/dhcp_provider.cc
+++ b/dhcp_provider.cc
@@ -31,14 +31,14 @@
DHCPConfigRefPtr DHCPProvider::CreateConfig(DeviceConstRefPtr device) {
VLOG(2) << __func__;
- return DHCPConfigRefPtr(new DHCPConfig(this, device, glib_));
+ return new DHCPConfig(this, device, glib_);
}
-DHCPConfigRefPtr DHCPProvider::GetConfig(unsigned int pid) {
+DHCPConfigRefPtr DHCPProvider::GetConfig(int pid) {
VLOG(2) << __func__;
PIDConfigMap::iterator it = configs_.find(pid);
if (it == configs_.end()) {
- return DHCPConfigRefPtr(NULL);
+ return NULL;
}
return it->second;
}
diff --git a/dhcp_provider.h b/dhcp_provider.h
index ca7014b..f60ac02 100644
--- a/dhcp_provider.h
+++ b/dhcp_provider.h
@@ -43,7 +43,7 @@
// Returns the DHCP configuration associated with DHCP client |pid|. Return
// NULL if |pid| is not bound to a configuration.
- DHCPConfigRefPtr GetConfig(unsigned int pid);
+ DHCPConfigRefPtr GetConfig(int pid);
// Binds a |pid| to a DHCP |config|. When a DHCP config spawns a new DHCP
// client, it binds itself to that client's |pid|.
@@ -56,7 +56,8 @@
private:
friend struct DefaultSingletonTraits<DHCPProvider>;
- FRIEND_TEST(DHCPConfigTest, StartSuccess);
+ friend class DHCPProviderTest;
+ FRIEND_TEST(DHCPProviderTest, CreateConfig);
typedef std::map<int, DHCPConfigRefPtr> PIDConfigMap;
diff --git a/dhcp_provider_unittest.cc b/dhcp_provider_unittest.cc
new file mode 100644
index 0000000..e36643a
--- /dev/null
+++ b/dhcp_provider_unittest.cc
@@ -0,0 +1,45 @@
+// Copyright (c) 2011 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/dhcp_provider.h"
+#include "shill/mock_control.h"
+#include "shill/mock_device.h"
+#include "shill/mock_glib.h"
+
+using testing::Test;
+
+namespace shill {
+
+namespace {
+const char kDeviceName[] = "testdevicename";
+} // namespace {}
+
+class DHCPProviderTest : public Test {
+ public:
+ DHCPProviderTest()
+ : device_(new MockDevice(&control_interface_,
+ NULL,
+ NULL,
+ kDeviceName,
+ 0)),
+ provider_(DHCPProvider::GetInstance()) {
+ provider_->glib_ = &glib_;
+ }
+
+ protected:
+ MockGLib glib_;
+ MockControl control_interface_;
+ scoped_refptr<MockDevice> device_;
+ DHCPProvider *provider_;
+};
+
+TEST_F(DHCPProviderTest, CreateConfig) {
+ DHCPConfigRefPtr config = provider_->CreateConfig(device_);
+ EXPECT_TRUE(config.get());
+ EXPECT_EQ(&glib_, config->glib_);
+ EXPECT_EQ(kDeviceName, config->GetDeviceName());
+ EXPECT_TRUE(provider_->configs_.empty());
+}
+
+} // namespace shill
diff --git a/dhcp_proxy_interface.h b/dhcp_proxy_interface.h
index 9ecc739..275a91b 100644
--- a/dhcp_proxy_interface.h
+++ b/dhcp_proxy_interface.h
@@ -15,6 +15,7 @@
virtual ~DHCPProxyInterface() {}
virtual void DoRebind(const std::string &interface) = 0;
+ virtual void DoRelease(const std::string &interface) = 0;
};
class DHCPListenerInterface {
diff --git a/dhcpcd_proxy.cc b/dhcpcd_proxy.cc
index 2552a29..ec308f6 100644
--- a/dhcpcd_proxy.cc
+++ b/dhcpcd_proxy.cc
@@ -80,6 +80,10 @@
Rebind(interface);
}
+void DHCPCDProxy::DoRelease(const string &interface) {
+ Release(interface);
+}
+
void DHCPCDProxy::Event(const uint32_t &pid,
const std::string &reason,
const DHCPConfig::Configuration &configuration) {
diff --git a/dhcpcd_proxy.h b/dhcpcd_proxy.h
index a178c93..ebb61b5 100644
--- a/dhcpcd_proxy.h
+++ b/dhcpcd_proxy.h
@@ -47,6 +47,7 @@
// Inherited from DHCPProxyInterface.
virtual void DoRebind(const std::string &interface);
+ virtual void DoRelease(const std::string &interface);
// Signal callbacks inherited from dhcpcd_proxy. Note that these callbacks are
// unused because signals are dispatched directly to the DHCP configuration
diff --git a/mock_dhcp_proxy.h b/mock_dhcp_proxy.h
new file mode 100644
index 0000000..dd390b9
--- /dev/null
+++ b/mock_dhcp_proxy.h
@@ -0,0 +1,22 @@
+// Copyright (c) 2011 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_DHCP_PROXY_H_
+#define SHILL_MOCK_DHCP_PROXY_H_
+
+#include <gmock/gmock.h>
+
+#include "shill/dhcp_proxy_interface.h"
+
+namespace shill {
+
+class MockDHCPProxy : public DHCPProxyInterface {
+ public:
+ MOCK_METHOD1(DoRebind, void(const std::string &interface));
+ MOCK_METHOD1(DoRelease, void(const std::string &interface));
+};
+
+} // namespace shill
+
+#endif // SHILL_MOCK_DHCP_PROXY_H_