[2246] Changes after review:

 - another patch contributed by dclink (thanks!)
 - BIND10 Guide updated
 - BSD/Sun interface detection cleaned up
 - MAC address checking improved
 - Developer's guide updated
 - Several whitespace cleanups

ChangeLog

+6XX.	[func]		dclink, tomek
+	libdhcp++: Interface detection implemented for FreeBSD, NetBSD,
+	OpenBSD, Mac OS X and Solaris 11. Thanks to David Carlier for
+	contributing a patch.
+	(Trac #2246, git abcd)
+
 691.	[bug]		marcin
 	libdhcp++: Created definitions for standard DHCPv4 options:
 	tftp-server-name (66) and boot-file-name (67). Also, fixed definition

doc/guide/bind10-guide.xml

@@ -4451,7 +4451,7 @@ Dhcp4/renew-timer	1000	integer	(default)
             available from <ulink url="http://www.isc.org/software/dhcp"/>.</simpara>
           </listitem>
           <listitem>
-            <simpara>Interface detection is currently working on Linux
+            <simpara>Raw sockets operation is working on Linux
             only. See <xref linkend="iface-detect"/> for details.</simpara>
           </listitem>
           <listitem>
@@ -5331,10 +5331,6 @@ Dhcp6/renew-timer	1000	integer	(default)
         <listitem>
           <simpara>DNS Update is not supported.</simpara>
         </listitem>
-        <listitem>
-          <simpara>Interface detection is currently working on Linux
-          only. See <xref linkend="iface-detect"/> for details.</simpara>
-        </listitem>
       </itemizedlist>
     </section>
 
@@ -5369,16 +5365,17 @@ Dhcp6/renew-timer	1000	integer	(default)
 <!-- TODO: point to doxygen docs -->
 
     <section id="iface-detect">
-      <title>Interface detection</title>
+      <title>Interface detection and Socket handling</title>
       <para>Both the DHCPv4 and DHCPv6 components share network
       interface detection routines. Interface detection is
-      currently only supported on Linux systems.</para>
-
-      <para>For non-Linux systems, there is currently a stub
-      implementation provided. The interface manager detects loopback
-      interfaces only as their name (lo or lo0) can be easily predicted.
-      Please contact the BIND 10 development team if you are interested
-      in running DHCP components on systems other than Linux.</para>
+      currently supported on Linux, all BSD family (FreeBSD, NetBSD,
+      OpenBSD), Mac OS X and Solaris 11 systems.</para>
+
+      <para>DHCPv4 requires special raw socket processing to send and receive
+      packets from hosts that do not have IPv4 address assigned yet. Support
+      for this operation is implemented on Linux only, so it is likely that
+      DHCPv4 component will not work in certain cases on systems other than
+      Linux.</para>
     </section>
 
 <!--

src/lib/dhcp/iface_mgr_bsd.cc

@@ -46,21 +46,21 @@ IfaceMgr::detectIfaces() {
     }
 
     typedef map<string, Iface> ifaceLst;
-    ifaceLst::iterator itf;
+    ifaceLst::iterator iface_iter;
     ifaceLst ifaces;
 
     // First lookup for getting interfaces ...
-    for(ifptr = iflist; ifptr != 0; ifptr = ifptr->ifa_next) {
+    for (ifptr = iflist; ifptr != 0; ifptr = ifptr->ifa_next) {
         const char * ifname = ifptr->ifa_name;
         uint ifindex = 0;
 
-        if(!(ifindex = if_nametoindex(ifname))) {
+        if (!(ifindex = if_nametoindex(ifname))) {
             // Interface name does not have corresponding index ...
             freeifaddrs(iflist);
             isc_throw(Unexpected, "Interface " << ifname << " has no index");
         }
 
-        if((itf = ifaces.find(ifname)) != ifaces.end()) {
+        if ((iface_iter = ifaces.find(ifname)) != ifaces.end()) {
             continue;
         }
 
@@ -70,8 +70,8 @@ IfaceMgr::detectIfaces() {
     }
 
     // Second lookup to get MAC and IP addresses
-    for(ifptr = iflist; ifptr != 0; ifptr = ifptr->ifa_next) {
-        if((itf = ifaces.find(ifptr->ifa_name)) == ifaces.end()) {
+    for (ifptr = iflist; ifptr != 0; ifptr = ifptr->ifa_next) {
+        if ((iface_iter = ifaces.find(ifptr->ifa_name)) == ifaces.end()) {
             continue;
         }
         // Common byte pointer for following data
@@ -82,8 +82,8 @@ IfaceMgr::detectIfaces() {
                 reinterpret_cast<struct sockaddr_dl *>(ifptr->ifa_addr);
             ptr = reinterpret_cast<uint8_t *>(LLADDR(ldata));
 
-            itf->second.setHWType(ldata->sdl_type);
-            itf->second.setMac(ptr, ldata->sdl_alen);
+            iface_iter->second.setHWType(ldata->sdl_type);
+            iface_iter->second.setMac(ptr, ldata->sdl_alen);
         } else if(ifptr->ifa_addr->sa_family == AF_INET6) {
             // IPv6 Addr
             struct sockaddr_in6 * adata =
@@ -91,7 +91,7 @@ IfaceMgr::detectIfaces() {
             ptr = reinterpret_cast<uint8_t *>(&adata->sin6_addr);
 
             IOAddress a = IOAddress::fromBytes(AF_INET6, ptr);
-            itf->second.addAddress(a);
+            iface_iter->second.addAddress(a);
         } else {
             // IPv4 Addr
             struct sockaddr_in * adata =
@@ -99,18 +99,16 @@ IfaceMgr::detectIfaces() {
             ptr = reinterpret_cast<uint8_t *>(&adata->sin_addr);
 
             IOAddress a = IOAddress::fromBytes(AF_INET, ptr);
-            itf->second.addAddress(a);
+            iface_iter->second.addAddress(a);
         }
     }
 
     freeifaddrs(iflist);
 
-    // Registers interfaces with at least an IP addresses
-    for(ifaceLst::const_iterator itf = ifaces.begin();
-        itf != ifaces.end(); ++ itf) {
-        if(itf->second.getAddresses().size() > 0) {
-            ifaces_.push_back(itf->second);
-        }
+    // Interfaces registering
+    for(ifaceLst::const_iterator iface_iter = ifaces.begin();
+        iface_iter != ifaces.end(); ++iface_iter) {
+        ifaces_.push_back(iface_iter->second);
     }
 }
 
@@ -119,7 +117,7 @@ IfaceMgr::detectIfaces() {
 /// Like Linux version, os specific flags
 ///
 /// @params flags
-void Iface::setFlags(uint32_t flags) {
+void Iface::setFlags(uint64_t flags) {
     flags_ = flags;
 
     flag_loopback_ = flags & IFF_LOOPBACK;

src/lib/dhcp/iface_mgr_sun.cc

@@ -46,7 +46,7 @@ IfaceMgr::detectIfaces() {
     }
 
     typedef std::map<string, Iface> ifaceLst;
-    ifaceLst::iterator itf;
+    ifaceLst::iterator iface_iter;
     ifaceLst ifaces;
 
     // First lookup for getting interfaces ...
@@ -60,7 +60,7 @@ IfaceMgr::detectIfaces() {
             isc_throw(Unexpected, "Interface " << ifname << " has no index");
         }
 
-        if((itf = ifaces.find(ifname)) != iface.end()) {
+        if((iface_iter = ifaces.find(ifname)) != iface.end()) {
             continue;
         }
 
@@ -70,8 +70,8 @@ IfaceMgr::detectIfaces() {
     }
 
     // Second lookup to get MAC and IP addresses
-    for(ifptr = iflist; ifptr != 0; ifptr = ifptr->ifa_next) {
-        if((itf = ifaces.find(ifptr->ifa_name)) == ifaces.end()) {
+    for (ifptr = iflist; ifptr != 0; ifptr = ifptr->ifa_next) {
+        if ((itf = ifaces.find(ifptr->ifa_name)) == ifaces.end()) {
             continue;
         }
         // Common byte pointer for following data
@@ -82,8 +82,8 @@ IfaceMgr::detectIfaces() {
                 reinterpret_cast<struct sockaddr_dl *>(ifptr->ifa_addr);
             ptr = reinterpret_cast<uint8_t *>(LLADDR(ldata));
 
-            itf->second.setHWType(ldata->sdl_type);
-            itf->second.setMac(ptr, ldata->sdl_alen);
+            iface_iter->second.setHWType(ldata->sdl_type);
+            iface_iter->second.setMac(ptr, ldata->sdl_alen);
         } else if(ifptr->ifa_addr->sa_family == AF_INET6) {
             // IPv6 Addr
             struct sockaddr_in6 * adata =
@@ -91,7 +91,7 @@ IfaceMgr::detectIfaces() {
             ptr = reinterpret_cast<uint8_t *>(& adata->sin6_addr);
 
             IOAddress a = IOAddress::fromBytes(AF_INET6, ptr);
-            itf->second.addAddress(a);
+            iface_iter->second.addAddress(a);
         } else {
             // IPv4 Addr
             struct sockaddr_in * adata =
@@ -99,18 +99,16 @@ IfaceMgr::detectIfaces() {
             ptr = reinterpret_cast<uint8_t *>(& adata->sin_addr);
 
             IOAddress a = IOAddress::fromBytes(AF_INET, ptr);
-            itf->second.addAddress(a);
+            iface_iter->second.addAddress(a);
         }
     }
 
     freeifaddrs(iflist);
 
-    // Registers interfaces with at least an IP addresses
-    for(ifaceLst::const_iterator itf = ifaces.begin();
-        itf != ifaces.end(); ++ itf) {
-        if(itf->second.getAddresses().size() > 0) {
-            ifaces_.push_back(itf->second);
-        }
+    // Interfaces registering
+    for (ifaceLst::const_iterator iface_iter = ifaces.begin();
+        iface_iter != ifaces.end(); ++iface_iter) {
+        ifaces_.push_back(iface_iter->second);
     }
 }
 

src/lib/dhcp/libdhcp++.dox

@@ -19,8 +19,9 @@
 
 libdhcp++ is an all-purpose DHCP-manipulation library, written in
 C++. It offers packet parsing and assembly, DHCPv4 and DHCPv6
-options parsing and ssembly, interface detection (currently on
-Linux systems only) and socket operations. It is a generic purpose library that
+options parsing and assembly, interface detection (currently on
+Linux, FreeBSD, NetBSD, OpenBSD, Max OS X, and Solaris 11) and socket operations.
+It is a generic purpose library that
 can be used by server, client, relay, performance tools and other DHCP-related
 tools. For server specific library, see \ref libdhcpsrv. Please do not
 add any server-specific code to libdhcp++ and use \ref libdhcpsrv instead.

src/lib/dhcp/tests/iface_mgr_unittest.cc

@@ -1364,7 +1364,7 @@ void parse_ifconfig(const std::string& textFile, IfaceMgr::IfaceCollection& ifac
 // TODO: temporarily disabled, see ticket #1529
 TEST_F(IfaceMgrTest, DISABLED_detectIfaces_linux) {
 
-    NakedIfaceMgr* ifacemgr = new NakedIfaceMgr();
+    scoped_ptr<NakedIfaceMgr> ifacemgr(new NakedIfaceMgr());
     IfaceMgr::IfaceCollection& detectedIfaces = ifacemgr->getIfacesLst();
 
     const std::string textFile = "ifconfig.txt";
@@ -1469,13 +1469,9 @@ TEST_F(IfaceMgrTest, DISABLED_detectIfaces_linux) {
             FAIL();
         }
     }
-
-    delete ifacemgr;
 }
 #endif
 
-#if defined(OS_LINUX) || defined(OS_BSD) || defined(OS_SUN)
-
 #if defined(OS_BSD)
 #include <net/if_dl.h>
 #endif
@@ -1484,19 +1480,23 @@ TEST_F(IfaceMgrTest, DISABLED_detectIfaces_linux) {
 #include <net/if.h>
 #include <ifaddrs.h>
 
-// Checks the index of this interface
+/// @brief Checks the index of this interface
+/// @param iface Kea interface structure to be checked
+/// @param ifptr original structure returned by getifaddrs
+/// @return true if index is returned properly
 bool
 checkIfIndex(const Iface & iface,
              struct ifaddrs *& ifptr) {
-    cout << "Checks index of " << iface.getName() << endl;
     return (iface.getIndex() == if_nametoindex(ifptr->ifa_name));
 }
 
-// Checks if the interface has proper flags set
+/// @brief Checks if the interface has proper flags set
+/// @param iface Kea interface structure to be checked
+/// @param ifptr original structure returned by getifaddrs
+/// @return true if flags are set properly
 bool
 checkIfFlags(const Iface & iface,
              struct ifaddrs *& ifptr) {
-    cout << "Checks flags of " << iface.getName() << endl;
         bool flag_loopback_ = ifptr->ifa_flags & IFF_LOOPBACK;
         bool flag_up_ = ifptr->ifa_flags & IFF_UP;
         bool flag_running_ = ifptr->ifa_flags & IFF_RUNNING;
@@ -1511,6 +1511,7 @@ checkIfFlags(const Iface & iface,
 /// @brief Checks if MAC Address/IP Addresses are properly well formed
 /// @param iface Kea interface structure to be checked
 /// @param ifptr original structure returned by getifaddrs
+/// @return true if addresses are returned properly
 bool
 checkIfAddrs(const Iface & iface, struct ifaddrs *& ifptr) {
     const unsigned char * p = 0;
@@ -1518,24 +1519,21 @@ checkIfAddrs(const Iface & iface, struct ifaddrs *& ifptr) {
     // Workaround for Linux ...
     if(ifptr->ifa_data != 0) {
         // We avoid localhost as it has no MAC Address
-        if(!strncmp(iface.getName().c_str(), "lo", 2)) {
+        if (!strncmp(iface.getName().c_str(), "lo", 2)) {
             return (true);
         }
 
-        // Typically unit-tests try to be silent. But interface detection is
-        // tricky enough to warrant additional prints.
-        cout << "Checks MAC Addr of " << iface.getName() << endl;
         struct ifreq ifr;
         memset(& ifr.ifr_name, 0, sizeof ifr.ifr_name);
         strncpy(ifr.ifr_name, iface.getName().c_str(), sizeof ifr.ifr_name);
 
         int s = -1; // Socket descriptor
 
-        if((s = socket(AF_INET, SOCK_DGRAM, 0)) < 0) {
+        if ((s = socket(AF_INET, SOCK_DGRAM, 0)) < 0) {
             isc_throw(Unexpected, "Cannot create AF_INET socket");
         }
 
-        if(ioctl(s, SIOCGIFHWADDR, & ifr) < 0) {
+        if (ioctl(s, SIOCGIFHWADDR, & ifr) < 0) {
             close(s);
             isc_throw(Unexpected, "Cannot set SIOCGIFHWADDR flag");
         }
@@ -1548,15 +1546,7 @@ checkIfAddrs(const Iface & iface, struct ifaddrs *& ifptr) {
         /// @todo: Check MAC address length. For some interfaces it is
         /// different than 6. Some have 0, while some exotic ones (like
         /// infiniband) have 20.
-        int i = 0;
-        while(i < 6) {
-            if(* (p + i) != * (iface.getMac() + i)) {
-                return (false);
-            }
-            ++ i;
-        }
-
-        return (true);
+        return (!memcmp(p, iface.getMac(), iface.getMacLen()));
     }
 #endif
 
@@ -1568,42 +1558,33 @@ checkIfAddrs(const Iface & iface, struct ifaddrs *& ifptr) {
 #if defined(OS_BSD)
         case AF_LINK: {
             // We avoid localhost as it has no MAC Address
-            if(!strncmp(iface.getName().c_str(), "lo", 2)) {
+            if (!strncmp(iface.getName().c_str(), "lo", 2)) {
                 return (true);
             }
 
-            cout << "Checks MAC Addr of " << iface.getName() << endl;
             struct sockaddr_dl * hwdata =
                    reinterpret_cast<struct sockaddr_dl *>(ifptr->ifa_addr);
             p = reinterpret_cast<uint8_t *>(LLADDR(hwdata));
 
             // Extract MAC address length
-            if(hwdata->sdl_alen != iface.getMacLen()) {
+            if (hwdata->sdl_alen != iface.getMacLen()) {
                 return (false);
             }
 
-            int i = 0;
-            while(i < hwdata->sdl_alen) {
-                if(* (p + i) != * (iface.getMac() + i)) {
-                    return (false);
-                }
-                ++ i;
-            }
-
-            return (true);
+            return (!memcmp(p, iface.getMac(), hwdata->sdl_alen));
         }
 #endif
         case AF_INET: {
-            cout << "Checks IPv4 address of " << iface.getName() << endl;
             struct sockaddr_in * v4data =
                    reinterpret_cast<struct sockaddr_in *>(ifptr->ifa_addr);
             p = reinterpret_cast<uint8_t *>(& v4data->sin_addr);
 
             IOAddress addrv4 = IOAddress::fromBytes(AF_INET, p);
 
-            for(Iface::AddressCollection::const_iterator a = iface.getAddresses().begin();
-                a != iface.getAddresses().end(); ++ a) {
-                if((* a).isV4() && (* a) == addrv4) {
+            for (Iface::AddressCollection::const_iterator a =
+                     iface.getAddresses().begin();
+                 a != iface.getAddresses().end(); ++ a) {
+                if(a->isV4() && (*a) == addrv4) {
                     return (true);
                 }
             }
@@ -1611,16 +1592,16 @@ checkIfAddrs(const Iface & iface, struct ifaddrs *& ifptr) {
             return (false);
         }
         case AF_INET6: {
-            cout << "Checks IPv6 address of " << iface.getName() << endl;
             struct sockaddr_in6 * v6data =
                    reinterpret_cast<struct sockaddr_in6 *>(ifptr->ifa_addr);
             p = reinterpret_cast<uint8_t *>(& v6data->sin6_addr);
 
             IOAddress addrv6 = IOAddress::fromBytes(AF_INET6, p);
 
-            for(Iface::AddressCollection::const_iterator a = iface.getAddresses().begin();
+            for(Iface::AddressCollection::const_iterator a =
+                    iface.getAddresses().begin();
                 a != iface.getAddresses().end(); ++ a) {
-                if((* a).isV6() && (* a) == addrv6) {
+                if(a->isV6() && (*a) == addrv6) {
                     return (true);
                 }
             }
@@ -1646,15 +1627,24 @@ TEST_F(IfaceMgrTest, detectIfaces) {
         isc_throw(Unexpected, "Cannot detect interfaces");
     }
 
-    for(ifptr = iflist; ifptr != 0; ifptr = ifptr->ifa_next) {
-        for(IfaceMgr::IfaceCollection::const_iterator i = detectedIfaces.begin();
+    for (ifptr = iflist; ifptr != 0; ifptr = ifptr->ifa_next) {
+        for (IfaceMgr::IfaceCollection::const_iterator i = detectedIfaces.begin();
             i != detectedIfaces.end(); ++ i) {
-            if(!strncmp(ifptr->ifa_name, (*i).getName().c_str(),
-                        (*i).getName().size())) {
+            if (!strncmp(ifptr->ifa_name, (*i).getName().c_str(),
+                         (*i).getName().size())) {
+
+                // Typically unit-tests try to be silent. But interface detection
+                // is tricky enough to warrant additional prints.
+                std::cout << "Checking interface " << i->getName() << std::endl;
+
+                // Check if interface index is reported properly
+                EXPECT_TRUE(checkIfIndex(*i, ifptr));
 
-                checkIfIndex(*i, ifptr);
-                checkIfFlags(*i, ifptr);
-                checkIfAddrs(*i, ifptr);
+                // Check if flags are reported properly
+                EXPECT_TRUE(checkIfFlags(*i, ifptr));
+
+                // Check if addresses are reported properly
+                EXPECT_TRUE(checkIfAddrs(*i, ifptr));
 
             }
         }
@@ -1665,7 +1655,6 @@ TEST_F(IfaceMgrTest, detectIfaces) {
 
     delete ifacemgr;
 }
-#endif
 
 volatile bool callback_ok;