BIND 10 trac2417, updated. 5017e9a8d6993c8604722b2385c450b0b54b5cf8 [2417] Added changes suggested in the code review.
BIND 10 source code commits
bind10-changes at lists.isc.org
Tue Nov 6 12:19:24 UTC 2012
The branch, trac2417 has been updated
via 5017e9a8d6993c8604722b2385c450b0b54b5cf8 (commit)
from 2c70372efd0b0ddb8059d5bbe9eb195f024d4df8 (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 5017e9a8d6993c8604722b2385c450b0b54b5cf8
Author: Marcin Siodelski <marcin at isc.org>
Date: Tue Nov 6 13:12:32 2012 +0100
[2417] Added changes suggested in the code review.
-----------------------------------------------------------------------
Summary of changes:
src/bin/dhcp6/config_parser.cc | 25 +++++++++++++++----
src/bin/dhcp6/dhcp6_messages.mes | 12 +++++++---
src/bin/dhcp6/dhcp6_srv.cc | 4 ++--
src/bin/dhcp6/tests/dhcp6_srv_unittest.cc | 28 +++++++++++-----------
src/lib/dhcp/libdhcp++.cc | 5 +++-
src/lib/dhcp/option_definition.h | 9 +++++--
src/lib/dhcp/tests/libdhcp++_unittest.cc | 23 ++++++++++++++++++
src/lib/dhcp/tests/option_definition_unittest.cc | 13 ++++++++++
8 files changed, 92 insertions(+), 27 deletions(-)
-----------------------------------------------------------------------
diff --git a/src/bin/dhcp6/config_parser.cc b/src/bin/dhcp6/config_parser.cc
index de60697..e330e19 100644
--- a/src/bin/dhcp6/config_parser.cc
+++ b/src/bin/dhcp6/config_parser.cc
@@ -617,7 +617,11 @@ private:
<< " spaces");
}
+ // Get option data from the configuration database ('data' field).
+ // Option data is specified by the user as case insensitive string
+ // of hexadecimal digits for each option.
std::string option_data = getStringParam("data");
+ // Transform string of hexadecimal digits into binary format.
std::vector<uint8_t> binary;
try {
util::encode::decodeHex(option_data, binary);
@@ -625,29 +629,40 @@ private:
isc_throw(Dhcp6ConfigError, "Parser error: option data is not a valid"
<< " string of hexadecimal digits: " << option_data);
}
-
+ // Get all existing DHCPv6 option definitions. The one that matches
+ // our option will be picked and used to create it.
OptionDefContainer option_defs = LibDHCP::getOptionDefs(Option::V6);
+ // Get search index #1. It allows searching for options definitions
+ // using option type value.
const OptionDefContainerTypeIndex& idx = option_defs.get<1>();
-
+ // Get all option definitions matching option code we want to create.
const OptionDefContainerTypeRange& range = idx.equal_range(option_code);
size_t num_defs = std::distance(range.first, range.second);
OptionPtr option;
+ // Currently we do not allow duplicated definitions and if there are
+ // any duplicates we issue internal server error.
if (num_defs > 1) {
isc_throw(Dhcp6ConfigError, "Internal error: currently it is not"
<< " supported to initialize multiple option definitions"
<< " for the same option code. This will be supported once"
<< " there option spaces are implemented.");
} else if (num_defs == 0) {
- // Create the actual option.
+ // @todo We have a limited set of option definitions intiialized at the moment.
+ // In the future we want to initialize option definitions for all options.
+ // Consequently error will be issued if option definition does not exist
+ // for a particular option code. For now it is ok to create generic option
+ // if definition does not exist.
OptionPtr option(new Option(Option::V6, static_cast<uint16_t>(option_code),
binary));
// If option is created succesfully, add it to the storage.
options_->push_back(option);
} else {
+ // We have exactly one option definition for the particular option code.
+ // use it to create option instance.
const OptionDefinitionPtr& def = *(range.first);
- // getFactory should never return NULL pointer so we skip
- // sanity check here.
+ // getFactory should never return NULL pointer.
Option::Factory* factory = def->getFactory();
+ assert(factory != NULL);
try {
OptionPtr option = factory(Option::V6, option_code, binary);
options_->push_back(option);
diff --git a/src/bin/dhcp6/dhcp6_messages.mes b/src/bin/dhcp6/dhcp6_messages.mes
index 071a3c7..aee34b5 100644
--- a/src/bin/dhcp6/dhcp6_messages.mes
+++ b/src/bin/dhcp6/dhcp6_messages.mes
@@ -110,9 +110,15 @@ This is a debug message issued during the IPv6 DHCP server startup.
It lists some information about the parameters with which the server
is running.
-% DHCP6_NO_SUBNET_FOR_ADDRESS fail to find subnet for address: %1
-This warning message indicates that server does not support subnet
-that received DHCPv6 packet comes from.
+% DHCP6_NO_SUBNET_DEF_OPT failed to find subnet for address %1 when adding default options
+This warning message indicates that when attempting to add default options to a response,
+the server found that it was not configured to support the subnet from which the DHCPv6
+request was received. The packet has been ignored.
+
+% DHCP6_NO_SUBNET_REQ_OPT failed to find subnet for address %1 when adding requested options
+This warning message indicates that when attempting to add requested options to a response,
+the server found that it was not configured to support the subnet from which the DHCPv6
+request was received. The packet has been ignored.
% DHCP6_CONFIG_LOAD_FAIL failed to load configuration: %1
This critical error message indicates that the initial DHCPv6
diff --git a/src/bin/dhcp6/dhcp6_srv.cc b/src/bin/dhcp6/dhcp6_srv.cc
index 6a3174b..946bccb 100644
--- a/src/bin/dhcp6/dhcp6_srv.cc
+++ b/src/bin/dhcp6/dhcp6_srv.cc
@@ -303,7 +303,7 @@ void Dhcpv6Srv::appendDefaultOptions(const Pkt6Ptr& question, Pkt6Ptr& answer) {
Subnet6Ptr subnet = CfgMgr::instance().getSubnet6(question->getRemoteAddr());
// Warn if subnet is not supported and quit.
if (!subnet) {
- LOG_WARN(dhcp6_logger, DHCP6_NO_SUBNET_FOR_ADDRESS)
+ LOG_WARN(dhcp6_logger, DHCP6_NO_SUBNET_DEF_OPT)
.arg(question->getRemoteAddr().toText());
return;
}
@@ -325,7 +325,7 @@ void Dhcpv6Srv::appendRequestedOptions(const Pkt6Ptr& question, Pkt6Ptr& answer)
// Get the subnet for a particular address.
Subnet6Ptr subnet = CfgMgr::instance().getSubnet6(question->getRemoteAddr());
if (!subnet) {
- LOG_WARN(dhcp6_logger, DHCP6_NO_SUBNET_FOR_ADDRESS)
+ LOG_WARN(dhcp6_logger, DHCP6_NO_SUBNET_REQ_OPT)
.arg(question->getRemoteAddr().toText());
return;
}
diff --git a/src/bin/dhcp6/tests/dhcp6_srv_unittest.cc b/src/bin/dhcp6/tests/dhcp6_srv_unittest.cc
index 3477a3a..ef53bb4 100644
--- a/src/bin/dhcp6/tests/dhcp6_srv_unittest.cc
+++ b/src/bin/dhcp6/tests/dhcp6_srv_unittest.cc
@@ -76,7 +76,7 @@ TEST_F(Dhcpv6SrvTest, basic) {
// fe80::1234 link-local address on eth0 interface. Obviously
// an attempt to bind this socket will fail.
Dhcpv6Srv* srv = NULL;
- ASSERT_NO_THROW( {
+ ASSERT_NO_THROW({
// open an unpriviledged port
srv = new Dhcpv6Srv(DHCP6_SERVER_PORT + 10000);
});
@@ -88,7 +88,7 @@ TEST_F(Dhcpv6SrvTest, DUID) {
// tests that DUID is generated properly
boost::scoped_ptr<Dhcpv6Srv> srv;
- ASSERT_NO_THROW( {
+ ASSERT_NO_THROW({
srv.reset(new Dhcpv6Srv(DHCP6_SERVER_PORT + 10000));
});
@@ -186,7 +186,7 @@ TEST_F(Dhcpv6SrvTest, solicitBasic) {
ElementPtr json = Element::fromJSON(config);
boost::scoped_ptr<NakedDhcpv6Srv> srv;
- ASSERT_NO_THROW( srv.reset(new NakedDhcpv6Srv()) );
+ ASSERT_NO_THROW(srv.reset(new NakedDhcpv6Srv()));
EXPECT_NO_THROW(x = configureDhcp6Server(*srv, json));
ASSERT_TRUE(x);
@@ -233,8 +233,8 @@ TEST_F(Dhcpv6SrvTest, solicitBasic) {
// check if we get response at all
ASSERT_TRUE(reply);
- EXPECT_EQ( DHCPV6_ADVERTISE, reply->getType() );
- EXPECT_EQ( 1234, reply->getTransid() );
+ EXPECT_EQ(DHCPV6_ADVERTISE, reply->getType());
+ EXPECT_EQ(1234, reply->getTransid());
// We have not requested option with code 1000 so it should not
// be included in the response.
@@ -258,28 +258,28 @@ TEST_F(Dhcpv6SrvTest, solicitBasic) {
EXPECT_EQ(DHCPV6_ADVERTISE, reply->getType());
OptionPtr tmp = reply->getOption(D6O_IA_NA);
- ASSERT_TRUE( tmp );
+ ASSERT_TRUE(tmp);
boost::shared_ptr<Option6IA> reply_ia =
boost::dynamic_pointer_cast<Option6IA>(tmp);
ASSERT_TRUE(reply_ia);
- EXPECT_EQ( 234, reply_ia->getIAID() );
+ EXPECT_EQ(234, reply_ia->getIAID());
// check that there's an address included
- EXPECT_TRUE( reply_ia->getOption(D6O_IAADDR));
+ EXPECT_TRUE(reply_ia->getOption(D6O_IAADDR));
// check that server included our own client-id
tmp = reply->getOption(D6O_CLIENTID);
- ASSERT_TRUE( tmp );
- EXPECT_EQ(clientid->getType(), tmp->getType() );
- ASSERT_EQ(clientid->len(), tmp->len() );
+ ASSERT_TRUE(tmp);
+ EXPECT_EQ(clientid->getType(), tmp->getType());
+ ASSERT_EQ(clientid->len(), tmp->len());
- EXPECT_TRUE( clientid->getData() == tmp->getData() );
+ EXPECT_TRUE(clientid->getData() == tmp->getData());
// check that server included its server-id
tmp = reply->getOption(D6O_SERVERID);
- EXPECT_EQ(tmp->getType(), srv->getServerID()->getType() );
- ASSERT_EQ(tmp->len(), srv->getServerID()->len() );
+ EXPECT_EQ(tmp->getType(), srv->getServerID()->getType());
+ ASSERT_EQ(tmp->len(), srv->getServerID()->len());
EXPECT_TRUE(tmp->getData() == srv->getServerID()->getData());
diff --git a/src/lib/dhcp/libdhcp++.cc b/src/lib/dhcp/libdhcp++.cc
index 7a7fde6..99a3b91 100644
--- a/src/lib/dhcp/libdhcp++.cc
+++ b/src/lib/dhcp/libdhcp++.cc
@@ -287,8 +287,11 @@ LibDHCP::initStdOptionDefs6() {
case D6O_STATUS_CODE:
definition->addRecordField(OptionDefinition::UINT16_TYPE);
definition->addRecordField(OptionDefinition::STRING_TYPE);
- default:
break;
+ default:
+ // The default case is intentionally left empty
+ // as it does not need any processing.
+ ;
}
try {
definition->validate();
diff --git a/src/lib/dhcp/option_definition.h b/src/lib/dhcp/option_definition.h
index 27b2bda..14a0bb1 100644
--- a/src/lib/dhcp/option_definition.h
+++ b/src/lib/dhcp/option_definition.h
@@ -413,10 +413,13 @@ private:
///
/// This container allows to search for DHCP option definition
/// using two indexes:
-/// - sequenced: used to access elements in the oreder they have
+/// - sequenced: used to access elements in the order they have
/// been added to the container
/// - option code: used to search defintions of options
/// with a specified option code (aka option type).
+/// Note that this container can hold multiple options with the
+/// same code. For this reason, the latter index can be used to
+/// obtain a <b>range of options for a particular option code.
///
/// @todo: need an index to search options using option space name
/// once option spaces are implemented.
@@ -433,7 +436,9 @@ typedef boost::multi_index_container<
// Use option type as the index key. The type is held
// in OptionDefinition object so we have to call
// OptionDefinition::getCode to retrieve this key
- // for each element.
+ // for each element. The option code is non-unique so
+ // multiple elements with the same option code can
+ // be returned by this index.
boost::multi_index::const_mem_fun<
OptionDefinition,
uint16_t,
diff --git a/src/lib/dhcp/tests/libdhcp++_unittest.cc b/src/lib/dhcp/tests/libdhcp++_unittest.cc
index ec7f055..0c9dc89 100644
--- a/src/lib/dhcp/tests/libdhcp++_unittest.cc
+++ b/src/lib/dhcp/tests/libdhcp++_unittest.cc
@@ -64,19 +64,42 @@ public:
static void testInitOptionDefs6(const uint16_t code,
const OptionBuffer& buf,
const std::type_info& expected_type) {
+ // Initialize stdandard options definitions. They are held
+ // in the static container throughout the program.
LibDHCP::initStdOptionDefs(Option::V6);
+ // Get all option definitions, we will use them to extract
+ // the definition for a particular option code.
OptionDefContainer options = LibDHCP::getOptionDefs(Option::V6);
+ // Get the container index #1. This one allows for searching
+ // option definitions using option code.
const OptionDefContainerTypeIndex& idx = options.get<1>();
+ // Get 'all' option definitions for a particular option code.
+ // For standard options we expect that the range returned
+ // will contain single option as their codes are unique.
OptionDefContainerTypeRange range = idx.equal_range(code);
ASSERT_EQ(1, std::distance(range.first, range.second));
+ // If we have single option definition returned, the
+ // first iterator holds it.
OptionDefinitionPtr def = *(range.first);
+ // It should not happen that option definition is NULL but
+ // let's make sure (test should take things like that into
+ // account).
ASSERT_TRUE(def);
+ // Check that option definition is valid.
ASSERT_NO_THROW(def->validate());
+ // Get the factory function for the particular option
+ // definition. We will use this factory function to
+ // create option instance.
Option::Factory* factory = NULL;
ASSERT_NO_THROW(factory = def->getFactory());
OptionPtr option;
+ // Create the option.
ASSERT_NO_THROW(option = factory(Option::V6, code, buf));
+ // Make sure it is not NULL.
ASSERT_TRUE(option);
+ // And the actual object type is the one that we expect.
+ // Note that for many options there are dedicated classes
+ // derived from Option class to represent them.
EXPECT_TRUE(typeid(*option) == expected_type);
}
};
diff --git a/src/lib/dhcp/tests/option_definition_unittest.cc b/src/lib/dhcp/tests/option_definition_unittest.cc
index 99e8a5e..7e92680 100644
--- a/src/lib/dhcp/tests/option_definition_unittest.cc
+++ b/src/lib/dhcp/tests/option_definition_unittest.cc
@@ -282,25 +282,38 @@ TEST_F(OptionDefinitionTest, factoryEmpty) {
}
TEST_F(OptionDefinitionTest, factoryBinary) {
+ // Binary option is the one that is represented by the generic
+ // Option class. In fact all options can be represented by this
+ // class but for some of them it is just natural. The SERVERID
+ // option consists of the option code, length and binary data so
+ // this one was picked for this test.
OptionDefinition opt_def("OPTION_SERVERID", D6O_SERVERID, "binary");
Option::Factory* factory(NULL);
EXPECT_NO_THROW(factory = opt_def.getFactory());
ASSERT_TRUE(factory != NULL);
+ // Prepare some dummy data (serverid): 0, 1, 2 etc.
OptionBuffer buf(14);
for (int i = 0; i < 14; ++i) {
buf[i] = i;
}
+ // Create option instance with the factory function.
+ // If the OptionDefinition code works properly than
+ // object of the type Option should be returned.
OptionPtr option_v6;
ASSERT_NO_THROW(
option_v6 = factory(Option::V6, D6O_SERVERID, buf);
);
// Expect base option type returned.
ASSERT_TRUE(typeid(*option_v6) == typeid(Option));
+ // Sanity check on universe, length and size. These are
+ // the basic parameters identifying any option.
EXPECT_EQ(Option::V6, option_v6->getUniverse());
EXPECT_EQ(4, option_v6->getHeaderLen());
ASSERT_EQ(buf.size(), option_v6->getData().size());
+ // Get the server id data from the option and compare
+ // against reference buffer. They are expected to match.
EXPECT_TRUE(std::equal(option_v6->getData().begin(),
option_v6->getData().end(),
buf.begin()));
More information about the bind10-changes
mailing list