BIND 10 trac1228, updated. 66bb38a4d0cf296f48181d624d22b1074688de38 [1228] InputBuffer::readVector() cleaned up.
BIND 10 source code commits
bind10-changes at lists.isc.org
Wed Nov 2 13:08:07 UTC 2011
The branch, trac1228 has been updated
via 66bb38a4d0cf296f48181d624d22b1074688de38 (commit)
via 7d2826b519f95b2fecd299e15952e897c5a60b2b (commit)
from c3b01cc59ba03c6054af4bae42e08965b3f60eb0 (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 66bb38a4d0cf296f48181d624d22b1074688de38
Author: Tomek Mrugalski <tomasz at isc.org>
Date: Wed Nov 2 14:07:44 2011 +0100
[1228] InputBuffer::readVector() cleaned up.
commit 7d2826b519f95b2fecd299e15952e897c5a60b2b
Author: Tomek Mrugalski <tomasz at isc.org>
Date: Wed Nov 2 14:06:39 2011 +0100
[1228] Changes after review.
ChangeLog entry added (it covers all recent DHCPv4 tickets)
New constructor in Option class.
Added new unittest for new construtor.
Minor improvements in pkt4_unitests.
-----------------------------------------------------------------------
Summary of changes:
ChangeLog | 8 ++++
src/lib/dhcp/libdhcp.cc | 9 ++---
src/lib/dhcp/option.cc | 43 ++++++++++++++++-------
src/lib/dhcp/option.h | 25 +++++++++++++
src/lib/dhcp/tests/option_unittest.cc | 62 ++++++++++++++++++++++++++++++++-
src/lib/dhcp/tests/pkt4_unittest.cc | 12 ++++---
src/lib/util/buffer.h | 8 +---
7 files changed, 136 insertions(+), 31 deletions(-)
-----------------------------------------------------------------------
diff --git a/ChangeLog b/ChangeLog
index f4cba7d..6ad0836 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,11 @@
+ 304. [func] tomek
+ libdhcp: Support for DHCPv4 packet manipulation is now implemented.
+ All fixed fields are now supported. Generic support for DHCPv4
+ options is available (both parsing and assembly). There is no code
+ that uses this new functionality yet, so it is not usable directly
+ at this time. This code will be used by upcoming b10-dhcp4 daemon.
+ (Trac #1228, git TBD)
+
303. [bug] jinmei
Changed the installation path for the UNIX domain file used
for the communication between b10-auth and b10-xfrout to a
diff --git a/src/lib/dhcp/libdhcp.cc b/src/lib/dhcp/libdhcp.cc
index 80b99de..b95a427 100644
--- a/src/lib/dhcp/libdhcp.cc
+++ b/src/lib/dhcp/libdhcp.cc
@@ -102,11 +102,9 @@ LibDHCP::unpackOptions4(const std::vector<uint8_t>& buf,
boost::shared_ptr<Option> opt;
switch(opt_type) {
default:
- /// TODO: Is this intermediate object really necessary here?
- vector<uint8_t> tmpVec(&buf[offset], &buf[offset] + opt_len);
- opt = boost::shared_ptr<Option>(new Option(Option::V4,
- opt_type,
- tmpVec));
+ opt = boost::shared_ptr<Option>(new Option(Option::V4, opt_type,
+ buf.begin()+offset,
+ buf.begin()+offset+opt_len));
}
options.insert(pair<int, boost::shared_ptr<Option> >(opt_type, opt));
@@ -114,7 +112,6 @@ LibDHCP::unpackOptions4(const std::vector<uint8_t>& buf,
}
}
-
unsigned int
LibDHCP::packOptions6(boost::shared_array<uint8_t> data,
unsigned int data_len,
diff --git a/src/lib/dhcp/option.cc b/src/lib/dhcp/option.cc
index 9f8635e..546805d 100644
--- a/src/lib/dhcp/option.cc
+++ b/src/lib/dhcp/option.cc
@@ -31,7 +31,7 @@ using namespace isc::util;
Option::Option(Universe u, unsigned short type)
:universe_(u), type_(type) {
- if (u==V4 && (type>255)) {
+ if ((u == V4) && (type > 255)) {
isc_throw(BadValue, "Can't create V4 option of type "
<< type << ", V4 options are in range 0..255");
}
@@ -43,13 +43,13 @@ Option::Option(Universe u, unsigned short type,
:universe_(u), type_(type),
offset_(offset)
{
- if (u==V4 && (type>255)) {
+ if ( (u == V4) && (type > 255)) {
isc_throw(BadValue, "Can't create V4 option of type "
<< type << ", V4 options are in range 0..255");
}
uint8_t* ptr = &buf[offset];
- data_ = std::vector<uint8_t>(ptr, ptr+len);
+ data_ = std::vector<uint8_t>(ptr, ptr + len);
// sanity checks
// TODO: universe must be in V4 and V6
@@ -57,13 +57,30 @@ Option::Option(Universe u, unsigned short type,
Option::Option(Universe u, unsigned short type, std::vector<uint8_t>& data)
:universe_(u), type_(type), data_(data) {
- if (u==V4 && data.size() > 255) {
+ if ( (u == V4) && (data.size() > 255) ) {
isc_throw(OutOfRange, "DHCPv4 Option " << type_ << " is too big."
<< "At most 255 bytes are supported.");
/// TODO Larger options can be stored as separate instances
/// of DHCPv4 options. Clients MUST concatenate them.
/// Fortunately, there are no such large options used today.
}
+ if ( (u == V4) && (type > 255) ) {
+ isc_throw(OutOfRange, "DHCPv4 Option type " << type_ << " is too big."
+ << "For DHCPv4 allowed type range is 0..255");
+ }
+}
+
+Option::Option(Universe u, uint16_t type, vector<uint8_t>::const_iterator first,
+ vector<uint8_t>::const_iterator last)
+ :universe_(u), type_(type) {
+ if ( (u == V4) && (type > 255) ) {
+ isc_throw(OutOfRange, "DHCPv4 Option type " << type_ << " is too big."
+ << "For DHCPv4 allowed type range is 0..255");
+ }
+ data_ = std::vector<uint8_t>(first, last);
+ if ( (u == V4) && (data_.size() > 255) ) {
+ isc_throw(OutOfRange, "DHCPv4 Option " << type_ << " is too big.");
+ }
}
@@ -82,7 +99,7 @@ void
Option::pack4(isc::util::OutputBuffer& buf) {
switch (universe_) {
case V4: {
- if (data_.size()>255) {
+ if (data_.size() > 255) {
isc_throw(OutOfRange, "DHCPv4 Option " << type_ << " is too big."
<< "At most 255 bytes are supported.");
/// TODO Larger options can be stored as separate instances
@@ -114,7 +131,7 @@ unsigned int
Option::pack4(boost::shared_array<uint8_t>& buf,
unsigned int buf_len,
unsigned int offset) {
- if ( offset+len() > buf_len ) {
+ if (offset + len() > buf_len) {
isc_throw(OutOfRange, "Failed to pack v4 option=" <<
type_ << ",len=" << len() << ": too small buffer.");
}
@@ -131,7 +148,7 @@ unsigned int
Option::pack6(boost::shared_array<uint8_t>& buf,
unsigned int buf_len,
unsigned int offset) {
- if ( offset+len() > buf_len ) {
+ if (offset+len() > buf_len) {
isc_throw(OutOfRange, "Failed to pack v6 option=" <<
type_ << ",len=" << len() << ": too small buffer.");
}
@@ -142,7 +159,7 @@ Option::pack6(boost::shared_array<uint8_t>& buf,
ptr = writeUint16(len() - getHeaderLen(), ptr);
- if (data_.size())
+ if (! data_.empty())
memcpy(ptr, &data_[0], data_.size());
// end of fixed part of this option
@@ -191,7 +208,7 @@ Option::unpack6(const boost::shared_array<uint8_t>& buf,
}
uint8_t* ptr = &buf[offset];
- data_ = std::vector<uint8_t>(ptr, ptr+parse_len);
+ data_ = std::vector<uint8_t>(ptr, ptr + parse_len);
offset_ = offset;
@@ -256,12 +273,12 @@ Option::delOption(unsigned short opt_type) {
std::string Option::toText(int indent /* =0 */ ) {
std::stringstream tmp;
- for (int i=0; i<indent; i++)
+ for (int i = 0; i < indent; i++)
tmp << " ";
tmp << "type=" << type_ << ", len=" << len()-getHeaderLen() << ": ";
- for (unsigned int i=0; i<data_.size(); i++) {
+ for (unsigned int i = 0; i < data_.size(); i++) {
if (i) {
tmp << ":";
}
@@ -270,8 +287,8 @@ std::string Option::toText(int indent /* =0 */ ) {
}
// print suboptions
- for (OptionCollection::const_iterator opt=options_.begin();
- opt!=options_.end();
+ for (OptionCollection::const_iterator opt = options_.begin();
+ opt != options_.end();
++opt) {
tmp << (*opt).second->toText(indent+2);
}
diff --git a/src/lib/dhcp/option.h b/src/lib/dhcp/option.h
index cd64991..ac4544b 100644
--- a/src/lib/dhcp/option.h
+++ b/src/lib/dhcp/option.h
@@ -90,6 +90,31 @@ public:
/// @param data content of the option
Option(Universe u, unsigned short type, std::vector<uint8_t>& data);
+
+ /// @brief Constructor, used for received options.
+ ///
+ /// This contructor is similar to the previous one, but it does not take
+ /// the whole vector<uint8_t>, but rather subset of it.
+ ///
+ /// TODO: This can be templated to use different containers, not just
+ /// vector. Prototype should look like this:
+ /// template<typename InputIterator> Option(Universe u, uint16_t type,
+ /// InputIterator first, InputIterator last);
+ ///
+ /// vector<int8_t> myData;
+ /// Example usage: new Option(V4, 123, myData.begin()+1, myData.end()-1)
+ /// This will create DHCPv4 option of type 123 that contains data from
+ /// trimmed (first and last byte removed) myData vector.
+ ///
+ /// @param u specifies universe (V4 or V6)
+ /// @param type option type (0-255 for V4 and 0-65535 for V6)
+ /// @param first iterator to the first element that should be copied
+ /// @param last iterator to the next element after the last one
+ /// to be copied.
+ Option(Universe u, uint16_t type,
+ std::vector<uint8_t>::const_iterator first,
+ std::vector<uint8_t>::const_iterator last);
+
/// @brief returns option universe (V4 or V6)
///
/// @return universe type
diff --git a/src/lib/dhcp/tests/option_unittest.cc b/src/lib/dhcp/tests/option_unittest.cc
index e9de7de..1e28c7c 100644
--- a/src/lib/dhcp/tests/option_unittest.cc
+++ b/src/lib/dhcp/tests/option_unittest.cc
@@ -69,7 +69,7 @@ TEST_F(OptionTest, v4_basic) {
const uint8_t dummyPayload[] =
{ 1, 2, 3, 4};
-TEST_F(OptionTest, v4_data) {
+TEST_F(OptionTest, v4_data1) {
vector<uint8_t> data(dummyPayload, dummyPayload + sizeof(dummyPayload));
@@ -114,6 +114,66 @@ TEST_F(OptionTest, v4_data) {
);
}
+// this is almost the same test as v4_data1, but it uses
+// different constructor
+TEST_F(OptionTest, v4_data2) {
+
+ vector<uint8_t> data(dummyPayload, dummyPayload + sizeof(dummyPayload));
+
+ vector<uint8_t> expData = data;
+
+ // Add fake data in front and end. Main purpose of this test is to check
+ // that only subset of the whole vector can be used for creating option.
+ data.insert(data.begin(), 56);
+ data.push_back(67);
+
+ // Data contains extra garbage at beginning and at the end. It should be
+ // ignored, as we pass interators to proper data. Only subset (limited by
+ // iterators) of the vector should be used.
+ // expData contains expected content (just valid data, without garbage).
+
+ Option* opt = 0;
+
+ // Create DHCPv4 option of type 123 that contains
+ // 4 bytes (sizeof(dummyPayload).
+ ASSERT_NO_THROW(
+ opt= new Option(Option::V4,
+ 123, // type
+ data.begin() + 1,
+ data.end() - 1);
+ );
+
+ // check that content is reported properly
+ EXPECT_EQ(123, opt->getType());
+ vector<uint8_t> optData = opt->getData();
+ ASSERT_EQ(optData.size(), expData.size());
+ EXPECT_EQ(optData, expData);
+ EXPECT_EQ(2, opt->getHeaderLen());
+ EXPECT_EQ(6, opt->len());
+
+ // now store that option into a buffer
+ OutputBuffer buf(100);
+ EXPECT_NO_THROW(
+ opt->pack4(buf);
+ );
+
+ // check content of that buffer
+
+ // 2 byte header + 4 bytes data
+ ASSERT_EQ(6, buf.getLength());
+
+ // that's how this option is supposed to look like
+ uint8_t exp[] = { 123, 4, 1, 2, 3, 4 };
+
+ /// TODO: use vector<uint8_t> getData() when it will be implemented
+ EXPECT_EQ(0, memcmp(exp, buf.getData(), 6));
+
+ // check that we can destroy that option
+ EXPECT_NO_THROW(
+ delete opt;
+ );
+}
+
TEST_F(OptionTest, v4_toText) {
vector<uint8_t> buf(3);
diff --git a/src/lib/dhcp/tests/pkt4_unittest.cc b/src/lib/dhcp/tests/pkt4_unittest.cc
index 096756f..322e867 100644
--- a/src/lib/dhcp/tests/pkt4_unittest.cc
+++ b/src/lib/dhcp/tests/pkt4_unittest.cc
@@ -283,6 +283,9 @@ TEST(Pkt4Test, hwAddr) {
vector<uint8_t> mac;
uint8_t expectedChaddr[Pkt4::MAX_CHADDR_LEN];
+ // We resize vector to specified length. It is more natural for fixed-length
+ // field, than clear it (shrink size to 0) and push_back each element
+ // (growing length back to MAX_CHADDR_LEN).
mac.resize(Pkt4::MAX_CHADDR_LEN);
Pkt4* pkt = 0;
@@ -447,10 +450,9 @@ TEST(Pkt4Test, options) {
vector<uint8_t> payload[5];
for (int i = 0; i < 5; i++) {
- payload[i].resize(3);
- payload[i][0] = i*10;
- payload[i][1] = i*10+1;
- payload[i][2] = i*10+2;
+ payload[i].push_back(i*10);
+ payload[i].push_back(i*10+1);
+ payload[i].push_back(i*10+2);
}
boost::shared_ptr<Option> opt1(new Option(Option::V4, 12, payload[0]));
@@ -483,7 +485,7 @@ TEST(Pkt4Test, options) {
pkt->pack();
);
- OutputBuffer& buf = pkt->getBuffer();
+ const OutputBuffer& buf = pkt->getBuffer();
// check that all options are stored, they should take sizeof(v4Opts)
ASSERT_EQ(static_cast<size_t>(Pkt4::DHCPV4_PKT_HDR_LEN) + sizeof(v4Opts),
buf.getLength());
diff --git a/src/lib/util/buffer.h b/src/lib/util/buffer.h
index 9e328a5..eb90d64 100644
--- a/src/lib/util/buffer.h
+++ b/src/lib/util/buffer.h
@@ -221,12 +221,8 @@ public:
isc_throw(InvalidBufferPosition, "read beyond end of buffer");
}
- if (data.size() < len)
- data.resize(len);
-
- const uint8_t* ptr = &data_[position_];
- data = std::vector<uint8_t>(ptr, ptr + len);
- position_ += len;
+ data.resize(len);
+ readData(&data[0], len);
}
private:
More information about the bind10-changes
mailing list