From: Marcin Siodelski Date: Mon, 24 Aug 2015 12:25:15 +0000 (+0200) Subject: [3970] Propagate errors emitted when closing the WatchSocket. X-Git-Tag: fd4o6_base~5^2~14 X-Git-Url: http://git.ipfire.org/gitweb/gitweb.cgi?a=commitdiff_plain;h=c3a1e5668e5af72e45d1240661e9de5a8156ee6b;p=thirdparty%2Fkea.git [3970] Propagate errors emitted when closing the WatchSocket. --- diff --git a/src/lib/dhcp_ddns/dhcp_ddns_messages.mes b/src/lib/dhcp_ddns/dhcp_ddns_messages.mes index fd0091a368..b379d477d5 100644 --- a/src/lib/dhcp_ddns/dhcp_ddns_messages.mes +++ b/src/lib/dhcp_ddns/dhcp_ddns_messages.mes @@ -89,14 +89,8 @@ caught in the application's send completion handler. This is a programmatic error that needs to be reported. Dependent upon the nature of the error the client may or may not continue operating normally. -% DHCP_DDNS_WATCH_SINK_CLOSE_ERROR Sink-side watch socket failed to close: %1 +% DHCP_DDNS_UDP_SENDER_WATCH_SOCKET_CLOSE_ERROR watch socket failed to close: %1 This is an error message that indicates the application was unable to close -the inbound side of a NCR sender's watch socket. While technically possible -this error is highly unlikely to occur and should not impair the application's -ability to process requests. - -% DHCP_DDNS_WATCH_SOURCE_CLOSE_ERROR Source-side watch socket failed to close: %1 -This is an error message that indicates the application was unable to close -the outbound side of a NCR sender's watch socket. While technically possible -this error is highly unlikely to occur and should not impair the application's -ability to process requests. +the inbound or outbound side of a NCR sender's watch socket. While technically +possible the error is highly unlikely to occur and should not impair the +application's ability to process requests. diff --git a/src/lib/dhcp_ddns/ncr_udp.cc b/src/lib/dhcp_ddns/ncr_udp.cc index 683147f04b..28e665f8d6 100644 --- a/src/lib/dhcp_ddns/ncr_udp.cc +++ b/src/lib/dhcp_ddns/ncr_udp.cc @@ -260,6 +260,7 @@ NameChangeUDPSender::open(isc::asiolink::IOService& io_service) { send_callback_->setDataSource(server_endpoint_); + closeWatchSocket(); watch_socket_.reset(new util::WatchSocket()); } @@ -288,6 +289,7 @@ NameChangeUDPSender::close() { socket_.reset(); + closeWatchSocket(); watch_socket_.reset(); } @@ -372,7 +374,17 @@ NameChangeUDPSender::ioReady() { return (false); } - +void +NameChangeUDPSender::closeWatchSocket() { + if (watch_socket_) { + std::string error_string; + watch_socket_->closeSocket(error_string); + if (!error_string.empty()) { + LOG_ERROR(dhcp_ddns_logger, DHCP_DDNS_UDP_SENDER_WATCH_SOCKET_CLOSE_ERROR) + .arg(error_string); + } + } +} }; // end of isc::dhcp_ddns namespace }; // end of isc namespace diff --git a/src/lib/dhcp_ddns/ncr_udp.h b/src/lib/dhcp_ddns/ncr_udp.h index 89646d118c..c927f9220e 100644 --- a/src/lib/dhcp_ddns/ncr_udp.h +++ b/src/lib/dhcp_ddns/ncr_udp.h @@ -548,6 +548,13 @@ public: virtual bool ioReady(); private: + + /// @brief Closes watch socket if the socket is open. + /// + /// This method closes watch socket if its instance exists and if the + /// socket is open. An error message is logged when this operation fails. + void closeWatchSocket(); + /// @brief IP address from which to send. isc::asiolink::IOAddress ip_address_; diff --git a/src/lib/util/tests/watch_socket_unittests.cc b/src/lib/util/tests/watch_socket_unittests.cc index 465084c94e..a17730c108 100644 --- a/src/lib/util/tests/watch_socket_unittests.cc +++ b/src/lib/util/tests/watch_socket_unittests.cc @@ -231,4 +231,27 @@ TEST(WatchSocketTest, badReadOnClear) { ASSERT_THROW(watch->markReady(), WatchSocketError); } +/// @brief Checks if the socket can be explicitly closed. +TEST(WatchSocketTest, explicitClose) { + WatchSocketPtr watch; + + // Create new instance of the socket. + ASSERT_NO_THROW(watch.reset(new WatchSocket())); + ASSERT_TRUE(watch); + + // Make sure it has been opened by checking that its descriptor + // is valid. + EXPECT_NE(watch->getSelectFd(), WatchSocket::SOCKET_NOT_VALID); + + // Close the socket. + std::string error_string; + ASSERT_TRUE(watch->closeSocket(error_string)); + + // Make sure that the descriptor is now invalid which indicates + // that the socket has been closed. + EXPECT_EQ(WatchSocket::SOCKET_NOT_VALID, watch->getSelectFd()); + // No errors should be reported. + EXPECT_TRUE(error_string.empty()); +} + } // end of anonymous namespace diff --git a/src/lib/util/watch_socket.cc b/src/lib/util/watch_socket.cc index a364edcd04..9f496d9e0e 100644 --- a/src/lib/util/watch_socket.cc +++ b/src/lib/util/watch_socket.cc @@ -116,17 +116,19 @@ WatchSocket::clearReady() { } } -void -WatchSocket::closeSocket() { +bool +WatchSocket::closeSocket(std::string& error_string) { + // Clear error string. + error_string.clear(); + // Close the pipe fds. Technically a close can fail (hugely unlikely) // but there's no recovery for it either. If one does fail we log it // and go on. Plus this is called by the destructor and no one likes // destructors that throw. if (source_ != SOCKET_NOT_VALID) { if (close(source_)) { -/* const char* errstr = strerror(errno); - LOG_ERROR(dhcp_ddns_logger, DHCP_DDNS_WATCH_SOURCE_CLOSE_ERROR) - .arg(errstr); */ + // An error occured. + error_string = strerror(errno); } source_ = SOCKET_NOT_VALID; @@ -134,13 +136,23 @@ WatchSocket::closeSocket() { if (sink_ != SOCKET_NOT_VALID) { if (close(sink_)) { -/* const char* errstr = strerror(errno); - LOG_ERROR(dhcp_ddns_logger, DHCP_DDNS_WATCH_SINK_CLOSE_ERROR) - .arg(errstr); */ + // An error occured. + if (error_string.empty()) { + error_string = strerror(errno); + } } sink_ = SOCKET_NOT_VALID; } + + // If any errors have been reported, return false. + return (error_string.empty() ? true : false); +} + +void +WatchSocket::closeSocket() { + std::string error_string; + closeSocket(error_string); } int diff --git a/src/lib/util/watch_socket.h b/src/lib/util/watch_socket.h index 0bdca4acf1..a2e1ad1ef2 100644 --- a/src/lib/util/watch_socket.h +++ b/src/lib/util/watch_socket.h @@ -114,11 +114,23 @@ public: /// pipe. int getSelectFd(); + /// @brief Closes the descriptors associated with the socket. + /// + /// This method is used to close the socket and capture errors that + /// may occur during this operation. + /// + /// @param [out] error_string Holds the error string if closing + /// the socket failed. It will hold empty string otherwise. + /// + /// @return true if the operation was successful, false otherwise. + bool closeSocket(std::string& error_string); + private: + /// @brief Closes the descriptors associated with the socket. /// - /// Used internally in the destructor and if an error occurs marking or - /// clearing the socket. + /// This method is called by the class destructor and it ignores + /// any errors that may occur while closing the sockets. void closeSocket(); /// @brief The end of the pipe to which the marker is written