[1540] Last changes after review.

src/lib/dhcp/iface_mgr.cc

@@ -591,7 +591,7 @@ IfaceMgr::send(const Pkt6Ptr& pkt) {
     cmsg->cmsg_type = IPV6_PKTINFO;
     cmsg->cmsg_len = CMSG_LEN(sizeof(struct in6_pktinfo));
     struct in6_pktinfo *pktinfo = convertPktInfo6(CMSG_DATA(cmsg));
-    memset(pktinfo, 0, sizeof(*pktinfo));
+    memset(pktinfo, 0, sizeof(struct in6_pktinfo));
     pktinfo->ipi6_ifindex = pkt->getIndex();
     m.msg_controllen = cmsg->cmsg_len;
 
@@ -707,7 +707,6 @@ IfaceMgr::receive4() {
 
     // Now we have a socket, let's get some data from it!
     struct sockaddr_in from_addr;
-    const uint32_t RCVBUFSIZE = 1500;
     static uint8_t buf[RCVBUFSIZE];
 
     memset(&control_buf_[0], 0, control_buf_len_);
@@ -773,19 +772,10 @@ IfaceMgr::receive4() {
 }
 
 Pkt6Ptr IfaceMgr::receive6() {
-    int result;
-    struct sockaddr_in6 from;
-    int ifindex = -1;
-
-    // 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. For now, we can assume that
-    // we don't support packets larger than 1500.
-    const uint32_t RCVBUFSIZE = 1500;
     static uint8_t buf[RCVBUFSIZE];
 
     memset(&control_buf_[0], 0, control_buf_len_);
+    struct sockaddr_in6 from;
     memset(&from, 0, sizeof(from));
 
     // Initialize our message header structure.
@@ -851,11 +841,12 @@ Pkt6Ptr IfaceMgr::receive6() {
     cout << "Trying to receive over UDP6 socket " << candidate->sockfd_ << " bound to "
          << candidate->addr_.toText() << "/port=" << candidate->port_ << " on "
          << iface->getFullName() << endl;
-    result = recvmsg(candidate->sockfd_, &m, 0);
+    int result = recvmsg(candidate->sockfd_, &m, 0);
 
     struct in6_addr to_addr;
     memset(&to_addr, 0, sizeof(to_addr));
 
+    int ifindex = -1;
     if (result >= 0) {
         struct in6_pktinfo* pktinfo = NULL;
 
@@ -866,7 +857,7 @@ Pkt6Ptr IfaceMgr::receive6() {
         //
         // 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;
+        bool found_pktinfo = false;
         struct cmsghdr* cmsg = CMSG_FIRSTHDR(&m);
         while (cmsg != NULL) {
             if ((cmsg->cmsg_level == IPPROTO_IPV6) &&
@@ -874,7 +865,8 @@ Pkt6Ptr IfaceMgr::receive6() {
                 pktinfo = convertPktInfo6(CMSG_DATA(cmsg));
                 to_addr = pktinfo->ipi6_addr;
                 ifindex = pktinfo->ipi6_ifindex;
-                found_pktinfo = 1;
+                found_pktinfo = true;
+                break;
             }
             cmsg = CMSG_NXTHDR(&m, cmsg);
         }

src/lib/dhcp/iface_mgr.h

@@ -42,6 +42,15 @@ public:
     /// maximum MAC address length (Infiniband uses 20 bytes)
     static const unsigned int MAX_MAC_LEN = 20;
 
+    /// @brief Packet reception buffer size
+    ///
+    /// 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. For now, we can assume that
+    /// we don't support packets larger than 1500.
+    static const uint32_t RCVBUFSIZE = 1500;
+
     /// Holds information about socket.
     struct SocketInfo {
         uint16_t sockfd_; /// socket descriptor

src/lib/dhcp/iface_mgr_linux.cc

@@ -20,6 +20,7 @@
 #include <dhcp/iface_mgr.h>
 #include <exceptions/exceptions.h>
 
+#include <stdint.h>
 #include <net/if.h>
 #include <linux/if_link.h>
 #include <linux/rtnetlink.h>
@@ -29,14 +30,27 @@ using namespace isc;
 using namespace isc::asiolink;
 using namespace isc::dhcp;
 
+/// @brief This code uses netlink interface. See RFC3549 for details.
+///
+/// For detailed description, see http://en.wikipedia.org/wiki/Netlink
+/// or RFC3549. According to Wikipedia: "Netlink was designed for and
+/// is used to transfer miscellaneous networking information between
+/// the Linux kernel space and user space processes. Many networking
+/// utilities use Netlink to communicate with the Linux kernel from
+/// user space, for example iproute2. Netlink consists of a standard
+/// socket-based interface for user space processes and an internal
+/// kernel API for kernel modules. It is designed to be a more
+/// flexible successor to ioctl. Originally, Netlink uses the
+/// AF_NETLINK socket family."
+
 /// This is a structure that defines context for netlink connection.
 struct rtnl_handle
 {
-    int                fd;
+    int fd;
     struct sockaddr_nl local;
     struct sockaddr_nl peer;
-    __u32              seq;
+    uint32_t seq;
-    __u32              dump;
+    uint32_t dump;
 };
 
 struct nlmsg_list
@@ -45,8 +59,8 @@ struct nlmsg_list
     struct nlmsghdr h;
 };
 
-const int sndbuf = 32768;
+const int SNDBUFSIZE = 32768;
-const int rcvbuf = 32768;
+const int RCVBUFSIZE = 32768;
 
 namespace isc {
 
@@ -63,11 +77,11 @@ void rtnl_open_socket(struct rtnl_handle& handle) {
         isc_throw(Unexpected, "Failed to create NETLINK socket.");
     }
 
-    if (setsockopt(handle.fd, SOL_SOCKET, SO_SNDBUF, &sndbuf, sizeof(sndbuf)) < 0) {
+    if (setsockopt(handle.fd, SOL_SOCKET, SO_SNDBUF, &SNDBUFSIZE, sizeof(SNDBUFSIZE)) < 0) {
         isc_throw(Unexpected, "Failed to set send buffer in NETLINK socket.");
     }
 
-    if (setsockopt(handle.fd, SOL_SOCKET, SO_RCVBUF, &sndbuf, sizeof(rcvbuf)) < 0) {
+    if (setsockopt(handle.fd, SOL_SOCKET, SO_RCVBUF, &RCVBUFSIZE, sizeof(RCVBUFSIZE)) < 0) {
         isc_throw(Unexpected, "Failed to set receive buffer in NETLINK socket.");
     }
 
@@ -212,7 +226,7 @@ void rtnl_process_reply(struct rtnl_handle &rth, struct nlmsg_list *&info) {
     msg.msg_iov = &iov;
     msg.msg_iovlen = 1;
 
-    char buf[rcvbuf];
+    char buf[RCVBUFSIZE];
 
     iov.iov_base = buf;
     while (1) {

src/lib/dhcp/tests/option_unittest.cc

@@ -65,10 +65,9 @@ TEST_F(OptionTest, v4_basic) {
         opt = new Option(Option::V4, 256),
         BadValue
     );
-    if (opt) {
+
-        delete opt;
+    delete opt;
-        opt = 0;
+    opt = 0;
-    }
 
     // 0 is a special PAD option
     EXPECT_THROW(
@@ -76,10 +75,8 @@ TEST_F(OptionTest, v4_basic) {
         BadValue
     );
 
-    if (opt) {
+    delete opt;
-        delete opt;
+    opt = 0;
-        opt = 0;
-    }
 
     // 255 is a special END option
     EXPECT_THROW(
@@ -87,10 +84,8 @@ TEST_F(OptionTest, v4_basic) {
         BadValue
     );
 
-    if (opt) {
+    delete opt;
-        delete opt;
+    opt = 0;
-        opt = 0;
-    }
 }
 
 const uint8_t dummyPayload[] =

src/lib/dhcp/tests/pkt6_unittest.cc

@@ -46,38 +46,43 @@ TEST_F(Pkt6Test, constructor) {
     delete pkt1;
 }
 
-// captured actual SOLICIT packet: transid=0x3d79fb
+/// @brief returns captured actual SOLICIT packet
-// options: client-id, in_na, dns-server, elapsed-time, option-request
+///
-// this code is autogenerated (see src/bin/dhcp6/tests/iface_mgr_unittest.c)
+/// Captured SOLICIT packet with transid=0x3d79fb and options: client-id,
+/// in_na, dns-server, elapsed-time, option-request
+/// This code was autogenerated (see src/bin/dhcp6/tests/iface_mgr_unittest.c),
+/// but we spent some time to make is less ugly than it used to be.
+///
+/// @return pointer to Pkt6 that represents received SOLICIT
 Pkt6* capture1() {
     Pkt6* pkt;
     uint8_t data[98];
-    data[0]=1;
+    data[0]  = 1;
-    data[1]=01;       data[2]=02;     data[3]=03;     data[4]=0;
+    data[1]  = 1;       data[2]  = 2;     data[3] = 3;      data[4]  = 0;
-    data[5]=1;        data[6]=0;      data[7]=14;     data[8]=0;
+    data[5]  = 1;       data[6]  = 0;     data[7] = 14;     data[8]  = 0;
-    data[9]=1;        data[10]=0;     data[11]=1;     data[12]=21;
+    data[9]  = 1;       data[10] = 0;     data[11] = 1;     data[12] = 21;
-    data[13]=158;     data[14]=60;    data[15]=22;    data[16]=0;
+    data[13] = 158;     data[14] = 60;    data[15] = 22;    data[16] = 0;
-    data[17]=30;      data[18]=140;   data[19]=155;   data[20]=115;
+    data[17] = 30;      data[18] = 140;   data[19] = 155;   data[20] = 115;
-    data[21]=73;      data[22]=0;     data[23]=3;     data[24]=0;
+    data[21] = 73;      data[22] = 0;     data[23] = 3;     data[24] = 0;
-    data[25]=40;      data[26]=0;     data[27]=0;     data[28]=0;
+    data[25] = 40;      data[26] = 0;     data[27] = 0;     data[28] = 0;
-    data[29]=1;       data[30]=255;   data[31]=255;   data[32]=255;
+    data[29] = 1;       data[30] = 255;   data[31] = 255;   data[32] = 255;
-    data[33]=255;     data[34]=255;   data[35]=255;   data[36]=255;
+    data[33] = 255;     data[34] = 255;   data[35] = 255;   data[36] = 255;
-    data[37]=255;     data[38]=0;     data[39]=5;     data[40]=0;
+    data[37] = 255;     data[38] = 0;     data[39] = 5;     data[40] = 0;
-    data[41]=24;      data[42]=32;    data[43]=1;     data[44]=13;
+    data[41] = 24;      data[42] = 32;    data[43] = 1;     data[44] = 13;
-    data[45]=184;     data[46]=0;     data[47]=1;     data[48]=0;
+    data[45] = 184;     data[46] = 0;     data[47] = 1;     data[48] = 0;
-    data[49]=0;       data[50]=0;     data[51]=0;     data[52]=0;
+    data[49] = 0;       data[50] = 0;     data[51] = 0;     data[52] = 0;
-    data[53]=0;       data[54]=0;     data[55]=0;     data[56]=18;
+    data[53] = 0;       data[54] = 0;     data[55] = 0;     data[56] = 18;
-    data[57]=52;      data[58]=255;   data[59]=255;   data[60]=255;
+    data[57] = 52;      data[58] = 255;   data[59] = 255;   data[60] = 255;
-    data[61]=255;     data[62]=255;   data[63]=255;   data[64]=255;
+    data[61] = 255;     data[62] = 255;   data[63] = 255;   data[64] = 255;
-    data[65]=255;     data[66]=0;     data[67]=23;    data[68]=0;
+    data[65] = 255;     data[66] = 0;     data[67] = 23;    data[68] = 0;
-    data[69]=16;      data[70]=32;    data[71]=1;     data[72]=13;
+    data[69] = 16;      data[70] = 32;    data[71] = 1;     data[72] = 13;
-    data[73]=184;     data[74]=0;     data[75]=1;     data[76]=0;
+    data[73] = 184;     data[74] = 0;     data[75] = 1;     data[76] = 0;
-    data[77]=0;       data[78]=0;     data[79]=0;     data[80]=0;
+    data[77] = 0;       data[78] = 0;     data[79] = 0;     data[80] = 0;
-    data[81]=0;       data[82]=0;     data[83]=0;     data[84]=221;
+    data[81] = 0;       data[82] = 0;     data[83] = 0;     data[84] = 221;
-    data[85]=221;     data[86]=0;     data[87]=8;     data[88]=0;
+    data[85] = 221;     data[86] = 0;     data[87] = 8;     data[88] = 0;
-    data[89]=2;       data[90]=0;     data[91]=100;   data[92]=0;
+    data[89] = 2;       data[90] = 0;     data[91] = 100;   data[92] = 0;
-    data[93]=6;       data[94]=0;     data[95]=2;     data[96]=0;
+    data[93] = 6;       data[94] = 0;     data[95] = 2;     data[96] = 0;
-    data[97]=23;
+    data[97] = 23;
 
     pkt = new Pkt6(data, sizeof(data));
     pkt->setRemotePort(546);
@@ -142,7 +147,7 @@ TEST_F(Pkt6Test, packUnpack) {
               parent->len());
 
     // create second packet,based on assembled data from the first one
-    Pkt6* clone = new Pkt6((const uint8_t*)parent->getBuffer().getData(),
+    Pkt6* clone = new Pkt6(static_cast<const uint8_t*>(parent->getBuffer().getData()),
                            parent->getBuffer().getLength());
 
     // now recreate options list