From: Thomas Markwalder Date: Wed, 25 Jun 2014 20:28:50 +0000 (-0400) Subject: [3407] Addressed review comments X-Git-Tag: trac3434_base~4^2 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=75d29c25146ca7451081be86e7a5e7d625e75d75;p=thirdparty%2Fkea.git [3407] Addressed review comments Added missing parameter and method commentary. Extended unit tests as suggested. Moved onreceipt_handler_ from SignalSet to anonymous namespace. --- diff --git a/src/bin/d2/d_controller.h b/src/bin/d2/d_controller.h index 6724a76258..c1e35c9e0d 100644 --- a/src/bin/d2/d_controller.h +++ b/src/bin/d2/d_controller.h @@ -405,6 +405,9 @@ protected: /// @return returns an Element that contains the results of shutdown /// command composed of an integer status value (0 means successful, /// non-zero means failure), and a string explanation of the outcome. + /// + /// @param args is a set of derivation-specific arguments (if any) + /// for the shutdown command. isc::data::ConstElementPtr shutdownProcess(isc::data::ConstElementPtr args); /// @brief Initializes signal handling @@ -437,6 +440,8 @@ protected: /// obtained from the IOSignal. This allows derivations to supply a /// custom signal processing method, while ensuring IOSignalQueue /// integrity. + /// + /// @param sequence_id id of the IOSignal instance "received" void ioSignalHandler(IOSignalId sequence_id); /// @brief Fetches the current process diff --git a/src/bin/d2/io_service_signal.h b/src/bin/d2/io_service_signal.h index 0df1821022..4b5fad8ea9 100644 --- a/src/bin/d2/io_service_signal.h +++ b/src/bin/d2/io_service_signal.h @@ -108,6 +108,7 @@ public: IOSignal(asiolink::IOService& io_service, int signum, IOSignalHandler handler); + /// @brief Destructor ~IOSignal(); /// @brief Static method for generating IOSignal sequence_ids. @@ -203,7 +204,7 @@ public: /// @throw IOSignalError if io_service is NULL. IOSignalQueue (IOServicePtr& io_service); - /// Destructor. + /// @brief Destructor. ~IOSignalQueue(); /// @brief Creates an IOSignal diff --git a/src/bin/d2/tests/d_controller_unittests.cc b/src/bin/d2/tests/d_controller_unittests.cc index 2f757e92ef..07e47b9a90 100644 --- a/src/bin/d2/tests/d_controller_unittests.cc +++ b/src/bin/d2/tests/d_controller_unittests.cc @@ -43,7 +43,7 @@ public: virtual ~DStubControllerTest() { } - + DStubControllerPtr controller_; }; @@ -355,7 +355,7 @@ TEST_F(DStubControllerTest, ioSignals) { // base class signal handler. controller_->recordSignalOnly(true); - // Setup to raise SIGHUP in 10 ms. + // Setup to raise SIGHUP in 10 ms. TimedSignal sighup(*getIOService(), SIGHUP, 10); TimedSignal sigint(*getIOService(), SIGINT, 10); TimedSignal sigterm(*getIOService(), SIGTERM, 10); @@ -380,17 +380,17 @@ TEST_F(DStubControllerTest, invalidConfigReload) { // new content is invalid JSON which will cause the config parse to fail. scheduleTimedWrite("{ \"string_test\": BOGUS JSON }", 100); - // Setup to raise SIGHUP in 200 ms. + // Setup to raise SIGHUP in 200 ms. TimedSignal sighup(*getIOService(), SIGHUP, 200); // Write the config and then run launch() for 500 ms - // After startup, which will load the initial configuration this enters - // the process's runIO() loop. We will first rewrite the config file. + // After startup, which will load the initial configuration this enters + // the process's runIO() loop. We will first rewrite the config file. // Next we process the SIGHUP signal which should cause us to reconfigure. time_duration elapsed_time; runWithConfig("{ \"string_test\": \"first value\" }", 500, elapsed_time); - // Context is still available post launch. Check to see that our + // Context is still available post launch. Check to see that our // configuration value is still the original value. std::string actual_value = ""; ASSERT_NO_THROW(getContext()->getParam("string_test", actual_value)); @@ -409,28 +409,30 @@ TEST_F(DStubControllerTest, validConfigReload) { // file is updated after we have done the initial configuration. scheduleTimedWrite("{ \"string_test\": \"second value\" }", 100); - // Setup to raise SIGHUP in 200 ms. + // Setup to raise SIGHUP in 200 ms and another at 400 ms. TimedSignal sighup(*getIOService(), SIGHUP, 200); + TimedSignal sighup2(*getIOService(), SIGHUP, 400); - // Write the config and then run launch() for 500 ms + // Write the config and then run launch() for 800 ms time_duration elapsed_time; - runWithConfig("{ \"string_test\": \"first value\" }", 500, elapsed_time); + runWithConfig("{ \"string_test\": \"first value\" }", 800, elapsed_time); - // Context is still available post launch. + // Context is still available post launch. // Check to see that our configuration value is what we expect. std::string actual_value = ""; ASSERT_NO_THROW(getContext()->getParam("string_test", actual_value)); EXPECT_EQ("second value", actual_value); - // Verify that we saw the signal. + // Verify that we saw two occurrences of the signal. std::vector& signals = controller_->getProcessedSignals(); - ASSERT_EQ(1, signals.size()); + ASSERT_EQ(2, signals.size()); EXPECT_EQ(SIGHUP, signals[0]); + EXPECT_EQ(SIGHUP, signals[1]); } // Tests that the SIGINT triggers a normal shutdown. TEST_F(DStubControllerTest, sigintShutdown) { - // Setup to raise SIGHUP in 1 ms. + // Setup to raise SIGHUP in 1 ms. TimedSignal sighup(*getIOService(), SIGINT, 1); // Write the config and then run launch() for 1000 ms @@ -448,7 +450,7 @@ TEST_F(DStubControllerTest, sigintShutdown) { // Tests that the SIGTERM triggers a normal shutdown. TEST_F(DStubControllerTest, sigtermShutdown) { - // Setup to raise SIGHUP in 1 ms. + // Setup to raise SIGHUP in 1 ms. TimedSignal sighup(*getIOService(), SIGTERM, 1); // Write the config and then run launch() for 1000 ms diff --git a/src/bin/d2/tests/d_test_stubs.h b/src/bin/d2/tests/d_test_stubs.h index ca2c3456f0..7fcf8b5f46 100644 --- a/src/bin/d2/tests/d_test_stubs.h +++ b/src/bin/d2/tests/d_test_stubs.h @@ -230,6 +230,8 @@ public: /// DControllerBase::processSignals will also be invoked. This switch is /// useful for ensuring that IOSignals are delivered as expected without /// incurring the full impact such as reconfiguring or shutting down. + /// + /// @param value boolean which if true enables record-only behavior void recordSignalOnly(bool value) { record_signal_only_ = value; } @@ -280,6 +282,14 @@ protected: /// @return returns a string containing the option letters. virtual const std::string getCustomOpts() const; + /// @brief Application-level "signal handler" + /// + /// Overrides the base class implementation such that this method + /// is invoked each time an IOSignal is processed. It records the + /// signal received and unless we are in record-only behavior, it + /// in invokes the base class implementation. + /// + /// @param signum OS signal value received virtual void processSignal(int signum); private: diff --git a/src/bin/d2/tests/io_service_signal_unittests.cc b/src/bin/d2/tests/io_service_signal_unittests.cc index 0ac0555972..bc01712f12 100644 --- a/src/bin/d2/tests/io_service_signal_unittests.cc +++ b/src/bin/d2/tests/io_service_signal_unittests.cc @@ -139,6 +139,7 @@ void dummyHandler(IOSignalId) { TEST(IOSignal, construction) { IOServicePtr io_service(new asiolink::IOService()); IOSignalPtr signal; + IOSignalPtr signal2; // Verify that handler cannot be empty. ASSERT_THROW(signal.reset(new IOSignal(*io_service, SIGINT, @@ -153,6 +154,16 @@ TEST(IOSignal, construction) { // Verify SIGINT is correct. EXPECT_EQ(SIGINT, signal->getSignum()); + + // Make a second signal. + ASSERT_NO_THROW(signal2.reset(new IOSignal(*io_service, SIGUSR1, + dummyHandler))); + + // Verify sequence_id is not the same as the previous one. + EXPECT_NE(signal2->getSequenceId(), signal->getSequenceId()); + + // Verify that the signal value is correct. + EXPECT_EQ(SIGUSR1, signal2->getSignum()); } // Tests IOSignalQueue constructors and exercises queuing methods. @@ -168,22 +179,52 @@ TEST(IOSignalQueue, constructionAndQueuing) { ASSERT_NO_THROW(queue.reset(new IOSignalQueue(io_service))); // Verify an empty handler is not allowed. - ASSERT_THROW(queue->pushSignal(SIGINT, IOSignalHandler()), - IOSignalError); + ASSERT_THROW(queue->pushSignal(SIGINT, IOSignalHandler()), IOSignalError); + + // Verify that we can queue valid entries. + std::vector ids; + ASSERT_NO_THROW(ids.push_back(queue->pushSignal(SIGINT, dummyHandler))); + ASSERT_NO_THROW(ids.push_back(queue->pushSignal(SIGUSR1, dummyHandler))); + ASSERT_NO_THROW(ids.push_back(queue->pushSignal(SIGUSR2, dummyHandler))); - // Verify we can queue up a valid entry. - IOSignalId sequence_id = queue->pushSignal(SIGINT, dummyHandler); + // Now verify that we can pop each one and what we pop is correct. + // Verify popping it again, throws. We'll do it in a non-sequential order. - // Verify we can pop the entry. - IOSignalPtr signal = queue->popSignal(sequence_id); + // Check the middle one. + IOSignalPtr signal; + ASSERT_NO_THROW(signal = queue->popSignal(ids[1])); ASSERT_TRUE(signal); + EXPECT_EQ(ids[1], signal->getSequenceId()); + EXPECT_EQ(SIGUSR1, signal->getSignum()); + ASSERT_THROW(queue->popSignal(ids[1]), IOSignalError); - // Verify the one we popped is right. - EXPECT_EQ(sequence_id, signal->getSequenceId()); + // Check the first one. + ASSERT_NO_THROW(signal = queue->popSignal(ids[0])); + ASSERT_TRUE(signal); + EXPECT_EQ(ids[0], signal->getSequenceId()); EXPECT_EQ(SIGINT, signal->getSignum()); + ASSERT_THROW(queue->popSignal(ids[0]), IOSignalError); + + // Check the last one. + ASSERT_NO_THROW(signal = queue->popSignal(ids[2])); + ASSERT_TRUE(signal); + EXPECT_EQ(ids[2], signal->getSequenceId()); + EXPECT_EQ(SIGUSR2, signal->getSignum()); + ASSERT_THROW(queue->popSignal(ids[2]), IOSignalError); + + // Now we will test clearing the queue. Queue three signals. + ids.clear(); + for (int i = 0; i < 3; ++i) { + ASSERT_NO_THROW(ids.push_back(queue->pushSignal(SIGINT, dummyHandler))); + } + + // Now clear the queue. + ASSERT_NO_THROW(queue->clear()); - // Verify popping it again, throws. - ASSERT_THROW(queue->popSignal(sequence_id), IOSignalError); + // We should not be able to dequeue any of them. + for (int i = 0; i < 3; ++i) { + ASSERT_THROW(queue->popSignal(ids[i]), IOSignalError); + } } // Test the basic mechanics of IOSignal by handling one signal occurrence. diff --git a/src/lib/util/signal_set.cc b/src/lib/util/signal_set.cc index 5fa4be813d..b1308e1a1c 100644 --- a/src/lib/util/signal_set.cc +++ b/src/lib/util/signal_set.cc @@ -78,17 +78,17 @@ void internalHandler(int sig) { states->push_back(sig); } +/// @brief Optional handler to execute at the time of signal receipt +BoolSignalHandler onreceipt_handler_ = BoolSignalHandler(); + }; // end anon namespace namespace isc { namespace util { -const BoolSignalHandler SignalSet::EMPTY_BOOL_HANDLER = BoolSignalHandler(); -BoolSignalHandler SignalSet::onreceipt_handler_ = EMPTY_BOOL_HANDLER; - bool SignalSet::invokeOnReceiptHandler(int sig) { - if (!SignalSet::onreceipt_handler_) { + if (!onreceipt_handler_) { return (false); } @@ -109,7 +109,7 @@ SignalSet::invokeOnReceiptHandler(int sig) { // Call the registered handler. bool signal_processed = false; try { - signal_processed = SignalSet::onreceipt_handler_(sig); + signal_processed = onreceipt_handler_(sig); } catch (const std::exception& ex) { // Restore the handler. We might fail to restore it, but we likely // have bigger issues anyway. @@ -291,7 +291,7 @@ SignalSet::setOnReceiptHandler(BoolSignalHandler handler) { void SignalSet::clearOnReceiptHandler() { - onreceipt_handler_ = EMPTY_BOOL_HANDLER; + onreceipt_handler_ = BoolSignalHandler(); } } // end of isc::util diff --git a/src/lib/util/signal_set.h b/src/lib/util/signal_set.h index fb1dc298b8..d10f0ce58a 100644 --- a/src/lib/util/signal_set.h +++ b/src/lib/util/signal_set.h @@ -79,10 +79,6 @@ typedef boost::function BoolSignalHandler; /// signals' queue. class SignalSet : public boost::noncopyable { public: - /// @brief Optional handler to execute at the time of signal receipt - static BoolSignalHandler onreceipt_handler_; - static const BoolSignalHandler EMPTY_BOOL_HANDLER; - /// @brief Constructor installing one signal. /// /// @param sig0 First signal.