[3405] Signal set can handle a signal it owns.

diff --git a/src/lib/util/io/signal_set.cc b/src/lib/util/io/signal_set.cc
index 87b9e1a4bb99534b809bab126ee6a38310781174..694b77d3bb22274198d190c3c37c8b9a7657d0dc 100644 (file)
--- a/src/lib/util/io/signal_set.cc
+++ b/src/lib/util/io/signal_set.cc
@@ -65,18 +65,6 @@ void internalHandler(int sig) {
     states->push_back(sig);
 }
 
-/// @brief Pops a next signal number from the static collection of signals.
-///
-/// The static collection of signals is updated by the internal signal
-/// handler being invoked when one of the installed signals is received by
-/// the process. This function removes the first element of the collection.
-void popNext() {
-    std::list<int>* states = getSignalStates();
-    if (!states->empty()) {
-        states->pop_front();
-    }
-}
-
 }
 
 namespace isc {
@@ -137,10 +125,13 @@ SignalSet::clear() {
 int
 SignalSet::getNext() const {
     std::list<int>* states = getSignalStates();
-    if (states->empty()) {
-        return (-1);
+    for (std::list<int>::iterator it = states->begin();
+         it != states->end(); ++it) {
+        if (local_signals_.find(*it) != local_signals_.end()) {
+            return (*it);
+        }
     }
-    return (*states->begin());
+    return (-1);
 }
 
 void
@@ -150,7 +141,16 @@ SignalSet::erase(const int sig) {
                   << " from a signal set: signal is not owned by the"
                   " signal set");
     }
+    // Remove globally registered signal.
     getRegisteredSignals()->erase(sig);
+    // Remove unhandled signals from the queue.
+    for (std::list<int>::iterator it = getSignalStates()->begin();
+         it != getSignalStates()->end(); ++it) {
+        if (*it == sig) {
+            it = getSignalStates()->erase(it);
+        }
+    }
+    // Remove locally registered signal.
     local_signals_.erase(sig);
 }
 
@@ -192,6 +192,18 @@ SignalSet::maskSignals(const int mask) const {
     sigprocmask(mask, &new_set, 0);
 }
 
+void
+SignalSet::popNext() {
+    std::list<int>* states = getSignalStates();
+    for (std::list<int>::iterator it = states->begin();
+         it != states->end(); ++it) {
+        if (local_signals_.find(*it) != local_signals_.end()) {
+            states->erase(it);
+            return;
+        }
+    }
+}
+
 void
 SignalSet::remove(const int sig) {
     // Unregister only if we own this signal.

diff --git a/src/lib/util/io/signal_set.h b/src/lib/util/io/signal_set.h
index 914a06cd06778ff90c2ef3045dfcea80eadb97f0..5d525595687edd360e5b89b82ce9f13919435083 100644 (file)
--- a/src/lib/util/io/signal_set.h
+++ b/src/lib/util/io/signal_set.h
@@ -26,6 +26,8 @@ namespace isc {
 namespace util {
 namespace io {
 
+/// @brief Exception thrown when the @c isc::util::io::SignalSet class
+/// experiences an error.
 class SignalSetError : public Exception {
 public:
     SignalSetError(const char* file, size_t line, const char* what) :
@@ -161,6 +163,13 @@ private:
     /// @param mask A mask to be applied to all signals.
     void maskSignals(const int mask) const;
 
+    /// @brief Pops a next signal number from the static collection of signals.
+    ///
+    /// The static collection of signals is updated by the internal signal
+    /// handler being invoked when one of the installed signals is received by
+    /// the process. This function removes the first element of the collection.
+    void popNext();
+
     /// @brief Unblocks signals in the set.
     ///
     /// This function unblocks the signals in a set.

diff --git a/src/lib/util/io/tests/signal_set_unittest.cc b/src/lib/util/io/tests/signal_set_unittest.cc
index 27cc24bac4894dab3cc198a86120d54966465d4c..9fcd4fda477ea102f8221a3bc4857a5db7df791f 100644 (file)
--- a/src/lib/util/io/tests/signal_set_unittest.cc
+++ b/src/lib/util/io/tests/signal_set_unittest.cc
@@ -23,34 +23,53 @@ namespace {
 using namespace isc;
 using namespace isc::util::io;
 
+/// @brief Test fixture class for @c isc::util::io::SignalSet class.
+///
+/// This class contains a handler function which records the signal
+/// being handled. It allows for checking whether the signal set
+/// has invoked the handler for the expected signal.
 class SignalSetTest : public ::testing::Test {
 public:
 
+    /// @brief Constructor.
+    ///
+    /// Resets the signal sets and variables being modified by the
+    /// signal handler function.
     SignalSetTest()
-        : signal_set_() {
+        : signal_set_(),
+          secondary_signal_set_() {
         handler_calls_ = 0;
         signum_ = -1;
     }
 
+    /// @brief Destructor.
+    ///
+    /// Uninstalls the signals from the signal sets.
     ~SignalSetTest() {
         if (signal_set_) {
             signal_set_->clear();
         }
+        if (secondary_signal_set_) {
+            secondary_signal_set_->clear();
+        }
     }
 
-    void handleNext() {
-        signal_set_->handleNext(boost::bind(&SignalSetTest::testHandler, _1));
-    }
-
+    /// @brief Signal handler used by unit tests.
+    ///
+    /// @param signum Signal being handled.
     static void testHandler(int signum) {
         signum_ = signum;
         ++handler_calls_;
     }
 
+    /// @brief Number of handler calls so far.
     static int handler_calls_;
+    /// @brief The last signal handled.
     static int signum_;
+    /// @brief Test signal set object.
     SignalSetPtr signal_set_;
-
+    /// @brief Second signal set object.
+    SignalSetPtr secondary_signal_set_;
 };
 
 int SignalSetTest::handler_calls_ = 0;
@@ -77,14 +96,14 @@ TEST_F(SignalSetTest, twoSignals) {
     // second one should be dropped.
     ASSERT_EQ(0, raise(SIGHUP));
     // Execute the first handler (for SIGHUP).
-    handleNext();
+    signal_set_->handleNext(boost::bind(&SignalSetTest::testHandler, _1));
     // The handler should have been called once and the signal
     // handled should be SIGHUP.
     EXPECT_EQ(1, handler_calls_);
     EXPECT_EQ(SIGHUP, signum_);
     // Next signal to be handled should be SIGINT.
     EXPECT_EQ(SIGINT, signal_set_->getNext());
-    handleNext();
+    signal_set_->handleNext(boost::bind(&SignalSetTest::testHandler, _1));
     EXPECT_EQ(2, handler_calls_);
     EXPECT_EQ(SIGINT, signum_);
     // There should be no more waiting handlers.
@@ -94,28 +113,58 @@ TEST_F(SignalSetTest, twoSignals) {
     EXPECT_NO_THROW(signal_set_->remove(SIGINT));
 }
 
+/// Check that the signal set can only handle signals owned by it.
+TEST_F(SignalSetTest, twoSignalSets) {
+    // Register handler for SIGHUP in the first signal set.
+    signal_set_.reset(new SignalSet(SIGHUP));
+    // Register handler for SIGINT in the second signal set.
+    secondary_signal_set_.reset(new SignalSet(SIGINT));
+    // Send SIGHUP.
+    ASSERT_EQ(0, raise(SIGHUP));
+    // Send SIGINT.
+    ASSERT_EQ(0, raise(SIGINT));
+    // Although the SIGHUP is the first signal received by the process
+    // it is not owned by the secondary signal set. The first signal
+    // to be handled by the secondary signal set is SIGINT.
+    EXPECT_EQ(SIGINT, secondary_signal_set_->getNext());
+    // The signal set owns SIGHUP so it should be the next to handle.
+    EXPECT_EQ(SIGHUP, signal_set_->getNext());
+    // Handle next signal owned by the secondary signal set.
+    secondary_signal_set_->handleNext(boost::bind(&SignalSetTest::testHandler,
+                                                  _1));
+    EXPECT_EQ(1, handler_calls_);
+    EXPECT_EQ(SIGINT, signum_);
+    // No more signals to be handled for this signal set.
+    EXPECT_EQ(-1, secondary_signal_set_->getNext());
+    // Handle next signal owned by the signal set.
+    signal_set_->handleNext(boost::bind(&SignalSetTest::testHandler, _1));
+    EXPECT_EQ(2, handler_calls_);
+    EXPECT_EQ(SIGHUP, signum_);
+    // No more signals to be handled by this signal set.
+    EXPECT_EQ(-1, signal_set_->getNext());
+}
+
 // Check that each signal set removes only the signals that it has been used
 // to register.
-TEST_F(SignalSetTest, twoSignalSets) {
+TEST_F(SignalSetTest, remove) {
     // Register handlers for SIGHUP using one signal set.
     ASSERT_NO_THROW(signal_set_.reset(new SignalSet(SIGHUP)));
     // Define another signal set and register a different signal.
-    SignalSetPtr other_set;
-    ASSERT_NO_THROW(other_set.reset(new SignalSet(SIGINT)));
+    ASSERT_NO_THROW(secondary_signal_set_.reset(new SignalSet(SIGINT)));
     // The SIGHUP has been already registsred with the other signal
     // set, so it should not be possible to register it again.
-    ASSERT_THROW(other_set->add(SIGHUP), SignalSetError);
+    ASSERT_THROW(secondary_signal_set_->add(SIGHUP), SignalSetError);
     // It shouldn't be possible to remove the signal registered in a different
     // signal set.
-    ASSERT_THROW(other_set->remove(SIGHUP), SignalSetError);
+    ASSERT_THROW(secondary_signal_set_->remove(SIGHUP), SignalSetError);
     // Remove all signals from the first signal set. The signals registered
     // with the other signal signal set should be preserved.
     ASSERT_NO_THROW(signal_set_->clear());
     // Check indirectly that the SIGINT is still registered. An attempt to
     // register registered signal should result in failure.
-    EXPECT_THROW(other_set->add(SIGINT), SignalSetError);
+    EXPECT_THROW(secondary_signal_set_->add(SIGINT), SignalSetError);
     // But, we should be able to regsiter SIGHUP.
-    EXPECT_NO_THROW(other_set->add(SIGHUP));
+    EXPECT_NO_THROW(secondary_signal_set_->add(SIGHUP));
 }
 
 /// Check that it is not possible to duplicate signals.