From: Razvan Becheriu Date: Tue, 18 Nov 2025 20:23:10 +0000 (+0200) Subject: [#4141] addressed review comments X-Git-Tag: Kea-3.1.4~22 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=bdc0d067f0cd22d4608f8024ee30504289ba0d63;p=thirdparty%2Fkea.git [#4141] addressed review comments --- diff --git a/src/lib/dhcp/iface_mgr.cc b/src/lib/dhcp/iface_mgr.cc index 17d71e2154..778897b5f3 100644 --- a/src/lib/dhcp/iface_mgr.cc +++ b/src/lib/dhcp/iface_mgr.cc @@ -1133,10 +1133,10 @@ Pkt4Ptr IfaceMgr::receive4Indirect(uint32_t timeout_sec, uint32_t timeout_usec / errno = 0; if (fcntl(s.socket_, F_GETFD) < 0 && (errno == EBADF)) { s.unusable_ = true; - } else if (!s.unusable_) { - // Add this socket to listening set - fd_event_handler_->add(s.socket_); + isc_throw(SocketFDError, "unexpected state (closed) for fd: " << s.socket_); } + // Add this socket to listening set + fd_event_handler_->add(s.socket_); } } } @@ -1177,7 +1177,8 @@ Pkt4Ptr IfaceMgr::receive4Indirect(uint32_t timeout_sec, uint32_t timeout_usec / if (errno == EINTR) { isc_throw(SignalInterruptOnSelect, strerror(errno)); } else if (errno == EBADF) { - isc_throw(SocketFDError, strerror(errno)); + isc_throw(SocketFDError, "interrupted by one invalid sockets with error: " + << strerror(errno)); } else { isc_throw(SocketReadError, strerror(errno)); } @@ -1195,35 +1196,29 @@ Pkt4Ptr IfaceMgr::receive4Indirect(uint32_t timeout_sec, uint32_t timeout_usec / // Let's find out which external socket has the data SocketCallbackInfo ex_sock; bool found = false; - bool fd_error = false; { std::lock_guard lock(callbacks_mutex_); for (SocketCallbackInfo& s : callbacks_) { - if (fd_event_handler_->hasError(s.socket_)) { - fd_error = true; + if (fd_event_handler_->readReady(s.socket_)) { + found = true; + + // something received over external socket + if (s.callback_) { + // Note the external socket to call its callback without + // the lock taken so it can be deleted. + ex_sock = s; + break; + } + } else if (fd_event_handler_->hasError(s.socket_)) { errno = 0; if (fcntl(s.socket_, F_GETFD) < 0 && (errno == EBADF)) { s.unusable_ = true; + isc_throw(SocketFDError, "unexpected state (closed) for fd: " << s.socket_); } - break; - } - if (!fd_event_handler_->readReady(s.socket_)) { - continue; - } - found = true; - - // something received over external socket - if (s.callback_) { - // Note the external socket to call its callback without - // the lock taken so it can be deleted. - ex_sock = s; - break; + isc_throw(SocketFDError, "unexpected socket error: " << strerror(errno)); } } } - if (fd_error) { - isc_throw(SocketFDError, strerror(errno)); - } if (ex_sock.callback_) { // Calling the external socket's callback provides its service @@ -1282,10 +1277,10 @@ Pkt4Ptr IfaceMgr::receive4Direct(uint32_t timeout_sec, uint32_t timeout_usec /* errno = 0; if (fcntl(s.socket_, F_GETFD) < 0 && (errno == EBADF)) { s.unusable_ = true; - } else if (!s.unusable_) { - // Add this socket to listening set - fd_event_handler_->add(s.socket_); + isc_throw(SocketFDError, "unexpected state (closed) for fd: " << s.socket_); } + // Add this socket to listening set + fd_event_handler_->add(s.socket_); } } } @@ -1309,7 +1304,8 @@ Pkt4Ptr IfaceMgr::receive4Direct(uint32_t timeout_sec, uint32_t timeout_usec /* if (errno == EINTR) { isc_throw(SignalInterruptOnSelect, strerror(errno)); } else if (errno == EBADF) { - isc_throw(SocketFDError, strerror(errno)); + isc_throw(SocketFDError, "interrupted by one invalid sockets with error: " + << strerror(errno)); } else { isc_throw(SocketReadError, strerror(errno)); } @@ -1318,35 +1314,29 @@ Pkt4Ptr IfaceMgr::receive4Direct(uint32_t timeout_sec, uint32_t timeout_usec /* // Let's find out which socket has the data SocketCallbackInfo ex_sock; bool found = false; - bool fd_error = false; { std::lock_guard lock(callbacks_mutex_); for (SocketCallbackInfo& s : callbacks_) { - if (fd_event_handler_->hasError(s.socket_)) { - fd_error = true; + if (fd_event_handler_->readReady(s.socket_)) { + found = true; + + // something received over external socket + if (s.callback_) { + // Note the external socket to call its callback without + // the lock taken so it can be deleted. + ex_sock = s; + break; + } + } else if (fd_event_handler_->hasError(s.socket_)) { errno = 0; if (fcntl(s.socket_, F_GETFD) < 0 && (errno == EBADF)) { s.unusable_ = true; + isc_throw(SocketFDError, "unexpected state (closed) for fd: " << s.socket_); } - break; - } - if (!fd_event_handler_->readReady(s.socket_)) { - continue; - } - found = true; - - // something received over external socket - if (s.callback_) { - // Note the external socket to call its callback without - // the lock taken so it can be deleted. - ex_sock = s; - break; + isc_throw(SocketFDError, "unexpected socket error: " << strerror(errno)); } } } - if (fd_error) { - isc_throw(SocketFDError, strerror(errno)); - } if (ex_sock.callback_) { // Calling the external socket's callback provides its service @@ -1433,10 +1423,10 @@ IfaceMgr::receive6Direct(uint32_t timeout_sec, uint32_t timeout_usec /* = 0 */ ) errno = 0; if (fcntl(s.socket_, F_GETFD) < 0 && (errno == EBADF)) { s.unusable_ = true; - } else if (!s.unusable_) { - // Add this socket to listening set - fd_event_handler_->add(s.socket_); + isc_throw(SocketFDError, "unexpected state (closed) for fd: " << s.socket_); } + // Add this socket to listening set + fd_event_handler_->add(s.socket_); } } } @@ -1460,7 +1450,8 @@ IfaceMgr::receive6Direct(uint32_t timeout_sec, uint32_t timeout_usec /* = 0 */ ) if (errno == EINTR) { isc_throw(SignalInterruptOnSelect, strerror(errno)); } else if (errno == EBADF) { - isc_throw(SocketFDError, strerror(errno)); + isc_throw(SocketFDError, "interrupted by one invalid sockets with error: " + << strerror(errno)); } else { isc_throw(SocketReadError, strerror(errno)); } @@ -1469,35 +1460,29 @@ IfaceMgr::receive6Direct(uint32_t timeout_sec, uint32_t timeout_usec /* = 0 */ ) // Let's find out which socket has the data SocketCallbackInfo ex_sock; bool found = false; - bool fd_error = false; { std::lock_guard lock(callbacks_mutex_); for (SocketCallbackInfo& s : callbacks_) { - if (fd_event_handler_->hasError(s.socket_)) { - fd_error = true; + if (fd_event_handler_->readReady(s.socket_)) { + found = true; + + // something received over external socket + if (s.callback_) { + // Note the external socket to call its callback without + // the lock taken so it can be deleted. + ex_sock = s; + break; + } + } else if (fd_event_handler_->hasError(s.socket_)) { errno = 0; if (fcntl(s.socket_, F_GETFD) < 0 && (errno == EBADF)) { s.unusable_ = true; + isc_throw(SocketFDError, "unexpected state (closed) for fd: " << s.socket_); } - break; - } - if (!fd_event_handler_->readReady(s.socket_)) { - continue; - } - found = true; - - // something received over external socket - if (s.callback_) { - // Note the external socket to call its callback without - // the lock taken so it can be deleted. - ex_sock = s; - break; + isc_throw(SocketFDError, "unexpected socket error: " << strerror(errno)); } } } - if (fd_error) { - isc_throw(SocketFDError, strerror(errno)); - } if (ex_sock.callback_) { // Calling the external socket's callback provides its service @@ -1554,10 +1539,10 @@ IfaceMgr::receive6Indirect(uint32_t timeout_sec, uint32_t timeout_usec /* = 0 */ errno = 0; if (fcntl(s.socket_, F_GETFD) < 0 && (errno == EBADF)) { s.unusable_ = true; - } else if (!s.unusable_) { - // Add this socket to listening set - fd_event_handler_->add(s.socket_); + isc_throw(SocketFDError, "unexpected state (closed) for fd: " << s.socket_); } + // Add this socket to listening set + fd_event_handler_->add(s.socket_); } } } @@ -1598,7 +1583,8 @@ IfaceMgr::receive6Indirect(uint32_t timeout_sec, uint32_t timeout_usec /* = 0 */ if (errno == EINTR) { isc_throw(SignalInterruptOnSelect, strerror(errno)); } else if (errno == EBADF) { - isc_throw(SocketFDError, strerror(errno)); + isc_throw(SocketFDError, "interrupted by one invalid sockets with error: " + << strerror(errno)); } else { isc_throw(SocketReadError, strerror(errno)); } @@ -1616,35 +1602,29 @@ IfaceMgr::receive6Indirect(uint32_t timeout_sec, uint32_t timeout_usec /* = 0 */ // Let's find out which external socket has the data SocketCallbackInfo ex_sock; bool found = false; - bool fd_error = false; { std::lock_guard lock(callbacks_mutex_); for (SocketCallbackInfo& s : callbacks_) { - if (fd_event_handler_->hasError(s.socket_)) { - fd_error = true; + if (fd_event_handler_->readReady(s.socket_)) { + found = true; + + // something received over external socket + if (s.callback_) { + // Note the external socket to call its callback without + // the lock taken so it can be deleted. + ex_sock = s; + break; + } + } else if (fd_event_handler_->hasError(s.socket_)) { errno = 0; if (fcntl(s.socket_, F_GETFD) < 0 && (errno == EBADF)) { s.unusable_ = true; + isc_throw(SocketFDError, "unexpected state (closed) for fd: " << s.socket_); } - break; - } - if (!fd_event_handler_->readReady(s.socket_)) { - continue; - } - found = true; - - // something received over external socket - if (s.callback_) { - // Note the external socket to call its callback without - // the lock taken so it can be deleted. - ex_sock = s; - break; + isc_throw(SocketFDError, "unexpected socket error: " << strerror(errno)); } } } - if (fd_error) { - isc_throw(SocketFDError, strerror(errno)); - } if (ex_sock.callback_) { // Calling the external socket's callback provides its service diff --git a/src/lib/dhcp/tests/iface_mgr_unittest.cc b/src/lib/dhcp/tests/iface_mgr_unittest.cc index ebfb8364cb..cc2f2617fc 100644 --- a/src/lib/dhcp/tests/iface_mgr_unittest.cc +++ b/src/lib/dhcp/tests/iface_mgr_unittest.cc @@ -734,9 +734,9 @@ public: // We call receive4() which should detect and remove the invalid socket. try { pkt4 = ifacemgr->receive4(RECEIVE_WAIT_MS(10)); - } catch (const SocketReadError& ex) { - EXPECT_EQ(std::string("Bad file descriptor"), - std::string(ex.what())); + } catch (const SocketFDError& ex) { + std::string err_msg("unexpected state (closed) for fd: "); + EXPECT_EQ(err_msg, std::string(ex.what()).substr(0, err_msg.length())); } catch (const std::exception& ex) { ADD_FAILURE() << "wrong exception thrown: " << ex.what(); } @@ -822,9 +822,9 @@ public: // We call receive6() which should detect and remove the invalid socket. try { pkt6 = ifacemgr->receive6(RECEIVE_WAIT_MS(10)); - } catch (const SocketReadError& ex) { - EXPECT_EQ(std::string("Bad file descriptor"), - std::string(ex.what())); + } catch (const SocketFDError& ex) { + std::string err_msg("unexpected state (closed) for fd: "); + EXPECT_EQ(err_msg, std::string(ex.what()).substr(0, err_msg.length())); } catch (const std::exception& ex) { ADD_FAILURE() << "wrong exception thrown: " << ex.what(); } diff --git a/src/lib/util/tests/fd_event_handler_unittests.h b/src/lib/util/tests/fd_event_handler_unittests.h index b5cc5927a6..047846e1f3 100644 --- a/src/lib/util/tests/fd_event_handler_unittests.h +++ b/src/lib/util/tests/fd_event_handler_unittests.h @@ -177,6 +177,8 @@ TEST_F(FDEventHandlerTest, badFD) { if (handler_->type() == FDEventHandler::TYPE_SELECT) { EXPECT_EQ(-1, handler_->waitEvent(0, 1000)); + // on error readReady is not actually returning the new state, + // but the read flag added before wait for the fd. EXPECT_TRUE(handler_->readReady(fd)); EXPECT_FALSE(handler_->hasError(fd)); EXPECT_EQ(EBADF, errno); @@ -247,6 +249,8 @@ TEST_F(FDEventHandlerTest, badFD) { EXPECT_EQ(EBADF, errno); + // on error readReady is not actually returning the new state, + // but the read flag added before wait for the fd. EXPECT_TRUE(handler_->readReady(pipe_fd_[0])); EXPECT_FALSE(handler_->hasError(pipe_fd_[0])); #endif @@ -305,6 +309,8 @@ TEST_F(FDEventHandlerTest, badFD) { EXPECT_EQ(EBADF, errno); + // on error readReady is not actually returning the new state, + // but the read flag added before wait for the fd. EXPECT_TRUE(handler_->readReady(pipe_fd_[1])); EXPECT_FALSE(handler_->hasError(pipe_fd_[1])); #endif