shill: provide ability to configure children to terminate with parent
If shill crashes and restarts, it will re-initialize its state about
its peer daemons (e.g. wpasupplicant, ModemManager). Child processes,
however, do not always provide a way to query their current state.
For dhcpcd, we deal with this problem by having the child process
(dhcpcd) monitor D-Bus events to determine when shill exits. When
that occurs, dhcpcd terminates itself. In this way, a newly spawned
shill process doesn't have to worry about an old dhcpcd conflicting
with one fired off by the (new) shill process.
Since pppd doesn't have integrated D-Bus support, doing the same
for pppd could be hairy. Instead, use prctl() to ask the kernel
to deliver SIGTERM to pppd when shill exits.
BUG=chromium:261710
TEST=manual
Manual test
-----------
- plug in ppp dongle
- wait for network to connect
- killall -9 shill
- egrep 'pppd.+Terminating on signal 15' /var/log/net.log
Change-Id: I7513ccfa8de0b154d233d8a4d6f1bc701af32974
Reviewed-on: https://gerrit.chromium.org/gerrit/66031
Commit-Queue: mukesh agrawal <quiche@chromium.org>
Reviewed-by: mukesh agrawal <quiche@chromium.org>
Tested-by: mukesh agrawal <quiche@chromium.org>
diff --git a/cellular.cc b/cellular.cc
index e5560a0..67a9d2e 100644
--- a/cellular.cc
+++ b/cellular.cc
@@ -859,7 +859,7 @@
weak_ptr_factory_.GetWeakPtr(),
death_callback));
if (new_ppp_task->Start(
- FilePath(PPPDevice::kDaemonPath), args, environment, &error)) {
+ FilePath(PPPDevice::kDaemonPath), args, environment, true, &error)) {
LOG(INFO) << "Forked pppd process.";
ppp_task_ = new_ppp_task.Pass();
}
diff --git a/external_task.cc b/external_task.cc
index c425a8d..4ea88d4 100644
--- a/external_task.cc
+++ b/external_task.cc
@@ -4,6 +4,9 @@
#include "shill/external_task.h"
+#include <signal.h>
+#include <sys/prctl.h>
+
#include <base/bind.h>
#include <base/bind_helpers.h>
@@ -45,6 +48,7 @@
bool ExternalTask::Start(const FilePath &program,
const vector<string> &arguments,
const map<string, string> &environment,
+ bool terminate_with_parent,
Error *error) {
CHECK(!pid_);
CHECK(!child_watch_tag_);
@@ -74,11 +78,14 @@
}
process_env.push_back(NULL);
+ GSpawnChildSetupFunc child_setup_func =
+ terminate_with_parent ? SetupTermination : NULL;
+
if (!glib_->SpawnAsync(NULL,
process_args.data(),
process_env.data(),
G_SPAWN_DO_NOT_REAP_CHILD,
- NULL,
+ child_setup_func,
NULL,
&pid_,
NULL)) {
@@ -127,4 +134,9 @@
delete task;
}
+// static
+void ExternalTask::SetupTermination(gpointer /*glib_user_data*/) {
+ prctl(PR_SET_PDEATHSIG, SIGTERM);
+}
+
} // namespace shill
diff --git a/external_task.h b/external_task.h
index 28b59c0..2e871db 100644
--- a/external_task.h
+++ b/external_task.h
@@ -58,6 +58,10 @@
// arguments |arguments|, and the environment variables specified in
// |environment|.
//
+ // If |terminate_with_parent| is true, the child process will be
+ // configured to terminate itself if this process dies. Otherwise,
+ // the child process will retain its default behavior.
+ //
// On success, returns true, and leaves |error| unmodified.
// On failure, returns false, and sets |error|.
//
@@ -67,6 +71,7 @@
virtual bool Start(const base::FilePath &program,
const std::vector<std::string> &arguments,
const std::map<std::string, std::string> &environment,
+ bool terminate_with_parent,
Error *error);
virtual void Stop();
@@ -90,6 +95,11 @@
static void Destroy(ExternalTask *task);
+ // This method is run in the child process (i.e. after fork(), but
+ // before exec()). It configures the child to receive a SIGTERM when
+ // the parent exits.
+ static void SetupTermination(gpointer glib_user_data);
+
ControlInterface *control_;
GLib *glib_;
ProcessKiller *process_killer_; // Field permits mocking.
diff --git a/external_task_unittest.cc b/external_task_unittest.cc
index b5663d8..4994cdd 100644
--- a/external_task_unittest.cc
+++ b/external_task_unittest.cc
@@ -211,16 +211,14 @@
ChildWatchAdd(
kPID, &external_task_->OnTaskDied, external_task_.get()))
.WillOnce(Return(kTag));
- EXPECT_FALSE(
- external_task_->Start(
- base::FilePath(kCommand), kCommandOptions, kCommandEnv, &error));
+ EXPECT_FALSE(external_task_->Start(
+ base::FilePath(kCommand), kCommandOptions, kCommandEnv, false, &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(external_task_->Start(
+ base::FilePath(kCommand), kCommandOptions, kCommandEnv, false, &error));
EXPECT_TRUE(error.IsSuccess());
EXPECT_EQ(kPID, external_task_->pid_);
EXPECT_EQ(kTag, external_task_->child_watch_tag_);
diff --git a/l2tp_ipsec_driver.cc b/l2tp_ipsec_driver.cc
index 699f007..397ed2c 100644
--- a/l2tp_ipsec_driver.cc
+++ b/l2tp_ipsec_driver.cc
@@ -199,7 +199,7 @@
LOG(INFO) << "L2TP/IPSec VPN process options: " << JoinString(options, ' ');
if (external_task_local->Start(
- FilePath(kL2TPIPSecVPNPath), options, environment, error)) {
+ FilePath(kL2TPIPSecVPNPath), options, environment, true, error)) {
external_task_ = external_task_local.Pass();
return true;
}
diff --git a/mock_external_task.h b/mock_external_task.h
index 6469c83..62c7455 100644
--- a/mock_external_task.h
+++ b/mock_external_task.h
@@ -19,10 +19,11 @@
const base::Callback<void(pid_t, int)> &death_callback);
virtual ~MockExternalTask();
- MOCK_METHOD4(Start,
+ MOCK_METHOD5(Start,
bool(const base::FilePath &file,
const std::vector<std::string> &arguments,
const std::map<std::string, std::string> &environment,
+ bool terminate_with_parent,
Error *error));
MOCK_METHOD0(Stop, void());
MOCK_METHOD0(OnDelete, void());