From: Razvan Becheriu Date: Fri, 15 May 2020 14:52:40 +0000 (+0300) Subject: [#1127] addressed review X-Git-Tag: Kea-1.7.8~89 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=97f976e91ed73d4ec6edd693ae457ae8c40242d4;p=thirdparty%2Fkea.git [#1127] addressed review --- diff --git a/src/lib/dhcpsrv/tests/mysql_host_data_source_unittest.cc b/src/lib/dhcpsrv/tests/mysql_host_data_source_unittest.cc index d19b9a2ad3..c41cca9c3a 100644 --- a/src/lib/dhcpsrv/tests/mysql_host_data_source_unittest.cc +++ b/src/lib/dhcpsrv/tests/mysql_host_data_source_unittest.cc @@ -368,12 +368,12 @@ TEST(MySqlConnection, checkTimeConversion) { } /// @brief This test verifies that database backend can operate in Read-Only mode. -TEST_F(MySqlHostDataSourceTest, testReadOnlyDatabase) { +TEST_F(MySqlHostDataSourceTest, DISABLED_testReadOnlyDatabase) { testReadOnlyDatabase(MYSQL_VALID_TYPE); } /// @brief This test verifies that database backend can operate in Read-Only mode. -TEST_F(MySqlHostDataSourceTest, testReadOnlyDatabaseMultiThreading) { +TEST_F(MySqlHostDataSourceTest, DISABLED_testReadOnlyDatabaseMultiThreading) { MultiThreadingMgr::instance().setMode(true); testReadOnlyDatabase(MYSQL_VALID_TYPE); } diff --git a/src/lib/dhcpsrv/tests/pgsql_host_data_source_unittest.cc b/src/lib/dhcpsrv/tests/pgsql_host_data_source_unittest.cc index 028c6aa786..17bf95cd5c 100644 --- a/src/lib/dhcpsrv/tests/pgsql_host_data_source_unittest.cc +++ b/src/lib/dhcpsrv/tests/pgsql_host_data_source_unittest.cc @@ -377,12 +377,12 @@ TEST(PgSqlHostDataSource, NoCallbackOnOpenFailMultiThreading) { } /// @brief This test verifies that database backend can operate in Read-Only mode. -TEST_F(PgSqlHostDataSourceTest, testReadOnlyDatabase) { +TEST_F(PgSqlHostDataSourceTest, DISABLED_testReadOnlyDatabase) { testReadOnlyDatabase(PGSQL_VALID_TYPE); } /// @brief This test verifies that database backend can operate in Read-Only mode. -TEST_F(PgSqlHostDataSourceTest, testReadOnlyDatabaseMultiThreading) { +TEST_F(PgSqlHostDataSourceTest, DISABLED_testReadOnlyDatabaseMultiThreading) { MultiThreadingMgr::instance().setMode(true); testReadOnlyDatabase(PGSQL_VALID_TYPE); } diff --git a/src/lib/hooks/hooks_manager.cc b/src/lib/hooks/hooks_manager.cc index b3b69b056d..e6392f6528 100644 --- a/src/lib/hooks/hooks_manager.cc +++ b/src/lib/hooks/hooks_manager.cc @@ -93,8 +93,8 @@ HooksManager::loadLibrariesInternal(const HookLibsCollection& libraries) { if (test_mode_) { return (true); } - // Unload current set of libraries (if any are loaded). - unloadLibrariesInternal(false); + + ServerHooks::getServerHooks().getParkingLotsPtr()->clear(); // Create the library manager and load the libraries. lm_collection_.reset(new LibraryManagerCollection(libraries)); @@ -122,19 +122,9 @@ HooksManager::loadLibraries(const HookLibsCollection& libraries) { // requested. void -HooksManager::unloadLibrariesInternal(bool initialize) { +HooksManager::unloadLibrariesInternal() { ServerHooks::getServerHooks().getParkingLotsPtr()->clear(); - if (initialize) { - init(); - } else { - // 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(); - } + init(); } void HooksManager::unloadLibraries() { @@ -229,7 +219,7 @@ HooksManager::setTestMode(bool mode) { } bool -HooksManager::getTestMode() const { +HooksManager::getTestMode() { return (getHooksManager().test_mode_); } diff --git a/src/lib/hooks/hooks_manager.h b/src/lib/hooks/hooks_manager.h index 7c8947d394..0f452b2e1d 100644 --- a/src/lib/hooks/hooks_manager.h +++ b/src/lib/hooks/hooks_manager.h @@ -318,12 +318,12 @@ public: /// /// @param mode the test mode flag which enables or disabled the /// functionality. - void setTestMode(bool mode); + static void setTestMode(bool mode); /// @brief Get test mode /// /// @return the test mode flag. - bool getTestMode() const; + static bool getTestMode(); private: @@ -417,10 +417,7 @@ private: bool loadLibrariesInternal(const HookLibsCollection& libraries); /// @brief Unload libraries - /// - /// @param initialize flag to indicate if initializing or just resetting the - /// @ref lm_collection_ and @ref callout_manager_. - void unloadLibrariesInternal(bool initialize = true); + void unloadLibrariesInternal(); /// @brief Are callouts present? /// diff --git a/src/lib/hooks/library_manager_collection.cc b/src/lib/hooks/library_manager_collection.cc index 743ebd1668..edb401160e 100644 --- a/src/lib/hooks/library_manager_collection.cc +++ b/src/lib/hooks/library_manager_collection.cc @@ -67,16 +67,11 @@ LibraryManagerCollection::loadLibraries() { // associated libraries are deleted, hence this link (LibraryManager -> // CalloutManager) is safe. // - // If the list of libraries is not empty, re-create the callout manager. + // The call of this function will result in re-creating the callout manager. // This deletes all callouts (including the pre-library and post- // library) ones. It is up to the libraries to re-register their callouts. // The pre-library and post-library callouts will also need to be // re-registered. - // - // If the list of libraries stays empty (as in the case of a reconfiguration - // where the hooks-libraries clause was empty and is not changed), try - // to re-use the existing callout manager (so retaining registered pre- - // and post-library callouts). callout_manager_.reset(new CalloutManager(library_names_.size())); // Now iterate through the libraries are load them one by one. We'll diff --git a/src/lib/hooks/tests/hooks_manager_unittest.cc b/src/lib/hooks/tests/hooks_manager_unittest.cc index 6e8cb4e8f4..1c6772355e 100644 --- a/src/lib/hooks/tests/hooks_manager_unittest.cc +++ b/src/lib/hooks/tests/hooks_manager_unittest.cc @@ -38,7 +38,7 @@ public: /// Reset the hooks manager. The hooks manager is a singleton, so needs /// to be reset for each test. HooksManagerTest() { - HooksManager::getHooksManager().setTestMode(false); + HooksManager::setTestMode(false); HooksManager::unloadLibraries(); } @@ -46,7 +46,7 @@ public: /// /// Unload all libraries and reset the shared manager. ~HooksManagerTest() { - HooksManager::getHooksManager().setTestMode(false); + HooksManager::setTestMode(false); HooksManager::unloadLibraries(); } @@ -443,7 +443,10 @@ TEST_F(HooksManagerTest, PrePostCalloutTest) { TEST_F(HooksManagerTest, TestModeEnabledPrePostSurviveLoad) { + // Load a single library. HookLibsCollection library_names; + library_names.push_back(make_pair(std::string(FULL_CALLOUT_LIBRARY), + data::ConstElementPtr())); // Load the pre- and post- callouts. HooksManager::preCalloutsLibraryHandle().registerCallout("hookpt_two", @@ -451,7 +454,7 @@ TEST_F(HooksManagerTest, TestModeEnabledPrePostSurviveLoad) { HooksManager::postCalloutsLibraryHandle().registerCallout("hookpt_two", testPostCallout); - HooksManager::getHooksManager().setTestMode(true); + HooksManager::setTestMode(true); // With the pre- and post- callouts above, the result expected is // @@ -487,7 +490,10 @@ TEST_F(HooksManagerTest, TestModeEnabledPrePostSurviveLoad) { TEST_F(HooksManagerTest, TestModeDisabledPrePostDoNotSurviveLoad) { + // Load a single library. HookLibsCollection library_names; + library_names.push_back(make_pair(std::string(FULL_CALLOUT_LIBRARY), + data::ConstElementPtr())); // Load the pre- and post- callouts. HooksManager::preCalloutsLibraryHandle().registerCallout("hookpt_two", @@ -495,7 +501,7 @@ TEST_F(HooksManagerTest, TestModeDisabledPrePostDoNotSurviveLoad) { HooksManager::postCalloutsLibraryHandle().registerCallout("hookpt_two", testPostCallout); - HooksManager::getHooksManager().setTestMode(false); + HooksManager::setTestMode(false); // With the pre- and post- callouts above, the result expected is // @@ -520,10 +526,9 @@ TEST_F(HooksManagerTest, TestModeDisabledPrePostDoNotSurviveLoad) { HooksManager::callCallouts(hookpt_two_index_, *handle); - // Expect no change so result = 0 result = 0; handle->getArgument("result", result); - EXPECT_EQ(0, result); + EXPECT_EQ(-15, result); } // Test with test mode enabled and the pre- and post- callout functions do not @@ -531,7 +536,10 @@ TEST_F(HooksManagerTest, TestModeDisabledPrePostDoNotSurviveLoad) { TEST_F(HooksManagerTest, TestModeEnabledTooLatePrePostDoNotSurvive) { + // Load a single library. HookLibsCollection library_names; + library_names.push_back(make_pair(std::string(FULL_CALLOUT_LIBRARY), + data::ConstElementPtr())); // Load the pre- and post- callouts. HooksManager::preCalloutsLibraryHandle().registerCallout("hookpt_two", @@ -557,17 +565,16 @@ TEST_F(HooksManagerTest, TestModeEnabledTooLatePrePostDoNotSurvive) { EXPECT_TRUE(HooksManager::loadLibraries(library_names)); handle = HooksManager::createCalloutHandle(); - HooksManager::getHooksManager().setTestMode(true); + HooksManager::setTestMode(true); handle->setArgument("result", static_cast(0)); handle->setArgument("data_2", static_cast(15)); HooksManager::callCallouts(hookpt_two_index_, *handle); - // Expect no change so result = 0 result = 0; handle->getArgument("result", result); - EXPECT_EQ(0, result); + EXPECT_EQ(-15, result); } // Check that everything works even with no libraries loaded. First that