BIND 10 trac2315, updated. 2548205bf705e3f2d9aeb75e35eb112206476d03 [2315] Changes as the result of the review.
BIND 10 source code commits
bind10-changes at lists.isc.org
Mon Jan 7 18:51:23 UTC 2013
The branch, trac2315 has been updated
via 2548205bf705e3f2d9aeb75e35eb112206476d03 (commit)
from bf592d0607a639ff454dcdc820a76ae0eaacbbf4 (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 2548205bf705e3f2d9aeb75e35eb112206476d03
Author: Marcin Siodelski <marcin at isc.org>
Date: Mon Jan 7 19:51:06 2013 +0100
[2315] Changes as the result of the review.
-----------------------------------------------------------------------
Summary of changes:
src/lib/dhcp/libdhcp++.cc | 28 ++++++++++++++
src/lib/dhcp/libdhcp++.h | 15 ++++++++
src/lib/dhcp/option_definition.h | 3 ++
src/lib/dhcp/tests/libdhcp++_unittest.cc | 60 ++++++++++++++++++++++++++++++
src/lib/dhcpsrv/cfgmgr.cc | 26 ++++++++++---
src/lib/dhcpsrv/cfgmgr.h | 13 ++++++-
src/lib/dhcpsrv/subnet.cc | 4 +-
src/lib/dhcpsrv/subnet.h | 4 +-
src/lib/dhcpsrv/tests/cfgmgr_unittest.cc | 40 ++++++++++++++++++--
src/lib/dhcpsrv/tests/subnet_unittest.cc | 4 +-
10 files changed, 181 insertions(+), 16 deletions(-)
-----------------------------------------------------------------------
diff --git a/src/lib/dhcp/libdhcp++.cc b/src/lib/dhcp/libdhcp++.cc
index b19ed21..a3921cc 100644
--- a/src/lib/dhcp/libdhcp++.cc
+++ b/src/lib/dhcp/libdhcp++.cc
@@ -74,6 +74,34 @@ LibDHCP::getOptionDef(const Option::Universe u, const uint16_t code) {
return (OptionDefinitionPtr());
}
+bool
+LibDHCP::isStandardOption(const Option::Universe u, const uint16_t code) {
+ if (u == Option::V6) {
+ if (code < 79 &&
+ code != 10 &&
+ code != 35) {
+ return (true);
+ }
+
+ } else if (u == Option::V4) {
+ if (!(code == 84 ||
+ code == 96 ||
+ (code > 101 && code < 112) ||
+ code == 115 ||
+ code == 126 ||
+ code == 127 ||
+ (code > 146 && code < 150) ||
+ (code > 177 && code < 208) ||
+ (code > 213 && code < 220) ||
+ (code > 221 && code < 224))) {
+ return (true);
+ }
+
+ }
+
+ return (false);
+}
+
OptionPtr
LibDHCP::optionFactory(Option::Universe u,
uint16_t type,
diff --git a/src/lib/dhcp/libdhcp++.h b/src/lib/dhcp/libdhcp++.h
index c325aa5..bc47405 100644
--- a/src/lib/dhcp/libdhcp++.h
+++ b/src/lib/dhcp/libdhcp++.h
@@ -55,6 +55,21 @@ public:
static OptionDefinitionPtr getOptionDef(const Option::Universe u,
const uint16_t code);
+ /// @brief Check if the specified option is a standard option.
+ ///
+ /// @param u universe (V4 or V6)
+ /// @param code option code.
+ ///
+ /// @return true if the specified option is a standard option.
+ /// @todo We arleady create option definitions for the subset if
+ /// standard options. We are aiming that this function checks
+ /// the presence of the standard option definition and if it finds
+ /// it, then the true value is returned. However, at this point
+ /// this is not doable because some of the definitions (for less
+ /// important options) are not created yet.
+ static bool isStandardOption(const Option::Universe u,
+ const uint16_t code);
+
/// @brief Factory function to create instance of option.
///
/// Factory method creates instance of specified option. The option
diff --git a/src/lib/dhcp/option_definition.h b/src/lib/dhcp/option_definition.h
index 3a76e15..efcaba0 100644
--- a/src/lib/dhcp/option_definition.h
+++ b/src/lib/dhcp/option_definition.h
@@ -500,6 +500,9 @@ typedef boost::multi_index_container<
>
> OptionDefContainer;
+/// Pointer to an option definition container.
+typedef boost::shared_ptr<OptionDefContainer> OptionDefContainerPtr;
+
/// Type of the index #1 - option type.
typedef OptionDefContainer::nth_index<1>::type OptionDefContainerTypeIndex;
/// Pair of iterators to represent the range of options definitions
diff --git a/src/lib/dhcp/tests/libdhcp++_unittest.cc b/src/lib/dhcp/tests/libdhcp++_unittest.cc
index e44ef58..a59da12 100644
--- a/src/lib/dhcp/tests/libdhcp++_unittest.cc
+++ b/src/lib/dhcp/tests/libdhcp++_unittest.cc
@@ -453,6 +453,66 @@ TEST_F(LibDhcpTest, unpackOptions4) {
EXPECT_TRUE(x == options.end()); // option 2 not found
}
+TEST_F(LibDhcpTest, isStandardOption4) {
+ // Get all option codes that are not occupied by standard options.
+ const uint16_t unassigned_codes[] = { 84, 96, 102, 103, 104, 105, 106, 107, 108,
+ 109, 110, 111, 115, 126, 127, 147, 148, 149,
+ 178, 179, 180, 181, 182, 183, 184, 185, 186,
+ 187, 188, 189, 190, 191, 192, 193, 194, 195,
+ 196, 197, 198, 199, 200, 201, 202, 203, 204,
+ 205, 206, 207, 214, 215, 216, 217, 218, 219,
+ 222, 223 };
+ const size_t unassigned_num = sizeof(unassigned_codes) / sizeof(unassigned_codes[0]);
+
+ // Try all possible option codes.
+ for (size_t i = 0; i < 256; ++i) {
+ // Some ranges of option codes are unassigned and thus the isStandardOption
+ // should return false for them.
+ bool check_unassigned = false;
+ // Check the array of unassigned options to find out whether option code
+ // is assigned to standard option or unassigned.
+ for (size_t j = 0; j < unassigned_num; ++j) {
+ // If option code is found within the array of unassigned options
+ // we the isStandardOption function should return false.
+ if (unassigned_codes[j] == i) {
+ check_unassigned = true;
+ EXPECT_FALSE(LibDHCP::isStandardOption(Option::V4,
+ unassigned_codes[j]))
+ << "Test failed for option code " << unassigned_codes[j];
+ break;
+ }
+ }
+ // If the option code belongs to the standard option then the
+ // isStandardOption should return true.
+ if (!check_unassigned) {
+ EXPECT_TRUE(LibDHCP::isStandardOption(Option::V4, i))
+ << "Test failed for the option code " << i;
+ }
+ }
+}
+
+TEST_F(LibDhcpTest, isStandardOption6) {
+ // All option codes in the range from 0 to 78 (except 10 and 35)
+ // identify the standard options.
+ for (uint16_t code = 0; code < 79; ++code) {
+ if (code != 10 && code != 35) {
+ EXPECT_TRUE(LibDHCP::isStandardOption(Option::V6, code))
+ << "Test failed for option code " << code;
+ }
+ }
+
+ // Check the option codes 10 and 35. They are unassigned.
+ EXPECT_FALSE(LibDHCP::isStandardOption(Option::V6, 10));
+ EXPECT_FALSE(LibDHCP::isStandardOption(Option::V6, 35));
+
+ // Check a range of option codes above 78. Those are option codes
+ // identifying non-standard options.
+ for (uint16_t code = 79; code < 512; ++code) {
+ EXPECT_FALSE(LibDHCP::isStandardOption(Option::V6, code))
+ << "Test failed for option code " << code;
+ }
+}
+
TEST_F(LibDhcpTest, stdOptionDefs4) {
// Create a buffer that holds dummy option data.
diff --git a/src/lib/dhcpsrv/cfgmgr.cc b/src/lib/dhcpsrv/cfgmgr.cc
index 1effe7c..193421f 100644
--- a/src/lib/dhcpsrv/cfgmgr.cc
+++ b/src/lib/dhcpsrv/cfgmgr.cc
@@ -13,6 +13,7 @@
// PERFORMANCE OF THIS SOFTWARE.
#include <asiolink/io_address.h>
+#include <dhcp/libdhcp++.h>
#include <dhcpsrv/cfgmgr.h>
using namespace isc::asiolink;
@@ -34,23 +35,36 @@ CfgMgr::addOptionDef(const OptionDefinitionPtr& def,
// This will be implemented when #2313 is merged.
if (option_space.empty()) {
isc_throw(BadValue, "option space name must not be empty");
- } else if (option_space == "dhcp4" || option_space == "dhcp6") {
- isc_throw(BadValue, "unable to override definition of option"
- << " in standard option space '" << option_space
- << "'.");
} else if (!def) {
+ // Option definition must point to a valid object.
isc_throw(MalformedOptionDefinition, "option definition must not be NULL");
+
} else if (getOptionDef(option_space, def->getCode())) {
+ // Option definition must not be overriden.
isc_throw(DuplicateOptionDefinition, "option definition already added"
<< " to option space " << option_space);
+
+ } else if ((option_space == "dhcp4" &&
+ LibDHCP::isStandardOption(Option::V4, def->getCode())) ||
+ (option_space == "dhcp6" &&
+ LibDHCP::isStandardOption(Option::V6, def->getCode()))) {
+ // We must not override standard (assigned) option. The standard options
+ // belong to dhcp4 or dhcp6 option space.
+ isc_throw(BadValue, "unable to override definition of option '"
+ << def->getCode() << "' in standard option space '"
+ << option_space << "'.");
+
}
+ // Actually add the definition to the option space.
option_def_spaces_[option_space].push_back(def);
}
const OptionDefContainer&
CfgMgr::getOptionDefs(const std::string& option_space) const {
+ // @todo Validate the option space once the #2313 is implemented.
+
// Get all option definitions for the particular option space.
- const std::map<std::string, OptionDefContainer>::const_iterator& defs =
+ const OptionDefsMap::const_iterator& defs =
option_def_spaces_.find(option_space);
// If there are no option definitions for the particular option space
// then return empty container.
@@ -65,6 +79,8 @@ CfgMgr::getOptionDefs(const std::string& option_space) const {
OptionDefinitionPtr
CfgMgr::getOptionDef(const std::string& option_space,
const uint16_t option_code) const {
+ // @todo Validate the option space once the #2313 is implemented.
+
// Get a reference to option definitions for a particular option space.
const OptionDefContainer& defs = getOptionDefs(option_space);
// If there are no matching option definitions then return the empty pointer.
diff --git a/src/lib/dhcpsrv/cfgmgr.h b/src/lib/dhcpsrv/cfgmgr.h
index 5cf40e0..6e3dd42 100644
--- a/src/lib/dhcpsrv/cfgmgr.h
+++ b/src/lib/dhcpsrv/cfgmgr.h
@@ -88,7 +88,8 @@ public:
/// @throw isc::dhcp::MalformedOptionDefinition when the pointer to
/// an option definition is NULL.
/// @throw isc::BadValue when the option space name is empty or
- /// option space name is one of reserved names: dhcp4 or dhcp6.
+ /// when trying to override the standard option (in dhcp4 or dhcp6
+ /// option space).
void addOptionDef(const OptionDefinitionPtr& def,
const std::string& option_space);
@@ -186,6 +187,7 @@ public:
/// 192.0.2.0/23 and 192.0.2.0/24 the same subnet or is it something
/// completely new?
void deleteSubnets4();
+
protected:
/// @brief Protected constructor.
@@ -217,9 +219,16 @@ protected:
private:
+ /// A map containing option definitions for various option spaces.
+ /// They key of this map is the name of the option space. The
+ /// value is the the option container holding option definitions
+ /// for the particular option space.
+ typedef std::map<std::string, OptionDefContainer> OptionDefsMap;
+
/// A map containing option definitions for different option spaces.
/// The map key holds an option space name.
- std::map<std::string, OptionDefContainer> option_def_spaces_;
+ OptionDefsMap option_def_spaces_;
+
};
} // namespace isc::dhcp
diff --git a/src/lib/dhcpsrv/subnet.cc b/src/lib/dhcpsrv/subnet.cc
index 70e762e..e6f5302 100644
--- a/src/lib/dhcpsrv/subnet.cc
+++ b/src/lib/dhcpsrv/subnet.cc
@@ -82,8 +82,8 @@ Subnet::getOptions(const std::string& option_space) const {
}
Subnet::OptionDescriptor
-Subnet::getOptionSingle(const std::string& option_space,
- const uint16_t option_code) {
+Subnet::getOptionDescriptor(const std::string& option_space,
+ const uint16_t option_code) {
const OptionContainer& options = getOptions(option_space);
if (options.empty()) {
return (OptionDescriptor(false));
diff --git a/src/lib/dhcpsrv/subnet.h b/src/lib/dhcpsrv/subnet.h
index 4cdcf11..d4a8428 100644
--- a/src/lib/dhcpsrv/subnet.h
+++ b/src/lib/dhcpsrv/subnet.h
@@ -270,8 +270,8 @@ public:
///
/// @return option descriptor found for the specified option space
/// and option code.
- OptionDescriptor getOptionSingle(const std::string& option_space,
- const uint16_t option_code);
+ OptionDescriptor getOptionDescriptor(const std::string& option_space,
+ const uint16_t option_code);
/// @brief returns the last address that was tried from this pool
///
diff --git a/src/lib/dhcpsrv/tests/cfgmgr_unittest.cc b/src/lib/dhcpsrv/tests/cfgmgr_unittest.cc
index 1b723c9..2e30b75 100644
--- a/src/lib/dhcpsrv/tests/cfgmgr_unittest.cc
+++ b/src/lib/dhcpsrv/tests/cfgmgr_unittest.cc
@@ -129,7 +129,7 @@ TEST_F(CfgMgrTest, getOptionDef) {
// add them to the different option space.
for (uint16_t code = 105; code < 115; ++code) {
std::ostringstream option_name;
- option_name << "option-" << code;
+ option_name << "option-other-" << code;
OptionDefinitionPtr def(new OptionDefinition(option_name.str(), code,
"uint16"));
ASSERT_NO_THROW(cfg_mgr.addOptionDef(def, "abcde"));
@@ -140,6 +140,12 @@ TEST_F(CfgMgrTest, getOptionDef) {
for (uint16_t code = 100; code < 110; ++code) {
OptionDefinitionPtr def = cfg_mgr.getOptionDef("isc", code);
ASSERT_TRUE(def);
+ // Check that the option name is in the format of 'option-[code]'.
+ // That way we make sure that the options that have the same codes
+ // within different option spaces are different.
+ std::ostringstream option_name;
+ option_name << "option-" << code;
+ EXPECT_EQ(option_name.str(), def->getName());
EXPECT_EQ(code, def->getCode());
}
@@ -147,20 +153,44 @@ TEST_F(CfgMgrTest, getOptionDef) {
for (uint16_t code = 105; code < 115; ++code) {
OptionDefinitionPtr def = cfg_mgr.getOptionDef("abcde", code);
ASSERT_TRUE(def);
+ // Check that the option name is in the format of 'option-other-[code]'.
+ // That way we make sure that the options that have the same codes
+ // within different option spaces are different.
+ std::ostringstream option_name;
+ option_name << "option-other-" << code;
+ EXPECT_EQ(option_name.str(), def->getName());
+
EXPECT_EQ(code, def->getCode());
}
+ // Check that an option definition can be added to the standard
+ // (dhcp4 and dhcp6) option spaces when the option code is not
+ // reserved by the standard option.
+ OptionDefinitionPtr def6(new OptionDefinition("option-foo", 79, "uint16"));
+ EXPECT_NO_THROW(cfg_mgr.addOptionDef(def6, "dhcp6"));
+
+ OptionDefinitionPtr def4(new OptionDefinition("option-foo", 222, "uint16"));
+ EXPECT_NO_THROW(cfg_mgr.addOptionDef(def4, "dhcp4"));
+
// Try to query the option definition from an non-existing
// option space and expect NULL pointer.
OptionDefinitionPtr def = cfg_mgr.getOptionDef("non-existing", 56);
- ASSERT_FALSE(def);
+ EXPECT_FALSE(def);
+
+ // Try to get the non-existing option definition from an
+ // existing option space.
+ EXPECT_FALSE(cfg_mgr.getOptionDef("isc", 56));
+
}
// This test verifies that the function that adds new option definition
// throws exceptions when arguments are invalid.
TEST_F(CfgMgrTest, addOptionDefNegative) {
CfgMgr& cfg_mgr = CfgMgr::instance();
- OptionDefinitionPtr def(new OptionDefinition("option-foo", 100, "uint16"));
+ // The option code 65 is reserved for standard options either in
+ // DHCPv4 or DHCPv6. Thus we expect that adding an option to this
+ // option space fails.
+ OptionDefinitionPtr def(new OptionDefinition("option-foo", 65, "uint16"));
// Try reserved option space names.
ASSERT_THROW(cfg_mgr.addOptionDef(def, "dhcp4"), isc::BadValue);
@@ -170,6 +200,10 @@ TEST_F(CfgMgrTest, addOptionDefNegative) {
// Try NULL option definition.
ASSERT_THROW(cfg_mgr.addOptionDef(OptionDefinitionPtr(), "isc"),
isc::dhcp::MalformedOptionDefinition);
+ // Try adding option definition twice and make sure that it
+ // fails on the second attempt.
+ ASSERT_NO_THROW(cfg_mgr.addOptionDef(def, "isc"));
+ EXPECT_THROW(cfg_mgr.addOptionDef(def, "isc"), DuplicateOptionDefinition);
}
// This test verifies if the configuration manager is able to hold and return
diff --git a/src/lib/dhcpsrv/tests/subnet_unittest.cc b/src/lib/dhcpsrv/tests/subnet_unittest.cc
index 7b439f6..2e10eaa 100644
--- a/src/lib/dhcpsrv/tests/subnet_unittest.cc
+++ b/src/lib/dhcpsrv/tests/subnet_unittest.cc
@@ -441,11 +441,11 @@ TEST(Subnet6Test, getOptionSingle) {
for (uint16_t code = 100; code < 110; ++code) {
std::ostringstream stream;
// First, try the invalid option space name.
- Subnet::OptionDescriptor desc = subnet->getOptionSingle("isc", code);
+ Subnet::OptionDescriptor desc = subnet->getOptionDescriptor("isc", code);
// Returned descriptor should contain NULL option ptr.
EXPECT_FALSE(desc.option);
// Now, try the valid option space.
- desc = subnet->getOptionSingle("dhcp6", code);
+ desc = subnet->getOptionDescriptor("dhcp6", code);
// Test that the option code matches the expected code.
ASSERT_TRUE(desc.option);
EXPECT_EQ(code, desc.option->getType());
More information about the bind10-changes
mailing list