diff --git a/src/lib/dhcp/libdhcp++.cc b/src/lib/dhcp/libdhcp++.cc index b19ed218d1..a3921cc6b6 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 c325aa5a0c..bc47405f98 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 3a76e150f0..efcaba0603 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 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 e44ef5807c..a59da125b9 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 1effe7ca0b..193421f801 100644 --- a/src/lib/dhcpsrv/cfgmgr.cc +++ b/src/lib/dhcpsrv/cfgmgr.cc @@ -13,6 +13,7 @@ // PERFORMANCE OF THIS SOFTWARE. #include +#include #include 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::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 5cf40e0d7d..6e3dd42d41 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 OptionDefsMap; + /// A map containing option definitions for different option spaces. /// The map key holds an option space name. - std::map 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 70e762e76c..e6f5302f68 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 4cdcf11154..d4a8428ab5 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 1b723c994c..2e30b75784 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 7b439f6f1d..2e10eaac68 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());