Commit 70fe4301 authored by Tomek Mrugalski's avatar Tomek Mrugalski 🛰
Browse files

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

parent 98781a7a
...@@ -66,15 +66,13 @@ CalloutManager::registerCallout(const std::string& name, CalloutPtr callout) { ...@@ -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. // Sanity check that the current library index is set to a valid value.
checkLibraryIndex(current_library_); 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 // Get the index associated with this hook (validating the name in the
// process). // process).
int hook_index = server_hooks_.getIndex(name); 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, // Iterate through the callout vector for the hook from start to end,
// looking for the first entry where the library index is greater than // looking for the first entry where the library index is greater than
// the present index. // the present index.
...@@ -238,6 +236,9 @@ CalloutManager::deregisterCallout(const std::string& name, CalloutPtr callout) { ...@@ -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. // Sanity check that the current library index is set to a valid value.
checkLibraryIndex(current_library_); 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 // Get the index associated with this hook (validating the name in the
// process). // process).
int hook_index = server_hooks_.getIndex(name); int hook_index = server_hooks_.getIndex(name);
...@@ -286,15 +287,13 @@ CalloutManager::deregisterCallout(const std::string& name, CalloutPtr callout) { ...@@ -286,15 +287,13 @@ CalloutManager::deregisterCallout(const std::string& name, CalloutPtr callout) {
bool bool
CalloutManager::deregisterAllCallouts(const std::string& name) { 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 // Get the index associated with this hook (validating the name in the
// process). // process).
int hook_index = server_hooks_.getIndex(name); 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 /// Construct a CalloutEntry matching the current library (the callout
/// pointer is NULL as we are not checking that). /// pointer is NULL as we are not checking that).
CalloutEntry target(current_library_, static_cast<CalloutPtr>(0)); CalloutEntry target(current_library_, static_cast<CalloutPtr>(0));
...@@ -324,6 +323,10 @@ CalloutManager::deregisterAllCallouts(const std::string& name) { ...@@ -324,6 +323,10 @@ CalloutManager::deregisterAllCallouts(const std::string& name) {
void void
CalloutManager::registerCommandHook(const std::string& command_name) { CalloutManager::registerCommandHook(const std::string& command_name) {
// New hooks could have been registered since the manager was constructed.
ensureVectorSize();
ServerHooks& hooks = ServerHooks::getServerHooks(); ServerHooks& hooks = ServerHooks::getServerHooks();
int hook_index = hooks.findIndex(ServerHooks::commandToHookName(command_name)); int hook_index = hooks.findIndex(ServerHooks::commandToHookName(command_name));
if (hook_index < 0) { if (hook_index < 0) {
...@@ -338,5 +341,14 @@ CalloutManager::registerCommandHook(const std::string& command_name) { ...@@ -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 util
} // namespace isc } // namespace isc
...@@ -365,6 +365,33 @@ public: ...@@ -365,6 +365,33 @@ public:
return (post_library_handle_); 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: private:
......
...@@ -48,6 +48,11 @@ ServerHooks::registerHook(const string& name) { ...@@ -48,6 +48,11 @@ ServerHooks::registerHook(const string& name) {
pair<HookCollection::iterator, bool> result = pair<HookCollection::iterator, bool> result =
hooks_.insert(make_pair(name, index)); 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) { if (!result.second) {
// There's a problem with hook libraries that need to be linked with // There's a problem with hook libraries that need to be linked with
......
...@@ -919,6 +919,26 @@ TEST_F(CalloutManagerTest, LibraryHandleRegisterCommandHandler) { ...@@ -919,6 +919,26 @@ TEST_F(CalloutManagerTest, LibraryHandleRegisterCommandHandler) {
NoSuchHook); 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 // The setting of the hook index is checked in the handles_unittest
// set of tests, as access restrictions mean it is not easily tested // set of tests, as access restrictions mean it is not easily tested
// on its own. // on its own.
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment