BIND 10 trac3316, updated. d1c6e7d8b55a9fb63c8b2aa73e8373cd1f9d50f8 [3316] Addressed review comments.
BIND 10 source code commits
bind10-changes at lists.isc.org
Mon Feb 17 17:53:43 UTC 2014
The branch, trac3316 has been updated
via d1c6e7d8b55a9fb63c8b2aa73e8373cd1f9d50f8 (commit)
from 282416056d98ce14b30998ae730c2ce1a4358dd0 (commit)
Those revisions listed above that are new to this repository have
not appeared on any other notification email; so we list those
revisions in full, below.
- Log -----------------------------------------------------------------
commit d1c6e7d8b55a9fb63c8b2aa73e8373cd1f9d50f8
Author: Marcin Siodelski <marcin at isc.org>
Date: Mon Feb 17 18:27:49 2014 +0100
[3316] Addressed review comments.
-----------------------------------------------------------------------
Summary of changes:
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 +++--
src/lib/dhcp/tests/opaque_data_tuple_unittest.cc | 64 ++++++++++++--------
src/lib/dhcp/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 432527f..2f62eff 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 205df2f..af26e4c 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 a303035..cf01a23 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 f35721a..5d81587 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 d1f9075..02691b7 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 4f16c66..0ac2bfe 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 343384f..1c53fbe 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 407c4b3..9c46f30 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 8ed915a..08ffd5c 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 eb99ad1..b2ca6bb 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[] = {
More information about the bind10-changes
mailing list