From: Francis Dupont Date: Sat, 4 Mar 2017 14:42:24 +0000 (+0100) Subject: [5145b] Rebased keeping host_parser in hooks X-Git-Tag: fdunparse2_base~4 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=cf8d112211cca033515369a42e5f452d20148fcd;p=thirdparty%2Fkea.git [5145b] Rebased keeping host_parser in hooks --- diff --git a/src/bin/agent/ca_cfg_mgr.cc b/src/bin/agent/ca_cfg_mgr.cc index 49b214ff88..bde6859e44 100644 --- a/src/bin/agent/ca_cfg_mgr.cc +++ b/src/bin/agent/ca_cfg_mgr.cc @@ -24,7 +24,7 @@ CtrlAgentCfgContext::CtrlAgentCfgContext() CtrlAgentCfgContext::CtrlAgentCfgContext(const CtrlAgentCfgContext& orig) : DCfgContextBase(),http_host_(orig.http_host_), http_port_(orig.http_port_), - libraries_(orig.libraries_) { + hooks_config_(orig.hooks_config_) { // We're copying pointers here only. The underlying data will be shared by // old and new context. That's how shared pointers work and I see no reason @@ -74,7 +74,7 @@ CtrlAgentCfgMgr::getConfigSummary(const uint32_t /*selection*/) { } // Finally, print the hook libraries names - const hooks::HookLibsCollection libs = ctx->getLibraries(); + const isc::hooks::HookLibsCollection libs = ctx->getHooksConfig().get(); s << ", " << libs.size() << " lib(s):"; for (auto lib = libs.begin(); lib != libs.end(); ++lib) { s << lib->first << " "; diff --git a/src/bin/agent/ca_cfg_mgr.h b/src/bin/agent/ca_cfg_mgr.h index 7dc795fbbb..46401a67d6 100644 --- a/src/bin/agent/ca_cfg_mgr.h +++ b/src/bin/agent/ca_cfg_mgr.h @@ -8,9 +8,9 @@ #define CTRL_AGENT_CFG_MGR_H #include +#include #include #include -#include namespace isc { namespace agent { @@ -61,7 +61,7 @@ public: /// /// @param type type of the server being controlled /// @return pointer to the Element that holds control-socket map (or NULL) - const data::ConstElementPtr getControlSocketInfo(ServerType type) const; + const isc::data::ConstElementPtr getControlSocketInfo(ServerType type) const; /// @brief Sets information about the control socket /// @@ -102,20 +102,20 @@ public: return (http_port_); } - /// @brief Returns a list of hook libraries - /// @return a list of hook libraries - const hooks::HookLibsCollection& getLibraries() const { - return (libraries_); + /// @brief Returns non-const reference to configured hooks libraries. + /// + /// @return non-const reference to configured hooks libraries. + isc::hooks::HooksConfig& getHooksConfig() { + return (hooks_config_); } - /// @brief Sets the list of hook libraries + /// @brief Returns const reference to configured hooks libraries. /// - /// @params libs a coolection of libraries to remember. - void setLibraries(const hooks::HookLibsCollection& libs) { - libraries_ = libs; + /// @return const reference to configured hooks libraries. + const isc::hooks::HooksConfig& getHooksConfig() const { + return (hooks_config_); } - private: /// @brief Private copy constructor @@ -132,7 +132,7 @@ private: CtrlAgentCfgContext& operator=(const CtrlAgentCfgContext& rhs); /// Socket information will be stored here (for all supported servers) - data::ConstElementPtr ctrl_sockets_[MAX_TYPE_SUPPORTED + 1]; + isc::data::ConstElementPtr ctrl_sockets_[MAX_TYPE_SUPPORTED + 1]; /// Hostname the CA should listen on. std::string http_host_; @@ -140,8 +140,8 @@ private: /// TCP port the CA should listen on. uint16_t http_port_; - /// List of hook libraries. - hooks::HookLibsCollection libraries_; + /// @brief Configured hooks libraries. + isc::hooks::HooksConfig hooks_config_; }; /// @brief Ctrl Agent Configuration Manager. diff --git a/src/bin/agent/simple_parser.cc b/src/bin/agent/simple_parser.cc index 68bf115d85..8a6c279e38 100644 --- a/src/bin/agent/simple_parser.cc +++ b/src/bin/agent/simple_parser.cc @@ -107,22 +107,21 @@ AgentSimpleParser::parse(const CtrlAgentCfgContextPtr& ctx, } // Finally, let's get the hook libs! - hooks::HooksLibrariesParser hooks_parser; + + using namespace isc::hooks; + HooksConfig& libraries = ctx->getHooksConfig(); ConstElementPtr hooks = config->get("hooks-libraries"); if (hooks) { - hooks_parser.parse(hooks); - hooks_parser.verifyLibraries(); - - hooks::HookLibsCollection libs; - hooks_parser.getLibraries(libs); - ctx->setLibraries(libs); + HooksLibrariesParser hooks_parser; + hooks_parser.parse(libraries, hooks); + libraries.verifyLibraries(hooks->getPosition()); } if (!check_only) { // This occurs last as if it succeeds, there is no easy way // revert it. As a result, the failure to commit a subsequent // change causes problems when trying to roll back. - hooks_parser.loadLibraries(); + libraries.loadLibraries(); } } diff --git a/src/bin/agent/tests/ca_cfg_mgr_unittests.cc b/src/bin/agent/tests/ca_cfg_mgr_unittests.cc index 1ff067a5f1..a3e6fcaafe 100644 --- a/src/bin/agent/tests/ca_cfg_mgr_unittests.cc +++ b/src/bin/agent/tests/ca_cfg_mgr_unittests.cc @@ -10,12 +10,12 @@ #include #include #include -#include #include #include using namespace isc::agent; using namespace isc::data; +using namespace isc::dhcp; using namespace isc::hooks; using namespace isc::process; @@ -123,11 +123,10 @@ TEST(CtrlAgentCfgMgr, contextSocketInfoCopy) { EXPECT_NO_THROW(ctx.setHttpPort(12345)); EXPECT_NO_THROW(ctx.setHttpHost("bellatrix")); - HookLibsCollection libs; + HooksConfig& libs = ctx.getHooksConfig(); string exp_name("testlib1.so"); ConstElementPtr exp_param(new StringElement("myparam")); - libs.push_back(make_pair(exp_name, exp_param)); - ctx.setLibraries(libs); + libs.add(exp_name, exp_param); // Make a copy. DCfgContextBasePtr copy_base(ctx.clone()); @@ -147,7 +146,7 @@ TEST(CtrlAgentCfgMgr, contextSocketInfoCopy) { EXPECT_EQ(socket3->str(), copy->getControlSocketInfo(CtrlAgentCfgContext::TYPE_DHCP6)->str()); // Check hook libs - HookLibsCollection libs2 = copy->getLibraries(); + const HookLibsCollection& libs2 = copy->getHooksConfig().get(); ASSERT_EQ(1, libs2.size()); EXPECT_EQ(exp_name, libs2[0].first); ASSERT_TRUE(libs2[0].second); @@ -160,19 +159,18 @@ TEST(CtrlAgentCfgMgr, contextHookParams) { CtrlAgentCfgContext ctx; // By default there should be no hooks. - HookLibsCollection libs = ctx.getLibraries(); - EXPECT_TRUE(libs.empty()); + HooksConfig& libs = ctx.getHooksConfig(); + EXPECT_TRUE(libs.get().empty()); - libs.push_back(std::make_pair("libone.so", ConstElementPtr())); - libs.push_back(std::make_pair("libtwo.so", Element::fromJSON("{\"foo\": true}"))); - libs.push_back(std::make_pair("libthree.so", Element::fromJSON("{\"bar\": 42}"))); + libs.add("libone.so", ConstElementPtr()); + libs.add("libtwo.so", Element::fromJSON("{\"foo\": true}")); + libs.add("libthree.so", Element::fromJSON("{\"bar\": 42}")); - ctx.setLibraries(libs); + const HooksConfig& stored_libs = ctx.getHooksConfig(); + EXPECT_EQ(3, stored_libs.get().size()); - HookLibsCollection stored_libs = ctx.getLibraries(); - EXPECT_EQ(3, stored_libs.size()); - - EXPECT_EQ(libs, stored_libs); + // @todo add a == operator to HooksConfig + EXPECT_EQ(libs.get(), stored_libs.get()); } /// Control Agent configurations used in tests. @@ -389,7 +387,7 @@ TEST_F(AgentParserTest, configParseHooks) { // The context now should have the library specified. CtrlAgentCfgContextPtr ctx = cfg_mgr_.getCtrlAgentCfgContext(); - HookLibsCollection libs = ctx->getLibraries(); + const HookLibsCollection libs = ctx->getHooksConfig().get(); ASSERT_EQ(1, libs.size()); EXPECT_EQ(string(BASIC_CALLOUT_LIBRARY), libs[0].first); ASSERT_TRUE(libs[0].second); diff --git a/src/bin/dhcp4/json_config_parser.cc b/src/bin/dhcp4/json_config_parser.cc index ab06f2213e..5b6102f370 100644 --- a/src/bin/dhcp4/json_config_parser.cc +++ b/src/bin/dhcp4/json_config_parser.cc @@ -42,6 +42,7 @@ using namespace isc; using namespace isc::dhcp; using namespace isc::data; using namespace isc::asiolink; +using namespace isc::hooks; namespace { @@ -431,11 +432,6 @@ configureDhcp4Server(Dhcpv4Srv&, isc::data::ConstElementPtr config_set, // for option definitions. This is equivalent to commiting empty container. LibDHCP::setRuntimeOptionDefs(OptionDefSpaceContainer()); - // Some of the parsers alter the state of the system in a way that can't - // easily be undone. (Or alter it in a way such that undoing the change has - // the same risk of failure as doing the change.) - hooks::HooksLibrariesParser hooks_parser; - // Answer will hold the result. ConstElementPtr answer; // Rollback informs whether error occurred and original data @@ -515,8 +511,10 @@ configureDhcp4Server(Dhcpv4Srv&, isc::data::ConstElementPtr config_set, } if (config_pair.first == "hooks-libraries") { - hooks_parser.parse(config_pair.second); - hooks_parser.verifyLibraries(); + HooksLibrariesParser hooks_parser; + HooksConfig& libraries = srv_cfg->getHooksConfig(); + hooks_parser.parse(libraries, config_pair.second); + libraries.verifyLibraries(config_pair.second->getPosition()); continue; } @@ -637,7 +635,9 @@ configureDhcp4Server(Dhcpv4Srv&, isc::data::ConstElementPtr config_set, // This occurs last as if it succeeds, there is no easy way // revert it. As a result, the failure to commit a subsequent // change causes problems when trying to roll back. - hooks_parser.loadLibraries(); + const HooksConfig& libraries = + CfgMgr::instance().getStagingCfg()->getHooksConfig(); + libraries.loadLibraries(); } catch (const isc::Exception& ex) { LOG_ERROR(dhcp4_logger, DHCP4_PARSER_COMMIT_FAIL).arg(ex.what()); diff --git a/src/bin/dhcp6/json_config_parser.cc b/src/bin/dhcp6/json_config_parser.cc index 649caee260..c029895149 100644 --- a/src/bin/dhcp6/json_config_parser.cc +++ b/src/bin/dhcp6/json_config_parser.cc @@ -54,6 +54,7 @@ using namespace isc; using namespace isc::data; using namespace isc::dhcp; using namespace isc::asiolink; +using namespace isc::hooks; namespace { @@ -638,11 +639,6 @@ configureDhcp6Server(Dhcpv6Srv&, isc::data::ConstElementPtr config_set, // for option definitions. This is equivalent to commiting empty container. LibDHCP::setRuntimeOptionDefs(OptionDefSpaceContainer()); - // Some of the parsers alter state of the system that can't easily - // be undone. (Or alter it in a way such that undoing the change - // has the same risk of failure as doing the change.) - hooks::HooksLibrariesParser hooks_parser; - // This is a way to convert ConstElementPtr to ElementPtr. // We need a config that can be edited, because we will insert // default values and will insert derived values as well. @@ -738,8 +734,10 @@ configureDhcp6Server(Dhcpv6Srv&, isc::data::ConstElementPtr config_set, } if (config_pair.first == "hooks-libraries") { - hooks_parser.parse(config_pair.second); - hooks_parser.verifyLibraries(); + HooksLibrariesParser hooks_parser; + HooksConfig& libraries = srv_config->getHooksConfig(); + hooks_parser.parse(libraries, config_pair.second); + libraries.verifyLibraries(config_pair.second->getPosition()); continue; } @@ -860,7 +858,9 @@ configureDhcp6Server(Dhcpv6Srv&, isc::data::ConstElementPtr config_set, // This occurs last as if it succeeds, there is no easy way to // revert it. As a result, the failure to commit a subsequent // change causes problems when trying to roll back. - hooks_parser.loadLibraries(); + const HooksConfig& libraries = + CfgMgr::instance().getStagingCfg()->getHooksConfig(); + libraries.loadLibraries(); } catch (const isc::Exception& ex) { LOG_ERROR(dhcp6_logger, DHCP6_PARSER_COMMIT_FAIL).arg(ex.what()); diff --git a/src/lib/Makefile.am b/src/lib/Makefile.am index 031a3498ab..6b8694cc45 100644 --- a/src/lib/Makefile.am +++ b/src/lib/Makefile.am @@ -1,3 +1,3 @@ # The following build order must be maintained. -SUBDIRS = exceptions util log cryptolink dns cc hooks asiolink testutils dhcp config \ +SUBDIRS = exceptions util log cryptolink dns cc asiolink testutils hooks dhcp config \ stats asiodns dhcp_ddns eval dhcpsrv cfgrpt process http diff --git a/src/lib/dhcpsrv/srv_config.cc b/src/lib/dhcpsrv/srv_config.cc index 15bfd03d31..f680128d53 100644 --- a/src/lib/dhcpsrv/srv_config.cc +++ b/src/lib/dhcpsrv/srv_config.cc @@ -110,6 +110,14 @@ SrvConfig::copy(SrvConfig& new_config) const { new_config.class_dictionary_.reset(new ClientClassDictionary(*class_dictionary_)); // Replace the D2 client configuration new_config.setD2ClientConfig(getD2ClientConfig()); + // Replace configured hooks libraries. + new_config.hooks_config_.clear(); + using namespace isc::hooks; + for (HookLibsCollection::const_iterator it = + hooks_config_.get().begin(); + it != hooks_config_.get().end(); ++it) { + new_config.hooks_config_.add(it->first, it->second); + } } void @@ -151,11 +159,49 @@ SrvConfig::equals(const SrvConfig& other) const { } } // Logging information is equal between objects, so check other values. - return ((*cfg_iface_ == *other.cfg_iface_) && - (*cfg_option_def_ == *other.cfg_option_def_) && - (*cfg_option_ == *other.cfg_option_) && - (*class_dictionary_ == *other.class_dictionary_) && - (*d2_client_config_ == *other.d2_client_config_)); + if ((*cfg_iface_ != *other.cfg_iface_) || + (*cfg_option_def_ != *other.cfg_option_def_) || + (*cfg_option_ != *other.cfg_option_) || + (*class_dictionary_ != *other.class_dictionary_) || + (*d2_client_config_ != *other.d2_client_config_)) { + return (false); + } + // Now only configured hooks libraries can differ. + // If number of configured hooks libraries are different, then + // configurations aren't equal. + if (hooks_config_.get().size() != other.hooks_config_.get().size()) { + return (false); + } + // Pass through all configured hooks libraries. + using namespace isc::hooks; + for (isc::hooks::HookLibsCollection::const_iterator this_it = + hooks_config_.get().begin(); + this_it != hooks_config_.get().end(); ++this_it) { + bool match = false; + for (isc::hooks::HookLibsCollection::const_iterator other_it = + other.hooks_config_.get().begin(); + other_it != other.hooks_config_.get().end(); ++other_it) { + if (this_it->first != other_it->first) { + continue; + } + if (isNull(this_it->second) && isNull(other_it->second)) { + match = true; + break; + } + if (isNull(this_it->second) || isNull(other_it->second)) { + continue; + } + if (this_it->second->equals(*other_it->second)) { + match = true; + break; + } + } + // No match found for the particular hooks library so return false. + if (!match) { + return (false); + } + } + return (true); } void diff --git a/src/lib/dhcpsrv/srv_config.h b/src/lib/dhcpsrv/srv_config.h index 4bfa4ccae3..6c12548dfd 100644 --- a/src/lib/dhcpsrv/srv_config.h +++ b/src/lib/dhcpsrv/srv_config.h @@ -22,6 +22,7 @@ #include #include #include +#include #include #include #include @@ -365,6 +366,20 @@ public: class_dictionary_ = dictionary; } + /// @brief Returns non-const reference to configured hooks libraries. + /// + /// @return non-const reference to configured hooks libraries. + isc::hooks::HooksConfig& getHooksConfig() { + return (hooks_config_); + } + + /// @brief Returns const reference to configured hooks libraries. + /// + /// @return const reference to configured hooks libraries. + const isc::hooks::HooksConfig& getHooksConfig() const { + return (hooks_config_); + } + /// @brief Copies the current configuration to a new configuration. /// /// This method copies the parameters stored in the configuration to @@ -593,6 +608,9 @@ private: /// @brief Pointer to the dictionary of global client class definitions ClientClassDictionaryPtr class_dictionary_; + /// @brief Configured hooks libraries. + isc::hooks::HooksConfig hooks_config_; + /// @brief Decline Period time /// /// This timer specifies decline probation period, the time after a declined diff --git a/src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc b/src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc index 15a40c633b..810f3dec0a 100644 --- a/src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc +++ b/src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc @@ -18,10 +18,10 @@ #include #include #include -#include #include #include #include +#include #include #include @@ -377,10 +377,12 @@ public: } if (config_pair.first == "hooks-libraries") { - hooks_libraries_parser_.reset(new HooksLibrariesParser()); - hooks_libraries_parser_->parse(config_pair.second); - hooks_libraries_parser_->verifyLibraries(); - hooks_libraries_parser_->loadLibraries(); + HooksLibrariesParser hook_parser; + HooksConfig& libraries = + CfgMgr::instance().getStagingCfg()->getHooksConfig(); + hook_parser.parse(libraries, config_pair.second); + libraries.verifyLibraries(config_pair.second->getPosition()); + libraries.loadLibraries(); continue; } } @@ -610,10 +612,10 @@ public: CfgMgr::instance().setD2ClientConfig(tmp); } - /// @brief Parsers used in the parsing of the configuration - /// - /// Allows the tests to interrogate the state of the parsers (if required). - boost::shared_ptr hooks_libraries_parser_; + /// Allows the tests to interrogate the state of the libraries (if required). + const isc::hooks::HookLibsCollection& getLibraries() { + return (CfgMgr::instance().getStagingCfg()->getHooksConfig().get()); + } /// @brief specifies IP protocol family (AF_INET or AF_INET6) uint16_t family_; @@ -1306,8 +1308,7 @@ TEST_F(ParseConfigTest, noHooksLibraries) { ASSERT_TRUE(rcode == 0) << error_text_; // Check that the parser recorded nothing. - isc::hooks::HookLibsCollection libraries; - hooks_libraries_parser_->getLibraries(libraries); + isc::hooks::HookLibsCollection libraries = getLibraries(); EXPECT_TRUE(libraries.empty()); // Check that there are still no libraries loaded. @@ -1329,8 +1330,7 @@ TEST_F(ParseConfigTest, oneHooksLibrary) { ASSERT_TRUE(rcode == 0) << error_text_; // Check that the parser recorded a single library. - HookLibsCollection libraries; - hooks_libraries_parser_->getLibraries(libraries); + isc::hooks::HookLibsCollection libraries = getLibraries(); ASSERT_EQ(1, libraries.size()); EXPECT_EQ(CALLOUT_LIBRARY_1, libraries[0].first); @@ -1355,8 +1355,7 @@ TEST_F(ParseConfigTest, twoHooksLibraries) { ASSERT_TRUE(rcode == 0) << error_text_; // Check that the parser recorded two libraries in the expected order. - HookLibsCollection libraries; - hooks_libraries_parser_->getLibraries(libraries); + isc::hooks::HookLibsCollection libraries = getLibraries(); ASSERT_EQ(2, libraries.size()); EXPECT_EQ(CALLOUT_LIBRARY_1, libraries[0].first); EXPECT_EQ(CALLOUT_LIBRARY_2, libraries[1].first); @@ -1393,8 +1392,7 @@ TEST_F(ParseConfigTest, reconfigureSameHooksLibraries) { // The list has not changed between the two parse operations. However, // the paramters (or the files they could point to) could have // changed, so the libraries are reloaded anyway. - HookLibsCollection libraries; - hooks_libraries_parser_->getLibraries(libraries); + isc::hooks::HookLibsCollection libraries = getLibraries(); ASSERT_EQ(2, libraries.size()); EXPECT_EQ(CALLOUT_LIBRARY_1, libraries[0].first); EXPECT_EQ(CALLOUT_LIBRARY_2, libraries[1].first); @@ -1432,8 +1430,7 @@ TEST_F(ParseConfigTest, reconfigureReverseHooksLibraries) { ASSERT_TRUE(rcode == 0) << error_text_; // The list has changed, and this is what we should see. - HookLibsCollection libraries; - hooks_libraries_parser_->getLibraries(libraries); + isc::hooks::HookLibsCollection libraries = getLibraries(); ASSERT_EQ(2, libraries.size()); EXPECT_EQ(CALLOUT_LIBRARY_2, libraries[0].first); EXPECT_EQ(CALLOUT_LIBRARY_1, libraries[1].first); @@ -1469,8 +1466,7 @@ TEST_F(ParseConfigTest, reconfigureZeroHooksLibraries) { ASSERT_TRUE(rcode == 0) << error_text_; // The list has changed, and this is what we should see. - HookLibsCollection libraries; - hooks_libraries_parser_->getLibraries(libraries); + isc::hooks::HookLibsCollection libraries = getLibraries(); EXPECT_TRUE(libraries.empty()); // Check that no libraries are currently loaded @@ -1501,8 +1497,7 @@ TEST_F(ParseConfigTest, invalidHooksLibraries) { // Check that the parser recorded the names but, as they were in error, // does not flag them as changed. - HookLibsCollection libraries; - hooks_libraries_parser_->getLibraries(libraries); + isc::hooks::HookLibsCollection libraries = getLibraries(); ASSERT_EQ(3, libraries.size()); EXPECT_EQ(CALLOUT_LIBRARY_1, libraries[0].first); EXPECT_EQ(NOT_PRESENT_LIBRARY, libraries[1].first); @@ -1543,8 +1538,7 @@ TEST_F(ParseConfigTest, reconfigureInvalidHooksLibraries) { // Check that the parser recorded the names but, as the library set was // incorrect, did not mark the configuration as changed. - HookLibsCollection libraries; - hooks_libraries_parser_->getLibraries(libraries); + isc::hooks::HookLibsCollection libraries = getLibraries(); ASSERT_EQ(3, libraries.size()); EXPECT_EQ(CALLOUT_LIBRARY_1, libraries[0].first); EXPECT_EQ(NOT_PRESENT_LIBRARY, libraries[1].first); @@ -1648,8 +1642,7 @@ TEST_F(ParseConfigTest, HooksLibrariesParameters) { ASSERT_EQ(0, rcode); // Check that the parser recorded the names. - HookLibsCollection libraries; - hooks_libraries_parser_->getLibraries(libraries); + isc::hooks::HookLibsCollection libraries = getLibraries(); ASSERT_EQ(3, libraries.size()); EXPECT_EQ(CALLOUT_LIBRARY_1, libraries[0].first); EXPECT_EQ(CALLOUT_LIBRARY_2, libraries[1].first); diff --git a/src/lib/dhcpsrv/tests/srv_config_unittest.cc b/src/lib/dhcpsrv/tests/srv_config_unittest.cc index 3a87083675..5622df51ae 100644 --- a/src/lib/dhcpsrv/tests/srv_config_unittest.cc +++ b/src/lib/dhcpsrv/tests/srv_config_unittest.cc @@ -400,4 +400,29 @@ TEST_F(SrvConfigTest, equality) { EXPECT_FALSE(conf1 != conf2); } +// Verifies that we can get and set configured hooks libraries +TEST_F(SrvConfigTest, hooksLibraries) { + SrvConfig conf(32); + isc::hooks::HooksConfig& libraries = conf.getHooksConfig(); + + // Upon construction configured hooks libraries should be empty. + EXPECT_EQ(0, libraries.get().size()); + + // Verify we can update it. + isc::data::ConstElementPtr elem0; + libraries.add("foo", elem0); + std::string config = "{ \"library\": \"bar\" }"; + isc::data::ConstElementPtr elem1 = isc::data::Element::fromJSON(config); + libraries.add("bar", elem1); + EXPECT_EQ(2, libraries.get().size()); + EXPECT_EQ(2, conf.getHooksConfig().get().size()); + + // Try to copy + SrvConfig copied(64); + ASSERT_TRUE(conf != copied); + ASSERT_NO_THROW(conf.copy(copied)); + ASSERT_TRUE(conf == copied); + EXPECT_EQ(2, copied.getHooksConfig().get().size()); +} + } // end of anonymous namespace diff --git a/src/lib/hooks/Makefile.am b/src/lib/hooks/Makefile.am index b3169bea14..d3f4205145 100644 --- a/src/lib/hooks/Makefile.am +++ b/src/lib/hooks/Makefile.am @@ -36,6 +36,7 @@ libkea_hooks_la_SOURCES += callout_manager.cc callout_manager.h libkea_hooks_la_SOURCES += hooks.h libkea_hooks_la_SOURCES += hooks_log.cc hooks_log.h libkea_hooks_la_SOURCES += hooks_manager.cc hooks_manager.h +libkea_hooks_la_SOURCES += hooks_config.cc hooks_config.h libkea_hooks_la_SOURCES += hooks_parser.cc hooks_parser.h libkea_hooks_la_SOURCES += libinfo.cc libinfo.h libkea_hooks_la_SOURCES += library_handle.cc library_handle.h diff --git a/src/lib/hooks/hooks_config.cc b/src/lib/hooks/hooks_config.cc new file mode 100644 index 0000000000..4ad74428e0 --- /dev/null +++ b/src/lib/hooks/hooks_config.cc @@ -0,0 +1,87 @@ +// Copyright (C) 2017 Internet Systems Consortium, Inc. ("ISC") +// +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this +// file, You can obtain one at http://mozilla.org/MPL/2.0/. + +#include + +#include +#include + +using namespace std; +using namespace isc; +using namespace isc::data; + +namespace isc { +namespace hooks { + +void +HooksConfig::verifyLibraries(const Element::Position& position) const { + // Check if the list of libraries has changed. If not, nothing is done + // - the command "DhcpN libreload" is required to reload the same + // libraries (this prevents needless reloads when anything else in the + // configuration is changed). + + // We no longer rely on this. Parameters can change. And even if the + // parameters stay the same, they could point to files that could + // change. + vector current_libraries = HooksManager::getLibraryNames(); + if (current_libraries.empty() && libraries_.empty()) { + return; + } + + // Library list has changed, validate each of the libraries specified. + vector lib_names = isc::hooks::extractNames(libraries_); + vector error_libs = HooksManager::validateLibraries(lib_names); + if (!error_libs.empty()) { + + // Construct the list of libraries in error for the message. + string error_list = error_libs[0]; + for (size_t i = 1; i < error_libs.size(); ++i) { + error_list += (string(", ") + error_libs[i]); + } + isc_throw(InvalidHooksLibraries, + "hooks libraries failed to validate - " + "library or libraries in error are: " + << error_list << "(" << position << ")"); + } +} + +void +HooksConfig::loadLibraries() const { + /// Commits the list of libraries to the configuration manager storage if + /// the list of libraries has changed. + /// @todo: Delete any stored CalloutHandles before reloading the + /// libraries + if (!HooksManager::loadLibraries(libraries_)) { + isc_throw(InvalidHooksLibraries, + "One or more hook libraries failed to load"); + } +} + +ElementPtr +HooksConfig::toElement() const { + // hooks-libraries is a list of maps + ElementPtr result = Element::createList(); + // Iterate through libraries + for (HookLibsCollection::const_iterator hl = libraries_.begin(); + hl != libraries_.end(); ++hl) { + // Entries are maps + ElementPtr map = Element::createMap(); + // Set the library name + map->set("library", Element::create(hl->first)); + // Set parameters + if (!isNull(hl->second)) { + map->set("parameters", hl->second); + } else { + map->set("parameters", Element::createMap()); + } + // Push to the list + result->add(map); + } + return (result); +} + +}; +}; diff --git a/src/lib/hooks/hooks_config.h b/src/lib/hooks/hooks_config.h new file mode 100644 index 0000000000..49a49c0410 --- /dev/null +++ b/src/lib/hooks/hooks_config.h @@ -0,0 +1,103 @@ +// Copyright (C) 2017 Internet Systems Consortium, Inc. ("ISC") +// +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this +// file, You can obtain one at http://mozilla.org/MPL/2.0/. + +#ifndef HOOKS_CONFIG_H +#define HOOKS_CONFIG_H + +#include +#include +#include +#include + +namespace isc { +namespace hooks { + +/// @brief Exception thrown when a library failed to validate +class InvalidHooksLibraries : public Exception { + public: + InvalidHooksLibraries(const char* file, size_t line, const char* what) : + isc::Exception(file, line, what) { }; +}; + +/// @brief Wrapper class that holds hooks libraries configuration +/// +/// This was moved from HooksLibrariesParser +/// +/// However, this class does more than just check the list of library names. +/// It does two other things: +/// # validate libraries +/// # load libraries +/// and it provides a toElement() method to unparse a configuration. +/// +/// @todo add toElement() unit tests +class HooksConfig : public isc::data::CfgToElement { +public: + /// @brief Default constructor. + /// + HooksConfig() : libraries_() { } + + /// @brief Adds additional hooks libraries. + /// + /// @param libname full filename with path to the library. + /// @param parameters map of parameters that configure the library. + void add(std::string libname, isc::data::ConstElementPtr parameters) { + libraries_.push_back(make_pair(libname, parameters)); + } + + /// @brief Provides access to the configured hooks libraries. + /// + /// @note The const reference returned is only valid as long as the + /// object that returned it. + const isc::hooks::HookLibsCollection& get() const { + return libraries_; + } + + /// @brief Removes all configured hooks libraries. + void clear() { + libraries_.clear(); + } + + /// @brief Verifies that libraries stored in libraries_ are valid. + /// + /// This method is a smart wrapper around @ref + /// isc::hooks::HooksManager::validateLibraries(). + /// It tries to validate all the libraries stored in libraries_. + /// + /// @param position position of the hooks-library map for error reporting + /// @throw InvalidHooksLibraries if any issue is discovered. + void verifyLibraries(const isc::data::Element::Position& position) const; + + /// @brief Commits hooks libraries configuration. + /// + /// This method calls necessary methods in HooksManager that will unload + /// any libraries that may be currently loaded and will load the actual + /// libraries. Providing that the specified libraries are valid and are + /// different to those already loaded, this method loads the new set of + /// libraries (and unloads the existing set). + /// + /// @throw InvalidHooksLibraries if the call to HooksManager fails. + void loadLibraries() const; + + /// @brief Unparse a configuration objet + /// + /// Returns an element which must parse into the same objet, i.e. + /// @code + /// for all valid config C parse(parse(C)->toElement()) == parse(C) + /// @endcode + /// + /// @return a pointer to a configuration which can be parsed into + /// the initial configuration object + isc::data::ElementPtr toElement() const; + +private: + /// @brief List of hooks libraries with their configuration parameters + isc::hooks::HookLibsCollection libraries_; +}; + +}; +}; + +#endif // HOOKS_CONFIG_H diff --git a/src/lib/hooks/hooks_parser.cc b/src/lib/hooks/hooks_parser.cc index 4c4d9057fe..cfd4a7e8f5 100644 --- a/src/lib/hooks/hooks_parser.cc +++ b/src/lib/hooks/hooks_parser.cc @@ -9,7 +9,6 @@ #include #include #include -#include #include #include #include @@ -23,19 +22,17 @@ using namespace isc::dhcp; namespace isc { namespace hooks { -// ******************** HooksLibrariesParser ************************* +// @todo use the flat style, split into list and item + void -HooksLibrariesParser::parse(ConstElementPtr value) { +HooksLibrariesParser::parse(HooksConfig& libraries, ConstElementPtr value) { // Initialize. - libraries_.clear(); + libraries.clear(); if (!value) { isc_throw(DhcpConfigError, "Tried to parse null hooks libraries"); } - // Let's store - position_ = value->getPosition(); - // This is the new syntax. Iterate through it and get each map. BOOST_FOREACH(ConstElementPtr library_entry, value->listValue()) { ConstElementPtr parameters; @@ -105,56 +102,9 @@ HooksLibrariesParser::parse(ConstElementPtr value) { " (" << library_entry->getPosition() << ")"); } - libraries_.push_back(make_pair(libname, parameters)); - } -} - -void HooksLibrariesParser::verifyLibraries() { - // Check if the list of libraries has changed. If not, nothing is done - // - the command "DhcpN libreload" is required to reload the same - // libraries (this prevents needless reloads when anything else in the - // configuration is changed). - - // We no longer rely on this. Parameters can change. And even if the - // parameters stay the same, they could point to files that could - // change. - vector current_libraries = HooksManager::getLibraryNames(); - if (current_libraries.empty() && libraries_.empty()) { - return; - } - - // Library list has changed, validate each of the libraries specified. - vector lib_names = isc::hooks::extractNames(libraries_); - vector error_libs = HooksManager::validateLibraries(lib_names); - if (!error_libs.empty()) { - - // Construct the list of libraries in error for the message. - string error_list = error_libs[0]; - for (size_t i = 1; i < error_libs.size(); ++i) { - error_list += (string(", ") + error_libs[i]); - } - isc_throw(DhcpConfigError, "hooks libraries failed to validate - " - "library or libraries in error are: " << error_list - << "(" << position_ << ")"); - } -} - -void -HooksLibrariesParser::loadLibraries() { - /// Commits the list of libraries to the configuration manager storage if - /// the list of libraries has changed. - /// @todo: Delete any stored CalloutHandles before reloading the - /// libraries - if (!HooksManager::loadLibraries(libraries_)) { - isc_throw(DhcpConfigError, "One or more hook libraries failed to load"); + libraries.add(libname, parameters); } } -// Method for testing -void -HooksLibrariesParser::getLibraries(isc::hooks::HookLibsCollection& libraries) { - libraries = libraries_; -} - } } diff --git a/src/lib/hooks/hooks_parser.h b/src/lib/hooks/hooks_parser.h index 62d405c41a..d6fc5ef646 100644 --- a/src/lib/hooks/hooks_parser.h +++ b/src/lib/hooks/hooks_parser.h @@ -9,7 +9,7 @@ #include #include -#include +#include namespace isc { namespace hooks { @@ -17,23 +17,7 @@ namespace hooks { /// @brief Parser for hooks library list /// /// This parser handles the list of hooks libraries. This is an optional list, -/// which may be empty. -/// -/// However, the parser does more than just check the list of library names. -/// It does two other things: -/// -/// -# The problem faced with the hooks libraries is that we wish to avoid -/// reloading the libraries if they have not changed. (This would cause the -/// "unload" and "load" methods to run. Although libraries should be written -/// to cope with this, it is feasible that such an action may be costly in -/// terms of time and resources, or may cause side effects such as clearing -/// an internal cache.) To this end, the parser also checks the list against -/// the list of libraries current loaded and notes if there are changes. -/// -# If there are, the parser validates the libraries; it opens them and -/// checks that the "version" function exists and returns the correct value. -/// -/// Only if the library list has changed and the libraries are valid will the -/// change be applied. +/// which may be empty, and is encapsulated into a @ref HooksConfig object. class HooksLibrariesParser : public isc::data::SimpleParser { public: @@ -68,51 +52,10 @@ public: /// -# That there is an optional 'parameters' element. /// -# That there are no other element. /// - /// If you want to check whether the library is really present (if the file - /// is on disk, it is really a library and that it could be loaded), call - /// @ref verifyLibraries(). - /// - /// This method stores parsed libraries in libraries_. + /// This method stores parsed libraries in libraries. /// /// @param value pointer to the content of parsed values - void parse(isc::data::ConstElementPtr value); - - /// @brief Verifies that libraries stored in libraries_ are valid. - /// - /// This method is a smart wrapper around @ref - /// isc::hooks::HooksManager::validateLibraries(). - /// It tries to validate all the libraries stored in libraries_. - /// @throw DhcpConfigError if any issue is discovered. - void verifyLibraries(); - - /// @brief Commits hooks libraries data - /// - /// This method calls necessary methods in HooksManager that will unload - /// any libraries that may be currently loaded and will load the actual - /// libraries. Providing that the specified libraries are valid and are - /// different to those already loaded, this method loads the new set of - /// libraries (and unloads the existing set). - /// - /// @throw DhcpConfigError if the call to HooksManager fails. - void loadLibraries(); - - /// @brief Returns list of parsed libraries - /// - /// Principally for testing, this returns the list of libraries as well as - /// an indication as to whether the list is different from the list of - /// libraries already loaded. - /// - /// @param [out] libraries List of libraries that were specified in the - /// new configuration. - void getLibraries(isc::hooks::HookLibsCollection& libraries); - -private: - /// List of hooks libraries with their configuration parameters - isc::hooks::HookLibsCollection libraries_; - - /// Position of the original element is stored in case we need to report an - /// error later. - isc::data::Element::Position position_; + void parse(HooksConfig& libraries, isc::data::ConstElementPtr value); }; }; // namespace isc::hooks