[1228] Missing tests for Pkt4 implemented.

- Missing tests for Pkt4 implemented.
- Option::addOption() now supports DHCPv4 options.
- Pkt4::addOption(), getOption() implemented.
- Include paths fixed in Makefile.am

src/lib/dhcp/option.cc

@@ -232,13 +232,6 @@ Option::valid() {
     return (true);
 }
 
-void
-isc::dhcp::Option::addOption(boost::shared_ptr<isc::dhcp::Option> opt) {
-    options_.insert(pair<int, boost::shared_ptr<Option> >(opt->getType(),
-                                                            opt));
-
-}
-
 boost::shared_ptr<isc::dhcp::Option>
 Option::getOption(unsigned short opt_type) {
     isc::dhcp::Option::Option6Collection::const_iterator x =
@@ -306,6 +299,18 @@ Option::getHeaderLen() {
     return 0; // should not happen
 }
 
+void
+Option::addOption(boost::shared_ptr<Option> opt) {
+    if (universe_ == V4) {
+        // check for uniqueness (DHCPv4 options must be unique)
+        if (getOption(opt->getType())) {
+            isc_throw(BadValue, "Option " << opt->getType()
+                      << " already present in this message.");
+        }
+    }
+    options_.insert(pair<int, boost::shared_ptr<Option> >(opt->getType(), opt));
+}
+
 Option::~Option() {
 
 }

src/lib/dhcp/pkt4.cc

@@ -88,7 +88,13 @@ size_t
 Pkt4::len() {
     size_t length = DHCPV4_PKT_HDR_LEN; // DHCPv4 header
 
-    /// TODO: Include options here (ticket #1228)
+    // ... and sum of lengths of all options
+    for (Option::Option6Collection::const_iterator it = options_.begin();
+         it != options_.end();
+         ++it) {
+        length += (*it).second->len();
+    }
+
     return (length);
 }
 
@@ -109,7 +115,7 @@ Pkt4::pack() {
     bufferOut_.writeData(sname_, MAX_SNAME_LEN);
     bufferOut_.writeData(file_, MAX_FILE_LEN);
 
-    /// TODO: Options should follow here (ticket #1228)
+    LibDHCP::packOptions(bufferOut_, options_);
 
     return (true);
 }
@@ -136,7 +142,11 @@ Pkt4::unpack() {
     bufferIn_.readData(sname_, MAX_SNAME_LEN);
     bufferIn_.readData(file_, MAX_FILE_LEN);
 
-    /// TODO: Parse options here (ticket #1228)
+    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_);
 
     return (true);
 }
@@ -220,6 +230,26 @@ Pkt4::DHCPTypeToBootpType(uint8_t dhcpType) {
     }
 }
 
+void
+Pkt4::addOption(boost::shared_ptr<Option> opt) {
+    // check for uniqueness (DHCPv4 options must be unique)
+    if (getOption(opt->getType())) {
+        isc_throw(BadValue, "Option " << opt->getType()
+                  << " already present in this message.");
+    }
+    options_.insert(pair<int, boost::shared_ptr<Option> >(opt->getType(), opt));
+}
+
+boost::shared_ptr<isc::dhcp::Option>
+Pkt4::getOption(uint8_t type) {
+    Option::Option6Collection::const_iterator x = options_.find(type);
+    if (x!=options_.end()) {
+        return (*x).second;
+    }
+    return boost::shared_ptr<isc::dhcp::Option>(); // NULL
+}
+
+
 } // end of namespace isc::dhcp
 
 } // end of namespace isc

src/lib/dhcp/pkt4.h

@@ -264,7 +264,7 @@ public:
     uint8_t
     getHlen() { return (hlen_); };
 
-    /// @brief Returns chaddr field
+    /// @brief Returns chaddr field.
     ///
     /// Note: This is 16 bytes long field. It doesn't have to be
     /// null-terminated. Do no use strlen() or similar on it.
@@ -274,7 +274,7 @@ public:
     getChaddr() { return (chaddr_); };
 
 
-    /// Returns reference to output buffer
+    /// @brief Returns reference to output buffer.
     ///
     /// Returned buffer will contain reasonable data only for
     /// output (TX) packet and after pack() was called.
@@ -286,6 +286,22 @@ public:
     isc::util::OutputBuffer&
     getBuffer() { return (bufferOut_); };
 
+    /// @brief Add an option.
+    ///
+    /// Throws BadValue if option with that type is already present.
+    ///
+    /// @param opt option to be added
+    void
+    addOption(boost::shared_ptr<Option> opt);
+
+    /// @brief Returns an option of specified type.
+    ///
+    /// @return returns option of requested type (or NULL)
+    ///         if no such option is present
+
+    boost::shared_ptr<Option>
+    getOption(uint8_t opt_type);
+
 protected:
 
     /// converts DHCP message type to BOOTP op type

src/lib/dhcp/tests/Makefile.am

 SUBDIRS = .
 
 AM_CPPFLAGS = -I$(top_builddir)/src/lib -I$(top_srcdir)/src/lib
-AM_CPPFLAGS += -I$(top_srcdir)/src/lib/asiolink
-AM_CPPFLAGS += -I$(top_builddir)/src/lib/asiolink
 AM_CPPFLAGS += $(BOOST_INCLUDES)
 AM_CXXFLAGS = $(B10_CXXFLAGS)
 

src/lib/dhcp/tests/option6_addrlst_unittest.cc

@@ -15,14 +15,12 @@
 #include <config.h>
 #include <iostream>
 #include <sstream>
-
 #include <arpa/inet.h>
 #include <gtest/gtest.h>
-
-#include "io_address.h"
-#include "dhcp/dhcp6.h"
-#include "dhcp/option.h"
-#include "dhcp/option6_addrlst.h"
+#include <asiolink/io_address.h>
+#include <dhcp/dhcp6.h>
+#include <dhcp/option.h>
+#include <dhcp/option6_addrlst.h>
 
 using namespace std;
 using namespace isc;
@@ -38,10 +36,10 @@ public:
 
 TEST_F(Option6AddrLstTest, basic) {
 
-    // limiting tests to just a 2001:db8::/32 as is *wrong*.
+    // Limiting tests to just a 2001:db8::/32 as is *wrong*.
     // Good tests check corner cases as well.
     // ffff:ffff:ffff:ffff:ffff:ffff:ffff:ffff:ffff checks
-    // for integer overflow
+    // for integer overflow.
     // ff02::face:b00c checks if multicast addresses
     // can be represented properly.
 

src/lib/dhcp/tests/pkt4_unittest.cc

@@ -20,16 +20,17 @@
 #include <boost/static_assert.hpp>
 #include <boost/shared_ptr.hpp>
 #include <boost/shared_array.hpp>
-
-#include "io_address.h"
-#include "dhcp/pkt4.h"
-#include "dhcp/dhcp4.h"
-#include "exceptions/exceptions.h"
+#include <util/buffer.h>
+#include <asiolink/io_address.h>
+#include <dhcp/pkt4.h>
+#include <dhcp/dhcp4.h>
+#include <exceptions/exceptions.h>
 
 using namespace std;
 using namespace isc;
 using namespace isc::asiolink;
 using namespace isc::dhcp;
+using namespace isc::util;
 using namespace boost;
 
 namespace {
@@ -433,19 +434,127 @@ TEST(Pkt4Test, file) {
 
 }
 
+static uint8_t v4Opts[] = {
+    12,  3, 0,   1,  2,
+    13,  3, 10, 11, 12,
+    14,  3, 20, 21, 22,
+    128, 3, 30, 31, 32,
+    254, 3, 40, 41, 42
+};
+
 TEST(Pkt4Test, options) {
-    // TODO
-    ASSERT_TRUE(false);
-}
+    Pkt4* pkt = new Pkt4(DHCPOFFER, 0);
+
+    vector<uint8_t> payload[5];
+    for (int i = 0; i < 5; i++) {
+        payload[i].resize(3);
+        payload[i][0] = i*10;
+        payload[i][1] = i*10+1;
+        payload[i][2] = i*10+2;
+    }
+
+    boost::shared_ptr<Option> opt1(new Option(Option::V4, 12, payload[0]));
+    boost::shared_ptr<Option> opt2(new Option(Option::V4, 13, payload[1]));
+    boost::shared_ptr<Option> opt3(new Option(Option::V4, 14, payload[2]));
+    boost::shared_ptr<Option> opt5(new Option(Option::V4,128, payload[3]));
+    boost::shared_ptr<Option> opt4(new Option(Option::V4,254, payload[4]));
+
+    pkt->addOption(opt1);
+    pkt->addOption(opt2);
+    pkt->addOption(opt3);
+    pkt->addOption(opt4);
+    pkt->addOption(opt5);
+
+    EXPECT_TRUE(pkt->getOption(12));
+    EXPECT_TRUE(pkt->getOption(13));
+    EXPECT_TRUE(pkt->getOption(14));
+    EXPECT_TRUE(pkt->getOption(128));
+    EXPECT_TRUE(pkt->getOption(254));
+    EXPECT_FALSE(pkt->getOption(127)); //  no such option
+
+    // options are unique in DHCPv4. It should not be possible
+    // to add more than one option of the same type.
+    EXPECT_THROW(
+        pkt->addOption(opt1),
+        BadValue
+    );
 
-TEST(Pkt4Test, packOptions) {
-    // TODO
-    ASSERT_TRUE(false);
+    EXPECT_NO_THROW(
+        pkt->pack();
+    );
+
+    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());
+
+    // 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
+    EXPECT_EQ(0, memcmp(ptr, v4Opts, sizeof(v4Opts)));
+
+    EXPECT_NO_THROW(
+        delete pkt;
+    );
 }
 
 TEST(Pkt4Test, unpackOptions) {
-    // TODO
-    ASSERT_TRUE(false);
+
+    vector<uint8_t> expectedFormat = generateTestPacket2();
+
+    for (int i=0; i < sizeof(v4Opts); i++) {
+        expectedFormat.push_back(v4Opts[i]);
+    }
+
+    // now expectedFormat contains fixed format and 5 options
+
+    shared_ptr<Pkt4> pkt(new Pkt4(&expectedFormat[0],
+                                  expectedFormat.size()));
+
+    EXPECT_NO_THROW(
+        pkt->unpack()
+    );
+
+    EXPECT_TRUE(pkt->getOption(12));
+    EXPECT_TRUE(pkt->getOption(13));
+    EXPECT_TRUE(pkt->getOption(14));
+    EXPECT_TRUE(pkt->getOption(128));
+    EXPECT_TRUE(pkt->getOption(254));
+
+    shared_ptr<Option> x = pkt->getOption(12);
+    ASSERT_TRUE(x); // option 1 should exist
+    EXPECT_EQ(12, x->getType());  // this should be option 12
+    ASSERT_EQ(3, x->getData().size()); // it should be of length 3
+    EXPECT_EQ(5, x->len()); // total option length 5
+    EXPECT_EQ(0, memcmp(&x->getData()[0], v4Opts+2, 3)); // data len=3
+
+    x = pkt->getOption(13);
+    ASSERT_TRUE(x); // option 13 should exist
+    EXPECT_EQ(13, x->getType());  // this should be option 13
+    ASSERT_EQ(3, x->getData().size()); // it should be of length 3
+    EXPECT_EQ(5, x->len()); // total option length 5
+    EXPECT_EQ(0, memcmp(&x->getData()[0], v4Opts+7, 3)); // data len=3
+
+    x = pkt->getOption(14);
+    ASSERT_TRUE(x); // option 14 should exist
+    EXPECT_EQ(14, x->getType());  // this should be option 14
+    ASSERT_EQ(3, x->getData().size()); // it should be of length 3
+    EXPECT_EQ(5, x->len()); // total option length 5
+    EXPECT_EQ(0, memcmp(&x->getData()[0], v4Opts+12, 3)); // data len=3
+
+    x = pkt->getOption(128);
+    ASSERT_TRUE(x); // option 3 should exist
+    EXPECT_EQ(128, x->getType());  // this should be option 254
+    ASSERT_EQ(3, x->getData().size()); // it should be of length 3
+    EXPECT_EQ(5, x->len()); // total option length 5
+    EXPECT_EQ(0, memcmp(&x->getData()[0], v4Opts+17, 3)); // data len=3
+
+    x = pkt->getOption(254);
+    ASSERT_TRUE(x); // option 3 should exist
+    EXPECT_EQ(254, x->getType());  // this should be option 254
+    ASSERT_EQ(3, x->getData().size()); // it should be of length 3
+    EXPECT_EQ(5, x->len()); // total option length 5
+    EXPECT_EQ(0, memcmp(&x->getData()[0], v4Opts+22, 3)); // data len=3
 }
 
 } // end of anonymous namespace

src/lib/dhcp/tests/pkt6_unittest.cc

@@ -18,10 +18,10 @@
 #include <arpa/inet.h>
 #include <gtest/gtest.h>
 
-#include "io_address.h"
-#include "dhcp/option.h"
-#include "dhcp/pkt6.h"
-#include "dhcp/dhcp6.h"
+#include <asiolink/io_address.h>
+#include <dhcp/option.h>
+#include <dhcp/pkt6.h>
+#include <dhcp/dhcp6.h>
 
 using namespace std;
 using namespace isc;