From 21da96d4bcb34a2289e5b16eead8ce181d8eb61c Mon Sep 17 00:00:00 2001
From: Marcin Siodelski <marcin@isc.org>
Date: Tue, 11 Dec 2012 17:31:34 +0100
Subject: [PATCH] [2526] Use V4 option definitions when unpacking an option.

Some of the unit tests had to be modified after this change. This is
because they used to create options which data length did not match the
actual std options' formats.
---
 src/lib/dhcp/libdhcp++.cc                | 47 ++++++++++++++++++++----
 src/lib/dhcp/pkt4.cc                     |  6 ++-
 src/lib/dhcp/tests/libdhcp++_unittest.cc | 24 +++++++-----
 src/lib/dhcp/tests/pkt4_unittest.cc      | 41 +++++++++++++--------
 4 files changed, 83 insertions(+), 35 deletions(-)

diff --git a/src/lib/dhcp/libdhcp++.cc b/src/lib/dhcp/libdhcp++.cc
index 1faa5e9906..e6f552eacf 100644
--- a/src/lib/dhcp/libdhcp++.cc
+++ b/src/lib/dhcp/libdhcp++.cc
@@ -65,10 +65,12 @@ const OptionDefContainer&
 LibDHCP::getOptionDefs(const Option::Universe u) {
     switch (u) {
     case Option::V4:
-        initStdOptionDefs4();
+        if (v4option_defs_.empty()) {
+            initStdOptionDefs4();
+        }
         return (v4option_defs_);
     case Option::V6:
-        if (v6option_defs_.size() == 0) {
+        if (v6option_defs_.empty()) {
             initStdOptionDefs6();
         }
         return (v6option_defs_);
@@ -192,8 +194,10 @@ size_t LibDHCP::unpackOptions4(const OptionBuffer& buf,
             continue;
 
         if (offset + 1 >= buf.size()) {
+            // opt_type must be cast to integer so as it is not treated as
+            // unsigned char value (a number is presented in error message).
             isc_throw(OutOfRange, "Attempt to parse truncated option "
-                      << opt_type);
+                      << static_cast<int>(opt_type));
         }
 
         uint8_t opt_len =  buf[offset++];
@@ -203,12 +207,41 @@ size_t LibDHCP::unpackOptions4(const OptionBuffer& buf,
                       << "-byte long buffer.");
         }
 
+        // Get the list of stdandard option definitions.
+        OptionDefContainer option_defs = LibDHCP::getOptionDefs(Option::V4);
+        // Get the search index #1. It allows to search for option definitions
+        // using option code.
+        const OptionDefContainerTypeIndex& idx = option_defs.get<1>();
+        // Get all options with the particular option code. Note that option code
+        // is non-unique within this container however at this point we expect
+        // to get one option definition with the particular code. If more are
+        // returned we report an error.
+        const OptionDefContainerTypeRange& range = idx.equal_range(opt_type);
+        // Get the number of returned option definitions for the option code.
+        size_t num_defs = distance(range.first, range.second);
+
+
         OptionPtr opt;
-        switch(opt_type) {
-        default:
+        if (num_defs > 1) {
+            // Multiple options of the same code are not supported right now!
+            isc_throw(isc::Unexpected, "Internal error: multiple option definitions"
+                      " for option type " << static_cast<int>(opt_type)
+                      << " returned. Currently it is not supported to initialize"
+                      << " multiple option definitions for the same option code."
+                      << " This will be supported once support for option spaces"
+                      << " is implemented");
+        } else if (num_defs == 0) {
             opt = OptionPtr(new Option(Option::V4, opt_type,
-                                       buf.begin()+offset,
-                                       buf.begin()+offset+opt_len));
+                                       buf.begin() + offset,
+                                       buf.begin() + offset + opt_len));
+        } else {
+            // The option definition has been found. Use it to create
+            // the option instance from the provided buffer chunk.
+            const OptionDefinitionPtr& def = *(range.first);
+            assert(def);
+            opt = def->optionFactory(Option::V4, opt_type,
+                                     buf.begin() + offset,
+                                     buf.begin() + offset + opt_len);
         }
 
         options.insert(std::make_pair(opt_type, opt));
diff --git a/src/lib/dhcp/pkt4.cc b/src/lib/dhcp/pkt4.cc
index 5be82113a9..35342951fe 100644
--- a/src/lib/dhcp/pkt4.cc
+++ b/src/lib/dhcp/pkt4.cc
@@ -15,6 +15,7 @@
 #include <asiolink/io_address.h>
 #include <dhcp/dhcp4.h>
 #include <dhcp/libdhcp++.h>
+#include <dhcp/option_int.h>
 #include <dhcp/pkt4.h>
 #include <exceptions/exceptions.h>
 
@@ -184,9 +185,10 @@ Pkt4::unpack() {
 }
 
 void Pkt4::check() {
-    boost::shared_ptr<Option> typeOpt = getOption(DHO_DHCP_MESSAGE_TYPE);
+    boost::shared_ptr<OptionInt<uint8_t> > typeOpt =
+        boost::dynamic_pointer_cast<OptionInt<uint8_t> >(getOption(DHO_DHCP_MESSAGE_TYPE));
     if (typeOpt) {
-        uint8_t msg_type = typeOpt->getUint8();
+        uint8_t msg_type = typeOpt->getValue();
         if (msg_type>DHCPLEASEACTIVE) {
             isc_throw(BadValue, "Invalid DHCP message type received:" << msg_type);
         }
diff --git a/src/lib/dhcp/tests/libdhcp++_unittest.cc b/src/lib/dhcp/tests/libdhcp++_unittest.cc
index 2162baa701..d8ad450ebe 100644
--- a/src/lib/dhcp/tests/libdhcp++_unittest.cc
+++ b/src/lib/dhcp/tests/libdhcp++_unittest.cc
@@ -348,13 +348,17 @@ TEST_F(LibDhcpTest, unpackOptions6) {
     EXPECT_TRUE(x == options.end()); // option 32000 not found */
 }
 
-
+/// V4 Options being used to test pack/unpack operations.
+/// These are variable length options only so as there
+/// is no restriction on the data length being carried by them.
+/// For simplicity, we assign data of the length 3 for each
+/// of them.
 static uint8_t v4Opts[] = {
-    12,  3, 0,   1,  2,
-    13,  3, 10, 11, 12,
-    14,  3, 20, 21, 22,
-    254, 3, 30, 31, 32,
-    128, 3, 40, 41, 42
+    12,  3, 0,   1,  2, // Hostname
+    60,  3, 10, 11, 12, // Class Id
+    14,  3, 20, 21, 22, // Merit Dump File
+    254, 3, 30, 31, 32, // Reserved
+    128, 3, 40, 41, 42  // Vendor specific
 };
 
 TEST_F(LibDhcpTest, packOptions4) {
@@ -368,7 +372,7 @@ TEST_F(LibDhcpTest, packOptions4) {
     }
 
     OptionPtr opt1(new Option(Option::V4, 12, payload[0]));
-    OptionPtr opt2(new Option(Option::V4, 13, payload[1]));
+    OptionPtr opt2(new Option(Option::V4, 60, payload[1]));
     OptionPtr opt3(new Option(Option::V4, 14, payload[2]));
     OptionPtr opt4(new Option(Option::V4,254, payload[3]));
     OptionPtr opt5(new Option(Option::V4,128, payload[4]));
@@ -405,9 +409,9 @@ TEST_F(LibDhcpTest, unpackOptions4) {
     EXPECT_EQ(5, x->second->len()); // total option length 5
     EXPECT_EQ(0, memcmp(&x->second->getData()[0], v4Opts+2, 3)); // data len=3
 
-    x = options.find(13);
-    ASSERT_FALSE(x == options.end()); // option 1 should exist
-    EXPECT_EQ(13, x->second->getType());  // this should be option 13
+    x = options.find(60);
+    ASSERT_FALSE(x == options.end()); // option 2 should exist
+    EXPECT_EQ(60, x->second->getType());  // this should be option 60
     ASSERT_EQ(3, x->second->getData().size()); // it should be of length 3
     EXPECT_EQ(5, x->second->len()); // total option length 5
     EXPECT_EQ(0, memcmp(&x->second->getData()[0], v4Opts+7, 3)); // data len=3
diff --git a/src/lib/dhcp/tests/pkt4_unittest.cc b/src/lib/dhcp/tests/pkt4_unittest.cc
index 37af9c68ea..eccc0397d0 100644
--- a/src/lib/dhcp/tests/pkt4_unittest.cc
+++ b/src/lib/dhcp/tests/pkt4_unittest.cc
@@ -453,13 +453,17 @@ TEST(Pkt4Test, file) {
 
 }
 
+/// V4 Options being used for pack/unpack testing.
+/// For test simplicity, all selected options have
+/// variable length data so as there are no restrictions
+/// on a length of their data.
 static uint8_t v4Opts[] = {
-    12,  3, 0,   1,  2,
-    13,  3, 10, 11, 12,
-    14,  3, 20, 21, 22,
-    53, 1, 1, // DHCP_MESSAGE_TYPE (required to not throw exception during unpack)
-    128, 3, 30, 31, 32,
-    254, 3, 40, 41, 42,
+    12,  3, 0,   1,  2, // Hostname
+    14,  3, 10, 11, 12, // Merit Dump File
+    53, 1, 1, // Message Type (required to not throw exception during unpack)
+    60,  3, 20, 21, 22, // Class Id
+    128, 3, 30, 31, 32, // Vendor specific
+    254, 3, 40, 41, 42, // Reserved
 };
 
 TEST(Pkt4Test, options) {
@@ -473,9 +477,9 @@ TEST(Pkt4Test, options) {
     }
 
     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> opt3(new Option(Option::V4, 14, payload[1]));
     boost::shared_ptr<Option> optMsgType(new Option(Option::V4, DHO_DHCP_MESSAGE_TYPE));
+    boost::shared_ptr<Option> opt2(new Option(Option::V4, 60, 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]));
     optMsgType->setUint8(static_cast<uint8_t>(DHCPDISCOVER));
@@ -488,7 +492,7 @@ TEST(Pkt4Test, options) {
     pkt->addOption(optMsgType);
 
     EXPECT_TRUE(pkt->getOption(12));
-    EXPECT_TRUE(pkt->getOption(13));
+    EXPECT_TRUE(pkt->getOption(60));
     EXPECT_TRUE(pkt->getOption(14));
     EXPECT_TRUE(pkt->getOption(128));
     EXPECT_TRUE(pkt->getOption(254));
@@ -540,12 +544,17 @@ TEST(Pkt4Test, unpackOptions) {
     boost::shared_ptr<Pkt4> pkt(new Pkt4(&expectedFormat[0],
                                 expectedFormat.size()));
 
+    try {
+        pkt->unpack();
+    } catch (const Exception& ex) {
+        std::cout << ex.what() << std::endl;
+    }
     EXPECT_NO_THROW(
         pkt->unpack()
     );
 
     EXPECT_TRUE(pkt->getOption(12));
-    EXPECT_TRUE(pkt->getOption(13));
+    EXPECT_TRUE(pkt->getOption(60));
     EXPECT_TRUE(pkt->getOption(14));
     EXPECT_TRUE(pkt->getOption(128));
     EXPECT_TRUE(pkt->getOption(254));
@@ -557,19 +566,19 @@ TEST(Pkt4Test, unpackOptions) {
     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);
+    x = pkt->getOption(14);
     ASSERT_TRUE(x); // option 13 should exist
-    EXPECT_EQ(13, x->getType());  // this should be option 13
+    EXPECT_EQ(14, 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
+    x = pkt->getOption(60);
+    ASSERT_TRUE(x); // option 60 should exist
+    EXPECT_EQ(60, x->getType());  // this should be option 60
     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
+    EXPECT_EQ(0, memcmp(&x->getData()[0], v4Opts+15, 3)); // data len=3
 
     x = pkt->getOption(128);
     ASSERT_TRUE(x); // option 3 should exist