[#4141] addressed review comments

diff --git a/src/lib/dhcp/iface_mgr.cc b/src/lib/dhcp/iface_mgr.cc
index 6ae7a51ff8c1e2087a490f1a3caa5451a2b81c90..a407398f46be01db253e8f713d4d0ac6aaf25a16 100644 (file)
--- 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<std::mutex> 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<std::mutex> 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<std::mutex> 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<std::mutex> 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 00cb6aa58215c7f6f3754df1e22ba73c4f09e291..d3f88b4031c84ec69993d17ff9e433c01f7c7964 100644 (file)
--- 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 9b9ab7eb3c566e168c223d4541a66c03748afb83..45e06c790a83ab4e1d433a4918a520f69f58fdf6 100644 (file)
--- 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 23bde2475786a3f7224f21c1840956a22a4d600e..e61b60547ea36a03ad39b1470ccf7d5478553f7d 100644 (file)
--- 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<int, struct epoll_event*> 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 0b38ee3025d569f0752c46d6bcae7de7f8dfc8c2..dfdbe92d3cce4807b0a4bac7cfb73bff2dc40c39 100644 (file)
--- 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 3eac6f2f5c09875ed7da8d1264a9c440aad2ce4a..e5893476e927b344edf19bc1dee65ca3d7c07321 100644 (file)
--- 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<int, struct kevent*> 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 bc33890dacbfc511128b16696a55195bd866ff52..531a4251a364ab678610328d0ac66638ede12220 100644 (file)
--- 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