BIND 10 trac2491, updated. 9430b1fef5b2cc99e97b4cfaf6ccd518bcd76198 [2491] Changes as a result of the review.
BIND 10 source code commits
bind10-changes at lists.isc.org
Thu Dec 6 15:39:13 UTC 2012
The branch, trac2491 has been updated
via 9430b1fef5b2cc99e97b4cfaf6ccd518bcd76198 (commit)
from c831a1e66e88bceb6a317dd8de0796b315681f4f (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 9430b1fef5b2cc99e97b4cfaf6ccd518bcd76198
Author: Marcin Siodelski <marcin at isc.org>
Date: Thu Dec 6 16:39:04 2012 +0100
[2491] Changes as a result of the review.
-----------------------------------------------------------------------
Summary of changes:
src/bin/dhcp6/dhcp6_srv.cc | 2 +-
src/lib/dhcp/libdhcp++.cc | 50 +++++++++---------
src/lib/dhcp/option_custom.cc | 60 +++++++++++++++++++---
src/lib/dhcp/option_custom.h | 2 +-
src/lib/dhcp/option_data_types.cc | 12 ++---
src/lib/dhcp/option_data_types.h | 4 +-
src/lib/dhcp/option_definition.cc | 37 +++++++++----
src/lib/dhcp/tests/libdhcp++_unittest.cc | 4 +-
src/lib/dhcp/tests/option_custom_unittest.cc | 16 ++----
src/lib/dhcp/tests/option_data_types_unittest.cc | 17 +++---
10 files changed, 125 insertions(+), 79 deletions(-)
-----------------------------------------------------------------------
diff --git a/src/bin/dhcp6/dhcp6_srv.cc b/src/bin/dhcp6/dhcp6_srv.cc
index 122345c..33bab99 100644
--- a/src/bin/dhcp6/dhcp6_srv.cc
+++ b/src/bin/dhcp6/dhcp6_srv.cc
@@ -368,7 +368,7 @@ OptionPtr Dhcpv6Srv::createStatusCode(uint16_t code, const std::string& text) {
assert(option_status);
// Set status code to 'code' (0 - means data field #0).
- option_status->writeInteger<uint16_t>(code, 0);
+ option_status->writeInteger(code, 0);
// Set a message (1 - means data field #1).
option_status->writeString(text, 1);
return (option_status);
diff --git a/src/lib/dhcp/libdhcp++.cc b/src/lib/dhcp/libdhcp++.cc
index d326557..934638f 100644
--- a/src/lib/dhcp/libdhcp++.cc
+++ b/src/lib/dhcp/libdhcp++.cc
@@ -340,28 +340,29 @@ LibDHCP::initStdOptionDefs6() {
// Some of the options comprise a "record" of data fields so
// we have to add those fields here.
switch(params[i].code) {
- case D6O_IA_NA:
- case D6O_IA_PD:
- for (int j = 0; j < 3; ++j) {
- definition->addRecordField(OPT_UINT32_TYPE);
- }
+ case D6O_CLIENT_FQDN:
+ definition->addRecordField(OPT_UINT8_TYPE);
+ definition->addRecordField(OPT_FQDN_TYPE);
+ break;
+ case D6O_GEOCONF_CIVIC:
+ definition->addRecordField(OPT_UINT8_TYPE);
+ definition->addRecordField(OPT_UINT16_TYPE);
+ definition->addRecordField(OPT_BINARY_TYPE);
break;
case D6O_IAADDR:
definition->addRecordField(OPT_IPV6_ADDRESS_TYPE);
definition->addRecordField(OPT_UINT32_TYPE);
definition->addRecordField(OPT_UINT32_TYPE);
break;
- case D6O_STATUS_CODE:
- definition->addRecordField(OPT_UINT16_TYPE);
- definition->addRecordField(OPT_STRING_TYPE);
- break;
- case D6O_VENDOR_CLASS:
+ case D6O_IA_NA:
+ definition->addRecordField(OPT_UINT32_TYPE);
+ definition->addRecordField(OPT_UINT32_TYPE);
definition->addRecordField(OPT_UINT32_TYPE);
- definition->addRecordField(OPT_BINARY_TYPE);
break;
- case D6O_VENDOR_OPTS:
+ case D6O_IA_PD:
+ definition->addRecordField(OPT_UINT32_TYPE);
+ definition->addRecordField(OPT_UINT32_TYPE);
definition->addRecordField(OPT_UINT32_TYPE);
- definition->addRecordField(OPT_BINARY_TYPE);
break;
case D6O_IAPREFIX:
definition->addRecordField(OPT_UINT32_TYPE);
@@ -369,25 +370,28 @@ LibDHCP::initStdOptionDefs6() {
definition->addRecordField(OPT_UINT8_TYPE);
definition->addRecordField(OPT_BINARY_TYPE);
break;
- case D6O_GEOCONF_CIVIC:
+ case D6O_LQ_QUERY:
definition->addRecordField(OPT_UINT8_TYPE);
- definition->addRecordField(OPT_UINT16_TYPE);
+ definition->addRecordField(OPT_IPV6_ADDRESS_TYPE);
+ break;
+ case D6O_LQ_RELAY_DATA:
+ definition->addRecordField(OPT_IPV6_ADDRESS_TYPE);
definition->addRecordField(OPT_BINARY_TYPE);
break;
case D6O_REMOTE_ID:
definition->addRecordField(OPT_UINT32_TYPE);
definition->addRecordField(OPT_BINARY_TYPE);
break;
- case D6O_CLIENT_FQDN:
- definition->addRecordField(OPT_UINT8_TYPE);
- definition->addRecordField(OPT_FQDN_TYPE);
+ case D6O_STATUS_CODE:
+ definition->addRecordField(OPT_UINT16_TYPE);
+ definition->addRecordField(OPT_STRING_TYPE);
break;
- case D6O_LQ_QUERY:
- definition->addRecordField(OPT_UINT8_TYPE);
- definition->addRecordField(OPT_IPV6_ADDRESS_TYPE);
+ case D6O_VENDOR_CLASS:
+ definition->addRecordField(OPT_UINT32_TYPE);
+ definition->addRecordField(OPT_BINARY_TYPE);
break;
- case D6O_LQ_RELAY_DATA:
- definition->addRecordField(OPT_IPV6_ADDRESS_TYPE);
+ case D6O_VENDOR_OPTS:
+ definition->addRecordField(OPT_UINT32_TYPE);
definition->addRecordField(OPT_BINARY_TYPE);
break;
default:
diff --git a/src/lib/dhcp/option_custom.cc b/src/lib/dhcp/option_custom.cc
index dccac84..0c3cb81 100644
--- a/src/lib/dhcp/option_custom.cc
+++ b/src/lib/dhcp/option_custom.cc
@@ -127,7 +127,16 @@ OptionCustom::createBuffers() {
std::vector<OptionBuffer> buffers;
OptionDataType data_type = definition_.getType();
+ // This function is called when an empty data buffer has been
+ // passed to the constructor. In such cases values for particular
+ // data fields will be set using modifier functions but for now
+ // we need to initialize a set of buffers that are specified
+ // for an option by its definition. Since there is no data yet,
+ // we are going to fill these buffers with default values.
if (data_type == OPT_RECORD_TYPE) {
+ // For record types we need to iterate over all data fields
+ // specified in option definition and create corresponding
+ // buffers for each of them.
const OptionDefinition::RecordFieldsCollection fields =
definition_.getRecordFields();
@@ -135,28 +144,52 @@ OptionCustom::createBuffers() {
field != fields.end(); ++field) {
OptionBuffer buf;
+ // For data types that have a fixed size we can use the
+ // utility function to get the buffer's size.
size_t data_size = OptionDataTypeUtil::getDataTypeLen(*field);
+ // For variable data sizes the utility function returns zero.
+ // It is ok for string values because the default string
+ // is 'empty'. However for FQDN the empty value is not valid
+ // so we initialize it to '.'.
if (data_size == 0 &&
*field == OPT_FQDN_TYPE) {
OptionDataTypeUtil::writeFqdn(".", buf);
} else {
+ // At this point we can resize the buffer. Note that
+ // for string values we are setting the empty buffer
+ // here.
buf.resize(data_size);
}
+ // We have the buffer with default value prepared so we
+ // add it to the set of buffers.
buffers.push_back(buf);
}
} else if (!definition_.getArrayType() &&
data_type != OPT_EMPTY_TYPE) {
+ // For either 'empty' options we don't have to create any buffers
+ // for obvious reason. For arrays we also don't create any buffers
+ // yet because the set of fields that belong to the array is open
+ // ended so we can't allocate required buffers until we know how
+ // many of them are needed.
+ // For non-arrays we have a single value being held by the option
+ // so we have to allocate exactly one buffer.
OptionBuffer buf;
size_t data_size = OptionDataTypeUtil::getDataTypeLen(data_type);
if (data_size == 0 &&
data_type == OPT_FQDN_TYPE) {
OptionDataTypeUtil::writeFqdn(".", buf);
} else {
+ // Note that if our option holds a string value then
+ // we are making empty buffer here.
buf.resize(data_size);
}
+ // Add a buffer that we have created and leave.
buffers.push_back(buf);
}
+ // The 'swap' is used here because we want to make sure that we
+ // don't touch buffers_ until we successfully allocate all
+ // buffers to be stored there.
std::swap(buffers, buffers_);
}
@@ -193,8 +226,12 @@ OptionCustom::createBuffers(const OptionBuffer& data_buf) {
// to obtain the length of the data is to read the FQDN. The
// utility function will return the size of the buffer on success.
if (*field == OPT_FQDN_TYPE) {
- OptionDataTypeUtil::readFqdn(OptionBuffer(data, data_buf.end()),
- data_size);
+ std::string fqdn =
+ OptionDataTypeUtil::readFqdn(OptionBuffer(data, data_buf.end()));
+ // The size of the buffer holding an FQDN is always
+ // 1 byte larger than the size of the string
+ // representation of this FQDN.
+ data_size = fqdn.size() + 1;
} else {
// In other case we are dealing with string or binary value
// which size can't be determined. Thus we consume the
@@ -248,8 +285,12 @@ OptionCustom::createBuffers(const OptionBuffer& data_buf) {
// a buffer so we have to actually read the FQDN from a buffer
// to get it.
if (data_type == OPT_FQDN_TYPE) {
- OptionDataTypeUtil::readFqdn(OptionBuffer(data, data_buf.end()),
- data_size);
+ std::string fqdn =
+ OptionDataTypeUtil::readFqdn(OptionBuffer(data, data_buf.end()));
+ // The size of the buffer holding an FQDN is always
+ // 1 byte larger than the size of the string
+ // representation of this FQDN.
+ data_size = fqdn.size() + 1;
}
// We don't perform other checks for data types that can't be
// used together with array indicator such as strings, empty field
@@ -275,8 +316,12 @@ OptionCustom::createBuffers(const OptionBuffer& data_buf) {
if (data_size == 0) {
// For FQDN we get the size by actually reading the FQDN.
if (data_type == OPT_FQDN_TYPE) {
- OptionDataTypeUtil::readFqdn(OptionBuffer(data, data_buf.end()),
- data_size);
+ std::string fqdn =
+ OptionDataTypeUtil::readFqdn(OptionBuffer(data, data_buf.end()));
+ // The size of the buffer holding an FQDN is always
+ // 1 bytes larger than the size of the string
+ // representation of this FQDN.
+ data_size = fqdn.size() + 1;
} else {
data_size = std::distance(data, data_buf.end());
}
@@ -454,8 +499,7 @@ OptionCustom::writeBoolean(const bool value, const uint32_t index) {
std::string
OptionCustom::readFqdn(const uint32_t index) const {
checkIndex(index);
- size_t len = 0;
- return (OptionDataTypeUtil::readFqdn(buffers_[index], len));
+ return (OptionDataTypeUtil::readFqdn(buffers_[index]));
}
void
diff --git a/src/lib/dhcp/option_custom.h b/src/lib/dhcp/option_custom.h
index a3d16ef..d9a4c18 100644
--- a/src/lib/dhcp/option_custom.h
+++ b/src/lib/dhcp/option_custom.h
@@ -200,7 +200,7 @@ public:
/// @return read integer value.
template<typename T>
T readInteger(const uint32_t index = 0) const {
- // Check thet tha index is not out of range.
+ // Check that the index is not out of range.
checkIndex(index);
// Check that T points to a valid integer type and this type
// is consistent with an option definition.
diff --git a/src/lib/dhcp/option_data_types.cc b/src/lib/dhcp/option_data_types.cc
index 459dea7..0027cfe 100644
--- a/src/lib/dhcp/option_data_types.cc
+++ b/src/lib/dhcp/option_data_types.cc
@@ -209,26 +209,20 @@ OptionDataTypeUtil::writeBool(const bool value,
}
std::string
-OptionDataTypeUtil::readFqdn(const std::vector<uint8_t>& buf,
- size_t& len) {
- len = 0;
-
+OptionDataTypeUtil::readFqdn(const std::vector<uint8_t>& buf) {
// If buffer is empty emit an error.
if (buf.empty()) {
isc_throw(BadDataTypeCast, "unable to read FQDN from a buffer."
<< " The buffer is empty.");
}
// Copy the data from a buffer to InputBuffer so as we can use
- // isc::dns::Name object to get the FQDN. This is not the most
- // efficient way to do it but currently there is no construtor
- // in Name that would use std::vector directly.
+ // isc::dns::Name object to get the FQDN.
isc::util::InputBuffer in_buf(static_cast<const void*>(&buf[0]), buf.size());
try {
// Try to create an object from the buffer. If exception is thrown
// it means that the buffer doesn't hold a valid domain name (invalid
// syntax).
isc::dns::Name name(in_buf);
- len = name.getLength();
return (name.toText());
} catch (const isc::Exception& ex) {
// Unable to convert the data in the buffer into FQDN.
@@ -246,7 +240,7 @@ OptionDataTypeUtil::writeFqdn(const std::string& fqdn,
buf.resize(buf.size() + labels.getDataLength());
size_t read_len = 0;
const uint8_t* data = labels.getData(&read_len);
- memcpy(static_cast<void*>(&buf[buf.size() - labels.getDataLength()]),
+ memcpy(static_cast<void*>(&buf[buf.size() - read_len]),
data, read_len);
}
} catch (const isc::Exception& ex) {
diff --git a/src/lib/dhcp/option_data_types.h b/src/lib/dhcp/option_data_types.h
index 1628735..ff5789d 100644
--- a/src/lib/dhcp/option_data_types.h
+++ b/src/lib/dhcp/option_data_types.h
@@ -351,13 +351,11 @@ public:
/// section 3.1.
///
/// @param buf input buffer holding a FQDN.
- /// @param [out] len number of bytes read from a buffer.
///
/// @throw BadDataTypeCast if a FQDN stored within a buffer is
/// invalid (e.g. empty, contains invalid characters, truncated).
/// @return fully qualified domain name in a text form.
- static std::string readFqdn(const std::vector<uint8_t>& buf,
- size_t& len);
+ static std::string readFqdn(const std::vector<uint8_t>& buf);
/// @brief Append FQDN into a buffer.
///
diff --git a/src/lib/dhcp/option_definition.cc b/src/lib/dhcp/option_definition.cc
index 434c6a2..7ed48e4 100644
--- a/src/lib/dhcp/option_definition.cc
+++ b/src/lib/dhcp/option_definition.cc
@@ -90,7 +90,7 @@ OptionDefinition::optionFactory(Option::Universe u, uint16_t type,
case OPT_UINT8_TYPE:
return (array_type_ ? factoryGeneric(u, type, begin, end) :
- factoryInteger<uint8_t>(u, type, begin, end));;
+ factoryInteger<uint8_t>(u, type, begin, end));
case OPT_INT8_TYPE:
return (array_type_ ? factoryGeneric(u, type, begin, end) :
@@ -113,27 +113,44 @@ OptionDefinition::optionFactory(Option::Universe u, uint16_t type,
factoryInteger<int32_t>(u, type, begin, end));
case OPT_IPV4_ADDRESS_TYPE:
+ // If definition specifies that an option is an array
+ // of IPv4 addresses we return an instance of specialized
+ // class (OptionAddrLst4). For non-array types there is no
+ // specialized class yet implemented so we drop through
+ // to return an instance of OptionCustom.
if (array_type_) {
return (factoryAddrList4(type, begin, end));
}
break;
case OPT_IPV6_ADDRESS_TYPE:
+ // Handle array type only here (see comments for
+ // OPT_IPV4_ADDRESS_TYPE case).
if (array_type_) {
return (factoryAddrList6(type, begin, end));
}
break;
default:
- if (u == Option::V6 &&
- (code_ == D6O_IA_NA || code_ == D6O_IA_PD) &&
- haveIA6Format()) {
- return (factoryIA6(type, begin, end));
-
- } else if (u == Option::V6 &&
- code_ == D6O_IAADDR &&
- haveIAAddr6Format()) {
- return (factoryIAAddr6(type, begin, end));
+ if (u == Option::V6) {
+ if ((code_ == D6O_IA_NA || code_ == D6O_IA_PD) &&
+ haveIA6Format()) {
+ // Return Option6IA instance for IA_PD and IA_NA option
+ // types only. We don't want to return Option6IA for other
+ // options that comprise 3 UINT32 data fields because
+ // Option6IA accessors' and modifiers' names are derived
+ // from the IA_NA and IA_PD options' field names: IAID,
+ // T1, T2. Using functions such as getIAID, getT1 etc. for
+ // options other than IA_NA and IA_PD would be bad practice
+ // and cause confusion.
+ return (factoryIA6(type, begin, end));
+
+ } else if (code_ == D6O_IAADDR && haveIAAddr6Format()) {
+ // Rerurn Option6IAAddr option instance for the IAADDR
+ // option only for the same reasons as described in
+ // for IA_NA and IA_PD above.
+ return (factoryIAAddr6(type, begin, end));
+ }
}
}
return (OptionPtr(new OptionCustom(*this, u, OptionBuffer(begin, end))));
diff --git a/src/lib/dhcp/tests/libdhcp++_unittest.cc b/src/lib/dhcp/tests/libdhcp++_unittest.cc
index d65d4a8..8ead521 100644
--- a/src/lib/dhcp/tests/libdhcp++_unittest.cc
+++ b/src/lib/dhcp/tests/libdhcp++_unittest.cc
@@ -69,8 +69,8 @@ public:
const std::type_info& expected_type) {
// Get all option definitions, we will use them to extract
// the definition for a particular option code.
- // We don't have to initialize option deinitions here because they
- // are initialized in the class'es constructor.
+ // We don't have to initialize option definitions here because they
+ // are initialized in the class's constructor.
OptionDefContainer options = LibDHCP::getOptionDefs(Option::V6);
// Get the container index #1. This one allows for searching
// option definitions using option code.
diff --git a/src/lib/dhcp/tests/option_custom_unittest.cc b/src/lib/dhcp/tests/option_custom_unittest.cc
index ec6b53d..044279b 100644
--- a/src/lib/dhcp/tests/option_custom_unittest.cc
+++ b/src/lib/dhcp/tests/option_custom_unittest.cc
@@ -686,11 +686,11 @@ TEST_F(OptionCustomTest, recordData) {
};
OptionBuffer buf;
- // Initialize field 0.
+ // Initialize field 0 to 8712.
writeInt<uint16_t>(8712, buf);
// Initialize field 1 to 'true'
buf.push_back(static_cast<unsigned short>(1));
- // Initialize field 2.
+ // Initialize field 2 to 'mydomain.example.com'.
buf.insert(buf.end(), fqdn_data, fqdn_data + sizeof(fqdn_data));
// Initialize field 3 to IPv4 address.
writeAddress(IOAddress("192.168.0.1"), buf);
@@ -700,18 +700,12 @@ TEST_F(OptionCustomTest, recordData) {
writeString("ABCD", buf);
boost::scoped_ptr<OptionCustom> option;
- try {
- option.reset(new OptionCustom(opt_def, Option::V6, buf.begin(), buf.end()));
- } catch (const Exception& ex) {
- std::cout << ex.what() << std::endl;
- }
-
ASSERT_NO_THROW(
option.reset(new OptionCustom(opt_def, Option::V6, buf.begin(), buf.end()));
);
ASSERT_TRUE(option);
- // We should have 5 data fields.
+ // We should have 6 data fields.
ASSERT_EQ(6, option->getDataFieldsNum());
// Verify value in the field 0.
@@ -739,7 +733,7 @@ TEST_F(OptionCustomTest, recordData) {
ASSERT_NO_THROW(value4 = option->readAddress(4));
EXPECT_EQ("2001:db8:1::1", value4.toText());
- // Verify value in the field 4.
+ // Verify value in the field 5.
std::string value5;
ASSERT_NO_THROW(value5 = option->readString(5));
EXPECT_EQ("ABCD", value5);
@@ -880,7 +874,7 @@ TEST_F(OptionCustomTest, setUint32Data) {
EXPECT_EQ(1234, value);
}
-// The purpose of this test is to verify that an opton comprising
+// The purpose of this test is to verify that an option comprising
// single IPv4 address can be created and that this address can
// be overriden by a new value.
TEST_F(OptionCustomTest, setIpv4AddressData) {
diff --git a/src/lib/dhcp/tests/option_data_types_unittest.cc b/src/lib/dhcp/tests/option_data_types_unittest.cc
index a185cdf..748a84b 100644
--- a/src/lib/dhcp/tests/option_data_types_unittest.cc
+++ b/src/lib/dhcp/tests/option_data_types_unittest.cc
@@ -216,6 +216,9 @@ TEST_F(OptionDataTypesTest, writeBool) {
// 'true' value being written.
ASSERT_NO_THROW(OptionDataTypeUtil::writeBool(false, buf));
ASSERT_EQ(2, buf.size());
+ // Check that the first value has not changed.
+ EXPECT_EQ(buf[0], 1);
+ // Check the the second value is correct.
EXPECT_EQ(buf[1], 0);
}
@@ -330,17 +333,11 @@ TEST_F(OptionDataTypesTest, writeInt) {
// integer value. Eventually the buffer holds all values and should
// match with the reference buffer.
std::vector<uint8_t> buf;
- // Write uint8_t
ASSERT_NO_THROW(OptionDataTypeUtil::writeInt<uint8_t>(127, buf));
- // Write uint16_t
ASSERT_NO_THROW(OptionDataTypeUtil::writeInt<uint16_t>(1023, buf));
- // Write uint32_t
ASSERT_NO_THROW(OptionDataTypeUtil::writeInt<uint32_t>(4096, buf));
- // Write int32_t
ASSERT_NO_THROW(OptionDataTypeUtil::writeInt<int32_t>(-1024, buf));
- // Write int16_t
ASSERT_NO_THROW(OptionDataTypeUtil::writeInt<int16_t>(512, buf));
- // Write int8_t
ASSERT_NO_THROW(OptionDataTypeUtil::writeInt<int8_t>(-127, buf));
// Make sure that the buffer has the same size as the reference
@@ -372,10 +369,8 @@ TEST_F(OptionDataTypesTest, readFqdn) {
// Read the buffer as FQDN and verify its correctness.
std::string fqdn;
- size_t len = 0;
- EXPECT_NO_THROW(fqdn = OptionDataTypeUtil::readFqdn(buf, len));
+ EXPECT_NO_THROW(fqdn = OptionDataTypeUtil::readFqdn(buf));
EXPECT_EQ("mydomain.example.com.", fqdn);
- EXPECT_EQ(len, buf.size());
// By resizing the buffer we simulate truncation. The first
// length field (8) indicate that the first label's size is
@@ -383,14 +378,14 @@ TEST_F(OptionDataTypesTest, readFqdn) {
// fails.
buf.resize(5);
EXPECT_THROW(
- OptionDataTypeUtil::readFqdn(buf, len),
+ OptionDataTypeUtil::readFqdn(buf),
isc::dhcp::BadDataTypeCast
);
// Another special case: provide an empty buffer.
buf.clear();
EXPECT_THROW(
- OptionDataTypeUtil::readFqdn(buf, len),
+ OptionDataTypeUtil::readFqdn(buf),
isc::dhcp::BadDataTypeCast
);
}
More information about the bind10-changes
mailing list