[2980] Added conditional initialization.

src/lib/hooks/callout_handle.cc

@@ -27,8 +27,10 @@ namespace isc {
 namespace hooks {
 
 // Constructor.
-CalloutHandle::CalloutHandle(const boost::shared_ptr<CalloutManager>& manager)
-    : arguments_(), context_collection_(), manager_(manager), skip_(false) {
+CalloutHandle::CalloutHandle(const boost::shared_ptr<CalloutManager>& manager,
+                    const boost::shared_ptr<LibraryManagerCollection>& lmcoll)
+    : lm_collection_(lmcoll), arguments_(), context_collection_(),
+      manager_(manager), skip_(false) {
 
     // Call the "context_create" hook.  We should be OK doing this - although
     // the constructor has not finished running, all the member variables
@@ -43,6 +45,21 @@ CalloutHandle::~CalloutHandle() {
     // the destructor is being called, all the member variables are still in
     // existence.
     manager_->callCallouts(ServerHooks::CONTEXT_DESTROY, *this);
+
+    // Explicitly clear the argument and context objects.  This should free up
+    // all memory that could have been allocated by libraries that were loaded.
+    arguments_.clear();
+    context_collection_.clear();
+
+    // Normal destruction of the remaining variables will include the
+    // of the reference count on the library manager collection (which holds
+    // the libraries that could have allocated memory in the argument and
+    // context members).  When that goes to zero, the libraries will be
+    // unloaded: however, at that point nothing in the hooks framework will
+    // access memory in the libraries' address space.  It is possible that some
+    // other data structure in the server (the program using the hooks library)
+    // still references address space, but that is outside the scope of this
+    // framework.
 }
 
 // Return the name of all argument items.

src/lib/hooks/callout_handle.h

@@ -54,6 +54,7 @@ public:
 
 class CalloutManager;
 class LibraryHandle;
+class LibraryManagerCollection;
 
 /// @brief Per-packet callout handle
 ///
@@ -110,12 +111,27 @@ public:
     /// Creates the object and calls the callouts on the "context_create"
     /// hook.
     ///
+    /// Of the two arguments passed, only the pointer to the callout manager is
+    /// actively used.  The second argument, the pointer to the library manager
+    /// collection, is used for lifetime control: after use, the callout handle
+    /// may contain pointers to memory allocated by the loaded libraries.  The
+    /// used of a shared pointer to the collection of library managers means
+    /// that the libraries that could have allocated memory in a callout handle
+    /// will not be unloaded until all such handles have been destroyed.  This
+    /// issue is discussed in more detail in the documentation for
+    /// isc::hooks::LibraryManager.
+    ///
     /// @param manager Pointer to the callout manager object.
-    CalloutHandle(const boost::shared_ptr<CalloutManager>& manager);
+    /// @param lmcoll Pointer to the library manager collection.  This has a
+    ///        null default for testing purposes.
+    CalloutHandle(const boost::shared_ptr<CalloutManager>& manager,
+                  const boost::shared_ptr<LibraryManagerCollection>& lmcoll =
+                        boost::shared_ptr<LibraryManagerCollection>());
 
     /// @brief Destructor
     ///
     /// Calls the context_destroy callback to release any per-packet context.
+    /// It also clears stored data to avoid problems during member destruction.
     ~CalloutHandle();
 
     /// @brief Set argument
@@ -339,6 +355,10 @@ private:
     const ElementCollection& getContextForLibrary() const;
 
     // Member variables
+    
+    /// Pointer to the collection of libraries for which this handle has been
+    /// created.
+    boost::shared_ptr<LibraryManagerCollection> lm_collection_;
 
     /// Collection of arguments passed to the callouts
     ElementCollection arguments_;

src/lib/hooks/hooks_manager.cc

@@ -12,9 +12,6 @@
 // OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR
 // PERFORMANCE OF THIS SOFTWARE.
 
-// TODO - This is a temporary implementation of the hooks manager - it is
-//        likely to be completely rewritte
-
 #include <hooks/callout_handle.h>
 #include <hooks/callout_manager.h>
 #include <hooks/callout_manager.h>
@@ -46,34 +43,6 @@ HooksManager::getHooksManager() {
     return (manager);
 }
 
-// Perform conditional initialization if nothing is loaded.
-
-void
-HooksManager::performConditionalInitialization() {
-
-    // Nothing present, so create the collection with any empty set of
-    // libraries, and get the CalloutManager.
-    vector<string> libraries;
-    lm_collection_.reset(new LibraryManagerCollection(libraries));
-    lm_collection_->loadLibraries();
-
-    callout_manager_ = lm_collection_->getCalloutManager();
-}
-
-// Create a callout handle
-
-boost::shared_ptr<CalloutHandle>
-HooksManager::createCalloutHandleInternal() {
-    conditionallyInitialize();
-    return (boost::shared_ptr<CalloutHandle>(
-                             new CalloutHandle(callout_manager_)));
-}
-
-boost::shared_ptr<CalloutHandle>
-HooksManager::createCalloutHandle() {
-    return (getHooksManager().createCalloutHandleInternal());
-}
-
 // Are callouts present?
 
 bool
@@ -141,7 +110,40 @@ void HooksManager::unloadLibraries() {
     getHooksManager().unloadLibrariesInternal();
 }
 
+// Create a callout handle
+
+boost::shared_ptr<CalloutHandle>
+HooksManager::createCalloutHandleInternal() {
+    conditionallyInitialize();
+    return (boost::shared_ptr<CalloutHandle>(
+            new CalloutHandle(callout_manager_, lm_collection_)));
+}
+
+boost::shared_ptr<CalloutHandle>
+HooksManager::createCalloutHandle() {
+    return (getHooksManager().createCalloutHandleInternal());
+}
+
+// Perform conditional initialization if nothing is loaded.
 
+void
+HooksManager::performConditionalInitialization() {
+
+    // Nothing present, so create the collection with any empty set of
+    // libraries, and get the CalloutManager.
+    vector<string> libraries;
+    lm_collection_.reset(new LibraryManagerCollection(libraries));
+    lm_collection_->loadLibraries();
+
+    callout_manager_ = lm_collection_->getCalloutManager();
+}
+
+// Shell around ServerHooks::registerHook()
+
+int
+HooksManager::registerHook(const std::string& name) {
+    return (ServerHooks::getServerHooks().registerHook(name));
+}
 
 } // namespace util
 } // namespace isc

src/lib/hooks/hooks_manager.h

@@ -15,6 +15,8 @@
 #ifndef HOOKS_MANAGER_H
 #define HOOKS_MANAGER_H
 
+#include <hooks/server_hooks.h>
+
 #include <boost/noncopyable.hpp>
 #include <boost/shared_ptr.hpp>
 
@@ -153,6 +155,28 @@ public:
     /// @return Shared pointer to a CalloutHandle object.
     static boost::shared_ptr<CalloutHandle> createCalloutHandle();
 
+    /// @brief Register Hook
+    ///
+    /// This is just a convenience shell around the ServerHooks::registerHook()
+    /// method.  It - along with the definitions of the two hook indexes for
+    /// the context_create and context_destroy methods - means that server
+    /// authors only need to deal with HooksManager and CalloutHandle, and not
+    /// include any other hooks framework classes.
+    ///
+    /// @param name Name of the hook
+    ///
+    /// @return Index of the hook, to be used in subsequent hook-related calls.
+    ///         This will be greater than or equal to zero (so allowing a
+    ///         negative value to indicate an invalid index).
+    ///
+    /// @throws DuplicateHook A hook with the same name has already been
+    ///         registered.
+    static int registerHook(const std::string& name);
+
+    /// Index numbers for pre-defined hooks.
+    static const int CONTEXT_CREATE = ServerHooks::CONTEXT_CREATE;
+    static const int CONTEXT_DESTROY = ServerHooks::CONTEXT_DESTROY;
+
 private:
 
     /// @brief Constructor

src/lib/hooks/tests/hooks_manager_unittest.cc

@@ -14,6 +14,7 @@
 
 #include <hooks/callout_handle.h>
 #include <hooks/hooks_manager.h>
+#include <hooks/server_hooks.h>
 
 #include <hooks/tests/common_test_class.h>
 #include <hooks/tests/test_libraries.h>
@@ -126,7 +127,7 @@ TEST_F(HooksManagerTest, LoadLibraries) {
     //
     // r3 = ((10 * d1 + d1) - d2) * d2 * d3 - d3
     {
-        SCOPED_TRACE("Doing calculation with libraries loaded");
+        SCOPED_TRACE("Calculation with libraries loaded");
         executeCallCallouts(10, 3, 33, 2, 62, 3, 183);
     }
 
@@ -136,7 +137,7 @@ TEST_F(HooksManagerTest, LoadLibraries) {
     // Re-execute the calculation - callouts can be called but as nothing
     // happens, the result should always be -1.
     {
-        SCOPED_TRACE("Doing calculation with libraries not loaded");
+        SCOPED_TRACE("Calculation with libraries not loaded");
         executeCallCallouts(-1, 3, -1, 22, -1, 83, -1);
     }
 }
@@ -170,7 +171,7 @@ TEST_F(HooksManagerTest, LoadLibrariesWithError) {
     //
     // r3 = ((10 * d1 + d1) - d2) * d2 * d3 - d3
     {
-        SCOPED_TRACE("Doing calculation with libraries loaded");
+        SCOPED_TRACE("Calculation with libraries loaded");
         executeCallCallouts(10, 3, 33, 2, 62, 3, 183);
     }
 
@@ -180,11 +181,157 @@ TEST_F(HooksManagerTest, LoadLibrariesWithError) {
     // Re-execute the calculation - callouts can be called but as nothing
     // happens, the result should always be -1.
     {
-        SCOPED_TRACE("Doing calculation with libraries not loaded");
+        SCOPED_TRACE("Calculation with libraries not loaded");
         executeCallCallouts(-1, 3, -1, 22, -1, 83, -1);
     }
 }
 
+// Test that we can unload a set of libraries while we have a CalloutHandle
+// created on them in existence, and can delete the handle afterwards.
+
+TEST_F(HooksManagerTest, CalloutHandleUnloadLibrary) {
+
+    // Set up the list of libraries to be loaded.
+    std::vector<std::string> library_names;
+    library_names.push_back(std::string(FULL_CALLOUT_LIBRARY));
+
+    // Load the libraries.
+    EXPECT_TRUE(HooksManager::loadLibraries(library_names));
+
+    // Execute the callouts.  Thiis library implements:
+    //
+    // r3 = (7 * d1 - d2) * d3
+    {
+        SCOPED_TRACE("Calculation with full callout library loaded");
+        executeCallCallouts(7, 4, 28, 8, 20, 2, 40);
+    }
+
+    // Get an outstanding callout handle on this library.
+    CalloutHandlePtr handle = HooksManager::createCalloutHandle();
+
+    // Execute once of the callouts again to ensure that the handle contains
+    // memory allocated by the library.
+    HooksManager::callCallouts(ServerHooks::CONTEXT_CREATE, *handle);
+
+    // Unload the libraries.
+    HooksManager::unloadLibraries();
+
+    // Deleting the callout handle should not cause a segmentation fault.
+    handle.reset();
+}
+
+// Test that we can load a new set of libraries while we have a CalloutHandle
+// created on them in existence, and can delete the handle afterwards.
+
+TEST_F(HooksManagerTest, CalloutHandleLoadLibrary) {
+
+    // Set up the list of libraries to be loaded.
+    std::vector<std::string> library_names;
+    library_names.push_back(std::string(FULL_CALLOUT_LIBRARY));
+
+    // Load the libraries.
+    EXPECT_TRUE(HooksManager::loadLibraries(library_names));
+
+    // Execute the callouts.  Thiis library implements:
+    //
+    // r3 = (7 * d1 - d2) * d3
+    {
+        SCOPED_TRACE("Calculation with full callout library loaded");
+        executeCallCallouts(7, 4, 28, 8, 20, 2, 40);
+    }
+
+    // Get an outstanding callout handle on this library and execute one of
+    // the callouts again to ensure that the handle contains memory allocated
+    // by the library.
+    CalloutHandlePtr handle = HooksManager::createCalloutHandle();
+    HooksManager::callCallouts(ServerHooks::CONTEXT_CREATE, *handle);
+
+    // Load a new library that implements the calculation
+    //
+    // r3 = (10 + d1) * d2 - d3
+    std::vector<std::string> new_library_names;
+    new_library_names.push_back(std::string(BASIC_CALLOUT_LIBRARY));
+
+    // Load the libraries.
+    EXPECT_TRUE(HooksManager::loadLibraries(new_library_names));
+
+    // Execute the calculation.  Note that we still have the CalloutHandle
+    // for the old library: however, this should not affect the new calculation.
+    {
+        SCOPED_TRACE("Calculation with basic callout library loaded");
+        executeCallCallouts(10, 7, 17, 3, 51, 16, 35);
+    }
+
+    // Deleting the old callout handle should not cause a segmentation fault.
+    handle.reset();
+}
+
+// This is effectively the same test as the LoadLibraries test.
+
+TEST_F(HooksManagerTest, ReloadSameLibraries) {
+
+    // Set up the list of libraries to be loaded.
+    std::vector<std::string> library_names;
+    library_names.push_back(std::string(FULL_CALLOUT_LIBRARY));
+    library_names.push_back(std::string(BASIC_CALLOUT_LIBRARY));
+
+    // Load the libraries.
+    EXPECT_TRUE(HooksManager::loadLibraries(library_names));
+
+    // Execute the callouts.  See the LoadLibraries test for an explanation of
+    // the calculation.
+    {
+        SCOPED_TRACE("Calculation with libraries loaded");
+        executeCallCallouts(10, 3, 33, 2, 62, 3, 183);
+    }
+
+    // Try reloading the libraries and re-execute the calculation - we should
+    // get the same results.
+    EXPECT_NO_THROW(HooksManager::loadLibraries(library_names));
+    {
+        SCOPED_TRACE("Calculation with libraries reloaded");
+        executeCallCallouts(10, 3, 33, 2, 62, 3, 183);
+    }
+}
+
+TEST_F(HooksManagerTest, ReloadLibrariesReverseOrder) {
+
+    // Set up the list of libraries to be loaded and load them.
+    std::vector<std::string> library_names;
+    library_names.push_back(std::string(FULL_CALLOUT_LIBRARY));
+    library_names.push_back(std::string(BASIC_CALLOUT_LIBRARY));
+    EXPECT_TRUE(HooksManager::loadLibraries(library_names));
+
+    // 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 given order
+    // gives.
+    //
+    // r3 = ((10 * d1 + d1) - d2) * d2 * d3 - d3
+    {
+        SCOPED_TRACE("Calculation with libraries loaded");
+        executeCallCallouts(10, 3, 33, 2, 62, 3, 183);
+    }
+
+    // Reload the libraries in the reverse order.
+    std::reverse(library_names.begin(), library_names.end());
+    EXPECT_TRUE(HooksManager::loadLibraries(library_names));
+
+    // The calculation in the reverse order gives:
+    //
+    // r3 = ((((7 + d1) * d1) * d2 - d2) - d3) * d3
+    {
+        SCOPED_TRACE("Calculation with libraries loaded in reverse order");
+        executeCallCallouts(7, 3, 30, 3, 87, 7, 560);
+    }
+}
+
 // Check that everything works even with no libraries loaded.  First that
 // calloutsPresent() always returns false.
 
@@ -199,4 +346,40 @@ TEST_F(HooksManagerTest, NoLibrariesCallCallouts) {
     executeCallCallouts(-1, 3, -1, 22, -1, 83, -1);
 }
 
+// Test the encapsulation of the ServerHooks::registerHook() method.
+
+TEST_F(HooksManagerTest, RegisterHooks) {
+    ServerHooks::getServerHooks().reset();
+    EXPECT_EQ(2, ServerHooks::getServerHooks().getCount());
+
+    // Check that the hook indexes are as expected. (Use temporary variables
+    // as it appears that Google test can't access the constants.)
+    int sh_cc = ServerHooks::CONTEXT_CREATE;
+    int hm_cc = HooksManager::CONTEXT_CREATE;
+    EXPECT_EQ(sh_cc, hm_cc);
+
+    int sh_cd = ServerHooks::CONTEXT_DESTROY;
+    int hm_cd = HooksManager::CONTEXT_DESTROY;
+    EXPECT_EQ(sh_cd, hm_cd);
+
+    // Register a few hooks and check we have the indexes as expected.
+    EXPECT_EQ(2, HooksManager::registerHook(string("alpha")));
+    EXPECT_EQ(3, HooksManager::registerHook(string("beta")));
+    EXPECT_EQ(4, HooksManager::registerHook(string("gamma")));
+    EXPECT_THROW(static_cast<void>(HooksManager::registerHook(string("alpha"))),
+                 DuplicateHook);
+
+    // ... an check the hooks are as we expect.
+    EXPECT_EQ(5, ServerHooks::getServerHooks().getCount());
+    vector<string> names = ServerHooks::getServerHooks().getHookNames();
+    sort(names.begin(), names.end());
+
+    EXPECT_EQ(string("alpha"), names[0]);
+    EXPECT_EQ(string("beta"), names[1]);
+    EXPECT_EQ(string("context_create"), names[2]);
+    EXPECT_EQ(string("context_destroy"), names[3]);
+    EXPECT_EQ(string("gamma"), names[4]);
+}
+
+
 } // Anonymous namespace