libchromeos: Move AsynchronousSignalHandler to an interface.

Both chromeos::AsynchronousSignalHandler and chromeos::Daemon
implemented the same Register/Unregister interface for signal handlers.
This patch moves that functionality to an abstract interface and makes
chromeos::ProcessReapper use that instead of having two methods.

BUG=None
TEST=Unittests still pass.

Change-Id: Ib2aa8c5279b5998e7c88c2211809901fa11a8f0a
Reviewed-on: https://chromium-review.googlesource.com/288752
Trybot-Ready: Alex Deymo <deymo@chromium.org>
Tested-by: Alex Deymo <deymo@chromium.org>
Reviewed-by: Alex Vakulenko <avakulenko@chromium.org>
Commit-Queue: Alex Deymo <deymo@chromium.org>

7 files changed, 80 insertions, 66 deletions

diff --git a/chromeos/asynchronous_signal_handler.h b/chromeos/asynchronous_signal_handler.h
index 407a7ed..4127c11 100644
--- a/chromeos/asynchronous_signal_handler.h
+++ b/chromeos/asynchronous_signal_handler.h
@@ -15,6 +15,7 @@
 #include <base/macros.h>
 #include <base/memory/scoped_ptr.h>
 #include <base/message_loop/message_loop.h>
+#include <chromeos/asynchronous_signal_handler_interface.h>
 #include <chromeos/chromeos_export.h>
 #include <chromeos/message_loops/message_loop.h>
 
@@ -22,36 +23,26 @@ namespace chromeos {
 // Sets up signal handlers for registered signals, and converts signal receipt
 // into a write on a pipe. Watches that pipe for data and, when some appears,
 // execute the associated callback.
-class CHROMEOS_EXPORT AsynchronousSignalHandler final {
+class CHROMEOS_EXPORT AsynchronousSignalHandler final :
+    public AsynchronousSignalHandlerInterface {
  public:
   AsynchronousSignalHandler();
-  ~AsynchronousSignalHandler();
+  ~AsynchronousSignalHandler() override;
 
-  // The callback called when a signal is received.
-  typedef base::Callback<bool(const struct signalfd_siginfo&)> SignalHandler;
+  using AsynchronousSignalHandlerInterface::SignalHandler;
 
   // Initialize the handler.
   void Init();
 
-  // Register a new handler for the given |signal|, replacing any previously
-  // registered handler. |callback| will be called on the thread the
-  // |AsynchronousSignalHandler| is bound to when a signal is received. The
-  // received |signalfd_siginfo| will be passed to |callback|. |callback| must
-  // returns |true| if the signal handler must be unregistered, and |false|
-  // otherwise. Due to an implementation detail, you cannot set any sigaction
-  // flags you might be accustomed to using. This might matter if you hoped to
-  // use SA_NOCLDSTOP to avoid getting a SIGCHLD when a child process receives a
-  // SIGSTOP.
-  void RegisterHandler(int signal, const SignalHandler& callback);
-
-  // Unregister a previously registered handler for the given |signal|.
-  void UnregisterHandler(int signal);
+  // AsynchronousSignalHandlerInterface overrides.
+  void RegisterHandler(int signal, const SignalHandler& callback) override;
+  void UnregisterHandler(int signal) override;
 
+ private:
   // Called from the main loop when we can read from |descriptor_|, indicated
   // that a signal was processed.
   void OnFileCanReadWithoutBlocking();
 
- private:
   // Controller used to manage watching of signalling pipe.
   MessageLoop::TaskId fd_watcher_task_{MessageLoop::kTaskIdNull};
 
diff --git a/chromeos/asynchronous_signal_handler_interface.h b/chromeos/asynchronous_signal_handler_interface.h
new file mode 100644
index 0000000..51e90c5
--- /dev/null
+++ b/chromeos/asynchronous_signal_handler_interface.h
@@ -0,0 +1,42 @@
+// Copyright 2015 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 LIBCHROMEOS_CHROMEOS_ASYNCHRONOUS_SIGNAL_HANDLER_INTERFACE_H_
+#define LIBCHROMEOS_CHROMEOS_ASYNCHRONOUS_SIGNAL_HANDLER_INTERFACE_H_
+
+#include <sys/signalfd.h>
+
+#include <base/callback.h>
+#include <chromeos/chromeos_export.h>
+
+namespace chromeos {
+
+// Sets up signal handlers for registered signals, and converts signal receipt
+// into a write on a pipe. Watches that pipe for data and, when some appears,
+// execute the associated callback.
+class CHROMEOS_EXPORT AsynchronousSignalHandlerInterface {
+ public:
+  virtual ~AsynchronousSignalHandlerInterface() = default;
+
+  // The callback called when a signal is received.
+  using SignalHandler = base::Callback<bool(const struct signalfd_siginfo&)>;
+
+  // Register a new handler for the given |signal|, replacing any previously
+  // registered handler. |callback| will be called on the thread the
+  // |AsynchronousSignalHandlerInterface| implementation is bound to when a
+  // signal is received. The received |signalfd_siginfo| will be passed to
+  // |callback|. |callback| must returns |true| if the signal handler must be
+  // unregistered, and |false| otherwise. Due to an implementation detail, you
+  // cannot set any sigaction flags you might be accustomed to using. This might
+  // matter if you hoped to use SA_NOCLDSTOP to avoid getting a SIGCHLD when a
+  // child process receives a SIGSTOP.
+  virtual void RegisterHandler(int signal, const SignalHandler& callback) = 0;
+
+  // Unregister a previously registered handler for the given |signal|.
+  virtual void UnregisterHandler(int signal) = 0;
+};
+
+}  // namespace chromeos
+
+#endif  // LIBCHROMEOS_CHROMEOS_ASYNCHRONOUS_SIGNAL_HANDLER_INTERFACE_H_
diff --git a/chromeos/daemons/daemon.cc b/chromeos/daemons/daemon.cc
index 95939b2..a964e95 100644
--- a/chromeos/daemons/daemon.cc
+++ b/chromeos/daemons/daemon.cc
@@ -48,7 +48,8 @@ void Daemon::QuitWithExitCode(int exit_code) {
 }
 
 void Daemon::RegisterHandler(
-    int signal, const AsynchronousSignalHandler::SignalHandler& callback) {
+    int signal,
+    const AsynchronousSignalHandlerInterface::SignalHandler& callback) {
   async_signal_handler_.RegisterHandler(signal, callback);
 }
 
diff --git a/chromeos/daemons/daemon.h b/chromeos/daemons/daemon.h
index f36ca7b..c1b7d2b 100644
--- a/chromeos/daemons/daemon.h
+++ b/chromeos/daemons/daemon.h
@@ -25,7 +25,7 @@ namespace chromeos {
 // specialize it by creating your own class and deriving it from
 // chromeos::Daemon. Override some of the virtual methods provide to fine-tune
 // its behavior to suit your daemon's needs.
-class CHROMEOS_EXPORT Daemon {
+class CHROMEOS_EXPORT Daemon : public AsynchronousSignalHandlerInterface {
  public:
   Daemon();
   virtual ~Daemon();
@@ -49,13 +49,15 @@ class CHROMEOS_EXPORT Daemon {
   // this method.
   void QuitWithExitCode(int exit_code);
 
+  // AsynchronousSignalHandlerInterface overrides.
   // Register/unregister custom signal handlers for the daemon. The semantics
   // are identical to AsynchronousSignalHandler::RegisterHandler and
   // AsynchronousSignalHandler::UnregisterHandler, except that handlers for
   // SIGTERM, SIGINT, and SIGHUP cannot be modified.
   void RegisterHandler(
-      int signal, const AsynchronousSignalHandler::SignalHandler& callback);
-  void UnregisterHandler(int signal);
+      int signal, const
+      AsynchronousSignalHandlerInterface::SignalHandler& callback) override;
+  void UnregisterHandler(int signal) override;
 
  protected:
   // Overload to provide your own initialization code that should happen just
diff --git a/chromeos/process_reaper.cc b/chromeos/process_reaper.cc
index 19eef03..c9fbb8e 100644
--- a/chromeos/process_reaper.cc
+++ b/chromeos/process_reaper.cc
@@ -17,38 +17,23 @@
 namespace chromeos {
 
 ProcessReaper::~ProcessReaper() {
-  if (!registered_)
-    Unregister();
+  Unregister();
 }
 
-void ProcessReaper::RegisterWithAsynchronousSignalHandler(
-      AsynchronousSignalHandler* async_signal_handler) {
-  CHECK(!registered_);
+void ProcessReaper::Register(
+      AsynchronousSignalHandlerInterface* async_signal_handler) {
+  CHECK(!async_signal_handler_);
   async_signal_handler_ = async_signal_handler;
   async_signal_handler->RegisterHandler(
       SIGCHLD,
       base::Bind(&ProcessReaper::HandleSIGCHLD, base::Unretained(this)));
-  registered_ = true;
-}
-
-void ProcessReaper::RegisterWithDaemon(Daemon* daemon) {
-  CHECK(!registered_);
-  daemon_ = daemon;
-  daemon->RegisterHandler(SIGCHLD, base::Bind(&ProcessReaper::HandleSIGCHLD,
-                                              base::Unretained(this)));
-  registered_ = true;
 }
 
 void ProcessReaper::Unregister() {
-  if (daemon_) {
-    daemon_->UnregisterHandler(SIGCHLD);
-    daemon_ = nullptr;
-  }
-  if (async_signal_handler_) {
-    async_signal_handler_->UnregisterHandler(SIGCHLD);
-    async_signal_handler_ = nullptr;
-  }
-  registered_ = false;
+  if (!async_signal_handler_)
+    return;
+  async_signal_handler_->UnregisterHandler(SIGCHLD);
+  async_signal_handler_ = nullptr;
 }
 
 bool ProcessReaper::WatchForChild(const tracked_objects::Location& from_here,
diff --git a/chromeos/process_reaper.h b/chromeos/process_reaper.h
index 1e69055..8a8189d 100644
--- a/chromeos/process_reaper.h
+++ b/chromeos/process_reaper.h
@@ -25,16 +25,15 @@ class CHROMEOS_EXPORT ProcessReaper final {
   ProcessReaper() = default;
   ~ProcessReaper();
 
-  // Register the ProcessReaper using either the provided chromeos::Daemon or
-  // chromeos::AsynchronousSignalHandler. You can call Unregister() to remove
-  // this ProcessReapper or it will be called during shutdown.
-  void RegisterWithAsynchronousSignalHandler(
-      AsynchronousSignalHandler* async_signal_handler);
-  void RegisterWithDaemon(Daemon* daemon);
-
-  // Unregisters the ProcessReaper from the chromeos::Daemon or
-  // chromeos::AsynchronousSignalHandler passed in RegisterWith*(). It doesn't
-  // do anything if not registered.
+  // Register the ProcessReaper using either the provided
+  // chromeos::AsynchronousSignalHandlerInterface. You can call Unregister() to
+  // remove this ProcessReapper or it will be called during shutdown.
+  // You can only register this ProcessReaper with one signal handler at a time.
+  void Register(AsynchronousSignalHandlerInterface* async_signal_handler);
+
+  // Unregisters the ProcessReaper from the
+  // chromeos::AsynchronousSignalHandlerInterface passed in Register(). It
+  // doesn't do anything if not registered.
   void Unregister();
 
   // Watch for the child process |pid| to finish and call |callback| when the
@@ -56,13 +55,9 @@ class CHROMEOS_EXPORT ProcessReaper final {
   };
   std::map<pid_t, WatchedProcess> watched_processes_;
 
-  // Whether the ProcessReaper already registered a signal.
-  bool registered_{false};
-
-  // The |async_signal_handler_| and |daemon_| are owned by the caller and only
-  // one of them is not nullptr.
-  AsynchronousSignalHandler* async_signal_handler_{nullptr};
-  Daemon* daemon_{nullptr};
+  // The |async_signal_handler_| is owned by the caller and is |nullptr| when
+  // not registered.
+  AsynchronousSignalHandlerInterface* async_signal_handler_{nullptr};
 
   DISALLOW_COPY_AND_ASSIGN(ProcessReaper);
 };
diff --git a/chromeos/process_reaper_unittest.cc b/chromeos/process_reaper_unittest.cc
index 44b4048..d49ce37 100644
--- a/chromeos/process_reaper_unittest.cc
+++ b/chromeos/process_reaper_unittest.cc
@@ -48,8 +48,7 @@ class ProcessReaperTest : public ::testing::Test {
   void SetUp() override {
     chromeos_loop_.SetAsCurrent();
     async_signal_handler_.Init();
-    process_reaper_.RegisterWithAsynchronousSignalHandler(
-        &async_signal_handler_);
+    process_reaper_.Register(&async_signal_handler_);
   }
 
  protected:
@@ -68,8 +67,7 @@ TEST_F(ProcessReaperTest, UnregisterWhenNotRegistered) {
 
 TEST_F(ProcessReaperTest, UnregisterAndReregister) {
   process_reaper_.Unregister();
-  process_reaper_.RegisterWithAsynchronousSignalHandler(
-      &async_signal_handler_);
+  process_reaper_.Register(&async_signal_handler_);
   // This checks that we can unregister the ProcessReaper and then destroy it.
   process_reaper_.Unregister();
 }