From: Thomas Markwalder Date: Wed, 26 Jun 2019 14:57:15 +0000 (-0400) Subject: [#691,!395] Review comments 3 X-Git-Tag: Kea-1.6.0-beta2~195 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=99ed96bcad77256ee43e1491aac7ceab4e5f2c88;p=thirdparty%2Fkea.git [#691,!395] Review comments 3 IfaceMgr and unit tests clean up. --- diff --git a/src/lib/dhcp/iface_mgr.cc b/src/lib/dhcp/iface_mgr.cc index d66bf0fcbf..c7d4bdd227 100644 --- a/src/lib/dhcp/iface_mgr.cc +++ b/src/lib/dhcp/iface_mgr.cc @@ -349,7 +349,7 @@ IfaceMgr::deleteExternalSocket(int socketfd) { int IfaceMgr::purgeBadSockets() { std::vector bad_fds; - BOOST_FOREACH(SocketCallbackInfo s, callbacks_) { + for (SocketCallbackInfo s : callbacks_) { errno = 0; if (fcntl(s.socket_, F_GETFD) < 0 && (errno == EBADF)) { bad_fds.push_back(s.socket_); diff --git a/src/lib/dhcp/iface_mgr.h b/src/lib/dhcp/iface_mgr.h index fc308526ad..9f36a4e50a 100644 --- a/src/lib/dhcp/iface_mgr.h +++ b/src/lib/dhcp/iface_mgr.h @@ -928,10 +928,11 @@ public: /// @brief Scans registered socket set and removes any that are invalid. /// - /// Walks the list of regiseterd external sockets and test each for - /// validity. If any are found to be invalid they are removed. This + /// Walks the list of registered external sockets and testing each for + /// validity. If any are found to be invalid they are removed. This is /// primarily a self-defense mechanism against hook libs or other users - /// of external sockets, leaving a closed socket registered by mistake. + /// of external sockets that may leave a closed socket registered by + /// mistake. /// /// @return A count of the sockets purged. int purgeBadSockets(); diff --git a/src/lib/dhcp/tests/iface_mgr_unittest.cc b/src/lib/dhcp/tests/iface_mgr_unittest.cc index 56e71001d4..2af93ddf83 100644 --- a/src/lib/dhcp/tests/iface_mgr_unittest.cc +++ b/src/lib/dhcp/tests/iface_mgr_unittest.cc @@ -52,6 +52,9 @@ const uint16_t PORT2 = 10548; // V4 socket // tolerance to 0.01s. const uint32_t TIMEOUT_TOLERANCE = 10000; +// Macro for making select wait time arguments for receive functions +#define RECEIVE_WAIT_MS(m) 0,(m*1000) + /// This test verifies that the socket read buffer can be used to /// receive the data and that the data can be read from it. TEST(IfaceTest, readBuffer) { @@ -692,18 +695,18 @@ public: int pipefd[2]; EXPECT_TRUE(pipe(pipefd) == 0); EXPECT_NO_THROW(ifacemgr->addExternalSocket(pipefd[0], - [this, &callback_ok](){ callback_ok = true; })); + [&callback_ok](){ callback_ok = true; })); // Let's create a second pipe and register it as well int secondpipe[2]; EXPECT_TRUE(pipe(secondpipe) == 0); EXPECT_NO_THROW(ifacemgr->addExternalSocket(secondpipe[0], - [this, &callback2_ok](){ callback2_ok = true; })); + [&callback2_ok](){ callback2_ok = true; })); // Verify a call with no data and normal external sockets works ok. Pkt4Ptr pkt4; - ASSERT_NO_THROW(pkt4 = ifacemgr->receive4(1)); + ASSERT_NO_THROW(pkt4 = ifacemgr->receive4(RECEIVE_WAIT_MS(10))); // No callback invocations and no DHCPv4 pkt. EXPECT_FALSE(callback_ok); @@ -716,7 +719,7 @@ public: // We call receive4() which should detect and remove the invalid socket. try { - pkt4 = ifacemgr->receive4(1); + pkt4 = ifacemgr->receive4(RECEIVE_WAIT_MS(10)); ADD_FAILURE() << "receive4 should have failed"; } catch (const SocketReadError& ex) { EXPECT_EQ(std::string("SELECT interrupted by one invalid sockets," @@ -735,7 +738,7 @@ public: EXPECT_EQ(38, write(secondpipe[1], "Hi, this is a message sent over a pipe", 38)); // Call recevie4 again, this should work. - ASSERT_NO_THROW(pkt4 = ifacemgr->receive4(1)); + ASSERT_NO_THROW(pkt4 = ifacemgr->receive4(RECEIVE_WAIT_MS(10))); // Should have callback2 data only. EXPECT_FALSE(callback_ok); @@ -775,18 +778,18 @@ public: int pipefd[2]; EXPECT_TRUE(pipe(pipefd) == 0); EXPECT_NO_THROW(ifacemgr->addExternalSocket(pipefd[0], - [this, &callback_ok](){ callback_ok = true; })); + [&callback_ok](){ callback_ok = true; })); // Let's create a second pipe and register it as well int secondpipe[2]; EXPECT_TRUE(pipe(secondpipe) == 0); EXPECT_NO_THROW(ifacemgr->addExternalSocket(secondpipe[0], - [this, &callback2_ok](){ callback2_ok = true; })); + [&callback2_ok](){ callback2_ok = true; })); // Verify a call with no data and normal external sockets works ok. Pkt6Ptr pkt6; - ASSERT_NO_THROW(pkt6 = ifacemgr->receive6(1)); + ASSERT_NO_THROW(pkt6 = ifacemgr->receive6(RECEIVE_WAIT_MS(10))); // No callback invocations and no DHCPv6 pkt. EXPECT_FALSE(callback_ok); @@ -799,7 +802,7 @@ public: // We call receive6() which should detect and remove the invalid socket. try { - pkt6 = ifacemgr->receive6(1); + pkt6 = ifacemgr->receive6(RECEIVE_WAIT_MS(10)); ADD_FAILURE() << "receive6 should have failed"; } catch (const SocketReadError& ex) { EXPECT_EQ(std::string("SELECT interrupted by one invalid sockets," @@ -818,7 +821,7 @@ public: EXPECT_EQ(38, write(secondpipe[1], "Hi, this is a message sent over a pipe", 38)); // Call recevie6 again, this should work. - ASSERT_NO_THROW(pkt6 = ifacemgr->receive6(1)); + ASSERT_NO_THROW(pkt6 = ifacemgr->receive6(RECEIVE_WAIT_MS(10))); // Should have callback2 data only. EXPECT_FALSE(callback_ok); @@ -2928,16 +2931,18 @@ TEST_F(IfaceMgrTest, DeleteExternalSockets4) { // Tests that an existing external socket that becomes invalid // is detected and purged, without affecting other sockets. -// Tests uses receive4(). -TEST_F(IfaceMgrTest, purgeExternalSockets4) { - // Run purge test without packet queuing. +// Tests uses receive4() without queuing.. +TEST_F(IfaceMgrTest, purgeExternalSockets4Direct) { purgeExternalSockets4Test(); - - // Run purge test with packet queuing. - purgeExternalSockets4Test(true); } +// Tests that an existing external socket that becomes invalid +// is detected and purged, without affecting other sockets. +// Tests uses receive4() with queuing.. +TEST_F(IfaceMgrTest, purgeExternalSockets4Indirect) { + purgeExternalSockets4Test(true); +} // Tests if a single external socket and its callback can be passed and // it is supported properly by receive6() method. @@ -3107,13 +3112,18 @@ TEST_F(IfaceMgrTest, DeleteExternalSockets6) { close(secondpipe[0]); } -// Tests if existing external socket become invalid and be purged and not -// not affect any other existing sockets. Tests uses receive6() -TEST_F(IfaceMgrTest, purgeExternalSockets6) { - // Run purge test without packet queuing. +// Tests that an existing external socket that becomes invalid +// is detected and purged, without affecting other sockets. +// Tests uses receive6() without queuing.. +TEST_F(IfaceMgrTest, purgeExternalSockets6Direct) { purgeExternalSockets6Test(); +} - // Run purge test with packet queuing. + +// Tests that an existing external socket that becomes invalid +// is detected and purged, without affecting other sockets. +// Tests uses receive6() with queuing.. +TEST_F(IfaceMgrTest, purgeExternalSockets6Indirect) { purgeExternalSockets6Test(true); }