[1238] Changes after review.

- IfaceMgr::Iface is now a class, not a struct.
- openSockets() now takes one argument that specifies port number
- Added missing doxygen comments
- Comments improved

src/bin/dhcp6/dhcp6_srv.cc

@@ -26,18 +26,17 @@ using namespace isc;
 using namespace isc::dhcp;
 using namespace isc::asiolink;
 
-Dhcpv6Srv::Dhcpv6Srv() {
+Dhcpv6Srv::Dhcpv6Srv(uint16_t port) {
+
+//void Dhcpv6Srv::Dhcpv6Srv_impl(uint16_t port) {
     cout << "Initialization" << endl;
 
-    // first call to instance() will create IfaceMgr (it's a singleton)
-    // it may throw something if things go wrong
+    // First call to instance() will create IfaceMgr (it's a singleton).
+    // It may throw something if things go wrong.
     IfaceMgr::instance();
 
-    try {
-        IfaceMgr::instance().openSockets();
-    } catch (const Exception& e) {
-
-    }
+    // Now try to open IPv6 sockets on detected interfaces.
+    IfaceMgr::instance().openSockets(port);
 
     /// @todo: instantiate LeaseMgr here once it is imlpemented.
 

src/bin/dhcp6/dhcp6_srv.h

@@ -17,8 +17,9 @@
 
 #include <boost/shared_ptr.hpp>
 #include <boost/noncopyable.hpp>
-#include "dhcp/pkt6.h"
-#include "dhcp/option.h"
+#include <dhcp/dhcp6.h>
+#include <dhcp/pkt6.h>
+#include <dhcp/option.h>
 #include <iostream>
 
 namespace isc {
@@ -41,7 +42,9 @@ public:
     /// In particular, creates IfaceMgr that will be responsible for
     /// network interaction. Will instantiate lease manager, and load
     /// old or create new DUID.
-    Dhcpv6Srv();
+    ///
+    /// @param port port on will all sockets will listen
+    Dhcpv6Srv(uint16_t port = DHCP6_SERVER_PORT);
 
     /// @brief Destructor. Used during DHCPv6 service shutdown.
     ~Dhcpv6Srv();

src/bin/dhcp6/iface_mgr.cc

@@ -176,21 +176,20 @@ IfaceMgr::detectIfaces() {
 }
 
 void
-IfaceMgr::openSockets() {
+IfaceMgr::openSockets(uint16_t port) {
     int sock1, sock2;
 
-    for (IfaceCollection::iterator iface=ifaces_.begin();
-         iface!=ifaces_.end();
-         ++iface) {
+    for (IfaceCollection::iterator iface = ifaces_.begin();
+         iface != ifaces_.end(); ++iface) {
 
         AddressCollection addrs = iface->getAddresses();
 
-        for (AddressCollection::iterator addr= addrs.begin();
+        for (AddressCollection::iterator addr = addrs.begin();
              addr != addrs.end();
              ++addr) {
 
-            sock1 = openSocket(iface->getName(), *addr, DHCP6_SERVER_PORT);
-            if (sock1<0) {
+            sock1 = openSocket(iface->getName(), *addr, port);
+            if (sock1 < 0) {
                 isc_throw(Unexpected, "Failed to open unicast socket on "
                           << " interface " << iface->getFullName());
             }
@@ -205,8 +204,8 @@ IfaceMgr::openSockets() {
             // this doesn't work too well on NetBSD
             sock2 = openSocket(iface->getName(),
                                IOAddress(ALL_DHCP_RELAY_AGENTS_AND_SERVERS),
-                               DHCP6_SERVER_PORT);
-            if (sock2<0) {
+                               port);
+            if (sock2 < 0) {
                 isc_throw(Unexpected, "Failed to open multicast socket on "
                           << " interface " << iface->getFullName());
                 iface->delSocket(sock1); // delete previously opened socket
@@ -217,16 +216,14 @@ IfaceMgr::openSockets() {
 
 void
 IfaceMgr::printIfaces(std::ostream& out /*= std::cout*/) {
-    for (IfaceCollection::const_iterator iface=ifaces_.begin();
-         iface!=ifaces_.end();
-         ++iface) {
+    for (IfaceCollection::const_iterator iface = ifaces_.begin();
+         iface != ifaces_.end(); ++iface) {
         out << "Detected interface " << iface->getFullName() << endl;
         out << "  " << iface->getAddresses().size() << " addr(s):" << endl;
         const AddressCollection addrs = iface->getAddresses();
 
         for (AddressCollection::const_iterator addr = addrs.begin();
-             addr != addrs.end();
-             ++addr) {
+             addr != addrs.end(); ++addr) {
             out << "  " << addr->toText() << endl;
         }
         out << "  mac: " << iface->getPlainMac() << endl;
@@ -235,11 +232,11 @@ IfaceMgr::printIfaces(std::ostream& out /*= std::cout*/) {
 
 IfaceMgr::Iface*
 IfaceMgr::getIface(int ifindex) {
-    for (IfaceCollection::iterator iface=ifaces_.begin();
-         iface!=ifaces_.end();
-         ++iface) {
-        if (iface->getIndex() == ifindex)
+    for (IfaceCollection::iterator iface = ifaces_.begin();
+         iface != ifaces_.end(); ++iface) {
+        if (iface->getIndex() == ifindex) {
             return (&(*iface));
+        }
     }
 
     return (NULL); // not found
@@ -247,11 +244,11 @@ IfaceMgr::getIface(int ifindex) {
 
 IfaceMgr::Iface*
 IfaceMgr::getIface(const std::string& ifname) {
-    for (IfaceCollection::iterator iface=ifaces_.begin();
-         iface!=ifaces_.end();
-         ++iface) {
-        if (iface->getName() == ifname)
+    for (IfaceCollection::iterator iface = ifaces_.begin();
+         iface != ifaces_.end(); ++iface) {
+        if (iface->getName() == ifname) {
             return (&(*iface));
+        }
     }
 
     return (NULL); // not found
@@ -300,8 +297,8 @@ IfaceMgr::openSocket4(Iface& iface, const IOAddress& addr, int port) {
                   << "/port=" << port);
     }
 
-    // if there is no support for IP_PKTINFO, we are really out of luck
-    // it will be difficult to undersand, where this packet came from
+    // If there is no support for IP_PKTINFO, we are really out of luck.
+    // It will be difficult to understand, where this packet came from.
 #if defined(IP_PKTINFO)
     int flag = 1;
     if (setsockopt(sock, IPPROTO_IP, IP_PKTINFO, &flag, sizeof(flag)) != 0) {
@@ -346,8 +343,8 @@ IfaceMgr::openSocket6(Iface& iface, const IOAddress& addr, int port) {
         isc_throw(Unexpected, "Failed to create UDP6 socket.");
     }
 
-    /* Set the REUSEADDR option so that we don't fail to start if
-       we're being restarted. */
+    // Set the REUSEADDR option so that we don't fail to start if
+    // we're being restarted.
     int flag = 1;
     if (setsockopt(sock, SOL_SOCKET, SO_REUSEADDR,
                    (char *)&flag, sizeof(flag)) < 0) {
@@ -361,14 +358,14 @@ IfaceMgr::openSocket6(Iface& iface, const IOAddress& addr, int port) {
                   << "/port=" << port);
     }
 #ifdef IPV6_RECVPKTINFO
-    /* RFC3542 - a new way */
+    // RFC3542 - a new way
     if (setsockopt(sock, IPPROTO_IPV6, IPV6_RECVPKTINFO,
                    &flag, sizeof(flag)) != 0) {
         close(sock);
         isc_throw(Unexpected, "setsockopt: IPV6_RECVPKTINFO failed.");
     }
 #else
-    /* RFC2292 - an old way */
+    // RFC2292 - an old way
     if (setsockopt(sock, IPPROTO_IPV6, IPV6_PKTINFO,
                    &flag, sizeof(flag)) != 0) {
         close(sock);
@@ -440,14 +437,10 @@ IfaceMgr::send(boost::shared_ptr<Pkt6>& pkt) {
 
     memset(&control_buf_[0], 0, control_buf_len_);
 
-    /*
-     * Initialize our message header structure.
-     */
+    // Initialize our message header structure.
     memset(&m, 0, sizeof(m));
 
-    /*
-     * Set the target address we're sending to.
-     */
+    // Set the target address we're sending to.
     sockaddr_in6 to;
     memset(&to, 0, sizeof(to));
     to.sin6_family = AF_INET6;
@@ -460,24 +453,20 @@ IfaceMgr::send(boost::shared_ptr<Pkt6>& pkt) {
     m.msg_name = &to;
     m.msg_namelen = sizeof(to);
 
-    /*
-     * Set the data buffer we're sending. (Using this wacky
-     * "scatter-gather" stuff... we only have a single chunk
-     * of data to send, so we declare a single vector entry.)
-     */
+    // Set the data buffer we're sending. (Using this wacky
+    // "scatter-gather" stuff... we only have a single chunk
+    // of data to send, so we declare a single vector entry.)
     v.iov_base = (char *) &pkt->data_[0];
     v.iov_len = pkt->data_len_;
     m.msg_iov = &v;
     m.msg_iovlen = 1;
 
-    /*
-     * Setting the interface is a bit more involved.
-     *
-     * We have to create a "control message", and set that to
-     * define the IPv6 packet information. We could set the
-     * source address if we wanted, but we can safely let the
-     * kernel decide what that should be.
-     */
+    // Setting the interface is a bit more involved.
+    //
+    // We have to create a "control message", and set that to
+    // define the IPv6 packet information. We could set the
+    // source address if we wanted, but we can safely let the
+    // kernel decide what that should be.
     m.msg_control = &control_buf_[0];
     m.msg_controllen = control_buf_len_;
     cmsg = CMSG_FIRSTHDR(&m);
@@ -549,35 +538,27 @@ IfaceMgr::receive6() {
     memset(&from, 0, sizeof(from));
     memset(&to_addr, 0, sizeof(to_addr));
 
-    /*
-     * Initialize our message header structure.
-     */
+    // Initialize our message header structure.
     memset(&m, 0, sizeof(m));
 
-    /*
-     * Point so we can get the from address.
-     */
+    // Point so we can get the from address.
     m.msg_name = &from;
     m.msg_namelen = sizeof(from);
 
-    /*
-     * Set the data buffer we're receiving. (Using this wacky
-     * "scatter-gather" stuff... but we that doesn't really make
-     * sense for us, so we use a single vector entry.)
-     */
+    // Set the data buffer we're receiving. (Using this wacky
+    // "scatter-gather" stuff... but we that doesn't really make
+    // sense for us, so we use a single vector entry.)
     v.iov_base = (void*)&pkt->data_[0];
     v.iov_len = pkt->data_len_;
     m.msg_iov = &v;
     m.msg_iovlen = 1;
 
-    /*
-     * Getting the interface is a bit more involved.
-     *
-     * We set up some space for a "control message". We have
-     * previously asked the kernel to give us packet
-     * information (when we initialized the interface), so we
-     * should get the destination address from that.
-     */
+    // Getting the interface is a bit more involved.
+    //
+    // We set up some space for a "control message". We have
+    // previously asked the kernel to give us packet
+    // information (when we initialized the interface), so we
+    // should get the destination address from that.
     m.msg_control = &control_buf_[0];
     m.msg_controllen = control_buf_len_;
 
@@ -611,14 +592,12 @@ IfaceMgr::receive6() {
     result = recvmsg(candidate->sockfd_, &m, 0);
 
     if (result >= 0) {
-        /*
-         * If we did read successfully, then we need to loop
-         * through the control messages we received and
-         * find the one with our destination address.
-         *
-         * We also keep a flag to see if we found it. If we
-         * didn't, then we consider this to be an error.
-         */
+        // If we did read successfully, then we need to loop
+        // through the control messages we received and
+        // find the one with our destination address.
+        //
+        // We also keep a flag to see if we found it. If we
+        // didn't, then we consider this to be an error.
         int found_pktinfo = 0;
         cmsg = CMSG_FIRSTHDR(&m);
         while (cmsg != NULL) {

src/bin/dhcp6/iface_mgr.h

@@ -46,6 +46,12 @@ public:
         isc::asiolink::IOAddress addr_; /// bound address
         uint16_t port_;   /// socket port
         uint16_t family_; /// IPv4 or IPv6
+
+        /// @brief SocketInfo constructor.
+        ///
+        /// @param sockfd socket descriptor
+        /// @param addr an address the socket is bound to
+        /// @param port a port the socket is bound to
         SocketInfo(uint16_t sockfd, const isc::asiolink::IOAddress& addr,
                    uint16_t port)
         :sockfd_(sockfd), addr_(addr), port_(port), family_(addr.getFamily()) { }
@@ -59,7 +65,8 @@ public:
     /// Iface structure represents network interface with all useful
     /// information, like name, interface index, MAC address and
     /// list of assigned addresses
-    struct Iface {
+    class Iface {
+    public:
         /// @brief Iface constructor.
         ///
         /// Creates Iface object that represents network interface.
@@ -222,24 +229,31 @@ public:
     void
     printIfaces(std::ostream& out = std::cout);
 
-    /// @brief Sends a packet.
+    /// @brief Sends an IPv6 packet.
     ///
-    /// Sends a packet. All parameters for actual transmission are specified in
+    /// Sends an IPv6 packet. All parameters for actual transmission are specified in
     /// Pkt6 structure itself. That includes destination address, src/dst port
     /// and interface over which data will be sent.
     ///
     /// @param pkt packet to be sent
     ///
     /// @return true if sending was successful
-    bool
-    send(boost::shared_ptr<Pkt6>& pkt);
+    bool send(boost::shared_ptr<Pkt6>& pkt);
 
-    bool
-    send(boost::shared_ptr<Pkt4>& pkt);
+    /// @brief Sends an IPv4 packet.
+    ///
+    /// Sends an IPv4 packet. All parameters for actual transmission are specified
+    /// in Pkt4 structure itself. That includes destination address, src/dst
+    /// port and interface over which data will be sent.
+    ///
+    /// @param pkt a packet to be sent
+    ///
+    /// @return true if sending was successful
+    bool send(boost::shared_ptr<Pkt4>& pkt);
 
-    /// @brief Tries to receive packet over open sockets.
+    /// @brief Tries to receive IPv6 packet over open IPv6 sockets.
     ///
-    /// Attempts to receive a single packet of any of the open sockets.
+    /// Attempts to receive a single IPv6 packet of any of the open IPv6 sockets.
     /// If reception is successful and all information about its sender
     /// are obtained, Pkt6 object is created and returned.
     ///
@@ -250,9 +264,19 @@ public:
     /// @return Pkt6 object representing received packet (or NULL)
     boost::shared_ptr<Pkt6> receive6();
 
+    /// @brief Tries to receive IPv4 packet over open IPv4 sockets.
+    ///
+    /// Attempts to receive a single IPv4 packet of any of the open IPv4 sockets.
+    /// If reception is successful and all information about its sender
+    /// are obtained, Pkt4 object is created and returned.
+    ///
+    /// TODO Start using select() and add timeout to be able
+    /// to not wait infinitely, but rather do something useful
+    /// (e.g. remove expired leases)
+    ///
+    /// @return Pkt4 object representing received packet (or NULL)
     boost::shared_ptr<Pkt4> receive4();
 
-    ///
     /// Opens UDP/IP socket and binds it to address, interface and port.
     ///
     /// Specific type of socket (UDP/IPv4 or UDP/IPv6) depends on passed addr
@@ -262,13 +286,20 @@ public:
     /// @param addr address to be bound.
     /// @param port UDP port.
     ///
+    /// Method will throw if socket creation, socket binding or multicast
+    /// join fails.
+    ///
     /// @return socket descriptor, if socket creation, binding and multicast
-    /// group join were all successful. -1 otherwise.
+    /// group join were all successful.
     uint16_t openSocket(const std::string& ifname,
                         const isc::asiolink::IOAddress& addr, int port);
 
-    /// Opens sockets on detected interfaces.
-    void openSockets();
+    /// Opens IPv6 sockets on detected interfaces.
+    ///
+    /// Will throw exception if socket creation fails.
+    ///
+    /// @param port specifies port number (usually DHCP6_SERVER_PORT)
+    void openSockets(uint16_t port);
 
     // don't use private, we need derived classes in tests
 protected:
@@ -276,15 +307,40 @@ protected:
     /// @brief Protected constructor.
     ///
     /// Protected constructor. This is a singleton class. We don't want
-    /// anyone to create instances of IfaceMgr. Use instance() method
+    /// anyone to create instances of IfaceMgr. Use instance() method instead.
     IfaceMgr();
 
     ~IfaceMgr();
 
+    /// @brief Opens IPv4 socket.
+    ///
+    /// Please do not use this method directly. Use openSocket instead.
+    ///
+    /// This method may throw exception if socket creation fails.
+    ///
+    /// @param iface reference to interface structure.
+    /// @param addr an address the created socket should be bound to
+    /// @param port a port that created socket should be bound to
+    ///
+    /// @return socket descriptor
     uint16_t openSocket4(Iface& iface, const isc::asiolink::IOAddress& addr, int port);
 
+    /// @brief Opens IPv6 socket.
+    ///
+    /// Please do not use this method directly. Use openSocket instead.
+    ///
+    /// This method may throw exception if socket creation fails.
+    ///
+    /// @param iface reference to interface structure.
+    /// @param addr an address the created socket should be bound to
+    /// @param port a port that created socket should be bound to
+    ///
+    /// @return socket descriptor
     uint16_t openSocket6(Iface& iface, const isc::asiolink::IOAddress& addr, int port);
 
+    /// @brief Adds an interface to list of known interfaces.
+    ///
+    /// @param iface reference to Iface object.
     void addInterface(const Iface& iface) {
         ifaces_.push_back(iface);
     }

src/bin/dhcp6/tests/dhcp6_srv_unittest.cc

@@ -53,19 +53,18 @@ public:
 };
 
 TEST_F(Dhcpv6SrvTest, basic) {
-    // there's almost no code now. What's there provides echo capability
-    // that is just a proof of concept and will be removed soon
-    // No need to thoroughly test it
-
     // srv has stubbed interface detection. It will read
     // interfaces.txt instead. It will pretend to have detected
     // fe80::1234 link-local address on eth0 interface. Obviously
     // an attempt to bind this socket will fail.
+    Dhcpv6Srv * srv = 0;
     EXPECT_NO_THROW( {
-        Dhcpv6Srv * srv = new Dhcpv6Srv();
-
+        // open an unpriviledged port
+        srv = new Dhcpv6Srv(DHCP6_SERVER_PORT + 10000);
+    });
+    if (srv) {
         delete srv;
-        });
+    }
 
 }
 
@@ -75,7 +74,7 @@ TEST_F(Dhcpv6SrvTest, Solicit_basic) {
 
     // a dummy content for client-id
     boost::shared_array<uint8_t> clntDuid(new uint8_t[32]);
-    for (int i=0; i<32; i++)
+    for (int i = 0; i < 32; i++)
         clntDuid[i] = 100+i;
 
     boost::shared_ptr<Pkt6> sol =