From 721eff49ce716e79b898ad578e21efb96e684684 Mon Sep 17 00:00:00 2001 From: Stephen Morris Date: Mon, 1 Jul 2013 15:01:24 +0100 Subject: [PATCH] [2981] Checkpoint Now awaiting the merging of #2980 (the hooks code) so that the configuration validation code can check if the libraries are valid before accepting the configuration. --- configure.ac | 1 + src/lib/dhcpsrv/cfgmgr.h | 4 +- src/lib/dhcpsrv/dhcp_parsers.cc | 62 ++++++++ src/lib/dhcpsrv/dhcp_parsers.h | 72 ++++++++- .../dhcpsrv/tests/basic_callout_library.cc | 115 +++++++++++++++ .../dhcpsrv/tests/dhcp_parsers_unittest.cc | 110 +++++++++++++- src/lib/dhcpsrv/tests/full_callout_library.cc | 137 ++++++++++++++++++ src/lib/dhcpsrv/tests/test_libraries.h.in | 55 +++++++ 8 files changed, 550 insertions(+), 6 deletions(-) create mode 100644 src/lib/dhcpsrv/tests/basic_callout_library.cc create mode 100644 src/lib/dhcpsrv/tests/full_callout_library.cc create mode 100644 src/lib/dhcpsrv/tests/test_libraries.h.in diff --git a/configure.ac b/configure.ac index 72a825edd..6e394a854 100644 --- a/configure.ac +++ b/configure.ac @@ -1426,6 +1426,7 @@ AC_OUTPUT([doc/version.ent src/bin/d2/tests/test_data_files_config.h src/bin/tests/process_rename_test.py src/lib/config/tests/data_def_unittests_config.h + src/lib/dhcpsrv/tests/test_libraries.h src/lib/python/isc/config/tests/config_test src/lib/python/isc/cc/tests/cc_test src/lib/python/isc/notify/tests/notify_out_test diff --git a/src/lib/dhcpsrv/cfgmgr.h b/src/lib/dhcpsrv/cfgmgr.h index 3c37a0969..6da9f20bb 100644 --- a/src/lib/dhcpsrv/cfgmgr.h +++ b/src/lib/dhcpsrv/cfgmgr.h @@ -50,7 +50,9 @@ namespace dhcp { /// 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. +/// 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. /// /// Below is a sketch of configuration inheritance (not implemented yet). /// Let's investigate the following configuration: diff --git a/src/lib/dhcpsrv/dhcp_parsers.cc b/src/lib/dhcpsrv/dhcp_parsers.cc index 4d1dc7308..224546d9c 100644 --- a/src/lib/dhcpsrv/dhcp_parsers.cc +++ b/src/lib/dhcpsrv/dhcp_parsers.cc @@ -41,6 +41,7 @@ ParserContext::ParserContext(Option::Universe universe): string_values_(new StringStorage()), options_(new OptionStorage()), option_defs_(new OptionDefStorage()), + hooks_libraries_(new HooksLibrariesStorage()), universe_(universe) { } @@ -50,6 +51,7 @@ 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_) { } @@ -65,6 +67,9 @@ 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); @@ -161,6 +166,63 @@ InterfaceListConfigParser::commit() { /// on all interfaces. } +// ******************** 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") { + isc_throw(BadValue, "Internal error. Hooks libraries " + "parser called for the wrong parameter: " << param_name); + } +} + +void +HooksLibrariesParser::build(ConstElementPtr value) { + + /// Extract the list of libraries. + HooksLibrariesStoragePtr libraries(new HooksLibrariesStorage()); + BOOST_FOREACH(ConstElementPtr iface, value->listValue()) { + string libname = iface->str(); + boost::erase_all(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 + + /// 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); +} + +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_); +} + // **************************** OptionDataParser ************************* OptionDataParser::OptionDataParser(const std::string&, OptionStoragePtr options, ParserContextPtr global_context) diff --git a/src/lib/dhcpsrv/dhcp_parsers.h b/src/lib/dhcpsrv/dhcp_parsers.h index e453204bd..66e098306 100644 --- a/src/lib/dhcpsrv/dhcp_parsers.h +++ b/src/lib/dhcpsrv/dhcp_parsers.h @@ -33,7 +33,6 @@ namespace dhcp { /// @brief Storage for option definitions. typedef OptionSpaceContainer OptionDefStorage; - /// @brief Shared pointer to option definitions storage. typedef boost::shared_ptr OptionDefStoragePtr; @@ -44,11 +43,18 @@ 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. /// -/// This template class is provides data value storage for configuration parameters -/// of a given data type. The values are stored by parameter name and as instances -/// of type "ValueType". +/// This template class is provides data value storage for configuration +/// parameters of a given data type. The values are stored by parameter name +/// and as instances of type "ValueType". /// /// @param ValueType is the data type of the elements to store. template @@ -151,6 +157,26 @@ public: /// @brief Storage for option definitions. OptionDefStoragePtr option_defs_; + /// @brief Hooks libraries pointer. + /// + /// The hooks libraries information is a vector of strings, each containing + /// the name of a library. Hooks libraries should only be reloaded if the + /// list of names has changed, so the list of current DHCP parameters + /// (in isc::dhcp::CfgMgr) contains an indication as to whether the list has + /// altered. This indication is implemented by storing a pointer to the + /// list of library names which is cleared when the libraries are loaded. + /// So either the pointer is null (meaning don't reload the libraries and + /// 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_; + /// @brief The parsing universe of this context. Option::Universe universe_; @@ -306,6 +332,44 @@ private: std::vector interfaces_; }; +/// @brief parser for hooks library list +/// +/// This parser handles the list of hooks libraries. This is an optional list, +/// which may be empty. +class HooksLibrariesParser : public DhcpConfigParser { +public: + + /// @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); + + /// @brief parses parameters value + /// + /// Parses configuration entry (list of parameters) and adds each element + /// to the hooks libraries list. + /// + /// @param value pointer to the content of parsed values + virtual void build(isc::data::ConstElementPtr value); + + /// @brief commits hooks libraries data + virtual void commit(); + +private: + /// List of hooks libraries. This will be NULL if there is no change to + /// the list. + HooksLibrariesStoragePtr libraries_; + + /// Parsing context which contains global values, options and option + /// definitions. + ParserContextPtr global_context_; +}; /// @brief Parser for option data value. /// diff --git a/src/lib/dhcpsrv/tests/basic_callout_library.cc b/src/lib/dhcpsrv/tests/basic_callout_library.cc new file mode 100644 index 000000000..ff39a9c8d --- /dev/null +++ b/src/lib/dhcpsrv/tests/basic_callout_library.cc @@ -0,0 +1,115 @@ +// 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 Basic callout library +/// +/// This is source of a test library for various test (LibraryManager and +/// HooksManager). The characteristics of the library produced from this +/// file are: +/// +/// - Only the "version" framework function is supplied. +/// +/// - A context_create callout is supplied. +/// +/// - Three "standard" callouts are supplied corresponding to the hooks +/// "lm_one", "lm_two", "lm_three". All do some trivial calculations +/// on the arguments supplied to it and the context variables, returning +/// intermediate results through the "result" argument. The result of +/// executing all four callouts in order is: +/// +/// @f[ (10 + data_1) * data_2 - data_3 @f] +/// +/// ...where data_1, data_2 and data_3 are the values passed in arguments of +/// the same name to the three callouts (data_1 passed to lm_one, data_2 to +/// lm_two etc.) and the result is returned in the argument "result". + +#include +#include + +using namespace isc::hooks; +using namespace std; + +extern "C" { + +// Callouts. All return their result through the "result" argument. + +int +context_create(CalloutHandle& handle) { + handle.setContext("result", static_cast(10)); + handle.setArgument("result", static_cast(10)); + return (0); +} + +// First callout adds the passed "data_1" argument to the initialized context +// value of 10. (Note that the value set by context_create is accessed through +// context and not the argument, so checking that context is correctly passed +// between callouts in the same library.) + +int +lm_one(CalloutHandle& handle) { + int data; + handle.getArgument("data_1", data); + + int result; + handle.getArgument("result", result); + + result += data; + handle.setArgument("result", result); + + return (0); +} + +// Second callout multiplies the current context value by the "data_2" +// argument. + +int +lm_two(CalloutHandle& handle) { + int data; + handle.getArgument("data_2", data); + + int result; + handle.getArgument("result", result); + + result *= data; + handle.setArgument("result", result); + + return (0); +} + +// Final callout subtracts the result in "data_3". + +int +lm_three(CalloutHandle& handle) { + int data; + handle.getArgument("data_3", data); + + int result; + handle.getArgument("result", result); + + result -= data; + handle.setArgument("result", result); + + return (0); +} + +// Framework functions. Only version() is supplied here. + +int +version() { + return (BIND10_HOOKS_VERSION); +} + +}; + diff --git a/src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc b/src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc index 687ef92b8..ef47080ec 100644 --- a/src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc +++ b/src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc @@ -20,6 +20,7 @@ #include #include #include +#include #include #include @@ -332,6 +333,8 @@ public: } 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_); } else { isc_throw(NotImplemented, "Parser error: configuration parameter not supported: " @@ -349,7 +352,7 @@ public: /// /// @return retuns 0 if the configuration parsed successfully, /// non-zero otherwise failure. - int parseConfiguration (std::string &config) { + int parseConfiguration(const std::string& config) { int rcode_ = 1; // Turn config into elements. // Test json just to make sure its valid. @@ -421,6 +424,14 @@ 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(){ @@ -517,3 +528,100 @@ 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 + + // Configuration string. This contains a valid library. + const std::string config = + "{ \"hooks_libraries\": [ " + " ]" + "}"; + + // Verify that the configuration string parses. + int rcode = parseConfiguration(config); + ASSERT_TRUE(rcode == 0); + + // @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. + const std::string quote("\""); + const std::string comma(", "); + + const std::string config = + std::string("{ ") + + std::string("\"hooks_libraries\": [") + + quote + std::string(BASIC_CALLOUT_LIBRARY) + quote + comma + + quote + std::string(FULL_CALLOUT_LIBRARY) + quote + + std::string("]") + + std::string("}"); + + // Verify that the configuration string parses. + int rcode = parseConfiguration(config); + ASSERT_TRUE(rcode == 0); + + // @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(FULL_CALLOUT_LIBRARY); + + ASSERT_TRUE(ptr); + EXPECT_TRUE(expected == *ptr); +} + +// Now parse +TEST_F(ParseConfigTest, invalidHooksLibrariesTest) { + + // @todo Initialize global library context to null + + // Configuration string. This contains an invalid library which should + // trigger an error in the "build" stage. + const std::string quote("\""); + const std::string comma(", "); + + const std::string config = + std::string("{ ") + + 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. + int rcode = parseConfiguration(config); + ASSERT_TRUE(rcode == 0); + + // @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); +} diff --git a/src/lib/dhcpsrv/tests/full_callout_library.cc b/src/lib/dhcpsrv/tests/full_callout_library.cc new file mode 100644 index 000000000..df7a76f0a --- /dev/null +++ b/src/lib/dhcpsrv/tests/full_callout_library.cc @@ -0,0 +1,137 @@ +// 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 Full callout library +/// +/// This is source of a test library for various test (LibraryManager and +/// HooksManager). The characteristics of the library produced from this +/// file are: +/// +/// The characteristics of this library are: +/// +/// - All three framework functions are supplied (version(), load() and +/// unload()), with unload() creating a marker file. The test code checks +/// for the presence of this file, so verifying that unload() has been run. +/// +/// - One standard and two non-standard callouts are supplied, with the latter +/// being registered by the load() function. +/// +/// All callouts do trivial calculations, the result of all being called in +/// sequence being +/// +/// @f[ ((7 * data_1) - data_2) * data_3 @f] +/// +/// ...where data_1, data_2 and data_3 are the values passed in arguments of +/// the same name to the three callouts (data_1 passed to lm_one, data_2 to +/// lm_two etc.) and the result is returned in the argument "result". + +#include +#include + +#include + +using namespace isc::hooks; + +extern "C" { + +// Callouts + +int +context_create(CalloutHandle& handle) { + handle.setContext("result", static_cast(7)); + handle.setArgument("result", static_cast(7)); + return (0); +} + +// First callout adds the passed "data_1" argument to the initialized context +// value of 7. (Note that the value set by context_create is accessed through +// context and not the argument, so checking that context is correctly passed +// between callouts in the same library.) + +int +lm_one(CalloutHandle& handle) { + int data; + handle.getArgument("data_1", data); + + int result; + handle.getArgument("result", result); + + result *= data; + handle.setArgument("result", result); + + return (0); +} + +// Second callout subtracts the passed value of data_2 from the current +// running total. + +static int +lm_nonstandard_two(CalloutHandle& handle) { + int data; + handle.getArgument("data_2", data); + + int result; + handle.getArgument("result", result); + + result -= data; + handle.setArgument("result", result); + + return (0); +} + +// Final callout multplies the current running total by data_3. + +static int +lm_nonstandard_three(CalloutHandle& handle) { + int data; + handle.getArgument("data_3", data); + + int result; + handle.getArgument("result", result); + + result *= data; + handle.setArgument("result", result); + + return (0); +} + +// Framework functions + +int +version() { + return (BIND10_HOOKS_VERSION); +} + +int +load(LibraryHandle& handle) { + // Register the non-standard functions + handle.registerCallout("lm_two", lm_nonstandard_two); + handle.registerCallout("lm_three", lm_nonstandard_three); + + return (0); +} + +int +unload() { + // Create the marker file. + std::fstream marker; + marker.open(MARKER_FILE, std::fstream::out); + marker.close(); + + return (0); +} + +}; + diff --git a/src/lib/dhcpsrv/tests/test_libraries.h.in b/src/lib/dhcpsrv/tests/test_libraries.h.in new file mode 100644 index 000000000..730fd8436 --- /dev/null +++ b/src/lib/dhcpsrv/tests/test_libraries.h.in @@ -0,0 +1,55 @@ +// 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 carse of differences in DLL naming between operating systems. + +#ifdef OS_BSD +#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 +// .so file. Note that we access the .so file - libtool creates this as a +// like to the real shared library. + +// Basic library with context_create and three "standard" callouts. +static const char* BASIC_CALLOUT_LIBRARY = "@abs_builddir@/.libs/libbcl" + DLL_SUFFIX; + +// Library with context_create and three "standard" callouts, as well as +// load() and unload() functions. +static const char* FULL_CALLOUT_LIBRARY = "@abs_builddir@/.libs/libfcl" + 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 -- GitLab