BIND 10 trac2526, updated. 0ebb7cd975d578f3dea516b790e502fdd65cb683 [2526] Changes as a result of the code review.
BIND 10 source code commits
bind10-changes at lists.isc.org
Fri Dec 14 16:43:00 UTC 2012
The branch, trac2526 has been updated
via 0ebb7cd975d578f3dea516b790e502fdd65cb683 (commit)
from 3759773a821827277aa2c8d457dd6fe63f1daf75 (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 0ebb7cd975d578f3dea516b790e502fdd65cb683
Author: Marcin Siodelski <marcin at isc.org>
Date: Fri Dec 14 17:42:50 2012 +0100
[2526] Changes as a result of the code review.
-----------------------------------------------------------------------
Summary of changes:
src/lib/dhcp/libdhcp++.cc | 51 ++++++++++++++++-------------
src/lib/dhcp/libdhcp++.h | 2 +-
src/lib/dhcp/tests/libdhcp++_unittest.cc | 21 ++++++------
src/lib/dhcp/tests/option_int_unittest.cc | 8 ++++-
src/lib/dhcp/tests/pkt4_unittest.cc | 5 ---
5 files changed, 47 insertions(+), 40 deletions(-)
-----------------------------------------------------------------------
diff --git a/src/lib/dhcp/libdhcp++.cc b/src/lib/dhcp/libdhcp++.cc
index a76bdab..b19ed21 100644
--- a/src/lib/dhcp/libdhcp++.cc
+++ b/src/lib/dhcp/libdhcp++.cc
@@ -102,31 +102,35 @@ LibDHCP::optionFactory(Option::Universe u,
size_t LibDHCP::unpackOptions6(const OptionBuffer& buf,
isc::dhcp::Option::OptionCollection& options) {
size_t offset = 0;
- size_t end = buf.size();
-
- while (offset +4 <= end) {
- uint16_t opt_type = buf[offset] * 256 + buf[offset + 1];
+ size_t length = buf.size();
+
+ // Get the list of stdandard option definitions.
+ const OptionDefContainer& option_defs = LibDHCP::getOptionDefs(Option::V6);
+ // Get the search index #1. It allows to search for option definitions
+ // using option code.
+ const OptionDefContainerTypeIndex& idx = option_defs.get<1>();
+
+ // The buffer being read comprises a set of options, each starting with
+ // a two-byte type code and a two-byte length field.
+ while (offset + 4 <= length) {
+ uint16_t opt_type = isc::util::readUint16(&buf[offset]);
offset += 2;
- uint16_t opt_len = buf[offset] * 256 + buf[offset + 1];
+ uint16_t opt_len = isc::util::readUint16(&buf[offset]);
offset += 2;
- if (offset + opt_len > end) {
+ if (offset + opt_len > length) {
// @todo: consider throwing exception here.
return (offset);
}
- // Get the list of stdandard option definitions.
- OptionDefContainer option_defs = LibDHCP::getOptionDefs(Option::V6);
- // Get the search index #1. It allows to search for option definitions
- // using option code.
- const OptionDefContainerTypeIndex& idx = option_defs.get<1>();
- // Get all options with the particular option code. Note that option code
- // is non-unique within this container however at this point we expect
- // to get one option definition with the particular code. If more are
- // returned we report an error.
+ // Get all definitions with the particular option code. Note that option
+ // code is non-unique within this container however at this point we
+ // expect to get one option definition with the particular code. If more
+ // are returned we report an error.
const OptionDefContainerTypeRange& range = idx.equal_range(opt_type);
// Get the number of returned option definitions for the option code.
size_t num_defs = distance(range.first, range.second);
+
OptionPtr opt;
if (num_defs > 1) {
// Multiple options of the same code are not supported right now!
@@ -164,7 +168,14 @@ size_t LibDHCP::unpackOptions4(const OptionBuffer& buf,
isc::dhcp::Option::OptionCollection& options) {
size_t offset = 0;
- // 2 byte - header of DHCPv4 option
+ // Get the list of stdandard option definitions.
+ const OptionDefContainer& option_defs = LibDHCP::getOptionDefs(Option::V4);
+ // Get the search index #1. It allows to search for option definitions
+ // using option code.
+ const OptionDefContainerTypeIndex& idx = option_defs.get<1>();
+
+ // The buffer being read comprises a set of options, each starting with
+ // a one-byte type code and a one-byte length field.
while (offset + 1 <= buf.size()) {
uint8_t opt_type = buf[offset++];
@@ -191,12 +202,7 @@ size_t LibDHCP::unpackOptions4(const OptionBuffer& buf,
<< "-byte long buffer.");
}
- // Get the list of stdandard option definitions.
- OptionDefContainer option_defs = LibDHCP::getOptionDefs(Option::V4);
- // Get the search index #1. It allows to search for option definitions
- // using option code.
- const OptionDefContainerTypeIndex& idx = option_defs.get<1>();
- // Get all options with the particular option code. Note that option code
+ // Get all definitions with the particular option code. Note that option code
// is non-unique within this container however at this point we expect
// to get one option definition with the particular code. If more are
// returned we report an error.
@@ -204,7 +210,6 @@ size_t LibDHCP::unpackOptions4(const OptionBuffer& buf,
// Get the number of returned option definitions for the option code.
size_t num_defs = distance(range.first, range.second);
-
OptionPtr opt;
if (num_defs > 1) {
// Multiple options of the same code are not supported right now!
diff --git a/src/lib/dhcp/libdhcp++.h b/src/lib/dhcp/libdhcp++.h
index 370106b..c325aa5 100644
--- a/src/lib/dhcp/libdhcp++.h
+++ b/src/lib/dhcp/libdhcp++.h
@@ -140,7 +140,7 @@ private:
///
/// @throw std::bad alloc if system went out of memory.
/// @throw MalformedOptionDefinition if any of the definitions
- /// is incorrect. This is programming error.
+ /// are incorrect. This is programming error.
static void initStdOptionDefs4();
/// Initialize standard DHCPv6 option definitions.
diff --git a/src/lib/dhcp/tests/libdhcp++_unittest.cc b/src/lib/dhcp/tests/libdhcp++_unittest.cc
index d8ad450..818de06 100644
--- a/src/lib/dhcp/tests/libdhcp++_unittest.cc
+++ b/src/lib/dhcp/tests/libdhcp++_unittest.cc
@@ -150,7 +150,8 @@ private:
}
};
-static const uint8_t packed[] = {
+// The DHCPv6 options in the wire format, used by multiple tests.
+const uint8_t v6packed[] = {
0, 1, 0, 5, 100, 101, 102, 103, 104, // CLIENT_ID (9 bytes)
0, 2, 0, 3, 105, 106, 107, // SERVER_ID (7 bytes)
0, 14, 0, 0, // RAPID_COMMIT (0 bytes)
@@ -254,8 +255,8 @@ TEST_F(LibDhcpTest, packOptions6) {
OutputBuffer assembled(512);
EXPECT_NO_THROW(LibDHCP::packOptions6(assembled, opts));
- EXPECT_EQ(sizeof(packed), assembled.getLength());
- EXPECT_EQ(0, memcmp(assembled.getData(), packed, sizeof(packed)));
+ EXPECT_EQ(sizeof(v6packed), assembled.getLength());
+ EXPECT_EQ(0, memcmp(assembled.getData(), v6packed, sizeof(v6packed)));
}
TEST_F(LibDhcpTest, unpackOptions6) {
@@ -267,10 +268,10 @@ TEST_F(LibDhcpTest, unpackOptions6) {
isc::dhcp::Option::OptionCollection options; // list of options
OptionBuffer buf(512);
- memcpy(&buf[0], packed, sizeof(packed));
+ memcpy(&buf[0], v6packed, sizeof(v6packed));
EXPECT_NO_THROW ({
- LibDHCP::unpackOptions6(OptionBuffer(buf.begin(), buf.begin() + sizeof(packed)),
+ LibDHCP::unpackOptions6(OptionBuffer(buf.begin(), buf.begin() + sizeof(v6packed)),
options);
});
@@ -281,14 +282,14 @@ TEST_F(LibDhcpTest, unpackOptions6) {
EXPECT_EQ(1, x->second->getType()); // this should be option 1
ASSERT_EQ(9, x->second->len()); // it should be of length 9
ASSERT_EQ(5, x->second->getData().size());
- EXPECT_EQ(0, memcmp(&x->second->getData()[0], packed + 4, 5)); // data len=5
+ EXPECT_EQ(0, memcmp(&x->second->getData()[0], v6packed + 4, 5)); // data len=5
x = options.find(2);
ASSERT_FALSE(x == options.end()); // option 2 should exist
EXPECT_EQ(2, x->second->getType()); // this should be option 2
ASSERT_EQ(7, x->second->len()); // it should be of length 7
ASSERT_EQ(3, x->second->getData().size());
- EXPECT_EQ(0, memcmp(&x->second->getData()[0], packed + 13, 3)); // data len=3
+ EXPECT_EQ(0, memcmp(&x->second->getData()[0], v6packed + 13, 3)); // data len=3
x = options.find(14);
ASSERT_FALSE(x == options.end()); // option 14 should exist
@@ -316,7 +317,7 @@ TEST_F(LibDhcpTest, unpackOptions6) {
expected_opts.push_back(0x6C6D); // equivalent to: 108, 109
expected_opts.push_back(0x6E6F); // equivalent to 110, 111
ASSERT_EQ(expected_opts.size(), opts.size());
- // Validated if option has been unpacked correctly.
+ // Validated if option has been un packed correctly.
EXPECT_TRUE(std::equal(expected_opts.begin(), expected_opts.end(),
opts.begin()));
@@ -395,11 +396,11 @@ TEST_F(LibDhcpTest, packOptions4) {
TEST_F(LibDhcpTest, unpackOptions4) {
- vector<uint8_t> packed(v4Opts, v4Opts + sizeof(v4Opts));
+ vector<uint8_t> v4packed(v4Opts, v4Opts + sizeof(v4Opts));
isc::dhcp::Option::OptionCollection options; // list of options
ASSERT_NO_THROW(
- LibDHCP::unpackOptions4(packed, options);
+ LibDHCP::unpackOptions4(v4packed, options);
);
isc::dhcp::Option::OptionCollection::const_iterator x = options.find(12);
diff --git a/src/lib/dhcp/tests/option_int_unittest.cc b/src/lib/dhcp/tests/option_int_unittest.cc
index 73659d9..81ebcf0 100644
--- a/src/lib/dhcp/tests/option_int_unittest.cc
+++ b/src/lib/dhcp/tests/option_int_unittest.cc
@@ -457,7 +457,10 @@ TEST_F(OptionIntTest, unpackSuboptions4) {
0x01, 0x02, 0x03, 0x04, // data, uint32_t value = 0x01020304
TEST_OPT_CODE + 1, 0x4, 0x01, 0x01, 0x01, 0x01 // suboption
};
- // Copy the data to a vector so as we can pas it to the
+ // Make sure that the buffer size is sufficient to copy the
+ // elements from the array.
+ ASSERT_GE(buf_.size(), sizeof(expected));
+ // Copy the data to a vector so as we can pass it to the
// OptionInt's constructor.
memcpy(&buf_[0], expected, sizeof(expected));
@@ -513,6 +516,9 @@ TEST_F(OptionIntTest, unpackSuboptions6) {
};
ASSERT_EQ(38, sizeof(expected));
+ // Make sure that the buffer's size is sufficient to
+ // copy the elements from the array.
+ ASSERT_GE(buf_.size(), sizeof(expected));
memcpy(&buf_[0], expected, sizeof(expected));
boost::shared_ptr<OptionInt<uint16_t> > opt;
diff --git a/src/lib/dhcp/tests/pkt4_unittest.cc b/src/lib/dhcp/tests/pkt4_unittest.cc
index eccc039..e484dcc 100644
--- a/src/lib/dhcp/tests/pkt4_unittest.cc
+++ b/src/lib/dhcp/tests/pkt4_unittest.cc
@@ -544,11 +544,6 @@ TEST(Pkt4Test, unpackOptions) {
boost::shared_ptr<Pkt4> pkt(new Pkt4(&expectedFormat[0],
expectedFormat.size()));
- try {
- pkt->unpack();
- } catch (const Exception& ex) {
- std::cout << ex.what() << std::endl;
- }
EXPECT_NO_THROW(
pkt->unpack()
);
More information about the bind10-changes
mailing list