From: Stephen Morris Date: Mon, 24 Jun 2013 13:01:46 +0000 (+0100) Subject: [2980] Added conditional initialization. X-Git-Tag: bind10-1.2.0beta1-release~345^2~15 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=d72ba709f11dee0d0bb90868fc042bd8caaf8626;p=thirdparty%2Fkea.git [2980] Added conditional initialization. --- diff --git a/src/lib/hooks/callout_handle.cc b/src/lib/hooks/callout_handle.cc index 27d76d7be7..5776a96e90 100644 --- a/src/lib/hooks/callout_handle.cc +++ b/src/lib/hooks/callout_handle.cc @@ -27,8 +27,10 @@ namespace isc { namespace hooks { // Constructor. -CalloutHandle::CalloutHandle(const boost::shared_ptr& manager) - : arguments_(), context_collection_(), manager_(manager), skip_(false) { +CalloutHandle::CalloutHandle(const boost::shared_ptr& manager, + const boost::shared_ptr& 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. diff --git a/src/lib/hooks/callout_handle.h b/src/lib/hooks/callout_handle.h index 3792c589d0..36a90f8689 100644 --- a/src/lib/hooks/callout_handle.h +++ b/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& manager); + /// @param lmcoll Pointer to the library manager collection. This has a + /// null default for testing purposes. + CalloutHandle(const boost::shared_ptr& manager, + const boost::shared_ptr& lmcoll = + boost::shared_ptr()); /// @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 lm_collection_; /// Collection of arguments passed to the callouts ElementCollection arguments_; diff --git a/src/lib/hooks/hooks_manager.cc b/src/lib/hooks/hooks_manager.cc index de1a2a03b4..a317a56e4c 100644 --- a/src/lib/hooks/hooks_manager.cc +++ b/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 #include #include @@ -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 libraries; - lm_collection_.reset(new LibraryManagerCollection(libraries)); - lm_collection_->loadLibraries(); - - callout_manager_ = lm_collection_->getCalloutManager(); -} - -// Create a callout handle - -boost::shared_ptr -HooksManager::createCalloutHandleInternal() { - conditionallyInitialize(); - return (boost::shared_ptr( - new CalloutHandle(callout_manager_))); -} - -boost::shared_ptr -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 +HooksManager::createCalloutHandleInternal() { + conditionallyInitialize(); + return (boost::shared_ptr( + new CalloutHandle(callout_manager_, lm_collection_))); +} + +boost::shared_ptr +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 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 diff --git a/src/lib/hooks/hooks_manager.h b/src/lib/hooks/hooks_manager.h index 222d97388e..aaefd7fd33 100644 --- a/src/lib/hooks/hooks_manager.h +++ b/src/lib/hooks/hooks_manager.h @@ -15,6 +15,8 @@ #ifndef HOOKS_MANAGER_H #define HOOKS_MANAGER_H +#include + #include #include @@ -153,6 +155,28 @@ public: /// @return Shared pointer to a CalloutHandle object. static boost::shared_ptr 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 diff --git a/src/lib/hooks/tests/hooks_manager_unittest.cc b/src/lib/hooks/tests/hooks_manager_unittest.cc index ec2374c37e..2487b6d1fa 100644 --- a/src/lib/hooks/tests/hooks_manager_unittest.cc +++ b/src/lib/hooks/tests/hooks_manager_unittest.cc @@ -14,6 +14,7 @@ #include #include +#include #include #include @@ -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 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 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 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 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 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(HooksManager::registerHook(string("alpha"))), + DuplicateHook); + + // ... an check the hooks are as we expect. + EXPECT_EQ(5, ServerHooks::getServerHooks().getCount()); + vector 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