[3195] Changes after review:

 - Guide updated
 - Iface::addUnicast() now refeses duplicates
 - addUnicast and clearUnicasts are now commented
 - new unit-tests
 - argument for addActiveIface is const reference again
 -

doc/guide/bind10-guide.xml

@@ -4696,6 +4696,12 @@ Dhcp6/subnet6/	list
         on the Dhcp6/interface list. It is not possible to specify more than one
         unicast address on a given interface.
       </para>
+      <para>
+        Care should be taken to specify proper unicast addresses. The server will
+        attempt to bind to those addresses specified, without any additional checks.
+        That approach is selected on purpose, so in the software can be used to
+        communicate over uncommon addresses if the administrator desires so.
+      </para>
     </section>
 
     <section>

src/lib/dhcp/iface_mgr.cc

@@ -177,6 +177,17 @@ IfaceMgr::IfaceMgr()
     }
 }
 
+void Iface::addUnicast(const isc::asiolink::IOAddress& addr) {
+    for (Iface::AddressCollection::const_iterator i = unicasts_.begin();
+         i != unicasts_.end(); ++i) {
+        if (*i == addr) {
+            isc_throw(BadValue, "Address " << addr.toText()
+                      << " already defined on the " << name_ << " interface.");
+        }
+    }
+    unicasts_.push_back(addr);
+}
+
 void IfaceMgr::closeSockets() {
     for (IfaceCollection::iterator iface = ifaces_.begin();
          iface != ifaces_.end(); ++iface) {
@@ -343,8 +354,10 @@ bool IfaceMgr::openSockets4(const uint16_t port, const bool use_bcast) {
 
             }
             if (sock < 0) {
+                const char* errstr = strerror(errno);
                 isc_throw(SocketConfigError, "failed to open IPv4 socket"
-                          << " supporting broadcast traffic");
+                          << " supporting broadcast traffic, reason:"
+                          << errstr);
             }
 
             count++;
@@ -375,8 +388,10 @@ bool IfaceMgr::openSockets6(const uint16_t port) {
 
             sock = openSocket(iface->getName(), *addr, port);
             if (sock < 0) {
+                const char* errstr = strerror(errno);
                 isc_throw(SocketConfigError, "failed to open unicast socket on "
-                          << addr->toText() << " on interface " << iface->getName());
+                          << addr->toText() << " on interface " << iface->getName()
+                          << ", reason: " << errstr);
             }
 
             count++;
@@ -404,9 +419,10 @@ bool IfaceMgr::openSockets6(const uint16_t port) {
 
             sock = openSocket(iface->getName(), *addr, port);
             if (sock < 0) {
+                const char* errstr = strerror(errno);
                 isc_throw(SocketConfigError, "failed to open link-local socket on "
                           << addr->toText() << " on interface "
-                          << iface->getName());
+                          << iface->getName() << ", reason: " << errstr);
             }
 
             // Binding socket to unicast address and then joining multicast group
@@ -431,8 +447,10 @@ bool IfaceMgr::openSockets6(const uint16_t port) {
                                    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());
+                          << " interface " << iface->getFullName() << ", reason:"
+                          << errstr);
                 iface->delSocket(sock); // delete previously opened socket
             }
 #endif
@@ -625,7 +643,9 @@ IfaceMgr::getLocalAddress(const IOAddress& remote_addr, const uint16_t port) {
         // interface.
         sock.open(asio::ip::udp::v4(), err_code);
         if (err_code) {
-            isc_throw(Unexpected, "failed to open UDPv4 socket");
+            const char* errstr = strerror(errno);
+            isc_throw(Unexpected, "failed to open UDPv4 socket, reason:"
+                      << errstr);
         }
         sock.set_option(asio::socket_base::broadcast(true), err_code);
         if (err_code) {
@@ -1190,7 +1210,7 @@ uint16_t IfaceMgr::getSocket(const isc::dhcp::Pkt6& pkt) {
             if ( (pkt.getRemoteAddr().getAddress().to_v6().is_link_local() &&
                 s->addr_.getAddress().to_v6().is_link_local()) ||
                  (!pkt.getRemoteAddr().getAddress().to_v6().is_link_local() &&
-                  s->addr_.getAddress().to_v6().is_link_local()) ) {
+                  !s->addr_.getAddress().to_v6().is_link_local()) ) {
                 candidate = s;
             }
         }
@@ -1226,5 +1246,6 @@ uint16_t IfaceMgr::getSocket(isc::dhcp::Pkt4 const& pkt) {
               << " does not have any suitable IPv4 sockets open.");
 }
 
+
 } // end of namespace isc::dhcp
 } // end of namespace isc

src/lib/dhcp/iface_mgr.h

@@ -264,14 +264,23 @@ public:
     /// @return collection of sockets added to interface
     const SocketCollection& getSockets() const { return sockets_; }
 
+    /// @brief Removes any unicast addresses
+    ///
+    /// Removes any unicast addresses that the server was configured to
+    /// listen on
     void clearUnicasts() {
         unicasts_.clear();
     }
 
-    void addUnicast(const isc::asiolink::IOAddress& addr) {
-        unicasts_.push_back(addr);
-    }
+    /// @brief Adds unicast the server should listen on
+    ///
+    /// @throw BadValue if specified address is already defined on interface
+    /// @param addr unicast address to listen on
+    void addUnicast(const isc::asiolink::IOAddress& addr);
 
+    /// @brief Returns a container of addresses the server should listen on
+    ///
+    /// @return address collection (may be empty)
     const AddressCollection& getUnicasts() const {
         return unicasts_;
     }

src/lib/dhcp/tests/iface_mgr_unittest.cc

@@ -802,6 +802,7 @@ TEST_F(IfaceMgrTest, sendReceive6) {
     // try to send/receive data over the closed socket. Closed socket's descriptor is
     // still being hold by IfaceMgr which will try to use it to receive data.
     close(socket1);
+    close(socket2);
     EXPECT_THROW(ifacemgr->receive6(10), SocketReadError);
     EXPECT_THROW(ifacemgr->send(sendPkt), SocketWriteError);
 }
@@ -1579,6 +1580,23 @@ TEST_F(IfaceMgrTest, DISABLED_openUnicastSockets) {
     EXPECT_TRUE(getSocketByAddr(sockets, IOAddress("figure-out-link-local-addr")));
 }
 
+// Checks if there is a protection against unicast duplicates.
+TEST_F(IfaceMgrTest, unicastDuplicates) {
+    NakedIfaceMgr ifacemgr;
+
+    Iface* iface = ifacemgr.getIface(LOOPBACK);
+    if (iface == NULL) {
+        cout << "Local loopback interface not found. Skipping test. " << endl;
+        return;
+    }
+
+    // Tell the interface that it should bind to this global interface
+    EXPECT_NO_THROW(iface->addUnicast(IOAddress("2001:db8::1")));
+
+    // Tell the interface that it should bind to this global interface
+    EXPECT_THROW(iface->addUnicast(IOAddress("2001:db8::1")), BadValue);
+}
+
 // This test requires addresses 2001:db8:15c::1/128 and fe80::1/64 to be
 // configured on loopback interface
 //

src/lib/dhcpsrv/cfgmgr.cc

@@ -269,16 +269,17 @@ std::string CfgMgr::getDataDir() {
 }
 
 void
-CfgMgr::addActiveIface(std::string iface) {
+CfgMgr::addActiveIface(const std::string& iface) {
 
     size_t pos = iface.find("/");
+    std::string iface_copy = iface;
 
     if (pos != std::string::npos) {
         std::string addr_string = iface.substr(pos + 1);
         try {
             IOAddress addr(addr_string);
-            iface = iface.substr(0,pos);
-            unicast_addrs_.insert(make_pair(iface, addr));
+            iface_copy = iface.substr(0,pos);
+            unicast_addrs_.insert(make_pair(iface_copy, addr));
         } catch (...) {
             isc_throw(BadValue, "Can't convert '" << addr_string
                       << "' into address in interface defition ('"
@@ -286,14 +287,14 @@ CfgMgr::addActiveIface(std::string iface) {
         }
     }
 
-    if (isIfaceListedActive(iface)) {
+    if (isIfaceListedActive(iface_copy)) {
         isc_throw(DuplicateListeningIface,
-                  "attempt to add duplicate interface '" << iface << "'"
+                  "attempt to add duplicate interface '" << iface_copy << "'"
                   " to the set of interfaces on which server listens");
     }
     LOG_DEBUG(dhcpsrv_logger, DHCPSRV_DBG_TRACE, DHCPSRV_CFGMGR_ADD_IFACE)
-        .arg(iface);
-    active_ifaces_.push_back(iface);
+        .arg(iface_copy);
+    active_ifaces_.push_back(iface_copy);
 }
 
 void

src/lib/dhcpsrv/cfgmgr.h

@@ -272,7 +272,7 @@ public:
     /// server should listen.
     ///
     /// @param iface A name of the interface being added to the listening set.
-    void addActiveIface(std::string iface);
+    void addActiveIface(const std::string& iface);
 
     /// @brief Sets the flag which indicates that server is supposed to listen
     /// on all available interfaces.
@@ -308,7 +308,9 @@ public:
     /// @brief returns unicast a given interface should listen on (or NULL)
     ///
     /// This method will return an address for a specified interface, if the
-    /// server is supposed to listen on.
+    /// server is supposed to listen on unicast address. This address is
+    /// intended to be used immediately. This pointer is valid only until
+    /// the next configuration change.
     ///
     /// @return IOAddress pointer (or NULL if none)
     const isc::asiolink::IOAddress*

src/lib/dhcpsrv/tests/cfgmgr_unittest.cc

@@ -579,14 +579,14 @@ TEST_F(CfgMgrTest, optionSpace6) {
 TEST_F(CfgMgrTest, addActiveIface) {
     CfgMgr& cfg_mgr = CfgMgr::instance();
 
-    cfg_mgr.addActiveIface("eth0");
-    cfg_mgr.addActiveIface("eth1");
+    EXPECT_NO_THROW(cfg_mgr.addActiveIface("eth0"));
+    EXPECT_NO_THROW(cfg_mgr.addActiveIface("eth1"));
 
     EXPECT_TRUE(cfg_mgr.isActiveIface("eth0"));
     EXPECT_TRUE(cfg_mgr.isActiveIface("eth1"));
     EXPECT_FALSE(cfg_mgr.isActiveIface("eth2"));
 
-    cfg_mgr.deleteActiveIfaces();
+    EXPECT_NO_THROW(cfg_mgr.deleteActiveIfaces());
 
     EXPECT_FALSE(cfg_mgr.isActiveIface("eth0"));
     EXPECT_FALSE(cfg_mgr.isActiveIface("eth1"));
@@ -599,9 +599,9 @@ TEST_F(CfgMgrTest, addActiveIface) {
 TEST_F(CfgMgrTest, addUnicastAddresses) {
     CfgMgr& cfg_mgr = CfgMgr::instance();
 
-    cfg_mgr.addActiveIface("eth1/2001:db8::1");
-    cfg_mgr.addActiveIface("eth2/2001:db8::2");
-    cfg_mgr.addActiveIface("eth3");
+    EXPECT_NO_THROW(cfg_mgr.addActiveIface("eth1/2001:db8::1"));
+    EXPECT_NO_THROW(cfg_mgr.addActiveIface("eth2/2001:db8::2"));
+    EXPECT_NO_THROW(cfg_mgr.addActiveIface("eth3"));
 
     EXPECT_TRUE(cfg_mgr.isActiveIface("eth1"));
     EXPECT_TRUE(cfg_mgr.isActiveIface("eth2"));
@@ -614,7 +614,7 @@ TEST_F(CfgMgrTest, addUnicastAddresses) {
     EXPECT_FALSE(cfg_mgr.getUnicast("eth3"));
     EXPECT_FALSE(cfg_mgr.getUnicast("eth4"));
 
-    cfg_mgr.deleteActiveIfaces();
+    EXPECT_NO_THROW(cfg_mgr.deleteActiveIfaces());
 
     EXPECT_FALSE(cfg_mgr.isActiveIface("eth1"));
     EXPECT_FALSE(cfg_mgr.isActiveIface("eth2"));
@@ -653,6 +653,27 @@ TEST_F(CfgMgrTest, activateAllIfaces) {
     EXPECT_FALSE(cfg_mgr.isActiveIface("eth2"));
 }
 
+/// @todo Add unit-tests for testing:
+/// - addActiveIface() with invalid interface name
+/// - addActiveIface() with the same interface twice
+/// - addActiveIface() with a bogus address
+///
+/// This is somewhat tricky. Care should be taken here, because it is rather
+/// difficult to decide if interface name is valid or not. Some servers, e.g.
+/// dibbler, allow to specify interface names that are not currently present in
+/// the system. The server accepts them, but upon discovering that they are
+/// yet available (for different definitions of not being available), adds
+/// the to to-be-activated list.
+///
+/// Cases covered by dibbler are:
+/// - missing interface (e.g. PPP connection that is not established yet)
+/// - downed interface (no link local address, no way to open sockets)
+/// - up, but not running interface (wifi up, but not associated)
+/// - tentative addresses (interface up and running, but DAD procedure is
+///   still in progress)
+/// - weird interfaces without link-local addresses (don't ask, 6rd tunnels
+///   look weird to me as well)
+
 // No specific tests for getSubnet6. That method (2 overloaded versions) is tested
 // in Dhcpv6SrvTest.selectSubnetAddr and Dhcpv6SrvTest.selectSubnetIface
 // (see src/bin/dhcp6/tests/dhcp6_srv_unittest.cc)