shill: l2tpipsec: Cleanup on disconnect.
Previously, we would mark the service as disconnected but we would
leave a bunch of resources around such as the l2tpipsec_manager
process.
BUG=chromium-os:36095
TEST=unit tests; established l2tp/ipsec, shut down server, checked
that shill disconnects right away and shuts down l2tpipsec_vpn and
pppd on the client.
Change-Id: I729eac375ca2229311decd52e20acfa4332e2a59
Reviewed-on: https://gerrit.chromium.org/gerrit/38019
Tested-by: Darin Petkov <petkov@chromium.org>
Reviewed-by: Paul Stewart <pstew@chromium.org>
Commit-Ready: Darin Petkov <petkov@chromium.org>
diff --git a/l2tp_ipsec_driver.cc b/l2tp_ipsec_driver.cc
index c01f7ae..89d9a3c 100644
--- a/l2tp_ipsec_driver.cc
+++ b/l2tp_ipsec_driver.cc
@@ -4,6 +4,7 @@
#include "shill/l2tp_ipsec_driver.h"
+#include <base/bind.h>
#include <base/file_util.h>
#include <base/string_util.h>
#include <chromeos/dbus/service_constants.h>
@@ -17,6 +18,7 @@
#include "shill/vpn.h"
#include "shill/vpn_service.h"
+using base::Bind;
using base::Closure;
using std::map;
using std::string;
@@ -384,7 +386,11 @@
LOG(INFO) << "IP configuration received: " << reason;
if (reason != kL2TPIPSecReasonConnect) {
- device_->OnDisconnected();
+ DCHECK(reason == kL2TPIPSecReasonDisconnect);
+ // Avoid destroying the RPC task inside the adaptor callback by doing it
+ // from the main event loop.
+ dispatcher()->PostTask(Bind(&DeleteRPCTask, rpc_task_.release()));
+ Cleanup(Service::kStateFailure);
return;
}
@@ -414,6 +420,11 @@
StopConnectTimeout();
}
+// static
+void L2TPIPSecDriver::DeleteRPCTask(RPCTask *rpc_task) {
+ delete rpc_task;
+}
+
KeyValueStore L2TPIPSecDriver::GetProvider(Error *error) {
SLOG(VPN, 2) << __func__;
KeyValueStore props = VPNDriver::GetProvider(error);
diff --git a/l2tp_ipsec_driver.h b/l2tp_ipsec_driver.h
index 4ff6489..e7231b8 100644
--- a/l2tp_ipsec_driver.h
+++ b/l2tp_ipsec_driver.h
@@ -71,7 +71,7 @@
FRIEND_TEST(L2TPIPSecDriverTest, InitOptionsNoHost);
FRIEND_TEST(L2TPIPSecDriverTest, InitPSKOptions);
FRIEND_TEST(L2TPIPSecDriverTest, Notify);
- FRIEND_TEST(L2TPIPSecDriverTest, NotifyFail);
+ FRIEND_TEST(L2TPIPSecDriverTest, NotifyDisconnected);
FRIEND_TEST(L2TPIPSecDriverTest, OnConnectionDisconnected);
FRIEND_TEST(L2TPIPSecDriverTest, OnL2TPIPSecVPNDied);
FRIEND_TEST(L2TPIPSecDriverTest, ParseIPConfiguration);
@@ -121,6 +121,8 @@
virtual void Notify(const std::string &reason,
const std::map<std::string, std::string> &dict);
+ static void DeleteRPCTask(RPCTask *rpc_task);
+
ControlInterface *control_;
Metrics *metrics_;
DeviceInfo *device_info_;
diff --git a/l2tp_ipsec_driver_unittest.cc b/l2tp_ipsec_driver_unittest.cc
index caa0fbc..cf4a297 100644
--- a/l2tp_ipsec_driver_unittest.cc
+++ b/l2tp_ipsec_driver_unittest.cc
@@ -48,7 +48,8 @@
service_(new MockVPNService(&control_, &dispatcher_, &metrics_,
&manager_, driver_)),
device_(new MockVPN(&control_, &dispatcher_, &metrics_, &manager_,
- kInterfaceName, kInterfaceIndex)) {
+ kInterfaceName, kInterfaceIndex)),
+ test_rpc_task_destroyed_(false) {
driver_->nss_ = &nss_;
driver_->process_killer_ = &process_killer_;
}
@@ -67,6 +68,10 @@
ASSERT_TRUE(temp_dir_.Delete());
}
+ void set_test_rpc_task_destroyed(bool destroyed) {
+ test_rpc_task_destroyed_ = destroyed;
+ }
+
protected:
static const char kInterfaceName[];
static const int kInterfaceIndex;
@@ -101,8 +106,33 @@
scoped_refptr<MockVPN> device_;
MockNSS nss_;
MockProcessKiller process_killer_;
+ bool test_rpc_task_destroyed_;
};
+namespace {
+
+class TestRPCTask : public RPCTask {
+ public:
+ TestRPCTask(ControlInterface *control, L2TPIPSecDriverTest *test);
+ virtual ~TestRPCTask();
+
+ private:
+ L2TPIPSecDriverTest *test_;
+};
+
+TestRPCTask::TestRPCTask(ControlInterface *control, L2TPIPSecDriverTest *test)
+ : RPCTask(control, test),
+ test_(test) {
+ test_->set_test_rpc_task_destroyed(false);
+}
+
+TestRPCTask::~TestRPCTask() {
+ test_->set_test_rpc_task_destroyed(true);
+ test_ = NULL;
+}
+
+} // namespace
+
const char L2TPIPSecDriverTest::kInterfaceName[] = "ppp0";
const int L2TPIPSecDriverTest::kInterfaceIndex = 123;
@@ -517,13 +547,20 @@
EXPECT_FALSE(driver_->IsConnectTimeoutStarted());
}
-TEST_F(L2TPIPSecDriverTest, NotifyFail) {
+TEST_F(L2TPIPSecDriverTest, NotifyDisconnected) {
map<string, string> dict;
driver_->device_ = device_;
+ driver_->rpc_task_.reset(new TestRPCTask(&control_, this));
+ EXPECT_FALSE(test_rpc_task_destroyed_);
EXPECT_CALL(*device_, OnDisconnected());
- driver_->StartConnectTimeout();
+ EXPECT_CALL(*device_, SetEnabled(false));
driver_->Notify(kL2TPIPSecReasonDisconnect, dict);
- EXPECT_TRUE(driver_->IsConnectTimeoutStarted());
+ EXPECT_FALSE(driver_->device_);
+ EXPECT_FALSE(test_rpc_task_destroyed_);
+ EXPECT_FALSE(driver_->rpc_task_.get());
+ dispatcher_.PostTask(MessageLoop::QuitClosure());
+ dispatcher_.DispatchForever();
+ EXPECT_TRUE(test_rpc_task_destroyed_);
}
TEST_F(L2TPIPSecDriverTest, VerifyPaths) {
diff --git a/rpc_task.cc b/rpc_task.cc
index 6e82fc5..906bc3c 100644
--- a/rpc_task.cc
+++ b/rpc_task.cc
@@ -23,11 +23,11 @@
unique_name_(base::UintToString(serial_number_++)),
adaptor_(control_interface->CreateRPCTaskAdaptor(this)) {
CHECK(delegate);
- SLOG(Task, 2) << "RPCTask " + unique_name_ + " created.";
+ LOG(INFO) << "RPCTask " + unique_name_ + " created.";
}
RPCTask::~RPCTask() {
- SLOG(Task, 2) << "RPCTask " + unique_name_ + " destroyed.";
+ LOG(INFO) << "RPCTask " + unique_name_ + " destroyed.";
}
void RPCTask::GetLogin(string *user, string *password) {