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>
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 @@
// 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);
+ // AsynchronousSignalHandlerInterface overrides.
+ void RegisterHandler(int signal, const SignalHandler& callback) override;
+ void UnregisterHandler(int signal) override;
- // Unregister a previously registered handler for the given |signal|.
- void UnregisterHandler(int signal);
-
+ 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::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 @@
// 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 @@
// 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 @@
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);
+ // 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::Daemon or
- // chromeos::AsynchronousSignalHandler passed in RegisterWith*(). It doesn't
- // do anything if not registered.
+ // 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 @@
};
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 @@
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, 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();
}