[1959] Implemented first set of changes from the code review.

src/lib/dhcp/iface_mgr.cc

@@ -70,7 +70,6 @@ void
 IfaceMgr::Iface::closeSockets() {
     for (SocketCollection::iterator sock = sockets_.begin();
          sock != sockets_.end(); ++sock) {
-        cout << "Closing socket " << sock->sockfd_ << endl;
         close(sock->sockfd_);
     }
     sockets_.clear();

src/lib/dhcp/iface_mgr.h

@@ -89,7 +89,7 @@ public:
         /// @param ifindex interface index (unique integer identifier)
         Iface(const std::string& name, int ifindex);
 
-        /// @brief Closes all open sockets on interface
+        /// @brief Closes all open sockets on interface.
         void closeSockets();
 
         /// @brief Returns full interface name as "ifname/ifindex" string.
@@ -204,6 +204,8 @@ public:
         /// in this collection. Note that functions like
         /// @ref IfaceMgr::openSocketFromIface use
         /// @ref IfaceMgr::openSocket internally.
+        /// The returned reference is only valid during the lifetime of
+        /// the IfaceMgr object that returned it.
         ///
         /// @return collection of sockets added to interface
         const SocketCollection& getSockets() const { return sockets_; }

src/lib/dhcp/libdhcp++.cc

@@ -56,7 +56,7 @@ LibDHCP::optionFactory(Option::Universe u,
         isc_throw(BadValue, "invalid universe specified (expected "
                   "Option::V4 or Option::V6");
     }
-    return(it->second(u, type, buf));
+    return (it->second(u, type, buf));
 }
 
 

src/lib/dhcp/libdhcp++.h

@@ -45,7 +45,6 @@ public:
                                               uint16_t type,
                                               const OptionBuffer& buf);
 
-
     /// Builds collection of options.
     ///
     /// Builds raw (on-wire) data for provided collection of options.

src/lib/dhcp/option.h

@@ -99,7 +99,6 @@ public:
         return factory(u, type, OptionBuffer());
     }
 
-
     /// @brief ctor, used for options constructed, usually during transmission
     ///
     /// @param u option universe (DHCPv4 or DHCPv6)

src/lib/dhcp/tests/iface_mgr_unittest.cc

@@ -59,6 +59,7 @@ public:
 
     ~IfaceMgrTest() {
     }
+
 };
 
 // We need some known interface to work reliably. Loopback interface
@@ -217,6 +218,94 @@ TEST_F(IfaceMgrTest, getIface) {
 
 }
 
+TEST_F(IfaceMgrTest, multipleSockets) {
+    boost::scoped_ptr<NakedIfaceMgr> ifacemgr(new NakedIfaceMgr());
+
+    // container for initialized socket descriptors
+    std::list<uint16_t> init_sockets;
+
+    // create socket #1
+    int socket1 = 0;
+    ASSERT_NO_THROW(
+        socket1 = ifacemgr->openSocketFromIface(LOOPBACK, PORT1, AF_INET);
+    );
+    ASSERT_GT(socket1, 0);
+    init_sockets.push_back(socket1);
+
+    // create socket #2
+    IOAddress loAddr("127.0.0.1");
+    int socket2 = 0;
+    ASSERT_NO_THROW(
+        socket2 = ifacemgr->openSocketFromRemoteAddress(loAddr, PORT2);
+    );
+    ASSERT_GT(socket2, 0);
+    init_sockets.push_back(socket2);
+
+    // Get loopback interface. If we don't find one we are unable to run
+    // this test but we don't want to fail.
+    IfaceMgr::Iface* iface_ptr = ifacemgr->getIface(LOOPBACK);
+    if (iface_ptr == NULL) {
+        cout << "Local loopback interface not found. Skipping test. " << endl;
+        return;
+    }
+    // Once sockets have been sucessfully opened, they are supposed to
+    // be on the list. Here we start to test if all expected sockets
+    // are on the list and no other (unexpected) socket is there.
+    IfaceMgr::SocketCollection sockets = iface_ptr->getSockets();
+    int matched_sockets = 0;
+    for (std::list<uint16_t>::iterator init_sockets_it =
+             init_sockets.begin();
+         init_sockets_it != init_sockets.end(); ++init_sockets_it) {
+        // Set socket descriptors non blocking in order to be able
+        // to call recv() on them without hang.
+        int flags = fcntl(*init_sockets_it, F_GETFL, 0);
+        ASSERT_GE(flags, 0);
+        ASSERT_GE(fcntl(*init_sockets_it, F_SETFL, flags | O_NONBLOCK), 0);
+        // recv() is expected to result in EWOULDBLOCK error on non-blocking
+        // socket in case socket is valid but simply no data are coming in.
+        char buf;
+        recv(*init_sockets_it, &buf, 1, MSG_PEEK);
+        EXPECT_EQ(EWOULDBLOCK, errno);
+        // Apart from the ability to use the socket we want to make
+        // sure that socket on the list is the one that we created.
+        for (IfaceMgr::SocketCollection::const_iterator socket_it =
+                 sockets.begin(); socket_it != sockets.end(); ++socket_it) {
+            if (*init_sockets_it == socket_it->sockfd_) {
+                // This socket is the one that we created.
+                ++matched_sockets;
+                break;
+            }
+        }
+    }
+    // all created sockets have been matched if this condition works.
+    EXPECT_EQ(sockets.size(), matched_sockets);
+
+    // closeSockets() is the other function that we want to test. It
+    // is supposed to close all sockets so as we will not be able to use
+    // them anymore communication.
+    ifacemgr->closeSockets();
+
+    // closed sockets are supposed to be removed from the list
+    sockets = iface_ptr->getSockets();
+    ASSERT_EQ(0, sockets.size());
+
+    // We are still in posession of socket descriptors that we created
+    // on the beginning of this test. We can use them to check whether
+    // closeSockets() only removed them from the list or they have been
+    // really closed.
+    for (std::list<uint16_t>::const_iterator init_sockets_it =
+             init_sockets.begin();
+         init_sockets_it != init_sockets.end(); ++init_sockets_it) {
+        // recv() must result in error when using invalid socket.
+        char buf;
+        recv(*init_sockets_it, &buf, 1, MSG_PEEK);
+        // EWOULDBLOCK would mean that socket is valid/open but
+        // simply no data is received so we have to check for
+        // other errors.
+        EXPECT_NE(EWOULDBLOCK, errno);
+    }
+}
+
 TEST_F(IfaceMgrTest, sockets6) {
     // testing socket operation in a portable way is tricky
     // without interface detection implemented
@@ -317,6 +406,15 @@ TEST_F(IfaceMgrTest, socketsFromRemoteAddress) {
     );
     EXPECT_GT(socket2, 0);
     close(socket2);
+
+    // Open v4 socket to connect to broadcast address.
+    int socket3  = 0;
+    IOAddress bcastAddr("255.255.255.255");
+    EXPECT_NO_THROW(
+        socket3 = ifacemgr->openSocketFromRemoteAddress(bcastAddr, PORT2);
+    );
+    EXPECT_GT(socket3, 0);
+    close(socket3);
 }
 
 // TODO: disabled due to other naming on various systems

src/lib/dhcp/tests/libdhcp++_unittest.cc

@@ -87,21 +87,45 @@ TEST(LibDhcpTest, optionFactory) {
                                              DHO_SUBNET_MASK,
                                              buf);
     // Check if non-NULL DHO_SUBNET_MASK option pointer has been returned.
-    EXPECT_TRUE(opt_subnet_mask);
-
+    ASSERT_TRUE(opt_subnet_mask);
+    // Validate if type and universe is correct.
+    EXPECT_EQ(Option::V4, opt_subnet_mask->getUniverse());
+    EXPECT_EQ(DHO_SUBNET_MASK, opt_subnet_mask->getType());
+    // Expect that option does not have content..
+    EXPECT_EQ(0, opt_subnet_mask->len() - opt_subnet_mask->getHeaderLen());
+
+    // Fill the time offset buffer with 4 bytes of data. Each byte set to 1.
+    OptionBuffer time_offset_buf(4, 1);
     OptionPtr opt_time_offset;
     opt_time_offset = LibDHCP::optionFactory(Option::V4,
                                              DHO_TIME_OFFSET,
-                                             buf);
+                                             time_offset_buf);
     // Check if non-NULL DHO_TIME_OFFSET option pointer has been returned.
-    EXPECT_TRUE(opt_time_offset);
-
+    ASSERT_TRUE(opt_time_offset);
+    // Validate if option length, type and universe is correct.
+    EXPECT_EQ(Option::V4, opt_time_offset->getUniverse());
+    EXPECT_EQ(DHO_TIME_OFFSET, opt_time_offset->getType());
+    EXPECT_EQ(time_offset_buf.size(),
+              opt_time_offset->len() - opt_time_offset->getHeaderLen());
+    // Validate data in the option.
+    EXPECT_TRUE(std::equal(time_offset_buf.begin(), time_offset_buf.end(),
+                           opt_time_offset->getData().begin()));
+
+    // Fill the client id buffer with 20 bytes of data. Each byte set to 2.
+    OptionBuffer clientid_buf(20, 2);
     OptionPtr opt_clientid;
     opt_clientid = LibDHCP::optionFactory(Option::V6,
                                           D6O_CLIENTID,
-                                          buf);
+                                          clientid_buf);
     // Check if non-NULL D6O_CLIENTID option pointer has been returned.
-    EXPECT_TRUE(opt_clientid);
+    ASSERT_TRUE(opt_clientid);
+    // Validate if option length, type and universe is correct.
+    EXPECT_EQ(Option::V6, opt_clientid->getUniverse());
+    EXPECT_EQ(D6O_CLIENTID, opt_clientid->getType());
+    EXPECT_EQ(clientid_buf.size(), opt_clientid->len() - opt_clientid->getHeaderLen());
+    // Validate data in the option.
+    EXPECT_TRUE(std::equal(clientid_buf.begin(), clientid_buf.end(),
+                           opt_clientid->getData().begin()));
 }
 
 TEST(LibDhcpTest, packOptions6) {