From: Razvan Becheriu Date: Thu, 20 Nov 2025 21:36:06 +0000 (+0200) Subject: [#4141] addressed review comments X-Git-Tag: Kea-3.1.4~17 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=ffe8b40c94b47cfbe3ea9f86c02161dd9904fe01;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 81cf98fe19..71050d5c11 100644 --- a/src/lib/dhcp/iface_mgr.cc +++ b/src/lib/dhcp/iface_mgr.cc @@ -332,10 +332,11 @@ IfaceMgr::addExternalSocket(int socketfd, SocketCallback callback) { isc_throw(BadValue, "Attempted to install callback for invalid socket " << socketfd); } - if (std::this_thread::get_id() != id_) { - isc_throw(InvalidOperation, "Attempted to register external socket from different thread " - << std::this_thread::get_id()); - } + // @todo: remove comment when exclusively allow external sockets actions on main thread. + //if (std::this_thread::get_id() != id_) { + // isc_throw(InvalidOperation, "Attempted to register external socket from different thread " + // << std::this_thread::get_id()); + //} std::lock_guard lock(callbacks_mutex_); for (SocketCallbackInfo& s : callbacks_) { // There's such a socket description there already. @@ -362,10 +363,11 @@ IfaceMgr::deleteExternalSocket(int socketfd) { void IfaceMgr::deleteExternalSocketInternal(int socketfd) { - if (std::this_thread::get_id() != id_) { - isc_throw(InvalidOperation, "Attempted to unregister external socket from different thread " - << std::this_thread::get_id()); - } + // @todo: remove comment when exclusively allow external sockets actions on main thread. + //if (std::this_thread::get_id() != id_) { + // isc_throw(InvalidOperation, "Attempted to unregister external socket from different thread " + // << std::this_thread::get_id()); + //} for (SocketCallbackInfoContainer::iterator s = callbacks_.begin(); s != callbacks_.end(); ++s) { if (s->socket_ == socketfd) { @@ -401,10 +403,11 @@ IfaceMgr::isExternalSocketUnusable(int fd) { void IfaceMgr::deleteAllExternalSockets() { - if (std::this_thread::get_id() != id_) { - isc_throw(InvalidOperation, "Attempted to unregister external sockets from different thread " - << std::this_thread::get_id()); - } + // @todo: remove comment when exclusively allow external sockets actions on main thread. + //if (std::this_thread::get_id() != id_) { + // isc_throw(InvalidOperation, "Attempted to unregister external sockets from different thread " + // << std::this_thread::get_id()); + //} std::lock_guard lock(callbacks_mutex_); callbacks_.clear(); } @@ -917,38 +920,44 @@ IfaceMgr::clearBoundAddresses() { void IfaceMgr::handleExternalSocketError(SocketCallbackInfo& s) { - 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_); - } - isc_throw(SocketFDError, "unexpected socket fd: " << s.socket_ << " error: " - << strerror(errno)); + errno = 0; + if (fcntl(s.socket_, F_GETFD) < 0 && (errno == EBADF)) { + s.unusable_ = true; + isc_throw(SocketFDError, "unexpected state (closed) for fd: " << s.socket_); } } void -IfaceMgr::handleIfaceSocketError(util::FDEventHandlerPtr fd_event_handler, const SocketInfo& s) { - if (fd_event_handler->hasError(s.sockfd_)) { - errno = 0; - if (fcntl(s.sockfd_, F_GETFD) < 0 && (errno == EBADF)) { - isc_throw(SocketFDError, "unexpected state (closed) for fd: " << s.sockfd_); - } - isc_throw(SocketFDError, "unexpected socket fd: " << s.sockfd_ << " error: " +IfaceMgr::handleIfaceSocketError(const SocketInfo& s) { + int flag = 0; + socklen_t opt_size = sizeof(flag); + if (getsockopt(s.sockfd_, SOL_SOCKET, SO_ERROR, + (char *)&flag, &opt_size) < 0) { + isc_throw(BadValue, "Can't get socket fd: " << s.sockfd_ << " flags - error: " << strerror(errno)); - } else { - int flag = 0; - socklen_t opt_size = sizeof(flag); - if (getsockopt(s.sockfd_, SOL_SOCKET, SO_ERROR, - (char *)&flag, &opt_size) < 0) { - isc_throw(BadValue, "Can't get socket fd: " << s.sockfd_ << " flags - error: " - << strerror(errno)); + } + if (flag > 0) { + isc_throw(SocketFDError, "socket fd: " << s.sockfd_ << " error flags: " << flag); + } +} + +void +IfaceMgr::handleClosedExternalSockets() { + bool found = false; + std::lock_guard lock(callbacks_mutex_); + for (SocketCallbackInfo& s : callbacks_) { + if (s.unusable_) { + continue; } - if (flag > 0) { - isc_throw(SocketFDError, "socket fd: " << s.sockfd_ << " error flags: " << flag); + errno = 0; + if (fcntl(s.socket_, F_GETFD) < 0 && (errno == EBADF)) { + s.unusable_ = true; + found = true; } } + if (found) { + isc_throw(SocketFDError, "unexpected state (closed) for external sockets"); + } } void @@ -1238,8 +1247,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, "interrupted by one invalid sockets with error: " - << strerror(errno)); + handleClosedExternalSockets(); + return (Pkt4Ptr()); } else { isc_throw(SocketReadError, strerror(errno)); } @@ -1260,7 +1269,8 @@ Pkt4Ptr IfaceMgr::receive4Indirect(uint32_t timeout_sec, uint32_t timeout_usec / { std::lock_guard lock(callbacks_mutex_); for (SocketCallbackInfo& s : callbacks_) { - if (fd_event_handler_->readReady(s.socket_)) { + if (fd_event_handler_->readReady(s.socket_) || + fd_event_handler_->hasError(s.socket_)) { found = true; // something received over external socket @@ -1360,8 +1370,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, "interrupted by one invalid sockets with error: " - << strerror(errno)); + handleClosedExternalSockets(); + return (Pkt4Ptr()); } else { isc_throw(SocketReadError, strerror(errno)); } @@ -1373,7 +1383,8 @@ Pkt4Ptr IfaceMgr::receive4Direct(uint32_t timeout_sec, uint32_t timeout_usec /* { std::lock_guard lock(callbacks_mutex_); for (SocketCallbackInfo& s : callbacks_) { - if (fd_event_handler_->readReady(s.socket_)) { + if (fd_event_handler_->readReady(s.socket_) || + fd_event_handler_->hasError(s.socket_)) { found = true; // something received over external socket @@ -1404,11 +1415,12 @@ Pkt4Ptr IfaceMgr::receive4Direct(uint32_t timeout_sec, uint32_t timeout_usec /* IfacePtr recv_if; for (const IfacePtr& iface : ifaces_) { for (const SocketInfo& s : iface->getSockets()) { - if (fd_event_handler_->readReady(s.sockfd_)) { + if (fd_event_handler_->readReady(s.sockfd_) || + fd_event_handler_->hasError(s.sockfd_)) { candidate.reset(new SocketInfo(s)); break; } else { - handleIfaceSocketError(fd_event_handler_, s); + handleIfaceSocketError(s); } } if (candidate) { @@ -1500,8 +1512,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, "interrupted by one invalid sockets with error: " - << strerror(errno)); + handleClosedExternalSockets(); + return (Pkt6Ptr()); } else { isc_throw(SocketReadError, strerror(errno)); } @@ -1513,7 +1525,8 @@ IfaceMgr::receive6Direct(uint32_t timeout_sec, uint32_t timeout_usec /* = 0 */ ) { std::lock_guard lock(callbacks_mutex_); for (SocketCallbackInfo& s : callbacks_) { - if (fd_event_handler_->readReady(s.socket_)) { + if (fd_event_handler_->readReady(s.socket_) || + fd_event_handler_->hasError(s.socket_)) { found = true; // something received over external socket @@ -1543,11 +1556,12 @@ IfaceMgr::receive6Direct(uint32_t timeout_sec, uint32_t timeout_usec /* = 0 */ ) // @todo: fix iface starvation for (const IfacePtr& iface : ifaces_) { for (const SocketInfo& s : iface->getSockets()) { - if (fd_event_handler_->readReady(s.sockfd_)) { + if (fd_event_handler_->readReady(s.sockfd_) || + fd_event_handler_->hasError(s.sockfd_)) { candidate.reset(new SocketInfo(s)); break; } else { - handleIfaceSocketError(fd_event_handler_, s); + handleIfaceSocketError(s); } } if (candidate) { @@ -1627,8 +1641,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, "interrupted by one invalid sockets with error: " - << strerror(errno)); + handleClosedExternalSockets(); + return (Pkt6Ptr()); } else { isc_throw(SocketReadError, strerror(errno)); } @@ -1649,7 +1663,8 @@ IfaceMgr::receive6Indirect(uint32_t timeout_sec, uint32_t timeout_usec /* = 0 */ { std::lock_guard lock(callbacks_mutex_); for (SocketCallbackInfo& s : callbacks_) { - if (fd_event_handler_->readReady(s.socket_)) { + if (fd_event_handler_->readReady(s.socket_) || + fd_event_handler_->hasError(s.socket_)) { found = true; // something received over external socket @@ -1740,14 +1755,15 @@ IfaceMgr::receiveDHCP4Packets() { // Let's find out which interface/socket has data. for (const IfacePtr& iface : ifaces_) { for (const SocketInfo& s : iface->getSockets()) { - if (receiver_fd_event_handler_->readReady(s.sockfd_)) { + if (receiver_fd_event_handler_->readReady(s.sockfd_) || + receiver_fd_event_handler_->hasError(s.sockfd_)) { receiveDHCP4Packet(*iface, s); // Can take time so check one more time the watch socket. if (dhcp_receiver_->shouldTerminate()) { return; } } else { - handleIfaceSocketError(receiver_fd_event_handler_, s); + handleIfaceSocketError(s); } } } @@ -1814,14 +1830,15 @@ IfaceMgr::receiveDHCP6Packets() { // Let's find out which interface/socket has data. for (const IfacePtr& iface : ifaces_) { for (const SocketInfo& s : iface->getSockets()) { - if (receiver_fd_event_handler_->readReady(s.sockfd_)) { + if (receiver_fd_event_handler_->readReady(s.sockfd_) || + receiver_fd_event_handler_->hasError(s.sockfd_)) { receiveDHCP6Packet(s); // Can take time so check one more time the watch socket. if (dhcp_receiver_->shouldTerminate()) { return; } } else { - handleIfaceSocketError(receiver_fd_event_handler_, s); + handleIfaceSocketError(s); } } } diff --git a/src/lib/dhcp/iface_mgr.h b/src/lib/dhcp/iface_mgr.h index a306088ab6..388f21785f 100644 --- a/src/lib/dhcp/iface_mgr.h +++ b/src/lib/dhcp/iface_mgr.h @@ -1632,10 +1632,11 @@ private: /// @brief Handle interface socket error. /// - /// @param fd_event_handler The fd event handler. /// @param s The interface socket info. - void handleIfaceSocketError(util::FDEventHandlerPtr fd_event_handler, - const SocketInfo& s); + void handleIfaceSocketError(const SocketInfo& s); + + /// @brief Handle closed external sockets. + void handleClosedExternalSockets(); /// Holds instance of a class derived from PktFilter, used by the /// IfaceMgr to open sockets and send/receive packets through these