shill: openvpn: Improve openvpn shutdown and resource cleanup.
This patch includes two changes:
- Improves ProcessKiller to guard against the case where shill has
already reaped the dead child process but ProcessKiller ignored that
fact because a child watch had been registered and was removed in
the meantime.
- OpenVPNDriver removes the child watch callback before disconnecting
the management interface to ensure cleaner termination and reaping
of the openvpn process through ProcessKiller.
BUG=chromium-os:39638
TEST=unit tests; tested on device by connecting and disconnecting to
OpenVPN and observing no left-over tun* interfaces or openvpn
processes; tested on device by manually modifying shill to sleep after
stopping the OpenVPN interface and observing no left-over tun*
interfaces or openvpn processes.
Change-Id: I19168cec3f5445938c910462b4a390bdbbc82639
Reviewed-on: https://gerrit.chromium.org/gerrit/45308
Tested-by: Darin Petkov <petkov@chromium.org>
Reviewed-by: Paul Stewart <pstew@chromium.org>
Commit-Queue: Darin Petkov <petkov@chromium.org>
diff --git a/mock_process_killer.h b/mock_process_killer.h
index 6240b5a..6399f34 100644
--- a/mock_process_killer.h
+++ b/mock_process_killer.h
@@ -16,7 +16,7 @@
MockProcessKiller();
virtual ~MockProcessKiller();
- MOCK_METHOD2(Wait, void(int pid, const base::Closure &callback));
+ MOCK_METHOD2(Wait, bool(int pid, const base::Closure &callback));
MOCK_METHOD2(Kill, void(int pid, const base::Closure &callback));
private:
diff --git a/openvpn_driver.cc b/openvpn_driver.cc
index 9c3d66a..a322476 100644
--- a/openvpn_driver.cc
+++ b/openvpn_driver.cc
@@ -162,6 +162,13 @@
SLOG(VPN, 2) << __func__ << "(" << Service::ConnectStateToString(state)
<< ", " << error_details << ")";
StopConnectTimeout();
+ if (child_watch_tag_) {
+ glib_->SourceRemove(child_watch_tag_);
+ child_watch_tag_ = 0;
+ }
+ // Disconnecting the management interface will terminate the openvpn
+ // process. Ensure this is handled robustly by first removing the child watch
+ // above and then terminating and reaping the process through ProcessKiller.
management_server_->Stop();
if (!tls_auth_file_.empty()) {
file_util::Delete(tls_auth_file_, false);
@@ -171,10 +178,6 @@
manager()->DeregisterDefaultServiceCallback(default_service_callback_tag_);
default_service_callback_tag_ = 0;
}
- if (child_watch_tag_) {
- glib_->SourceRemove(child_watch_tag_);
- child_watch_tag_ = 0;
- }
rpc_task_.reset();
int interface_index = -1;
if (device_) {
diff --git a/process_killer.cc b/process_killer.cc
index 7496c15..67229e9 100644
--- a/process_killer.cc
+++ b/process_killer.cc
@@ -4,6 +4,9 @@
#include "shill/process_killer.h"
+#include <errno.h>
+#include <sys/wait.h>
+
using base::Closure;
using std::map;
@@ -28,20 +31,42 @@
return g_process_killer.Pointer();
}
-void ProcessKiller::Wait(int pid, const Closure &callback) {
+bool ProcessKiller::Wait(int pid, const Closure &callback) {
LOG(INFO) << "Waiting for pid " << pid;
if (!callback.is_null()) {
callbacks_[pid] = callback;
}
+ // Check if the child process is dead already. This guards against the case
+ // when the caller had registered a child watch on that process but the
+ // process exited before the caller removed the watch and invoked this.
+ int status = 0;
+ pid_t rpid = waitpid(pid, &status, WNOHANG);
+ if (rpid == -1) {
+ DCHECK_EQ(ECHILD, errno);
+ LOG(INFO) << "No such child -- assuming process has already exited.";
+ OnProcessDied(pid, 0, this);
+ return false;
+ }
+ if (rpid == pid) {
+ LOG(INFO) << "Process has already exited.";
+ OnProcessDied(pid, status, this);
+ return false;
+ }
g_child_watch_add(pid, OnProcessDied, this);
+ return true;
}
void ProcessKiller::Kill(int pid, const Closure &callback) {
- Wait(pid, callback);
+ if (!Wait(pid, callback)) {
+ LOG(INFO) << "Process already dead, no need to kill.";
+ return;
+ }
LOG(INFO) << "Killing pid " << pid;
// TODO(petkov): Consider sending subsequent periodic signals and raising the
// signal to SIGKILL if the process keeps running.
- kill(pid, SIGTERM);
+ if (kill(pid, SIGTERM) == -1) {
+ PLOG(ERROR) << "SIGTERM failed";
+ }
}
// static
diff --git a/process_killer.h b/process_killer.h
index 30cb580..70c367c 100644
--- a/process_killer.h
+++ b/process_killer.h
@@ -26,8 +26,9 @@
// Uses GLib to wait asynchronously for the process to exit and reap it. GLib
// supports only a single callback per process ID so there should be no other
// child watch callbacks registered for this |pid|. If |callback| is non-null,
- // runs it when the process exits.
- virtual void Wait(int pid, const base::Closure &callback);
+ // runs it when the process exits. Returns false if the process has already
+ // exited.
+ virtual bool Wait(int pid, const base::Closure &callback);
// Terminates process |pid| and reaps it through Wait(pid, callback).
virtual void Kill(int pid, const base::Closure &callback);