[3984] Changes after review:

 - Added comments in AllocEngine
 - decline message clarified
 - counters are now unsigned
 - several asserts added in unit-tests
 - missing doxygen entry added

src/lib/dhcpsrv/alloc_engine.cc

@@ -1402,11 +1402,14 @@ AllocEngine::reclaimExpiredLeases4(const size_t max_leases, const uint16_t timeo
                 queueRemovalNameChangeRequest(lease, lease->hwaddr_);
             }
 
+            // Let's check if the lease that just expired is in DECLINED state.
+            // If it is, we need to conduct couple extra steps and also force
+            // its removal.
             bool remove_tmp = remove_lease;
             if (lease->state_ == Lease::STATE_DECLINED) {
-                // There's no point keep declined lease after its reclaimation.
-                // Declined lease doesn't have any client identifying information
-                // anymore.
+                // There's no point in keeping declined lease after its
+                // reclaimation. Declined lease doesn't have any client
+                // identifying information anymore.
                 remove_tmp = true;
 
                 // Do extra steps required for declined lease reclaimation:
@@ -1462,8 +1465,9 @@ AllocEngine::reclaimDeclined(const Lease4Ptr& lease) {
         return;
     }
 
-    LOG_INFO(alloc_engine_logger, ALLOC_ENGINE_V4_DECLINED_RECOVERED).
-        arg(lease->addr_.toText()).arg(lease->valid_lft_);
+    LOG_INFO(alloc_engine_logger, ALLOC_ENGINE_V4_DECLINED_RECOVERED)
+        .arg(lease->addr_.toText())
+        .arg(lease->valid_lft_);
 
     StatsMgr& stats_mgr = StatsMgr::instance();
 

src/lib/dhcpsrv/alloc_engine_messages.mes

@@ -52,7 +52,8 @@ sooner.
 This informational message indicates that the specified address was reported
 as duplicate (client sent DECLINE) and the server marked this address as
 unvailable for a period of time. This time now has elapsed and the address
-is now returned to available pool. This step concludes decline recovery process.
+has been returned to the available pool. This step concludes decline recovery
+process.
 
 % ALLOC_ENGINE_V4_DISCOVER_ADDRESS_CONFLICT %1: conflicting reservation for address %2 with existing lease %3
 This warning message is issued when the DHCP server finds that the

src/lib/dhcpsrv/tests/alloc_engine_expiration_unittest.cc

@@ -737,7 +737,7 @@ public:
     ///
     /// @param remove see description above
     void testReclaimDeclined4(bool remove) {
-        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)) {
@@ -770,7 +770,7 @@ public:
         int subnet2_cnt = 0;
 
         // Let's move all leases to declined,expired state.
-        for (int i = 0; i < TEST_LEASES_NUM; ++i) {
+        for (unsigned int i = 0; i < TEST_LEASES_NUM; ++i) {
 
             // Move the lease to declined state
             decline(i, 100);

src/lib/dhcpsrv/tests/alloc_engine_utils.cc

@@ -77,9 +77,9 @@ AllocEngine4Test::testReuseLease4(const AllocEnginePtr& engine,
     ASSERT_TRUE(engine);
 
     if (existing_lease) {
-        // If old lease was specified, we need to add it to the database.
-        // Let's wipe any leases for that address (if any). We ignore
-        // any errors (previous lease may not exist)
+        // If an existing lease was specified, we need to add it to the
+        // database. Let's wipe any leases for that address (if any). We
+        // ignore any errors (previous lease may not exist)
         LeaseMgrFactory::instance().deleteLease(existing_lease->addr_);
 
         // Let's add it.
@@ -114,12 +114,14 @@ Lease4Ptr
 AllocEngine4Test::generateDeclinedLease(const std::string& addr,
                                         time_t probation_period,
                                         int32_t expired) {
+    // There's an assumption that hardware address is always present for IPv4
+    // packet (always non-null). Client-id is optional (may be null).
     HWAddrPtr hwaddr(new HWAddr());
     time_t now = time(NULL);
     Lease4Ptr declined(new Lease4(addr, hwaddr, ClientIdPtr(), 495,
                                   100, 200, now, subnet_->getID()));
     declined->decline(probation_period);
-    declined->cltt_ = time(NULL) - probation_period + expired;
+    declined->cltt_ = now - probation_period + expired;
     return (declined);
 }
 

src/lib/dhcpsrv/tests/alloc_engine_utils.h

@@ -389,7 +389,7 @@ public:
 
     /// @brief Generic test used for lease allocation and reuse
     ///
-    /// This test inserts old_lease (if specified, may be null) into the
+    /// This test inserts existing_lease (if specified, may be null) into the
     /// LeaseMgr, then conducts lease allocation (pretends that client
     /// sent either Discover or Request, depending on fake_allocation).
     /// Allocated lease is then returned (using result) for further inspection.
@@ -397,6 +397,7 @@ public:
     /// @param alloc_engine allocation engine
     /// @param existing_lease optional lease to be inserted in the database
     /// @param addr address to be requested by client
+    /// @param fake_allocation true = DISCOVER, false = REQUEST
     /// @param exp_result expected result
     /// @param result [out] allocated lease
     void testReuseLease4(const AllocEnginePtr& alloc_engine,
@@ -410,9 +411,9 @@ public:
     ///
     /// expired parameter controls probation period. Positive value
     /// means that the lease will expire in X seconds. Negative means
-    /// that the lease had expired X seconds ago. 0 means it expires now.
+    /// that the lease expired X seconds ago. 0 means it expires now.
     /// Probation period is a parameter that specifies for how long
-    /// a lease will become unavailable after decline.
+    /// a lease will stay unavailable after decline.
     ///
     /// @param addr address of the lease
     /// @param probation_period expressed in seconds

src/lib/dhcpsrv/tests/generic_lease_mgr_unittest.cc

@@ -1691,6 +1691,7 @@ GenericLeaseMgrTest::testGetExpiredLeases4() {
         int index = static_cast<int>(std::distance(expired_leases.rbegin(), lease));
         // Multiple current index by two, because only leases with even indexes
         // should have been returned.
+        ASSERT_LE(2 * index, leases.size());
         EXPECT_EQ(leases[2 * index]->addr_, (*lease)->addr_);
     }
 
@@ -1723,6 +1724,7 @@ GenericLeaseMgrTest::testGetExpiredLeases4() {
     for (Lease4Collection::iterator lease = expired_leases.begin();
          lease != expired_leases.end(); ++lease) {
         int index = static_cast<int>(std::distance(expired_leases.begin(), lease));
+        ASSERT_LE(2 * index, leases.size());
         EXPECT_EQ(leases[2 * index]->addr_, (*lease)->addr_);
     }
 
@@ -1742,6 +1744,7 @@ GenericLeaseMgrTest::testGetExpiredLeases4() {
     for (Lease4Collection::iterator lease = expired_leases.begin();
          lease != expired_leases.end(); ++lease) {
         int index = static_cast<int>(std::distance(expired_leases.begin(), lease));
+        ASSERT_LE(2 * index, leases.size());
         EXPECT_EQ(leases[2 * index]->addr_, (*lease)->addr_);
     }
 
@@ -2104,7 +2107,7 @@ GenericLeaseMgrTest::testGetDeclinedLeases4() {
             leases[i]->decline(1000);
         }
 
-        // Mark every other lease as expired.
+        // Mark every second lease as expired.
         if (i % 2 == 0) {
             // Set client last transmission time to the value older than the
             // valid lifetime to make it expired. The expiration time also
@@ -2140,8 +2143,8 @@ GenericLeaseMgrTest::testGetDeclinedLeases4() {
     // not pay attention to state, just expiration timers.
     ASSERT_EQ(static_cast<size_t>(leases.size() / 2), expired_leases.size());
 
-    int declined_state = 0;
-    int default_state = 0;
+    unsigned int declined_state = 0;
+    unsigned int default_state = 0;
 
     // The expired leases should be returned from the most to least expired.
     // This matches the reverse order to which they have been added.