shill: create external_task abstraction
This moves code out of L2TPIPSecDriver into a new ExternalTask
class, and updates the L2TPIPSecDriver code appropriately.
While there:
- fix a couple bug in L2TPIPSecDriverTest::ExpectInFlags.
(The function was returning early, and not checking the value
of the flag.)
- remove RpcTask::GetRpcInterfaceIdentifier, since the only
place it was called from was a test.
BUG=chromium:209938
TEST=unit tests, manual
Manual test
-----------
- connect to CrOS_WPA2_Buffalo2_AG300H_2.4GHz
- /usr/local/lib/flimflam/test/connect-vpn l2tpipsec-psk \
Cisco-ASA-173 172.22.22.173 \
Cisco-ASA-173 chromeos_psk \
CrOS_Password <password>
- check that ppp0 has an IP address
- egrep "l2tpipsec_vpn.+pppd: local" /var/log/net.log
(should match)
Change-Id: I54aed25f14597d0324249aaddba2095d296ded7d
Reviewed-on: https://gerrit.chromium.org/gerrit/57193
Reviewed-by: Darin Petkov <petkov@chromium.org>
Commit-Queue: mukesh agrawal <quiche@chromium.org>
Tested-by: mukesh agrawal <quiche@chromium.org>
diff --git a/Makefile b/Makefile
index 5fc8edd..cf0aaeb 100644
--- a/Makefile
+++ b/Makefile
@@ -235,6 +235,7 @@
ethernet_eap_provider.o \
ethernet_eap_service.o \
ethernet_service.o \
+ external_task.o \
event_dispatcher.o \
file_io.o \
file_reader.o \
@@ -369,6 +370,7 @@
ethernet_eap_provider_unittest.o \
ethernet_eap_service_unittest.o \
ethernet_service_unittest.o \
+ external_task_unittest.o \
file_reader_unittest.o \
hook_table_unittest.o \
http_proxy_unittest.o \
@@ -411,6 +413,7 @@
mock_ethernet_eap_provider.o \
mock_ethernet_service.o \
mock_event_dispatcher.o \
+ mock_external_task.o \
mock_glib.o \
mock_http_request.o \
mock_ip_address_store.o \
diff --git a/dhcp_config.cc b/dhcp_config.cc
index 70aaab2..1774214 100644
--- a/dhcp_config.cc
+++ b/dhcp_config.cc
@@ -176,6 +176,8 @@
bool DHCPConfig::Start() {
SLOG(DHCP, 2) << __func__ << ": " << device_name();
+ // TODO(quiche): This should be migrated to use ExternalTask.
+ // (crbug.com/246263).
vector<char *> args;
args.push_back(const_cast<char *>(kDHCPCDPath));
args.push_back(const_cast<char *>("-B")); // Run in foreground.
diff --git a/external_task.cc b/external_task.cc
new file mode 100644
index 0000000..f9282a8
--- /dev/null
+++ b/external_task.cc
@@ -0,0 +1,116 @@
+// Copyright (c) 2013 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/external_task.h"
+
+#include "shill/error.h"
+#include "shill/process_killer.h"
+
+namespace shill {
+
+using base::FilePath;
+using std::map;
+using std::string;
+using std::vector;
+
+ExternalTask::ExternalTask(
+ ControlInterface *control,
+ GLib *glib,
+ const base::WeakPtr<RPCTaskDelegate> &task_delegate,
+ const base::Callback<void(pid_t, int)> &death_callback)
+ : control_(control),
+ glib_(glib),
+ process_killer_(ProcessKiller::GetInstance()),
+ task_delegate_(task_delegate),
+ death_callback_(death_callback),
+ pid_(0),
+ child_watch_tag_(0) {
+ CHECK(task_delegate_);
+}
+
+ExternalTask::~ExternalTask() {
+ ExternalTask::Stop();
+}
+
+bool ExternalTask::Start(const FilePath &program,
+ const vector<string> &arguments,
+ const map<string, string> &environment,
+ Error *error) {
+ CHECK(!pid_);
+ CHECK(!child_watch_tag_);
+ CHECK(!rpc_task_);
+
+ scoped_ptr<RPCTask> local_rpc_task(new RPCTask(control_, this));
+
+ // const_cast is safe here, because exec*() (and SpawnAsync) do not
+ // modify the strings passed to them. This isn't captured in the
+ // exec*() prototypes, due to limitations in ISO C.
+ // http://pubs.opengroup.org/onlinepubs/009695399/functions/exec.html
+ vector<char *> process_args;
+ process_args.push_back(const_cast<char *>(program.value().c_str()));
+ for (const auto &option : arguments) {
+ process_args.push_back(const_cast<char *>(option.c_str()));
+ }
+ process_args.push_back(NULL);
+
+ vector<char *> process_env;
+ vector<string> env_vars(local_rpc_task->GetEnvironment());
+ for (const auto &env_pair : environment) {
+ env_vars.push_back(string(env_pair.first + "=" + env_pair.second));
+ }
+ for (const auto &env_var : env_vars) {
+ // See above regarding const_cast.
+ process_env.push_back(const_cast<char *>(env_var.c_str()));
+ }
+ process_env.push_back(NULL);
+
+ if (!glib_->SpawnAsync(NULL,
+ process_args.data(),
+ process_env.data(),
+ G_SPAWN_DO_NOT_REAP_CHILD,
+ NULL,
+ NULL,
+ &pid_,
+ NULL)) {
+ Error::PopulateAndLog(error, Error::kInternalError,
+ string("Unable to spawn: ") + process_args[0]);
+ return false;
+ }
+ child_watch_tag_ = glib_->ChildWatchAdd(pid_, OnTaskDied, this);
+ rpc_task_.reset(local_rpc_task.release());
+ return true;
+}
+
+void ExternalTask::Stop() {
+ if (child_watch_tag_) {
+ glib_->SourceRemove(child_watch_tag_);
+ child_watch_tag_ = 0;
+ }
+ if (pid_) {
+ process_killer_->Kill(pid_, base::Closure());
+ pid_ = 0;
+ }
+ rpc_task_.reset();
+}
+
+void ExternalTask::GetLogin(string *user, string *password) {
+ return task_delegate_->GetLogin(user, password);
+}
+
+void ExternalTask::Notify(const string &event,
+ const map<string, string> &details) {
+ return task_delegate_->Notify(event, details);
+}
+
+// static
+void ExternalTask::OnTaskDied(GPid pid, gint status, gpointer data) {
+ LOG(INFO) << __func__ << "(" << pid << ", " << status << ")";
+ ExternalTask *me = reinterpret_cast<ExternalTask *>(data);
+ me->child_watch_tag_ = 0;
+ CHECK_EQ(pid, me->pid_);
+ me->pid_ = 0;
+ me->death_callback_.Run(pid, status);
+}
+
+} // namespace shill
diff --git a/external_task.h b/external_task.h
new file mode 100644
index 0000000..813a08e
--- /dev/null
+++ b/external_task.h
@@ -0,0 +1,91 @@
+// Copyright (c) 2013 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_EXTERNAL_TASK_H_
+#define SHILL_EXTERNAL_TASK_H_
+
+#include <sys/types.h>
+
+#include <map>
+#include <string>
+#include <vector>
+
+#include <base/callback.h>
+#include <base/file_path.h>
+#include <base/memory/scoped_ptr.h>
+#include <base/memory/weak_ptr.h>
+#include <gtest/gtest_prod.h> // for FRIEND_TEST
+
+#include "shill/glib.h"
+#include "shill/rpc_task.h"
+
+namespace shill {
+
+class ControlInterface;
+class Error;
+class ProcessKiller;
+
+class ExternalTask : public RPCTaskDelegate {
+ public:
+ ExternalTask(ControlInterface *control,
+ GLib *glib,
+ const base::WeakPtr<RPCTaskDelegate> &task_delegate,
+ const base::Callback<void(pid_t, int)> &death_callback);
+ virtual ~ExternalTask();
+
+ // Forks off a process to run |program|, with the command-line
+ // arguments |arguments|, and the environment variables specified in
+ // |environment|.
+ //
+ // On success, returns true, and leaves |error| unmodified.
+ // On failure, returns false, and sets |error|.
+ //
+ // |environment| SHOULD NOT contain kRPCTaskServiceVariable or
+ // kRPCTaskPathVariable, as that may prevent the child process
+ // from communicating back to the ExternalTask.
+ virtual bool Start(const base::FilePath &program,
+ const std::vector<std::string> &arguments,
+ const std::map<std::string, std::string> &environment,
+ Error *error);
+ virtual void Stop();
+
+ private:
+ friend class ExternalTaskTest;
+ FRIEND_TEST(ExternalTaskTest, Destructor);
+ FRIEND_TEST(ExternalTaskTest, GetLogin);
+ FRIEND_TEST(ExternalTaskTest, Notify);
+ FRIEND_TEST(ExternalTaskTest, OnTaskDied);
+ FRIEND_TEST(ExternalTaskTest, Start);
+ FRIEND_TEST(ExternalTaskTest, Stop);
+ FRIEND_TEST(ExternalTaskTest, StopNotStarted);
+
+ // Implements RPCTaskDelegate.
+ virtual void GetLogin(std::string *user, std::string *password) override;
+ virtual void Notify(
+ const std::string &event,
+ const std::map<std::string, std::string> &details) override;
+ // Called when the external process exits.
+ static void OnTaskDied(GPid pid, gint status, gpointer data);
+
+ ControlInterface *control_;
+ GLib *glib_;
+ ProcessKiller *process_killer_; // Field permits mocking.
+
+ scoped_ptr<RPCTask> rpc_task_;
+ base::WeakPtr<RPCTaskDelegate> task_delegate_;
+ base::Callback<void(pid_t, int)> death_callback_;
+
+ // The PID of the spawned process. May be 0 if no process has been
+ // spawned yet or the process has died.
+ pid_t pid_;
+
+ // Child exit watch callback source tag.
+ unsigned int child_watch_tag_;
+
+ DISALLOW_COPY_AND_ASSIGN(ExternalTask);
+};
+
+} // namespace shill
+
+#endif // SHILL_EXTERNAL_TASK_H_
diff --git a/external_task_unittest.cc b/external_task_unittest.cc
new file mode 100644
index 0000000..7922d64
--- /dev/null
+++ b/external_task_unittest.cc
@@ -0,0 +1,249 @@
+// Copyright (c) 2012 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/external_task.h"
+
+#include <map>
+#include <set>
+#include <string>
+#include <vector>
+
+#include <base/bind.h>
+#include <base/file_path.h>
+#include <base/memory/weak_ptr.h>
+#include <base/string_util.h>
+#include <gmock/gmock.h>
+#include <gtest/gtest.h>
+
+#include "shill/event_dispatcher.h"
+#include "shill/nice_mock_control.h"
+#include "shill/mock_adaptors.h"
+#include "shill/mock_glib.h"
+#include "shill/mock_process_killer.h"
+
+using std::map;
+using std::set;
+using std::string;
+using std::vector;
+using testing::_;
+using testing::Matcher;
+using testing::MatchesRegex;
+using testing::NiceMock;
+using testing::Return;
+using testing::SetArgumentPointee;
+using testing::StrEq;
+
+namespace shill {
+
+class ExternalTaskTest : public testing::Test,
+ public RPCTaskDelegate {
+ public:
+ ExternalTaskTest()
+ : weak_ptr_factory_(this),
+ death_callback_(
+ base::Bind(&ExternalTaskTest::TaskDiedCallback,
+ weak_ptr_factory_.GetWeakPtr())),
+ external_task_(
+ new ExternalTask(&control_, &glib_, weak_ptr_factory_.GetWeakPtr(),
+ death_callback_)),
+ test_rpc_task_destroyed_(false) {
+ external_task_->process_killer_ = &process_killer_;
+ }
+
+ virtual ~ExternalTaskTest() {}
+
+ virtual void TearDown() {
+ if (!external_task_) {
+ return;
+ }
+
+ if (external_task_->child_watch_tag_) {
+ EXPECT_CALL(glib_, SourceRemove(external_task_->child_watch_tag_));
+ }
+
+ if (external_task_->pid_) {
+ EXPECT_CALL(process_killer_, Kill(external_task_->pid_, _));
+ }
+ }
+
+ void set_test_rpc_task_destroyed(bool destroyed) {
+ test_rpc_task_destroyed_ = destroyed;
+ }
+
+ protected:
+ // Implements RPCTaskDelegate interface.
+ MOCK_METHOD2(GetLogin, void(string *user, string *password));
+ MOCK_METHOD2(Notify, void(const string &reason,
+ const map<string, string> &dict));
+
+ MOCK_METHOD2(TaskDiedCallback, void(pid_t dead_process, int exit_status));
+
+ NiceMockControl control_;
+ EventDispatcher dispatcher_;
+ MockGLib glib_;
+ MockProcessKiller process_killer_;
+ base::WeakPtrFactory<ExternalTaskTest> weak_ptr_factory_;
+ base::Callback<void(pid_t, int)> death_callback_;
+ scoped_ptr<ExternalTask> external_task_;
+ bool test_rpc_task_destroyed_;
+};
+
+namespace {
+
+class TestRPCTask : public RPCTask {
+ public:
+ TestRPCTask(ControlInterface *control, ExternalTaskTest *test);
+ virtual ~TestRPCTask();
+
+ private:
+ ExternalTaskTest *test_;
+};
+
+TestRPCTask::TestRPCTask(ControlInterface *control, ExternalTaskTest *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
+
+TEST_F(ExternalTaskTest, Destructor) {
+ const unsigned int kTag = 123;
+ external_task_->child_watch_tag_ = kTag;
+ EXPECT_CALL(glib_, SourceRemove(kTag));
+ const int kPID = 123456;
+ external_task_->pid_ = kPID;
+ EXPECT_CALL(process_killer_, Kill(kPID, _));
+ external_task_->rpc_task_.reset(new TestRPCTask(&control_, this));
+ external_task_.reset();
+ EXPECT_TRUE(test_rpc_task_destroyed_);
+}
+
+namespace {
+
+// Returns true iff. there is at least one anchored match in |arg|,
+// for each item in |expected_values|. Order of items does not matter.
+//
+// |arg| is a NULL-terminated array of C-strings.
+// |expected_values| is a container of regular expressions (as strings).
+MATCHER_P(HasElementsMatching, expected_values, "") {
+ for (const auto &expected_value : expected_values) {
+ auto regex_matcher(MatchesRegex(expected_value).impl());
+ char **arg_local = arg;
+ while (*arg_local) {
+ if (regex_matcher.MatchAndExplain(*arg_local, result_listener)) {
+ break;
+ }
+ ++arg_local;
+ }
+ if (*arg_local == NULL) {
+ *result_listener << "missing value " << expected_value << "\n";
+ arg_local = arg;
+ while (*arg_local) {
+ *result_listener << "received: " << *arg_local << "\n";
+ ++arg_local;
+ }
+ return false;
+ }
+ }
+ return true;
+}
+
+} // namespace
+
+TEST_F(ExternalTaskTest, Start) {
+ const string kCommand = "/run/me";
+ const vector<string> kCommandOptions{"arg1", "arg2"};
+ const map<string, string> kCommandEnv{{"env1", "val1"}, {"env2", "val2"}};
+ const vector<string> kExpectedEnv{"SHILL_TASK_SERVICE=.+",
+ "SHILL_TASK_PATH=.+", "env1=val1",
+ "env2=val2"};
+ const int kPID = 234678;
+ EXPECT_CALL(glib_, SpawnAsync(_,
+ HasElementsMatching(kCommandOptions),
+ HasElementsMatching(kExpectedEnv),
+ _, _, _, _, _))
+ .WillOnce(Return(false))
+ .WillOnce(DoAll(SetArgumentPointee<6>(kPID), Return(true)));
+ const int kTag = 6;
+ Error error;
+ EXPECT_CALL(glib_,
+ ChildWatchAdd(
+ kPID, &external_task_->OnTaskDied, external_task_.get()))
+ .WillOnce(Return(kTag));
+ EXPECT_FALSE(
+ external_task_->Start(
+ base::FilePath(kCommand), kCommandOptions, kCommandEnv, &error));
+ EXPECT_EQ(Error::kInternalError, error.type());
+ EXPECT_FALSE(external_task_->rpc_task_);
+
+ error.Reset();
+ EXPECT_TRUE(
+ external_task_->Start(
+ base::FilePath(kCommand), kCommandOptions, kCommandEnv, &error));
+ EXPECT_TRUE(error.IsSuccess());
+ EXPECT_EQ(kPID, external_task_->pid_);
+ EXPECT_EQ(kTag, external_task_->child_watch_tag_);
+ EXPECT_TRUE(external_task_->rpc_task_);
+}
+
+TEST_F(ExternalTaskTest, Stop) {
+ const unsigned int kTag = 123;
+ external_task_->child_watch_tag_ = kTag;
+ EXPECT_CALL(glib_, SourceRemove(kTag));
+ const int kPID = 123456;
+ external_task_->pid_ = kPID;
+ EXPECT_CALL(process_killer_, Kill(kPID, _));
+ external_task_->rpc_task_.reset(new TestRPCTask(&control_, this));
+ external_task_->Stop();
+
+ EXPECT_EQ(0, external_task_->child_watch_tag_);
+ EXPECT_EQ(0, external_task_->pid_);
+ EXPECT_FALSE(external_task_->rpc_task_);
+ EXPECT_TRUE(test_rpc_task_destroyed_);
+}
+
+TEST_F(ExternalTaskTest, StopNotStarted) {
+ EXPECT_CALL(glib_, SourceRemove(_)).Times(0);
+ EXPECT_CALL(process_killer_, Kill(_, _)).Times(0);
+ external_task_->Stop();
+ EXPECT_FALSE(test_rpc_task_destroyed_);
+}
+
+TEST_F(ExternalTaskTest, GetLogin) {
+ string username;
+ string password;
+ EXPECT_CALL(*this, GetLogin(&username, &password));
+ EXPECT_CALL(*this, Notify(_, _)).Times(0);
+ external_task_->GetLogin(&username, &password);
+}
+
+TEST_F(ExternalTaskTest, Notify) {
+ const string kReason("you may already have won!");
+ const map<string, string> &kArgs{
+ {"arg1", "val1"},
+ {"arg2", "val2"}};
+ EXPECT_CALL(*this, GetLogin(_, _)).Times(0);
+ EXPECT_CALL(*this, Notify(kReason, kArgs));
+ external_task_->Notify(kReason, kArgs);
+}
+
+TEST_F(ExternalTaskTest, OnTaskDied) {
+ const int kPID = 99999;
+ const int kExitStatus = 1;
+ external_task_->child_watch_tag_ = 333;
+ external_task_->pid_ = kPID;
+ EXPECT_CALL(process_killer_, Kill(_, _)).Times(0);
+ EXPECT_CALL(*this, TaskDiedCallback(kPID, kExitStatus));
+ ExternalTask::OnTaskDied(kPID, kExitStatus, external_task_.get());
+ EXPECT_EQ(0, external_task_->child_watch_tag_);
+ EXPECT_EQ(0, external_task_->pid_);
+}
+
+} // namespace shill
diff --git a/l2tp_ipsec_driver.cc b/l2tp_ipsec_driver.cc
index a4069e6..f239b9f 100644
--- a/l2tp_ipsec_driver.cc
+++ b/l2tp_ipsec_driver.cc
@@ -23,8 +23,6 @@
#include "shill/l2tp_ipsec_driver.h"
-#include <sys/wait.h>
-
#include <base/bind.h>
#include <base/file_util.h>
#include <base/string_util.h>
@@ -34,10 +32,10 @@
#include "shill/certificate_file.h"
#include "shill/device_info.h"
#include "shill/error.h"
+#include "shill/external_task.h"
#include "shill/logging.h"
#include "shill/manager.h"
#include "shill/nss.h"
-#include "shill/process_killer.h"
#include "shill/vpn.h"
#include "shill/vpn_service.h"
@@ -105,10 +103,8 @@
device_info_(device_info),
glib_(glib),
nss_(NSS::GetInstance()),
- process_killer_(ProcessKiller::GetInstance()),
certificate_file_(new CertificateFile(glib)),
- pid_(0),
- child_watch_tag_(0) {}
+ weak_ptr_factory_(this) {}
L2TPIPSecDriver::~L2TPIPSecDriver() {
IdleService();
@@ -125,7 +121,6 @@
StartConnectTimeout(kDefaultConnectTimeoutSeconds);
service_ = service;
service_->SetState(Service::kStateConfiguring);
- rpc_task_.reset(new RPCTask(control_, this));
if (!SpawnL2TPIPSecVPN(error)) {
FailService(Service::kFailureInternal);
}
@@ -165,20 +160,12 @@
<< Service::ConnectFailureToString(failure) << ")";
StopConnectTimeout();
DeletePSKFile();
- if (child_watch_tag_) {
- glib_->SourceRemove(child_watch_tag_);
- child_watch_tag_ = 0;
- }
- if (pid_) {
- process_killer_->Kill(pid_, Closure());
- pid_ = 0;
- }
+ external_task_.reset();
if (device_) {
device_->OnDisconnected();
device_->SetEnabled(false);
device_ = NULL;
}
- rpc_task_.reset();
if (service_) {
if (state == Service::kStateFailure) {
service_->SetFailure(failure);
@@ -198,56 +185,25 @@
bool L2TPIPSecDriver::SpawnL2TPIPSecVPN(Error *error) {
SLOG(VPN, 2) << __func__;
+ scoped_ptr<ExternalTask> external_task_local(
+ new ExternalTask(control_, glib_,
+ weak_ptr_factory_.GetWeakPtr(),
+ Bind(&L2TPIPSecDriver::OnL2TPIPSecVPNDied,
+ weak_ptr_factory_.GetWeakPtr())));
vector<string> options;
+ map<string, string> environment; // No env vars passed.
if (!InitOptions(&options, error)) {
return false;
}
LOG(INFO) << "L2TP/IPSec VPN process options: " << JoinString(options, ' ');
- // TODO(petkov): This code needs to be abstracted away in a separate external
- // process module (crosbug.com/27131).
- vector<char *> process_args;
- process_args.push_back(const_cast<char *>(kL2TPIPSecVPNPath));
- for (vector<string>::const_iterator it = options.begin();
- it != options.end(); ++it) {
- process_args.push_back(const_cast<char *>(it->c_str()));
+ if (external_task_local->Start(
+ FilePath(kL2TPIPSecVPNPath), options, environment, error)) {
+ external_task_.reset(external_task_local.release());
+ return true;
}
- process_args.push_back(NULL);
-
- vector<string> environment;
- InitEnvironment(&environment);
-
- vector<char *> process_env;
- for (vector<string>::const_iterator it = environment.begin();
- it != environment.end(); ++it) {
- process_env.push_back(const_cast<char *>(it->c_str()));
- }
- process_env.push_back(NULL);
-
- CHECK(!pid_);
- if (!glib_->SpawnAsync(NULL,
- process_args.data(),
- process_env.data(),
- G_SPAWN_DO_NOT_REAP_CHILD,
- NULL,
- NULL,
- &pid_,
- NULL)) {
- Error::PopulateAndLog(error, Error::kInternalError,
- string("Unable to spawn: ") + process_args[0]);
- return false;
- }
- CHECK(!child_watch_tag_);
- child_watch_tag_ = glib_->ChildWatchAdd(pid_, OnL2TPIPSecVPNDied, this);
- return true;
-}
-
-void L2TPIPSecDriver::InitEnvironment(vector<string> *environment) {
- environment->push_back(string(kRPCTaskServiceVariable) + "=" +
- rpc_task_->GetRpcConnectionIdentifier());
- environment->push_back(string(kRPCTaskPathVariable) + "=" +
- rpc_task_->GetRpcIdentifier());
+ return false;
}
bool L2TPIPSecDriver::InitOptions(vector<string> *options, Error *error) {
@@ -375,14 +331,8 @@
return false;
}
-// static
-void L2TPIPSecDriver::OnL2TPIPSecVPNDied(GPid pid, gint status, gpointer data) {
- LOG(INFO) << __func__ << "(" << pid << ", " << status << ")";
- L2TPIPSecDriver *me = reinterpret_cast<L2TPIPSecDriver *>(data);
- me->child_watch_tag_ = 0;
- CHECK_EQ(pid, me->pid_);
- me->pid_ = 0;
- me->FailService(TranslateExitStatusToFailure(status));
+void L2TPIPSecDriver::OnL2TPIPSecVPNDied(pid_t /*pid*/, int status) {
+ FailService(TranslateExitStatusToFailure(status));
// TODO(petkov): Figure if we need to restart the connection.
}
@@ -472,9 +422,9 @@
if (reason != kL2TPIPSecReasonConnect) {
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()));
+ // Avoid destroying the ExternalTask that called us (and is still on
+ // the stack), by deferring deletion to the main event loop.
+ dispatcher()->PostTask(Bind(&DeleteExternalTask, external_task_.release()));
FailService(Service::kFailureUnknown);
return;
}
@@ -507,8 +457,8 @@
}
// static
-void L2TPIPSecDriver::DeleteRPCTask(RPCTask *rpc_task) {
- delete rpc_task;
+void L2TPIPSecDriver::DeleteExternalTask(ExternalTask *external_task) {
+ delete external_task;
}
KeyValueStore L2TPIPSecDriver::GetProvider(Error *error) {
diff --git a/l2tp_ipsec_driver.h b/l2tp_ipsec_driver.h
index ffbe05c..342e5c1 100644
--- a/l2tp_ipsec_driver.h
+++ b/l2tp_ipsec_driver.h
@@ -5,13 +5,14 @@
#ifndef SHILL_L2TP_IPSEC_DRIVER_H_
#define SHILL_L2TP_IPSEC_DRIVER_H_
+#include <sys/types.h>
+
#include <vector>
#include <base/file_path.h>
#include <base/memory/scoped_ptr.h>
#include <gtest/gtest_prod.h> // for FRIEND_TEST
-#include "shill/glib.h"
#include "shill/ipconfig.h"
#include "shill/rpc_task.h"
#include "shill/service.h"
@@ -33,10 +34,10 @@
class CertificateFile;
class ControlInterface;
class DeviceInfo;
+class ExternalTask;
class GLib;
class Metrics;
class NSS;
-class ProcessKiller;
class L2TPIPSecDriver : public VPNDriver,
public RPCTaskDelegate {
@@ -87,8 +88,6 @@
bool SpawnL2TPIPSecVPN(Error *error);
- void InitEnvironment(std::vector<std::string> *environment);
-
bool InitOptions(std::vector<std::string> *options, Error *error);
bool InitPSKOptions(std::vector<std::string> *options, Error *error);
void InitNSSOptions(std::vector<std::string> *options);
@@ -128,9 +127,6 @@
IPConfig::Properties *properties,
std::string *interface_name);
- // Called when the l2tpipsec_vpn process exits.
- static void OnL2TPIPSecVPNDied(GPid pid, gint status, gpointer data);
-
static Service::ConnectFailure TranslateExitStatusToFailure(int status);
// Inherit from VPNDriver to add custom properties.
@@ -140,8 +136,10 @@
virtual void GetLogin(std::string *user, std::string *password);
virtual void Notify(const std::string &reason,
const std::map<std::string, std::string> &dict);
+ // Called when the l2tpipsec_vpn process exits.
+ void OnL2TPIPSecVPNDied(pid_t pid, int status);
- static void DeleteRPCTask(RPCTask *rpc_task);
+ static void DeleteExternalTask(ExternalTask *external_task);
void ReportConnectionMetrics();
@@ -150,20 +148,13 @@
DeviceInfo *device_info_;
GLib *glib_;
NSS *nss_;
- ProcessKiller *process_killer_;
VPNServiceRefPtr service_;
- scoped_ptr<RPCTask> rpc_task_;
+ scoped_ptr<ExternalTask> external_task_;
base::FilePath psk_file_;
scoped_ptr<CertificateFile> certificate_file_;
VPNRefPtr device_;
-
- // The PID of the spawned l2tpipsec_vpn process. May be 0 if no process has
- // been spawned yet or the process has died.
- int pid_;
-
- // Child exit watch callback source tag.
- unsigned int child_watch_tag_;
+ base::WeakPtrFactory<L2TPIPSecDriver> weak_ptr_factory_;
DISALLOW_COPY_AND_ASSIGN(L2TPIPSecDriver);
};
diff --git a/l2tp_ipsec_driver_unittest.cc b/l2tp_ipsec_driver_unittest.cc
index 1cd3641..ac33f5d 100644
--- a/l2tp_ipsec_driver_unittest.cc
+++ b/l2tp_ipsec_driver_unittest.cc
@@ -6,6 +6,7 @@
#include <base/file_util.h>
#include <base/files/scoped_temp_dir.h>
+#include <base/memory/weak_ptr.h>
#include <base/string_util.h>
#include <chromeos/vpn-manager/service_error.h>
#include <gtest/gtest.h>
@@ -15,11 +16,11 @@
#include "shill/mock_adaptors.h"
#include "shill/mock_certificate_file.h"
#include "shill/mock_device_info.h"
+#include "shill/mock_external_task.h"
#include "shill/mock_glib.h"
#include "shill/mock_manager.h"
#include "shill/mock_metrics.h"
#include "shill/mock_nss.h"
-#include "shill/mock_process_killer.h"
#include "shill/mock_vpn.h"
#include "shill/mock_vpn_service.h"
#include "shill/vpn.h"
@@ -31,6 +32,7 @@
using std::vector;
using testing::_;
using testing::ElementsAreArray;
+using testing::Mock;
using testing::NiceMock;
using testing::Return;
using testing::ReturnRef;
@@ -53,10 +55,9 @@
device_(new MockVPN(&control_, &dispatcher_, &metrics_, &manager_,
kInterfaceName, kInterfaceIndex)),
certificate_file_(new MockCertificateFile()),
- test_rpc_task_destroyed_(false) {
+ weak_ptr_factory_(this) {
driver_->nss_ = &nss_;
driver_->certificate_file_.reset(certificate_file_); // Passes ownership.
- driver_->process_killer_ = &process_killer_;
}
virtual ~L2TPIPSecDriverTest() {}
@@ -66,17 +67,11 @@
}
virtual void TearDown() {
- driver_->child_watch_tag_ = 0;
- driver_->pid_ = 0;
driver_->device_ = NULL;
driver_->service_ = NULL;
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;
@@ -145,34 +140,9 @@
scoped_refptr<MockVPN> device_;
MockNSS nss_;
MockCertificateFile *certificate_file_; // Owned by |driver_|.
- MockProcessKiller process_killer_;
- bool test_rpc_task_destroyed_;
+ base::WeakPtrFactory<L2TPIPSecDriverTest> weak_ptr_factory_;
};
-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;
@@ -186,13 +156,10 @@
vector<string>::const_iterator it =
find(options.begin(), options.end(), flag);
- EXPECT_TRUE(it != options.end());
- if (it != options.end())
- return; // Don't crash below.
+ ASSERT_TRUE(it != options.end()); // Don't crash below.
+ EXPECT_EQ(flag, *it);
it++;
- EXPECT_TRUE(it != options.end());
- if (it != options.end())
- return; // Don't crash below.
+ ASSERT_TRUE(it != options.end()); // Don't crash below.
EXPECT_EQ(value, *it);
}
@@ -212,29 +179,25 @@
TEST_F(L2TPIPSecDriverTest, Cleanup) {
driver_->IdleService(); // Ensure no crash.
- const unsigned int kTag = 123;
- driver_->child_watch_tag_ = kTag;
- EXPECT_CALL(glib_, SourceRemove(kTag));
- const int kPID = 123456;
- driver_->pid_ = kPID;
- EXPECT_CALL(process_killer_, Kill(kPID, _));
driver_->device_ = device_;
driver_->service_ = service_;
+ driver_->external_task_.reset(
+ new MockExternalTask(&control_,
+ &glib_,
+ weak_ptr_factory_.GetWeakPtr(),
+ base::Callback<void(pid_t, int)>()));
EXPECT_CALL(*device_, OnDisconnected());
EXPECT_CALL(*device_, SetEnabled(false));
EXPECT_CALL(*service_, SetFailure(Service::kFailureBadPassphrase));
- driver_->rpc_task_.reset(new RPCTask(&control_, this));
FilePath psk_file = SetupPSKFile();
StartConnectTimeout(0);
- driver_->FailService(Service::kFailureBadPassphrase);
+ driver_->FailService(Service::kFailureBadPassphrase); // Trigger Cleanup.
EXPECT_FALSE(file_util::PathExists(psk_file));
EXPECT_TRUE(driver_->psk_file_.empty());
- EXPECT_EQ(0, driver_->child_watch_tag_);
- EXPECT_EQ(0, driver_->pid_);
- EXPECT_FALSE(driver_->rpc_task_.get());
EXPECT_FALSE(driver_->device_);
EXPECT_FALSE(driver_->service_);
EXPECT_FALSE(driver_->IsConnectTimeoutStarted());
+ EXPECT_FALSE(driver_->external_task_);
driver_->service_ = service_;
EXPECT_CALL(*service_, SetState(Service::kStateIdle));
@@ -249,18 +212,6 @@
EXPECT_TRUE(driver_->psk_file_.empty());
}
-TEST_F(L2TPIPSecDriverTest, InitEnvironment) {
- vector<string> env;
- driver_->rpc_task_.reset(new RPCTask(&control_, this));
- driver_->InitEnvironment(&env);
- ASSERT_EQ(2, env.size());
- EXPECT_EQ(
- string(kRPCTaskServiceVariable) + "=" + RPCTaskMockAdaptor::kRpcConnId,
- env[0]);
- EXPECT_EQ(string(kRPCTaskPathVariable) + "=" + RPCTaskMockAdaptor::kRpcId,
- env[1]);
-}
-
TEST_F(L2TPIPSecDriverTest, InitOptionsNoHost) {
Error error;
vector<string> options;
@@ -444,60 +395,49 @@
}
TEST_F(L2TPIPSecDriverTest, OnL2TPIPSecVPNDied) {
- const int kPID = 99999;
- driver_->child_watch_tag_ = 333;
- driver_->pid_ = kPID;
+ const int kPID = 123456;
driver_->service_ = service_;
EXPECT_CALL(*service_, SetFailure(Service::kFailureDNSLookup));
- EXPECT_CALL(process_killer_, Kill(_, _)).Times(0);
- L2TPIPSecDriver::OnL2TPIPSecVPNDied(
- kPID, vpn_manager::kServiceErrorResolveHostnameFailed << 8, driver_);
- EXPECT_EQ(0, driver_->child_watch_tag_);
- EXPECT_EQ(0, driver_->pid_);
+ driver_->OnL2TPIPSecVPNDied(
+ kPID, vpn_manager::kServiceErrorResolveHostnameFailed << 8);
EXPECT_FALSE(driver_->service_);
}
-namespace {
-MATCHER(CheckEnv, "") {
- if (!arg || !arg[0] || !arg[1] || arg[2]) {
- return false;
- }
- return StartsWithASCII(arg[0], "SHILL_TASK_", true);
-}
-} // namespace
-
TEST_F(L2TPIPSecDriverTest, SpawnL2TPIPSecVPN) {
Error error;
+ // Fail without sufficient arguments.
EXPECT_FALSE(driver_->SpawnL2TPIPSecVPN(&error));
EXPECT_TRUE(error.IsFailure());
+ // Provide the required arguments.
static const char kHost[] = "192.168.2.254";
SetArg(flimflam::kProviderHostProperty, kHost);
- driver_->rpc_task_.reset(new RPCTask(&control_, this));
- const int kPID = 234678;
- EXPECT_CALL(glib_, SpawnAsync(_, _, CheckEnv(), _, _, _, _, _))
- .WillOnce(Return(false))
- .WillOnce(DoAll(SetArgumentPointee<6>(kPID), Return(true)));
- const int kTag = 6;
- EXPECT_CALL(glib_, ChildWatchAdd(kPID, &driver_->OnL2TPIPSecVPNDied, driver_))
- .WillOnce(Return(kTag));
- error.Reset();
+ // TODO(quiche): Instead of setting expectations based on what
+ // ExternalTask will call, mock out ExternalTask. Non-trivial,
+ // though, because ExternalTask is constructed during the
+ // call to driver_->Connect.
+ EXPECT_CALL(glib_, SpawnAsync(_, _, _, _, _, _, _, _)).
+ WillOnce(Return(false)).
+ WillOnce(Return(true));
+ EXPECT_CALL(glib_, ChildWatchAdd(_, _, _));
+
EXPECT_FALSE(driver_->SpawnL2TPIPSecVPN(&error));
- EXPECT_EQ(Error::kInternalError, error.type());
- error.Reset();
EXPECT_TRUE(driver_->SpawnL2TPIPSecVPN(&error));
- EXPECT_TRUE(error.IsSuccess());
- EXPECT_EQ(kPID, driver_->pid_);
- EXPECT_EQ(kTag, driver_->child_watch_tag_);
}
TEST_F(L2TPIPSecDriverTest, Connect) {
EXPECT_CALL(*service_, SetState(Service::kStateConfiguring));
static const char kHost[] = "192.168.2.254";
SetArg(flimflam::kProviderHostProperty, kHost);
+
+ // TODO(quiche): Instead of setting expectations based on what
+ // ExternalTask will call, mock out ExternalTask. Non-trivial,
+ // though, because ExternalTask is constructed during the
+ // call to driver_->Connect.
EXPECT_CALL(glib_, SpawnAsync(_, _, _, _, _, _, _, _)).WillOnce(Return(true));
EXPECT_CALL(glib_, ChildWatchAdd(_, _, _)).WillOnce(Return(1));
+
Error error;
driver_->Connect(service_, &error);
EXPECT_TRUE(error.IsSuccess());
@@ -644,18 +584,24 @@
TEST_F(L2TPIPSecDriverTest, NotifyDisconnected) {
map<string, string> dict;
+ base::Callback<void(pid_t, int)> death_callback;
+ MockExternalTask *local_external_task =
+ new MockExternalTask(&control_, &glib_, weak_ptr_factory_.GetWeakPtr(),
+ death_callback);
driver_->device_ = device_;
- driver_->rpc_task_.reset(new TestRPCTask(&control_, this));
- EXPECT_FALSE(test_rpc_task_destroyed_);
+ driver_->external_task_.reset(local_external_task); // passes ownership
EXPECT_CALL(*device_, OnDisconnected());
EXPECT_CALL(*device_, SetEnabled(false));
+ EXPECT_CALL(*local_external_task, OnDelete())
+ .Times(0); // Not until event loop.
driver_->Notify(kL2TPIPSecReasonDisconnect, dict);
EXPECT_FALSE(driver_->device_);
- EXPECT_FALSE(test_rpc_task_destroyed_);
- EXPECT_FALSE(driver_->rpc_task_.get());
+ EXPECT_FALSE(driver_->external_task_.get());
+ Mock::VerifyAndClearExpectations(local_external_task);
+
+ EXPECT_CALL(*local_external_task, OnDelete());
dispatcher_.PostTask(MessageLoop::QuitClosure());
dispatcher_.DispatchForever();
- EXPECT_TRUE(test_rpc_task_destroyed_);
}
TEST_F(L2TPIPSecDriverTest, VerifyPaths) {
diff --git a/mock_external_task.cc b/mock_external_task.cc
new file mode 100644
index 0000000..c0db032
--- /dev/null
+++ b/mock_external_task.cc
@@ -0,0 +1,20 @@
+// Copyright (c) 2013 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/mock_external_task.h"
+
+namespace shill {
+
+MockExternalTask::MockExternalTask(
+ ControlInterface *control,
+ GLib *glib,
+ const base::WeakPtr<RPCTaskDelegate> &task_delegate,
+ const base::Callback<void(pid_t, int)> &death_callback)
+ : ExternalTask(control, glib, task_delegate, death_callback) {}
+
+MockExternalTask::~MockExternalTask() {
+ OnDelete();
+}
+
+} // namespace shill
diff --git a/mock_external_task.h b/mock_external_task.h
new file mode 100644
index 0000000..6469c83
--- /dev/null
+++ b/mock_external_task.h
@@ -0,0 +1,36 @@
+// Copyright (c) 2013 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_EXTERNAL_TASK_H_
+#define SHILL_MOCK_EXTERNAL_TASK_H_
+
+#include <gmock/gmock.h>
+
+#include "shill/external_task.h"
+
+namespace shill {
+
+class MockExternalTask : public ExternalTask {
+public:
+ MockExternalTask(ControlInterface *control,
+ GLib *glib,
+ const base::WeakPtr<RPCTaskDelegate> &task_delegate,
+ const base::Callback<void(pid_t, int)> &death_callback);
+ virtual ~MockExternalTask();
+
+ MOCK_METHOD4(Start,
+ bool(const base::FilePath &file,
+ const std::vector<std::string> &arguments,
+ const std::map<std::string, std::string> &environment,
+ Error *error));
+ MOCK_METHOD0(Stop, void());
+ MOCK_METHOD0(OnDelete, void());
+
+private:
+ DISALLOW_COPY_AND_ASSIGN(MockExternalTask);
+};
+
+} // namespace shill
+
+#endif // SHILL_MOCK_EXTERNAL_TASK_H_
diff --git a/openvpn_driver.cc b/openvpn_driver.cc
index bdb186f..adfc259 100644
--- a/openvpn_driver.cc
+++ b/openvpn_driver.cc
@@ -236,8 +236,8 @@
}
LOG(INFO) << "OpenVPN process options: " << JoinString(options, ' ');
- // TODO(petkov): This code needs to be abstracted away in a separate external
- // process module (crosbug.com/27131).
+ // TODO(quiche): This should be migrated to use ExternalTask.
+ // (crbug.com/246263).
vector<char *> process_args;
process_args.push_back(const_cast<char *>(kOpenVPNPath));
for (vector<string>::const_iterator it = options.begin();
diff --git a/rpc_task.cc b/rpc_task.cc
index 906bc3c..25eb081 100644
--- a/rpc_task.cc
+++ b/rpc_task.cc
@@ -12,6 +12,7 @@
using std::map;
using std::string;
+using std::vector;
namespace shill {
@@ -30,7 +31,7 @@
LOG(INFO) << "RPCTask " + unique_name_ + " destroyed.";
}
-void RPCTask::GetLogin(string *user, string *password) {
+void RPCTask::GetLogin(string *user, string *password) const {
delegate_->GetLogin(user, password);
}
@@ -38,14 +39,20 @@
delegate_->Notify(reason, dict);
}
+vector<string> RPCTask::GetEnvironment() const {
+ return vector<string>{
+ string(kRPCTaskServiceVariable) + "=" +
+ adaptor_->GetRpcConnectionIdentifier(),
+ string(kRPCTaskPathVariable) + "=" +
+ adaptor_->GetRpcIdentifier()};
+}
+
+// TODO(quiche): remove after moving OpenVPNDriver over to ExternalTask.
string RPCTask::GetRpcIdentifier() const {
return adaptor_->GetRpcIdentifier();
}
-string RPCTask::GetRpcInterfaceIdentifier() const {
- return adaptor_->GetRpcInterfaceIdentifier();
-}
-
+// TODO(quiche): remove after moving OpenVPNDriver over to ExternalTask.
string RPCTask::GetRpcConnectionIdentifier() const {
return adaptor_->GetRpcConnectionIdentifier();
}
diff --git a/rpc_task.h b/rpc_task.h
index 5d8a264..d1fd251 100644
--- a/rpc_task.h
+++ b/rpc_task.h
@@ -7,6 +7,7 @@
#include <map>
#include <string>
+#include <vector>
#include <base/basictypes.h>
#include <base/memory/scoped_ptr.h>
@@ -40,7 +41,7 @@
RPCTask(ControlInterface *control_interface, RPCTaskDelegate *delegate);
virtual ~RPCTask();
- virtual void GetLogin(std::string *user, std::string *password);
+ virtual void GetLogin(std::string *user, std::string *password) const;
virtual void Notify(const std::string &reason,
const std::map<std::string, std::string> &dict);
@@ -48,8 +49,10 @@
// instance.
const std::string &UniqueName() const { return unique_name_; }
+ // Generates environment variable strings for a child process to
+ // communicate back to us over RPC.
+ virtual std::vector<std::string> GetEnvironment() const;
std::string GetRpcIdentifier() const;
- std::string GetRpcInterfaceIdentifier() const;
std::string GetRpcConnectionIdentifier() const;
private:
diff --git a/rpc_task_unittest.cc b/rpc_task_unittest.cc
index a1b8f31..f7a435b 100644
--- a/rpc_task_unittest.cc
+++ b/rpc_task_unittest.cc
@@ -11,6 +11,7 @@
using std::map;
using std::string;
+using std::vector;
namespace shill {
@@ -51,10 +52,18 @@
last_notify_dict_ = dict;
}
+TEST_F(RPCTaskTest, GetEnvironment) {
+ vector<string> env = task_.GetEnvironment();
+ ASSERT_EQ(2, env.size());
+ EXPECT_EQ(
+ string(kRPCTaskServiceVariable) + "=" + RPCTaskMockAdaptor::kRpcConnId,
+ env[0]);
+ EXPECT_EQ(string(kRPCTaskPathVariable) + "=" + RPCTaskMockAdaptor::kRpcId,
+ env[1]);
+}
+
TEST_F(RPCTaskTest, GetRpcIdentifiers) {
EXPECT_EQ(RPCTaskMockAdaptor::kRpcId, task_.GetRpcIdentifier());
- EXPECT_EQ(RPCTaskMockAdaptor::kRpcInterfaceId,
- task_.GetRpcInterfaceIdentifier());
EXPECT_EQ(RPCTaskMockAdaptor::kRpcConnId, task_.GetRpcConnectionIdentifier());
}