From c706b28ae11ad17fde72a2f06a2cdf17f62a5514 Mon Sep 17 00:00:00 2001 From: Stephen Morris Date: Wed, 24 Jul 2013 14:51:10 +0100 Subject: [PATCH] [2981] Checkpoint prior to rebasing on updated master --- configure.ac | 2 + src/bin/dhcp4/ctrl_dhcp4_srv.cc | 2 + src/bin/dhcp6/config_parser.cc | 20 ++ src/bin/dhcp6/dhcp6.spec | 18 + src/bin/dhcp6/tests/Makefile.am | 19 +- src/bin/dhcp6/tests/callout_library_1.cc | 22 ++ src/bin/dhcp6/tests/callout_library_2.cc | 22 ++ src/bin/dhcp6/tests/callout_library_common.h | 80 +++++ src/bin/dhcp6/tests/config_parser_unittest.cc | 323 +++++++++++++++--- src/bin/dhcp6/tests/marker_file.h.in | 28 ++ src/bin/dhcp6/tests/test_libraries.h.in | 51 +++ src/lib/dhcpsrv/Makefile.am | 2 + src/lib/dhcpsrv/cfgmgr.cc | 18 - src/lib/dhcpsrv/cfgmgr.h | 62 +--- src/lib/dhcpsrv/dhcp_parsers.cc | 89 ++--- src/lib/dhcpsrv/dhcp_parsers.h | 75 ++-- src/lib/dhcpsrv/tests/Makefile.am | 3 +- src/lib/dhcpsrv/tests/cfgmgr_unittest.cc | 33 -- .../dhcpsrv/tests/dhcp_parsers_unittest.cc | 175 ++++++---- src/lib/hooks/hooks_manager.h | 7 + 20 files changed, 763 insertions(+), 288 deletions(-) create mode 100644 src/bin/dhcp6/tests/callout_library_1.cc create mode 100644 src/bin/dhcp6/tests/callout_library_2.cc create mode 100644 src/bin/dhcp6/tests/callout_library_common.h create mode 100644 src/bin/dhcp6/tests/marker_file.h.in create mode 100644 src/bin/dhcp6/tests/test_libraries.h.in diff --git a/configure.ac b/configure.ac index 6e394a8547..3a94efaf78 100644 --- a/configure.ac +++ b/configure.ac @@ -1381,6 +1381,8 @@ AC_OUTPUT([doc/version.ent src/bin/dbutil/run_dbutil.sh src/bin/dbutil/tests/dbutil_test.sh src/bin/ddns/ddns.py + src/bin/dhcp6/tests/marker_file.h + src/bin/dhcp6/tests/test_libraries.h src/bin/xfrin/tests/xfrin_test src/bin/xfrin/xfrin.py src/bin/xfrin/run_b10-xfrin.sh diff --git a/src/bin/dhcp4/ctrl_dhcp4_srv.cc b/src/bin/dhcp4/ctrl_dhcp4_srv.cc index eb7d5599a0..13bf83bf80 100644 --- a/src/bin/dhcp4/ctrl_dhcp4_srv.cc +++ b/src/bin/dhcp4/ctrl_dhcp4_srv.cc @@ -121,6 +121,8 @@ ControlledDhcpv4Srv::dhcp4CommandHandler(const string& command, ConstElementPtr ConstElementPtr answer = isc::config::createAnswer(0, "Shutting down."); return (answer); + } else if (command == "libreload") { + // TODO Reload libraries } ConstElementPtr answer = isc::config::createAnswer(1, diff --git a/src/bin/dhcp6/config_parser.cc b/src/bin/dhcp6/config_parser.cc index 74012accaa..d566bd4147 100644 --- a/src/bin/dhcp6/config_parser.cc +++ b/src/bin/dhcp6/config_parser.cc @@ -430,6 +430,8 @@ DhcpConfigParser* createGlobal6DhcpConfigParser(const std::string& config_id) { globalContext()->string_values_); } else if (config_id.compare("lease-database") == 0) { parser = new DbAccessParser(config_id); + } else if (config_id.compare("hooks-libraries") == 0) { + parser = new HooksLibrariesParser(config_id); } else { isc_throw(NotImplemented, "Parser error: Global configuration parameter not supported: " @@ -464,6 +466,11 @@ configureDhcp6Server(Dhcpv6Srv&, isc::data::ConstElementPtr config_set) { ParserPtr subnet_parser; ParserPtr option_parser; + // 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.) + ParserPtr hooks_parser; + // The subnet parsers implement data inheritance by directly // accessing global storage. For this reason the global data // parsers must store the parsed data into global storages @@ -495,6 +502,14 @@ configureDhcp6Server(Dhcpv6Srv&, isc::data::ConstElementPtr config_set) { subnet_parser = parser; } else if (config_pair.first == "option-data") { option_parser = parser; + + } else if (config_pair.first == "hooks-libraries") { + // Executing the commit will alter currently loaded hooks + // libraries. Check if the supplied libraries are valid, + // but defer the commit until after everything else has + // committed. + hooks_parser = parser; + hooks_parser->build(config_pair.second); } else { // Those parsers should be started before other // parsers so we can call build straight away. @@ -571,6 +586,11 @@ configureDhcp6Server(Dhcpv6Srv&, isc::data::ConstElementPtr config_set) { return (answer); } + // Now commit any changes that have been validated but not yet committed. + if (hooks_parser) { + hooks_parser->commit(); + } + LOG_INFO(dhcp6_logger, DHCP6_CONFIG_COMPLETE).arg(config_details); // Everything was fine. Configuration is successful. diff --git a/src/bin/dhcp6/dhcp6.spec b/src/bin/dhcp6/dhcp6.spec index bb5de0a086..44348c00e5 100644 --- a/src/bin/dhcp6/dhcp6.spec +++ b/src/bin/dhcp6/dhcp6.spec @@ -3,6 +3,19 @@ "module_name": "Dhcp6", "module_description": "DHCPv6 server daemon", "config_data": [ + { + "item_name": "hooks-libraries", + "item_type": "list", + "item_optional": true, + "item_default": [], + "list_item_spec": + { + "item_name": "hooks-library", + "item_type": "string", + "item_optional": true, + } + }, + { "item_name": "interface", "item_type": "list", "item_optional": false, @@ -295,6 +308,11 @@ "item_optional": true } ] + }, + + { + "command_name": "libreload", + "command_description": "Reloads the current hooks libraries.", } ] } diff --git a/src/bin/dhcp6/tests/Makefile.am b/src/bin/dhcp6/tests/Makefile.am index a2e2ed09db..c1d25e5d2f 100644 --- a/src/bin/dhcp6/tests/Makefile.am +++ b/src/bin/dhcp6/tests/Makefile.am @@ -27,7 +27,8 @@ AM_CPPFLAGS += -I$(top_srcdir)/src/bin AM_CPPFLAGS += $(BOOST_INCLUDES) AM_CPPFLAGS += -DINSTALL_PROG=\"$(abs_top_srcdir)/install-sh\" -CLEANFILES = $(builddir)/interfaces.txt $(builddir)/logger_lockfile +CLEANFILES = $(builddir)/interfaces.txt $(builddir)/logger_lockfile +CLEANFILES += $(builddir)/load_marker.txt $(builddir)/unload_marker.txt AM_CXXFLAGS = $(B10_CXXFLAGS) if USE_CLANGPP @@ -44,9 +45,18 @@ TESTS_ENVIRONMENT = \ TESTS = if HAVE_GTEST +# Build shared libraries for testing. +lib_LTLIBRARIES = libco1.la libco2.la + +libco1_la_SOURCES = callout_library_1.cc callout_library_common.h +libco1_la_CXXFLAGS = $(AM_CXXFLAGS) +libco1_la_CPPFLAGS = $(AM_CPPFLAGS) + +libco2_la_SOURCES = callout_library_2.cc callout_library_common.h +libco2_la_CXXFLAGS = $(AM_CXXFLAGS) +libco2_la_CPPFLAGS = $(AM_CPPFLAGS) TESTS += dhcp6_unittests - dhcp6_unittests_SOURCES = dhcp6_unittests.cc dhcp6_unittests_SOURCES += dhcp6_srv_unittest.cc dhcp6_unittests_SOURCES += ctrl_dhcp6_srv_unittest.cc @@ -55,7 +65,9 @@ dhcp6_unittests_SOURCES += ../dhcp6_srv.h ../dhcp6_srv.cc dhcp6_unittests_SOURCES += ../dhcp6_log.h ../dhcp6_log.cc dhcp6_unittests_SOURCES += ../ctrl_dhcp6_srv.cc dhcp6_unittests_SOURCES += ../config_parser.cc ../config_parser.h -nodist_dhcp6_unittests_SOURCES = ../dhcp6_messages.h ../dhcp6_messages.cc + +nodist_dhcp6_unittests_SOURCES = ../dhcp6_messages.h ../dhcp6_messages.cc +nodist_dhcp6_unittests_SOURCES += marker_file.h test_libraries.h dhcp6_unittests_CPPFLAGS = $(AM_CPPFLAGS) $(GTEST_INCLUDES) dhcp6_unittests_LDFLAGS = $(AM_LDFLAGS) $(GTEST_LDFLAGS) @@ -68,6 +80,7 @@ dhcp6_unittests_LDADD += $(top_builddir)/src/lib/dhcpsrv/libb10-dhcpsrv.la dhcp6_unittests_LDADD += $(top_builddir)/src/lib/exceptions/libb10-exceptions.la dhcp6_unittests_LDADD += $(top_builddir)/src/lib/log/libb10-log.la dhcp6_unittests_LDADD += $(top_builddir)/src/lib/util/libb10-util.la + endif noinst_PROGRAMS = $(TESTS) diff --git a/src/bin/dhcp6/tests/callout_library_1.cc b/src/bin/dhcp6/tests/callout_library_1.cc new file mode 100644 index 0000000000..471bb6f7fe --- /dev/null +++ b/src/bin/dhcp6/tests/callout_library_1.cc @@ -0,0 +1,22 @@ +// Copyright (C) 2013 Internet Systems Consortium, Inc. ("ISC") +// +// Permission to use, copy, modify, and/or distribute this software for any +// purpose with or without fee is hereby granted, provided that the above +// copyright notice and this permission notice appear in all copies. +// +// THE SOFTWARE IS PROVIDED "AS IS" AND ISC DISCLAIMS ALL WARRANTIES WITH +// REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY +// AND FITNESS. IN NO EVENT SHALL ISC BE LIABLE FOR ANY SPECIAL, DIRECT, +// INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM +// LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE +// OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR +// PERFORMANCE OF THIS SOFTWARE. + +/// @file +/// @brief Marker file callout library +/// +/// This is the source of a test library for the DHCP parser and configuration +/// tests. See callout_common.cc for details. + +static const int LIBRARY_NUMBER = 1; +#include "callout_library_common.h" diff --git a/src/bin/dhcp6/tests/callout_library_2.cc b/src/bin/dhcp6/tests/callout_library_2.cc new file mode 100644 index 0000000000..b0b46379ce --- /dev/null +++ b/src/bin/dhcp6/tests/callout_library_2.cc @@ -0,0 +1,22 @@ +// Copyright (C) 2013 Internet Systems Consortium, Inc. ("ISC") +// +// Permission to use, copy, modify, and/or distribute this software for any +// purpose with or without fee is hereby granted, provided that the above +// copyright notice and this permission notice appear in all copies. +// +// THE SOFTWARE IS PROVIDED "AS IS" AND ISC DISCLAIMS ALL WARRANTIES WITH +// REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY +// AND FITNESS. IN NO EVENT SHALL ISC BE LIABLE FOR ANY SPECIAL, DIRECT, +// INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM +// LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE +// OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR +// PERFORMANCE OF THIS SOFTWARE. + +/// @file +/// @brief Marker file callout library +/// +/// This is the source of a test library for the DHCP parser and configuration +/// tests. See callout_common.cc for details. + +static const int LIBRARY_NUMBER = 2; +#include "callout_library_common.h" diff --git a/src/bin/dhcp6/tests/callout_library_common.h b/src/bin/dhcp6/tests/callout_library_common.h new file mode 100644 index 0000000000..e8d4b5ac60 --- /dev/null +++ b/src/bin/dhcp6/tests/callout_library_common.h @@ -0,0 +1,80 @@ +// Copyright (C) 2013 Internet Systems Consortium, Inc. ("ISC") +// +// Permission to use, copy, modify, and/or distribute this software for any +// purpose with or without fee is hereby granted, provided that the above +// copyright notice and this permission notice appear in all copies. +// +// THE SOFTWARE IS PROVIDED "AS IS" AND ISC DISCLAIMS ALL WARRANTIES WITH +// REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY +// AND FITNESS. IN NO EVENT SHALL ISC BE LIABLE FOR ANY SPECIAL, DIRECT, +// INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM +// LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE +// OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR +// PERFORMANCE OF THIS SOFTWARE. + +/// @file +/// @brief Marker file callout library +/// +/// This is the source of a test library for the DHCP parser and configuration +/// tests. +/// +/// To check that they libraries are loaded and unloaded correctly, the load +/// and unload functions in this library maintain two marker files - the load +/// marker file and the unload marker file. The functions append a single +/// to the single line in the file, creating the file if need be. In +/// this way, the test code can determine whether the load/unload functions +/// have been run and, if so, in what order. +/// +/// This file is the common library file for the tests. It will not compile +/// by itself - it is included into each callout library which specifies the +/// missing constant LIBRARY_NUMBER before the inclusion. + +#include +#include "marker_file.h" + +#include + +using namespace isc::hooks; +using namespace std; + +extern "C" { + +/// @brief Append digit to marker file +/// +/// If the marker file does not exist, create it. Then append the single +/// digit (given by the constant LIBRARY_NUMBER) defined earlier to it and +/// close the file. +/// +/// @param name Name of the file to open +/// +/// @return 0 on success, non-zero on error. +int +appendDigit(const char* name) { + // Open the file and check if successful. + fstream file(name, fstream::out | fstream::app); + if (!file.good()) { + return (1); + } + + // Add the library number to it and close. + file << LIBRARY_NUMBER; + file.close(); + + return (0); +} + +// Framework functions +int +version() { + return (BIND10_HOOKS_VERSION); +} + +int load(LibraryHandle&) { + return (appendDigit(LOAD_MARKER_FILE)); +} + +int unload() { + return (appendDigit(UNLOAD_MARKER_FILE)); +} + +}; diff --git a/src/bin/dhcp6/tests/config_parser_unittest.cc b/src/bin/dhcp6/tests/config_parser_unittest.cc index d9c5859a64..b2da421611 100644 --- a/src/bin/dhcp6/tests/config_parser_unittest.cc +++ b/src/bin/dhcp6/tests/config_parser_unittest.cc @@ -24,15 +24,23 @@ #include #include #include +#include + +#include "test_libraries.h" +#include "marker_file.h" #include #include #include #include +#include #include +#include +#include #include +#include using namespace std; using namespace isc; @@ -40,6 +48,7 @@ using namespace isc::dhcp; using namespace isc::asiolink; using namespace isc::data; using namespace isc::config; +using namespace isc::hooks; namespace { @@ -71,6 +80,10 @@ public: ~Dhcp6ParserTest() { // Reset configuration database after each test. resetConfiguration(); + + // ... and delete the hooks library marker files if present + unlink(LOAD_MARKER_FILE); + unlink(UNLOAD_MARKER_FILE); }; // Checks if config_result (result of DHCP server configuration) has @@ -169,17 +182,66 @@ public: return (stream.str()); } + /// @brief Parse configuration + /// + /// Parses a configuration and executes a configuration of the server. + /// If the operation fails, the current test will register a failure. + /// + /// @param config Configuration to parse + /// @param operation Operation being performed. In the case of an error, + /// the error text will include the string "unable to .". + /// + /// @return true if the configuration succeeded, false if not. In the + /// latter case, a failure will have been added to the current test. + bool + executeConfiguration(const std::string& config, const char* operation) { + ConstElementPtr status; + try { + ElementPtr json = Element::fromJSON(config); + status = configureDhcp6Server(srv_, json); + } catch (const std::exception& ex) { + ADD_FAILURE() << "Unable to " << operation << ". " + << "The following configuration was used: " << std::endl + << config << std::endl + << " and the following error message was returned:" + << ex.what() << std::endl; + return (false); + } + + // The status object must not be NULL + if (!status) { + ADD_FAILURE() << "Unable to " << operation << ". " + << "The configuration function returned a null pointer."; + return (false); + } + + // Store the answer if we need it. + + // Returned value should be 0 (configuration success) + comment_ = parseAnswer(rcode_, status); + if (rcode_ != 0) { + string reason = ""; + if (comment_) { + reason = string(" (") + comment_->stringValue() + string(")"); + } + ADD_FAILURE() << "Unable to " << operation << ". " + << "The configuration function returned error code " + << rcode_ << reason; + return (false); + } + + return (true); + } + /// @brief Reset configuration database. /// - /// This function resets configuration data base by - /// removing all subnets and option-data. Reset must - /// be performed after each test to make sure that - /// contents of the database do not affect result of - /// subsequent tests. + /// This function resets configuration data base by removing all subnets + /// option-data, and hooks libraries. The reset must be performed after each + /// test to make sure that contents of the database do not affect the + /// results of subsequent tests. void resetConfiguration() { - ConstElementPtr status; - string config = "{ \"interface\": [ \"all\" ]," + "\"hooks-libraries\": [ ]," "\"preferred-lifetime\": 3000," "\"rebind-timer\": 2000, " "\"renew-timer\": 1000, " @@ -187,32 +249,8 @@ public: "\"subnet6\": [ ], " "\"option-def\": [ ], " "\"option-data\": [ ] }"; - - try { - ElementPtr json = Element::fromJSON(config); - status = configureDhcp6Server(srv_, json); - } catch (const std::exception& ex) { - FAIL() << "Fatal error: unable to reset configuration database" - << " after the test. The following configuration was used" - << " to reset database: " << std::endl - << config << std::endl - << " and the following error message was returned:" - << ex.what() << std::endl; - } - - // status object must not be NULL - if (!status) { - FAIL() << "Fatal error: unable to reset configuration database" - << " after the test. Configuration function returned" - << " NULL pointer" << std::endl; - } - comment_ = parseAnswer(rcode_, status); - // returned value should be 0 (configuration success) - if (rcode_ != 0) { - FAIL() << "Fatal error: unable to reset configuration database" - << " after the test. Configuration function returned" - << " error code " << rcode_ << std::endl; - } + static_cast(executeConfiguration(config, + "reset configuration database")); } /// @brief Test invalid option parameter value. @@ -277,13 +315,66 @@ public: expected_data_len)); } - int rcode_; ///< return core (see @ref isc::config::parseAnswer) - Dhcpv6Srv srv_; ///< instance of the Dhcp6Srv used during tests + /// @brief Check marker file + /// + /// Marker files are used by the load/unload functions in the hooks + /// libraries in these tests to signal whether they have been loaded or + /// unloaded. The file (if present) contains a single line holding + /// a set of characters. + /// + /// This convenience function checks the file to see if the characters + /// are those expected. + /// + /// @param name Name of the marker file. + /// @param expected Characters expected. If a marker file is present, + /// it is expected to contain characters. Therefore a value of NULL + /// is used to signify that the marker file is not expected to be + /// present. + /// + /// @return true if all tests pass, false if not (in which case a failure + /// will have been logged). + bool + checkMarkerFile(const char* name, const char* expected) { + // Open the file for input + fstream file(name, fstream::in); - ConstElementPtr comment_; ///< comment (see @ref isc::config::parseAnswer) + // Is it open? + if (!file.is_open()) { - string valid_iface_; ///< name of a valid network interface (present in system) - string bogus_iface_; ///< name of a invalid network interface (not present in system) + // No. This is OK if we don't expected is to be present but is + // a failure otherwise. + if (expected == NULL) { + return (true); + } + ADD_FAILURE() << "Unable to open " << name << ". It was expected " + << "to be present and to contain the string '" + << expected << "'"; + return (false); + } else if (expected == NULL) { + + // File is open but we don't expect it to be present. + ADD_FAILURE() << "Opened " << name << " but it is not expected to " + << "be present."; + return (false); + } + + // OK, is open, so read the data and see what we have. Compare it + // against what is expected. + string content; + getline(file, content); + + string expected_str(expected); + EXPECT_EQ(expected_str, content) << "Data was read from " << name; + file.close(); + + return (expected_str == content); + } + + int rcode_; ///< Return code (see @ref isc::config::parseAnswer) + Dhcpv6Srv srv_; ///< Instance of the Dhcp6Srv used during tests + ConstElementPtr comment_; ///< Comment (see @ref isc::config::parseAnswer) + string valid_iface_; ///< Valid network interface name (present in system) + string bogus_iface_; ///< invalid network interface name (not in system) }; // Goal of this test is a verification if a very simple config update @@ -1850,4 +1941,160 @@ TEST_F(Dhcp6ParserTest, stdOptionDataEncapsulate) { EXPECT_FALSE(desc.option->getOption(112)); } +// Tests of the hooks libraries configuration. + +// Helper function to return a configuration containing an arbitrary number +// of hooks libraries. +std::string +buildHooksLibrariesConfig(const std::vector& libraries) { + const string quote("\""); + + // Create the first part of the configuration string. + string config = + "{ \"interface\": [ \"all\" ]," + "\"hooks-libraries\": ["; + + // Append the libraries (separated by commas if needed) + for (int i = 0; i < libraries.size(); ++i) { + if (i > 0) { + config += string(", "); + } + config += (quote + libraries[i] + quote); + } + + // Append the remainder of the configuration. + config += string( + "]," + "\"rebind-timer\": 2000," + "\"renew-timer\": 1000," + "\"option-data\": [ {" + " \"name\": \"foo\"," + " \"space\": \"vendor-opts-space\"," + " \"code\": 110," + " \"data\": \"1234\"," + " \"csv-format\": True" + " }," + " {" + " \"name\": \"foo2\"," + " \"space\": \"vendor-opts-space\"," + " \"code\": 111," + " \"data\": \"192.168.2.1\"," + " \"csv-format\": True" + " } ]," + "\"option-def\": [ {" + " \"name\": \"foo\"," + " \"code\": 110," + " \"type\": \"uint32\"," + " \"array\": False," + " \"record-types\": \"\"," + " \"space\": \"vendor-opts-space\"," + " \"encapsulate\": \"\"" + " }," + " {" + " \"name\": \"foo2\"," + " \"code\": 111," + " \"type\": \"ipv4-address\"," + " \"array\": False," + " \"record-types\": \"\"," + " \"space\": \"vendor-opts-space\"," + " \"encapsulate\": \"\"" + " } ]" + "}"); + + return (config); +} + +// Convenience function for creating hooks library configuration with one or +// two character string constants. +std::string +buildHooksLibrariesConfig(const char* library1 = NULL, + const char* library2 = NULL) { + std::vector libraries; + if (library1 != NULL) { + libraries.push_back(string(library1)); + if (library2 != NULL) { + libraries.push_back(string(library2)); + } + } + return (buildHooksLibrariesConfig(libraries)); +} + + +// The goal of this test is to verify the configuration of hooks libraries if +// none are specified. +TEST_F(Dhcp6ParserTest, NoHooksLibraries) { +// std::vector libraries = HooksManager::getLibraryNames(); +// ASSERT_TRUE(libraries.empty()); + + // Parse a configuration containing no names. + string config = buildHooksLibrariesConfig(); + if (!executeConfiguration(config, + "set configuration with no hooks libraries")) { + return; + } +// libraries = HooksManager::getLibraryNames(); +// ASSERT_TRUE(libraries.empty()); +} + +// Verify parsing fails with one library that will fail validation. +TEST_F(Dhcp6ParserTest, InvalidLibrary) { +// std::vector libraries = HooksManager::getLibraryNames(); +// ASSERT_TRUE(libraries.empty()); + + // Parse a configuration containing a failing library. + string config = buildHooksLibrariesConfig(NOT_PRESENT_LIBRARY); + + ConstElementPtr status; + ElementPtr json = Element::fromJSON(config); + ASSERT_NO_THROW(status = configureDhcp6Server(srv_, json)); + + // The status object must not be NULL + ASSERT_FALSE(status); + + // Returned value should not be 0 + comment_ = parseAnswer(rcode_, status); + EXPECT_NE(0, rcode_); + std::cerr << "Reason for success: " << comment_; +} + +// Verify the configuration of hooks libraries with two being specified. +TEST_F(Dhcp6ParserTest, LibrariesSpecified) { +// std::vector libraries = HooksManager::getLibraryNames(); +// ASSERT_TRUE(libraries.empty()); + + // Marker files should not be present. + EXPECT_TRUE(checkMarkerFile(LOAD_MARKER_FILE, NULL)); + EXPECT_TRUE(checkMarkerFile(UNLOAD_MARKER_FILE, NULL)); + + // Set up the configuration with two libraries and load them. + string config = buildHooksLibrariesConfig(CALLOUT_LIBRARY_1, + CALLOUT_LIBRARY_2); + ASSERT_TRUE(executeConfiguration(config, + "loading two valid libraries")); + + // Expect two libraries to be loaded in the correct order (load marker file + // is present, no unload marker file). +// std::vector libraries = HooksManager::getLibraryNames(); +// ASSERT_EQ(2, libraries.size()); + EXPECT_TRUE(checkMarkerFile(LOAD_MARKER_FILE, "12")); + EXPECT_TRUE(checkMarkerFile(UNLOAD_MARKER_FILE, NULL)); + + // Unload the libraries. The load file should not have changed, but + // the unload one should indicate the unload() functions have been run. + config = buildHooksLibrariesConfig(); + ASSERT_TRUE(executeConfiguration(config, "unloading libraries")); + EXPECT_TRUE(checkMarkerFile(LOAD_MARKER_FILE, "12")); + EXPECT_TRUE(checkMarkerFile(UNLOAD_MARKER_FILE, "21")); + + // Expect the hooks system to say that none are loaded. + // libraries = HooksManager::getLibraryNames(); + // EXPECT_TRUE(libraries.empty()); + + } +// libraries = HooksManager::getLibraryNames(); +// ASSERT_TRUE(libraries.empty()); + + + + }; diff --git a/src/bin/dhcp6/tests/marker_file.h.in b/src/bin/dhcp6/tests/marker_file.h.in new file mode 100644 index 0000000000..11b98ee10b --- /dev/null +++ b/src/bin/dhcp6/tests/marker_file.h.in @@ -0,0 +1,28 @@ +// Copyright (C) 2013 Internet Systems Consortium, Inc. ("ISC") +// +// Permission to use, copy, modify, and/or distribute this software for any +// purpose with or without fee is hereby granted, provided that the above +// copyright notice and this permission notice appear in all copies. +// +// THE SOFTWARE IS PROVIDED "AS IS" AND ISC DISCLAIMS ALL WARRANTIES WITH +// REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY +// AND FITNESS. IN NO EVENT SHALL ISC BE LIABLE FOR ANY SPECIAL, DIRECT, +// INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM +// LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE +// OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR +// PERFORMANCE OF THIS SOFTWARE. + +#ifndef MARKER_FILE_H +#define MARKER_FILE_H + +/// @file +/// Define a marker file that is used in tests to prove that an "unload" +/// function has been called. + +namespace { +const char* LOAD_MARKER_FILE = "@abs_builddir@/load_marker.txt"; +const char* UNLOAD_MARKER_FILE = "@abs_builddir@/unload_marker.txt"; +} + +#endif // MARKER_FILE_H + diff --git a/src/bin/dhcp6/tests/test_libraries.h.in b/src/bin/dhcp6/tests/test_libraries.h.in new file mode 100644 index 0000000000..b5e80a04f7 --- /dev/null +++ b/src/bin/dhcp6/tests/test_libraries.h.in @@ -0,0 +1,51 @@ +// Copyright (C) 2013 Internet Systems Consortium, Inc. ("ISC") +// +// Permission to use, copy, modify, and/or distribute this software for any +// purpose with or without fee is hereby granted, provided that the above +// copyright notice and this permission notice appear in all copies. +// +// THE SOFTWARE IS PROVIDED "AS IS" AND ISC DISCLAIMS ALL WARRANTIES WITH +// REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY +// AND FITNESS. IN NO EVENT SHALL ISC BE LIABLE FOR ANY SPECIAL, DIRECT, +// INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM +// LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE +// OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR +// PERFORMANCE OF THIS SOFTWARE. + +#ifndef TEST_LIBRARIES_H +#define TEST_LIBRARIES_H + +#include + +namespace { + + +// Take care of differences in DLL naming between operating systems. + +#ifdef OS_OSX +#define DLL_SUFFIX ".dylib" + +#else +#define DLL_SUFFIX ".so" + +#endif + + +// Names of the libraries used in these tests. These libraries are built using +// libtool, so we need to look in the hidden ".libs" directory to locate the +// shared library. + +// Library with load/unload functions creating marker files to check their +// operation. +static const char* CALLOUT_LIBRARY_1 = "@abs_builddir@/.libs/libco1" + DLL_SUFFIX; +static const char* CALLOUT_LIBRARY_2 = "@abs_builddir@/.libs/libco2" + DLL_SUFFIX; + +// Name of a library which is not present. +static const char* NOT_PRESENT_LIBRARY = "@abs_builddir@/.libs/libnothere" + DLL_SUFFIX; +} // anonymous namespace + + +#endif // TEST_LIBRARIES_H diff --git a/src/lib/dhcpsrv/Makefile.am b/src/lib/dhcpsrv/Makefile.am index 29e8c2f2cc..e1680ac2a3 100644 --- a/src/lib/dhcpsrv/Makefile.am +++ b/src/lib/dhcpsrv/Makefile.am @@ -59,6 +59,8 @@ libb10_dhcpsrv_la_CXXFLAGS = $(AM_CXXFLAGS) libb10_dhcpsrv_la_CPPFLAGS = $(AM_CPPFLAGS) $(LOG4CPLUS_INCLUDES) libb10_dhcpsrv_la_LIBADD = $(top_builddir)/src/lib/asiolink/libb10-asiolink.la libb10_dhcpsrv_la_LIBADD += $(top_builddir)/src/lib/dhcp/libb10-dhcp++.la +libb10_dhcpsrv_la_LIBADD += $(top_builddir)/src/lib/hooks/libb10-hooks.la +libb10_dhcpsrv_la_LIBADD += $(top_builddir)/src/lib/log/libb10-log.la libb10_dhcpsrv_la_LIBADD += $(top_builddir)/src/lib/util/libb10-util.la libb10_dhcpsrv_la_LIBADD += $(top_builddir)/src/lib/cc/libb10-cc.la libb10_dhcpsrv_la_LDFLAGS = -no-undefined -version-info 3:0:0 diff --git a/src/lib/dhcpsrv/cfgmgr.cc b/src/lib/dhcpsrv/cfgmgr.cc index b38ff3cc26..592efb7942 100644 --- a/src/lib/dhcpsrv/cfgmgr.cc +++ b/src/lib/dhcpsrv/cfgmgr.cc @@ -266,24 +266,6 @@ std::string CfgMgr::getDataDir() { return (datadir_); } -void -CfgMgr::setHooksLibraries(const std::vector& hooks_libraries) { - hooks_libraries_.reset(new std::vector(hooks_libraries)); -} - -boost::shared_ptr > -CfgMgr::getAndClearHooksLibraries() { - // Create shared pointer pointing to nothing. - boost::shared_ptr > libraries; - - // Set the new variable to point to the stored hooks libraries and clear - // the stored value. - libraries.swap(hooks_libraries_); - - return (libraries); -} - - CfgMgr::CfgMgr() :datadir_(DHCP_DATA_DIR) { diff --git a/src/lib/dhcpsrv/cfgmgr.h b/src/lib/dhcpsrv/cfgmgr.h index 6da9f20bb3..05c1752944 100644 --- a/src/lib/dhcpsrv/cfgmgr.h +++ b/src/lib/dhcpsrv/cfgmgr.h @@ -38,21 +38,10 @@ namespace dhcp { /// @brief Configuration Manager /// /// This singleton class holds the whole configuration for DHCPv4 and DHCPv6 -/// servers. It currently holds information about: -/// - Zero or more subnets. Each subnet may contain zero or more pools. Pool4 -/// and Pool6 is the most basic "chunk" of configuration. It contains a range -/// of assignable addresses. -/// - Hook libraries. Hooks library names are stored here, but only up to the -/// point that the libraries are reloaded. In more detail: libraries -/// containing hooks callouts can only be loaded and reloaded safely (or, more -/// accurately, unloaded safely) when no data structure in the server contains -/// a reference to any memory allocated by a function in them. In practice -/// this means when there are no packets being activly processed. Rather than -/// take a chance that the configuration code will do the unload/load at the -/// right time, the configuration code sets the names of the new libraries in -/// this object and the server decides when to reconfigure the hooks. The -/// presence or absence of the names of the hooks libraries here is an -/// indication of whether the libraries should be reloaded. +/// servers. It currently holds information about zero or more subnets6. +/// Each subnet may contain zero or more pools. Pool4 and Pool6 is the most +/// basic "chunk" of configuration. It contains a range of assignable +/// addresses. /// /// Below is a sketch of configuration inheritance (not implemented yet). /// Let's investigate the following configuration: @@ -79,7 +68,6 @@ namespace dhcp { /// routines, so there is no storage capability in a global scope for /// subnet-specific parameters. /// -/// ARE THESE DONE? /// @todo: Implement Subnet4 support (ticket #2237) /// @todo: Implement option definition support /// @todo: Implement parameter inheritance @@ -241,6 +229,7 @@ public: /// completely new? void deleteSubnets4(); + /// @brief returns path do the data directory /// /// This method returns a path to writeable directory that DHCP servers @@ -248,25 +237,6 @@ public: /// @return data directory std::string getDataDir(); - /// @brief Sets list of hooks libraries - /// - /// Sets the list of hooks libraries. It is possible for there to be no - /// hooks libraries, in which case this is indicated by an emopty vector. - /// - /// @param hooks_libraries Vector (possibly empty) of current hook libraries. - void setHooksLibraries(const std::vector& hooks_libraries); - - /// @brief Get and clear list of hooks libraries - /// - /// Gets the currently-set vector of hooks libraries. If there is no data - /// (as opposed to an empty vector), there has been no change to the data - /// since the last time this method was called. Should there be a necessity - /// to know this information, it can be obtained from the HooksManager. - /// - /// @return Pointer to vector of strings listing the hooks libraries. This - /// may be empty. - boost::shared_ptr > getAndClearHooksLibraries(); - protected: /// @brief Protected constructor. @@ -313,28 +283,6 @@ private: /// @brief directory where data files (e.g. server-id) are stored std::string datadir_; - - /// @brief Hooks libraries - /// - /// Unlike other configuration items that can be referenced all the time, - /// this is a "one-shot" item. When the list of libraries is altered, the - /// server needs to know about the change: once the libraries are loaded, - /// the list is ignored. As configuration updates cause an update of the - /// entire configuration and we wish to reload the libraries only if the - /// list has changed, we could check the library list against that stored - /// in the hooks manager. Unfortunately, since the libraries are reloaded - /// when a new packet is received, this would mean a set of string - /// comparisons on each packet. Instead, the data is flagged to indicate - /// that it has changed. - /// - /// The parsing checks the set of hooks libraries in the configuration - /// against the list stored in the HooksManager and only updates the data - /// here if they have changed. Although a flag could be used to indicate - /// a change, a more streamlined approach is used: the data in this object - /// is cleared when it is read. As the data is a vector of strings and as - /// an empty vector is valid data, we'll store the data as a shared pointer - /// to a vector of strings. The pointer is zeroed when the data is read. - boost::shared_ptr > hooks_libraries_; }; } // namespace isc::dhcp diff --git a/src/lib/dhcpsrv/dhcp_parsers.cc b/src/lib/dhcpsrv/dhcp_parsers.cc index 224546d9cc..bd0311103f 100644 --- a/src/lib/dhcpsrv/dhcp_parsers.cc +++ b/src/lib/dhcpsrv/dhcp_parsers.cc @@ -16,19 +16,22 @@ #include #include #include +#include #include #include +#include #include #include -#include #include -#include #include +#include +#include using namespace std; using namespace isc::data; +using namespace isc::hooks; namespace isc { namespace dhcp { @@ -41,7 +44,6 @@ ParserContext::ParserContext(Option::Universe universe): string_values_(new StringStorage()), options_(new OptionStorage()), option_defs_(new OptionDefStorage()), - hooks_libraries_(new HooksLibrariesStorage()), universe_(universe) { } @@ -51,7 +53,6 @@ ParserContext::ParserContext(const ParserContext& rhs): string_values_(new StringStorage(*(rhs.string_values_))), options_(new OptionStorage(*(rhs.options_))), option_defs_(new OptionDefStorage(*(rhs.option_defs_))), - hooks_libraries_(new HooksLibrariesStorage(*(rhs.hooks_libraries_))), universe_(rhs.universe_) { } @@ -67,9 +68,6 @@ ParserContext::operator=(const ParserContext& rhs) { options_ = OptionStoragePtr(new OptionStorage(*(rhs.options_))); option_defs_ = OptionDefStoragePtr(new OptionDefStorage(*(rhs.option_defs_))); - hooks_libraries_ = - HooksLibrariesStoragePtr(new HooksLibrariesStorage( - *(rhs.hooks_libraries_))); universe_ = rhs.universe_; } return (*this); @@ -168,12 +166,11 @@ InterfaceListConfigParser::commit() { // ******************** HooksLibrariesParser ************************* -HooksLibrariesParser::HooksLibrariesParser(const std::string& param_name, - ParserContextPtr global_context) - : libraries_(), global_context_(global_context) { - - // SanitY check on the name. - if (param_name != "hooks_libraries") { +HooksLibrariesParser::HooksLibrariesParser(const std::string& param_name) + : libraries_(), changed_(false) +{ + // Sanity check on the name. + if (param_name != "hooks-libraries") { isc_throw(BadValue, "Internal error. Hooks libraries " "parser called for the wrong parameter: " << param_name); } @@ -181,46 +178,54 @@ HooksLibrariesParser::HooksLibrariesParser(const std::string& param_name, void HooksLibrariesParser::build(ConstElementPtr value) { + // Initialize. + libraries_.clear(); + changed_ = false; - /// Extract the list of libraries. - HooksLibrariesStoragePtr libraries(new HooksLibrariesStorage()); + // Extract the list of libraries. BOOST_FOREACH(ConstElementPtr iface, value->listValue()) { string libname = iface->str(); boost::erase_all(libname, "\""); - libraries->push_back(libname); + libraries_.push_back(libname); } - /// @todo A two-stage process. The first stage checks if the libraries - /// element has changed. If not, nothing is done - the command - /// "DhcpN reload_hooks" is required to reload the same libraries (this - /// prevents needless reloads when anything in the configuration is - /// changed). - /// - /// If the libraries have changed, the next step is to validate each of the - /// libraries. This should be a method on HooksManager which should create - /// a LibraryManager for it and call a new method "validateLibrary()". - /// That method will open a library (verifying that it exists) and check - /// version() (both that it exists and returned the right value). If these - /// checks succeed, it is considered a success. The library is closed when - /// the LibraryManager is deleted. - /// @TODO Validate the library list + // 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). +/* + vector current_libraries = HooksManager::getLibraryNames(); + if (current_libraries == libraries_) { + return; + } - /// The library list has changed, so store the new list. (This clears the - /// local pointer libraries as a side-effect, but as that is being - /// destroyed on exit, it is not an issue). - libraries_.swap(libraries); + // Library list has changed, validate each of the libraries specified. + string error_libs = HooksManager::validateLibraries(libraries_); + if (!error_libs.empty()) { + isc_throw(DhcpConfigError, "hooks libraries failed to validate - " + "library or libraries in error are: " + error_libs); + } +*/ + // The library list has changed and the libraries are valid, so flag for + // update when commit() is called. + changed_ = true; } void HooksLibrariesParser::commit() { - /// Commits the list of libraries to the configuration manager storage. - /// Note that the list stored here could be empty, which will signify - /// no change. - /// - /// We use "swap" to reduce overhead - as this parser is being destroyed - /// after the commit, there is no reason to retain a pointer to the hooks - /// library data in it. - global_context_->hooks_libraries_.swap(libraries_); + /// Commits the list of libraries to the configuration manager storage if + /// the list of libraries has changed. + if (changed_) { + HooksManager::loadLibraries(libraries_); + } +} + +// Method for testing +void +HooksLibrariesParser::getLibraries(std::vector& libraries, + bool& changed) { + libraries = libraries_; + changed = changed_; } // **************************** OptionDataParser ************************* diff --git a/src/lib/dhcpsrv/dhcp_parsers.h b/src/lib/dhcpsrv/dhcp_parsers.h index 66e0983060..4e339a50ac 100644 --- a/src/lib/dhcpsrv/dhcp_parsers.h +++ b/src/lib/dhcpsrv/dhcp_parsers.h @@ -23,6 +23,8 @@ #include #include +#include + #include #include #include @@ -43,11 +45,6 @@ typedef OptionSpaceContainer OptionStoragePtr; -/// @brief Storage for hooks libraries -typedef std::vector HooksLibrariesStorage; -/// @brief Pointer to storage for hooks libraries -typedef boost::shared_ptr HooksLibrariesStoragePtr; - /// @brief A template class that stores named elements of a given data type. @@ -169,13 +166,7 @@ public: /// the list of current names can be obtained from the HooksManager) or it /// is non-null (this is the new list of names, reload the libraries when /// possible). - /// - /// The same applies to the parser context. The pointer is null (which - /// means that the isc::dhcp::HooksLibrariesParser::build method has - /// compared the library list in the configuration against the library - /// list in the HooksManager and has found no change) or it is non-null - /// (in which case this is the list of new library names). - HooksLibrariesStoragePtr hooks_libraries_; + boost::shared_ptr > hooks_libraries_; /// @brief The parsing universe of this context. Option::Universe universe_; @@ -336,39 +327,71 @@ private: /// /// 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 constly in +/// terms of time and resources, or may cause side effects such as clearning +/// 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. class HooksLibrariesParser : public DhcpConfigParser { public: - /// @brief constructor + /// @brief Constructor /// /// As this is a dedicated parser, it must be used to parse /// "hooks_libraries" parameter only. All other types will throw exception. /// /// @param param_name name of the configuration parameter being parsed. - /// @param GERBIL + /// /// @throw BadValue if supplied parameter name is not "hooks_libraries" - HooksLibrariesParser(const std::string& param_name, - ParserContextPtr global_context); + HooksLibrariesParser(const std::string& param_name); - /// @brief parses parameters value + /// @brief Parses parameters value /// /// Parses configuration entry (list of parameters) and adds each element - /// to the hooks libraries list. + /// to the hooks libraries list. The method also checks whether the + /// list of libraries is the same as that already loaded. If not, it + /// checks each of the libraries in the list for validity (they exist and + /// have a "version" function that returns the correct value). /// /// @param value pointer to the content of parsed values virtual void build(isc::data::ConstElementPtr value); - /// @brief commits hooks libraries data + /// @brief Commits hooks libraries data + /// + /// 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). virtual void commit(); -private: - /// List of hooks libraries. This will be NULL if there is no change to - /// the list. - HooksLibrariesStoragePtr libraries_; + /// @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 libraries (out) List of libraries that were specified in the + /// new configuration. + /// @param changed (out) true if the list is different from that currently + /// loaded. + void getLibraries(std::vector& libraries, bool& changed); - /// Parsing context which contains global values, options and option - /// definitions. - ParserContextPtr global_context_; +private: + /// List of hooks libraries. + std::vector libraries_; + + /// Indicator flagging that the list of libraries has changed. + bool changed_; }; /// @brief Parser for option data value. diff --git a/src/lib/dhcpsrv/tests/Makefile.am b/src/lib/dhcpsrv/tests/Makefile.am index 9a81ef6820..ba1e39bd65 100644 --- a/src/lib/dhcpsrv/tests/Makefile.am +++ b/src/lib/dhcpsrv/tests/Makefile.am @@ -67,8 +67,9 @@ libdhcpsrv_unittests_LDADD += $(top_builddir)/src/lib/dhcp/libb10-dhcp++.la libdhcpsrv_unittests_LDADD += $(top_builddir)/src/lib/config/libb10-cfgclient.la libdhcpsrv_unittests_LDADD += $(top_builddir)/src/lib/cc/libb10-cc.la libdhcpsrv_unittests_LDADD += $(top_builddir)/src/lib/asiolink/libb10-asiolink.la -libdhcpsrv_unittests_LDADD += $(top_builddir)/src/lib/exceptions/libb10-exceptions.la +libdhcpsrv_unittests_LDADD += $(top_builddir)/src/lib/hooks/libb10-hooks.la libdhcpsrv_unittests_LDADD += $(top_builddir)/src/lib/log/libb10-log.la +libdhcpsrv_unittests_LDADD += $(top_builddir)/src/lib/exceptions/libb10-exceptions.la libdhcpsrv_unittests_LDADD += $(GTEST_LDADD) endif diff --git a/src/lib/dhcpsrv/tests/cfgmgr_unittest.cc b/src/lib/dhcpsrv/tests/cfgmgr_unittest.cc index acf23ab206..77c3e36775 100644 --- a/src/lib/dhcpsrv/tests/cfgmgr_unittest.cc +++ b/src/lib/dhcpsrv/tests/cfgmgr_unittest.cc @@ -165,7 +165,6 @@ public: CfgMgr::instance().deleteSubnets4(); CfgMgr::instance().deleteSubnets6(); CfgMgr::instance().deleteOptionDefs(); - static_cast(CfgMgr::instance().getAndClearHooksLibraries()); } /// @brief generates interface-id option based on provided text @@ -183,7 +182,6 @@ public: CfgMgr::instance().deleteSubnets4(); CfgMgr::instance().deleteSubnets6(); CfgMgr::instance().deleteOptionDefs(); - static_cast(CfgMgr::instance().getAndClearHooksLibraries()); } }; @@ -579,35 +577,4 @@ TEST_F(CfgMgrTest, optionSpace6) { // in Dhcpv6SrvTest.selectSubnetAddr and Dhcpv6SrvTest.selectSubnetIface // (see src/bin/dhcp6/tests/dhcp6_srv_unittest.cc) -// Checks that the hooks libraries can be set correctly. -TEST_F(CfgMgrTest, hooksLibraries) { - std::vector test_libraries; - test_libraries.push_back("/usr/lib/alpha.so"); - test_libraries.push_back("/usr/lib/beta.so"); - - std::vector no_libraries; - - // The pointer should be empty initially. - boost::shared_ptr > config_libraries = - CfgMgr::instance().getAndClearHooksLibraries(); - EXPECT_FALSE(config_libraries); - - // Set the new set of libraries and get them. - CfgMgr::instance().setHooksLibraries(test_libraries); - config_libraries = CfgMgr::instance().getAndClearHooksLibraries(); - ASSERT_TRUE(config_libraries); - EXPECT_TRUE(test_libraries == *config_libraries); - - // Expect the get operation to have cleared the stored libraries. - config_libraries = CfgMgr::instance().getAndClearHooksLibraries(); - EXPECT_FALSE(config_libraries); - - // Check that the methods also work with an empty library vector. - CfgMgr::instance().setHooksLibraries(no_libraries); - config_libraries = CfgMgr::instance().getAndClearHooksLibraries(); - ASSERT_TRUE(config_libraries); - EXPECT_TRUE(config_libraries->empty()); -} - - } // end of anonymous namespace diff --git a/src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc b/src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc index ef47080eca..5e935a289b 100644 --- a/src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc +++ b/src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc @@ -22,18 +22,21 @@ #include #include #include +#include #include #include +#include #include #include using namespace std; using namespace isc; -using namespace isc::dhcp; -using namespace isc::data; using namespace isc::config; +using namespace isc::data; +using namespace isc::dhcp; +using namespace isc::hooks; namespace { @@ -277,7 +280,7 @@ public: ConfigPair config_pair; try { - // Iteraate over the config elements. + // Iterate over the config elements. const std::map& values_map = config_set->mapValue(); BOOST_FOREACH(config_pair, values_map) { @@ -317,24 +320,34 @@ public: /// @brief Create an element parser based on the element name. /// - /// Note that currently it only supports option-defs and option-data, + /// Creates a parser for the appropriate element and stores a pointer to it + /// in the appropriate class variable. + /// + /// Note that the method currently it only supports option-defs, option-data + /// and hooks-libraries. /// /// @param config_id is the name of the configuration element. - /// @return returns a raw pointer to DhcpConfigParser. Note caller is - /// responsible for deleting it once no longer needed. + /// + /// @return returns a shared pointer to DhcpConfigParser. + /// /// @throw throws NotImplemented if element name isn't supported. - DhcpConfigParser* createConfigParser(const std::string& config_id) { - DhcpConfigParser* parser = NULL; + ParserPtr createConfigParser(const std::string& config_id) { + ParserPtr parser; if (config_id.compare("option-data") == 0) { - parser = new OptionDataListParser(config_id, - parser_context_->options_, - parser_context_, - UtestOptionDataParser::factory); + parser.reset(new OptionDataListParser(config_id, + parser_context_->options_, + parser_context_, + UtestOptionDataParser::factory)); + } else if (config_id.compare("option-def") == 0) { - parser = new OptionDefListParser(config_id, - parser_context_->option_defs_); - } else if (config_id.compare("hooks_libraries") == 0) { - parser = new HooksLibrariesParser(config_id, parser_context_); + parser.reset(new OptionDefListParser(config_id, + parser_context_->option_defs_)); + + } else if (config_id.compare("hooks-libraries") == 0) { + parser.reset(new HooksLibrariesParser(config_id)); + hooks_libraries_parser_ = + boost::dynamic_pointer_cast(parser); + } else { isc_throw(NotImplemented, "Parser error: configuration parameter not supported: " @@ -344,7 +357,7 @@ public: return (parser); } - /// @brief Convenicee method for parsing a configuration + /// @brief Convenience method for parsing a configuration /// /// Given a configuration string, convert it into Elements /// and parse them. @@ -360,7 +373,8 @@ public: EXPECT_TRUE(json); if (json) { ConstElementPtr status = parseElementSet(json); - ConstElementPtr comment_ = parseAnswer(rcode_, status); + ConstElementPtr comment = parseAnswer(rcode_, status); + error_text_ = comment->stringValue(); } return (rcode_); @@ -424,14 +438,6 @@ public: return (option_ptr); } - /// @brief Returns hooks libraries from the parser context - /// - /// Returns the pointer to the vector of strings from the parser context. - HooksLibrariesStoragePtr getHooksLibrariesPtr() { - return (parser_context_->hooks_libraries_); - } - - /// @brief Wipes the contents of the context to allowing another parsing /// during a given test if needed. void reset_context(){ @@ -440,10 +446,21 @@ public: CfgMgr::instance().deleteSubnets6(); CfgMgr::instance().deleteOptionDefs(); parser_context_.reset(new ParserContext(Option::V6)); + + // Ensure no hooks libraries are loaded. + HooksManager::unloadLibraries(); } + /// @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_; + /// @brief Parser context - provides storage for options and definitions ParserContextPtr parser_context_; + + /// @brief Error string if the parsing failed + std::string error_text_; }; /// @brief Check Basic parsing of option definitions. @@ -471,6 +488,7 @@ TEST_F(ParseConfigTest, basicOptionDefTest) { int rcode = parseConfiguration(config); ASSERT_TRUE(rcode == 0); + // Verify that the option definition can be retrieved. OptionDefinitionPtr def = getOptionDef("isc", 100); ASSERT_TRUE(def); @@ -528,41 +546,58 @@ TEST_F(ParseConfigTest, basicOptionDataTest) { }; // Anonymous namespace -/// @brief Check Basic parsing of hooks libraries -/// /// These tests check basic operation of the HooksLibrariesParser. -TEST_F(ParseConfigTest, emptyHooksLibrariesTest) { - // @todo Initialize global library context to null +// hooks-libraries that do not contain anything. +TEST_F(ParseConfigTest, noHooksLibrariesTest) { - // Configuration string. This contains a valid library. - const std::string config = - "{ \"hooks_libraries\": [ " - " ]" - "}"; + // Configuration with hooks-libraries not present. + string config = "{ \"hooks-libraries\": [] }"; // Verify that the configuration string parses. int rcode = parseConfiguration(config); - ASSERT_TRUE(rcode == 0); + ASSERT_TRUE(rcode == 0) << error_text_; + + // Check that the parser recorded no change to the current state + // (as the test starts with no hooks libraries loaded). + std::vector libraries; + bool changed; + hooks_libraries_parser_->getLibraries(libraries, changed); + EXPECT_TRUE(libraries.empty()); + EXPECT_FALSE(changed); + + // Load a single library and repeat the parse. + vector basic_library; + basic_library.push_back(string(BASIC_CALLOUT_LIBRARY)); + HooksManager::loadLibraries(basic_library); + + rcode = parseConfiguration(config); + ASSERT_TRUE(rcode == 0) << error_text_; + + // This time the change should have been recorded. + hooks_libraries_parser_->getLibraries(libraries, changed); + EXPECT_TRUE(libraries.empty()); + EXPECT_TRUE(changed); + + // But repeating it again and we are back to no change. + rcode = parseConfiguration(config); + ASSERT_TRUE(rcode == 0) << error_text_; + hooks_libraries_parser_->getLibraries(libraries, changed); + EXPECT_TRUE(libraries.empty()); + EXPECT_FALSE(changed); - // @todo modify after the hooks check has been added. At the moment, the - // string should parse to an empty string. - HooksLibrariesStoragePtr ptr = getHooksLibrariesPtr(); - EXPECT_TRUE(ptr); - EXPECT_EQ(0, ptr->size()); } + TEST_F(ParseConfigTest, validHooksLibrariesTest) { - // @todo Initialize global library context to null - - // Configuration string. This contains a valid library. + // Configuration string. This contains a set of valid libraries. const std::string quote("\""); const std::string comma(", "); const std::string config = std::string("{ ") + - std::string("\"hooks_libraries\": [") + + std::string("\"hooks-libraries\": [") + quote + std::string(BASIC_CALLOUT_LIBRARY) + quote + comma + quote + std::string(FULL_CALLOUT_LIBRARY) + quote + std::string("]") + @@ -570,24 +605,34 @@ TEST_F(ParseConfigTest, validHooksLibrariesTest) { // Verify that the configuration string parses. int rcode = parseConfiguration(config); - ASSERT_TRUE(rcode == 0); + ASSERT_TRUE(rcode == 0) << error_text_; - // @todo modify after the hooks check has been added. At the moment, the - // string should parse to an empty string. - HooksLibrariesStoragePtr ptr = getHooksLibrariesPtr(); - EXPECT_TRUE(ptr); + // Check that the parser holds two libraries and the configuration is + // recorded as having changed. + std::vector libraries; + bool changed; + hooks_libraries_parser_->getLibraries(libraries, changed); + EXPECT_EQ(2, libraries.size()); + EXPECT_TRUE(changed); // The expected libraries should be the list of libraries specified // in the given order. std::vector expected; expected.push_back(BASIC_CALLOUT_LIBRARY); expected.push_back(FULL_CALLOUT_LIBRARY); + EXPECT_TRUE(expected == libraries); - ASSERT_TRUE(ptr); - EXPECT_TRUE(expected == *ptr); + // Parse the string again. + rcode = parseConfiguration(config); + ASSERT_TRUE(rcode == 0) << error_text_; + + // The list has not changed, and this is what we should see. + hooks_libraries_parser_->getLibraries(libraries, changed); + EXPECT_EQ(2, libraries.size()); + EXPECT_FALSE(changed); } -// Now parse +// Check with a set of libraries, some of which are invalid. TEST_F(ParseConfigTest, invalidHooksLibrariesTest) { // @todo Initialize global library context to null @@ -599,29 +644,19 @@ TEST_F(ParseConfigTest, invalidHooksLibrariesTest) { const std::string config = std::string("{ ") + - std::string("\"hooks_libraries\": [") + + std::string("\"hooks-libraries\": [") + quote + std::string(BASIC_CALLOUT_LIBRARY) + quote + comma + quote + std::string(NOT_PRESENT_LIBRARY) + quote + comma + quote + std::string(FULL_CALLOUT_LIBRARY) + quote + std::string("]") + std::string("}"); - // Verify that the configuration string parses. + // Verify that the configuration fails to parse. (Syntactically it's OK, + // but the library is invalid). int rcode = parseConfiguration(config); - ASSERT_TRUE(rcode == 0); + ASSERT_FALSE(rcode == 0) << error_text_; - // @todo modify after the hooks check has been added. At the moment, the - // string should parse to an empty string. - HooksLibrariesStoragePtr ptr = getHooksLibrariesPtr(); - EXPECT_TRUE(ptr); - - // The expected libraries should be the list of libraries specified - // in the given order. - std::vector expected; - expected.push_back(BASIC_CALLOUT_LIBRARY); - expected.push_back(NOT_PRESENT_LIBRARY); - expected.push_back(FULL_CALLOUT_LIBRARY); - - ASSERT_TRUE(ptr); - EXPECT_TRUE(expected == *ptr); + // Check that the message contains the library in error. + EXPECT_FALSE(error_text_.find(NOT_PRESENT_LIBRARY) == string::npos) << + "Error text returned from parse failure is " << error_text_; } diff --git a/src/lib/hooks/hooks_manager.h b/src/lib/hooks/hooks_manager.h index 03aa52113a..393c1ea7ea 100644 --- a/src/lib/hooks/hooks_manager.h +++ b/src/lib/hooks/hooks_manager.h @@ -86,6 +86,13 @@ public: /// In the latter case, an error message will have been output. static void unloadLibraries(); + /// @brief Reload libraries + /// + /// Reloads the current libraries. This causes all the libraries' "unload" + /// functions to run, the libraries to be closed, reopened, and all the + /// "load" functions run. + static void reloadLibraries(); + /// @brief Are callouts present? /// /// Checks loaded libraries and returns true if at lease one callout