[3975] Addressed review comments.

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();

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

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

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;
         }

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

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.

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);