[2980] Finished LibraryManager functionality

LibraryManager can now load and unload libraries.  Multiple
libraries can be loaded at the same time and the callouts
execute correctly.

src/lib/hooks/callout_manager.cc

@@ -14,6 +14,7 @@
 
 #include <hooks/callout_handle.h>
 #include <hooks/callout_manager.h>
+#include <hooks/hooks_log.h>
 
 #include <boost/static_assert.hpp>
 
@@ -47,6 +48,8 @@ CalloutManager::registerCallout(const std::string& name, CalloutPtr callout) {
             // current index, so insert the new element ahead of this one.
             hook_vector_[hook_index].insert(i, make_pair(current_library_,
                                                          callout));
+            LOG_DEBUG(hooks_logger, HOOKS_DBG_CALLS, HOOKS_REGISTER_CALLOUT)
+                .arg(current_library_).arg(name);
             return;
         }
     }
@@ -104,7 +107,19 @@ CalloutManager::callCallouts(int hook_index, CalloutHandle& callout_handle) {
 
             // Call the callout
             // @todo Log the return status if non-zero
-            static_cast<void>((*i->second)(callout_handle));
+            int status = (*i->second)(callout_handle);
+            if (status == 0) {
+                LOG_DEBUG(hooks_logger, HOOKS_DBG_EXTENDED_CALLS, HOOKS_CALLOUT)
+                    .arg(current_library_)
+                    .arg(ServerHooks::getServerHooks().getName(current_hook_))
+                    .arg(reinterpret_cast<void*>(i->second));
+            } else {
+                LOG_WARN(hooks_logger, HOOKS_CALLOUT_ERROR)
+                    .arg(current_library_)
+                    .arg(ServerHooks::getServerHooks().getName(current_hook_))
+                    .arg(reinterpret_cast<void*>(i->second));
+            }
+
         }
 
         // Reset the current hook and library indexs to an invalid value to
@@ -150,7 +165,13 @@ CalloutManager::deregisterCallout(const std::string& name, CalloutPtr callout) {
                                    hook_vector_[hook_index].end());
 
     // Return an indication of whether anything was removed.
-    return (initial_size != hook_vector_[hook_index].size());
+    bool removed = initial_size != hook_vector_[hook_index].size();
+    if (removed) {
+        LOG_DEBUG(hooks_logger, HOOKS_DBG_EXTENDED_CALLS,
+                  HOOKS_DEREGISTER_CALLOUT).arg(current_library_).arg(name);
+    }
+
+    return (removed);
 }
 
 // Deregister all callouts on a given hook.
@@ -179,7 +200,14 @@ CalloutManager::deregisterAllCallouts(const std::string& name) {
                                    hook_vector_[hook_index].end());
 
     // Return an indication of whether anything was removed.
-    return (initial_size != hook_vector_[hook_index].size());
+    bool removed = initial_size != hook_vector_[hook_index].size();
+    if (removed) {
+        LOG_DEBUG(hooks_logger, HOOKS_DBG_EXTENDED_CALLS,
+                  HOOKS_DEREGISTER_ALL_CALLOUTS).arg(current_library_)
+                                                .arg(name);
+    }
+
+    return (removed);
 }
 
 } // namespace util

src/lib/hooks/hooks_log.h

@@ -21,7 +21,7 @@
 namespace isc {
 namespace hooks {
 
-/// \brief Hooks system Logging
+/// @brief Hooks debug Logging levels
 ///
 /// Defines the levels used to output debug messages in the Hooks framework.
 /// Note that higher numbers equate to more verbose (and detailed) output.
@@ -37,12 +37,12 @@ const int HOOKS_DBG_CALLS = DBGLVL_TRACE_BASIC_DATA;
 const int HOOKS_DBG_EXTENDED_CALLS = DBGLVL_TRACE_DETAIL_DATA;
 
 
-/// \brief HOOKS Logger
+/// @brief Hooks Logger
 ///
 /// Define the logger used to log messages.  We could define it in multiple
 /// modules, but defining in a single module and linking to it saves time and
 /// space.
-extern isc::log::Logger hooks_logger;    // isc::hooks::logger is the HOOKS logger
+extern isc::log::Logger hooks_logger;
 
 } // namespace hooks
 } // namespace isc

src/lib/hooks/hooks_messages.mes

@@ -14,12 +14,38 @@
 
 $NAMESPACE isc::hooks
 
+% HOOKS_CALLOUT hooks library with index %1 has called a callout on hook %2 that has address %3
+Only output at a high debugging level, this message indicates that
+a callout on the named hook registered by the library with the given
+index (in the list of loaded libraries) has been called and returned a
+success state.  The address of the callout is given in the message
+
+% HOOKS_CALLOUT_ERROR error returned by callout on hook %1 registered by library with index %2 (callout address %3)
+If a callout returns an error status when called, this warning message is
+issues.  It identifies the hook to which the callout is attached, and the
+index of the library (in the list of loaded libraries) that registered it.
+The address of the callout is also given.
+
+% HOOKS_CALLOUT_REMOVED callout removed from hook %1 for library %2
+This is a debug message issued during library unloading.  It notes that one
+of more callouts registered by that library have been removed from the
+specified hook.
+
 % HOOKS_CLOSE_ERROR failed to close hook library %1: %2
 BIND 10 has failed to close the named hook library for the stated reason.
 Although this is an error, this should not affect the running system other
 than as a loss of resources.  If this error persists, you should restart
 BIND 10.
 
+% HOOKS_DEREGISTER_ALL_CALLOUTS hook library at index %1 deregistered all callouts on hook %2
+A debug message issued when all callouts on the specified hook registered
+by the library with the given index were removed.
+
+% HOOKS_DEREGISTER_CALLOUT hook library at index %1 deregistered a callout on hook %2
+A debug message issued when all instances of a particular callouts on the
+hook identified in the message that were registered by the library with the
+given index were removed.
+
 % HOOKS_INCORRECT_VERSION hook library %1 is at version %2, require version %3
 BIND 10 has detected that the named hook library has been built against
 a version of BIND 10 that is incompatible with the version of BIND 10
@@ -29,6 +55,18 @@ This is most likely due to the installation of a new version of BIND 10
 without rebuilding the hook library.  A rebuild and re-install of the library
 should fix the problem in most cases.
 
+% HOOKS_LIBRARY_LOADED hooks library %1 successfully loaded
+This information message is issued when a user-supplied hooks library has been
+successfully loaded.
+
+% HOOKS_LIBRARY_UNLOADED hooks library %1 successfully unloaded
+This information message is issued when a user-supplied hooks library has been
+successfully unloaded.
+
+% HOOKS_LIBRARY_VERSION hooks library %1 reports its version as %2
+A debug  message issued when the version check on the hooks library has
+succeeded.
+
 % HOOKS_LOAD 'load' function in hook library %1 returned success
 This is a debug message issued when the "load" function has been found in a
 hook library and has been successfully called.
@@ -39,6 +77,9 @@ called.  The function returned a non-zero status (also given in the message)
 which was interpreted as an error.  The library has been unloaded and
 no callouts from it will be installed.
 
+% HOOKS_LOAD_LIBRARY loading hooks library %1
+This is a debug message called when the specified library is being loaded.
+
 % HOOKS_NO_LOAD no 'load' function found in hook library %1
 This is a debug message saying that the specified library was loaded but
 no function called "load" was found in it.  Providing the library contained
@@ -61,7 +102,11 @@ BIND 10 failed to open the specified hook library for the stated reason. The
 library has not been loaded.  BIND 10 will continue to function, but without
 the services offered by the library.
 
-% HOOKS_REGISTER_CALLOUT library %1 has registered a callout for hook '%2'
+% HOOKS_REGISTER_CALLOUT hooks library with index %1 registered callout for hook '%2'
+This is a debug message, output when a library (whose index in the list of
+libraries (being) loaded is given) registers a callout.
+
+% HOOKS_REGISTER_STD_CALLOUT hooks library %1 registered standard callout for hook %2
 This is a debug message, output when the library loading function has located
 a standard callout (a callout with the same name as a hook point) and
 registered it.
@@ -75,3 +120,6 @@ During the unloading of a library, an "unload" function was found.  It was
 called, but returned an error (non-zero) status, resulting in the issuing of
 this message.  The unload process continued after this message and the library
 has been unloaded.
+
+% HOOKS_UNLOAD_LIBRARY unloading library %1
+This is a debug message called when the specified library is being unloaded.

src/lib/hooks/library_manager.cc

@@ -102,6 +102,8 @@ LibraryManager::checkVersion() const {
         int version = (*pointers.ver_ptr)();
         if (version == BIND10_HOOKS_VERSION) {
             // All OK, version checks out
+            LOG_DEBUG(hooks_logger, HOOKS_DBG_CALLS, HOOKS_LIBRARY_VERSION)
+                      .arg(library_name_).arg(version);
             return (true);
 
         } else {
@@ -124,7 +126,7 @@ LibraryManager::registerStandardCallouts() {
     manager_->setLibraryIndex(index_);
     LibraryHandle library_handle(manager_.get());
 
-    // Iterate through the list of known hooks
+    // Iterate through the list of known hooksv
     vector<string> hook_names = ServerHooks::getServerHooks().getHookNames();
     for (int i = 0; i < hook_names.size(); ++i) {
 
@@ -141,8 +143,7 @@ LibraryManager::registerStandardCallouts() {
         pointers.dlsym_ptr = dlsym(dl_handle_, hook_names[i].c_str());
         if (pointers.callout_ptr != NULL) {
             // Found a symbol, so register it.
-            //library_handle.registerCallout(hook_names[i], callout_ptr);
-            LOG_DEBUG(hooks_logger, HOOKS_DBG_CALLS, HOOKS_REGISTER_CALLOUT)
+            LOG_DEBUG(hooks_logger, HOOKS_DBG_CALLS, HOOKS_REGISTER_STD_CALLOUT)
                 .arg(library_name_).arg(hook_names[i]);
             library_handle.registerCallout(hook_names[i], pointers.callout_ptr);
 
@@ -205,7 +206,7 @@ LibraryManager::runUnload() {
         void*               dlsym_ptr;
     } pointers;
 
-    // Zero the union, whatever the size of the pointers.
+    // Zero the union, whatever the relative size of the pointers.
     pointers.unload_ptr = NULL;
     pointers.dlsym_ptr = NULL;
 
@@ -233,5 +234,89 @@ LibraryManager::runUnload() {
     return (true);
 }
 
+// The main library loading function.
+
+bool
+LibraryManager::loadLibrary() {
+    LOG_DEBUG(hooks_logger, HOOKS_DBG_TRACE, HOOKS_LOAD_LIBRARY)
+        .arg(library_name_);
+
+    // In the following, if a method such as openLibrary() fails, it will
+    // have issued an error message so there is no need to issue another one
+    // here.
+
+    if (openLibrary()) {
+
+        // Library opened OK, see if a version function is present and if so,
+        // check it.
+        if (checkVersion()) {
+
+            // Version OK, so now register the standard callouts and call the
+            // librarie's load() function if present.
+            registerStandardCallouts();
+            if (runLoad()) {
+
+                // Success - the library has been successfully loaded.
+                LOG_INFO(hooks_logger, HOOKS_LIBRARY_LOADED).arg(library_name_);
+                return (true);
+            } else {
+
+                // The load function failed, so back out.  We can't just close
+                // the library as (a) we need to call the library's "unload"
+                // function (if present) in case "load" allocated resources that
+                // need to be freed and (b) - we need to remove any callouts
+                // that have been installed.
+                static_cast<void>(unloadLibrary());
+            }
+        } else {
+
+            // Version check failed so close the library and free up resources.
+            // Ignore the status return here - we already have an error
+            // consition.
+            static_cast<void>(closeLibrary());
+        }
+    }
+
+    return (false);
+}
+
+// The library unloading function.  Call the unload() function (if present),
+// remove callouts from the callout manager, then close the library.
+
+bool
+LibraryManager::unloadLibrary() {
+    LOG_DEBUG(hooks_logger, HOOKS_DBG_TRACE, HOOKS_UNLOAD_LIBRARY)
+        .arg(library_name_);
+
+    // Call the unload() function if present.  Note that this is done first -
+    // operations take place in the reverse order to which they were done when
+    // the library was loaded.
+    bool result = runUnload();
+
+    // Regardless of status, remove all callouts associated with this library
+    // on all hooks.
+    vector<string> hooks = ServerHooks::getServerHooks().getHookNames();
+    manager_->setLibraryIndex(index_);
+    for (int i = 0; i < hooks.size(); ++i) {
+        bool removed = manager_->deregisterAllCallouts(hooks[i]);
+        if (removed) {
+            LOG_DEBUG(hooks_logger, HOOKS_DBG_CALLS, HOOKS_CALLOUT_REMOVED)
+                .arg(hooks[i]).arg(library_name_);
+        }
+    }
+
+    // ... and close the library.
+    result = result && closeLibrary();
+    if (result) {
+
+        // Issue the informational message only if the library was unloaded
+        // with no problems.  If there was an issue, an error message would
+        // have been issued.
+        LOG_INFO(hooks_logger, HOOKS_LIBRARY_UNLOADED).arg(library_name_);
+    }
+
+    return (result);
+}
+
 } // namespace hooks
 } // namespace isc

src/lib/hooks/library_manager.h

@@ -97,7 +97,10 @@ public:
     ///
     /// Open the library and check the version.  If all is OK, load all
     /// standard symbols then call "load" if present.
-    bool loadLibrary() {return true;}
+    ///
+    /// @return true if the library loaded successfully, false otherwise.
+    ///         In the latter case, the library will be unloaded if possible.
+    bool loadLibrary();
 
     /// @brief Unloads a library
     ///
@@ -106,7 +109,12 @@ public:
     ///
     /// However, see the caveat in the class header about when it is safe
     /// to unload libraries.
-    bool unloadLibrary() {return false;}
+    ///
+    /// @return true if the library unloaded successfully, false if an error
+    ///         occurred in the process (most likely the unload() function
+    ///         (if present) returned an error).  Even if an error did occur,
+    ///         the library is closed if possible.
+    bool unloadLibrary();
 
     /// @brief Return library name
     ///

src/lib/hooks/tests/Makefile.am

@@ -28,7 +28,7 @@ TESTS =
 if HAVE_GTEST
 # Build shared libraries for testing.
 lib_LTLIBRARIES = libnvl.la libivl.la libbcl.la liblcl.la liblecl.la \
-                  libucl.la
+                  libucl.la libfcl.la
  
 # No version function
 libnvl_la_SOURCES  = no_version_library.cc
@@ -62,6 +62,11 @@ libucl_la_SOURCES  = unload_callout_library.cc
 libucl_la_CXXFLAGS = $(AM_CXXFLAGS)
 libucl_la_CPPFLAGS = $(AM_CPPFLAGS) $(LOG4CPLUS_INCLUDES)
 
+# The full callout library - contains all three framework functions.
+libfcl_la_SOURCES  = full_callout_library.cc
+libfcl_la_CXXFLAGS = $(AM_CXXFLAGS)
+libfcl_la_CPPFLAGS = $(AM_CPPFLAGS) $(LOG4CPLUS_INCLUDES)
+
 TESTS += run_unittests
 run_unittests_SOURCES  = run_unittests.cc
 run_unittests_SOURCES += callout_handle_unittest.cc

src/lib/hooks/tests/basic_callout_library.cc

@@ -38,9 +38,10 @@
 ///   "result".
 
 #include <hooks/hooks.h>
-#include <iostream>
+#include <fstream>
 
 using namespace isc::hooks;
+using namespace std;
 
 extern "C" {
 
@@ -62,10 +63,9 @@ lm_one(CalloutHandle& handle) {
     handle.getArgument("data_1", data);
 
     int result;
-    handle.getContext("result", result);
+    handle.getArgument("result", result);
 
     result += data;
-    handle.setContext("result", result);
     handle.setArgument("result", result);
 
     return (0);
@@ -80,10 +80,9 @@ lm_two(CalloutHandle& handle) {
     handle.getArgument("data_2", data);
 
     int result;
-    handle.getContext("result", result);
+    handle.getArgument("result", result);
 
     result *= data;
-    handle.setContext("result", result);
     handle.setArgument("result", result);
 
     return (0);
@@ -97,7 +96,7 @@ lm_three(CalloutHandle& handle) {
     handle.getArgument("data_3", data);
 
     int result;
-    handle.getContext("result", result);
+    handle.getArgument("result", result);
 
     result -= data;
     handle.setArgument("result", result);

src/lib/hooks/tests/library_manager_unittest.cc.in

@@ -23,6 +23,7 @@
 #include <algorithm>
 #include <fstream>
 #include <string>
+#include <iostream>
 
 #include <unistd.h>
 
@@ -50,7 +51,7 @@ public:
 
         // Set up the callout manager with these hooks.  Assume a maximum of
         // four libraries.
-        callout_manager_.reset(new CalloutManager(1));
+        callout_manager_.reset(new CalloutManager(4));
 
         // Set up the callout handle.
         callout_handle_.reset(new CalloutHandle(callout_manager_));
@@ -61,11 +62,71 @@ public:
 
     /// @brief Destructor
     ///
-    /// Ensures a marker file is removed after the test.
+    /// Ensures a marker file is removed after each test.
     ~LibraryManagerTest() {
         static_cast<void>(unlink(MARKER_FILE));
     }
 
+    /// @brief Call callouts test
+    ///
+    /// All of the loaded libraries have four callouts: a context_create
+    /// callout and three callouts that are attached to hooks lm_one,
+    /// lm_two and lm_three.
+    ///
+    /// context_create initializes the calculation by setting a seed
+    /// value of r0 say.
+    ///
+    /// Callout lm_one is passed a value d1 and performs a simple arithmetic
+    /// operation on it yielding a result r1.  Hence we can say that
+    /// @f[ r1 = lm1(r0, d1) @f]
+    ///
+    /// Callout lm_two is passed a value d2 and peforms another simple
+    /// arithmetic operation on it and d2, yielding r2, i.e.
+    /// @f[ r2 = lm2(d1, d2) @f]
+    ///
+    /// lm_three does a similar operation giving @f[ r3 = lm3(r2, d3) @f].
+    ///
+    /// The details of the operations lm1, lm2 and lm3 depend on the library.
+    /// However the sequence of calls needed to do this set of calculations
+    /// is identical regardless of the exact functions. This method performs
+    /// those operations and checks the results.
+    ///
+    /// It is assumed that callout_manager_ and callout_handle_ have been
+    /// set up appropriately.
+    ///
+    /// @param r0...r3, d1..d3 Values and intermediate values expected.  They
+    ///        are ordered so that the variables are used in the order left to
+    ///        right.
+    void executeCallCallouts(int r0, int d1, int r1, int d2, int r2, int d3,
+                             int r3) {
+        static const char* COMMON_TEXT = " callout returned the wong value";
+        int result;
+
+        // Seed the calculation.
+        callout_manager_->callCallouts(ServerHooks::CONTEXT_CREATE,
+                                       *callout_handle_);
+        callout_handle_->getArgument("result", result);
+        EXPECT_EQ(r0, result) << "context_create" << COMMON_TEXT;
+
+        // Perform the first calculation.
+        callout_handle_->setArgument("data_1", d1);
+        callout_manager_->callCallouts(lm_one_index_, *callout_handle_);
+        callout_handle_->getArgument("result", result);
+        EXPECT_EQ(r1, result) << "lm_one" << COMMON_TEXT;
+
+        // ... the second ...
+        callout_handle_->setArgument("data_2", d2);
+        callout_manager_->callCallouts(lm_two_index_, *callout_handle_);
+        callout_handle_->getArgument("result", result);
+        EXPECT_EQ(r2, result) << "lm_two" << COMMON_TEXT;
+
+        // ... and the third.
+        callout_handle_->setArgument("data_3", d3);
+        callout_manager_->callCallouts(lm_three_index_, *callout_handle_);
+        callout_handle_->getArgument("result", result);
+        EXPECT_EQ(r3, result) << "lm_three" << COMMON_TEXT;
+    }
+
     /// Hook indexes.  These are somewhat ubiquitous, so are made public for
     /// ease of reference instead of being accessible by a function.
     int lm_one_index_;
@@ -122,6 +183,7 @@ public:
 // .so file.  Note that we access the .so file - libtool creates this as a
 // like to the real shared library.
 static const char* BASIC_CALLOUT_LIBRARY = "@abs_builddir@/.libs/libbcl.so";
+static const char* FULL_CALLOUT_LIBRARY = "@abs_builddir@/.libs/libfcl.so";
 static const char* INCORRECT_VERSION_LIBRARY = "@abs_builddir@/.libs/libivl.so";
 static const char* LOAD_CALLOUT_LIBRARY = "@abs_builddir@/.libs/liblcl.so";
 static const char* LOAD_ERROR_CALLOUT_LIBRARY =
@@ -135,7 +197,7 @@ namespace {
 
 // Tests that OpenLibrary reports an error for an unknown library.
 
-TEST_F(LibraryManagerTest, NonExistentLibrary) {
+TEST_F(LibraryManagerTest, NoLibrary) {
     PublicLibraryManager lib_manager(std::string(NOT_PRESENT_LIBRARY),
                                      0, callout_manager_);
     EXPECT_FALSE(lib_manager.openLibrary());
@@ -157,7 +219,7 @@ TEST_F(LibraryManagerTest, OpenClose) {
 
 // Check that the code handles the case of a library with no version function.
 
-TEST_F(LibraryManagerTest, NoVersionFunction) {
+TEST_F(LibraryManagerTest, NoVersion) {
     PublicLibraryManager lib_manager(std::string(NO_VERSION_LIBRARY),
                                      0, callout_manager_);
     // Open should succeed.
@@ -173,7 +235,7 @@ TEST_F(LibraryManagerTest, NoVersionFunction) {
 // Check that the code handles the case of a library with a version function
 // that returns an incorrect version number.
 
-TEST_F(LibraryManagerTest, IncorrectVersionReturned) {
+TEST_F(LibraryManagerTest, WrongVersion) {
     PublicLibraryManager lib_manager(std::string(INCORRECT_VERSION_LIBRARY),
                                      0, callout_manager_);
     // Open should succeed.
@@ -218,30 +280,11 @@ TEST_F(LibraryManagerTest, RegisterStandardCallouts) {
     // Load the standard callouts
     EXPECT_NO_THROW(lib_manager.registerStandardCallouts());
 
-    int result = 0;
-
-    // Now execute the callouts in the order expected.  context_create
-    // always comes first.  This sets the context value to 10.
-    callout_manager_->callCallouts(ServerHooks::CONTEXT_CREATE,
-                                   *callout_handle_);
-
-    // First callout adds 5 to the context value.
-    callout_handle_->setArgument("data_1", static_cast<int>(5));
-    callout_manager_->callCallouts(lm_one_index_, *callout_handle_);
-    callout_handle_->getArgument("result", result);
-    EXPECT_EQ(15, result);
-
-    // Second callout multiples the running total by 7
-    callout_handle_->setArgument("data_2", static_cast<int>(7));
-    callout_manager_->callCallouts(lm_two_index_, *callout_handle_);
-    callout_handle_->getArgument("result", result);
-    EXPECT_EQ(105, result);
-
-    // Third callout subtracts 17 from the running total.
-    callout_handle_->setArgument("data_3", static_cast<int>(17));
-    callout_manager_->callCallouts(lm_three_index_, *callout_handle_);
-    callout_handle_->getArgument("result", result);
-    EXPECT_EQ(88, result);
+    // Now execute the callouts in the order expected.  The library performs
+    // the calculation:
+    //
+    // r3 = (10 + d1) * d2 - d3
+    executeCallCallouts(10, 5, 15, 7, 105, 17, 88);
 
     // Explicitly clear the callout_handle_ so that we can delete the library.
     // This is the only object to contain memory allocated by it.
@@ -288,29 +331,11 @@ TEST_F(LibraryManagerTest, CheckLoadCalled) {
     EXPECT_FALSE(callout_manager_->calloutsPresent(
                  ServerHooks::CONTEXT_DESTROY));
 
-    // Now execute the callouts in the order expected.
-    // only the first callout should be executed and the
-    // always comes first.  This sets the context value to 10.
-    callout_manager_->callCallouts(ServerHooks::CONTEXT_CREATE,
-                                   *callout_handle_);
-
-    // First callout multiplies the passed data by 5.
-    callout_handle_->setArgument("data_1", static_cast<int>(5));
-    callout_manager_->callCallouts(lm_one_index_, *callout_handle_);
-    callout_handle_->getArgument("result", result);
-    EXPECT_EQ(25, result);
-
-    // Second callout adds 7 to the stored data.
-    callout_handle_->setArgument("data_2", static_cast<int>(7));
-    callout_manager_->callCallouts(lm_two_index_, *callout_handle_);
-    callout_handle_->getArgument("result", result);
-    EXPECT_EQ(32, result);
-
-    // Third callout multiplies the running total by 10
-    callout_handle_->setArgument("data_3", static_cast<int>(10));
-    callout_manager_->callCallouts(lm_three_index_, *callout_handle_);
-    callout_handle_->getArgument("result", result);
-    EXPECT_EQ(320, result);
+    // Now execute the callouts in the order expected.  The library performs
+    // the calculation:
+    //
+    // r3 = (5 * d1 + d2) * d3
+    executeCallCallouts(5, 5, 25, 7, 32, 10, 320);
 
     // Explicitly clear the callout_handle_ so that we can delete the library.
     // This is the only object to contain memory allocated by it.
@@ -341,10 +366,6 @@ TEST_F(LibraryManagerTest, CheckLoadError) {
     EXPECT_TRUE(lib_manager.closeLibrary());
 }
 
-// TODO Check that unload is called.  This causes problems with testing
-// in that it can't communicate anything back to the caller.  So we'll
-// test a successful call by checking whether a marker file is created.
-
 // No unload function
 
 TEST_F(LibraryManagerTest, CheckNoUnload) {
@@ -387,7 +408,8 @@ TEST_F(LibraryManagerTest, CheckUnloadError) {
     EXPECT_TRUE(lib_manager.closeLibrary());
 }
 
-// Unload function works
+// Check that the case of the library's unload() function returning a
+// success is handled correcty.
 
 TEST_F(LibraryManagerTest, CheckUnload) {
 
@@ -421,4 +443,164 @@ TEST_F(LibraryManagerTest, CheckUnload) {
     EXPECT_TRUE(lib_manager.closeLibrary());
 }
 
+// Test the operation of unloadLibrary().  We load a library with a set
+// of callouts then unload it.  We need to check that the callouts have been
+// removed.  We'll also check that the library's unload() function was called
+// as well.
+
+TEST_F(LibraryManagerTest, LibUnload) {
+
+    // Load the only library, specifying the index of 0 as it's the only
+    // library.  This should load all callouts.
+    PublicLibraryManager lib_manager(std::string(FULL_CALLOUT_LIBRARY),
+                               0, callout_manager_);
+    EXPECT_TRUE(lib_manager.openLibrary());
+
+    // Check the version of the library.
+    EXPECT_TRUE(lib_manager.checkVersion());
+
+    // No callouts should be registered at the moment.
+    EXPECT_FALSE(callout_manager_->calloutsPresent(lm_one_index_));
+    EXPECT_FALSE(callout_manager_->calloutsPresent(lm_two_index_));
+    EXPECT_FALSE(callout_manager_->calloutsPresent(lm_three_index_));
+
+    // Load the single standard callout and check it is registered correctly.
+    EXPECT_NO_THROW(lib_manager.registerStandardCallouts());
+    EXPECT_TRUE(callout_manager_->calloutsPresent(lm_one_index_));
+    EXPECT_FALSE(callout_manager_->calloutsPresent(lm_two_index_));
+    EXPECT_FALSE(callout_manager_->calloutsPresent(lm_three_index_));
+
+    // Call the load function to load the other callouts.
+    EXPECT_TRUE(lib_manager.runLoad());
+    EXPECT_TRUE(callout_manager_->calloutsPresent(lm_one_index_));
+    EXPECT_TRUE(callout_manager_->calloutsPresent(lm_two_index_));
+    EXPECT_TRUE(callout_manager_->calloutsPresent(lm_three_index_));
+
+    // Unload the library and check that the callouts have been removed from
+    // the CalloutManager.
+    lib_manager.unloadLibrary();
+    EXPECT_FALSE(callout_manager_->calloutsPresent(lm_one_index_));
+    EXPECT_FALSE(callout_manager_->calloutsPresent(lm_two_index_));
+    EXPECT_FALSE(callout_manager_->calloutsPresent(lm_three_index_));
+}
+
+// Now come the loadLibrary() tests that make use of all the methods tested
+// above.  These tests are really to make sure that the methods have been
+// tied toget correctly.
+
+// First test the basic error cases - no library, no version function, version
+// function returning an error.
+
+TEST_F(LibraryManagerTest, LoadLibraryNoLibrary) {
+    LibraryManager lib_manager(std::string(NOT_PRESENT_LIBRARY), 0,
+                                           callout_manager_);
+    EXPECT_FALSE(lib_manager.loadLibrary());
+}
+
+// Check that the code handles the case of a library with no version function.
+
+TEST_F(LibraryManagerTest, LoadLibraryNoVersion) {
+    LibraryManager lib_manager(std::string(NO_VERSION_LIBRARY), 0,
+                                           callout_manager_);
+    EXPECT_FALSE(lib_manager.loadLibrary());
+}
+
+// Check that the code handles the case of a library with a version function
+// that returns an incorrect version number.
+
+TEST_F(LibraryManagerTest, LoadLibraryWrongVersion) {
+    LibraryManager lib_manager(std::string(INCORRECT_VERSION_LIBRARY), 0,
+                                           callout_manager_);
+    EXPECT_FALSE(lib_manager.loadLibrary());
+}
+
+// Check that the full loadLibrary call works.
+
+TEST_F(LibraryManagerTest, LoadLibrary) {
+    LibraryManager lib_manager(std::string(FULL_CALLOUT_LIBRARY), 0,
+                                           callout_manager_);
+    EXPECT_TRUE(lib_manager.loadLibrary());
+
+    // Now execute the callouts in the order expected.  The library performs
+    // the calculation:
+    //
+    // r3 = (7 * d1 - d2) * d3
+    executeCallCallouts(7, 5, 35, 9, 26, 3, 78);
+
+    // All done, so unload the library.  First we have to delete the
+    // CalloutHandle as it may contain memory allocated by that library.
+    callout_handle_.reset();
+
+    EXPECT_TRUE(lib_manager.unloadLibrary());
+
+    // Check that the callouts have been removed from the callout manager.
+    EXPECT_FALSE(callout_manager_->calloutsPresent(lm_one_index_));
+    EXPECT_FALSE(callout_manager_->calloutsPresent(lm_two_index_));
+    EXPECT_FALSE(callout_manager_->calloutsPresent(lm_three_index_));
+}
+
+// Now test for multiple libraries.  We'll load the full callout library
+// first, then load some of the libraries with missing framework functions.
+// This will check that when searching for framework functions, only the
+// specified library is checked, not other loaded libraries. We will
+// load a second library with suitable callouts and check that the callouts
+// are added correctly. Finally, we'll unload one of the libraries and
+// check that only the callouts belonging to that library were removed.
+
+TEST_F(LibraryManagerTest, LoadMultipleLibraries) {
+    // Load a library with all framework functions.
+    LibraryManager lib_manager_1(std::string(FULL_CALLOUT_LIBRARY), 0,
+                                 callout_manager_);
+    EXPECT_TRUE(lib_manager_1.loadLibrary());
+
+    // Attempt to load a library with no version() function.  We should detect
+    // this and not end up calling the function from the already loaded
+    // library.
+    LibraryManager lib_manager_2(std::string(NO_VERSION_LIBRARY), 1,
+                                 callout_manager_);
+    EXPECT_FALSE(lib_manager_2.loadLibrary());
+
+    // Attempt to load the library with an incorrect version.  This should
+    // be detected.
+    LibraryManager lib_manager_3(std::string(INCORRECT_VERSION_LIBRARY), 1,
+                                 callout_manager_);
+    EXPECT_FALSE(lib_manager_3.loadLibrary());
+
+    // Load the basic callout library.  This only has standard callouts so,
+    // if the first library's load() function gets called, some callouts
+    // will be registered twice and lead to incorrect results.
+    LibraryManager lib_manager_4(std::string(BASIC_CALLOUT_LIBRARY), 1,
+                                 callout_manager_);
+    EXPECT_TRUE(lib_manager_4.loadLibrary());
+
+    // Execute the callouts.  The first library implements the calculation.
+    //
+    // r3 = (7 * d1 - d2) * d3
+    // 
+    // The last-loaded library implements the calculation
+    //
+    // r3 = (10 + d1) * d2 - d3
+    //
+    // Putting the processing for each library together in the appropriate
+    // order, we get:
+    //
+    // r3 = ((10 * d1 + d1) - d2) * d2 * d3 - d3
+    executeCallCallouts(10, 3, 33, 2, 62, 3, 183);
+
+    // All done, so unload the first library.  First we have to delete the
+    // CalloutHandle as it may contain memory allocated by that library.
+    callout_handle_.reset();
+    EXPECT_TRUE(lib_manager_1.unloadLibrary());
+    //EXPECT_TRUE(lib_manager_4.unloadLibrary());
+
+    // Now execute the callouts again and check that the results are as
+    // expected for the new calculation.
+    callout_handle_.reset(new CalloutHandle(callout_manager_));
+    executeCallCallouts(10, 5, 15, 7, 105, 17, 88);
+
+    // ... and tidy up.
+    callout_handle_.reset();
+    EXPECT_TRUE(lib_manager_4.unloadLibrary());
+}
+
 } // Anonymous namespace

src/lib/hooks/tests/load_callout_library.cc

@@ -62,7 +62,6 @@ lm_one(CalloutHandle& handle) {
     handle.getContext("result", result);
 
     result *= data;
-    handle.setContext("result", result);
     handle.setArgument("result", result);
 
     return (0);
@@ -77,10 +76,9 @@ lm_nonstandard_two(CalloutHandle& handle) {
     handle.getArgument("data_2", data);
 
     int result;
-    handle.getContext("result", result);
+    handle.getArgument("result", result);
 
     result += data;
-    handle.setContext("result", result);
     handle.setArgument("result", result);
 
     return (0);
@@ -94,7 +92,7 @@ lm_nonstandard_three(CalloutHandle& handle) {
     handle.getArgument("data_3", data);
 
     int result;
-    handle.getContext("result", result);
+    handle.getArgument("result", result);
 
     result *= data;
     handle.setArgument("result", result);

src/lib/hooks/tests/unload_callout_library.cc

@@ -40,7 +40,8 @@ version() {
     return (BIND10_HOOKS_VERSION);
 }
 
-int unload() {
+int
+unload() {
     // Create the marker file.
     std::fstream marker;
     marker.open(MARKER_FILE, std::fstream::out);