[5577] Improved the fix, added unit-test.

diff --git a/src/lib/hooks/callout_manager.cc b/src/lib/hooks/callout_manager.cc
index 9e93439465a5ebd4449a18c14c7ed5f6bbff566d..1569f641a20fb0dab3723e931d054d09e22a4383 100644 (file)
--- a/src/lib/hooks/callout_manager.cc
+++ b/src/lib/hooks/callout_manager.cc
@@ -66,15 +66,13 @@ CalloutManager::registerCallout(const std::string& name, CalloutPtr callout) {
     // Sanity check that the current library index is set to a valid value.
     checkLibraryIndex(current_library_);
 
+    // New hooks could have been registered since the manager was constructed.
+    ensureVectorSize();
+
     // Get the index associated with this hook (validating the name in the
     // process).
     int hook_index = server_hooks_.getIndex(name);
 
-    // New hooks can have been registered since the manager was constructed.
-    if (hook_index >= hook_vector_.size()) {
-        hook_vector_.resize(server_hooks_.getCount());
-    }
-
     // Iterate through the callout vector for the hook from start to end,
     // looking for the first entry where the library index is greater than
     // the present index.
@@ -238,6 +236,9 @@ CalloutManager::deregisterCallout(const std::string& name, CalloutPtr callout) {
     // Sanity check that the current library index is set to a valid value.
     checkLibraryIndex(current_library_);
 
+    // New hooks could have been registered since the manager was constructed.
+    ensureVectorSize();
+
     // Get the index associated with this hook (validating the name in the
     // process).
     int hook_index = server_hooks_.getIndex(name);
@@ -286,15 +287,13 @@ CalloutManager::deregisterCallout(const std::string& name, CalloutPtr callout) {
 bool
 CalloutManager::deregisterAllCallouts(const std::string& name) {
 
+    // New hooks could have been registered since the manager was constructed.
+    ensureVectorSize();
+
     // Get the index associated with this hook (validating the name in the
     // process).
     int hook_index = server_hooks_.getIndex(name);
 
-    // New hooks can have been registered since the manager was constructed.
-    if (hook_index >= hook_vector_.size()) {
-        return (false);
-    }
-
     /// Construct a CalloutEntry matching the current library (the callout
     /// pointer is NULL as we are not checking that).
     CalloutEntry target(current_library_, static_cast<CalloutPtr>(0));
@@ -324,6 +323,10 @@ CalloutManager::deregisterAllCallouts(const std::string& name) {
 
 void
 CalloutManager::registerCommandHook(const std::string& command_name) {
+
+    // New hooks could have been registered since the manager was constructed.
+    ensureVectorSize();
+
     ServerHooks& hooks = ServerHooks::getServerHooks();
     int hook_index = hooks.findIndex(ServerHooks::commandToHookName(command_name));
     if (hook_index < 0) {
@@ -338,5 +341,14 @@ CalloutManager::registerCommandHook(const std::string& command_name) {
     }
 }
 
+void
+CalloutManager::ensureVectorSize() {
+    ServerHooks& hooks = ServerHooks::getServerHooks();
+    if (hooks.getCount() > hook_vector_.size()) {
+        // Uh oh, there are more hook points that our vector allows.
+        hook_vector_.resize(hooks.getCount());
+    }
+}
+
 } // namespace util
 } // namespace isc

diff --git a/src/lib/hooks/callout_manager.h b/src/lib/hooks/callout_manager.h
index 1cf9cdb2eb039ce4664d36fb195583b78fefc9d7..5cd8ca516c4e55d6bfe1384044aeb848b8ad4dda 100644 (file)
--- a/src/lib/hooks/callout_manager.h
+++ b/src/lib/hooks/callout_manager.h
@@ -365,6 +365,33 @@ public:
         return (post_library_handle_);
     }
 
+
+    /// @brief Return number of currently available hooks
+    size_t getVectorSize() const {
+        return (hook_vector_.size());
+    }
+
+    /// @brief This method checks whether the hook_vector_ size is suffucient
+    ///        and extends it if necessary.
+    ///
+    /// The problem is as follows: some hooks are initialized statically
+    /// from global static objects. ServerHooks object creates hooks_ collection
+    /// and CalloutManager creates its own hook_vector_ and both are initialized
+    /// to the same size. All works well so far. However, if some code at a
+    /// later time (e.g. a hook library) register new hook point, then
+    /// ServerHooks::registerHook() will extend its hooks_ collection, but
+    /// the CalloutManager will keep the old hook_vector_ that is too small by
+    /// one. Now when the library is unloaded, deregisterAllCallouts will
+    /// go through all hook points and will eventually hit the one that
+    /// will return index greater than the hook_vector_ size.
+    ///
+    /// To solve this problem, ensureVectorSize was implemented. It should
+    /// be called (presumably from ServerHooks) every time a new hook point
+    /// is registered. It checks whether the vector size is sufficient and
+    /// extends it if necessary. It is safe to call it multiple times. It
+    /// may grow the vector size, but will never shrink it.
+    void ensureVectorSize();
+
     //@}
 
 private:

diff --git a/src/lib/hooks/server_hooks.cc b/src/lib/hooks/server_hooks.cc
index 07c8f1ac33eed3faf681be03d505fd7d8b4185a5..293f78e637f45f4ed146211292a3ffc9ac7a10bf 100644 (file)
--- a/src/lib/hooks/server_hooks.cc
+++ b/src/lib/hooks/server_hooks.cc
@@ -48,6 +48,11 @@ ServerHooks::registerHook(const string& name) {
     pair<HookCollection::iterator, bool> result =
         hooks_.insert(make_pair(name, index));
 
+    /// @todo: We also need to call CalloutManager::ensureVectorSize(), so it
+    /// adjusts its vector. Since CalloutManager is not a singleton, there's
+    /// no getInstance() or similar. Also, CalloutManager uses ServerHooks,
+    /// so such a call would induce circular dependencies. Ugh.
+
     if (!result.second) {
 
         // There's a problem with hook libraries that need to be linked with

diff --git a/src/lib/hooks/tests/callout_manager_unittest.cc b/src/lib/hooks/tests/callout_manager_unittest.cc
index f1f93f8969a6dc536aa904a3017a9a2d8686db28..60a0ccde011357d50b729e70e800c0497b55df26 100644 (file)
--- a/src/lib/hooks/tests/callout_manager_unittest.cc
+++ b/src/lib/hooks/tests/callout_manager_unittest.cc
@@ -919,6 +919,26 @@ TEST_F(CalloutManagerTest, LibraryHandleRegisterCommandHandler) {
                  NoSuchHook);
 }
 
+// This test checks if the CalloutManager can adjust its own hook_vector_ size.
+TEST_F(CalloutManagerTest, VectorSize) {
+
+    size_t s = getCalloutManager()->getVectorSize();
+
+    ServerHooks& hooks = ServerHooks::getServerHooks();
+
+    EXPECT_NO_THROW(hooks.registerHook("a_new_one"));
+
+    // Now load a callout. Name of the hook point the new callout is installed
+    // on doesn't matter. CM should do sanity checks and adjust anyway.
+    getCalloutManager()->getPostLibraryHandle().registerCallout("alpha",
+                                                                callout_four);
+
+    // The vector size should have been increased by one, because there's
+    // one new hook point now.
+    EXPECT_EQ(s + 1, getCalloutManager()->getVectorSize());
+}
+
+
 // The setting of the hook index is checked in the handles_unittest
 // set of tests, as access restrictions mean it is not easily tested
 // on its own.