BIND 10 trac3180, updated. c71753a27a64d409f28629f308dfe9f7afb7e0ae [3180] Addressed review comments.
BIND 10 source code commits
bind10-changes at lists.isc.org
Wed Oct 9 07:41:22 UTC 2013
The branch, trac3180 has been updated
via c71753a27a64d409f28629f308dfe9f7afb7e0ae (commit)
from ff3ddd8332fbaab5ff76f140ae9f185a2d2d6e34 (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 c71753a27a64d409f28629f308dfe9f7afb7e0ae
Author: Marcin Siodelski <marcin at isc.org>
Date: Tue Oct 8 21:05:48 2013 +0200
[3180] Addressed review comments.
-----------------------------------------------------------------------
Summary of changes:
src/bin/dhcp4/dhcp4_srv.cc | 4 +--
src/bin/dhcp4/tests/dhcp4_srv_unittest.cc | 1 +
src/lib/dhcp/option.h | 28 ++++++++++++---
src/lib/dhcp/option_int.h | 2 ++
src/lib/dhcp/pkt4.cc | 53 ++++++++++++++++-------------
src/lib/dhcp/pkt6.cc | 3 ++
src/lib/dhcp/tests/option_unittest.cc | 11 +++---
tests/tools/perfdhcp/pkt_transform.cc | 12 +++----
tests/tools/perfdhcp/pkt_transform.h | 8 ++---
9 files changed, 78 insertions(+), 44 deletions(-)
-----------------------------------------------------------------------
diff --git a/src/bin/dhcp4/dhcp4_srv.cc b/src/bin/dhcp4/dhcp4_srv.cc
index c6332a9..b7b9ef6 100644
--- a/src/bin/dhcp4/dhcp4_srv.cc
+++ b/src/bin/dhcp4/dhcp4_srv.cc
@@ -1181,9 +1181,9 @@ Dhcpv4Srv::unpackOptions(const OptionBuffer& buf,
size_t offset = 0;
OptionDefContainer option_defs;
- if (option_space == "dhcp6") {
+ if (option_space == "dhcp4") {
// Get the list of stdandard option definitions.
- option_defs = LibDHCP::getOptionDefs(Option::V6);
+ option_defs = LibDHCP::getOptionDefs(Option::V4);
} else if (!option_space.empty()) {
OptionDefContainerPtr option_defs_ptr =
CfgMgr::instance().getOptionDefs(option_space);
diff --git a/src/bin/dhcp4/tests/dhcp4_srv_unittest.cc b/src/bin/dhcp4/tests/dhcp4_srv_unittest.cc
index 2b33630..48daa77 100644
--- a/src/bin/dhcp4/tests/dhcp4_srv_unittest.cc
+++ b/src/bin/dhcp4/tests/dhcp4_srv_unittest.cc
@@ -1664,6 +1664,7 @@ TEST_F(Dhcpv4SrvTest, Hooks) {
// - option (option space 'foobar')
// - sub option (option space 'foo')
// - sub option (option space 'bar')
+// @todo Add more thorough unit tests for unpackOptions.
TEST_F(Dhcpv4SrvTest, unpackOptions) {
// Create option definition for each level of encapsulation. Each option
// definition is for the option code 1. Options may have the same
diff --git a/src/lib/dhcp/option.h b/src/lib/dhcp/option.h
index 85fa165..dbdb3af 100644
--- a/src/lib/dhcp/option.h
+++ b/src/lib/dhcp/option.h
@@ -45,12 +45,32 @@ typedef boost::shared_ptr<OptionBuffer> OptionBufferPtr;
class Option;
typedef boost::shared_ptr<Option> OptionPtr;
-/// A collection of DHCPv6 options
+/// A collection of DHCP (v4 or v6) options
typedef std::multimap<unsigned int, OptionPtr> OptionCollection;
-/// This type describes a callback function to parse options from buffer.
-typedef boost::function< size_t(const OptionBuffer&, const std::string,
- OptionCollection&, size_t*, size_t*)
+/// @brief This type describes a callback function to parse options from buffer.
+///
+/// @note The last two parameters should be specified in the callback function
+/// parameters list only if DHCPv6 options are parsed. Exclude these parameters
+/// from the callback function defined to parse DHCPv4 options.
+///
+/// @param buffer A buffer holding options to be parsed.
+/// @param encapsulated_space A name of the option space to which options being
+/// parsed belong.
+/// @param [out] options A container to which parsed options should be appended.
+/// @param relay_msg_offset A pointer to a size_t value. It indicates the
+/// offset to beginning of relay_msg option. This parameter should be specified
+/// for DHCPv6 options only.
+/// @param relay_msg_len A pointer to a size_t value. It holds the length of
+/// of the relay_msg option. This parameter should be specified for DHCPv6
+/// options only.
+///
+/// @return An offset to the first byte after last parsed option.
+typedef boost::function< size_t(const OptionBuffer& buffer,
+ const std::string encapsulated_space,
+ OptionCollection& options,
+ size_t* relay_msg_offset,
+ size_t* relay_msg_len)
> UnpackOptionsCallback;
diff --git a/src/lib/dhcp/option_int.h b/src/lib/dhcp/option_int.h
index 46fe148..cbdbcb0 100644
--- a/src/lib/dhcp/option_int.h
+++ b/src/lib/dhcp/option_int.h
@@ -47,6 +47,7 @@ public:
///
/// @throw isc::dhcp::InvalidDataType if data field type provided
/// as template parameter is not a supported integer type.
+ /// @todo Extend constructor to set encapsulated option space name.
OptionInt(Option::Universe u, uint16_t type, T value)
: Option(u, type), value_(value) {
if (!OptionDataTypeTraits<T>::integer_type) {
@@ -69,6 +70,7 @@ public:
/// @throw isc::OutOfRange if provided buffer is shorter than data size.
/// @throw isc::dhcp::InvalidDataType if data field type provided
/// as template parameter is not a supported integer type.
+ /// @todo Extend constructor to set encapsulated option space name.
OptionInt(Option::Universe u, uint16_t type, OptionBufferConstIter begin,
OptionBufferConstIter end)
: Option(u, type) {
diff --git a/src/lib/dhcp/pkt4.cc b/src/lib/dhcp/pkt4.cc
index fa83173..4fc1c42 100644
--- a/src/lib/dhcp/pkt4.cc
+++ b/src/lib/dhcp/pkt4.cc
@@ -162,61 +162,66 @@ void
Pkt4::unpack() {
// input buffer (used during message reception)
- isc::util::InputBuffer bufferIn(&data_[0], data_.size());
+ isc::util::InputBuffer buffer_in(&data_[0], data_.size());
- if (bufferIn.getLength() < DHCPV4_PKT_HDR_LEN) {
+ if (buffer_in.getLength() < DHCPV4_PKT_HDR_LEN) {
isc_throw(OutOfRange, "Received truncated DHCPv4 packet (len="
- << bufferIn.getLength() << " received, at least "
+ << buffer_in.getLength() << " received, at least "
<< DHCPV4_PKT_HDR_LEN << "is expected");
}
- op_ = bufferIn.readUint8();
- uint8_t htype = bufferIn.readUint8();
- uint8_t hlen = bufferIn.readUint8();
- hops_ = bufferIn.readUint8();
- transid_ = bufferIn.readUint32();
- secs_ = bufferIn.readUint16();
- flags_ = bufferIn.readUint16();
- ciaddr_ = IOAddress(bufferIn.readUint32());
- yiaddr_ = IOAddress(bufferIn.readUint32());
- siaddr_ = IOAddress(bufferIn.readUint32());
- giaddr_ = IOAddress(bufferIn.readUint32());
+ op_ = buffer_in.readUint8();
+ uint8_t htype = buffer_in.readUint8();
+ uint8_t hlen = buffer_in.readUint8();
+ hops_ = buffer_in.readUint8();
+ transid_ = buffer_in.readUint32();
+ secs_ = buffer_in.readUint16();
+ flags_ = buffer_in.readUint16();
+ ciaddr_ = IOAddress(buffer_in.readUint32());
+ yiaddr_ = IOAddress(buffer_in.readUint32());
+ siaddr_ = IOAddress(buffer_in.readUint32());
+ giaddr_ = IOAddress(buffer_in.readUint32());
vector<uint8_t> hw_addr(MAX_CHADDR_LEN, 0);
- bufferIn.readVector(hw_addr, MAX_CHADDR_LEN);
- bufferIn.readData(sname_, MAX_SNAME_LEN);
- bufferIn.readData(file_, MAX_FILE_LEN);
+ buffer_in.readVector(hw_addr, MAX_CHADDR_LEN);
+ buffer_in.readData(sname_, MAX_SNAME_LEN);
+ buffer_in.readData(file_, MAX_FILE_LEN);
hw_addr.resize(hlen);
hwaddr_ = HWAddrPtr(new HWAddr(hw_addr, htype));
- if (bufferIn.getLength() == bufferIn.getPosition()) {
+ if (buffer_in.getLength() == buffer_in.getPosition()) {
// this is *NOT* DHCP packet. It does not have any DHCPv4 options. In
// particular, it does not have magic cookie, a 4 byte sequence that
// differentiates between DHCP and BOOTP packets.
isc_throw(InvalidOperation, "Received BOOTP packet. BOOTP is not supported.");
}
- if (bufferIn.getLength() - bufferIn.getPosition() < 4) {
+ if (buffer_in.getLength() - buffer_in.getPosition() < 4) {
// there is not enough data to hold magic DHCP cookie
isc_throw(Unexpected, "Truncated or no DHCP packet.");
}
- uint32_t magic = bufferIn.readUint32();
+ uint32_t magic = buffer_in.readUint32();
if (magic != DHCP_OPTIONS_COOKIE) {
isc_throw(Unexpected, "Invalid or missing DHCP magic cookie");
}
- size_t opts_len = bufferIn.getLength() - bufferIn.getPosition();
+ size_t opts_len = buffer_in.getLength() - buffer_in.getPosition();
vector<uint8_t> opts_buffer;
- // First use of readVector.
- bufferIn.readVector(opts_buffer, opts_len);
+ // Use readVector because a function which parses option requires
+ // a vector as an input.
+ buffer_in.readVector(opts_buffer, opts_len);
if (callback_.empty()) {
LibDHCP::unpackOptions4(opts_buffer, options_);
} else {
- callback_(opts_buffer, "dhcp4", options_, 0, 0);
+ // The last two arguments are set to NULL because they are
+ // specific to DHCPv6 options parsing. They are unused for
+ // DHCPv4 case. In DHCPv6 case they hold are the relay message
+ // offset and length.
+ callback_(opts_buffer, "dhcp4", options_, NULL, NULL);
}
// @todo check will need to be called separately, so hooks can be called
diff --git a/src/lib/dhcp/pkt6.cc b/src/lib/dhcp/pkt6.cc
index ed3765b..c4dcfbb 100644
--- a/src/lib/dhcp/pkt6.cc
+++ b/src/lib/dhcp/pkt6.cc
@@ -330,6 +330,9 @@ Pkt6::unpackMsg(OptionBuffer::const_iterator begin,
if (callback_.empty()) {
LibDHCP::unpackOptions6(opt_buffer, options_);
} else {
+ // The last two arguments hold the DHCPv6 Relay message offset and
+ // length. Setting them to NULL because we are dealing with the
+ // not-relayed message.
callback_(opt_buffer, "dhcp6", options_, 0, 0);
}
} catch (const Exception& e) {
diff --git a/src/lib/dhcp/tests/option_unittest.cc b/src/lib/dhcp/tests/option_unittest.cc
index ba36fbb..5dbc3eb 100644
--- a/src/lib/dhcp/tests/option_unittest.cc
+++ b/src/lib/dhcp/tests/option_unittest.cc
@@ -591,11 +591,11 @@ TEST_F(OptionTest, unpackCallback) {
// Create a buffer which holds two sub options.
const char opt_data[] = {
0x00, 0x01, // sub option code = 1
- 0x00, 0x02, // sub option length = 1
- 0x00, 0x01, // sub option data
+ 0x00, 0x02, // sub option length = 2
+ 0x00, 0x01, // sub option data (2 bytes)
0x00, 0x02, // sub option code = 2
- 0x00, 0x02, // sub option length
- 0x00, 0x01 // sub option data
+ 0x00, 0x02, // sub option length = 2
+ 0x01, 0x01 // sub option data (2 bytes)
};
OptionBuffer opt_buf(opt_data, opt_data + sizeof(opt_data));
@@ -605,6 +605,9 @@ TEST_F(OptionTest, unpackCallback) {
ASSERT_FALSE(cb.executed_);
// Create an option and install a callback.
NakedOption option;
+ // Parameters from _1 to _5 are placeholders for the actual values
+ // to be passed to the callback function. See: boost::bind documentation
+ // at http://www.boost.org/doc/libs/1_54_0/libs/bind/bind.html.
option.setCallback(boost::bind(&CustomUnpackCallback::execute, &cb,
_1, _2, _3, _4, _5));
// Parse options. It should result in a call to our callback function.
diff --git a/tests/tools/perfdhcp/pkt_transform.cc b/tests/tools/perfdhcp/pkt_transform.cc
index 15f15f1..0267d33 100644
--- a/tests/tools/perfdhcp/pkt_transform.cc
+++ b/tests/tools/perfdhcp/pkt_transform.cc
@@ -32,7 +32,7 @@ namespace perfdhcp {
bool
PktTransform::pack(const Option::Universe universe,
const OptionBuffer& in_buffer,
- const Option::OptionCollection& options,
+ const OptionCollection& options,
const size_t transid_offset,
const uint32_t transid,
util::OutputBuffer& out_buffer) {
@@ -75,7 +75,7 @@ PktTransform::pack(const Option::Universe universe,
bool
PktTransform::unpack(const Option::Universe universe,
const OptionBuffer& in_buffer,
- const Option::OptionCollection& options,
+ const OptionCollection& options,
const size_t transid_offset,
uint32_t& transid) {
@@ -113,13 +113,13 @@ PktTransform::unpack(const Option::Universe universe,
void
PktTransform::packOptions(const OptionBuffer& in_buffer,
- const Option::OptionCollection& options,
+ const OptionCollection& options,
util::OutputBuffer& out_buffer) {
try {
// If there are any options on the list, we will use provided
// options offsets to override them in the output buffer
// with new contents.
- for (Option::OptionCollection::const_iterator it = options.begin();
+ for (OptionCollection::const_iterator it = options.begin();
it != options.end(); ++it) {
// Get options with their position (offset).
boost::shared_ptr<LocalizedOption> option =
@@ -157,8 +157,8 @@ PktTransform::packOptions(const OptionBuffer& in_buffer,
void
PktTransform::unpackOptions(const OptionBuffer& in_buffer,
- const Option::OptionCollection& options) {
- for (Option::OptionCollection::const_iterator it = options.begin();
+ const OptionCollection& options) {
+ for (OptionCollection::const_iterator it = options.begin();
it != options.end(); ++it) {
boost::shared_ptr<LocalizedOption> option =
diff --git a/tests/tools/perfdhcp/pkt_transform.h b/tests/tools/perfdhcp/pkt_transform.h
index 51c1c0b..c8b7602 100644
--- a/tests/tools/perfdhcp/pkt_transform.h
+++ b/tests/tools/perfdhcp/pkt_transform.h
@@ -66,7 +66,7 @@ public:
/// \return false, if pack operation failed.
static bool pack(const dhcp::Option::Universe universe,
const dhcp::OptionBuffer& in_buffer,
- const dhcp::Option::OptionCollection& options,
+ const dhcp::OptionCollection& options,
const size_t transid_offset,
const uint32_t transid,
util::OutputBuffer& out_buffer);
@@ -88,7 +88,7 @@ public:
/// \return false, if unpack operation failed.
static bool unpack(const dhcp::Option::Universe universe,
const dhcp::OptionBuffer& in_buffer,
- const dhcp::Option::OptionCollection& options,
+ const dhcp::OptionCollection& options,
const size_t transid_offset,
uint32_t& transid);
@@ -135,7 +135,7 @@ private:
///
/// \throw isc::Unexpected if options update failed.
static void packOptions(const dhcp::OptionBuffer& in_buffer,
- const dhcp::Option::OptionCollection& options,
+ const dhcp::OptionCollection& options,
util::OutputBuffer& out_buffer);
/// \brief Reads contents of specified options from buffer.
@@ -159,7 +159,7 @@ private:
///
/// \throw isc::Unexpected if options unpack failed.
static void unpackOptions(const dhcp::OptionBuffer& in_buffer,
- const dhcp::Option::OptionCollection& options);
+ const dhcp::OptionCollection& options);
};
More information about the bind10-changes
mailing list