From: Razvan Becheriu Date: Wed, 13 May 2020 08:05:01 +0000 (+0300) Subject: [#1127] never update lm_collection_ outside init functions X-Git-Tag: Kea-1.7.8~95 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=7dcb9b8215ea913efebc05b8c342364f6535fc40;p=thirdparty%2Fkea.git [#1127] never update lm_collection_ outside init functions --- diff --git a/src/bin/dhcp4/tests/hooks_unittest.cc b/src/bin/dhcp4/tests/hooks_unittest.cc index 4e3d0ae858..9c4203e331 100644 --- a/src/bin/dhcp4/tests/hooks_unittest.cc +++ b/src/bin/dhcp4/tests/hooks_unittest.cc @@ -129,7 +129,7 @@ public: HooksManager::preCalloutsLibraryHandle().deregisterAllCallouts("lease4_decline"); HooksManager::preCalloutsLibraryHandle().deregisterAllCallouts("host4_identifier"); - HooksManager::getSharedCalloutManager().reset(); + HooksManager::getHooksManager().setSharedCalloutManager(); delete srv_; } @@ -2378,7 +2378,8 @@ TEST_F(HooksDhcpv4SrvTest, HooksDecline) { IfaceMgr::instance().openSockets4(); // Libraries will be reloaded later - HooksManager::getSharedCalloutManager().reset(new CalloutManager(0)); + HooksManager::getHooksManager().setSharedCalloutManager( + boost::shared_ptr(new CalloutManager(0))); // Install a callout EXPECT_NO_THROW(HooksManager::preCalloutsLibraryHandle().registerCallout( @@ -2430,7 +2431,8 @@ TEST_F(HooksDhcpv4SrvTest, HooksDeclineSkip) { IfaceMgr::instance().openSockets4(); // Libraries will be reloaded later - HooksManager::getSharedCalloutManager().reset(new CalloutManager(0)); + HooksManager::getHooksManager().setSharedCalloutManager( + boost::shared_ptr(new CalloutManager(0))); // Install a callout EXPECT_NO_THROW(HooksManager::preCalloutsLibraryHandle().registerCallout( @@ -2481,7 +2483,8 @@ TEST_F(HooksDhcpv4SrvTest, HooksDeclineDrop) { IfaceMgr::instance().openSockets4(); // Libraries will be reloaded later - HooksManager::getSharedCalloutManager().reset(new CalloutManager(0)); + HooksManager::getHooksManager().setSharedCalloutManager( + boost::shared_ptr(new CalloutManager(0))); // Install a callout EXPECT_NO_THROW(HooksManager::preCalloutsLibraryHandle().registerCallout( diff --git a/src/bin/dhcp6/tests/hooks_unittest.cc b/src/bin/dhcp6/tests/hooks_unittest.cc index e6ff78b28a..11a2d0face 100644 --- a/src/bin/dhcp6/tests/hooks_unittest.cc +++ b/src/bin/dhcp6/tests/hooks_unittest.cc @@ -138,7 +138,7 @@ public: ~HooksDhcpv6SrvTest() { // Clear shared manager - HooksManager::getSharedCalloutManager().reset(); + HooksManager::getHooksManager().setSharedCalloutManager(); } @@ -4418,7 +4418,8 @@ TEST_F(HooksDhcpv6SrvTest, basicLease6Decline) { IfaceMgrTestConfig test_config(true); // Libraries will be reloaded later - HooksManager::getSharedCalloutManager().reset(new CalloutManager(0)); + HooksManager::getHooksManager().setSharedCalloutManager( + boost::shared_ptr(new CalloutManager(0))); // Install lease6_decline callout EXPECT_NO_THROW(HooksManager::preCalloutsLibraryHandle().registerCallout( @@ -4471,7 +4472,8 @@ TEST_F(HooksDhcpv6SrvTest, lease6DeclineSkip) { IfaceMgrTestConfig test_config(true); // Libraries will be reloaded later - HooksManager::getSharedCalloutManager().reset(new CalloutManager(0)); + HooksManager::getHooksManager().setSharedCalloutManager( + boost::shared_ptr(new CalloutManager(0))); // Install lease6_decline_skip callout. It will set the status to skip EXPECT_NO_THROW(HooksManager::preCalloutsLibraryHandle().registerCallout( @@ -4521,7 +4523,8 @@ TEST_F(HooksDhcpv6SrvTest, lease6DeclineDrop) { IfaceMgrTestConfig test_config(true); // Libraries will be reloaded later - HooksManager::getSharedCalloutManager().reset(new CalloutManager(0)); + HooksManager::getHooksManager().setSharedCalloutManager( + boost::shared_ptr(new CalloutManager(0))); // Install lease6_decline_drop callout. It will set the status to drop EXPECT_NO_THROW(HooksManager::preCalloutsLibraryHandle().registerCallout( diff --git a/src/lib/hooks/hooks_manager.cc b/src/lib/hooks/hooks_manager.cc index 3056ef7847..5e7d36bff6 100644 --- a/src/lib/hooks/hooks_manager.cc +++ b/src/lib/hooks/hooks_manager.cc @@ -26,7 +26,8 @@ namespace hooks { // Constructor -HooksManager::HooksManager() { +HooksManager::HooksManager() : loaded_(false) { + init(); } // Return reference to singleton hooks manager. @@ -41,7 +42,6 @@ HooksManager::getHooksManager() { bool HooksManager::calloutsPresentInternal(int index) { - conditionallyInitialize(); return (callout_manager_->calloutsPresent(index)); } @@ -52,7 +52,6 @@ HooksManager::calloutsPresent(int index) { bool HooksManager::commandHandlersPresentInternal(const std::string& command_name) { - conditionallyInitialize(); return (callout_manager_->commandHandlersPresent(command_name)); } @@ -65,7 +64,6 @@ HooksManager::commandHandlersPresent(const std::string& command_name) { void HooksManager::callCalloutsInternal(int index, CalloutHandle& handle) { - conditionallyInitialize(); callout_manager_->callCallouts(index, handle); } @@ -77,7 +75,6 @@ HooksManager::callCallouts(int index, CalloutHandle& handle) { void HooksManager::callCommandHandlersInternal(const std::string& command_name, CalloutHandle& handle) { - conditionallyInitialize(); callout_manager_->callCommandHandlers(command_name, handle); } @@ -87,17 +84,17 @@ HooksManager::callCommandHandlers(const std::string& command_name, getHooksManager().callCommandHandlersInternal(command_name, handle); } - // Load the libraries. This will delete the previously-loaded libraries -// (if present) and load new ones. +// (if present) and load new ones. If loading libraries fails, initialize with +// empty list. bool HooksManager::loadLibrariesInternal(const HookLibsCollection& libraries) { // Unload current set of libraries (if any are loaded). - unloadLibrariesInternal(); + unloadLibrariesInternal(false); // Create the library manager and load the libraries. - lm_collection_.reset(new LibraryManagerCollection(libraries)); + lm_collection_.reset(new LibraryManagerCollection(libraries, shared_callout_manager_)); bool status = lm_collection_->loadLibraries(); if (status) { @@ -106,10 +103,11 @@ HooksManager::loadLibrariesInternal(const HookLibsCollection& libraries) { } else { // Unable to load libraries, reset to state before this function was // called. - lm_collection_.reset(); - callout_manager_.reset(); + init(); } + loaded_ = true; + return (status); } @@ -118,19 +116,21 @@ HooksManager::loadLibraries(const HookLibsCollection& libraries) { return (getHooksManager().loadLibrariesInternal(libraries)); } -// Unload the libraries. This just deletes all internal objects which will -// cause the libraries to be unloaded. +// Unload the libraries. This just deletes all internal objects (which will +// cause the libraries to be unloaded) and initializes them with empty list if +// requested. void -HooksManager::unloadLibrariesInternal() { - // The order of deletion does not matter here, as each library manager - // holds its own pointer to the callout manager. However, we may as - // well delete the library managers first: if there are no other references - // to the callout manager, the second statement will delete it, which may - // ease debugging. - lm_collection_.reset(); - callout_manager_.reset(); +HooksManager::unloadLibrariesInternal(bool initialize) { ServerHooks::getServerHooks().getParkingLotsPtr()->clear(); + if (initialize) { + init(); + } else { + lm_collection_.reset(); + callout_manager_.reset(); + } + + loaded_ = false; } void HooksManager::unloadLibraries() { @@ -141,9 +141,7 @@ void HooksManager::unloadLibraries() { boost::shared_ptr HooksManager::createCalloutHandleInternal() { - conditionallyInitialize(); - return (boost::shared_ptr( - new CalloutHandle(callout_manager_, lm_collection_))); + return (boost::make_shared(callout_manager_, lm_collection_)); } boost::shared_ptr @@ -155,14 +153,12 @@ HooksManager::createCalloutHandle() { std::vector HooksManager::getLibraryNamesInternal() const { - return (lm_collection_ ? lm_collection_->getLibraryNames() - : std::vector()); + return (lm_collection_->getLibraryNames()); } HookLibsCollection HooksManager::getLibraryInfoInternal() const { - return (lm_collection_ ? lm_collection_->getLibraryInfo() - : HookLibsCollection()); + return (lm_collection_->getLibraryInfo()); } std::vector @@ -175,17 +171,15 @@ HooksManager::getLibraryInfo() { return (getHooksManager().getLibraryInfoInternal()); } -// Perform conditional initialization if nothing is loaded. +// Perform initialization void -HooksManager::performConditionalInitialization() { - +HooksManager::init() { // Nothing present, so create the collection with any empty set of // libraries, and get the CalloutManager. HookLibsCollection libraries; - lm_collection_.reset(new LibraryManagerCollection(libraries)); + lm_collection_.reset(new LibraryManagerCollection(libraries, shared_callout_manager_)); lm_collection_->loadLibraries(); - callout_manager_ = lm_collection_->getCalloutManager(); } @@ -200,7 +194,6 @@ HooksManager::registerHook(const std::string& name) { isc::hooks::LibraryHandle& HooksManager::preCalloutsLibraryHandleInternal() { - conditionallyInitialize(); return (callout_manager_->getPreLibraryHandle()); } @@ -211,7 +204,6 @@ HooksManager::preCalloutsLibraryHandle() { isc::hooks::LibraryHandle& HooksManager::postCalloutsLibraryHandleInternal() { - conditionallyInitialize(); return (callout_manager_->getPostLibraryHandle()); } @@ -227,10 +219,17 @@ HooksManager::validateLibraries(const std::vector& libraries) { return (LibraryManagerCollection::validateLibraries(libraries)); } -// Shared callout manager -boost::shared_ptr& +boost::shared_ptr HooksManager::getSharedCalloutManager() { - return (getHooksManager().shared_callout_manager_); + return (shared_callout_manager_); +} + +void +HooksManager::setSharedCalloutManager(boost::shared_ptr manager) { + shared_callout_manager_ = manager; + if (!loaded_) { + init(); + } } } // namespace util diff --git a/src/lib/hooks/hooks_manager.h b/src/lib/hooks/hooks_manager.h index 903ad6aa6e..ecb050ca3c 100644 --- a/src/lib/hooks/hooks_manager.h +++ b/src/lib/hooks/hooks_manager.h @@ -226,13 +226,20 @@ 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. + /// @brief Get the shared callout manager /// /// @return A reference to the shared callout manager - static boost::shared_ptr& getSharedCalloutManager(); + boost::shared_ptr getSharedCalloutManager(); + + /// @brief Set the shared callout manager + /// + /// This function sets the shared callout manager and explicitly updates + /// @ref callout_manager_ (by calling @ref init) if @ref loadLibraries has + /// not yet been called. + /// + /// @param manager The shared callout manager + void setSharedCalloutManager(boost::shared_ptr manager = + boost::shared_ptr()); /// @brief Park an object (packet). /// @@ -411,7 +418,10 @@ private: bool loadLibrariesInternal(const HookLibsCollection& libraries); /// @brief Unload libraries - void unloadLibrariesInternal(); + /// + /// @param initialize flag to indicate if intializing or just resetting the + /// @ref lm_collection_ and @ref callout_manager_ + void unloadLibrariesInternal(bool initialize = true); /// @brief Are callouts present? /// @@ -479,29 +489,8 @@ private: /// @brief Initialization to No Libraries /// - /// Initializes the hooks manager with an "empty set" of libraries. This - /// method is called if conditionallyInitialize() determines that such - /// initialization is needed. - void performConditionalInitialization(); - - /// @brief Conditional initialization of the hooks manager - /// - /// loadLibraries() performs the initialization of the HooksManager, - /// setting up the internal structures and loading libraries. However, - /// in some cases, server authors may not do that. This method is called - /// whenever any hooks execution function is invoked (checking callouts, - /// calling callouts or returning a callout handle). If the HooksManager - /// is uninitialized, it will initialize it with an "empty set" - /// of libraries. - /// - /// For speed, the test of whether initialization is required is done - /// in-line here. The actual initialization is performed in - /// performConditionalInitialization(). - void conditionallyInitialize() { - if (!lm_collection_) { - performConditionalInitialization(); - } - } + /// Initializes the hooks manager with an "empty set" of libraries. + void init(); // Members @@ -514,6 +503,8 @@ private: /// Shared callout manager to survive library reloads. boost::shared_ptr shared_callout_manager_; + /// Loaded flag to indicate if @ref loadLibraries has been called + bool loaded_; }; } // namespace util diff --git a/src/lib/hooks/library_manager_collection.cc b/src/lib/hooks/library_manager_collection.cc index e814ac7f3a..17ee10ad03 100644 --- a/src/lib/hooks/library_manager_collection.cc +++ b/src/lib/hooks/library_manager_collection.cc @@ -7,7 +7,6 @@ #include #include -#include #include #include @@ -29,7 +28,7 @@ boost::shared_ptr LibraryManagerCollection::getCalloutManager() const { // Only return a pointer if we have a CalloutManager created. - if (! callout_manager_) { + if (!callout_manager_) { isc_throw(LoadLibrariesNotCalled, "must load hooks libraries before " "attempting to retrieve a CalloutManager for them"); } @@ -37,8 +36,9 @@ LibraryManagerCollection::getCalloutManager() const { return (callout_manager_); } -LibraryManagerCollection::LibraryManagerCollection(const HookLibsCollection& libraries) - :library_info_(libraries) { +LibraryManagerCollection::LibraryManagerCollection(const HookLibsCollection& libraries, + const boost::shared_ptr& manager) + : shared_callout_manager_(manager), library_info_(libraries) { // We need to split hook libs into library names and library parameters. for (HookLibsCollection::const_iterator it = libraries.begin(); @@ -78,7 +78,7 @@ LibraryManagerCollection::loadLibraries() { // to re-use the existing callout manager (so retaining registered pre- // and post-library callouts). if (library_names_.empty()) { - callout_manager_ = HooksManager::getSharedCalloutManager(); + callout_manager_ = shared_callout_manager_; } if (!library_names_.empty() || !callout_manager_) { callout_manager_.reset(new CalloutManager(library_names_.size())); diff --git a/src/lib/hooks/library_manager_collection.h b/src/lib/hooks/library_manager_collection.h index df52649ecb..f1b481b813 100644 --- a/src/lib/hooks/library_manager_collection.h +++ b/src/lib/hooks/library_manager_collection.h @@ -8,6 +8,7 @@ #define LIBRARY_MANAGER_COLLECTION_H #include +#include #include #include @@ -74,7 +75,9 @@ public: /// @param libraries List of libraries that this collection will manage. /// The order of the libraries is important. It holds the library /// names and its configuration parameters. - LibraryManagerCollection(const HookLibsCollection& libraries); + LibraryManagerCollection(const HookLibsCollection& libraries, + const boost::shared_ptr& manager = + HooksManager::getHooksManager().getSharedCalloutManager()); /// @brief Destructor /// @@ -152,6 +155,8 @@ protected: void unloadLibraries(); private: + /// Shared Callout manager to be associated with the libraries + boost::shared_ptr shared_callout_manager_; /// Vector of library names std::vector library_names_; diff --git a/src/lib/hooks/tests/hooks_manager_unittest.cc b/src/lib/hooks/tests/hooks_manager_unittest.cc index 8d5eff104e..44b39cb6c7 100644 --- a/src/lib/hooks/tests/hooks_manager_unittest.cc +++ b/src/lib/hooks/tests/hooks_manager_unittest.cc @@ -46,7 +46,7 @@ public: /// Unload all libraries and reset the shared manager. ~HooksManagerTest() { HooksManager::unloadLibraries(); - HooksManager::getSharedCalloutManager().reset(); + HooksManager::getHooksManager().setSharedCalloutManager(); } @@ -445,7 +445,8 @@ TEST_F(HooksManagerTest, PrePostCalloutShared) { HookLibsCollection library_names; // Initialize the shared manager. - HooksManager::getSharedCalloutManager().reset(new CalloutManager(0)); + HooksManager::getHooksManager().setSharedCalloutManager( + boost::shared_ptr(new CalloutManager(0))); // Load the pre- and post- callouts. HooksManager::preCalloutsLibraryHandle().registerCallout("hookpt_two", @@ -492,7 +493,8 @@ TEST_F(HooksManagerTest, PrePostCalloutSharedNotEmpty) { data::ConstElementPtr())); // Initialize the shared manager. - HooksManager::getSharedCalloutManager().reset(new CalloutManager(0)); + HooksManager::getHooksManager().setSharedCalloutManager( + boost::shared_ptr(new CalloutManager(0))); // Load the pre- and post- callouts. HooksManager::preCalloutsLibraryHandle().registerCallout("hookpt_two", @@ -538,7 +540,8 @@ TEST_F(HooksManagerTest, PrePostCalloutSharedTooLate) { EXPECT_TRUE(HooksManager::loadLibraries(library_names)); // Initialize the shared manager (after loadLibraries so too late) - HooksManager::getSharedCalloutManager().reset(new CalloutManager(0)); + HooksManager::getHooksManager().setSharedCalloutManager( + boost::shared_ptr(new CalloutManager(0))); // Load the pre- and post- callouts. HooksManager::preCalloutsLibraryHandle().registerCallout("hookpt_two", diff --git a/src/lib/hooks/tests/library_manager_collection_unittest.cc b/src/lib/hooks/tests/library_manager_collection_unittest.cc index 6ace663abd..6066c6e58a 100644 --- a/src/lib/hooks/tests/library_manager_collection_unittest.cc +++ b/src/lib/hooks/tests/library_manager_collection_unittest.cc @@ -8,6 +8,7 @@ #include #include +#include #include #include #include @@ -46,27 +47,33 @@ private: } }; +} // namespace + +namespace isc { +namespace hooks { /// @brief Public library manager collection class /// /// This is an instance of the LibraryManagerCollection class but with the /// protected methods made public for test purposes. -class PublicLibraryManagerCollection - : public isc::hooks::LibraryManagerCollection { +class PublicLibraryManagerCollection : public LibraryManagerCollection { public: /// @brief Constructor /// /// @param List of libraries that this collection will manage. The order /// of the libraries is important. PublicLibraryManagerCollection(const HookLibsCollection& libraries) - : LibraryManagerCollection(libraries) - {} + : LibraryManagerCollection(libraries) { + } /// Public methods that call protected methods on the superclass. using LibraryManagerCollection::unloadLibraries; }; +} // namespace hooks +} // namespace isc +namespace { // This is effectively the same test as for LibraryManager, but using the // LibraryManagerCollection object.