[2526] Changes as a result of the code review.

src/lib/dhcp/libdhcp++.cc

@@ -102,31 +102,35 @@ LibDHCP::optionFactory(Option::Universe u,
 size_t LibDHCP::unpackOptions6(const OptionBuffer& buf,
                                isc::dhcp::Option::OptionCollection& options) {
     size_t offset = 0;
-    size_t end = buf.size();
-
-    while (offset +4 <= end) {
-        uint16_t opt_type = buf[offset] * 256 + buf[offset + 1];
+    size_t length = buf.size();
+
+    // Get the list of stdandard option definitions.
+    const OptionDefContainer& option_defs = LibDHCP::getOptionDefs(Option::V6);
+    // Get the search index #1. It allows to search for option definitions
+    // using option code.
+    const OptionDefContainerTypeIndex& idx = option_defs.get<1>();
+
+    // The buffer being read comprises a set of options, each starting with
+    // a two-byte type code and a two-byte length field.
+    while (offset + 4 <= length) {
+        uint16_t opt_type = isc::util::readUint16(&buf[offset]);
         offset += 2;
-        uint16_t opt_len = buf[offset] * 256 + buf[offset + 1];
+        uint16_t opt_len = isc::util::readUint16(&buf[offset]);
         offset += 2;
 
-        if (offset + opt_len > end) {
+        if (offset + opt_len > length) {
             // @todo: consider throwing exception here.
             return (offset);
         }
 
-        // Get the list of stdandard option definitions.
-        OptionDefContainer option_defs = LibDHCP::getOptionDefs(Option::V6);
-        // 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.
+        // Get all definitions 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;
         if (num_defs > 1) {
             // Multiple options of the same code are not supported right now!
@@ -164,7 +168,14 @@ size_t LibDHCP::unpackOptions4(const OptionBuffer& buf,
                                  isc::dhcp::Option::OptionCollection& options) {
     size_t offset = 0;
 
-    // 2 byte - header of DHCPv4 option
+    // Get the list of stdandard option definitions.
+    const 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>();
+
+    // The buffer being read comprises a set of options, each starting with
+    // a one-byte type code and a one-byte length field.
     while (offset + 1 <= buf.size()) {
         uint8_t opt_type = buf[offset++];
 
@@ -191,12 +202,7 @@ 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
+        // Get all definitions 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.
@@ -204,7 +210,6 @@ size_t LibDHCP::unpackOptions4(const OptionBuffer& buf,
         // Get the number of returned option definitions for the option code.
         size_t num_defs = distance(range.first, range.second);
 
-
         OptionPtr opt;
         if (num_defs > 1) {
             // Multiple options of the same code are not supported right now!

src/lib/dhcp/libdhcp++.h

@@ -140,7 +140,7 @@ private:
     ///
     /// @throw std::bad alloc if system went out of memory.
     /// @throw MalformedOptionDefinition if any of the definitions
-    /// is incorrect. This is programming error.
+    /// are incorrect. This is programming error.
     static void initStdOptionDefs4();
 
     /// Initialize standard DHCPv6 option definitions.

src/lib/dhcp/tests/libdhcp++_unittest.cc

@@ -150,7 +150,8 @@ private:
     }
 };
 
-static const uint8_t packed[] = {
+// The DHCPv6 options in the wire format, used by multiple tests.
+const uint8_t v6packed[] = {
     0, 1, 0, 5, 100, 101, 102, 103, 104, // CLIENT_ID (9 bytes)
     0, 2, 0, 3, 105, 106, 107, // SERVER_ID (7 bytes)
     0, 14, 0, 0, // RAPID_COMMIT (0 bytes)
@@ -254,8 +255,8 @@ TEST_F(LibDhcpTest, packOptions6) {
     OutputBuffer assembled(512);
 
     EXPECT_NO_THROW(LibDHCP::packOptions6(assembled, opts));
-    EXPECT_EQ(sizeof(packed), assembled.getLength());
-    EXPECT_EQ(0, memcmp(assembled.getData(), packed, sizeof(packed)));
+    EXPECT_EQ(sizeof(v6packed), assembled.getLength());
+    EXPECT_EQ(0, memcmp(assembled.getData(), v6packed, sizeof(v6packed)));
 }
 
 TEST_F(LibDhcpTest, unpackOptions6) {
@@ -267,10 +268,10 @@ TEST_F(LibDhcpTest, unpackOptions6) {
     isc::dhcp::Option::OptionCollection options; // list of options
 
     OptionBuffer buf(512);
-    memcpy(&buf[0], packed, sizeof(packed));
+    memcpy(&buf[0], v6packed, sizeof(v6packed));
 
     EXPECT_NO_THROW ({
-            LibDHCP::unpackOptions6(OptionBuffer(buf.begin(), buf.begin() + sizeof(packed)),
+            LibDHCP::unpackOptions6(OptionBuffer(buf.begin(), buf.begin() + sizeof(v6packed)),
                                     options);
     });
 
@@ -281,14 +282,14 @@ TEST_F(LibDhcpTest, unpackOptions6) {
     EXPECT_EQ(1, x->second->getType());  // this should be option 1
     ASSERT_EQ(9, x->second->len()); // it should be of length 9
     ASSERT_EQ(5, x->second->getData().size());
-    EXPECT_EQ(0, memcmp(&x->second->getData()[0], packed + 4, 5)); // data len=5
+    EXPECT_EQ(0, memcmp(&x->second->getData()[0], v6packed + 4, 5)); // data len=5
 
         x = options.find(2);
     ASSERT_FALSE(x == options.end()); // option 2 should exist
     EXPECT_EQ(2, x->second->getType());  // this should be option 2
     ASSERT_EQ(7, x->second->len()); // it should be of length 7
     ASSERT_EQ(3, x->second->getData().size());
-    EXPECT_EQ(0, memcmp(&x->second->getData()[0], packed + 13, 3)); // data len=3
+    EXPECT_EQ(0, memcmp(&x->second->getData()[0], v6packed + 13, 3)); // data len=3
 
     x = options.find(14);
     ASSERT_FALSE(x == options.end()); // option 14 should exist
@@ -316,7 +317,7 @@ TEST_F(LibDhcpTest, unpackOptions6) {
     expected_opts.push_back(0x6C6D); // equivalent to: 108, 109
     expected_opts.push_back(0x6E6F); // equivalent to 110, 111
     ASSERT_EQ(expected_opts.size(), opts.size());
-    // Validated if option has been unpacked correctly.
+    // Validated if option has been un packed correctly.
     EXPECT_TRUE(std::equal(expected_opts.begin(), expected_opts.end(),
                            opts.begin()));
 
@@ -395,11 +396,11 @@ TEST_F(LibDhcpTest, packOptions4) {
 
 TEST_F(LibDhcpTest, unpackOptions4) {
 
-    vector<uint8_t> packed(v4Opts, v4Opts + sizeof(v4Opts));
+    vector<uint8_t> v4packed(v4Opts, v4Opts + sizeof(v4Opts));
     isc::dhcp::Option::OptionCollection options; // list of options
 
     ASSERT_NO_THROW(
-        LibDHCP::unpackOptions4(packed, options);
+        LibDHCP::unpackOptions4(v4packed, options);
     );
 
     isc::dhcp::Option::OptionCollection::const_iterator x = options.find(12);

src/lib/dhcp/tests/option_int_unittest.cc

@@ -457,7 +457,10 @@ TEST_F(OptionIntTest, unpackSuboptions4) {
         0x01, 0x02, 0x03, 0x04, // data, uint32_t value = 0x01020304
         TEST_OPT_CODE + 1, 0x4, 0x01, 0x01, 0x01, 0x01 // suboption
     };
-    // Copy the data to a vector so as we can pas it to the
+    // Make sure that the buffer size is sufficient to copy the
+    // elements from the array.
+    ASSERT_GE(buf_.size(), sizeof(expected));
+    // Copy the data to a vector so as we can pass it to the
     // OptionInt's constructor.
     memcpy(&buf_[0], expected, sizeof(expected));
 
@@ -513,6 +516,9 @@ TEST_F(OptionIntTest, unpackSuboptions6) {
     };
     ASSERT_EQ(38, sizeof(expected));
 
+    // Make sure that the buffer's size is sufficient to
+    // copy the elements from the array.
+    ASSERT_GE(buf_.size(), sizeof(expected));
     memcpy(&buf_[0], expected, sizeof(expected));
 
     boost::shared_ptr<OptionInt<uint16_t> > opt;

src/lib/dhcp/tests/pkt4_unittest.cc

@@ -544,11 +544,6 @@ 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()
     );