From d1c6e7d8b55a9fb63c8b2aa73e8373cd1f9d50f8 Mon Sep 17 00:00:00 2001 From: Marcin Siodelski Date: Mon, 17 Feb 2014 18:27:49 +0100 Subject: [PATCH] [3316] Addressed review comments. --- src/bin/dhcp6/dhcp6_srv.cc | 10 ++- src/bin/dhcp6/tests/dhcp6_srv_unittest.cc | 2 +- src/bin/dhcp6/tests/wireshark.cc | 2 +- src/lib/dhcp/opaque_data_tuple.cc | 2 +- src/lib/dhcp/opaque_data_tuple.h | 10 +-- src/lib/dhcp/option_definition.cc | 8 +-- src/lib/dhcp/option_vendor_class.cc | 2 +- src/lib/dhcp/option_vendor_class.h | 15 +++-- .../dhcp/tests/opaque_data_tuple_unittest.cc | 64 +++++++++++-------- .../tests/option_vendor_class_unittest.cc | 14 +++- 10 files changed, 78 insertions(+), 51 deletions(-) diff --git a/src/bin/dhcp6/dhcp6_srv.cc b/src/bin/dhcp6/dhcp6_srv.cc index 432527f9da..2f62eff462 100644 --- a/src/bin/dhcp6/dhcp6_srv.cc +++ b/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()); } diff --git a/src/bin/dhcp6/tests/dhcp6_srv_unittest.cc b/src/bin/dhcp6/tests/dhcp6_srv_unittest.cc index 205df2f04b..af26e4cee5 100644 --- a/src/bin/dhcp6/tests/dhcp6_srv_unittest.cc +++ b/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 diff --git a/src/bin/dhcp6/tests/wireshark.cc b/src/bin/dhcp6/tests/wireshark.cc index a303035c93..cf01a23839 100644 --- a/src/bin/dhcp6/tests/wireshark.cc +++ b/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 diff --git a/src/lib/dhcp/opaque_data_tuple.cc b/src/lib/dhcp/opaque_data_tuple.cc index f35721ad1f..5d81587517 100644 --- a/src/lib/dhcp/opaque_data_tuple.cc +++ b/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" diff --git a/src/lib/dhcp/opaque_data_tuple.h b/src/lib/dhcp/opaque_data_tuple.h index d1f907522c..02691b79c0 100644 --- a/src/lib/dhcp/opaque_data_tuple.h +++ b/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. diff --git a/src/lib/dhcp/option_definition.cc b/src/lib/dhcp/option_definition.cc index 4f16c66fa3..0ac2bfeab9 100644 --- a/src/lib/dhcp/option_definition.cc +++ b/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))); } diff --git a/src/lib/dhcp/option_vendor_class.cc b/src/lib/dhcp/option_vendor_class.cc index 343384fd4a..1c53fbeae6 100644 --- a/src/lib/dhcp/option_vendor_class.cc +++ b/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]); } diff --git a/src/lib/dhcp/option_vendor_class.h b/src/lib/dhcp/option_vendor_class.h index 407c4b3d16..9c46f30d59 100644 --- a/src/lib/dhcp/option_vendor_class.h +++ b/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); diff --git a/src/lib/dhcp/tests/opaque_data_tuple_unittest.cc b/src/lib/dhcp/tests/opaque_data_tuple_unittest.cc index 8ed915ac61..08ffd5ced1 100644 --- a/src/lib/dhcp/tests/opaque_data_tuple_unittest.cc +++ b/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[] = { diff --git a/src/lib/dhcp/tests/option_vendor_class_unittest.cc b/src/lib/dhcp/tests/option_vendor_class_unittest.cc index eb99ad12cf..b2ca6bbcfb 100644 --- a/src/lib/dhcp/tests/option_vendor_class_unittest.cc +++ b/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[] = {