BIND 10 trac2591, updated. e0ffe686d622e43dfbf3e286252044d1fd20a6ed [2591] Changes as a result of the code review.
BIND 10 source code commits
bind10-changes at lists.isc.org
Tue Jan 22 18:31:13 UTC 2013
The branch, trac2591 has been updated
via e0ffe686d622e43dfbf3e286252044d1fd20a6ed (commit)
from 933430e40bb35135d741df5b9c8b944c5bf9be83 (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 e0ffe686d622e43dfbf3e286252044d1fd20a6ed
Author: Marcin Siodelski <marcin at isc.org>
Date: Tue Jan 22 18:40:55 2013 +0100
[2591] Changes as a result of the code review.
-----------------------------------------------------------------------
Summary of changes:
src/bin/dhcp4/dhcp4_srv.cc | 55 +++++++-
src/bin/dhcp4/dhcp4_srv.h | 13 ++
src/bin/dhcp4/tests/dhcp4_srv_unittest.cc | 39 +++---
src/lib/dhcp/option_int_array.h | 7 +
src/lib/dhcp/tests/option_int_array_unittest.cc | 169 ++++++++++-------------
src/lib/dhcpsrv/subnet.cc | 11 +-
6 files changed, 171 insertions(+), 123 deletions(-)
-----------------------------------------------------------------------
diff --git a/src/bin/dhcp4/dhcp4_srv.cc b/src/bin/dhcp4/dhcp4_srv.cc
index 72ae017..a866a5f 100644
--- a/src/bin/dhcp4/dhcp4_srv.cc
+++ b/src/bin/dhcp4/dhcp4_srv.cc
@@ -39,9 +39,6 @@ using namespace std;
// These are hardcoded parameters. Currently this is a skeleton server that only
// grants those options and a single, fixed, hardcoded lease.
-const std::string HARDCODED_GATEWAY = "192.0.2.1";
-const std::string HARDCODED_DNS_SERVER = "192.0.2.2";
-const std::string HARDCODED_DOMAIN_NAME = "isc.example.com";
const std::string HARDCODED_SERVER_ID = "192.0.2.1";
Dhcpv4Srv::Dhcpv4Srv(uint16_t port, const char* dbconfig) {
@@ -271,6 +268,40 @@ void Dhcpv4Srv::appendRequestedOptions(const Pkt4Ptr& question, Pkt4Ptr& msg) {
}
}
+void
+Dhcpv4Srv::appendBasicOptions(const Pkt4Ptr& question, Pkt4Ptr& msg) {
+ // Identify options that we always want to send to the
+ // client (if they are configured).
+ static const uint16_t required_options[] = {
+ DHO_SUBNET_MASK,
+ DHO_ROUTERS,
+ DHO_DOMAIN_NAME_SERVERS,
+ DHO_DOMAIN_NAME };
+
+ static size_t required_options_size =
+ sizeof(required_options) / sizeof(required_options[0]);
+
+ // Get the subnet.
+ Subnet4Ptr subnet = selectSubnet(question);
+ if (!subnet) {
+ return;
+ }
+
+ // Try to find all 'required' options in the outgoing
+ // message. Those that are not present will be added.
+ for (int i = 0; i < required_options_size; ++i) {
+ OptionPtr opt = msg->getOption(required_options[i]);
+ if (!opt) {
+ // Check whether option has been configured.
+ Subnet::OptionDescriptor desc =
+ subnet->getOptionDescriptor("dhcp4", required_options[i]);
+ if (desc.option) {
+ msg->addOption(desc.option);
+ }
+ }
+ }
+}
+
void Dhcpv4Srv::assignLease(const Pkt4Ptr& question, Pkt4Ptr& answer) {
// We need to select a subnet the client is connected in.
@@ -340,10 +371,12 @@ void Dhcpv4Srv::assignLease(const Pkt4Ptr& question, Pkt4Ptr& answer) {
opt->setUint32(lease->valid_lft_);
answer->addOption(opt);
- // @todo: include real router information here
// Router (type 3)
- opt = OptionPtr(new Option4AddrLst(DHO_ROUTERS, IOAddress(HARDCODED_GATEWAY)));
- answer->addOption(opt);
+ Subnet::OptionDescriptor opt_routers =
+ subnet->getOptionDescriptor("dhcp4", DHO_ROUTERS);
+ if (opt_routers.option) {
+ answer->addOption(opt_routers.option);
+ }
// Subnet mask (type 1)
answer->addOption(getNetmaskOption(subnet));
@@ -386,6 +419,11 @@ Pkt4Ptr Dhcpv4Srv::processDiscover(Pkt4Ptr& discover) {
assignLease(discover, offer);
+ // There are a few basic options that we always want to
+ // include in the response. If client did not request
+ // them we append them for him.
+ appendBasicOptions(discover, offer);
+
return (offer);
}
@@ -399,6 +437,11 @@ Pkt4Ptr Dhcpv4Srv::processRequest(Pkt4Ptr& request) {
assignLease(request, ack);
+ // There are a few basic options that we always want to
+ // include in the response. If client did not request
+ // them we append them for him.
+ appendBasicOptions(request, ack);
+
return (ack);
}
diff --git a/src/bin/dhcp4/dhcp4_srv.h b/src/bin/dhcp4/dhcp4_srv.h
index ee5ec46..a138920 100644
--- a/src/bin/dhcp4/dhcp4_srv.h
+++ b/src/bin/dhcp4/dhcp4_srv.h
@@ -187,6 +187,19 @@ protected:
/// @param answer OFFER or ACK/NAK message (lease options will be added here)
void assignLease(const Pkt4Ptr& question, Pkt4Ptr& answer);
+ /// @brief Append basic options if they are not present.
+ ///
+ /// This function adds the following basic options if they
+ /// are not yet added to the message:
+ /// - Subnet Mask,
+ /// - Router,
+ /// - Name Server,
+ /// - Domain Name.
+ ///
+ /// @param question DISCOVER or REQUEST message from a client.
+ /// @param msg the message to add options to.
+ void appendBasicOptions(const Pkt4Ptr& question, Pkt4Ptr& msg);
+
/// @brief Attempts to renew received addresses
///
/// Attempts to renew existing lease. This typically includes finding a lease that
diff --git a/src/bin/dhcp4/tests/dhcp4_srv_unittest.cc b/src/bin/dhcp4/tests/dhcp4_srv_unittest.cc
index 2fe4c33..3772306 100644
--- a/src/bin/dhcp4/tests/dhcp4_srv_unittest.cc
+++ b/src/bin/dhcp4/tests/dhcp4_srv_unittest.cc
@@ -70,12 +70,16 @@ public:
CfgMgr::instance().deleteSubnets4();
CfgMgr::instance().addSubnet4(subnet_);
+
+ // Add Router option.
+ Option4AddrLstPtr opt_routers(new Option4AddrLst(DHO_ROUTERS));
+ opt_routers->setAddress(IOAddress("192.0.2.2"));
+ subnet_->addOption(opt_routers, false, "dhcp4");
}
- /// @brief Add 'Paramter Request List' option to the packet.
+ /// @brief Add 'Parameter Request List' option to the packet.
///
- /// This function PRL option comprising the following
- /// option codes:
+ /// This function PRL option comprising the following option codes:
/// - 5 - Name Server
/// - 15 - Domain Name
/// - 7 - Log Server
@@ -91,15 +95,13 @@ public:
std::vector<uint8_t> opts;
// Let's request options that have been configured for the subnet.
- opts.push_back(DHO_DOMAIN_NAME_SERVERS);
- opts.push_back(DHO_DOMAIN_NAME);
- opts.push_back(DHO_LOG_SERVERS);
- opts.push_back(DHO_COOKIE_SERVERS);
+ option_prl->addValue(DHO_DOMAIN_NAME_SERVERS);
+ option_prl->addValue(DHO_DOMAIN_NAME);
+ option_prl->addValue(DHO_LOG_SERVERS);
+ option_prl->addValue(DHO_COOKIE_SERVERS);
// Let's also request the option that hasn't been configured. In such
// case server should ignore request for this particular option.
- opts.push_back(DHO_LPR_SERVERS);
- // Put the requested option codes into the 'Parameter Request List'.
- option_prl->setValues(opts);
+ option_prl->addValue(DHO_LPR_SERVERS);
// And add 'Parameter Request List' option into the DISCOVER packet.
pkt->addOption(option_prl);
}
@@ -160,7 +162,6 @@ public:
EXPECT_TRUE(a->getOption(DHO_DHCP_SERVER_IDENTIFIER));
EXPECT_TRUE(a->getOption(DHO_DHCP_LEASE_TIME));
EXPECT_TRUE(a->getOption(DHO_SUBNET_MASK));
- EXPECT_TRUE(a->getOption(DHO_ROUTERS));
// Check that something is offered
EXPECT_TRUE(a->getYiaddr().toText() != "0.0.0.0");
@@ -424,10 +425,13 @@ TEST_F(Dhcpv4SrvTest, processDiscover) {
messageCheck(pkt, offer);
+ // There are some options that are always present in the
+ // message, even if not requested.
+ EXPECT_TRUE(offer->getOption(DHO_DOMAIN_NAME));
+ EXPECT_TRUE(offer->getOption(DHO_DOMAIN_NAME_SERVERS));
+
// We did not request any options so they should not be present
// in the OFFER.
- EXPECT_FALSE(offer->getOption(DHO_DOMAIN_NAME));
- EXPECT_FALSE(offer->getOption(DHO_DOMAIN_NAME_SERVERS));
EXPECT_FALSE(offer->getOption(DHO_LOG_SERVERS));
EXPECT_FALSE(offer->getOption(DHO_COOKIE_SERVERS));
EXPECT_FALSE(offer->getOption(DHO_LPR_SERVERS));
@@ -523,10 +527,13 @@ TEST_F(Dhcpv4SrvTest, processRequest) {
messageCheck(req, ack);
- // We did not request any options so they should not be present
+ // There are some options that are always present in the
+ // message, even if not requested.
+ EXPECT_TRUE(ack->getOption(DHO_DOMAIN_NAME));
+ EXPECT_TRUE(ack->getOption(DHO_DOMAIN_NAME_SERVERS));
+
+ // We did not request any options so these should not be present
// in the ACK.
- EXPECT_FALSE(ack->getOption(DHO_DOMAIN_NAME));
- EXPECT_FALSE(ack->getOption(DHO_DOMAIN_NAME_SERVERS));
EXPECT_FALSE(ack->getOption(DHO_LOG_SERVERS));
EXPECT_FALSE(ack->getOption(DHO_COOKIE_SERVERS));
EXPECT_FALSE(ack->getOption(DHO_LPR_SERVERS));
diff --git a/src/lib/dhcp/option_int_array.h b/src/lib/dhcp/option_int_array.h
index d40c472..1babee5 100644
--- a/src/lib/dhcp/option_int_array.h
+++ b/src/lib/dhcp/option_int_array.h
@@ -124,6 +124,13 @@ public:
unpack(begin, end);
}
+ /// @brief Adds a new value to the array.
+ ///
+ /// @param value a value being added.
+ void addValue(const T value) {
+ values_.push_back(value);
+ }
+
/// Writes option in wire-format to buf, returns pointer to first unused
/// byte after stored option.
///
diff --git a/src/lib/dhcp/tests/option_int_array_unittest.cc b/src/lib/dhcp/tests/option_int_array_unittest.cc
index 1aeb584..cd868d8 100644
--- a/src/lib/dhcp/tests/option_int_array_unittest.cc
+++ b/src/lib/dhcp/tests/option_int_array_unittest.cc
@@ -294,6 +294,52 @@ public:
EXPECT_TRUE(std::equal(buf_.begin(), buf_.begin() + opt_len, out_data.begin()));;
}
+ /// @brief Test ability to set all values.
+ ///
+ /// @tparam T numeric type to perform the test for.
+ template<typename T>
+ void setValuesTest() {
+ const uint16_t opt_code = 100;
+ // Create option with empty vector of values.
+ boost::shared_ptr<OptionIntArray<T> >
+ opt(new OptionIntArray<T>(Option::V6, opt_code));
+ // Initialize vector with some data and pass to the option.
+ std::vector<T> values;
+ for (int i = 0; i < 10; ++i) {
+ values.push_back(numeric_limits<uint8_t>::max() - i);
+ }
+ opt->setValues(values);
+
+ // Check if universe, option type and data was set correctly.
+ EXPECT_EQ(Option::V6, opt->getUniverse());
+ EXPECT_EQ(opt_code, opt->getType());
+ std::vector<T> returned_values = opt->getValues();
+ EXPECT_TRUE(std::equal(values.begin(), values.end(), returned_values.begin()));
+ }
+
+ /// @brief Test ability to add values one by one.
+ ///
+ /// @tparam T numeric type to perform the test for.
+ template<typename T>
+ void addValuesTest() {
+ const uint16_t opt_code = 100;
+ // Create option with empty vector of values.
+ boost::shared_ptr<OptionIntArray<T> >
+ opt(new OptionIntArray<T>(Option::V6, opt_code));
+ // Initialize vector with some data and add the same data
+ // to the option.
+ std::vector<T> values;
+ for (int i = 0; i < 10; ++i) {
+ values.push_back(numeric_limits<T>::max() - i);
+ opt->addValue(numeric_limits<T>::max() - i);
+ }
+
+ // Check if universe, option type and data was set correctly.
+ EXPECT_EQ(Option::V6, opt->getUniverse());
+ EXPECT_EQ(opt_code, opt->getType());
+ std::vector<T> returned_values = opt->getValues();
+ EXPECT_TRUE(std::equal(values.begin(), values.end(), returned_values.begin()));
+ }
OptionBuffer buf_; ///< Option buffer
OutputBuffer out_buf_; ///< Output buffer
@@ -371,118 +417,51 @@ TEST_F(OptionIntArrayTest, bufferToInt32V6) {
}
TEST_F(OptionIntArrayTest, setValuesUint8) {
- const uint16_t opt_code = 100;
- // Create option with empty vector of values.
- boost::shared_ptr<OptionIntArray<uint8_t> >
- opt(new OptionIntArray<uint8_t>(Option::V6, opt_code));
- // Initialize vector with some data and pass to the option.
- std::vector<uint8_t> values;
- for (int i = 0; i < 10; ++i) {
- values.push_back(numeric_limits<uint8_t>::max() - i);
- }
- opt->setValues(values);
-
- // Check if universe, option type and data was set correctly.
- EXPECT_EQ(Option::V6, opt->getUniverse());
- EXPECT_EQ(opt_code, opt->getType());
- std::vector<uint8_t> returned_values = opt->getValues();
- EXPECT_TRUE(std::equal(values.begin(), values.end(), returned_values.begin()));
+ setValuesTest<uint8_t>();
}
TEST_F(OptionIntArrayTest, setValuesInt8) {
- const uint16_t opt_code = 100;
- // Create option with empty vector of values.
- boost::shared_ptr<OptionIntArray<int8_t> >
- opt(new OptionIntArray<int8_t>(Option::V6, opt_code));
- // Initialize vector with some data and pass to the option.
- std::vector<int8_t> values;
- for (int i = 0; i < 10; ++i) {
- values.push_back(numeric_limits<int8_t>::min() + i);
- }
- opt->setValues(values);
-
- // Check if universe, option type and data was set correctly.
- EXPECT_EQ(Option::V6, opt->getUniverse());
- EXPECT_EQ(opt_code, opt->getType());
- std::vector<int8_t> returned_values = opt->getValues();
- EXPECT_TRUE(std::equal(values.begin(), values.end(), returned_values.begin()));
+ setValuesTest<int8_t>();
}
TEST_F(OptionIntArrayTest, setValuesUint16) {
- const uint16_t opt_code = 101;
- // Create option with empty vector of values.
- boost::shared_ptr<OptionIntArray<uint16_t> >
- opt(new OptionIntArray<uint16_t>(Option::V6, opt_code));
- // Initialize vector with some data and pass to the option.
- std::vector<uint16_t> values;
- for (int i = 0; i < 10; ++i) {
- values.push_back(numeric_limits<uint16_t>::max() - i);
- }
- opt->setValues(values);
-
- // Check if universe, option type and data was set correctly.
- EXPECT_EQ(Option::V6, opt->getUniverse());
- EXPECT_EQ(opt_code, opt->getType());
- std::vector<uint16_t> returned_values = opt->getValues();
- EXPECT_TRUE(std::equal(values.begin(), values.end(), returned_values.begin()));
+ setValuesTest<uint16_t>();
}
TEST_F(OptionIntArrayTest, setValuesInt16) {
- const uint16_t opt_code = 101;
- // Create option with empty vector of values.
- boost::shared_ptr<OptionIntArray<int16_t> >
- opt(new OptionIntArray<int16_t>(Option::V6, opt_code));
- // Initialize vector with some data and pass to the option.
- std::vector<int16_t> values;
- for (int i = 0; i < 10; ++i) {
- values.push_back(numeric_limits<int16_t>::min() + i);
- }
- opt->setValues(values);
-
- // Check if universe, option type and data was set correctly.
- EXPECT_EQ(Option::V6, opt->getUniverse());
- EXPECT_EQ(opt_code, opt->getType());
- std::vector<int16_t> returned_values = opt->getValues();
- EXPECT_TRUE(std::equal(values.begin(), values.end(), returned_values.begin()));
+ setValuesTest<int16_t>();
}
TEST_F(OptionIntArrayTest, setValuesUint32) {
- const uint32_t opt_code = 101;
- // Create option with empty vector of values.
- boost::shared_ptr<OptionIntArray<uint32_t> >
- opt(new OptionIntArray<uint32_t>(Option::V6, opt_code));
- // Initialize vector with some data and pass to the option.
- std::vector<uint32_t> values;
- for (int i = 0; i < 10; ++i) {
- values.push_back(numeric_limits<uint32_t>::max() - i);
- }
- opt->setValues(values);
-
- // Check if universe, option type and data was set correctly.
- EXPECT_EQ(Option::V6, opt->getUniverse());
- EXPECT_EQ(opt_code, opt->getType());
- std::vector<uint32_t> returned_values = opt->getValues();
- EXPECT_TRUE(std::equal(values.begin(), values.end(), returned_values.begin()));
+ setValuesTest<uint16_t>();
}
TEST_F(OptionIntArrayTest, setValuesInt32) {
- const uint32_t opt_code = 101;
- // Create option with empty vector of values.
- boost::shared_ptr<OptionIntArray<int32_t> >
- opt(new OptionIntArray<int32_t>(Option::V6, opt_code));
- // Initialize vector with some data and pass to the option.
- std::vector<int32_t> values;
- for (int i = 0; i < 10; ++i) {
- values.push_back(numeric_limits<int32_t>::min() + i);
- }
- opt->setValues(values);
+ setValuesTest<int16_t>();
+}
- // Check if universe, option type and data was set correctly.
- EXPECT_EQ(Option::V6, opt->getUniverse());
- EXPECT_EQ(opt_code, opt->getType());
- std::vector<int32_t> returned_values = opt->getValues();
- EXPECT_TRUE(std::equal(values.begin(), values.end(), returned_values.begin()));
+TEST_F(OptionIntArrayTest, addValuesUint8) {
+ addValuesTest<uint8_t>();
}
+TEST_F(OptionIntArrayTest, addValuesInt8) {
+ addValuesTest<int8_t>();
+}
+
+TEST_F(OptionIntArrayTest, addValuesUint16) {
+ addValuesTest<uint16_t>();
+}
+
+TEST_F(OptionIntArrayTest, addValuesInt16) {
+ addValuesTest<int16_t>();
+}
+
+TEST_F(OptionIntArrayTest, addValuesUint32) {
+ addValuesTest<uint16_t>();
+}
+
+TEST_F(OptionIntArrayTest, addValuesInt32) {
+ addValuesTest<int16_t>();
+}
} // anonymous namespace
diff --git a/src/lib/dhcpsrv/subnet.cc b/src/lib/dhcpsrv/subnet.cc
index 436fd49..c33efda 100644
--- a/src/lib/dhcpsrv/subnet.cc
+++ b/src/lib/dhcpsrv/subnet.cc
@@ -14,6 +14,7 @@
#include <asiolink/io_address.h>
#include <dhcpsrv/addr_utilities.h>
+#include <dhcpsrv/option_space.h>
#include <dhcpsrv/subnet.h>
#include <sstream>
@@ -46,12 +47,10 @@ bool Subnet::inRange(const isc::asiolink::IOAddress& addr) const {
void
Subnet::addOption(const OptionPtr& option, bool persistent,
const std::string& option_space) {
- // @todo Once the #2313 is merged we need to use the OptionSpace object to
- // validate the option space name here. For now, let's check that the name
- // is not empty as the empty namespace has a special meaning here - it is
- // returned when desired namespace is not found when getOptions is called.
- if (option_space.empty()) {
- isc_throw(isc::BadValue, "option space name must not be empty");
+ // Check that the option space name is valid.
+ if (!OptionSpace::validateName(option_space)) {
+ isc_throw(isc::BadValue, "invalid option space name: '"
+ << option_space << "'");
}
validateOption(option);
More information about the bind10-changes
mailing list