From: Razvan Becheriu Date: Mon, 17 Nov 2025 10:30:55 +0000 (+0200) Subject: [#4141] addressed review comments X-Git-Tag: Kea-3.1.4~24 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=42d4c38a116265ff7eaf5c193385687aa2a150ec;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 6ae7a51ff8..a407398f46 100644 --- a/src/lib/dhcp/iface_mgr.cc +++ b/src/lib/dhcp/iface_mgr.cc @@ -337,6 +337,7 @@ IfaceMgr::addExternalSocket(int socketfd, SocketCallback callback) { // Update the callback and we're done if (s.socket_ == socketfd) { s.callback_ = callback; + s.unusable_ = false; return; } } @@ -1126,6 +1127,9 @@ Pkt4Ptr IfaceMgr::receive4Indirect(uint32_t timeout_sec, uint32_t timeout_usec / std::lock_guard lock(callbacks_mutex_); if (!callbacks_.empty()) { for (SocketCallbackInfo& s : callbacks_) { + if (s.unusable_) { + continue; + } errno = 0; if (fcntl(s.socket_, F_GETFD) < 0 && (errno == EBADF)) { s.unusable_ = true; @@ -1172,6 +1176,8 @@ Pkt4Ptr IfaceMgr::receive4Indirect(uint32_t timeout_sec, uint32_t timeout_usec / // signal or for some other reason. if (errno == EINTR) { isc_throw(SignalInterruptOnSelect, strerror(errno)); + } else if (errno == EBADF) { + isc_throw(SocketFDError, strerror(errno)); } else { isc_throw(SocketReadError, strerror(errno)); } @@ -1258,6 +1264,9 @@ Pkt4Ptr IfaceMgr::receive4Direct(uint32_t timeout_sec, uint32_t timeout_usec /* std::lock_guard lock(callbacks_mutex_); if (!callbacks_.empty()) { for (SocketCallbackInfo& s : callbacks_) { + if (s.unusable_) { + continue; + } errno = 0; if (fcntl(s.socket_, F_GETFD) < 0 && (errno == EBADF)) { s.unusable_ = true; @@ -1287,6 +1296,8 @@ Pkt4Ptr IfaceMgr::receive4Direct(uint32_t timeout_sec, uint32_t timeout_usec /* // signal or for some other reason. if (errno == EINTR) { isc_throw(SignalInterruptOnSelect, strerror(errno)); + } else if (errno == EBADF) { + isc_throw(SocketFDError, strerror(errno)); } else { isc_throw(SocketReadError, strerror(errno)); } @@ -1389,6 +1400,9 @@ IfaceMgr::receive6Direct(uint32_t timeout_sec, uint32_t timeout_usec /* = 0 */ ) std::lock_guard lock(callbacks_mutex_); if (!callbacks_.empty()) { for (SocketCallbackInfo& s : callbacks_) { + if (s.unusable_) { + continue; + } errno = 0; if (fcntl(s.socket_, F_GETFD) < 0 && (errno == EBADF)) { s.unusable_ = true; @@ -1418,6 +1432,8 @@ IfaceMgr::receive6Direct(uint32_t timeout_sec, uint32_t timeout_usec /* = 0 */ ) // signal or for some other reason. if (errno == EINTR) { isc_throw(SignalInterruptOnSelect, strerror(errno)); + } else if (errno == EBADF) { + isc_throw(SocketFDError, strerror(errno)); } else { isc_throw(SocketReadError, strerror(errno)); } @@ -1490,6 +1506,9 @@ IfaceMgr::receive6Indirect(uint32_t timeout_sec, uint32_t timeout_usec /* = 0 */ std::lock_guard lock(callbacks_mutex_); if (!callbacks_.empty()) { for (SocketCallbackInfo& s : callbacks_) { + if (s.unusable_) { + continue; + } errno = 0; if (fcntl(s.socket_, F_GETFD) < 0 && (errno == EBADF)) { s.unusable_ = true; @@ -1536,6 +1555,8 @@ IfaceMgr::receive6Indirect(uint32_t timeout_sec, uint32_t timeout_usec /* = 0 */ // signal or for some other reason. if (errno == EINTR) { isc_throw(SignalInterruptOnSelect, strerror(errno)); + } else if (errno == EBADF) { + isc_throw(SocketFDError, strerror(errno)); } else { isc_throw(SocketReadError, strerror(errno)); } diff --git a/src/lib/dhcp/iface_mgr.h b/src/lib/dhcp/iface_mgr.h index 00cb6aa582..d3f88b4031 100644 --- a/src/lib/dhcp/iface_mgr.h +++ b/src/lib/dhcp/iface_mgr.h @@ -96,6 +96,14 @@ public: isc::Exception(file, line, what) { } }; +/// @brief IfaceMgr exception thrown when there is an error detected for +/// the respective socket file descriptor. +class SocketFDError : public Exception { +public: + SocketFDError(const char* file, size_t line, const char* what) : + isc::Exception(file, line, what) { } +}; + /// @brief Represents a single network interface /// /// Iface structure represents network interface with all useful @@ -151,7 +159,7 @@ public: Iface(const std::string& name, unsigned int ifindex); /// @brief Destructor. - ~Iface() { } + ~Iface() = default; /// @brief Closes all open sockets on interface. void closeSockets(); @@ -196,13 +204,17 @@ public: /// @brief Returns MAC length. /// /// @return length of MAC address - size_t getMacLen() const { return mac_len_; } + size_t getMacLen() const { + return (mac_len_); + } /// @brief Returns pointer to MAC address. /// /// Note: Returned pointer is only valid as long as the interface object /// that returned it. - const uint8_t* getMac() const { return mac_; } + const uint8_t* getMac() const { + return (mac_); + } /// @brief Sets flag_*_ fields based on bitmask value returned by OS /// @@ -216,22 +228,30 @@ public: /// @brief Returns interface index. /// /// @return interface index - unsigned int getIndex() const { return ifindex_; } + unsigned int getIndex() const { + return (ifindex_); + } /// @brief Returns interface name. /// /// @return interface name - std::string getName() const { return name_; } + std::string getName() const { + return (name_); + } /// @brief Sets up hardware type of the interface. /// /// @param type hardware type - void setHWType(uint16_t type ) { hardware_type_ = type; } + void setHWType(uint16_t type ) { + hardware_type_ = type; + } /// @brief Returns hardware type of the interface. /// /// @return hardware type - uint16_t getHWType() const { return hardware_type_; } + uint16_t getHWType() const { + return (hardware_type_); + } /// @brief Returns all addresses available on an interface. /// @@ -251,7 +271,9 @@ public: /// mostly be useful for clients with wifi/vpn/virtual interfaces. /// /// @return collection of addresses - const AddressCollection& getAddresses() const { return addrs_; } + const AddressCollection& getAddresses() const { + return (addrs_); + } /// @brief Returns IPv4 address assigned to the interface. /// @@ -344,7 +366,9 @@ public: /// the IfaceMgr object that returned it. /// /// @return collection of sockets added to interface - const SocketCollection& getSockets() const { return sockets_; } + const SocketCollection& getSockets() const { + return (sockets_); + } /// @brief Removes any unicast addresses /// @@ -364,7 +388,7 @@ public: /// /// @return address collection (may be empty) const AddressCollection& getUnicasts() const { - return unicasts_; + return (unicasts_); } /// @brief Returns the pointer to the buffer used for data reading. @@ -807,7 +831,9 @@ public: /// main() function completes, you should not worry much about this. /// /// @return container with all interfaces. - const IfaceCollection& getIfaces() { return (ifaces_); } + const IfaceCollection& getIfaces() { + return (ifaces_); + } /// @brief Removes detected interfaces. /// @@ -1154,9 +1180,11 @@ public: /// @brief Returns number of detected interfaces. /// /// @return number of detected interfaces - uint16_t countIfaces() { return ifaces_.size(); } + uint16_t countIfaces() { + return (ifaces_.size()); + } - /// @brief Adds external socket and a callback + /// @brief Adds external socket and a callback. /// /// Specifies external socket and a callback that will be called /// when data will be received over that socket. @@ -1172,10 +1200,16 @@ public: /// @brief Deletes external socket /// + /// External sockets should be removed from IfaceMgr before being closed + /// by the external API. + /// /// @param socketfd socket descriptor void deleteExternalSocket(int socketfd); /// @brief Deletes all external sockets. + /// + /// External sockets should be removed from IfaceMgr before being closed + /// by the external API. void deleteAllExternalSockets(); /// @brief Set packet filter object to handle sending and receiving DHCPv4 diff --git a/src/lib/util/epoll_event_handler.cc b/src/lib/util/epoll_event_handler.cc index 9b9ab7eb3c..45e06c790a 100644 --- a/src/lib/util/epoll_event_handler.cc +++ b/src/lib/util/epoll_event_handler.cc @@ -23,7 +23,7 @@ EPollEventHandler::EPollEventHandler() : FDEventHandler(TYPE_EPOLL), epollfd_(-1 if (epollfd_ == -1) { isc_throw(Unexpected, "error opening epoll: " << strerror(errno)); } - if (pipe(pipefd_)) { + if (pipe(pipe_fd_)) { close(epollfd_); isc_throw(Unexpected, "error opening internal epoll pipe: " << strerror(errno)); } @@ -32,8 +32,8 @@ EPollEventHandler::EPollEventHandler() : FDEventHandler(TYPE_EPOLL), epollfd_(-1 EPollEventHandler::~EPollEventHandler() { close(epollfd_); - close(pipefd_[1]); - close(pipefd_[0]); + close(pipe_fd_[0]); + close(pipe_fd_[1]); } void EPollEventHandler::add(int fd) { @@ -74,7 +74,7 @@ int EPollEventHandler::waitEvent(uint32_t timeout_sec, uint32_t timeout_usec /* } struct epoll_event dummy; memset(&dummy, 0, sizeof(dummy)); - dummy.data.fd = pipefd_[0]; + dummy.data.fd = pipe_fd_[0]; dummy.events |= EPOLLIN; epoll_ctl(epollfd_, EPOLL_CTL_ADD, dummy.data.fd, &dummy); used_data_.push_back(dummy); diff --git a/src/lib/util/epoll_event_handler.h b/src/lib/util/epoll_event_handler.h index 23bde24757..e61b60547e 100644 --- a/src/lib/util/epoll_event_handler.h +++ b/src/lib/util/epoll_event_handler.h @@ -70,9 +70,9 @@ private: /// @brief The map with file descriptor to data reference. std::unordered_map map_; - /// @brief The pipe used to permit calling @ref waitEvent with no - /// registered file descriptors. - int pipefd_[2]; + /// @brief The pipe used to permit calling @ref waitEvent + /// with no registered file descriptors. + int pipe_fd_[2]; }; } // namespace isc::util; diff --git a/src/lib/util/kqueue_event_handler.cc b/src/lib/util/kqueue_event_handler.cc index 0b38ee3025..dfdbe92d3c 100644 --- a/src/lib/util/kqueue_event_handler.cc +++ b/src/lib/util/kqueue_event_handler.cc @@ -23,7 +23,7 @@ KQueueEventHandler::KQueueEventHandler() : FDEventHandler(TYPE_KQUEUE), kqueuefd if (kqueuefd_ == -1) { isc_throw(Unexpected, "error opening kqueue: " << strerror(errno)); } - if (pipe(pipefd_)) { + if (pipe(pipe_fd_)) { close(kqueuefd_); isc_throw(Unexpected, "error opening internal kqueue pipe: " << strerror(errno)); } @@ -32,8 +32,8 @@ KQueueEventHandler::KQueueEventHandler() : FDEventHandler(TYPE_KQUEUE), kqueuefd KQueueEventHandler::~KQueueEventHandler() { close(kqueuefd_); - close(pipefd_[1]); - close(pipefd_[0]); + close(pipe_fd_[0]); + close(pipe_fd_[1]); } void KQueueEventHandler::add(int fd) { @@ -76,12 +76,13 @@ int KQueueEventHandler::waitEvent(uint32_t timeout_sec, uint32_t timeout_usec /* } struct kevent dummy; memset(&dummy, 0, sizeof(dummy)); - EV_SET(&dummy, pipefd_[0], EVFILT_READ, EV_ADD, 0, 0, 0); + EV_SET(&dummy, pipe_fd_[0], EVFILT_READ, EV_ADD, 0, 0, 0); kevent(kqueuefd_, &dummy, 1, 0, 0, 0); used_data_.push_back(dummy); int result = 0; if (errors_.empty()) { - result = kevent(kqueuefd_, 0, 0, used_data_.data(), used_data_.size(), select_timeout_p); + result = kevent(kqueuefd_, 0, 0, used_data_.data(), used_data_.size(), + select_timeout_p); for (int i = 0; i < result; ++i) { map_.emplace(used_data_[i].ident, &used_data_[i]); } diff --git a/src/lib/util/kqueue_event_handler.h b/src/lib/util/kqueue_event_handler.h index 3eac6f2f5c..e5893476e9 100644 --- a/src/lib/util/kqueue_event_handler.h +++ b/src/lib/util/kqueue_event_handler.h @@ -72,9 +72,9 @@ private: /// @brief The map with file descriptor to data reference. std::unordered_multimap map_; - /// @brief The pipe used to permit calling @ref waitEvent with no - /// registered file descriptors. - int pipefd_[2]; + /// @brief The pipe used to permit calling @ref waitEvent + /// with no registered file descriptors. + int pipe_fd_[2]; }; } // namespace isc::util; diff --git a/src/lib/util/tests/fd_event_handler_unittests.h b/src/lib/util/tests/fd_event_handler_unittests.h index bc33890dac..531a4251a3 100644 --- a/src/lib/util/tests/fd_event_handler_unittests.h +++ b/src/lib/util/tests/fd_event_handler_unittests.h @@ -41,20 +41,20 @@ public: /// @brief Constructor. FDEventHandlerTest() { handler_.reset(new FDEventHandlerType); - pipe(pipefd_); + pipe(pipe_fd_); } /// @brief Destructor. ~FDEventHandlerTest() { - close(pipefd_[1]); - close(pipefd_[0]); + close(pipe_fd_[1]); + close(pipe_fd_[0]); } /// @brief The tested fd event handler. FDEventHandlerPtr handler_; /// @brief The pipe used for testing read and write operations. - int pipefd_[2]; + int pipe_fd_[2]; }; TEST_F(FDEventHandlerTest, events) { @@ -64,45 +64,45 @@ TEST_F(FDEventHandlerTest, events) { EXPECT_THROW(handler_->add(-1), BadValue); - EXPECT_NO_THROW(handler_->add(pipefd_[0])); + EXPECT_NO_THROW(handler_->add(pipe_fd_[0])); - EXPECT_FALSE(handler_->readReady(pipefd_[0])); - EXPECT_FALSE(handler_->readReady(pipefd_[1])); + EXPECT_FALSE(handler_->readReady(pipe_fd_[0])); + EXPECT_FALSE(handler_->readReady(pipe_fd_[1])); EXPECT_EQ(0, handler_->waitEvent(0, 1000)); - EXPECT_FALSE(handler_->readReady(pipefd_[0])); - EXPECT_FALSE(handler_->readReady(pipefd_[1])); + EXPECT_FALSE(handler_->readReady(pipe_fd_[0])); + EXPECT_FALSE(handler_->readReady(pipe_fd_[1])); - EXPECT_EQ(1, write(pipefd_[1], &MARKER, sizeof(MARKER))); + EXPECT_EQ(1, write(pipe_fd_[1], &MARKER, sizeof(MARKER))); EXPECT_EQ(1, handler_->waitEvent(0, 1000)); - EXPECT_TRUE(handler_->readReady(pipefd_[0])); - EXPECT_FALSE(handler_->readReady(pipefd_[1])); + EXPECT_TRUE(handler_->readReady(pipe_fd_[0])); + EXPECT_FALSE(handler_->readReady(pipe_fd_[1])); - EXPECT_EQ(1, write(pipefd_[1], &MARKER, sizeof(MARKER))); + EXPECT_EQ(1, write(pipe_fd_[1], &MARKER, sizeof(MARKER))); EXPECT_EQ(1, handler_->waitEvent(0, 1000)); - EXPECT_TRUE(handler_->readReady(pipefd_[0])); - EXPECT_FALSE(handler_->readReady(pipefd_[1])); + EXPECT_TRUE(handler_->readReady(pipe_fd_[0])); + EXPECT_FALSE(handler_->readReady(pipe_fd_[1])); unsigned char data; - EXPECT_EQ(1, read(pipefd_[0], &data, sizeof(data))); + EXPECT_EQ(1, read(pipe_fd_[0], &data, sizeof(data))); EXPECT_EQ(1, handler_->waitEvent(0, 1000)); - EXPECT_TRUE(handler_->readReady(pipefd_[0])); - EXPECT_FALSE(handler_->readReady(pipefd_[1])); + EXPECT_TRUE(handler_->readReady(pipe_fd_[0])); + EXPECT_FALSE(handler_->readReady(pipe_fd_[1])); - EXPECT_EQ(1, read(pipefd_[0], &data, sizeof(data))); + EXPECT_EQ(1, read(pipe_fd_[0], &data, sizeof(data))); EXPECT_EQ(0, handler_->waitEvent(0, 1000)); - EXPECT_FALSE(handler_->readReady(pipefd_[0])); - EXPECT_FALSE(handler_->readReady(pipefd_[1])); + EXPECT_FALSE(handler_->readReady(pipe_fd_[0])); + EXPECT_FALSE(handler_->readReady(pipe_fd_[1])); EXPECT_NO_THROW(handler_->clear()); @@ -137,15 +137,9 @@ TEST_F(FDEventHandlerTest, events) { TEST_F(FDEventHandlerTest, badFD) { errno = 0; - int fd; - - if (handler_->type() != FDEventHandler::TYPE_SELECT) { - // epoll does not allow add of /dev/zero to registered events. - fd = pipefd_[0]; - EXPECT_EQ(1, write(pipefd_[1], &MARKER, sizeof(MARKER))); - } else { - fd = open("/dev/zero", O_RDONLY, 0); - } + int fd = pipe_fd_[0]; + // epoll does not allow add of /dev/zero to registered events. + EXPECT_EQ(1, write(pipe_fd_[1], &MARKER, sizeof(MARKER))); ASSERT_GE(fd, 0); @@ -182,10 +176,17 @@ TEST_F(FDEventHandlerTest, badFD) { EXPECT_EQ(EBADF, errno); } - if (handler_->type() != FDEventHandler::TYPE_SELECT) { - close(pipefd_[1]); - pipe(pipefd_); - } + close(pipe_fd_[1]); + pipe(pipe_fd_); +} + +TEST_F(FDEventHandlerTest, hup) { + EXPECT_NO_THROW(handler_->add(pipe_fd_[0])); + close(pipe_fd_[1]); + EXPECT_EQ(1, handler_->waitEvent(0, 1000)); + EXPECT_TRUE(handler_->readReady(pipe_fd_[0])); + close(pipe_fd_[0]); + pipe(pipe_fd_); } } // end of anonymous namespace