[3315] Changes after review:

 - SessionCallback renamed to SocketCallback
 - Removed references in comments to session
 - removed unused names string in receive{4,6}
 - extended unit-test to check that unregistered callback is not
   triggered
 - Implemented similar tests for receive6()
 - Fixed a bug that was revealed by those new unit-tests :)

src/lib/dhcp/iface_mgr.cc

@@ -227,7 +227,7 @@ IfaceMgr::isDirectResponseSupported() const {
 }
 
 void
-IfaceMgr::addExternalSocket(int socketfd, SessionCallback callback) {
+IfaceMgr::addExternalSocket(int socketfd, SocketCallback callback) {
     for (SocketCallbackContainer::iterator s = callbacks_.begin();
          s != callbacks_.end(); ++s) {
 
@@ -240,7 +240,7 @@ IfaceMgr::addExternalSocket(int socketfd, SessionCallback callback) {
     }
 
     // Add a new entry to the callbacks vector
-    SocketCallback x;
+    SocketCallbackInfo x;
     x.socket_ = socketfd;
     x.callback_ = callback;
     callbacks_.push_back(x);
@@ -819,7 +819,6 @@ IfaceMgr::receive4(uint32_t timeout_sec, uint32_t timeout_usec /* = 0 */) {
     IfaceCollection::const_iterator iface;
     fd_set sockets;
     int maxfd = 0;
-    stringstream names;
 
     FD_ZERO(&sockets);
 
@@ -834,7 +833,6 @@ IfaceMgr::receive4(uint32_t timeout_sec, uint32_t timeout_usec /* = 0 */) {
 
             // Only deal with IPv4 addresses.
             if (s->addr_.isV4()) {
-                names << s->sockfd_ << "(" << iface->getName() << ") ";
 
                 // Add this socket to listening set
                 FD_SET(s->sockfd_, &sockets);
@@ -845,7 +843,7 @@ IfaceMgr::receive4(uint32_t timeout_sec, uint32_t timeout_usec /* = 0 */) {
         }
     }
 
-    // if there are any session sockets registered...
+    // if there are any callbacks for external sockets registered...
     if (!callbacks_.empty()) {
         for (SocketCallbackContainer::const_iterator s = callbacks_.begin();
              s != callbacks_.end(); ++s) {
@@ -853,7 +851,6 @@ IfaceMgr::receive4(uint32_t timeout_sec, uint32_t timeout_usec /* = 0 */) {
             if (maxfd < s->socket_) {
                 maxfd = s->socket_;
             }
-            names << s->socket_ << "(session)";
         }
     }
 
@@ -877,13 +874,11 @@ IfaceMgr::receive4(uint32_t timeout_sec, uint32_t timeout_usec /* = 0 */) {
             continue;
         }
 
-        // something received over session socket
+        // something received over external socket
 
-        // in theory we could call io_service.run_one() here, instead of
-        // implementing callback mechanism, but that would introduce
-        // asiolink dependency to libdhcp++ and that is something we want
-        // to avoid (see CPE market and out long term plans for minimalistic
-        // implementations.
+        // Calling the external socket's callback provides its service
+        // layer access without integrating any specific features
+        // in IfaceMgr
         if (s->callback_) {
             s->callback_();
         }
@@ -925,7 +920,6 @@ Pkt6Ptr IfaceMgr::receive6(uint32_t timeout_sec, uint32_t timeout_usec /* = 0 */
     const SocketInfo* candidate = 0;
     fd_set sockets;
     int maxfd = 0;
-    stringstream names;
 
     FD_ZERO(&sockets);
 
@@ -940,7 +934,6 @@ Pkt6Ptr IfaceMgr::receive6(uint32_t timeout_sec, uint32_t timeout_usec /* = 0 */
 
             // Only deal with IPv6 addresses.
             if (s->addr_.isV6()) {
-                names << s->sockfd_ << "(" << iface->getName() << ") ";
 
                 // Add this socket to listening set
                 FD_SET(s->sockfd_, &sockets);
@@ -951,7 +944,7 @@ Pkt6Ptr IfaceMgr::receive6(uint32_t timeout_sec, uint32_t timeout_usec /* = 0 */
         }
     }
 
-    // if there are any session sockets registered...
+    // if there are any callbacks for external sockets registered...
     if (!callbacks_.empty()) {
         for (SocketCallbackContainer::const_iterator s = callbacks_.begin();
              s != callbacks_.end(); ++s) {
@@ -961,7 +954,6 @@ Pkt6Ptr IfaceMgr::receive6(uint32_t timeout_sec, uint32_t timeout_usec /* = 0 */
             if (maxfd < s->socket_) {
                 maxfd = s->socket_;
             }
-            names << s->socket_ << "(session)";
         }
     }
 
@@ -982,13 +974,11 @@ Pkt6Ptr IfaceMgr::receive6(uint32_t timeout_sec, uint32_t timeout_usec /* = 0 */
     for (SocketCallbackContainer::iterator s = callbacks_.begin();
          s != callbacks_.end(); ++s) {
         if (FD_ISSET(s->socket_, &sockets)) {
-            // something received over session socket
+            // something received over external socket
             if (s->callback_) {
-                // in theory we could call io_service.run_one() here, instead of
-                // implementing callback mechanism, but that would introduce
-                // asiolink dependency to libdhcp++ and that is something we want
-                // to avoid (see CPE market and out long term plans for minimalistic
-                // implementations.
+                // Calling the external socket's callback provides its service
+                // layer access without integrating any specific features
+                // in IfaceMgr
                 s->callback_();
             }
         }

src/lib/dhcp/iface_mgr.h

@@ -394,20 +394,20 @@ boost::function<void(const std::string& errmsg)> IfaceMgrErrorMsgCallback;
 ///
 class IfaceMgr : public boost::noncopyable {
 public:
-    /// Defines callback used when commands are received over control session.
-    typedef void (*SessionCallback) (void);
+    /// Defines callback used when data is received over external sockets.
+    typedef boost::function<void ()> SocketCallback;
 
     /// Keeps callback information for external sockets.
-    struct SocketCallback {
+    struct SocketCallbackInfo {
         /// Socket descriptor of the external socket.
         int socket_;
 
         /// A callback that will be called when data arrives over socket_.
-        SessionCallback callback_;
+        SocketCallback callback_;
     };
 
     /// Defines storage container for callbacks for external sockets
-    typedef std::list<SocketCallback> SocketCallbackContainer;
+    typedef std::list<SocketCallbackInfo> SocketCallbackContainer;
 
     /// @brief Packet reception buffer size
     ///
@@ -797,14 +797,14 @@ public:
     /// @return number of detected interfaces
     uint16_t countIfaces() { return ifaces_.size(); }
 
-    /// @brief Sets external socket and a callback
+    /// @brief Adds external socket and a callback
     ///
-    /// Specifies session socket and a callback that will be called
+    /// Specifies external socket and a callback that will be called
     /// when data will be received over that socket.
     ///
     /// @param socketfd socket descriptor
     /// @param callback callback function
-    void addExternalSocket(int socketfd, SessionCallback callback);
+    void addExternalSocket(int socketfd, SocketCallback callback);
 
     /// @brief Deletes external socket
 

src/lib/dhcp/tests/iface_mgr_unittest.cc

@@ -2601,9 +2601,9 @@ void my_callback2(void) {
     callback2_ok = true;
 }
 
-// Tests if a signle external socket and its callback can be passed and
+// Tests if a single external socket and its callback can be passed and
 // it is supported properly by receive4() method.
-TEST_F(IfaceMgrTest, SingleExternalSession) {
+TEST_F(IfaceMgrTest, SingleExternalSocket) {
 
     callback_ok = false;
 
@@ -2642,7 +2642,7 @@ TEST_F(IfaceMgrTest, SingleExternalSession) {
 
 // Tests if multiple external sockets and their callbacks can be passed and
 // it is supported properly by receive4() method.
-TEST_F(IfaceMgrTest, MiltipleControlSessions) {
+TEST_F(IfaceMgrTest, MiltipleExternalSockets) {
 
     callback_ok = false;
     callback2_ok = false;
@@ -2683,7 +2683,8 @@ TEST_F(IfaceMgrTest, MiltipleControlSessions) {
     EXPECT_FALSE(callback2_ok);
 
     // Read the data sent, because our test callbacks are too dumb to actually
-    // do it.
+    // do it. We don't care about the content read, because we're testing
+    // the callbacks, not pipes.
     char buf[80];
     read(pipefd[0], buf, 80);
 
@@ -2714,7 +2715,7 @@ TEST_F(IfaceMgrTest, MiltipleControlSessions) {
 
 // Tests if existing external socket can be deleted and that such deletion does
 // not affect any other existing sockets.
-TEST_F(IfaceMgrTest, DeleteControlSessions) {
+TEST_F(IfaceMgrTest, DeleteExternalSockets) {
 
     callback_ok = false;
     callback2_ok = false;
@@ -2748,6 +2749,18 @@ TEST_F(IfaceMgrTest, DeleteControlSessions) {
     EXPECT_FALSE(callback_ok);
     EXPECT_TRUE(callback2_ok);
 
+    // Let's reset the status
+    callback_ok = false;
+    callback2_ok = false;
+
+    // Now let's send something over the first callback that was unregistered.
+    // We should NOT receive any callback.
+    EXPECT_EQ(38, write(pipefd[1], "Hi, this is a message sent over a pipe", 38));
+
+    // Now check that the first callback is NOT called.
+    ASSERT_NO_THROW(pkt4 = ifacemgr->receive4(1));
+    EXPECT_FALSE(callback_ok);
+
     // close both pipe ends
     close(pipefd[1]);
     close(pipefd[0]);