[1228] Work on option support in DHCPv4

- More code migrated to vector<uint8_t>
- Option4Collection type deemed unnecessary and removed
- Removed data_len_ field. data_.size() will be used instead
- unitests for new functionalities in Option class implemented

src/lib/dhcp/libdhcp.cc

@@ -14,16 +14,17 @@
 
 #include <boost/shared_array.hpp>
 #include <boost/shared_ptr.hpp>
-#include "dhcp/libdhcp.h"
+#include <util/buffer.h>
+#include <dhcp/libdhcp.h>
 #include "config.h"
-#include "dhcp6.h"
-
-#include "option.h"
-#include "option6_ia.h"
-#include "option6_iaaddr.h"
+#include <dhcp/dhcp6.h>
+#include <dhcp/option.h>
+#include <dhcp/option6_ia.h>
+#include <dhcp/option6_iaaddr.h>
 
 using namespace std;
 using namespace isc::dhcp;
+using namespace isc::util;
 
 // static array with factories for options
 std::map<unsigned short, Option::Factory*> LibDHCP::v6factories_;
@@ -89,7 +90,7 @@ LibDHCP::packOptions6(boost::shared_array<uint8_t> data,
                       unsigned int offset,
                       const isc::dhcp::Option::Option6Collection& options) {
     try {
-        for (isc::dhcp::Option::Option6Collection::const_iterator it = options.begin();
+        for (Option::Option6Collection::const_iterator it = options.begin();
              it != options.end();
              ++it) {
             unsigned short opt_len = (*it).second->len();
@@ -97,7 +98,7 @@ LibDHCP::packOptions6(boost::shared_array<uint8_t> data,
                 isc_throw(OutOfRange, "Failed to build option " <<
                           (*it).first << ": out of buffer");
             }
-            offset = (*it).second->pack(data, data_len, offset);
+            offset = it->second->pack(data, data_len, offset);
         }
     }
     catch (const Exception& e) {
@@ -107,6 +108,17 @@ LibDHCP::packOptions6(boost::shared_array<uint8_t> data,
     return (offset);
 }
 
+void
+LibDHCP::packOptions(isc::util::OutputBuffer& buf,
+                     const Option::Option6Collection& options) {
+    for (Option::Option6Collection::const_iterator it = options.begin();
+         it != options.end();
+         ++it) {
+        it->second->pack4(buf);
+    }
+}
+
+
 bool
 LibDHCP::OptionFactoryRegister(Option::Universe u,
                                unsigned short opt_type,

src/lib/dhcp/libdhcp.h

@@ -16,7 +16,8 @@
 #define LIBDHCP_H_
 
 #include <iostream>
-#include "dhcp/pkt6.h"
+#include <util/buffer.h>
+#include <dhcp/pkt6.h>
 
 namespace isc {
 namespace dhcp {
@@ -41,6 +42,22 @@ public:
                  unsigned int offset,
                  const isc::dhcp::Option::Option6Collection& options);
 
+
+    /// @brief Stores options in a buffer.
+    ///
+    /// Stores all options defined in options containers in a on-wire
+    /// format in output buffer specified by buf.
+    ///
+    /// May throw different exceptions if option assembly fails. There
+    /// may be different reasons (option too large, option malformed,
+    /// too many options etc.)
+    ///
+    /// @param buf
+    /// @param options
+    static void
+    packOptions(isc::util::OutputBuffer& buf,
+                const isc::dhcp::Option::Option6Collection& options);
+
     ///
     /// Parses provided buffer and creates Option objects.
     ///

src/lib/dhcp/option.cc

@@ -29,16 +29,19 @@ using namespace isc::dhcp;
 using namespace isc::util;
 
 Option::Option(Universe u, unsigned short type)
-    :universe_(u), type_(type), data_len_(0) {
-
+    :universe_(u), type_(type) {
 
+    if (u==V4 && (type>255)) {
+        isc_throw(BadValue, "Can't create V4 option of type "
+                  << type << ", V4 options are in range 0..255");
+    }
 }
 
 Option::Option(Universe u, unsigned short type,
                const boost::shared_array<uint8_t>& buf,
                unsigned int offset, unsigned int len)
     :universe_(u), type_(type),
-     data_len_(len), offset_(offset)
+     offset_(offset)
 {
     if (u==V4 && (type>255)) {
         isc_throw(BadValue, "Can't create V4 option of type "
@@ -54,7 +57,13 @@ Option::Option(Universe u, unsigned short type,
 
 Option::Option(Universe u, unsigned short type, std::vector<uint8_t>& data)
     :universe_(u), type_(type), data_(data) {
-
+    if (u==V4 && data.size() > 255) {
+        isc_throw(OutOfRange, "DHCPv4 Option " << type_ << " is too big."
+                  << "At most 255 bytes are supported.");
+        /// TODO Larger options can be stored as separate instances
+        /// of DHCPv4 options. Clients MUST concatenate them.
+        /// Fortunately, there are no such large options used today.
+    }
 }
 
 
@@ -62,28 +71,56 @@ unsigned int
 Option::pack(boost::shared_array<uint8_t>& buf,
              unsigned int buf_len,
              unsigned int offset) {
+    if (universe_ != V6) {
+        isc_throw(BadValue, "Failed to pack " << type_ << " option. Do not "
+                  << "use this method for options other than DHCPv6.");
+    }
+    return pack6(buf, buf_len, offset);
+}
+
+void
+Option::pack4(isc::util::OutputBuffer& buf) {
     switch (universe_) {
-    case V4:
-        return pack4(buf, buf_len, offset);
+    case V4: {
+        if (data_.size()>255) {
+            isc_throw(OutOfRange, "DHCPv4 Option " << type_ << " is too big."
+                      << "At most 255 bytes are supported.");
+            /// TODO Larger options can be stored as separate instances
+            /// of DHCPv4 options. Clients MUST concatenate them.
+            /// Fortunately, there are no such large options used today.
+        }
+
+        buf.writeUint8(type_);
+        buf.writeUint8(len() - getHeaderLen());
+
+        buf.writeData(&data_[0], data_.size());
+
+        LibDHCP::packOptions(buf, options_);
+        return;
+    }
     case V6:
-        return pack6(buf, buf_len, offset);
+        /// TODO: Do we need a sanity check for option size here?
+        buf.writeUint16(type_);
+        buf.writeUint16(len() - getHeaderLen());
+
+        LibDHCP::packOptions(buf, options_);
+        return;
     default:
-        isc_throw(BadValue, "Unknown universe defined for Option " << type_);
+        isc_throw(OutOfRange, "Invalid universe type" << universe_);
     }
 }
 
-
 unsigned int
 Option::pack4(boost::shared_array<uint8_t>& buf,
              unsigned int buf_len,
              unsigned int offset) {
     if ( offset+len() > buf_len ) {
         isc_throw(OutOfRange, "Failed to pack v4 option=" <<
-                  type_ << ",len=" << data_len_ << ": too small buffer.");
+                  type_ << ",len=" << len() << ": too small buffer.");
     }
     uint8_t *ptr = &buf[offset];
     ptr[0] = type_;
-    ptr[1] = data_len_;
+    ptr[1] = len() - getHeaderLen();
     ptr += 2;
     memcpy(ptr, &data_[0], data_.size());
 
@@ -105,11 +142,11 @@ Option::pack6(boost::shared_array<uint8_t>& buf,
 
     ptr = writeUint16(len() - getHeaderLen(), ptr);
 
-    if (data_len_)
+    if (data_.size())
         memcpy(ptr, &data_[0], data_.size());
 
     // end of fixed part of this option
-    offset += OPTION6_HDR_LEN + data_len_;
+    offset += OPTION6_HDR_LEN + data_.size();
 
     return LibDHCP::packOptions6(buf, buf_len, offset, options_);
 }
@@ -157,7 +194,6 @@ Option::unpack6(const boost::shared_array<uint8_t>& buf,
     data_ = std::vector<uint8_t>(ptr, ptr+parse_len);
 
     offset_ = offset;
-    data_len_ = buf_len;
 
     return (offset+parse_len);
 
@@ -165,11 +201,13 @@ Option::unpack6(const boost::shared_array<uint8_t>& buf,
     //                               options_);
 }
 
+/// Returns length of the complete option (data length + DHCPv4/DHCPv6
+/// option header)
 unsigned short
 Option::len() {
 
     // length of the whole option is header and data stored in this option...
-    int length = getHeaderLen() + data_len_;
+    int length = getHeaderLen() + data_.size();
 
     // ... and sum of lengths of all suboptions
     for (Option::Option6Collection::iterator it = options_.begin();
@@ -228,9 +266,9 @@ std::string Option::toText(int indent /* =0 */ ) {
     for (int i=0; i<indent; i++)
         tmp << " ";
 
-    tmp << "type=" << type_ << ", len=" << data_len_ << ": ";
+    tmp << "type=" << type_ << ", len=" << len()-getHeaderLen() << ": ";
 
-    for (unsigned int i=0; i<data_len_; i++) {
+    for (unsigned int i=0; i<data_.size(); i++) {
         if (i) {
             tmp << ":";
         }

src/lib/dhcp/option.h

@@ -20,6 +20,7 @@
 #include <vector>
 #include <boost/shared_ptr.hpp>
 #include <boost/shared_array.hpp>
+#include <util/buffer.h>
 
 namespace isc {
 namespace dhcp {
@@ -35,10 +36,6 @@ public:
     /// defines option universe DHCPv4 or DHCPv6
     enum Universe { V4, V6 };
 
-    /// a collection of DHCPv4 options
-    typedef std::map<unsigned int, boost::shared_ptr<Option> >
-    Option4Collection;
-
     /// a collection of DHCPv6 options
     typedef std::multimap<unsigned int, boost::shared_ptr<Option> >
     Option6Collection;
@@ -99,11 +96,13 @@ public:
     Universe
     getUniverse() { return universe_; };
 
-    /// @brief writes option in wire-format to buf
+    /// @brief Writes option in wire-format to a buffer.
     ///
     /// Writes option in wire-format to buffer, returns pointer to first unused
     /// byte after stored option (that is useful for writing options one after
-    /// another)
+    /// another). Used in DHCPv6 options.
+    ///
+    /// TODO: Migrate DHCPv6 code to pack(OutputBuffer& buf) version
     ///
     /// @param buf pointer to a buffer
     /// @param buf_len length of the buffer
@@ -116,6 +115,18 @@ public:
          unsigned int buf_len,
          unsigned int offset);
 
+    /// @brief Writes option in a wire-format to a buffer.
+    ///
+    /// Method will throw if option storing fails for some reason.
+    ///
+    /// TODO Once old (DHCPv6) implementation is rewritten,
+    /// unify pack4() and pack6() and rename them to just pack().
+    ///
+    /// @param buf output buffer (option will be stored there)
+    virtual void
+    pack4(isc::util::OutputBuffer& buf);
+
+
     /// @brief Parses buffer.
     ///
     /// Parses received buffer, returns offset to the first unused byte after
@@ -270,10 +281,8 @@ protected:
     /// contains content of this data
     std::vector<uint8_t> data_;
 
-    /// length of data only. Use len() if you want to
-    /// know proper length with option header overhead
-    unsigned int data_len_;
-
+    /// TODO: Remove this field. vector<uint8_t> should be used
+    /// instead.
     /// data is a shared_pointer that points out to the
     /// whole packet. offset_ specifies where data for
     /// this option begins.

src/lib/dhcp/pkt4.h

@@ -382,7 +382,7 @@ protected:
     uint8_t msg_type_;
 
     /// collection of options present in this message
-    isc::dhcp::Option::Option4Collection options_;
+    isc::dhcp::Option::Option6Collection options_;
 }; // Pkt4 class
 
 } // isc::dhcp namespace

src/lib/dhcp/tests/option_unittest.cc

@@ -20,6 +20,7 @@
 #include <gtest/gtest.h>
 #include <boost/shared_ptr.hpp>
 #include <exceptions/exceptions.h>
+#include <util/buffer.h>
 
 #include "dhcp/dhcp6.h"
 #include "dhcp/option.h"
@@ -27,6 +28,7 @@
 using namespace std;
 using namespace isc;
 using namespace isc::dhcp;
+using namespace isc::util;
 
 namespace {
 class OptionTest : public ::testing::Test {
@@ -72,30 +74,56 @@ TEST_F(OptionTest, v4_data) {
     vector<uint8_t> data(dummyPayload, dummyPayload + sizeof(dummyPayload));
 
     Option* opt = 0;
+
+    // create DHCPv4 option of type 123
+    // that contains 4 bytes of data
     ASSERT_NO_THROW(
         opt= new Option(Option::V4,
                         123, // type
                         data);
     );
 
+    // check that content is reported properly
     EXPECT_EQ(123, opt->getType());
     vector<uint8_t> optData = opt->getData();
     ASSERT_EQ(optData.size(), data.size());
-
     EXPECT_EQ(optData, data);
+    EXPECT_EQ(2, opt->getHeaderLen());
+    EXPECT_EQ(6, opt->len());
 
-    // TODO
-    ASSERT_TRUE(false);
-}
+    // now store that option into a buffer
+    OutputBuffer buf(100);
+    EXPECT_NO_THROW(
+        opt->pack4(buf);
+    );
+
+    // check content of that buffer
+
+    // 2 byte header + 4 bytes data
+    ASSERT_EQ(6, buf.getLength());
+
+    // that's how this option is supposed to look like
+    uint8_t exp[] = { 123, 4, 1, 2, 3, 4 };
+
+    /// TODO: use vector<uint8_t> getData() when it will be implemented
+    EXPECT_EQ(0, memcmp(exp, buf.getData(), 6));
 
-TEST_F(OptionTest, v4_addgetdel) {
-    // TODO
-    ASSERT_TRUE(false);
+    // check that we can destroy that option
+    EXPECT_NO_THROW(
+        delete opt;
+    );
 }
 
 TEST_F(OptionTest, v4_toText) {
-    // TODO
-    ASSERT_TRUE(false);
+
+    vector<uint8_t> buf(3);
+    buf[0] = 0;
+    buf[1] = 0xf;
+    buf[2] = 0xff;
+
+    Option opt(Option::V4, 253, buf);
+
+    EXPECT_EQ("type=253, len=3: 00:0f:ff", opt.toText());
 }
 
 // tests simple constructor
@@ -126,7 +154,7 @@ TEST_F(OptionTest, v6_data1) {
     EXPECT_EQ(333, opt->getType());
 
     ASSERT_EQ(11, opt->len());
-    ASSERT_EQ(11, opt->getData().size());
+    ASSERT_EQ(7, opt->getData().size());
     EXPECT_EQ(0, memcmp(&buf[3], &opt->getData()[0], 7) );
 
     int offset = opt->pack(buf, 32, 20);