[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

@@ -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

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:

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)

src/lib/dhcpsrv/dhcp_parsers.h

@@ -33,7 +33,6 @@ namespace dhcp {
 /// @brief Storage for option definitions.
 typedef OptionSpaceContainer<OptionDefContainer,
                              OptionDefinitionPtr> OptionDefStorage;
-
 /// @brief Shared pointer to option definitions storage.
 typedef boost::shared_ptr<OptionDefStorage> OptionDefStoragePtr;
 
@@ -44,11 +43,18 @@ typedef OptionSpaceContainer<Subnet::OptionContainer,
 /// @brief Shared pointer to option storage.
 typedef boost::shared_ptr<OptionStorage> OptionStoragePtr;
 
+/// @brief Storage for hooks libraries
+typedef std::vector<std::string> HooksLibrariesStorage;
+/// @brief Pointer to storage for hooks libraries
+typedef boost::shared_ptr<HooksLibrariesStorage> 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<typename ValueType>
@@ -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<std::string> 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.
 ///

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 <hooks/hooks.h>
+#include <fstream>
+
+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<int>(10));
+    handle.setArgument("result", static_cast<int>(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);
+}
+
+};
+

src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc

@@ -20,6 +20,7 @@
 #include <dhcpsrv/cfgmgr.h>
 #include <dhcpsrv/subnet.h>
 #include <dhcpsrv/dhcp_parsers.h>
+#include <dhcpsrv/tests/test_libraries.h>
 #include <exceptions/exceptions.h>
 
 #include <gtest/gtest.h>
@@ -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<std::string> 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<std::string> 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);
+}

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 <hooks/hooks.h>
+#include <hooks/tests/marker_file.h>
+
+#include <fstream>
+
+using namespace isc::hooks;
+
+extern "C" {
+
+// Callouts
+
+int
+context_create(CalloutHandle& handle) {
+    handle.setContext("result", static_cast<int>(7));
+    handle.setArgument("result", static_cast<int>(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);
+}
+
+};
+

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 <config.h>
+
+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