[878] Many improvements in IfaceMgr after review

doc/Doxyfile

@@ -574,7 +574,7 @@ INPUT                  = ../src/lib/cc ../src/lib/config \
     ../src/lib/log ../src/lib/asiolink/ ../src/lib/nsas \
     ../src/lib/testutils ../src/lib/cache ../src/lib/server_common/ \
     ../src/bin/sockcreator/ ../src/lib/util/ \
-    ../src/lib/resolve ../src/lib/acl
+    ../src/lib/resolve ../src/lib/acl ../src/bin/dhcp6
 
 # This tag can be used to specify the character encoding of the source files
 # that doxygen parses. Internally doxygen uses the UTF-8 encoding, which is

src/bin/dhcp6/iface_mgr.cc

@@ -48,11 +48,6 @@ IfaceMgr::instance() {
     return (*instance_);
 }
 
-IfaceMgr::Iface::Iface()
-    : name_(""), ifindex_(0), mac_len_(0) {
-    memset(mac_, 0, 20);
-}
-
 IfaceMgr::Iface::Iface(const std::string& name, int ifindex)
     :name_(name), ifindex_(ifindex), mac_len_(0) {
     memset(mac_, 0, 20);
@@ -84,6 +79,9 @@ IfaceMgr::IfaceMgr() {
     cout << "IfaceMgr initialization." << endl;
 
     try {
+        // required for sending/receiving packets
+        // let's keep it in front, just in case someone
+        // wants to send anything during initialization
         control_buf_len_ = CMSG_SPACE(sizeof(struct in6_pktinfo));
         control_buf_ = new char[control_buf_len_];
 
@@ -140,6 +138,11 @@ IfaceMgr::detectIfaces() {
         ifaces_.push_back(iface);
         interfaces.close();
     } catch (std::exception& ex) {
+        // TODO: deallocate whatever memory we used
+        // not that important, since this function is going to be
+        // thrown away as soon as we get proper interface detection
+        // implemented
+
         // TODO Do LOG_FATAL here
         std::cerr << "Interface detection failed." << std::endl;
         throw ex;
@@ -171,6 +174,7 @@ IfaceMgr::openSockets() {
                               DHCP6_SERVER_PORT, true);
             if (sock<0) {
                 cout << "Failed to open multicast socket." << endl;
+                close(sendsock_);
                 return (false);
             }
             recvsock_ = sock;
@@ -181,18 +185,18 @@ IfaceMgr::openSockets() {
 }
 
 void
-IfaceMgr::printIfaces() {
+IfaceMgr::printIfaces(std::ostream& out /*= std::cout*/) {
     for (IfaceLst::const_iterator iface=ifaces_.begin();
          iface!=ifaces_.end();
          ++iface) {
-        cout << "Detected interface " << iface->getFullName() << endl;
-        cout << "  " << iface->addrs_.size() << " addr(s):" << endl;
+        out << "Detected interface " << iface->getFullName() << endl;
+        out << "  " << iface->addrs_.size() << " addr(s):" << endl;
         for (Addr6Lst::const_iterator addr=iface->addrs_.begin();
              addr != iface->addrs_.end();
              ++addr) {
-            cout << "  " << addr->toText() << endl;
+            out << "  " << addr->toText() << endl;
         }
-        cout << "  mac: " << iface->getPlainMac() << endl;
+        out << "  mac: " << iface->getPlainMac() << endl;
     }
 }
 
@@ -205,7 +209,7 @@ IfaceMgr::getIface(int ifindex) {
             return (&(*iface));
     }
 
-    return 0; // not found
+    return (NULL); // not found
 }
 
 IfaceMgr::Iface*
@@ -217,12 +221,24 @@ IfaceMgr::getIface(const std::string& ifname) {
             return (&(*iface));
     }
 
-    return (0); // not found
+    return (NULL); // not found
 }
 
+
+/**
+ * Opens UDP/IPv6 socket and binds it to specific address, interface nad port.
+ *
+ * @param ifname name of the interface
+ * @param addr address to be bound.
+ * @param port UDP port.
+ * @param mcast Should multicast address also be bound?
+ *
+ * @return socket descriptor, if socket creation, binding and multicast
+ * group join were all successful. -1 otherwise.
+ */
 int
 IfaceMgr::openSocket(const std::string& ifname,
-                     const IOAddress & addr,
+                     const IOAddress& addr,
                      int port,
                      bool mcast) {
     struct sockaddr_storage name;
@@ -247,7 +263,7 @@ IfaceMgr::openSocket(const std::string& ifname,
 #endif
     name_len = sizeof(*addr6);
 
-    // XXX: use sockcreator once it becomes available
+    // TODO: use sockcreator once it becomes available
 
     // make a socket
     int sock = socket(AF_INET6, SOCK_DGRAM, 0);
@@ -262,12 +278,14 @@ IfaceMgr::openSocket(const std::string& ifname,
     if (setsockopt(sock, SOL_SOCKET, SO_REUSEADDR,
                    (char *)&flag, sizeof(flag)) < 0) {
         cout << "Can't set SO_REUSEADDR option on dhcpv6 socket." << endl;
+        close(sock);
         return (-1);
     }
 
     if (bind(sock, (struct sockaddr *)&name, name_len) < 0) {
         cout << "Failed to bind socket " << sock << " to " << addr.toText()
              << "/port=" << port << endl;
+        close(sock);
         return (-1);
     }
 
@@ -276,6 +294,7 @@ IfaceMgr::openSocket(const std::string& ifname,
     if (setsockopt(sock, IPPROTO_IPV6, IPV6_RECVPKTINFO,
                    &flag, sizeof(flag)) != 0) {
         cout << "setsockopt: IPV6_RECVPKTINFO failed." << endl;
+        close(sock);
         return (-1);
     }
 #else
@@ -283,6 +302,7 @@ IfaceMgr::openSocket(const std::string& ifname,
     if (setsockopt(sock, IPPROTO_IPV6, IPV6_PKTINFO,
                    &flag, sizeof(flag)) != 0) {
         cout << "setsockopt: IPV6_PKTINFO: failed." << endl;
+        close(sock);
         return (-1);
     }
 #endif
@@ -307,6 +327,15 @@ IfaceMgr::openSocket(const std::string& ifname,
     return (sock);
 }
 
+/**
+ * joins multicast group
+ *
+ * @param sock socket file descriptor
+ * @param ifname name of the interface (DHCPv6 uses link-scoped mc groups)
+ * @param mcast multicast address to join (string)
+ *
+ * @return true if joined successfully, false otherwise
+ */
 bool
 IfaceMgr::joinMcast(int sock, const std::string& ifname,
 const std::string & mcast) {
@@ -332,6 +361,17 @@ const std::string & mcast) {
     return (true);
 }
 
+/**
+ * Sends UDP packet over IPv6.
+ *
+ * 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 A packet object that is going to be sent.
+ *
+ * @return True, if transmission was successful. False otherwise.
+ */
 bool
 IfaceMgr::send(Pkt6 &pkt) {
     struct msghdr m;
@@ -405,6 +445,16 @@ IfaceMgr::send(Pkt6 &pkt) {
     return (result);
 }
 
+
+/**
+ * Attempts to receive UDP/IPv6 packet over open sockets.
+ *
+ * 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 Object prepresenting received packet.
+ */
 Pkt6*
 IfaceMgr::receive() {
     struct msghdr m;
@@ -418,7 +468,14 @@ IfaceMgr::receive() {
     char addr_str[INET6_ADDRSTRLEN];
 
     try {
-        pkt = new Pkt6(1500);
+        // RFC3315 states that server responses may be
+        // fragmented if they are over MTU. There is no
+        // text whether client's packets may be larger
+        // than 1500. Nevertheless to be on the safe side
+        // we use larger buffer. This buffer limit is checked
+        // during reception (see iov_len below), so we are
+        // safe
+        pkt = new Pkt6(65536);
     } catch (std::exception& ex) {
         cout << "Failed to create new packet." << endl;
         return (0);
@@ -495,9 +552,8 @@ IfaceMgr::receive() {
         return (0);
     }
 
-
     // That's ugly.
-    // TODO add IOAddress constructor that will take struct in6_addr* parameter
+    // TODO add IOAddress constructor that will take struct in6_addr*
     inet_ntop(AF_INET6, &to_addr, addr_str,INET6_ADDRSTRLEN);
     pkt->local_addr_ = IOAddress(string(addr_str));
 
@@ -511,8 +567,9 @@ IfaceMgr::receive() {
         pkt->iface_ = received->name_;
     } else {
         cout << "Received packet over unknown interface (ifindex="
-             << pkt->ifindex_ << endl;
-        pkt->iface_ = "[unknown]";
+             << pkt->ifindex_ << ")." << endl;
+        delete pkt;
+        return (0);
     }
 
     pkt->data_len_ = result;

src/bin/dhcp6/iface_mgr.h

@@ -30,61 +30,73 @@ namespace isc {
     class IfaceMgr {
     public:
         typedef std::list<isc::asiolink::IOAddress> Addr6Lst;
-        struct Iface { // XXX: could be a class as well
-            std::string name_;
-            int ifindex_;
+        struct Iface { // TODO: could be a class as well
+            std::string name_; // network interface name
+            int ifindex_; // interface index (a value that uniquely indentifies
+                          // an interface
             Addr6Lst addrs_;
-            char mac_[20]; //
+            char mac_[20]; // Infiniband used 20 bytes indentifiers
             int mac_len_;
 
-            Iface();
             Iface(const std::string& name, int ifindex);
             std::string getFullName() const;
             std::string getPlainMac() const;
 
+            int sendsock_; // socket used to sending data
+            int recvsock_; // socket used for receiving data
+
             // next field is not needed, let's keep it in cointainers
         };
 
-	// TODO performance improvement: we may change this into
-	//      2 maps (ifindex-indexed and name-indexed)
+        // TODO performance improvement: we may change this into
+        //      2 maps (ifindex-indexed and name-indexed) and
+        //      also hide it (make it public make tests easier for now)
         typedef std::list<Iface> IfaceLst;
 
         static IfaceMgr& instance();
-        static void instanceCreate();
 
         Iface * getIface(int ifindex);
         Iface * getIface(const std::string& ifname);
 
-        bool openSockets();
-        void printIfaces();
-
-        int openSocket(const std::string& ifname,
-                       const isc::asiolink::IOAddress& addr,
-                       int port, bool multicast);
-        bool joinMcast(int sock, const std::string& ifname,
-                       const std::string& mcast);
+        void printIfaces(std::ostream& out = std::cout);
 
         bool send(Pkt6& pkt);
         Pkt6* receive();
 
-	// don't use private, we need derived classes in tests
+        // don't use private, we need derived classes in tests
     protected:
         IfaceMgr(); // don't create IfaceMgr directly, use instance() method
         ~IfaceMgr();
 
         void detectIfaces();
 
-        // XXX: having 2 maps (ifindex->iface and ifname->iface would)
+        int openSocket(const std::string& ifname,
+                       const isc::asiolink::IOAddress& addr,
+                       int port, bool multicast);
+
+        // TODO: having 2 maps (ifindex->iface and ifname->iface would)
         //      probably be better for performance reasons
         IfaceLst ifaces_;
 
         static IfaceMgr * instance_;
 
-        int recvsock_; // XXX: should be fd_set eventually, but we have only
+        // TODO: Also keep this interface on Iface once interface detection
+        // is implemented. We may need it e.g. to close all sockets on
+        // specific interface
+        int recvsock_; // TODO: should be fd_set eventually, but we have only
         int sendsock_; // 2 sockets for now. Will do for until next release
+        // we can't use the same socket, as receiving socket
+        // is bound to multicast address. And we all know what happens
+        // to people who try to use multicast as source address.
 
         char * control_buf_;
         int control_buf_len_;
+
+    private:
+        bool openSockets();
+        static void instanceCreate();
+        bool joinMcast(int sock, const std::string& ifname,
+                       const std::string& mcast);
     };
 };
 

src/bin/dhcp6/tests/iface_mgr_unittest.cc

@@ -30,12 +30,19 @@ using namespace isc::asiolink;
 
 namespace {
 class NakedIfaceMgr: public IfaceMgr {
-    // "naked" Interface Manager, exposes internal fields 
+    // "naked" Interface Manager, exposes internal fields
 public:
     NakedIfaceMgr() { }
     IfaceLst & getIfacesLst() { return ifaces_; }
     void setSendSock(int sock) { sendsock_ = sock; }
     void setRecvSock(int sock) { recvsock_ = sock; }
+
+    int openSocket(const std::string& ifname,
+                   const isc::asiolink::IOAddress& addr,
+                   int port, bool multicast) {
+        return IfaceMgr::openSocket(ifname, addr, port, multicast);
+    }
+
 };
 
 // dummy class for now, but this will be expanded when needed
@@ -102,29 +109,29 @@ TEST_F(IfaceMgrTest, getIface) {
 
 TEST_F(IfaceMgrTest, detectIfaces) {
 
-    // test detects that interfaces can be detected 
+    // test detects that interfaces can be detected
     // there is no code for that now, but interfaces are
     // read from file
     fstream fakeifaces("interfaces.txt", ios::out);
     fakeifaces << "eth0 fe80::1234";
     fakeifaces.close();
-    
+
     // this is not usable on systems that don't have eth0
     // interfaces. Nevertheless, this fake interface should
     // be on list, but if_nametoindex() will fail.
-    
+
     IfaceMgr & ifacemgr = IfaceMgr::instance();
-    
+
     ASSERT_TRUE( ifacemgr.getIface("eth0") != NULL );
-    
+
     IfaceMgr::Iface * eth0 = ifacemgr.getIface("eth0");
-    
+
     // there should be one address
     EXPECT_EQ(1, eth0->addrs_.size());
-    
+
     IOAddress * addr = &(*eth0->addrs_.begin());
     ASSERT_TRUE( addr != NULL );
-    
+
     EXPECT_STREQ( "fe80::1234", addr->toText().c_str() );
 }
 
@@ -132,28 +139,34 @@ TEST_F(IfaceMgrTest, sockets) {
     // testing socket operation in a portable way is tricky
     // without interface detection implemented
 
-    IfaceMgr & ifacemgr = IfaceMgr::instance();
+    NakedIfaceMgr * ifacemgr = new NakedIfaceMgr();
 
     IOAddress loAddr("::1");
 
     // bind multicast socket to port 10547
-    int socket1 = ifacemgr.openSocket("lo", loAddr, 10547, true);
+    int socket1 = ifacemgr->openSocket("lo", loAddr, 10547, true);
     EXPECT_GT(socket1, 0); // socket > 0
 
     // bind unicast socket to port 10548
-    int socket2 = ifacemgr.openSocket("lo", loAddr, 10548, false);
+    int socket2 = ifacemgr->openSocket("lo", loAddr, 10548, false);
     EXPECT_GT(socket2, 0);
 
     // good to check that both sockets can be opened at once
 
     close(socket1);
     close(socket2);
+
+    delete ifacemgr;
 }
 
 TEST_F(IfaceMgrTest, sendReceive) {
     // testing socket operation in a portable way is tricky
     // without interface detection implemented
 
+    fstream fakeifaces("interfaces.txt", ios::out);
+    fakeifaces << "lo ::1";
+    fakeifaces.close();
+
     NakedIfaceMgr * ifacemgr = new NakedIfaceMgr();
 
     // let's assume that every supported OS have lo interface
@@ -168,14 +181,14 @@ TEST_F(IfaceMgrTest, sendReceive) {
 
     // prepare dummy payload
     for (int i=0;i<128; i++) {
-	sendPkt.data_[i] = i;
+        sendPkt.data_[i] = i;
     }
 
     sendPkt.remote_port_ = 10547;
     sendPkt.remote_addr_ = IOAddress("::1");
     sendPkt.ifindex_ = 1;
     sendPkt.iface_ = "lo";
-    
+
     Pkt6 * rcvPkt;
 
     EXPECT_EQ(true, ifacemgr->send(sendPkt));
@@ -186,7 +199,8 @@ TEST_F(IfaceMgrTest, sendReceive) {
 
     // let's check that we received what was sent
     EXPECT_EQ(sendPkt.data_len_, rcvPkt->data_len_);
-    EXPECT_EQ(0, memcmp(&sendPkt.data_[0], &rcvPkt->data_[0], rcvPkt->data_len_) );
+    EXPECT_EQ(0, memcmp(&sendPkt.data_[0], &rcvPkt->data_[0],
+                        rcvPkt->data_len_) );
 
     EXPECT_EQ(sendPkt.remote_addr_, rcvPkt->remote_addr_);
     EXPECT_EQ(rcvPkt->remote_port_, 10546);