[#4141] addressed review comments

diff --git a/src/lib/dhcp/iface_mgr.cc b/src/lib/dhcp/iface_mgr.cc
index 17d71e21543ba5114adeed7ad78ca1c6c198fbd0..778897b5f364faa606cc2cdabd67f46025d69b26 100644 (file)
--- 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<std::mutex> 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<std::mutex> 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<std::mutex> 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<std::mutex> 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 ebfb8364cb4a1b715f877d27a73da4cd76e84d85..cc2f2617fc887bab0a72d54278909d7a13d2aff7 100644 (file)
--- 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 b5cc5927a6403cc2a949a61ec287274bca2a0b58..047846e1f3c143031ce0a5adafc72f7de4dbfaf4 100644 (file)
--- 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