From: Tomek Mrugalski Date: Thu, 1 Oct 2015 12:04:07 +0000 (+0200) Subject: [3984] Changes after review: X-Git-Tag: trac4074_base~7^2 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=d6d0a5437591c3613440403d64e5cff94f83070a;p=thirdparty%2Fkea.git [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 --- diff --git a/src/lib/dhcpsrv/alloc_engine.cc b/src/lib/dhcpsrv/alloc_engine.cc index e3ac0696ee..1f17447e10 100644 --- a/src/lib/dhcpsrv/alloc_engine.cc +++ b/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(); diff --git a/src/lib/dhcpsrv/alloc_engine_messages.mes b/src/lib/dhcpsrv/alloc_engine_messages.mes index e490139ca0..474338ddb2 100644 --- a/src/lib/dhcpsrv/alloc_engine_messages.mes +++ b/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 diff --git a/src/lib/dhcpsrv/tests/alloc_engine_expiration_unittest.cc b/src/lib/dhcpsrv/tests/alloc_engine_expiration_unittest.cc index 87175f9b01..55ab33e26c 100644 --- a/src/lib/dhcpsrv/tests/alloc_engine_expiration_unittest.cc +++ b/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); diff --git a/src/lib/dhcpsrv/tests/alloc_engine_utils.cc b/src/lib/dhcpsrv/tests/alloc_engine_utils.cc index 402827e9f7..8b8bbe9f38 100644 --- a/src/lib/dhcpsrv/tests/alloc_engine_utils.cc +++ b/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); } diff --git a/src/lib/dhcpsrv/tests/alloc_engine_utils.h b/src/lib/dhcpsrv/tests/alloc_engine_utils.h index 6c9aa59de2..d00158501f 100644 --- a/src/lib/dhcpsrv/tests/alloc_engine_utils.h +++ b/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 diff --git a/src/lib/dhcpsrv/tests/generic_lease_mgr_unittest.cc b/src/lib/dhcpsrv/tests/generic_lease_mgr_unittest.cc index 0e5ba491b9..efcb58d91e 100644 --- a/src/lib/dhcpsrv/tests/generic_lease_mgr_unittest.cc +++ b/src/lib/dhcpsrv/tests/generic_lease_mgr_unittest.cc @@ -1691,6 +1691,7 @@ GenericLeaseMgrTest::testGetExpiredLeases4() { int index = static_cast(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(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(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(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.