[2981] Added "libreload" command handling

src/bin/dhcp4/ctrl_dhcp4_srv.cc

@@ -19,24 +19,28 @@
 #include <cc/session.h>
 #include <config/ccsession.h>
 #include <dhcp/iface_mgr.h>
-#include <dhcpsrv/dhcp_config_parser.h>
-#include <dhcpsrv/cfgmgr.h>
+#include <dhcp4/config_parser.h>
 #include <dhcp4/ctrl_dhcp4_srv.h>
 #include <dhcp4/dhcp4_log.h>
 #include <dhcp4/spec_config.h>
-#include <dhcp4/config_parser.h>
+#include <dhcpsrv/cfgmgr.h>
+#include <dhcpsrv/dhcp_config_parser.h>
 #include <exceptions/exceptions.h>
+#include <hooks/hooks_manager.h>
 #include <util/buffer.h>
 
 #include <cassert>
 #include <iostream>
 #include <sstream>
+#include <string>
+#include <vector>
 
 using namespace isc::asiolink;
 using namespace isc::cc;
 using namespace isc::config;
 using namespace isc::data;
 using namespace isc::dhcp;
+using namespace isc::hooks;
 using namespace isc::log;
 using namespace isc::util;
 using namespace std;
@@ -142,7 +146,19 @@ ControlledDhcpv4Srv::dhcp4CommandHandler(const string& command, ConstElementPtr
                                  "Shutting down.");
         return (answer);
     } else if (command == "libreload") {
-        // TODO Reload libraries
+        // TODO delete any stored CalloutHandles referring to the old libraries
+        // Get list of currently loaded libraries and reload them.
+        vector<string> loaded = HooksManager::getLibraryNames();
+        bool status = HooksManager::loadLibraries(loaded);
+        if (!status) {
+            LOG_ERROR(dhcp4_logger, DHCP4_RELOAD_FAIL);
+            ConstElementPtr answer = isc::config::createAnswer(1,
+                                     "Failed to reload hooks libraries.");
+            return (answer);
+        }
+        ConstElementPtr answer = isc::config::createAnswer(0,
+                                 "Hooks libraries successfully reloaded.");
+        return (answer);
     }
 
     ConstElementPtr answer = isc::config::createAnswer(1,

src/bin/dhcp4/dhcp4_messages.mes

@@ -226,6 +226,11 @@ a different hardware address. One possible reason for using different
 hardware address is that a cloned virtual machine was not updated and
 both clones use the same client-id.
 
+% DHCP4_RELOAD_FAIL reload of hooks libraries failed
+A "libreload" command was issued to reload the hooks libraries but for
+some reason the reload failed.  Other error messages issued from the
+hooks framework will indicate the nature of the problem.
+
 % DHCP4_RESPONSE_DATA responding with packet type %1, data is <%2>
 A debug message listing the data returned to the client.
 

src/bin/dhcp4/tests/Makefile.am

@@ -32,6 +32,7 @@ AM_CPPFLAGS += -DTEST_DATA_BUILDDIR=\"$(abs_top_builddir)/src/bin/dhcp6/tests\"
 AM_CPPFLAGS += -DINSTALL_PROG=\"$(abs_top_srcdir)/install-sh\"
 
 CLEANFILES = $(builddir)/interfaces.txt $(builddir)/logger_lockfile
+CLEANFILES += $(builddir)/load_marker.txt $(builddir)/unload_marker.txt
 
 AM_CXXFLAGS = $(B10_CXXFLAGS)
 if USE_CLANGPP
@@ -68,6 +69,7 @@ dhcp4_unittests_SOURCES += dhcp4_unittests.cc
 dhcp4_unittests_SOURCES += dhcp4_srv_unittest.cc
 dhcp4_unittests_SOURCES += ctrl_dhcp4_srv_unittest.cc
 dhcp4_unittests_SOURCES += config_parser_unittest.cc
+dhcp4_unittests_SOURCES += marker_file.cc
 nodist_dhcp4_unittests_SOURCES = ../dhcp4_messages.h ../dhcp4_messages.cc
 nodist_dhcp4_unittests_SOURCES += marker_file.h test_libraries.h
 

src/bin/dhcp4/tests/config_parser_unittest.cc

@@ -43,6 +43,7 @@ using namespace isc::asiolink;
 using namespace isc::config;
 using namespace isc::data;
 using namespace isc::dhcp;
+using namespace isc::dhcp::test;
 using namespace isc::hooks;
 using namespace std;
 
@@ -316,61 +317,6 @@ public:
                                                "reset configuration database"));
     }
 
-    /// @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);
-
-        // Is it open?
-        if (!file.is_open()) {
-
-            // 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);
-    }
-
     boost::scoped_ptr<Dhcpv4Srv> srv_;
 
     int rcode_;

src/bin/dhcp4/tests/ctrl_dhcp4_srv_unittest.cc

@@ -17,6 +17,10 @@
 #include <config/ccsession.h>
 #include <dhcp/dhcp4.h>
 #include <dhcp4/ctrl_dhcp4_srv.h>
+#include <hooks/hooks_manager.h>
+
+#include "marker_file.h"
+#include "test_libraries.h"
 
 #include <boost/scoped_ptr.hpp>
 #include <gtest/gtest.h>
@@ -26,13 +30,16 @@
 #include <sstream>
 
 #include <arpa/inet.h>
+#include <unistd.h>
 
 using namespace std;
 using namespace isc;
-using namespace isc::dhcp;
 using namespace isc::asiolink;
-using namespace isc::data;
 using namespace isc::config;
+using namespace isc::data;
+using namespace isc::dhcp;
+using namespace isc::dhcp::test;
+using namespace isc::hooks;
 
 namespace {
 
@@ -45,10 +52,25 @@ public:
 class CtrlDhcpv4SrvTest : public ::testing::Test {
 public:
     CtrlDhcpv4SrvTest() {
+        reset();
     }
 
     ~CtrlDhcpv4SrvTest() {
+        reset();
     };
+
+    /// @brief Reset hooks data
+    ///
+    /// Resets the data for the hooks-related portion of the test by ensuring
+    /// that no libraries are loaded and that any marker files are deleted.
+    void reset() {
+        // Unload any previously-loaded libraries.
+        HooksManager::unloadLibraries();
+
+        // Get rid of any marker files.
+        static_cast<void>(unlink(LOAD_MARKER_FILE));
+        static_cast<void>(unlink(UNLOAD_MARKER_FILE));
+    }
 };
 
 TEST_F(CtrlDhcpv4SrvTest, commands) {
@@ -82,4 +104,46 @@ TEST_F(CtrlDhcpv4SrvTest, commands) {
     EXPECT_EQ(0, rcode); // expect success
 }
 
+// Check that the "libreload" command will reload libraries
+
+TEST_F(CtrlDhcpv4SrvTest, libreload) {
+    // Ensure no marker files to start with.
+    ASSERT_FALSE(checkMarkerFileExists(LOAD_MARKER_FILE));
+    ASSERT_FALSE(checkMarkerFileExists(UNLOAD_MARKER_FILE));
+
+    // Load two libraries
+    std::vector<std::string> libraries;
+    libraries.push_back(CALLOUT_LIBRARY_1);
+    libraries.push_back(CALLOUT_LIBRARY_2);
+    HooksManager::loadLibraries(libraries);
+
+    // Check they are loaded.
+    std::vector<std::string> loaded_libraries =
+        HooksManager::getLibraryNames();
+    ASSERT_TRUE(libraries == loaded_libraries);
+
+    // ... which also included checking that the marker file created by the
+    // load functions exists.
+    EXPECT_TRUE(checkMarkerFile(LOAD_MARKER_FILE, "12"));
+    EXPECT_FALSE(checkMarkerFileExists(UNLOAD_MARKER_FILE));
+
+    // Now execute the "libreload" command.  This should cause the libraries
+    // to unload and to reload.
+
+    // Use empty parameters list
+    ElementPtr params(new isc::data::MapElement());
+    int rcode = -1;
+
+    ConstElementPtr result =
+        ControlledDhcpv4Srv::execDhcpv4ServerCommand("libreload", params);
+    ConstElementPtr comment = parseAnswer(rcode, result);
+    EXPECT_EQ(0, rcode); // expect success
+
+    // Check that the libraries have unloaded and reloaded.  The libraries are
+    // unloaded in the reverse order to which they are loaded.  When they load,
+    // they should append information to the loading marker file.
+    EXPECT_TRUE(checkMarkerFile(UNLOAD_MARKER_FILE, "21"));
+    EXPECT_TRUE(checkMarkerFile(LOAD_MARKER_FILE, "1212"));
+}
+
 } // End of anonymous namespace

src/bin/dhcp4/tests/marker_file.cc

+// 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.
+
+#include "marker_file.h"
+
+#include <gtest/gtest.h>
+
+#include <fstream>
+#include <string>
+
+namespace isc {
+namespace dhcp {
+namespace test {
+
+using namespace std;
+
+// Check the marker file.
+
+bool
+checkMarkerFile(const char* name, const char* expected) {
+    // Open the file for input
+    fstream file(name, fstream::in);
+
+    // Is it open?
+    if (!file.is_open()) {
+
+        // 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);
+}
+
+// Check if the marker file exists - this is a wrapper for "access(2)" and
+// really tests if the file exists and is accessible
+
+bool
+checkMarkerFileExists(const char* name) {
+    return (access(name, F_OK) == 0);
+}
+
+} // namespace test
+} // namespace dhcp
+} // namespace isc

src/bin/dhcp4/tests/marker_file.h.in

@@ -17,12 +17,55 @@
 
 /// @file
 /// Define a marker file that is used in tests to prove that an "unload"
-/// function has been called.
+/// 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";
+const char* const LOAD_MARKER_FILE = "@abs_builddir@/load_marker.txt";
+const char* const UNLOAD_MARKER_FILE = "@abs_builddir@/unload_marker.txt";
 }
 
+namespace isc {
+namespace dhcp {
+namespace test {
+
+/// @brief Check marker file
+///
+/// This function is used in some of the DHCP server tests.
+///
+/// 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);
+
+/// @brief Check marker file exists
+///
+/// This function is used in some of the DHCP server tests.
+///
+/// Checkes that the specified file does NOT exist.
+///
+/// @param name Name of the marker file.
+///
+/// @return true if file exists, false if not.
+bool
+checkMarkerFileExists(const char* name);
+
+} // namespace test
+} // namespace dhcp
+} // namespace isc
+
 #endif // MARKER_FILE_H
 

src/bin/dhcp4/tests/test_libraries.h.in

@@ -37,13 +37,13 @@ namespace {
 
 // Library with load/unload functions creating marker files to check their
 // operation.
-static const char* CALLOUT_LIBRARY_1 = "@abs_builddir@/.libs/libco1"
+const char* const CALLOUT_LIBRARY_1 = "@abs_builddir@/.libs/libco1"
                                            DLL_SUFFIX;
-static const char* CALLOUT_LIBRARY_2 = "@abs_builddir@/.libs/libco2"
+const char* const 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"
+const char* const NOT_PRESENT_LIBRARY = "@abs_builddir@/.libs/libnothere"
                                          DLL_SUFFIX;
 } // anonymous namespace
 

src/bin/dhcp6/tests/marker_file.h.in

@@ -24,5 +24,9 @@ const char* LOAD_MARKER_FILE = "@abs_builddir@/load_marker.txt";
 const char* UNLOAD_MARKER_FILE = "@abs_builddir@/unload_marker.txt";
 }
 
+namespace isc {
+namespace dhcp {
+namespace test {
+
 #endif // MARKER_FILE_H
 

src/lib/dhcpsrv/dhcp_parsers.cc

@@ -274,6 +274,8 @@ HooksLibrariesParser::commit() {
     /// Commits the list of libraries to the configuration manager storage if
     /// the list of libraries has changed.
     if (changed_) {
+        // TODO Delete any stored CalloutHandles before reloading the
+        // libraries
         HooksManager::loadLibraries(libraries_);
     }
 }

src/lib/hooks/hooks_manager.h

@@ -86,13 +86,6 @@ 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