[master] Merged trac5095 (unloadLibraries removes preCalloutsLibraryHandle)

src/bin/dhcp4/tests/hooks_unittest.cc

@@ -1,4 +1,4 @@
-// Copyright (C) 2015-2016 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2015-2017 Internet Systems Consortium, Inc. ("ISC")
 //
 // This Source Code Form is subject to the terms of the Mozilla Public
 // License, v. 2.0. If a copy of the MPL was not distributed with this
@@ -13,6 +13,7 @@
 #include <config/command_mgr.h>
 #include <hooks/server_hooks.h>
 #include <hooks/hooks_manager.h>
+#include <hooks/callout_manager.h>
 #include <dhcpsrv/cfgmgr.h>
 #include <dhcp/tests/iface_mgr_test_config.h>
 #include <dhcp/option.h>
@@ -111,6 +112,7 @@ public:
         HooksManager::preCalloutsLibraryHandle().deregisterAllCallouts("lease4_release");
         HooksManager::preCalloutsLibraryHandle().deregisterAllCallouts("lease4_decline");
 
+        HooksManager::getSharedCalloutManager().reset();
         delete srv_;
     }
 
@@ -1603,16 +1605,13 @@ TEST_F(HooksDhcpv4SrvTest, lease4ReleaseSkip) {
 }
 
 // Checks that decline4 hooks (lease4_decline) are triggered properly.
-/// @todo: There is a bug in HooksManager that causes the callouts installed
-/// using preCalloutsLibraryHandle() to be uninstalled when loadLibrary
-/// is called. This has changed recently (ticket #5031) as it calls the
-/// load/unload every time, regardless if the hooks-libraries clause is in the
-/// config file or not. #5095 has been submitted for this issue. Please
-/// enable this test once #5095 is fixed.
-TEST_F(HooksDhcpv4SrvTest, DISABLED_HooksDecline) {
+TEST_F(HooksDhcpv4SrvTest, HooksDecline) {
     IfaceMgrTestConfig test_config(true);
     IfaceMgr::instance().openSockets4();
 
+    // Libraries will be reloaded later
+    HooksManager::getSharedCalloutManager().reset(new CalloutManager(0));
+
     // Install a callout
     EXPECT_NO_THROW(HooksManager::preCalloutsLibraryHandle().registerCallout(
                         "lease4_decline", lease4_decline_callout));
@@ -1655,12 +1654,13 @@ TEST_F(HooksDhcpv4SrvTest, DISABLED_HooksDecline) {
 }
 
 // Checks that decline4 hook is able to drop the packet.
-/// @todo See HooksDhcpv4SrvTest.HooksDecline description for details.
-/// Please reenable this once #5095 is fixed.
-TEST_F(HooksDhcpv4SrvTest, DISABLED_HooksDeclineDrop) {
+TEST_F(HooksDhcpv4SrvTest, HooksDeclineDrop) {
     IfaceMgrTestConfig test_config(true);
     IfaceMgr::instance().openSockets4();
 
+    // Libraries will be reloaded later
+    HooksManager::getSharedCalloutManager().reset(new CalloutManager(0));
+
     // Install a callout
     EXPECT_NO_THROW(HooksManager::preCalloutsLibraryHandle().registerCallout(
                         "lease4_decline", lease4_decline_drop_callout));

src/bin/dhcp6/tests/hooks_unittest.cc

@@ -1,4 +1,4 @@
-// Copyright (C) 2013-2016 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2013-2017 Internet Systems Consortium, Inc. ("ISC")
 //
 // This Source Code Form is subject to the terms of the Mozilla Public
 // License, v. 2.0. If a copy of the MPL was not distributed with this
@@ -18,6 +18,7 @@
 #include <util/buffer.h>
 #include <util/range_utilities.h>
 #include <hooks/server_hooks.h>
+#include <hooks/callout_manager.h>
 
 #include <dhcp6/tests/dhcp6_test_utils.h>
 #include <dhcp6/tests/dhcp6_client.h>
@@ -113,10 +114,17 @@ public:
 
         // Clear static buffers
         resetCalloutBuffers();
+
+        // Reset the hook system in its original state
+        HooksManager::unloadLibraries();
     }
 
     /// @brief destructor (deletes Dhcpv6Srv)
     ~HooksDhcpv6SrvTest() {
+
+        // Clear shared manager
+        HooksManager::getSharedCalloutManager().reset();
+
     }
 
     /// @brief creates an option with specified option code
@@ -2079,10 +2087,12 @@ TEST_F(HooksDhcpv6SrvTest, skipLease6Rebind) {
 
 // This test checks that the basic decline hook (lease6_decline) is
 // triggered properly.
-/// @todo: Reenable this once #5095 is fixed.
-TEST_F(HooksDhcpv6SrvTest, DISABLED_basicLease6Decline) {
+TEST_F(HooksDhcpv6SrvTest, basicLease6Decline) {
     IfaceMgrTestConfig test_config(true);
 
+    // Libraries will be reloaded later
+    HooksManager::getSharedCalloutManager().reset(new CalloutManager(0));
+
     // Install lease6_decline callout
     EXPECT_NO_THROW(HooksManager::preCalloutsLibraryHandle().registerCallout(
                         "lease6_decline", lease6_decline_callout));
@@ -2127,10 +2137,12 @@ TEST_F(HooksDhcpv6SrvTest, DISABLED_basicLease6Decline) {
 }
 
 // Test that the lease6_decline hook point can handle SKIP status.
-/// @todo: Reenable this once #5095 is fixed.
-TEST_F(HooksDhcpv6SrvTest, DISABLED_lease6DeclineSkip) {
+TEST_F(HooksDhcpv6SrvTest, lease6DeclineSkip) {
     IfaceMgrTestConfig test_config(true);
 
+    // Libraries will be reloaded later
+    HooksManager::getSharedCalloutManager().reset(new CalloutManager(0));
+
     // Install lease6_decline callout. It will set the status to skip
     EXPECT_NO_THROW(HooksManager::preCalloutsLibraryHandle().registerCallout(
                         "lease6_decline", lease6_decline_skip_callout));
@@ -2172,10 +2184,12 @@ TEST_F(HooksDhcpv6SrvTest, DISABLED_lease6DeclineSkip) {
 }
 
 // Test that the lease6_decline hook point can handle DROP status.
-/// @todo: Reenable this once #5095 is fixed.
-TEST_F(HooksDhcpv6SrvTest, DISABLED_lease6DeclineDrop) {
+TEST_F(HooksDhcpv6SrvTest, lease6DeclineDrop) {
     IfaceMgrTestConfig test_config(true);
 
+    // Libraries will be reloaded later
+    HooksManager::getSharedCalloutManager().reset(new CalloutManager(0));
+
     // Install lease6_decline callout. It will set the status to skip
     EXPECT_NO_THROW(HooksManager::preCalloutsLibraryHandle().registerCallout(
                         "lease6_decline", lease6_decline_drop_callout));

src/lib/hooks/callout_manager.cc

@@ -1,4 +1,4 @@
-// Copyright (C) 2013-2015 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2013-2015,2017 Internet Systems Consortium, Inc. ("ISC")
 //
 // This Source Code Form is subject to the terms of the Mozilla Public
 // License, v. 2.0. If a copy of the MPL was not distributed with this
@@ -184,7 +184,7 @@ CalloutManager::callCallouts(int hook_index, CalloutHandle& callout_handle) {
             .arg(server_hooks_.getName(current_hook_))
             .arg(stopwatch.logFormatTotalDuration());
 
-        // Reset the current hook and library indexs to an invalid value to
+        // Reset the current hook and library indexes to an invalid value to
         // catch any programming errors.
         current_hook_ = -1;
         current_library_ = -1;

src/lib/hooks/hooks_manager.cc

@@ -1,4 +1,4 @@
-// Copyright (C) 2013-2016 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2013-2017 Internet Systems Consortium, Inc. ("ISC")
 //
 // This Source Code Form is subject to the terms of the Mozilla Public
 // License, v. 2.0. If a copy of the MPL was not distributed with this
@@ -199,5 +199,11 @@ HooksManager::validateLibraries(const std::vector<std::string>& libraries) {
     return (LibraryManagerCollection::validateLibraries(libraries));
 }
 
+// Shared callout manager
+boost::shared_ptr<CalloutManager>&
+HooksManager::getSharedCalloutManager() {
+    return (getHooksManager().shared_callout_manager_);
+}
+
 } // namespace util
 } // namespace isc

src/lib/hooks/hooks_manager.h

@@ -1,4 +1,4 @@
-// Copyright (C) 2013-2016 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2013-2017 Internet Systems Consortium, Inc. ("ISC")
 //
 // This Source Code Form is subject to the terms of the Mozilla Public
 // License, v. 2.0. If a copy of the MPL was not distributed with this
@@ -199,6 +199,14 @@ public:
     static const int CONTEXT_CREATE = ServerHooks::CONTEXT_CREATE;
     static const int CONTEXT_DESTROY = ServerHooks::CONTEXT_DESTROY;
 
+    /// @brief Return the shared callout manager
+    ///
+    /// Declared as static as other methods but only one for the
+    /// singleton will be created.
+    ///
+    /// @return A reference to the shared callout manager
+    static boost::shared_ptr<CalloutManager>& getSharedCalloutManager();
+
 private:
 
     /// @brief Constructor
@@ -312,6 +320,10 @@ private:
 
     /// Callout manager for the set of library managers.
     boost::shared_ptr<CalloutManager> callout_manager_;
+
+    /// Shared callout manager to survive library reloads.
+    boost::shared_ptr<CalloutManager> shared_callout_manager_;
+
 };
 
 } // namespace util

src/lib/hooks/library_handle.h

@@ -1,4 +1,4 @@
-// Copyright (C) 2013-2016 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2013-2017 Internet Systems Consortium, Inc. ("ISC")
 //
 // This Source Code Form is subject to the terms of the Mozilla Public
 // License, v. 2.0. If a copy of the MPL was not distributed with this
@@ -135,10 +135,10 @@ public:
     ///    }
     ///]
     ///
-    /// The first library has no parameters, so regardles of the name
+    /// The first library has no parameters, so regardless of the name
     /// specified, for that library getParameter will always return NULL.
     ///
-    /// For the second paramter, depending the following calls will return:
+    /// For the second parameter, depending the following calls will return:
     /// - x = getParameter("mail") will return instance of
     ///   isc::data::StringElement. The content can be accessed with
     ///   x->stringValue() and will return std::string.

src/lib/hooks/library_manager_collection.cc

@@ -1,10 +1,11 @@
-// Copyright (C) 2013-2016 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2013-2017 Internet Systems Consortium, Inc. ("ISC")
 //
 // This Source Code Form is subject to the terms of the Mozilla Public
 // License, v. 2.0. If a copy of the MPL was not distributed with this
 // file, You can obtain one at http://mozilla.org/MPL/2.0/.
 
 #include <hooks/callout_manager.h>
+#include <hooks/hooks_manager.h>
 #include <hooks/library_manager.h>
 #include <hooks/library_manager_collection.h>
 
@@ -52,17 +53,34 @@ LibraryManagerCollection::loadLibraries() {
     // Unload libraries if any are loaded.
     static_cast<void>(unloadLibraries());
 
-    // Create the callout manager.  A pointer to this is maintained by
-    // each library.  Note that the callout manager does not hold any memory
-    // allocated by a library: although a library registers a callout (and so
-    // causes the creation of an entry in the CalloutManager's callout list),
-    // that creation is done by the CalloutManager itself.  The CalloutManager
-    // is created within the server.
+    // Access the callout manager, (re)creating it if required.
     //
-    // The upshot of this is that it is therefore safe for the CalloutManager
-    // to be deleted after all associated libraries are deleted, hence this
-    // link (LibraryManager -> CalloutManager) is safe.
-    callout_manager_.reset(new CalloutManager(library_names_.size()));
+    // A pointer to the callout manager is maintained by each as well as by
+    // the HooksManager itself.  Note that the callout manager does not hold any
+    // memory allocated by a library: although a library registers a callout
+    // (and so causes the creation of an entry in the CalloutManager's callout
+    // list), that creation is done by the CalloutManager itself.  The
+    // CalloutManager is created within the server. The upshot of this is that
+    // it is therefore safe for the CalloutManager to be deleted after all
+    // associated libraries are deleted, hence this link (LibraryManager ->
+    // CalloutManager) is safe.
+    //
+    // If the list of libraries is not empty, re-create the callout manager.
+    // This deletes all callouts (including the pre-library and post-
+    // library) ones.  It is up to the libraries to re-register their callouts.
+    //  The pre-library and post-library callouts will also need to be
+    // re-registered.
+    //
+    // If the list of libraries stays empty (as in the case of a reconfiguration
+    // where the hooks-libraries clause was empty and is not changed), try
+    // to re-use the existing callout manager (so retaining registered pre-
+    // and post-library callouts).
+    if (library_names_.empty()) {
+        callout_manager_ = HooksManager::getSharedCalloutManager();
+    }
+    if (!library_names_.empty() || !callout_manager_) {
+        callout_manager_.reset(new CalloutManager(library_names_.size()));
+    }
 
     // Now iterate through the libraries are load them one by one.  We'll
     for (size_t i = 0; i < library_names_.size(); ++i) {

src/lib/hooks/library_manager_collection.h

@@ -1,4 +1,4 @@
-// Copyright (C) 2013-2016 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2013-2017 Internet Systems Consortium, Inc. ("ISC")
 //
 // This Source Code Form is subject to the terms of the Mozilla Public
 // License, v. 2.0. If a copy of the MPL was not distributed with this
@@ -98,7 +98,7 @@ public:
     ///
     /// Returns a callout manager that can be used with this set of loaded
     /// libraries (even if the number of loaded libraries is zero).  This
-    /// method may only be caslled after loadLibraries() has been called.
+    /// method may only be called after loadLibraries() has been called.
     ///
     /// @return Pointer to a callout manager for this set of libraries.
     ///

src/lib/hooks/tests/hooks_manager_unittest.cc

@@ -1,4 +1,4 @@
-// Copyright (C) 2013-2016 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2013-2017 Internet Systems Consortium, Inc. ("ISC")
 //
 // This Source Code Form is subject to the terms of the Mozilla Public
 // License, v. 2.0. If a copy of the MPL was not distributed with this
@@ -40,9 +40,10 @@ public:
 
     /// @brief Destructor
     ///
-    /// Unload all libraries.
+    /// Unload all libraries and reset the shared manager.
     ~HooksManagerTest() {
         HooksManager::unloadLibraries();
+        HooksManager::getSharedCalloutManager().reset();
     }
 
 
@@ -391,6 +392,144 @@ TEST_F(HooksManagerTest, PrePostCalloutTest) {
     EXPECT_EQ(-15, result);
 }
 
+// Test with a shared manager the pre- and post- callout functions survive
+// a reload
+
+TEST_F(HooksManagerTest, PrePostCalloutShared) {
+
+    HookLibsCollection library_names;
+
+    // Initialize the shared manager.
+    HooksManager::getSharedCalloutManager().reset(new CalloutManager(0));
+
+    // Load the pre- and post- callouts.
+    HooksManager::preCalloutsLibraryHandle().registerCallout("hookpt_two",
+                                                             testPreCallout);
+    HooksManager::postCalloutsLibraryHandle().registerCallout("hookpt_two",
+                                                              testPostCallout);
+
+    // With the pre- and post- callouts above, the result expected is
+    //
+    // 1027 * 2
+    CalloutHandlePtr handle = HooksManager::createCalloutHandle();
+    handle->setArgument("result", static_cast<int>(0));
+    handle->setArgument("data_2", static_cast<int>(15));
+
+    HooksManager::callCallouts(hookpt_two_index_, *handle);
+
+    int result = 0;
+    handle->getArgument("result", result);
+    EXPECT_EQ(2054, result);
+
+    // ... and check that the pre- and post- callout functions survive a
+    // reload.
+    EXPECT_TRUE(HooksManager::loadLibraries(library_names));
+    handle = HooksManager::createCalloutHandle();
+
+    handle->setArgument("result", static_cast<int>(0));
+    handle->setArgument("data_2", static_cast<int>(15));
+
+    HooksManager::callCallouts(hookpt_two_index_, *handle);
+
+    // Expect same value i.e. 1027 * 2 
+    result = 0;
+    handle->getArgument("result", result);
+    EXPECT_EQ(2054, result);
+}
+
+// Test with a shared manager the pre- and post- callout functions survive
+// a reload but not with a not empty list of libraries
+
+TEST_F(HooksManagerTest, PrePostCalloutSharedNotEmpty) {
+
+    HookLibsCollection library_names;
+    library_names.push_back(make_pair(std::string(FULL_CALLOUT_LIBRARY),
+                                      data::ConstElementPtr()));
+
+    // Initialize the shared manager.
+    HooksManager::getSharedCalloutManager().reset(new CalloutManager(0));
+
+    // Load the pre- and post- callouts.
+    HooksManager::preCalloutsLibraryHandle().registerCallout("hookpt_two",
+                                                             testPreCallout);
+    HooksManager::postCalloutsLibraryHandle().registerCallout("hookpt_two",
+                                                              testPostCallout);
+
+    // With the pre- and post- callouts above, the result expected is
+    //
+    // 1027 * 2
+    CalloutHandlePtr handle = HooksManager::createCalloutHandle();
+    handle->setArgument("result", static_cast<int>(0));
+    handle->setArgument("data_2", static_cast<int>(15));
+
+    HooksManager::callCallouts(hookpt_two_index_, *handle);
+
+    int result = 0;
+    handle->getArgument("result", result);
+    EXPECT_EQ(2054, result);
+
+    // ... and check that the pre- and post- callout functions don't survive a
+    // reload with a not empty list of libraries.
+    EXPECT_TRUE(HooksManager::loadLibraries(library_names));
+    handle = HooksManager::createCalloutHandle();
+
+    handle->setArgument("result", static_cast<int>(0));
+    handle->setArgument("data_2", static_cast<int>(15));
+
+    HooksManager::callCallouts(hookpt_two_index_, *handle);
+
+    // Expect result - data_2
+    result = 0;
+    handle->getArgument("result", result);
+    EXPECT_EQ(-15, result);
+}
+
+// Test with a shared manager the pre- and post- callout functions don't
+// survive a reload if the shared manager is initialized too late.
+
+TEST_F(HooksManagerTest, PrePostCalloutSharedTooLate) {
+
+    HookLibsCollection library_names;
+    EXPECT_TRUE(HooksManager::loadLibraries(library_names));
+
+    // Initialize the shared manager (after loadLibraries so too late)
+    HooksManager::getSharedCalloutManager().reset(new CalloutManager(0));
+
+    // Load the pre- and post- callouts.
+    HooksManager::preCalloutsLibraryHandle().registerCallout("hookpt_two",
+                                                             testPreCallout);
+    HooksManager::postCalloutsLibraryHandle().registerCallout("hookpt_two",
+                                                              testPostCallout);
+
+    // With the pre- and post- callouts above, the result expected is
+    //
+    // 1027 * 2
+    CalloutHandlePtr handle = HooksManager::createCalloutHandle();
+    handle->setArgument("result", static_cast<int>(0));
+    handle->setArgument("data_2", static_cast<int>(15));
+
+    HooksManager::callCallouts(hookpt_two_index_, *handle);
+
+    int result = 0;
+    handle->getArgument("result", result);
+    EXPECT_EQ(2054, result);
+
+    // ... and check that the pre- and post- callout functions don't survive a
+    // reload.
+    EXPECT_TRUE(HooksManager::loadLibraries(library_names));
+    handle = HooksManager::createCalloutHandle();
+
+    handle->setArgument("result", static_cast<int>(0));
+    handle->setArgument("data_2", static_cast<int>(15));
+
+    HooksManager::callCallouts(hookpt_two_index_, *handle);
+
+    // Expect no change so result = 0
+    result = 0;
+    handle->getArgument("result", result);
+    EXPECT_EQ(0, result);
+}
+
 // Check that everything works even with no libraries loaded.  First that
 // calloutsPresent() always returns false.
 
@@ -542,7 +681,7 @@ TEST_F(HooksManagerTest, validateLibraries) {
 // This test verifies that the specified parameters are accessed properly.
 TEST_F(HooksManagerTest, LibraryParameters) {
 
-    // Prepare paramters for the callout parameters library.
+    // Prepare parameters for the callout parameters library.
     ElementPtr params = Element::createMap();
     params->set("svalue", Element::create("string value"));
     params->set("ivalue", Element::create(42));

src/lib/hooks/tests/library_manager_unittest.cc

@@ -1,4 +1,4 @@
-// Copyright (C) 2013-2016 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2013-2017 Internet Systems Consortium, Inc. ("ISC")
 //
 // This Source Code Form is subject to the terms of the Mozilla Public
 // License, v. 2.0. If a copy of the MPL was not distributed with this
@@ -572,7 +572,7 @@ TEST_F(LibraryManagerTest, libraryLoggerSetup) {
     EXPECT_TRUE(lib_manager.loadLibrary());
 
     // After loading the library, the global logging dictionary should
-    // contain log messages registerd for this library.
+    // contain log messages registered for this library.
     const MessageDictionaryPtr& dict = MessageDictionary::globalDictionary();
     EXPECT_EQ("basic callout load %1", dict->getText("BCL_LOAD_START"));
     EXPECT_EQ("basic callout load end", dict->getText("BCL_LOAD_END"));