[3251] Addressed review comments.

src/lib/dhcp/iface_mgr.cc

@@ -477,41 +477,41 @@ bool IfaceMgr::openSockets6(const uint16_t port) {
                 continue;
             }
 
-            sock = openSocket(iface->getName(), *addr, port);
+            // Open socket and join multicast group only if the interface
+            // is multicast-capable.
+            // @todo The DHCPv6 requires multicast so we may want to think
+            // whether we want to open the socket on a multicast-incapable
+            // interface or not. For now, we prefer to be liberal and allow
+            // it for some odd use cases which may utilize non-multicast
+            // interfaces. Perhaps a warning should be emitted if the
+            // interface is not a multicast one.
+            sock = openSocket(iface->getName(), *addr, port,
+                              iface->flag_multicast_);
             if (sock < 0) {
                 const char* errstr = strerror(errno);
-                isc_throw(SocketConfigError, "failed to open link-local socket on "
-                          << addr->toText() << " on interface "
+                isc_throw(SocketConfigError, "failed to open link-local"
+                          " socket on " << addr->toText() << " on interface "
                           << iface->getName() << ", reason: " << errstr);
             }
 
-            /*            // Binding socket to unicast address and then joining multicast group
-            // works well on Mac OS (and possibly other BSDs), but does not work
-            // on Linux.
-            if ( !PktFilter6::joinMulticast(sock, iface->getName(),
-                                            string(ALL_DHCP_RELAY_AGENTS_AND_SERVERS))) {
-                close(sock);
-                isc_throw(SocketConfigError, "Failed to join "
-                          << ALL_DHCP_RELAY_AGENTS_AND_SERVERS
-                          << " multicast group.");
-                          } */
-
             count++;
 
             /// @todo: Remove this ifdef once we start supporting BSD systems.
 #if defined(OS_LINUX)
             // To receive multicast traffic, Linux requires binding socket to
             // a multicast group. That in turn doesn't work on NetBSD.
-
-            int sock2 = openSocket(iface->getName(),
-                                   IOAddress(ALL_DHCP_RELAY_AGENTS_AND_SERVERS),
-                                   port);
-            if (sock2 < 0) {
-                const char* errstr = strerror(errno);
-                isc_throw(SocketConfigError, "Failed to open multicast socket on "
-                          << " interface " << iface->getFullName() << ", reason:"
-                          << errstr);
-                iface->delSocket(sock); // delete previously opened socket
+            if (iface->flag_multicast_) {
+                int sock2 =
+                    openSocket(iface->getName(),
+                               IOAddress(ALL_DHCP_RELAY_AGENTS_AND_SERVERS),
+                               port);
+                if (sock2 < 0) {
+                    const char* errstr = strerror(errno);
+                    isc_throw(SocketConfigError, "Failed to open multicast"
+                              " socket on interface " << iface->getFullName()
+                              << ", reason:" << errstr);
+                    iface->delSocket(sock); // delete previously opened socket
+                }
             }
 #endif
         }

src/lib/dhcp/iface_mgr.h

@@ -976,13 +976,17 @@ private:
     /// Holds instance of a class derived from PktFilter, used by the
     /// IfaceMgr to open sockets and send/receive packets through these
     /// sockets. It is possible to supply custom object using
-    /// setPacketFilter class. Various Packet Filters differ mainly by using
+    /// setPacketFilter method. Various Packet Filters differ mainly by using
     /// different types of sockets, e.g. SOCK_DGRAM,  SOCK_RAW and different
     /// families, e.g. AF_INET, AF_PACKET etc. Another possible type of
     /// Packet Filter is the one used for unit testing, which doesn't
     /// open sockets but rather mimics their behavior (mock object).
     PktFilterPtr packet_filter_;
 
+    /// Holds instance of a class derived from PktFilter6, used by the
+    /// IfaceMgr to manage sockets used to send and receive DHCPv6
+    /// messages. It is possible to supply a custom object using
+    /// setPacketFilter method.
     PktFilter6Ptr packet_filter6_;
 };
 

src/lib/dhcp/pkt_filter6.cc

@@ -22,6 +22,8 @@ PktFilter6::joinMulticast(int sock, const std::string& ifname,
                           const std::string & mcast) {
 
     struct ipv6_mreq mreq;
+    memset(&mreq, 0, sizeof(ipv6_mreq));
+
     // Convert the multicast address to a binary form.
     if (inet_pton(AF_INET6, mcast.c_str(), &mreq.ipv6mr_multiaddr) <= 0) {
         return (false);

src/lib/dhcp/pkt_filter6.h

@@ -65,16 +65,26 @@ class Iface;
 /// but simply create a pool of fake interfaces which configuration
 /// can be freely modified by a test. This however implies that operations
 /// on sockets must be simulated.
+///
+/// @note This class is named after @c PktFilter abstract class which exposes
+/// similar interface for DHVPv4. However, the PktFilter class is devoted to
+/// solve the problem of sending DHCPv4 messages to the hosts which don't have
+/// an IP address yet (a.k.a. direct DHCPv4 traffic). Where required, the
+/// custom implementations of @c PktFilter are provided to send and receive
+/// messages through raw sockets. In order to filter out the desired traffic
+/// Linux Packet Filtering or Berkeley Packet Filtering is used, hence the
+/// name of the class. In case of DHCPv6 regular IPv6/UDPv6 sockets are used
+/// and derived classes do not use Linux or Berkeley Packet Filtering.
 class PktFilter6 {
 public:
 
     /// @brief Virtual Destructor.
     virtual ~PktFilter6() { }
 
-    /// @brief Open a socket.
+    /// @brief Opens a socket.
     ///
     /// This function open an IPv6 socket on an interface and binds it to a
-    /// specified UDP port and IP address.
+    /// specified UDP port and IPv6 address.
     ///
     /// @param iface Interface descriptor.
     /// @param addr Address on the interface to be used to send packets.
@@ -89,7 +99,7 @@ public:
                                   const uint16_t port,
                                   const bool join_multicast) = 0;
 
-    /// @brief Receive DHCPv6 message on the interface.
+    /// @brief Receives DHCPv6 message on the interface.
     ///
     /// This function receives a single DHCPv6 message through using a socket
     /// open on a specified interface.
@@ -99,7 +109,7 @@ public:
     /// @return A pointer to received message.
     virtual Pkt6Ptr receive(const SocketInfo& socket_info) = 0;
 
-    /// @brief Send DHCPv6 message through a specified interface and socket.
+    /// @brief Sends DHCPv6 message through a specified interface and socket.
     ///
     /// This function sends a DHCPv6 message through a specified interface and
     /// socket. In general, there may be multiple sockets open on a single

src/lib/dhcp/pkt_filter_inet6.cc

@@ -47,7 +47,7 @@ PktFilterInet6::openSocket(const Iface& iface,
     addr6.sin6_len = sizeof(addr6);
 #endif
 
-    // TODO: use sockcreator once it becomes available
+    // @todo use sockcreator once it becomes available
 
     // make a socket
     int sock = socket(AF_INET6, SOCK_DGRAM, 0);
@@ -55,8 +55,7 @@ PktFilterInet6::openSocket(const Iface& iface,
         isc_throw(SocketConfigError, "Failed to create UDP6 socket.");
     }
 
-    // Set the REUSEADDR option so that we don't fail to start if
-    // we're being restarted.
+    // Set SO_REUSEADDR option.
     int flag = 1;
     if (setsockopt(sock, SOL_SOCKET, SO_REUSEADDR,
                    (char *)&flag, sizeof(flag)) < 0) {
@@ -85,18 +84,15 @@ PktFilterInet6::openSocket(const Iface& iface,
     }
 #endif
 
-    // multicast stuff
-    if (addr.getAddress().to_v6().is_multicast()) {
-        // both mcast (ALL_DHCP_RELAY_AGENTS_AND_SERVERS and ALL_DHCP_SERVERS)
-        // are link and site-scoped, so there is no sense to join those groups
-        // with global addresses.
-
-        if (join_multicast && !joinMulticast(sock, iface.getName(),
-                           std::string(ALL_DHCP_RELAY_AGENTS_AND_SERVERS))) {
-            close(sock);
-            isc_throw(SocketConfigError, "Failed to join " << ALL_DHCP_RELAY_AGENTS_AND_SERVERS
-                      << " multicast group.");
-        }
+    // Join All_DHCP_Relay_Agents_and_Servers multicast group if
+    // requested.
+    if (join_multicast &&
+        !joinMulticast(sock, iface.getName(),
+                       std::string(ALL_DHCP_RELAY_AGENTS_AND_SERVERS))) {
+        close(sock);
+        isc_throw(SocketConfigError, "Failed to join "
+                  << ALL_DHCP_RELAY_AGENTS_AND_SERVERS
+                  << " multicast group.");
     }
 
     return (SocketInfo(addr, port, sock));

src/lib/dhcp/pkt_filter_inet6.h

@@ -34,7 +34,7 @@ public:
     /// Initializes a control buffer used in the message transmission.
     PktFilterInet6();
 
-    /// @brief Open a socket.
+    /// @brief Opens a socket.
     ///
     /// This function open an IPv6 socket on an interface and binds it to a
     /// specified UDP port and IP address.
@@ -54,10 +54,13 @@ public:
                                   const uint16_t port,
                                   const bool join_multicast);
 
-    /// @brief Receive DHCPv6 message on the interface.
+    /// @brief Receives DHCPv6 message on the interface.
     ///
-    /// This function receives a single DHCPv6 message through using a socket
-    /// open on a specified interface.
+    /// This function receives a single DHCPv6 message through a socket
+    /// open on a specified interface. This function will block if there is
+    /// no message waiting on the specified socket. Therefore the @c IfaceMgr
+    /// must first check that there is any message on the socket (using
+    /// select function) prior to calling this function.
     ///
     /// @param socket_info A structure holding socket information.
     ///
@@ -66,7 +69,7 @@ public:
     /// reception.
     virtual Pkt6Ptr receive(const SocketInfo& socket_info);
 
-    /// @brief Send DHCPv6 message through a specified interface and socket.
+    /// @brief Sends DHCPv6 message through a specified interface and socket.
     ///
     /// Thie function sends a DHCPv6 message through a specified interface and
     /// socket. In general, there may be multiple sockets open on a single

src/lib/dhcp/pkt_filter_lpf.h

@@ -24,11 +24,10 @@ namespace dhcp {
 
 /// @brief Packet handling class using Linux Packet Filtering
 ///
-/// This class provides methods to send and recive packet using raw sockets
-/// and Linux Packet Filtering.
-///
-/// @warning This class is not implemented yet. Therefore all functions
-/// currently throw isc::NotImplemented exception.
+/// This class provides methods to send and recive DHCPv4 messages using raw
+/// sockets and Linux Packet Filtering. It is used by @c isc::dhcp::IfaceMgr
+/// to send DHCPv4 messages to the hosts which don't have an IPv4 address
+/// assigned yet.
 class PktFilterLPF : public PktFilter {
 public:
 

src/lib/dhcp/tests/iface_mgr_unittest.cc

@@ -158,7 +158,7 @@ public:
     /// @brief Returns the collection of existing interfaces.
     IfaceCollection& getIfacesLst() { return (ifaces_); }
 
-    /// @brief This function creates fictious interfaces with fictious
+    /// @brief This function creates fictitious interfaces with fictious
     /// addresses.
     ///
     /// These interfaces can be used in tests that don't actually try
@@ -195,6 +195,8 @@ public:
     /// - up always true
     /// - running always true
     /// - inactive always to false
+    /// - multicast always to true
+    /// - broadcast always to true
     ///
     /// If one needs to modify the default flag settings, the setIfaceFlags
     /// function should be used.
@@ -208,9 +210,12 @@ public:
         if (name == "lo") {
             iface.flag_loopback_ = true;
         }
+        iface.flag_multicast_ = true;
+        iface.flag_broadcast_ = true;
         iface.flag_up_ = true;
         iface.flag_running_ = true;
         iface.inactive4_ = false;
+        iface.inactive6_ = false;
         return (iface);
     }
 
@@ -288,7 +293,9 @@ public:
     ///
     /// @param iface An interface on which sockets are open.
     /// @param unicast_num A number of unicast addresses bound.
-    void checkSocketsCount6(const Iface& iface, const int unicast_num) {
+    /// @param link_local_num A number of link local addresses bound.
+    void checkSocketsCount6(const Iface& iface, const int unicast_num,
+                            const int link_local_num = 1) {
         // On local-loopback interface, there should be no sockets.
         if (iface.flag_loopback_) {
             ASSERT_TRUE(iface.getSockets().empty())
@@ -297,13 +304,16 @@ public:
             return;
         }
 #if defined OS_LINUX
-        // On Linux, there is an additional socket bound to ff02::1:2.
-        ASSERT_EQ(unicast_num + 2, iface.getSockets().size())
+        // On Linux, for each link-local address there may be an
+        // additional socket opened and bound to ff02::1:2. This socket
+        // is only opened if the interface is multicast-capable.
+        ASSERT_EQ(unicast_num + (iface.flag_multicast_ ? link_local_num : 0)
+                  + link_local_num, iface.getSockets().size())
             << "invalid number of sockets on interface "
             << iface.getName();
 #else
         // On non-Linux, there is no additional socket.
-        ASSERT_EQ(unicast_num + 1, iface.getSockets().size())
+        ASSERT_EQ(unicast_num + link_local_num, iface.getSockets().size())
             << "invalid number of sockets on interface "
             << iface.getName();
 
@@ -1505,6 +1515,92 @@ TEST_F(IfaceMgrTest, openSockets6LinkLocal) {
 #endif
 }
 
+// This test checks that socket is not open on the interface which doesn't
+// have a link-local address.
+TEST_F(IfaceMgrTest, openSockets6NoLinkLocal) {
+    NakedIfaceMgr ifacemgr;
+
+    // Remove all real interfaces and create a set of dummy interfaces.
+    ifacemgr.createIfaces();
+
+    boost::shared_ptr<PktFilter6Stub> filter(new PktFilter6Stub());
+    ASSERT_TRUE(filter);
+    ASSERT_NO_THROW(ifacemgr.setPacketFilter(filter));
+
+    // Remove a link local address from eth0. If there is no link-local
+    // address, the socket should not open.
+    ASSERT_TRUE(ifacemgr.getIface("eth0")->
+                delAddress(IOAddress("fe80::3a60:77ff:fed5:cdef")));
+
+    // Simulate opening sockets using the dummy packet filter.
+    bool success = false;
+    ASSERT_NO_THROW(success = ifacemgr.openSockets6(DHCP6_SERVER_PORT));
+    EXPECT_TRUE(success);
+
+    // Check that the number of sockets is correct on each interface.
+    checkSocketsCount6(*ifacemgr.getIface("lo"), 0);
+    // The thrid parameter specifies that the number of link-local
+    // addresses for eth0 is equal to 0.
+    checkSocketsCount6(*ifacemgr.getIface("eth0"), 0, 0);
+    checkSocketsCount6(*ifacemgr.getIface("eth1"), 0, 1);
+
+    // There should be no sockets open on eth0 because it neither has
+    // link-local nor global unicast addresses.
+    EXPECT_FALSE(ifacemgr.isBound("eth0", "fe80::3a60:77ff:fed5:cdef"));
+    EXPECT_FALSE(ifacemgr.isBound("eth0", "2001:db8:1::1"));
+    // Socket on eth1 should be bound to link local only.
+    EXPECT_TRUE(ifacemgr.isBound("eth1", "fe80::3a60:77ff:fed5:abcd"));
+
+    // If we are on Linux, there is one more socket bound to ff02::1:2
+#if defined OS_LINUX
+    EXPECT_FALSE(ifacemgr.isBound("eth0", ALL_DHCP_RELAY_AGENTS_AND_SERVERS));
+#endif
+
+}
+
+// This test checks that socket is open on the non-muticast-capable
+// interface. This socket simply doesn't join multicast group.
+TEST_F(IfaceMgrTest, openSockets6NotMulticast) {
+    NakedIfaceMgr ifacemgr;
+
+    // Remove all real interfaces and create a set of dummy interfaces.
+    ifacemgr.createIfaces();
+
+    boost::shared_ptr<PktFilter6Stub> filter(new PktFilter6Stub());
+    ASSERT_TRUE(filter);
+    ASSERT_NO_THROW(ifacemgr.setPacketFilter(filter));
+
+    // Make eth0 multicast-incapable.
+    ifacemgr.getIface("eth0")->flag_multicast_ = false;
+
+    // Simulate opening sockets using the dummy packet filter.
+    bool success = false;
+    ASSERT_NO_THROW(success = ifacemgr.openSockets6(DHCP6_SERVER_PORT));
+    EXPECT_TRUE(success);
+
+    // Check that the number of sockets is correct on each interface.
+    checkSocketsCount6(*ifacemgr.getIface("lo"), 0);
+    checkSocketsCount6(*ifacemgr.getIface("eth0"), 0);
+    checkSocketsCount6(*ifacemgr.getIface("eth1"), 0);
+
+    // Sockets on eth0 should be bound to link-local and should not be bound
+    // to global unicast address, even though this address is configured on
+    // the eth0.
+    EXPECT_TRUE(ifacemgr.isBound("eth0", "fe80::3a60:77ff:fed5:cdef"));
+    EXPECT_FALSE(ifacemgr.isBound("eth0", "2001:db8:1::1"));
+    // The eth0 is not a multicast-capable interface, so the socket should
+    // not be bound to the multicast address.
+    EXPECT_FALSE(ifacemgr.isBound("eth0", ALL_DHCP_RELAY_AGENTS_AND_SERVERS));
+    // Socket on eth1 should be bound to link local only.
+    EXPECT_TRUE(ifacemgr.isBound("eth1", "fe80::3a60:77ff:fed5:abcd"));
+
+    // If we are on Linux, there is one more socket bound to ff02::1:2
+    // on eth1.
+#if defined OS_LINUX
+    EXPECT_TRUE(ifacemgr.isBound("eth1", ALL_DHCP_RELAY_AGENTS_AND_SERVERS));
+#endif
+}
+
 // This test checks that the sockets are opened and bound to link local
 // and unicast addresses which have been explicitly specified.
 TEST_F(IfaceMgrTest, openSockets6Unicast) {
@@ -1546,6 +1642,52 @@ TEST_F(IfaceMgrTest, openSockets6Unicast) {
 
 }
 
+// This test checks that the socket is open and bound to a global unicast
+// address if the link-local address does not exist for the particular
+// interface.
+TEST_F(IfaceMgrTest, openSockets6UnicastOnly) {
+    NakedIfaceMgr ifacemgr;
+
+    // Remove all real interfaces and create a set of dummy interfaces.
+    ifacemgr.createIfaces();
+
+    boost::shared_ptr<PktFilter6Stub> filter(new PktFilter6Stub());
+    ASSERT_TRUE(filter);
+    ASSERT_NO_THROW(ifacemgr.setPacketFilter(filter));
+
+    // Configure the eth0 to open socket on the unicast address, apart
+    // from link-local address.
+    ifacemgr.getIface("eth0")->addUnicast(IOAddress("2001:db8:1::1"));
+    // Explicitly remove the link-local address from eth0.
+    ASSERT_TRUE(ifacemgr.getIface("eth0")->
+                delAddress(IOAddress("fe80::3a60:77ff:fed5:cdef")));
+
+    // Simulate opening sockets using the dummy packet filter.
+    bool success = false;
+    ASSERT_NO_THROW(success = ifacemgr.openSockets6(DHCP6_SERVER_PORT));
+    EXPECT_TRUE(success);
+
+    // Check that we have correct number of sockets on each interface.
+    checkSocketsCount6(*ifacemgr.getIface("lo"), 0);
+    checkSocketsCount6(*ifacemgr.getIface("eth0"), 1, 0);
+    checkSocketsCount6(*ifacemgr.getIface("eth1"), 0);
+
+    // The link-local address is not present on eth0. Therefore, no socket
+    // must be bound to this address, nor to multicast address.
+    EXPECT_FALSE(ifacemgr.isBound("eth0", "fe80::3a60:77ff:fed5:cdef"));
+    EXPECT_FALSE(ifacemgr.isBound("eth0", ALL_DHCP_RELAY_AGENTS_AND_SERVERS));
+    // There should be one socket bound to unicast address.
+    EXPECT_TRUE(ifacemgr.isBound("eth0", "2001:db8:1::1"));
+    // eth1 should have one socket, bound to link-local address.
+    EXPECT_TRUE(ifacemgr.isBound("eth1", "fe80::3a60:77ff:fed5:abcd"));
+
+    // If we are on Linux, there is one more socket bound to ff02::1:2
+#if defined OS_LINUX
+    EXPECT_TRUE(ifacemgr.isBound("eth1", ALL_DHCP_RELAY_AGENTS_AND_SERVERS));
+#endif
+
+}
+
 // This test checks that no sockets are open for the interface which is down.
 TEST_F(IfaceMgrTest, openSockets6IfaceDown) {
     NakedIfaceMgr ifacemgr;
@@ -1645,7 +1787,7 @@ TEST_F(IfaceMgrTest, openSockets6IfaceInactive) {
 
 }
 
-// That that the openSockets6 function does not throw if there are no interfaces
+// Test that the openSockets6 function does not throw if there are no interfaces
 // detected. This function is expected to return false.
 TEST_F(IfaceMgrTest, openSockets6NoIfaces) {
     NakedIfaceMgr ifacemgr;

src/lib/dhcp/tests/pkt_filter6_test_utils.h

@@ -81,7 +81,7 @@ public:
     /// @param rcvd_msg An instance of the message to be tested.
     void testRcvdMessage(const Pkt6Ptr& rcvd_msg) const;
 
-    std::string ifname_;   ///< Loopback interface name
+    std::string ifname_;   ///< Loopback interface name.
     uint16_t ifindex_;     ///< Loopback interface index.
     uint16_t port_;        ///< A port number used for the test.
     isc::dhcp::SocketInfo sock_info_; ///< A structure holding socket info.