From: Marcin Siodelski Date: Wed, 14 Oct 2015 17:28:04 +0000 (+0200) Subject: [3975] Addressed review comments. X-Git-Tag: trac3874_base~15^2 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=ee0daa0fd81ff327190fea1768b04732eb3b9aa3;p=thirdparty%2Fkea.git [3975] Addressed review comments. --- diff --git a/src/bin/dhcp4/ctrl_dhcp4_srv.cc b/src/bin/dhcp4/ctrl_dhcp4_srv.cc index 3169626a96..cddadb086a 100644 --- a/src/bin/dhcp4/ctrl_dhcp4_srv.cc +++ b/src/bin/dhcp4/ctrl_dhcp4_srv.cc @@ -241,8 +241,10 @@ ControlledDhcpv4Srv::~ControlledDhcpv4Srv() { try { cleanup(); - // Stop worker thread running timers, if it is running. - TimerMgr::instance()->stopThread(); + // Stop worker thread running timers, if it is running. Then + // unregister any timers. + timer_mgr_->stopThread(); + timer_mgr_->unregisterTimers(); // Close the command socket (if it exists). CommandMgr::instance().closeCommandSocket(); diff --git a/src/bin/dhcp4/tests/kea_controller_unittest.cc b/src/bin/dhcp4/tests/kea_controller_unittest.cc index 49c8282920..4e75770d5c 100644 --- a/src/bin/dhcp4/tests/kea_controller_unittest.cc +++ b/src/bin/dhcp4/tests/kea_controller_unittest.cc @@ -86,7 +86,7 @@ public: /// @brief Runs timers for specified time. /// - /// Internally, this method calls @c IfaceMgr::receive6 to run the + /// Internally, this method calls @c IfaceMgr::receive4 to run the /// callbacks for the installed timers. /// /// @param timeout_ms Amount of time after which the method returns. @@ -397,7 +397,10 @@ TEST_F(JSONFileBackendTest, timers) { EXPECT_TRUE(lease_expired->stateExpiredReclaimed()); // Second lease should have been removed. - EXPECT_FALSE(lease_mgr.getLease4(IOAddress("10.0.0.2"))); + ASSERT_NO_THROW( + lease_reclaimed = lease_mgr.getLease4(IOAddress("10.0.0.2")); + ); + EXPECT_FALSE(lease_reclaimed); } } // End of anonymous namespace diff --git a/src/bin/dhcp6/tests/kea_controller_unittest.cc b/src/bin/dhcp6/tests/kea_controller_unittest.cc index 125ade3a1b..4778670673 100644 --- a/src/bin/dhcp6/tests/kea_controller_unittest.cc +++ b/src/bin/dhcp6/tests/kea_controller_unittest.cc @@ -343,7 +343,10 @@ TEST_F(JSONFileBackendTest, timers) { EXPECT_TRUE(lease_expired->stateExpiredReclaimed()); // Second lease should have been removed. - EXPECT_FALSE(lease_mgr.getLease6(Lease::TYPE_NA, IOAddress("3000::2"))); + ASSERT_NO_THROW( + lease_reclaimed = lease_mgr.getLease6(Lease::TYPE_NA, IOAddress("3000::2")) + ); + EXPECT_FALSE(lease_reclaimed); } } // End of anonymous namespace diff --git a/src/lib/dhcpsrv/alloc_engine.cc b/src/lib/dhcpsrv/alloc_engine.cc index b947e18590..11753300d8 100644 --- a/src/lib/dhcpsrv/alloc_engine.cc +++ b/src/lib/dhcpsrv/alloc_engine.cc @@ -1631,7 +1631,7 @@ AllocEngine::reclaimExpiredLeases4(const size_t max_leases, const uint16_t timeo } LOG_DEBUG(alloc_engine_logger, ALLOC_ENGINE_DBG_TRACE, - ALLOC_ENGINE_V6_LEASES_RECLAMATION_TIMEOUT) + ALLOC_ENGINE_V4_LEASES_RECLAMATION_TIMEOUT) .arg(timeout); break; } diff --git a/src/lib/dhcpsrv/alloc_engine_messages.mes b/src/lib/dhcpsrv/alloc_engine_messages.mes index 5635df38af..1aab52f2b0 100644 --- a/src/lib/dhcpsrv/alloc_engine_messages.mes +++ b/src/lib/dhcpsrv/alloc_engine_messages.mes @@ -68,25 +68,6 @@ client sending the DHCPDISCOVER has a reservation for the specified address. The allocation engine will try to offer this address to the client. -% ALLOC_ENGINE_V4_RECLAIMED_LEASES_DELETE begin deletion of reclaimed leases expired more than %1 seconds ago -This debug message is issued when the allocation engine begins -deletion of the reclaimed leases which have expired more than -a specified number of seconds ago. This operation is triggered -periodically according to the "flush-reclaimed-timer-wait-time" -parameter. The "hold-reclaimed-time" parameter defines a number -of seconds for which the leases are stored before they are -removed. - -% ALLOC_ENGINE_V4_RECLAIMED_LEASES_DELETE_COMPLETE successfully deleted %1 expired-reclaimed leases -This debug message is issued when the server successfully deletes -"expired-reclaimed" leases from the lease database. The number of -deleted leases is included in the log message. - -% ALLOC_ENGINE_V4_RECLAIMED_LEASES_DELETE_FAILED deletion of expired-reclaimed leases failed: %1 -This error message is issued when the deletion of "expired-reclaimed" -leases from the database failed. The error message is appended to -the log message. - % ALLOC_ENGINE_V4_LEASE_RECLAMATION_FAILED failed to reclaim the lease %1: %2 This error message is logged when the allocation engine fails to reclaim an expired lease. The reason for the failure is included in the @@ -157,6 +138,25 @@ offer the lease specified in the hint. This situation may occur when: (a) client doesn't have any reservations, (b) client has reservation but the reserved address is leased to another client. +% ALLOC_ENGINE_V4_RECLAIMED_LEASES_DELETE begin deletion of reclaimed leases expired more than %1 seconds ago +This debug message is issued when the allocation engine begins +deletion of the reclaimed leases which have expired more than +a specified number of seconds ago. This operation is triggered +periodically according to the "flush-reclaimed-timer-wait-time" +parameter. The "hold-reclaimed-time" parameter defines a number +of seconds for which the leases are stored before they are +removed. + +% ALLOC_ENGINE_V4_RECLAIMED_LEASES_DELETE_COMPLETE successfully deleted %1 expired-reclaimed leases +This debug message is issued when the server successfully deletes +"expired-reclaimed" leases from the lease database. The number of +deleted leases is included in the log message. + +% ALLOC_ENGINE_V4_RECLAIMED_LEASES_DELETE_FAILED deletion of expired-reclaimed leases failed: %1 +This error message is issued when the deletion of "expired-reclaimed" +leases from the database failed. The error message is appended to +the log message. + % ALLOC_ENGINE_V4_REQUEST_ADDRESS_RESERVED %1: requested address %2 is reserved This message is issued when the allocation engine refused to allocate address requested by the client because this diff --git a/src/lib/dhcpsrv/tests/alloc_engine_expiration_unittest.cc b/src/lib/dhcpsrv/tests/alloc_engine_expiration_unittest.cc index 43aa51f36c..e10b5d8c3f 100644 --- a/src/lib/dhcpsrv/tests/alloc_engine_expiration_unittest.cc +++ b/src/lib/dhcpsrv/tests/alloc_engine_expiration_unittest.cc @@ -596,7 +596,7 @@ public: /// @brief Test that leases can be reclaimed without being removed. void testReclaimExpiredLeasesUpdateState() { - for (int i = 0; i < TEST_LEASES_NUM; ++i) { + for (unsigned int i = 0; i < TEST_LEASES_NUM; ++i) { // Mark leases with even indexes as expired. if (evenLeaseIndex(i)) { // The higher the index, the more expired the lease. @@ -617,7 +617,7 @@ public: /// @brief Test that the leases may be reclaimed by being deleted. void testReclaimExpiredLeasesDelete() { - for (int i = 0; i < TEST_LEASES_NUM; ++i) { + for (unsigned int i = 0; i < TEST_LEASES_NUM; ++i) { // Mark leases with even indexes as expired. if (evenLeaseIndex(i)) { // The higher the index, the more expired the lease. @@ -639,7 +639,7 @@ public: /// @brief Test that it is possible to specify the limit for the number /// of reclaimed leases. void testReclaimExpiredLeasesLimit() { - for (int i = 0; i < TEST_LEASES_NUM; ++i) { + for (unsigned int i = 0; i < TEST_LEASES_NUM; ++i) { // Mark all leaes as expired. The higher the index the less // expired the lease. expire(i, 1000 - i); @@ -652,7 +652,7 @@ public: BOOST_STATIC_ASSERT(TEST_LEASES_NUM % reclamation_group_size == 0); // Leases will be reclaimed in groups of 10. - for (int i = reclamation_group_size; i < TEST_LEASES_NUM; + for (unsigned int i = reclamation_group_size; i < TEST_LEASES_NUM; i += reclamation_group_size) { // Reclaim 10 most expired leases out of TEST_LEASES_NUM. Since @@ -680,7 +680,7 @@ public: // DNS must be started for the D2 client to accept NCRs. ASSERT_NO_THROW(enableDDNS()); - for (int i = 0; i < TEST_LEASES_NUM; ++i) { + for (unsigned int i = 0; i < TEST_LEASES_NUM; ++i) { // Expire all leases with even indexes. if (evenLeaseIndex(i)) { // The higher the index, the more expired the lease. @@ -709,7 +709,7 @@ public: // DNS must be started for the D2 client to accept NCRs. ASSERT_NO_THROW(enableDDNS()); - for (int i = 0; i < TEST_LEASES_NUM; ++i) { + for (unsigned int i = 0; i < TEST_LEASES_NUM; ++i) { // Expire only leases with even indexes. if (evenLeaseIndex(i)) { // The higher the index, the more expired the lease. @@ -721,7 +721,7 @@ public: BOOST_STATIC_ASSERT(TEST_LEASES_NUM % reclamation_group_size == 0); // Leases will be reclaimed in groups of 10 - for (int i = 10; i < TEST_LEASES_NUM; i += reclamation_group_size) { + for (unsigned int i = 10; i < TEST_LEASES_NUM; i += reclamation_group_size) { // Reclaim 10 most expired leases. Note that the leases with the // higher index are more expired. For example, if the // TEST_LEASES_NUM is equal to 100, the most expired lease will @@ -827,7 +827,7 @@ public: /// @brief This test verfies that callouts are executed for each expired /// lease when installed. void testReclaimExpiredLeasesHooks() { - for (int i = 0; i < TEST_LEASES_NUM; ++i) { + for (unsigned int i = 0; i < TEST_LEASES_NUM; ++i) { if (evenLeaseIndex(i)) { expire(i, 1000 - i); } @@ -857,7 +857,7 @@ public: /// @brief This test verfies that callouts are executed for each expired /// lease and that the lease is not reclaimed when skip flag is set. void testReclaimExpiredLeasesHooksWithSkip() { - for (int i = 0; i < TEST_LEASES_NUM; ++i) { + for (unsigned int i = 0; i < TEST_LEASES_NUM; ++i) { if (evenLeaseIndex(i)) { expire(i, 1000 - i); } @@ -887,7 +887,7 @@ public: /// the execution of the lease reclamation routine. void testReclaimExpiredLeasesTimeout(const uint16_t timeout) { // Leases are segregated from the most expired to the least expired. - for (int i = 0; i < TEST_LEASES_NUM; ++i) { + for (unsigned int i = 0; i < TEST_LEASES_NUM; ++i) { expire(i, 2000 - i); } @@ -929,7 +929,7 @@ public: /// @brief This test verifies that expired-reclaimed leases are removed /// from the lease database. void testDeleteExpiredReclaimedLeases() { - for (int i = 0; i < TEST_LEASES_NUM; ++i) { + for (unsigned int i = 0; i < TEST_LEASES_NUM; ++i) { // Mark leases with even indexes as expired. if (evenLeaseIndex(i)) { // The higher the index, the more expired the lease. @@ -1225,7 +1225,7 @@ ExpirationAllocEngine6Test::testReclaimExpiredLeasesStats() { // This test requires that the number of leases is an even number. BOOST_STATIC_ASSERT(TEST_LEASES_NUM % 2 == 0); - for (int i = 0; i < TEST_LEASES_NUM; ++i) { + for (unsigned int i = 0; i < TEST_LEASES_NUM; ++i) { // Mark all leaes as expired. The higher the index the less // expired the lease. expire(i, 1000 - i); @@ -1238,7 +1238,7 @@ ExpirationAllocEngine6Test::testReclaimExpiredLeasesStats() { // Leases will be reclaimed in groups of 8. const size_t reclamation_group_size = 8; - for (int i = reclamation_group_size; i < TEST_LEASES_NUM; + for (unsigned int i = reclamation_group_size; i < TEST_LEASES_NUM; i += reclamation_group_size) { // Reclaim 8 most expired leases out of TEST_LEASES_NUM. @@ -1570,7 +1570,7 @@ ExpirationAllocEngine4Test::testReclaimExpiredLeasesWithDDNSAndClientId() { // DNS must be started for the D2 client to accept NCRs. ASSERT_NO_THROW(enableDDNS()); - for (int i = 0; i < TEST_LEASES_NUM; ++i) { + for (unsigned int i = 0; i < TEST_LEASES_NUM; ++i) { // Set client identifiers for leases with even indexes only. if (evenLeaseIndex(i)) { setUniqueClientId(i); @@ -1599,7 +1599,7 @@ ExpirationAllocEngine4Test::testReclaimExpiredLeasesStats() { // This test requires that the number of leases is an even number. BOOST_STATIC_ASSERT(TEST_LEASES_NUM % 2 == 0); - for (int i = 0; i < TEST_LEASES_NUM; ++i) { + for (unsigned int i = 0; i < TEST_LEASES_NUM; ++i) { // Mark all leaes as expired. The higher the index the less // expired the lease. expire(i, 1000 - i); @@ -1611,7 +1611,7 @@ ExpirationAllocEngine4Test::testReclaimExpiredLeasesStats() { // Leases will be reclaimed in groups of 8. const size_t reclamation_group_size = 8; - for (int i = reclamation_group_size; i < TEST_LEASES_NUM; + for (unsigned int i = reclamation_group_size; i < TEST_LEASES_NUM; i += reclamation_group_size) { // Reclaim 8 most expired leases out of TEST_LEASES_NUM. diff --git a/src/lib/dhcpsrv/tests/cfg_expiration_unittest.cc b/src/lib/dhcpsrv/tests/cfg_expiration_unittest.cc index eb29bae3cf..8d5d36d0f7 100644 --- a/src/lib/dhcpsrv/tests/cfg_expiration_unittest.cc +++ b/src/lib/dhcpsrv/tests/cfg_expiration_unittest.cc @@ -194,6 +194,14 @@ public: /// @brief Maximum number of reclamation attempts after which all leases /// should be reclaimed. uint16_t max_unwarned_cycles; + + /// @brief Constructor + /// + /// Sets all numeric values to 0xFFFF and the boolean values to false. + RecordedParams() + : max_leases(0xFFFF), timeout(0xFFFF), remove_lease(false), + max_unwarned_cycles(0xFFFF) { + } }; /// @brief Constructor. @@ -346,6 +354,10 @@ public: // Test that the reclamation routines are called with the appropriate parameters. TEST_F(CfgExpirationTimersTest, reclamationParameters) { + // Set this value to true, to make sure that the timer callback would + // modify this value to false. + stub_->reclaim_params_.remove_lease = true; + // Set parameters to some non-default values. cfg_.setMaxReclaimLeases(1000); cfg_.setMaxReclaimTime(1500);