[1239] Compilation fixes, tests are now passing.

src/bin/dhcp4/dhcp4_srv.cc

@@ -35,7 +35,7 @@ Dhcpv4Srv::Dhcpv4Srv(uint16_t port) {
     /// @todo: instantiate LeaseMgr here once it is imlpemented.
     IfaceMgr::instance().printIfaces();
 
-    IfaceMgr::instance().openSockets4();
+    IfaceMgr::instance().openSockets4(port);
 
     setServerID();
 

src/bin/dhcp4/tests/Makefile.am

@@ -37,10 +37,10 @@ dhcp4_unittests_SOURCES += dhcp4_srv_unittest.cc
 dhcp4_unittests_CPPFLAGS = $(AM_CPPFLAGS) $(GTEST_INCLUDES)
 dhcp4_unittests_LDFLAGS = $(AM_LDFLAGS) $(GTEST_LDFLAGS)
 dhcp4_unittests_LDADD = $(GTEST_LDADD)
-dhcp4_unittests_LDADD += $(top_builddir)/src/lib/asiolink/libasiolink.la
 dhcp4_unittests_LDADD += $(top_builddir)/src/lib/dhcp/libdhcp.la
 dhcp4_unittests_LDADD += $(top_builddir)/src/lib/exceptions/libexceptions.la
 dhcp4_unittests_LDADD += $(top_builddir)/src/lib/log/liblog.la
+dhcp4_unittests_LDADD += $(top_builddir)/src/lib/asiolink/libasiolink.la
 endif
 
 noinst_PROGRAMS = $(TESTS)

src/bin/dhcp4/tests/interfaces.txt

+eth0 fe80::1
+
+# This file is needed to make stub interface detection work.

src/lib/dhcp/iface_mgr.cc

@@ -53,9 +53,7 @@ IfaceMgr::instance() {
 }
 
 IfaceMgr::Iface::Iface(const std::string& name, int ifindex)
-    :name_(name), ifindex_(ifindex), mac_len_(0), flag_loopback_(false),
-     flag_up_(false), flag_running_(false), flag_multicast_(false),
-     flag_broadcast_(false), flags_(0), hardware_type_(0)
+    :name_(name), ifindex_(ifindex), mac_len_(0)
 {
     memset(mac_, 0, sizeof(mac_));
 }
@@ -133,13 +131,27 @@ IfaceMgr::IfaceMgr()
     }
 }
 
+void IfaceMgr::closeSockets() {
+    for (IfaceCollection::iterator iface = ifaces_.begin();
+         iface != ifaces_.end(); ++iface) {
+
+        for (SocketCollection::iterator sock = iface->sockets_.begin();
+             sock != iface->sockets_.end(); ++sock) {
+            cout << "Closing socket " << sock->sockfd_ << endl;
+            close(sock->sockfd_);
+        }
+        iface->sockets_.clear();
+    }
+
+}
+
 IfaceMgr::~IfaceMgr() {
     // control_buf_ is deleted automatically (scoped_ptr)
     control_buf_len_ = 0;
 }
 
 void
-IfaceMgr::stubDetectIfaces() {
+IfaceMgr::detectIfaces() {
     string ifaceName, linkLocal;
 
     // TODO do the actual detection. Currently interface detection is faked
@@ -178,13 +190,7 @@ IfaceMgr::stubDetectIfaces() {
     }
 }
 
-#if !defined(OS_LINUX) && !defined(OS_BSD)
-void IfaceMgr::detectIfaces() {
-    stubDetectIfaces();
-}
-#endif
-
-bool IfaceMgr::openSockets4() {
+bool IfaceMgr::openSockets4(uint16_t port) {
     int sock;
     int count = 0;
 
@@ -194,11 +200,13 @@ bool IfaceMgr::openSockets4() {
 
         cout << "Trying interface " << iface->getFullName() << endl;
 
+#if 0
         if (iface->flag_loopback_ ||
             !iface->flag_up_ ||
             !iface->flag_running_) {
             continue;
         }
+#endif
 
         AddressCollection addrs = iface->getAddresses();
 
@@ -211,8 +219,7 @@ bool IfaceMgr::openSockets4() {
                 continue;
             }
 
-            sock = openSocket(iface->getName(), *addr,
-                              DHCP4_SERVER_PORT);
+            sock = openSocket(iface->getName(), *addr, port);
             if (sock<0) {
                 cout << "Failed to open unicast socket." << endl;
                 return (false);
@@ -225,7 +232,7 @@ bool IfaceMgr::openSockets4() {
 
 }
 
-bool IfaceMgr::openSockets6() {
+bool IfaceMgr::openSockets6(uint16_t port) {
     int sock;
     int count = 0;
 
@@ -244,8 +251,7 @@ bool IfaceMgr::openSockets6() {
                 continue;
             }
 
-            sock = openSocket(iface->getName(), *addr,
-                              DHCP6_SERVER_PORT);
+            sock = openSocket(iface->getName(), *addr, port);
             if (sock<0) {
                 cout << "Failed to open unicast socket." << endl;
                 return (false);
@@ -282,18 +288,11 @@ IfaceMgr::printIfaces(std::ostream& out /*= std::cout*/) {
          ++iface) {
 
         out << "Detected interface " << iface->getFullName()
-             << ", hwtype=" << iface->hardware_type_ << ", maclen=" << iface->mac_len_
              << ", mac=" << iface->getPlainMac() << endl;
-        out << "flags=" << hex << iface->flags_ << dec << "("
-            << (iface->flag_loopback_?"LOOPBACK ":"")
-            << (iface->flag_up_?"UP ":"")
-            << (iface->flag_running_?"RUNNING ":"")
-            << (iface->flag_multicast_?"MULTICAST ":"")
-            << (iface->flag_broadcast_?"BROADCAST ":"")
-            << ")" << endl;
-        out << "  " << iface->addrs_.size() << " addr(s):" << endl;
-        for (AddressCollection::const_iterator addr=iface->addrs_.begin();
-             addr != iface->addrs_.end();
+        out << "flags=" <<  endl;
+        out << "  " << iface->getAddresses().size() << " addr(s):" << endl;
+        for (AddressCollection::const_iterator addr=iface->getAddresses().begin();
+             addr != iface->getAddresses().end();
              ++addr) {
             out << "  " << addr->toText() << endl;
         }
@@ -324,8 +323,7 @@ IfaceMgr::getIface(const std::string& ifname) {
     return (NULL); // not found
 }
 
-uint16_t
-IfaceMgr::openSocket(const std::string& ifname,
+int IfaceMgr::openSocket(const std::string& ifname,
                      const IOAddress& addr,
                      int port) {
     Iface* iface = getIface(ifname);
@@ -343,8 +341,7 @@ IfaceMgr::openSocket(const std::string& ifname,
     }
 }
 
-uint16_t
-IfaceMgr::openSocket4(Iface& iface, const IOAddress& addr, int port) {
+int IfaceMgr::openSocket4(Iface& iface, const IOAddress& addr, int port) {
 
     cout << "Creating UDP4 socket on " << iface.getFullName()
          << " " << addr.toText() << "/port=" << port << endl;
@@ -388,8 +385,7 @@ IfaceMgr::openSocket4(Iface& iface, const IOAddress& addr, int port) {
     return (sock);
 }
 
-uint16_t
-IfaceMgr::openSocket6(Iface& iface, const IOAddress& addr, int port) {
+int IfaceMgr::openSocket6(Iface& iface, const IOAddress& addr, int port) {
 
     cout << "Creating UDP6 socket on " << iface.getFullName()
          << " " << addr.toText() << "/port=" << port << endl;
@@ -656,12 +652,15 @@ IfaceMgr::receive4() {
     IfaceCollection::const_iterator iface;
     for (iface = ifaces_.begin(); iface != ifaces_.end(); ++iface) {
 
+#if 0
+        // uncomment this once #1237 is merged
         // Let's skip loopback and downed interfaces.
         if (iface->flag_loopback_ ||
             !iface->flag_up_ ||
             !iface->flag_running_) {
             continue;
         }
+#endif
 
         SocketCollection::const_iterator s = iface->sockets_.begin();
         while (s != iface->sockets_.end()) {

src/lib/dhcp/libdhcp.cc

@@ -17,6 +17,7 @@
 #include <util/buffer.h>
 #include <dhcp/libdhcp.h>
 #include "config.h"
+#include <dhcp/dhcp4.h>
 #include <dhcp/dhcp6.h>
 #include <dhcp/option.h>
 #include <dhcp/option6_ia.h>

src/lib/dhcp/pkt4.cc

@@ -50,7 +50,6 @@ Pkt4::Pkt4(uint8_t msg_type, uint32_t transid)
       bufferOut_(DHCPV4_PKT_HDR_LEN),
       msg_type_(msg_type)
 {
-    /// TODO: fixed fields, uncomment in ticket #1224
     memset(chaddr_, 0, MAX_CHADDR_LEN);
     memset(sname_, 0, MAX_SNAME_LEN);
     memset(file_, 0, MAX_FILE_LEN);
@@ -63,7 +62,6 @@ Pkt4::Pkt4(const uint8_t* data, size_t len)
       ifindex_(-1),
       local_port_(DHCP4_SERVER_PORT),
       remote_port_(DHCP4_CLIENT_PORT),
-      /// TODO Fixed fields, uncomment in ticket #1224
       op_(BOOTREQUEST),
       transid_(0),
       secs_(0),
@@ -116,8 +114,15 @@ Pkt4::pack() {
     bufferOut_.writeData(sname_, MAX_SNAME_LEN);
     bufferOut_.writeData(file_, MAX_FILE_LEN);
 
+    // write DHCP magic cookie
+    bufferOut_.writeUint32(DHCP_OPTIONS_COOKIE);
+
     LibDHCP::packOptions(bufferOut_, options_);
 
+    // add END option that indicates end of options
+    // (End option is very simple, just a 255 octet)
+    bufferOut_.writeUint8(DHO_END);
+
     return (true);
 }
 bool
@@ -147,8 +152,26 @@ Pkt4::unpack() {
     bufferIn.readData(sname_, MAX_SNAME_LEN);
     bufferIn.readData(file_, MAX_FILE_LEN);
 
+    if (bufferIn.getLength() == bufferIn.getPosition()) {
+        // this is *NOT* DHCP packet. It does not have any DHCPv4 options. In
+        // particular, it does not have magic cookie, a 4 byte sequence that
+        // differentiates between DHCP and BOOTP packets.
+        return (true);
+    }
+
+    if (bufferIn.getLength() - bufferIn.getPosition() < 4) {
+      // there is not enough data to hold magic DHCP cookie
+      isc_throw(Unexpected, "Truncated or no DHCP packet.");
+    }
+
+    uint32_t magic = bufferIn.readUint32();
+    if (magic != DHCP_OPTIONS_COOKIE) {
+      isc_throw(Unexpected, "Invalid or missing DHCP magic cookie");
+    }
+
     size_t opts_len = bufferIn.getLength() - bufferIn.getPosition();
     vector<uint8_t> optsBuffer;
+
     // fist use of readVector
     bufferIn.readVector(optsBuffer, opts_len);
     LibDHCP::unpackOptions4(optsBuffer, options_);
@@ -156,23 +179,40 @@ Pkt4::unpack() {
     return (true);
 }
 
+void Pkt4::check() {
+    boost::shared_ptr<Option> typeOpt = getOption(DHO_DHCP_MESSAGE_TYPE);
+    if (typeOpt) {
+        uint8_t msg_type = typeOpt->getUint8();
+        if (msg_type>DHCPLEASEACTIVE) {
+            isc_throw(BadValue, "Invalid DHCP message type received:" << msg_type);
+        }
+        msg_type_ = msg_type;
+
+    } else {
+        isc_throw(Unexpected, "Missing DHCP Message Type option");
+    }
+}
+
 void Pkt4::repack() {
-  cout << "Convering RX packet to TX packet: " << bufferIn_.getLength() << " bytes." << endl;
+    cout << "Convering RX packet to TX packet: " << data_.size() << " bytes." << endl;
 
-  vector<uint8_t> buf;
-  bufferIn_.readVector(buf, bufferIn_.getLength());
-  bufferOut_.writeData(&buf[0], bufferIn_.getLength());
+    bufferOut_.writeData(&data_[0], data_.size());
 }
 
 std::string
 Pkt4::toText() {
     stringstream tmp;
-    tmp << "localAddr=[" << local_addr_.toText() << "]:" << local_port_
-        << " remoteAddr=[" << remote_addr_.toText()
-        << "]:" << remote_port_ << endl;
-    tmp << "msgtype=" << msg_type_
-        << ", transid=0x" << hex << transid_ << dec
-        << endl;
+    tmp << "localAddr=" << local_addr_.toText() << ":" << local_port_
+        << " remoteAddr=" << remote_addr_.toText()
+        << ":" << remote_port_ << ", msgtype=" << int(msg_type_)
+        << ", transid=0x" << hex << transid_ << dec << endl;
+
+    for (isc::dhcp::Option::OptionCollection::iterator opt=options_.begin();
+         opt != options_.end();
+         ++opt) {
+        tmp << "  " << opt->second->toText() << std::endl;
+    }
+
 
     return tmp.str();
 }
@@ -262,7 +302,6 @@ Pkt4::getOption(uint8_t type) {
     return boost::shared_ptr<isc::dhcp::Option>(); // NULL
 }
 
-
 } // end of namespace isc::dhcp
 
 } // end of namespace isc

src/lib/dhcp/pkt4.h

@@ -78,6 +78,17 @@ public:
     bool
     unpack();
 
+    /// @brief performs sanity check on a packet.
+    ///
+    /// This is usually performed after unpack(). It checks if packet is sane:
+    /// required options are present, fields have sane content etc.
+    /// For example verifies that DHCP_MESSAGE_TYPE is present and have
+    /// reasonable value. This method is expected to grow significantly.
+    /// It makes sense to separate unpack() and check() for testing purposes.
+    ///
+    /// Method will throw exception if anomaly is found.
+    void check();
+
     /// @brief Copies content of input buffer to output buffer.
     ///
     /// This is mostly a diagnostic function. It is being used for sending

src/lib/dhcp/tests/pkt4_unittest.cc

@@ -486,13 +486,14 @@ TEST(Pkt4Test, options) {
     );
 
     const OutputBuffer& buf = pkt->getBuffer();
-    // check that all options are stored, they should take sizeof(v4Opts)
-    ASSERT_EQ(static_cast<size_t>(Pkt4::DHCPV4_PKT_HDR_LEN) + sizeof(v4Opts),
-              buf.getLength());
+    // check that all options are stored, they should take sizeof(v4Opts),
+    // DHCP magic cookie (4 bytes), and OPTION_END added (just one byte)
+    ASSERT_EQ(static_cast<size_t>(Pkt4::DHCPV4_PKT_HDR_LEN) + sizeof(DHCP_OPTIONS_COOKIE)
+              + sizeof(v4Opts) + 1, buf.getLength());
 
     // that that this extra data actually contain our options
     const uint8_t* ptr = static_cast<const uint8_t*>(buf.getData());
-    ptr += Pkt4::DHCPV4_PKT_HDR_LEN; // rewind to end of fixed part
+    ptr += Pkt4::DHCPV4_PKT_HDR_LEN + sizeof(DHCP_OPTIONS_COOKIE); // rewind to end of fixed part
     EXPECT_EQ(0, memcmp(ptr, v4Opts, sizeof(v4Opts)));
 
     EXPECT_NO_THROW(
@@ -504,6 +505,11 @@ TEST(Pkt4Test, unpackOptions) {
 
     vector<uint8_t> expectedFormat = generateTestPacket2();
 
+    expectedFormat.push_back(0x63);
+    expectedFormat.push_back(0x82);
+    expectedFormat.push_back(0x53);
+    expectedFormat.push_back(0x63);
+
     for (int i = 0; i < sizeof(v4Opts); i++) {
         expectedFormat.push_back(v4Opts[i]);
     }