[3316] Addressed review comments.

src/bin/dhcp6/dhcp6_srv.cc

@@ -2446,22 +2446,20 @@ void Dhcpv6Srv::classifyPacket(const Pkt6Ptr& pkt) {
     }
 
     std::ostringstream classes;
-
     if (vclass->hasTuple(DOCSIS3_CLASS_MODEM)) {
-        pkt->addClass(DOCSIS3_CLASS_MODEM);
-        classes << DOCSIS3_CLASS_MODEM;
+        classes << "VENDOR_CLASS_" << DOCSIS3_CLASS_MODEM;
 
     } else if (vclass->hasTuple(DOCSIS3_CLASS_EROUTER)) {
-        pkt->addClass(DOCSIS3_CLASS_EROUTER);
         classes << DOCSIS3_CLASS_EROUTER;
 
     } else {
-        pkt->addClass(vclass->getTuple(0).getText());
-        classes << vclass->getTuple(0);
+        classes << vclass->getTuple(0).getText();
 
     }
 
+    // If there is no class identified, leave.
     if (!classes.str().empty()) {
+        pkt->addClass(classes.str());
         LOG_DEBUG(dhcp6_logger, DBG_DHCP6_BASIC, DHCP6_CLASS_ASSIGNED)
             .arg(classes.str());
     }

src/bin/dhcp6/tests/dhcp6_srv_unittest.cc

@@ -1723,7 +1723,7 @@ TEST_F(Dhcpv6SrvTest, clientClassification) {
     srv.classifyPacket(sol1);
 
     // It should belong to docsis3.0 class. It should not belong to eRouter1.0
-    EXPECT_TRUE(sol1->inClass("docsis3.0"));
+    EXPECT_TRUE(sol1->inClass("VENDOR_CLASS_docsis3.0"));
     EXPECT_FALSE(sol1->inClass("eRouter1.0"));
 
     // Let's get a relayed SOLICIT. This particular relayed SOLICIT has

src/bin/dhcp6/tests/wireshark.cc

@@ -1,4 +1,4 @@
-// Copyright (C) 2013  Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2013-2014 Internet Systems Consortium, Inc. ("ISC")
 //
 // Permission to use, copy, modify, and/or distribute this software for any
 // purpose with or without fee is hereby granted, provided that the above

src/lib/dhcp/opaque_data_tuple.cc

@@ -60,7 +60,7 @@ OpaqueDataTuple::pack(isc::util::OutputBuffer& buf) const {
     if (getLength() == 0) {
         isc_throw(OpaqueDataTupleError, "failed to create on-wire format of the"
                   " opaque data field, because the field appears to be empty");
-    } else if ((1 << getDataFieldSize() * 8) <= getLength()) {
+    } else if ((1 << (getDataFieldSize() * 8)) <= getLength()) {
         isc_throw(OpaqueDataTupleError, "failed to create on-wire format of the"
                   " opaque data field, because current data length "
                   << getLength() << " exceeds the maximum size for the length"

src/lib/dhcp/opaque_data_tuple.h

@@ -25,7 +25,7 @@
 namespace isc {
 namespace dhcp {
 
-/// @brief Exception to be thrown when there operation on @c OpaqueDataTuple
+/// @brief Exception to be thrown when the operation on @c OpaqueDataTuple
 /// object results in an error.
 class OpaqueDataTupleError : public Exception {
 public:
@@ -72,7 +72,7 @@ public:
     ///
     /// @param length_field_type Indicates a length of the field which holds
     /// the size of the tuple.
-    OpaqueDataTuple(LengthFieldType length_field_type = LENGTH_2_BYTES);
+    OpaqueDataTuple(LengthFieldType length_field_type);
 
     /// @brief Constructor
     ///
@@ -81,7 +81,7 @@ public:
     ///
     /// @param length_field_type Indicates the length of the field holding the
     /// opaque data size.
-    /// @param begin Iterator pointing to the begining of the buffer holding
+    /// @param begin Iterator pointing to the beginning of the buffer holding
     /// wire data.
     /// @param end Iterator pointing to the end of the buffer holding wire data.
     /// @tparam InputIterator Type of the iterators passed to this function.
@@ -96,7 +96,7 @@ public:
     /// @brief Appends data to the tuple.
     ///
     /// This function appends the data of the specified length to the tuple.
-    /// If the speficified buffer length is greater than the size of the buffer,
+    /// If the specified buffer length is greater than the size of the buffer,
     /// the behavior of this function is undefined.
     ///
     /// @param data Iterator pointing to the beginning of the buffer being
@@ -213,7 +213,7 @@ public:
     ///
     /// This function allows opaque data with the length of 0.
     ///
-    /// @param begin Iterator pointing to the begining of the buffer holding
+    /// @param begin Iterator pointing to the beginning of the buffer holding
     /// wire data.
     /// @param end Iterator pointing to the end of the buffer holding wire data.
     /// @tparam InputIterator Type of the iterators passed to this function.

src/lib/dhcp/option_definition.cc

@@ -668,21 +668,21 @@ OptionDefinition::factorySpecialFormatOption(Option::Universe u,
             // a specialized class to handle it.
             return (OptionPtr(new Option6ClientFqdn(begin, end)));
         } else if (getCode() == D6O_VENDOR_OPTS && haveVendor6Format()) {
-            // Vendor-Specific Information.
+            // Vendor-Specific Information (option code 17)
             return (OptionPtr(new OptionVendor(Option::V6, begin, end)));
         } else if (getCode() == D6O_VENDOR_CLASS && haveVendorClass6Format()) {
-            // Vendor Class.
+            // Vendor Class (option code 16).
             return (OptionPtr(new OptionVendorClass(Option::V6, begin, end)));
         }
     } else {
         if ((getCode() == DHO_FQDN) && haveFqdn4Format()) {
             return (OptionPtr(new Option4ClientFqdn(begin, end)));
-            // V-I VendorClass
         } else if ((getCode() == DHO_VIVCO_SUBOPTIONS) &&
                    haveVendorClass4Format()) {
+            // V-I Vendor Class (option code 124).
             return (OptionPtr(new OptionVendorClass(Option::V4, begin, end)));
         } else if (getCode() == DHO_VIVSO_SUBOPTIONS && haveVendor4Format()) {
-            // Vendor-Specific Information.
+            // Vendor-Specific Information (option code 125).
             return (OptionPtr(new OptionVendor(Option::V4, begin, end)));
 
         }

src/lib/dhcp/option_vendor_class.cc

@@ -126,7 +126,7 @@ OptionVendorClass::getTuple(const size_t at) const {
     if (at >= getTuplesNum()) {
         isc_throw(isc::OutOfRange, "attempted to get an opaque data for the"
                   " vendor option at position " << at << " which is out of"
-                  " range");
+                  " range. There are only " << getTuplesNum() << " tuples");
     }
     return (tuples_[at]);
 }

src/lib/dhcp/option_vendor_class.h

@@ -29,10 +29,10 @@ namespace dhcp {
 /// @brief This class encapsulates DHCPv6 Vendor Class and DHCPv4 V-I Vendor
 /// Class options.
 ///
-/// The format of DHCPv6 Vendor Class option is described in section 22.16 of
-/// RFC3315 and the format of the DHCPv4 V-I Vendor Class option is described
-/// in section 3 of RFC3925. Each of these options carries enterprise id
-/// followed by the collection of tuples carring opaque data. A single tuple
+/// The format of DHCPv6 Vendor Class option (16) is described in section 22.16
+/// of RFC3315 and the format of the DHCPv4 V-I Vendor Class option (124) is
+/// described in section 3 of RFC3925. Each of these options carries enterprise
+/// id followed by the collection of tuples carring opaque data. A single tuple
 /// consists of the field holding opaque data length and the actual data.
 /// In case of the DHCPv4 V-I Vendor Class each tuple is preceded by the
 /// 4-byte long enterprise id. Also, the field which carries the length of
@@ -54,6 +54,13 @@ public:
 
     /// @brief Constructor.
     ///
+    /// This constructor instance of the DHCPv4 V-I Vendor Class option (124)
+    /// or DHCPv6 Vendor Class option (16), depending on universe specified.
+    /// If the universe is v4, the constructor adds one empty tuple to the
+    /// option, as per RFC3925, section 3. the complete option must hold at
+    /// least one data-len field for opaque data. If the specified universe
+    /// is v6, the constructor adds no tuples.
+    ///
     /// @param u universe (v4 or v6).
     /// @param vendor_id vendor enterprise id (unique 32-bit integer).
     OptionVendorClass(Option::Universe u, const uint32_t vendor_id);

src/lib/dhcp/tests/opaque_data_tuple_unittest.cc

@@ -29,7 +29,7 @@ namespace {
 // This test checks that when the default constructor is called, the data buffer
 // is empty.
 TEST(OpaqueDataTuple, constructor) {
-    OpaqueDataTuple tuple;
+    OpaqueDataTuple tuple(OpaqueDataTuple::LENGTH_2_BYTES);
     // There should be no data in the tuple.
     EXPECT_EQ(0, tuple.getLength());
     EXPECT_TRUE(tuple.getData().empty());
@@ -73,7 +73,7 @@ TEST(OpaqueDataTuple, constructorParse2Bytes) {
 
 // This test checks that it is possible to set the tuple data using raw buffer.
 TEST(OpaqueDataTuple, assignData) {
-    OpaqueDataTuple tuple;
+    OpaqueDataTuple tuple(OpaqueDataTuple::LENGTH_2_BYTES);
     // Initially the tuple buffer should be empty.
     OpaqueDataTuple::Buffer buf = tuple.getData();
     ASSERT_TRUE(buf.empty());
@@ -98,10 +98,10 @@ TEST(OpaqueDataTuple, assignData) {
     EXPECT_TRUE(std::equal(buf.begin(), buf.end(), data2));
 }
 
-// This test checks thet it is possible to append the data to the tuple using
+// This test checks that it is possible to append the data to the tuple using
 // raw buffer.
 TEST(OpaqueDataTuple, appendData) {
-    OpaqueDataTuple tuple;
+    OpaqueDataTuple tuple(OpaqueDataTuple::LENGTH_2_BYTES);
     // Initially the tuple buffer should be empty.
     OpaqueDataTuple::Buffer buf = tuple.getData();
     ASSERT_TRUE(buf.empty());
@@ -131,7 +131,7 @@ TEST(OpaqueDataTuple, appendData) {
 
 // This test checks that it is possible to assign the string to the tuple.
 TEST(OpaqueDataTuple, assignString) {
-    OpaqueDataTuple tuple;
+    OpaqueDataTuple tuple(OpaqueDataTuple::LENGTH_2_BYTES);
     // Initially, the tuple should be empty.
     ASSERT_EQ(0, tuple.getLength());
     // Assign some string data.
@@ -148,7 +148,7 @@ TEST(OpaqueDataTuple, assignString) {
 
 // This test checks that it is possible to append the string to the tuple.
 TEST(OpaqueDataTuple, appendString) {
-    OpaqueDataTuple tuple;
+    OpaqueDataTuple tuple(OpaqueDataTuple::LENGTH_2_BYTES);
     // Initially the tuple should be empty.
     ASSERT_EQ(0, tuple.getLength());
     // Append the string to it.
@@ -163,19 +163,20 @@ TEST(OpaqueDataTuple, appendString) {
     EXPECT_EQ("First part and second part", tuple.getText());
 }
 
-// This test checks that equals function correctly checks that the tuple
-// holds a given string but it doesn't hold the other.
+// This test verifies that equals function correctly checks that the tuple
+// holds a given string but it doesn't hold the other. This test also
+// checks the assignment operator for the tuple.
 TEST(OpaqueDataTuple, equals) {
-    OpaqueDataTuple tuple;
+    OpaqueDataTuple tuple(OpaqueDataTuple::LENGTH_2_BYTES);
     // Tuple is supposed to be empty so it is not equal xyz.
     EXPECT_FALSE(tuple.equals("xyz"));
     // Assign xyz.
-    tuple = "xyz";
+    EXPECT_NO_THROW(tuple = "xyz");
     // The tuple should be equal xyz, but not abc.
     EXPECT_FALSE(tuple.equals("abc"));
     EXPECT_TRUE(tuple.equals("xyz"));
     // Assign abc to the tuple.
-    tuple = "abc";
+    EXPECT_NO_THROW(tuple = "abc");
     // It should be now opposite.
     EXPECT_TRUE(tuple.equals("abc"));
     EXPECT_FALSE(tuple.equals("xyz"));
@@ -184,7 +185,7 @@ TEST(OpaqueDataTuple, equals) {
 // This test checks that the conversion of the data in the tuple to the string
 // is performed correctly.
 TEST(OpaqueDataTuple, getText) {
-    OpaqueDataTuple tuple;
+    OpaqueDataTuple tuple(OpaqueDataTuple::LENGTH_2_BYTES);
     // Initially the tuple should be empty.
     ASSERT_TRUE(tuple.getText().empty());
     // ASCII representation of 'Hello world'.
@@ -200,16 +201,16 @@ TEST(OpaqueDataTuple, getText) {
 
 // This test verifies the behavior of (in)equality and assignment operators.
 TEST(OpaqueDataTuple, operators) {
-    OpaqueDataTuple tuple;
+    OpaqueDataTuple tuple(OpaqueDataTuple::LENGTH_2_BYTES);
     // Tuple should be empty initially.
     ASSERT_EQ(0, tuple.getLength());
     // Check assignment.
-    tuple = "Hello World";
+    EXPECT_NO_THROW(tuple = "Hello World");
     EXPECT_EQ(11, tuple.getLength());
     EXPECT_TRUE(tuple == "Hello World");
     EXPECT_TRUE(tuple != "Something else");
     // Assign something else to make sure it affects the tuple.
-    tuple = "Something else";
+    EXPECT_NO_THROW(tuple = "Something else");
     EXPECT_EQ(14, tuple.getLength());
     EXPECT_TRUE(tuple == "Something else");
     EXPECT_TRUE(tuple != "Hello World");
@@ -218,19 +219,19 @@ TEST(OpaqueDataTuple, operators) {
 // This test verifies that the tuple is inserted in the textual format to the
 // output stream.
 TEST(OpaqueDataTuple, operatorOutputStream) {
-    OpaqueDataTuple tuple;
+    OpaqueDataTuple tuple(OpaqueDataTuple::LENGTH_2_BYTES);
     // The tuple should be empty initially.
     ASSERT_EQ(0, tuple.getLength());
     // The tuple is empty, so assigning its content to the output stream should
     // be no-op and result in the same text in the stream.
     std::ostringstream s;
     s << "Some text";
-    s << tuple;
+    EXPECT_NO_THROW(s << tuple);
     EXPECT_EQ("Some text", s.str());
     // Now, let's assign some text to the tuple and call operator again.
     // The new text should be added to the stream.
-    tuple = " and some other text";
-    s << tuple;
+    EXPECT_NO_THROW(tuple = " and some other text");
+    EXPECT_NO_THROW(s << tuple);
     EXPECT_EQ(s.str(), "Some text and some other text");
 
 }
@@ -238,19 +239,19 @@ TEST(OpaqueDataTuple, operatorOutputStream) {
 // This test verifies that the value of the tuple can be initialized from the
 // input stream.
 TEST(OpaqueDataTuple, operatorInputStream) {
-    OpaqueDataTuple tuple;
+    OpaqueDataTuple tuple(OpaqueDataTuple::LENGTH_2_BYTES);
     // The tuple should be empty initially.
     ASSERT_EQ(0, tuple.getLength());
     // The input stream has some text. This text should be appended to the
     // tuple.
     std::istringstream s;
     s.str("Some text");
-    s >> tuple;
+    EXPECT_NO_THROW(s >> tuple);
     EXPECT_EQ("Some text", tuple.getText());
     // Now, let's assign some other text to the stream. This new text should be
     // assigned to the tuple.
     s.str("And some other");
-    s >> tuple;
+    EXPECT_NO_THROW(s >> tuple);
     EXPECT_EQ("And some other", tuple.getText());
 }
 
@@ -349,6 +350,19 @@ TEST(OpaqueDataTuple, pack2Bytes) {
     // append the data to the current buffer.
     ASSERT_NO_THROW(tuple.pack(out_buf));
     EXPECT_EQ(1028, out_buf.getLength());
+
+    // Check that we can render the buffer of the maximal allowed size.
+    data.assign(65535, 1);
+    ASSERT_NO_THROW(tuple.assign(data.begin(), data.size()));
+    ASSERT_NO_THROW(tuple.pack(out_buf));
+
+    out_buf.clear();
+
+    // Append one additional byte. The total length of the tuple now exceeds
+    // the maximal value. An attempt to render it should throw an exception.
+    data.assign(1, 1);
+    ASSERT_NO_THROW(tuple.append(data.begin(), data.size()));
+    EXPECT_THROW(tuple.pack(out_buf), OpaqueDataTupleError);
 }
 
 // This test verifies that the tuple is decoded from the wire format.
@@ -369,7 +383,7 @@ TEST(OpaqueDataTuple, unpack1Byte) {
 // the wire format.
 TEST(OpaqueDataTuple, unpack1ByteZeroLength) {
     OpaqueDataTuple tuple(OpaqueDataTuple::LENGTH_1_BYTE);
-    tuple = "Hello world";
+    EXPECT_NO_THROW(tuple = "Hello world");
     ASSERT_NE(tuple.getLength(), 0);
 
     const char wire_data[] = {
@@ -390,7 +404,7 @@ TEST(OpaqueDataTuple, unpack1ByteEmptyBuffer) {
     EXPECT_THROW(tuple.unpack(wire_data, wire_data), OpaqueDataTupleError);
 }
 
-// This test verifies that exception if thrown when parsing truncated buffer.
+// This test verifies that exception is thrown when parsing truncated buffer.
 TEST(OpaqueDataTuple, unpack1ByteTruncatedBuffer) {
    OpaqueDataTuple tuple(OpaqueDataTuple::LENGTH_1_BYTE);
     const char wire_data[] = {
@@ -425,7 +439,7 @@ TEST(OpaqueDataTuple, unpack2Byte) {
 TEST(OpaqueDataTuple, unpack2ByteZeroLength) {
     OpaqueDataTuple tuple(OpaqueDataTuple::LENGTH_2_BYTES);
     // Set some data for the tuple.
-    tuple = "Hello world";
+    EXPECT_NO_THROW(tuple = "Hello world");
     ASSERT_NE(tuple.getLength(), 0);
     // The buffer holds just a length field with the value of 0.
     const char wire_data[] = {

src/lib/dhcp/tests/option_vendor_class_unittest.cc

@@ -59,7 +59,7 @@ TEST(OptionVendorClass, addTuple) {
     // Initially there should be no tuples (for DHCPv6).
     ASSERT_EQ(0, vendor_class.getTuplesNum());
     // Create a new tuple and add it to the option.
-    OpaqueDataTuple tuple;
+    OpaqueDataTuple tuple(OpaqueDataTuple::LENGTH_2_BYTES);
     tuple = "xyz";
     vendor_class.addTuple(tuple);
     // The option should now hold one tuple.
@@ -362,7 +362,11 @@ TEST(OptionVendorClass, unpack6Truncated) {
 }
 
 // This test checks that exception is thrown when parsing DHCPv4 V-I Vendor
-// Class option which doesn't have opaque data length.
+// Class option which doesn't have opaque data length. This test is different
+// from the corresponding test for v6 in that, the v4 test expects that
+// exception is thrown when parsing DHCPv4 option without data-len field
+// (has no tuples), whereas for DHCPv6 option it is perfectly ok that
+// option has no tuples (see class constructor).
 TEST(OptionVendorClass, unpack4NoTuple) {
     // Prepare data to decode.
     const uint8_t buf_data[] = {
@@ -375,7 +379,11 @@ TEST(OptionVendorClass, unpack4NoTuple) {
 }
 
 // This test checks that the DHCPv6 Vendor Class option containing no opaque
-// data is parsed correctly.
+// data is parsed correctly. This test is different from the corresponding
+// test for v4 in that, the v6 test checks that the option parsing succeeds
+// when option has no opaque data tuples, whereas the v4 test expects that
+// parsing fails for DHCPv4 option which doesn't have opaque-data (see
+// class constructor).
 TEST(OptionVendorClass, unpack6NoTuple) {
     // Prepare data to decode.
     const uint8_t buf_data[] = {