From b45f1ab92f69f6e5270b10a3b8d59c51185d8a9b Mon Sep 17 00:00:00 2001 From: Thomas Markwalder Date: Fri, 15 Mar 2019 10:34:38 -0400 Subject: [PATCH] [#401,!254] Implemented CfgOption::replace src/lib/dhcpsrv/cfg_option.* CfgOption::replace() - new method to update an OptionDescriptor CfgOption::createDescriptorOption() - new returns a bool indicating whether or not the option_ instance was replaced --- src/lib/dhcpsrv/cfg_option.cc | 50 +++++++++----- src/lib/dhcpsrv/cfg_option.h | 16 ++++- src/lib/dhcpsrv/tests/cfg_option_unittest.cc | 69 ++++++++++++++++++-- 3 files changed, 111 insertions(+), 24 deletions(-) diff --git a/src/lib/dhcpsrv/cfg_option.cc b/src/lib/dhcpsrv/cfg_option.cc index e1268322c2..54d4f8f456 100644 --- a/src/lib/dhcpsrv/cfg_option.cc +++ b/src/lib/dhcpsrv/cfg_option.cc @@ -68,6 +68,31 @@ CfgOption::add(const OptionDescriptor& desc, const std::string& option_space) { } } +void +CfgOption::replace(const OptionDescriptor& desc, const std::string& option_space) { + if (!desc.option_) { + isc_throw(isc::BadValue, "option being replaced must not be NULL"); + } + + // Check for presence of options. + OptionContainerPtr options = getAll(option_space); + if (!options) { + isc_throw(isc::BadValue, "option space" << option_space << " does not exist"); + } + + // Find the option we want to replace. + OptionContainerTypeIndex& idx = options->get<1>(); + OptionContainerTypeIndex::const_iterator od_itr = idx.find(desc.option_->getType()); + if (od_itr == idx.end()) { + isc_throw(isc::BadValue, "cannot replace option: " + << option_space << ":" << desc.option_->getType() + << ", it does not exist"); + } + + idx.replace(od_itr, desc); +} + + std::list CfgOption::getVendorIdsSpaceNames() const { std::list ids = getVendorIds(); @@ -103,25 +128,17 @@ CfgOption::createOptions(CfgOptionDefPtr cfg_def) { // Iterate over all the option descriptors in // all the spaces and instantiate the options // based on the given definitions. - - // Descriptors can only be fetched as copies of - // what is in the container. Since we don't - // currently have a replace mechanism, we'll - // create a revamped set of descriptors and then - // copy them on top of ourself. - CfgOption revamped; for (auto space : getOptionSpaceNames()) { for (auto opt_desc : *(getAll(space))) { - createDescriptorOption(cfg_def, space, opt_desc); - revamped.add(opt_desc, space); + if (createDescriptorOption(cfg_def, space, opt_desc)) { + // Option was recreated, let's replace the descriptor. + replace(opt_desc,space); + } } } - - // Copy the updated descriptors over our own. - revamped.copyTo(*this); } -void +bool CfgOption::createDescriptorOption(CfgOptionDefPtr cfg_def, const std::string& space, OptionDescriptor& opt_desc) { if (!opt_desc.option_) { @@ -129,7 +146,6 @@ CfgOption::createDescriptorOption(CfgOptionDefPtr cfg_def, const std::string& sp "validateCreateOption: descriptor has no option instance"); } - Option::Universe universe = opt_desc.option_->getUniverse(); uint16_t code = opt_desc.option_->getType(); @@ -162,7 +178,8 @@ CfgOption::createDescriptorOption(CfgOptionDefPtr cfg_def, const std::string& sp // If there's no definition and no formatted string, we'll // settle for the generic option already in the descriptor. - return; + // Indicate no-change by returning false. + return (false); } try { @@ -181,6 +198,9 @@ CfgOption::createDescriptorOption(CfgOptionDefPtr cfg_def, const std::string& sp isc_throw(InvalidOperation, "could not create option: " << space << "." << code << " from data specified, reason: " << ex.what()); } + + // Indicate we replaced the definition. + return(true); } void diff --git a/src/lib/dhcpsrv/cfg_option.h b/src/lib/dhcpsrv/cfg_option.h index b5eb66b855..f6b2ee80cc 100644 --- a/src/lib/dhcpsrv/cfg_option.h +++ b/src/lib/dhcpsrv/cfg_option.h @@ -333,13 +333,15 @@ public: /// @throw isc::BadValue if the option space is invalid. void add(const OptionDescriptor& desc, const std::string& option_space); + void replace(const OptionDescriptor& desc, const std::string& option_space); + /// @brief Merges another option configuration into this one. /// /// This method calls @c mergeTo() to add this configuration's /// options into @c other (skipping any duplicates). Next it calls /// @c createDescriptorOption() for each option descriptor in the /// merged set. This (re)-creates each descriptor's option based on - /// the merged set of opt definitioins. Finally, it calls + /// the merged set of opt definitions. Finally, it calls /// @c copyTo() to overwrite this configuration's options with /// the merged set in @c other. /// @@ -354,6 +356,15 @@ public: /// @param option configurations to merge with. void merge(CfgOptionDefPtr cfg_def, CfgOption& other); + /// @brief Re-create the option in each descriptor based on given definitions + /// + /// Invokes @c createDescriptorOption() on each option descriptor in + /// each option space, passing in the the given dictionary of option + /// definitions. If the descriptor's option is re-created, then the + /// descriptor is updated by calling @c replace(). + /// + /// @param cfg_def set of of user-defined option definitions to use + /// when creating option instances. void createOptions(CfgOptionDefPtr cfg_def); /// @brief Creates an option descriptor's option based on a set of option defs @@ -392,10 +403,11 @@ public: /// @param space the option space name of the option /// @param opt_desc OptionDescriptor describing the option. /// + /// @return True if the descriptor's option instance was replaced. /// @throw InvalidOperation if the descriptor conveys a formatted value and /// there is no definition matching the option code in the given space, or /// if the definition factory invocation fails. - static void createDescriptorOption(CfgOptionDefPtr cfg_def, const std::string& space, + static bool createDescriptorOption(CfgOptionDefPtr cfg_def, const std::string& space, OptionDescriptor& opt_desc); /// @brief Merges this configuration to another configuration. diff --git a/src/lib/dhcpsrv/tests/cfg_option_unittest.cc b/src/lib/dhcpsrv/tests/cfg_option_unittest.cc index afe7260661..d53ddb35b2 100644 --- a/src/lib/dhcpsrv/tests/cfg_option_unittest.cc +++ b/src/lib/dhcpsrv/tests/cfg_option_unittest.cc @@ -216,6 +216,55 @@ TEST_F(CfgOptionTest, add) { EXPECT_TRUE(options->empty()); } +// This test verifies that options can be replaced with udpated content. +TEST_F(CfgOptionTest, replace) { + CfgOption cfg; + + // Let's add some options to the config to the config. + OptionStringPtr option(new OptionString(Option::V6, 1, "one")); + ASSERT_NO_THROW(cfg.add(option, false, "isc")); + + option.reset(new OptionString(Option::V6, 2, "two")); + ASSERT_NO_THROW(cfg.add(option, false, "isc")); + + option.reset(new OptionString(Option::V6, 3, "three")); + ASSERT_NO_THROW(cfg.add(option, false, "isc")); + + // Now let's make sure we can find them and they are as expected. + OptionDescriptor desc = cfg.get("isc", 1); + ASSERT_TRUE(desc.option_); + ASSERT_EQ(desc.option_->toText(),"type=00001, len=00003: \"one\" (string)"); + + desc = cfg.get("isc", 2); + ASSERT_TRUE(desc.option_); + ASSERT_EQ(desc.option_->toText(),"type=00002, len=00003: \"two\" (string)"); + + desc = cfg.get("isc", 3); + ASSERT_TRUE(desc.option_); + ASSERT_EQ(desc.option_->toText(),"type=00003, len=00005: \"three\" (string)"); + + // Now let's replace one and three. + desc.option_.reset(new OptionString(Option::V6, 1, "new one")); + ASSERT_NO_THROW(cfg.replace(desc, "isc")); + + desc.option_.reset(new OptionString(Option::V6, 3, "new three")); + ASSERT_NO_THROW(cfg.replace(desc, "isc")); + + // Now let's make sure we can find them again and they are as expected. + desc = cfg.get("isc", 1); + ASSERT_TRUE(desc.option_); + ASSERT_EQ(desc.option_->toText(),"type=00001, len=00007: \"new one\" (string)"); + + desc = cfg.get("isc", 2); + ASSERT_TRUE(desc.option_); + ASSERT_EQ(desc.option_->toText(),"type=00002, len=00003: \"two\" (string)"); + + desc = cfg.get("isc", 3); + ASSERT_TRUE(desc.option_); + ASSERT_EQ(desc.option_->toText(),"type=00003, len=00009: \"new three\" (string)"); +} + + // This test verifies that one configuration can be merged into another. TEST_F(CfgOptionTest, mergeTo) { CfgOption cfg_src; @@ -482,7 +531,9 @@ TEST_F(CfgOptionTest, createDescriptorOptionValid) { option->setData(value.begin(), value.end()); OptionDescriptorPtr desc(new OptionDescriptor(option, false)); - ASSERT_NO_THROW(CfgOption::createDescriptorOption(defs, space, *desc)); + bool updated = false; + ASSERT_NO_THROW(updated = CfgOption::createDescriptorOption(defs, space, *desc)); + ASSERT_TRUE(updated); OptionStringPtr opstr = boost::dynamic_pointer_cast(desc->option_); ASSERT_TRUE(opstr); EXPECT_EQ("type=012, len=011: \"example.org\" (string)", opstr->toText()); @@ -493,7 +544,8 @@ TEST_F(CfgOptionTest, createDescriptorOptionValid) { option.reset(new Option(Option::V4, 2)); desc.reset(new OptionDescriptor(option, false, value)); - ASSERT_NO_THROW(CfgOption::createDescriptorOption(defs, space, *desc)); + ASSERT_NO_THROW(updated = CfgOption::createDescriptorOption(defs, space, *desc)); + ASSERT_TRUE(updated); std::cout << "option:" << desc->option_->toText() << std::endl; Option4AddrLstPtr opaddrs = boost::dynamic_pointer_cast(desc->option_); ASSERT_TRUE(opaddrs); @@ -504,7 +556,8 @@ TEST_F(CfgOptionTest, createDescriptorOptionValid) { option.reset(new Option(Option::V4, 1, OptionBuffer(1, 0x77))); desc.reset(new OptionDescriptor(option, false)); - ASSERT_NO_THROW(CfgOption::createDescriptorOption(defs, space, *desc)); + ASSERT_NO_THROW(updated = CfgOption::createDescriptorOption(defs, space, *desc)); + ASSERT_TRUE(updated); OptionUint8Ptr opint = boost::dynamic_pointer_cast(desc->option_); ASSERT_TRUE(opint); EXPECT_EQ("type=001, len=001: 119 (uint8)", opint->toText()); @@ -513,17 +566,19 @@ TEST_F(CfgOptionTest, createDescriptorOptionValid) { option.reset(new Option(Option::V4, 2)); desc.reset(new OptionDescriptor(option, false, "1,2,3")); - ASSERT_NO_THROW(CfgOption::createDescriptorOption(defs, space, *desc)); + ASSERT_NO_THROW(updated = CfgOption::createDescriptorOption(defs, space, *desc)); + ASSERT_TRUE(updated); OptionUint8ArrayPtr oparray = boost::dynamic_pointer_cast(desc->option_); ASSERT_TRUE(oparray); EXPECT_EQ("type=002, len=003: 1(uint8) 2(uint8) 3(uint8)", oparray->toText()); // Finally, a generic, undefined option - option.reset(new Option(Option::V4, 2, OptionBuffer(1, 0x77))); + option.reset(new Option(Option::V4, 199, OptionBuffer(1, 0x77))); desc.reset(new OptionDescriptor(option, false)); - ASSERT_NO_THROW(CfgOption::createDescriptorOption(defs, space, *desc)); - EXPECT_EQ("type=002, len=001: 119(uint8)", desc->option_->toText()); + ASSERT_NO_THROW(updated = CfgOption::createDescriptorOption(defs, space, *desc)); + ASSERT_FALSE(updated); + EXPECT_EQ("type=199, len=001: 77", desc->option_->toText()); } // This test verifies that encapsulated options are added as sub-options -- 2.47.2