[1228] Changes after review.

ChangeLog entry added (it covers all recent DHCPv4 tickets)
New constructor in Option class.
Added new unittest for new construtor.
Minor improvements in pkt4_unitests.

ChangeLog

@@ -1,3 +1,11 @@
+ 304.	[func]		tomek
+	libdhcp: Support for DHCPv4 packet manipulation is now implemented.
+	All fixed fields are now supported. Generic support for DHCPv4
+	options is available (both parsing and assembly). There is no code
+	that uses this new functionality yet, so it is not usable directly
+	at this time. This code will be used by upcoming b10-dhcp4 daemon.
+	(Trac #1228, git TBD)
+
 303.	[bug]		jinmei
 	Changed the installation path for the UNIX domain file used
 	for the communication between b10-auth and b10-xfrout to a

src/lib/dhcp/libdhcp.cc

@@ -102,11 +102,9 @@ LibDHCP::unpackOptions4(const std::vector<uint8_t>& buf,
         boost::shared_ptr<Option> opt;
         switch(opt_type) {
         default:
-            /// TODO: Is this intermediate object really necessary here?
-            vector<uint8_t> tmpVec(&buf[offset], &buf[offset] + opt_len);
-            opt = boost::shared_ptr<Option>(new Option(Option::V4,
-                                                       opt_type,
-                                                       tmpVec));
+            opt = boost::shared_ptr<Option>(new Option(Option::V4, opt_type,
+                                                       buf.begin()+offset,
+                                                       buf.begin()+offset+opt_len));
         }
 
         options.insert(pair<int, boost::shared_ptr<Option> >(opt_type, opt));
@@ -114,7 +112,6 @@ LibDHCP::unpackOptions4(const std::vector<uint8_t>& buf,
     }
 }
 
-
 unsigned int
 LibDHCP::packOptions6(boost::shared_array<uint8_t> data,
                       unsigned int data_len,

src/lib/dhcp/option.cc

@@ -31,7 +31,7 @@ using namespace isc::util;
 Option::Option(Universe u, unsigned short type)
     :universe_(u), type_(type) {
 
-    if (u==V4 && (type>255)) {
+    if ((u == V4) && (type > 255)) {
         isc_throw(BadValue, "Can't create V4 option of type "
                   << type << ", V4 options are in range 0..255");
     }
@@ -43,13 +43,13 @@ Option::Option(Universe u, unsigned short type,
     :universe_(u), type_(type),
      offset_(offset)
 {
-    if (u==V4 && (type>255)) {
+    if ( (u == V4) && (type > 255)) {
         isc_throw(BadValue, "Can't create V4 option of type "
                   << type << ", V4 options are in range 0..255");
     }
 
     uint8_t* ptr = &buf[offset];
-    data_ = std::vector<uint8_t>(ptr, ptr+len);
+    data_ = std::vector<uint8_t>(ptr, ptr + len);
 
     // sanity checks
     // TODO: universe must be in V4 and V6
@@ -57,13 +57,30 @@ 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) {
+    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.
     }
+    if ( (u == V4) && (type > 255) ) {
+        isc_throw(OutOfRange, "DHCPv4 Option type " << type_ << " is too big."
+                  << "For DHCPv4 allowed type range is 0..255");
+    }
+}
+
+Option::Option(Universe u, uint16_t type, vector<uint8_t>::const_iterator first,
+               vector<uint8_t>::const_iterator last)
+    :universe_(u), type_(type) {
+    if ( (u == V4) && (type > 255) ) {
+        isc_throw(OutOfRange, "DHCPv4 Option type " << type_ << " is too big."
+                  << "For DHCPv4 allowed type range is 0..255");
+    }
+    data_ = std::vector<uint8_t>(first, last);
+    if ( (u == V4) && (data_.size() > 255) ) {
+        isc_throw(OutOfRange, "DHCPv4 Option " << type_ << " is too big.");
+    }
 }
 
 
@@ -82,7 +99,7 @@ void
 Option::pack4(isc::util::OutputBuffer& buf) {
     switch (universe_) {
     case V4: {
-        if (data_.size()>255) {
+        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
@@ -114,7 +131,7 @@ unsigned int
 Option::pack4(boost::shared_array<uint8_t>& buf,
              unsigned int buf_len,
              unsigned int offset) {
-    if ( offset+len() > buf_len ) {
+    if (offset + len() > buf_len) {
         isc_throw(OutOfRange, "Failed to pack v4 option=" <<
                   type_ << ",len=" << len() << ": too small buffer.");
     }
@@ -131,7 +148,7 @@ unsigned int
 Option::pack6(boost::shared_array<uint8_t>& buf,
              unsigned int buf_len,
              unsigned int offset) {
-    if ( offset+len() > buf_len ) {
+    if (offset+len() > buf_len) {
         isc_throw(OutOfRange, "Failed to pack v6 option=" <<
                   type_ << ",len=" << len() << ": too small buffer.");
     }
@@ -142,7 +159,7 @@ Option::pack6(boost::shared_array<uint8_t>& buf,
 
     ptr = writeUint16(len() - getHeaderLen(), ptr);
 
-    if (data_.size())
+    if (! data_.empty())
         memcpy(ptr, &data_[0], data_.size());
 
     // end of fixed part of this option
@@ -191,7 +208,7 @@ Option::unpack6(const boost::shared_array<uint8_t>& buf,
     }
 
     uint8_t* ptr = &buf[offset];
-    data_ = std::vector<uint8_t>(ptr, ptr+parse_len);
+    data_ = std::vector<uint8_t>(ptr, ptr + parse_len);
 
     offset_ = offset;
 
@@ -256,12 +273,12 @@ Option::delOption(unsigned short opt_type) {
 std::string Option::toText(int indent /* =0 */ ) {
     std::stringstream tmp;
 
-    for (int i=0; i<indent; i++)
+    for (int i = 0; i < indent; i++)
         tmp << " ";
 
     tmp << "type=" << type_ << ", len=" << len()-getHeaderLen() << ": ";
 
-    for (unsigned int i=0; i<data_.size(); i++) {
+    for (unsigned int i = 0; i < data_.size(); i++) {
         if (i) {
             tmp << ":";
         }
@@ -270,8 +287,8 @@ std::string Option::toText(int indent /* =0 */ ) {
     }
 
     // print suboptions
-    for (OptionCollection::const_iterator opt=options_.begin();
-         opt!=options_.end();
+    for (OptionCollection::const_iterator opt = options_.begin();
+         opt != options_.end();
          ++opt) {
         tmp << (*opt).second->toText(indent+2);
     }

src/lib/dhcp/option.h

@@ -90,6 +90,31 @@ public:
     /// @param data content of the option
     Option(Universe u, unsigned short type, std::vector<uint8_t>& data);
 
+
+    /// @brief Constructor, used for received options.
+    ///
+    /// This contructor is similar to the previous one, but it does not take
+    /// the whole vector<uint8_t>, but rather subset of it.
+    ///
+    /// TODO: This can be templated to use different containers, not just
+    /// vector. Prototype should look like this:
+    /// template<typename InputIterator> Option(Universe u, uint16_t type,
+    /// InputIterator first, InputIterator last);
+    ///
+    /// vector<int8_t> myData;
+    /// Example usage: new Option(V4, 123, myData.begin()+1, myData.end()-1)
+    /// This will create DHCPv4 option of type 123 that contains data from
+    /// trimmed (first and last byte removed) myData vector.
+    ///
+    /// @param u specifies universe (V4 or V6)
+    /// @param type option type (0-255 for V4 and 0-65535 for V6)
+    /// @param first iterator to the first element that should be copied
+    /// @param last iterator to the next element after the last one
+    ///        to be copied.
+    Option(Universe u, uint16_t type,
+           std::vector<uint8_t>::const_iterator first,
+           std::vector<uint8_t>::const_iterator last);
+
     /// @brief returns option universe (V4 or V6)
     ///
     /// @return universe type

src/lib/dhcp/tests/option_unittest.cc

@@ -69,7 +69,7 @@ TEST_F(OptionTest, v4_basic) {
 const uint8_t dummyPayload[] =
 { 1, 2, 3, 4};
 
-TEST_F(OptionTest, v4_data) {
+TEST_F(OptionTest, v4_data1) {
 
     vector<uint8_t> data(dummyPayload, dummyPayload + sizeof(dummyPayload));
 
@@ -114,6 +114,66 @@ TEST_F(OptionTest, v4_data) {
     );
 }
 
+// this is almost the same test as v4_data1, but it uses
+// different constructor
+TEST_F(OptionTest, v4_data2) {
+
+    vector<uint8_t> data(dummyPayload, dummyPayload + sizeof(dummyPayload));
+
+    vector<uint8_t> expData = data;
+
+    // Add fake data in front and end. Main purpose of this test is to check
+    // that only subset of the whole vector can be used for creating option.
+    data.insert(data.begin(), 56);
+    data.push_back(67);
+
+    // Data contains extra garbage at beginning and at the end. It should be
+    // ignored, as we pass interators to proper data. Only subset (limited by
+    // iterators) of the vector should be used.
+    // expData contains expected content (just valid data, without garbage).
+
+    Option* opt = 0;
+
+    // Create DHCPv4 option of type 123 that contains
+    // 4 bytes (sizeof(dummyPayload).
+    ASSERT_NO_THROW(
+        opt= new Option(Option::V4,
+                        123, // type
+                        data.begin() + 1,
+                        data.end() - 1);
+    );
+
+    // check that content is reported properly
+    EXPECT_EQ(123, opt->getType());
+    vector<uint8_t> optData = opt->getData();
+    ASSERT_EQ(optData.size(), expData.size());
+    EXPECT_EQ(optData, expData);
+    EXPECT_EQ(2, opt->getHeaderLen());
+    EXPECT_EQ(6, opt->len());
+
+    // 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));
+
+    // check that we can destroy that option
+    EXPECT_NO_THROW(
+        delete opt;
+    );
+}
+
 TEST_F(OptionTest, v4_toText) {
 
     vector<uint8_t> buf(3);

src/lib/dhcp/tests/pkt4_unittest.cc

@@ -283,6 +283,9 @@ TEST(Pkt4Test, hwAddr) {
     vector<uint8_t> mac;
     uint8_t expectedChaddr[Pkt4::MAX_CHADDR_LEN];
 
+    // We resize vector to specified length. It is more natural for fixed-length
+    // field, than clear it (shrink size to 0) and push_back each element
+    // (growing length back to MAX_CHADDR_LEN).
     mac.resize(Pkt4::MAX_CHADDR_LEN);
 
     Pkt4* pkt = 0;
@@ -447,10 +450,9 @@ TEST(Pkt4Test, options) {
 
     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;
+        payload[i].push_back(i*10);
+        payload[i].push_back(i*10+1);
+        payload[i].push_back(i*10+2);
     }
 
     boost::shared_ptr<Option> opt1(new Option(Option::V4, 12, payload[0]));
@@ -483,7 +485,7 @@ TEST(Pkt4Test, options) {
         pkt->pack();
     );
 
-    OutputBuffer& buf = pkt->getBuffer();
+    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());