From: Tomek Mrugalski Date: Wed, 16 May 2018 11:56:17 +0000 (+0200) Subject: [5577] Improved the fix, added unit-test. X-Git-Tag: trac5549a_base~17 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=70fe43018420de41650f5d7711a799dca0993b86;p=thirdparty%2Fkea.git [5577] Improved the fix, added unit-test. --- diff --git a/src/lib/hooks/callout_manager.cc b/src/lib/hooks/callout_manager.cc index 9e93439465..1569f641a2 100644 --- 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(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 1cf9cdb2eb..5cd8ca516c 100644 --- 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 07c8f1ac33..293f78e637 100644 --- a/src/lib/hooks/server_hooks.cc +++ b/src/lib/hooks/server_hooks.cc @@ -48,6 +48,11 @@ ServerHooks::registerHook(const string& name) { pair 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 f1f93f8969..60a0ccde01 100644 --- 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.