From: Tomek Mrugalski Date: Mon, 3 Feb 2014 15:09:14 +0000 (+0100) Subject: [3315] Changes after review: X-Git-Tag: bind10-1.2.0beta1-release~78^2~3 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=22d124644f6fc0480295c4d02104687965afd24b;p=thirdparty%2Fkea.git [3315] Changes after review: - SessionCallback renamed to SocketCallback - Removed references in comments to session - removed unused names string in receive{4,6} - extended unit-test to check that unregistered callback is not triggered - Implemented similar tests for receive6() - Fixed a bug that was revealed by those new unit-tests :) --- diff --git a/src/lib/dhcp/iface_mgr.cc b/src/lib/dhcp/iface_mgr.cc index cf13028027..ae58c7e7f5 100644 --- a/src/lib/dhcp/iface_mgr.cc +++ b/src/lib/dhcp/iface_mgr.cc @@ -227,7 +227,7 @@ IfaceMgr::isDirectResponseSupported() const { } void -IfaceMgr::addExternalSocket(int socketfd, SessionCallback callback) { +IfaceMgr::addExternalSocket(int socketfd, SocketCallback callback) { for (SocketCallbackContainer::iterator s = callbacks_.begin(); s != callbacks_.end(); ++s) { @@ -240,7 +240,7 @@ IfaceMgr::addExternalSocket(int socketfd, SessionCallback callback) { } // Add a new entry to the callbacks vector - SocketCallback x; + SocketCallbackInfo x; x.socket_ = socketfd; x.callback_ = callback; callbacks_.push_back(x); @@ -819,7 +819,6 @@ IfaceMgr::receive4(uint32_t timeout_sec, uint32_t timeout_usec /* = 0 */) { IfaceCollection::const_iterator iface; fd_set sockets; int maxfd = 0; - stringstream names; FD_ZERO(&sockets); @@ -834,7 +833,6 @@ IfaceMgr::receive4(uint32_t timeout_sec, uint32_t timeout_usec /* = 0 */) { // Only deal with IPv4 addresses. if (s->addr_.isV4()) { - names << s->sockfd_ << "(" << iface->getName() << ") "; // Add this socket to listening set FD_SET(s->sockfd_, &sockets); @@ -845,7 +843,7 @@ IfaceMgr::receive4(uint32_t timeout_sec, uint32_t timeout_usec /* = 0 */) { } } - // if there are any session sockets registered... + // if there are any callbacks for external sockets registered... if (!callbacks_.empty()) { for (SocketCallbackContainer::const_iterator s = callbacks_.begin(); s != callbacks_.end(); ++s) { @@ -853,7 +851,6 @@ IfaceMgr::receive4(uint32_t timeout_sec, uint32_t timeout_usec /* = 0 */) { if (maxfd < s->socket_) { maxfd = s->socket_; } - names << s->socket_ << "(session)"; } } @@ -877,13 +874,11 @@ IfaceMgr::receive4(uint32_t timeout_sec, uint32_t timeout_usec /* = 0 */) { continue; } - // something received over session socket + // something received over external socket - // in theory we could call io_service.run_one() here, instead of - // implementing callback mechanism, but that would introduce - // asiolink dependency to libdhcp++ and that is something we want - // to avoid (see CPE market and out long term plans for minimalistic - // implementations. + // Calling the external socket's callback provides its service + // layer access without integrating any specific features + // in IfaceMgr if (s->callback_) { s->callback_(); } @@ -925,7 +920,6 @@ Pkt6Ptr IfaceMgr::receive6(uint32_t timeout_sec, uint32_t timeout_usec /* = 0 */ const SocketInfo* candidate = 0; fd_set sockets; int maxfd = 0; - stringstream names; FD_ZERO(&sockets); @@ -940,7 +934,6 @@ Pkt6Ptr IfaceMgr::receive6(uint32_t timeout_sec, uint32_t timeout_usec /* = 0 */ // Only deal with IPv6 addresses. if (s->addr_.isV6()) { - names << s->sockfd_ << "(" << iface->getName() << ") "; // Add this socket to listening set FD_SET(s->sockfd_, &sockets); @@ -951,7 +944,7 @@ Pkt6Ptr IfaceMgr::receive6(uint32_t timeout_sec, uint32_t timeout_usec /* = 0 */ } } - // if there are any session sockets registered... + // if there are any callbacks for external sockets registered... if (!callbacks_.empty()) { for (SocketCallbackContainer::const_iterator s = callbacks_.begin(); s != callbacks_.end(); ++s) { @@ -961,7 +954,6 @@ Pkt6Ptr IfaceMgr::receive6(uint32_t timeout_sec, uint32_t timeout_usec /* = 0 */ if (maxfd < s->socket_) { maxfd = s->socket_; } - names << s->socket_ << "(session)"; } } @@ -982,13 +974,11 @@ Pkt6Ptr IfaceMgr::receive6(uint32_t timeout_sec, uint32_t timeout_usec /* = 0 */ for (SocketCallbackContainer::iterator s = callbacks_.begin(); s != callbacks_.end(); ++s) { if (FD_ISSET(s->socket_, &sockets)) { - // something received over session socket + // something received over external socket if (s->callback_) { - // in theory we could call io_service.run_one() here, instead of - // implementing callback mechanism, but that would introduce - // asiolink dependency to libdhcp++ and that is something we want - // to avoid (see CPE market and out long term plans for minimalistic - // implementations. + // Calling the external socket's callback provides its service + // layer access without integrating any specific features + // in IfaceMgr s->callback_(); } } diff --git a/src/lib/dhcp/iface_mgr.h b/src/lib/dhcp/iface_mgr.h index 75cbc533fc..aa4e12af46 100644 --- a/src/lib/dhcp/iface_mgr.h +++ b/src/lib/dhcp/iface_mgr.h @@ -394,20 +394,20 @@ boost::function IfaceMgrErrorMsgCallback; /// class IfaceMgr : public boost::noncopyable { public: - /// Defines callback used when commands are received over control session. - typedef void (*SessionCallback) (void); + /// Defines callback used when data is received over external sockets. + typedef boost::function SocketCallback; /// Keeps callback information for external sockets. - struct SocketCallback { + struct SocketCallbackInfo { /// Socket descriptor of the external socket. int socket_; /// A callback that will be called when data arrives over socket_. - SessionCallback callback_; + SocketCallback callback_; }; /// Defines storage container for callbacks for external sockets - typedef std::list SocketCallbackContainer; + typedef std::list SocketCallbackContainer; /// @brief Packet reception buffer size /// @@ -797,14 +797,14 @@ public: /// @return number of detected interfaces uint16_t countIfaces() { return ifaces_.size(); } - /// @brief Sets external socket and a callback + /// @brief Adds external socket and a callback /// - /// Specifies session socket and a callback that will be called + /// Specifies external socket and a callback that will be called /// when data will be received over that socket. /// /// @param socketfd socket descriptor /// @param callback callback function - void addExternalSocket(int socketfd, SessionCallback callback); + void addExternalSocket(int socketfd, SocketCallback callback); /// @brief Deletes external socket diff --git a/src/lib/dhcp/tests/iface_mgr_unittest.cc b/src/lib/dhcp/tests/iface_mgr_unittest.cc index a1adf0b82d..3f7add8897 100644 --- a/src/lib/dhcp/tests/iface_mgr_unittest.cc +++ b/src/lib/dhcp/tests/iface_mgr_unittest.cc @@ -2601,9 +2601,9 @@ void my_callback2(void) { callback2_ok = true; } -// Tests if a signle external socket and its callback can be passed and +// Tests if a single external socket and its callback can be passed and // it is supported properly by receive4() method. -TEST_F(IfaceMgrTest, SingleExternalSession) { +TEST_F(IfaceMgrTest, SingleExternalSocket) { callback_ok = false; @@ -2642,7 +2642,7 @@ TEST_F(IfaceMgrTest, SingleExternalSession) { // Tests if multiple external sockets and their callbacks can be passed and // it is supported properly by receive4() method. -TEST_F(IfaceMgrTest, MiltipleControlSessions) { +TEST_F(IfaceMgrTest, MiltipleExternalSockets) { callback_ok = false; callback2_ok = false; @@ -2683,7 +2683,8 @@ TEST_F(IfaceMgrTest, MiltipleControlSessions) { EXPECT_FALSE(callback2_ok); // Read the data sent, because our test callbacks are too dumb to actually - // do it. + // do it. We don't care about the content read, because we're testing + // the callbacks, not pipes. char buf[80]; read(pipefd[0], buf, 80); @@ -2714,7 +2715,7 @@ TEST_F(IfaceMgrTest, MiltipleControlSessions) { // Tests if existing external socket can be deleted and that such deletion does // not affect any other existing sockets. -TEST_F(IfaceMgrTest, DeleteControlSessions) { +TEST_F(IfaceMgrTest, DeleteExternalSockets) { callback_ok = false; callback2_ok = false; @@ -2748,6 +2749,18 @@ TEST_F(IfaceMgrTest, DeleteControlSessions) { EXPECT_FALSE(callback_ok); EXPECT_TRUE(callback2_ok); + // Let's reset the status + callback_ok = false; + callback2_ok = false; + + // Now let's send something over the first callback that was unregistered. + // We should NOT receive any callback. + EXPECT_EQ(38, write(pipefd[1], "Hi, this is a message sent over a pipe", 38)); + + // Now check that the first callback is NOT called. + ASSERT_NO_THROW(pkt4 = ifacemgr->receive4(1)); + EXPECT_FALSE(callback_ok); + // close both pipe ends close(pipefd[1]); close(pipefd[0]);