From ea9b68c038d0f757c4703bb70080e45ae9b2cceb Mon Sep 17 00:00:00 2001 From: Razvan Becheriu Date: Thu, 12 Dec 2019 13:08:39 +0200 Subject: [PATCH 1/9] [#1040] throw exception on delete non existent lease --- src/lib/dhcpsrv/cql_lease_mgr.cc | 4 +- src/lib/dhcpsrv/mysql_lease_mgr.cc | 40 +++++++++++++++++-- src/lib/dhcpsrv/pgsql_lease_mgr.cc | 36 ++++++++++++++++- .../tests/generic_lease_mgr_unittest.cc | 8 ++-- 4 files changed, 77 insertions(+), 11 deletions(-) diff --git a/src/lib/dhcpsrv/cql_lease_mgr.cc b/src/lib/dhcpsrv/cql_lease_mgr.cc index 1ac260b26c..be27b5d950 100644 --- a/src/lib/dhcpsrv/cql_lease_mgr.cc +++ b/src/lib/dhcpsrv/cql_lease_mgr.cc @@ -2660,7 +2660,7 @@ CqlLeaseMgr::deleteLease(const Lease4Ptr &lease) { } catch (const Exception &exception) { LOG_DEBUG(dhcpsrv_logger, DHCPSRV_DBG_TRACE_DETAIL, DHCPSRV_CQL_LEASE_EXCEPTION_THROWN) .arg(exception.what()); - return false; + isc_throw(NoSuchLease, exception.what()); } return true; } @@ -2684,7 +2684,7 @@ CqlLeaseMgr::deleteLease(const Lease6Ptr &lease) { } catch (const Exception &exception) { LOG_DEBUG(dhcpsrv_logger, DHCPSRV_DBG_TRACE_DETAIL, DHCPSRV_CQL_LEASE_EXCEPTION_THROWN) .arg(exception.what()); - return false; + isc_throw(NoSuchLease, exception.what()); } return true; } diff --git a/src/lib/dhcpsrv/mysql_lease_mgr.cc b/src/lib/dhcpsrv/mysql_lease_mgr.cc index 465201ca30..6dea85aca6 100644 --- a/src/lib/dhcpsrv/mysql_lease_mgr.cc +++ b/src/lib/dhcpsrv/mysql_lease_mgr.cc @@ -2836,7 +2836,9 @@ MySqlLeaseMgr::deleteLeaseCommon(StatementIndex stindex, MYSQL_BIND* bind) { // See how many rows were affected. Note that the statement may delete // multiple rows. - return (static_cast(mysql_stmt_affected_rows(ctx->conn_.statements_[stindex]))); + int affected_rows = mysql_stmt_affected_rows(ctx->conn_.statements_[stindex]); + + return (affected_rows); } bool @@ -2862,7 +2864,23 @@ MySqlLeaseMgr::deleteLease(const Lease4Ptr& lease) { inbind[1].buffer = reinterpret_cast(&expire); inbind[1].buffer_length = sizeof(expire); - return (deleteLeaseCommon(DELETE_LEASE4, inbind) > 0); + auto affected_rows = deleteLeaseCommon(DELETE_LEASE4, inbind); + + // Check success case first as it is the most likely outcome. + if (affected_rows == 1) { + return (true); + } + + // If no rows affected, lease doesn't exist. + if (affected_rows == 0) { + isc_throw(NoSuchLease, "unable to update lease for address " << + lease->addr_.toText() << " as it does not exist"); + } + + // Should not happen - primary key constraint should only have selected + // one row. + isc_throw(DbOperationError, "apparently updated more than one lease " + "that had the address " << lease->addr_.toText()); } bool @@ -2893,7 +2911,23 @@ MySqlLeaseMgr::deleteLease(const Lease6Ptr& lease) { inbind[1].buffer = reinterpret_cast(&expire); inbind[1].buffer_length = sizeof(expire); - return (deleteLeaseCommon(DELETE_LEASE6, inbind) > 0); + auto affected_rows = deleteLeaseCommon(DELETE_LEASE6, inbind); + + // Check success case first as it is the most likely outcome. + if (affected_rows == 1) { + return (true); + } + + // If no rows affected, lease doesn't exist. + if (affected_rows == 0) { + isc_throw(NoSuchLease, "unable to update lease for address " << + lease->addr_.toText() << " as it does not exist"); + } + + // Should not happen - primary key constraint should only have selected + // one row. + isc_throw(DbOperationError, "apparently updated more than one lease " + "that had the address " << lease->addr_.toText()); } uint64_t diff --git a/src/lib/dhcpsrv/pgsql_lease_mgr.cc b/src/lib/dhcpsrv/pgsql_lease_mgr.cc index e618238bb9..c43c451ba4 100644 --- a/src/lib/dhcpsrv/pgsql_lease_mgr.cc +++ b/src/lib/dhcpsrv/pgsql_lease_mgr.cc @@ -2037,7 +2037,23 @@ PgSqlLeaseMgr::deleteLease(const Lease4Ptr& lease) { lease->old_valid_lft_); bind_array.add(expire_str); - return (deleteLeaseCommon(DELETE_LEASE4, bind_array) > 0); + auto affected_rows = deleteLeaseCommon(DELETE_LEASE4, bind_array); + + // Check success case first as it is the most likely outcome. + if (affected_rows == 1) { + return (true); + } + + // If no rows affected, lease doesn't exist. + if (affected_rows == 0) { + isc_throw(NoSuchLease, "unable to update lease for address " << + lease->addr_.toText() << " as it does not exist"); + } + + // Should not happen - primary key constraint should only have selected + // one row. + isc_throw(DbOperationError, "apparently updated more than one lease " + "that had the address " << lease->addr_.toText()); } bool @@ -2057,7 +2073,23 @@ PgSqlLeaseMgr::deleteLease(const Lease6Ptr& lease) { lease->old_valid_lft_); bind_array.add(expire_str); - return (deleteLeaseCommon(DELETE_LEASE6, bind_array) > 0); + auto affected_rows = deleteLeaseCommon(DELETE_LEASE6, bind_array); + + // Check success case first as it is the most likely outcome. + if (affected_rows == 1) { + return (true); + } + + // If no rows affected, lease doesn't exist. + if (affected_rows == 0) { + isc_throw(NoSuchLease, "unable to update lease for address " << + lease->addr_.toText() << " as it does not exist"); + } + + // Should not happen - primary key constraint should only have selected + // one row. + isc_throw(DbOperationError, "apparently updated more than one lease " + "that had the address " << lease->addr_.toText()); } uint64_t diff --git a/src/lib/dhcpsrv/tests/generic_lease_mgr_unittest.cc b/src/lib/dhcpsrv/tests/generic_lease_mgr_unittest.cc index 41ded622dc..5dbac0eeda 100644 --- a/src/lib/dhcpsrv/tests/generic_lease_mgr_unittest.cc +++ b/src/lib/dhcpsrv/tests/generic_lease_mgr_unittest.cc @@ -528,7 +528,7 @@ GenericLeaseMgrTest::testLease4NullClientId() { EXPECT_TRUE(lmptr_->deleteLease(leases[1])); l_returned = lmptr_->getLease4(ioaddress4_[1]); EXPECT_FALSE(l_returned); - EXPECT_FALSE(lmptr_->deleteLease(leases[1])); + EXPECT_THROW(lmptr_->deleteLease(leases[1]), isc::dhcp::NoSuchLease); // Check that the second address is still there. l_returned = lmptr_->getLease4(ioaddress4_[2]); @@ -708,7 +708,7 @@ GenericLeaseMgrTest::testAddGetDelete6() { Lease6Ptr non_existing_lease(new Lease6(Lease::TYPE_NA, IOAddress(addr789), duid, iaid, 100, 200, subnet_id)); - EXPECT_FALSE(lmptr_->deleteLease(non_existing_lease)); + EXPECT_THROW(lmptr_->deleteLease(non_existing_lease), isc::dhcp::NoSuchLease); // this one should succeed EXPECT_TRUE(lmptr_->deleteLease(x)); @@ -800,7 +800,7 @@ GenericLeaseMgrTest::testBasicLease4() { EXPECT_TRUE(lmptr_->deleteLease(leases[1])); l_returned = lmptr_->getLease4(ioaddress4_[1]); EXPECT_FALSE(l_returned); - EXPECT_FALSE(lmptr_->deleteLease(leases[1])); + EXPECT_THROW(lmptr_->deleteLease(leases[1]), isc::dhcp::NoSuchLease); // Check that the second address is still there. l_returned = lmptr_->getLease4(ioaddress4_[2]); @@ -878,7 +878,7 @@ GenericLeaseMgrTest::testBasicLease6() { EXPECT_TRUE(lmptr_->deleteLease(leases[1])); l_returned = lmptr_->getLease6(leasetype6_[1], ioaddress6_[1]); EXPECT_FALSE(l_returned); - EXPECT_FALSE(lmptr_->deleteLease(leases[1])); + EXPECT_THROW(lmptr_->deleteLease(leases[1]), isc::dhcp::NoSuchLease); // Check that the second address is still there. l_returned = lmptr_->getLease6(leasetype6_[2], ioaddress6_[2]); -- GitLab From 010fd685297686b4804131fd20a65e3ffb095cda Mon Sep 17 00:00:00 2001 From: Razvan Becheriu Date: Thu, 12 Dec 2019 13:12:52 +0200 Subject: [PATCH 2/9] [#1040] throw exception on delete non existent lease --- src/lib/dhcpsrv/memfile_lease_mgr.cc | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/src/lib/dhcpsrv/memfile_lease_mgr.cc b/src/lib/dhcpsrv/memfile_lease_mgr.cc index eec7b2d758..0398f419ca 100644 --- a/src/lib/dhcpsrv/memfile_lease_mgr.cc +++ b/src/lib/dhcpsrv/memfile_lease_mgr.cc @@ -405,7 +405,7 @@ public: int64_t assigned = 0; int64_t declined = 0; for (Lease4StorageSubnetIdIndex::const_iterator lease = lower; - lease != upper; ++lease) { + lease != upper; ++lease) { // If we've hit the next subnet, add rows for the current subnet // and wipe the accumulators if ((*lease)->subnet_id_ != cur_id) { @@ -548,7 +548,7 @@ public: int64_t declined = 0; int64_t assigned_pds = 0; for (Lease6StorageSubnetIdIndex::const_iterator lease = lower; - lease != upper; ++lease) { + lease != upper; ++lease) { // If we've hit the next subnet, add rows for the current subnet // and wipe the accumulators if ((*lease)->subnet_id_ != cur_id) { @@ -1105,8 +1105,9 @@ Memfile_LeaseMgr::getLeases6Internal(Lease::Type type, std::pair l = idx.equal_range(boost::make_tuple(duid.getDuid(), iaid, type)); + for (Lease6StorageDuidIaidTypeIndex::const_iterator lease = - l.first; lease != l.second; ++lease) { + l.first; lease != l.second; ++lease) { collection.push_back(Lease6Ptr(new Lease6(**lease))); } } @@ -1144,8 +1145,9 @@ Memfile_LeaseMgr::getLeases6Internal(Lease::Type type, std::pair l = idx.equal_range(boost::make_tuple(duid.getDuid(), iaid, type)); + for (Lease6StorageDuidIaidTypeIndex::const_iterator lease = - l.first; lease != l.second; ++lease) { + l.first; lease != l.second; ++lease) { // Filter out the leases which subnet id doesn't match. if ((*lease)->subnet_id_ == subnet_id) { collection.push_back(Lease6Ptr(new Lease6(**lease))); @@ -1484,7 +1486,8 @@ Memfile_LeaseMgr::deleteLeaseInternal(const Lease4Ptr& lease) { Lease4Storage::iterator l = storage4_.find(addr); if (l == storage4_.end()) { // No such lease - return (false); + isc_throw(NoSuchLease, "unable to update lease for address " << + lease->addr_.toText() << " as it does not exist"); } else { if (persistLeases(V4)) { // Copy the lease. The valid lifetime needs to be modified and @@ -1523,7 +1526,8 @@ Memfile_LeaseMgr::deleteLeaseInternal(const Lease6Ptr& lease) { Lease6Storage::iterator l = storage6_.find(addr); if (l == storage6_.end()) { // No such lease - return (false); + isc_throw(NoSuchLease, "unable to update lease for address " << + lease->addr_.toText() << " as it does not exist"); } else { if (persistLeases(V6)) { // Copy the lease. The lifetimes need to be modified and we -- GitLab From 3d7fee96be9759f4ea495f740fb6409204ec64fd Mon Sep 17 00:00:00 2001 From: Razvan Becheriu Date: Thu, 12 Dec 2019 13:59:43 +0200 Subject: [PATCH 3/9] [#1040] fixed unit tests --- src/lib/dhcpsrv/tests/alloc_engine_utils.cc | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/src/lib/dhcpsrv/tests/alloc_engine_utils.cc b/src/lib/dhcpsrv/tests/alloc_engine_utils.cc index 3348807cd2..90b3529a6b 100644 --- a/src/lib/dhcpsrv/tests/alloc_engine_utils.cc +++ b/src/lib/dhcpsrv/tests/alloc_engine_utils.cc @@ -80,7 +80,11 @@ AllocEngine4Test::testReuseLease4(const AllocEnginePtr& engine, // 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); + try { + LeaseMgrFactory::instance().deleteLease(existing_lease); + } catch (...) { + // catch all exceptions + } // Let's add it. ASSERT_TRUE(LeaseMgrFactory::instance().addLease(existing_lease)); @@ -534,7 +538,11 @@ AllocEngine6Test::testReuseLease6(const AllocEnginePtr& engine, // 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); + try { + LeaseMgrFactory::instance().deleteLease(existing_lease); + } catch (...) { + // catch all exceptions + } // Let's add it. ASSERT_TRUE(LeaseMgrFactory::instance().addLease(existing_lease)); -- GitLab From eefabada978cd5aa41bae2f776699fbd409a307e Mon Sep 17 00:00:00 2001 From: Razvan Becheriu Date: Thu, 12 Dec 2019 14:14:32 +0200 Subject: [PATCH 4/9] [#1040] ignore delete errors related to concurrent deletes on delete reclaimed leases --- src/lib/dhcpsrv/cql_lease_mgr.cc | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/src/lib/dhcpsrv/cql_lease_mgr.cc b/src/lib/dhcpsrv/cql_lease_mgr.cc index be27b5d950..ee89413be2 100644 --- a/src/lib/dhcpsrv/cql_lease_mgr.cc +++ b/src/lib/dhcpsrv/cql_lease_mgr.cc @@ -2713,8 +2713,12 @@ CqlLeaseMgr::deleteExpiredReclaimedLeases4(const uint32_t secs) { std::unique_ptr exchange4(new CqlLease4Exchange(dbconn_)); exchange4->getLeaseCollection(CqlLease4Exchange::GET_LEASE4_EXPIRE, data, leases); for (Lease4Ptr &lease : leases) { - if (deleteLease(lease)) { - ++deleted; + try { + if (deleteLease(lease)) { + ++deleted; + } + } catch (NoSuchLease &ex) { + // catch exceptions related to concurrent deletes } } return (deleted); @@ -2744,8 +2748,12 @@ CqlLeaseMgr::deleteExpiredReclaimedLeases6(const uint32_t secs) { std::unique_ptr exchange6(new CqlLease6Exchange(dbconn_)); exchange6->getLeaseCollection(CqlLease6Exchange::GET_LEASE6_EXPIRE, data, leases); for (Lease6Ptr &lease : leases) { - if (deleteLease(lease)) { - ++deleted; + try { + if (deleteLease(lease)) { + ++deleted; + } + } catch (NoSuchLease &ex) { + // catch exceptions related to concurrent deletes } } return (deleted); -- GitLab From 366a23369a4d859d5b74e2b218e7301b22720d96 Mon Sep 17 00:00:00 2001 From: Razvan Becheriu Date: Thu, 12 Dec 2019 14:59:14 +0200 Subject: [PATCH 5/9] [#1040] fixed unit tests --- src/hooks/dhcp/lease_cmds/lease_cmds.cc | 33 ++++++++++++++++++------- 1 file changed, 24 insertions(+), 9 deletions(-) diff --git a/src/hooks/dhcp/lease_cmds/lease_cmds.cc b/src/hooks/dhcp/lease_cmds/lease_cmds.cc index c78e8e1cdc..88b5887629 100644 --- a/src/hooks/dhcp/lease_cmds/lease_cmds.cc +++ b/src/hooks/dhcp/lease_cmds/lease_cmds.cc @@ -1071,9 +1071,13 @@ LeaseCmdsImpl::lease4DelHandler(CalloutHandle& handle) { } } - if (LeaseMgrFactory::instance().deleteLease(lease4)) { - setSuccessResponse(handle, "IPv4 lease deleted."); - } else { + try { + if (LeaseMgrFactory::instance().deleteLease(lease)) { + setSuccessResponse(handle, "IPv4 lease deleted."); + } else { + setErrorResponse (handle, "IPv4 lease not found.", CONTROL_RESULT_EMPTY); + } + } catch (NoSuchLease &ex) { setErrorResponse (handle, "IPv4 lease not found.", CONTROL_RESULT_EMPTY); } } catch (const std::exception& ex) { @@ -1169,10 +1173,17 @@ LeaseCmdsImpl::lease6BulkApplyHandler(CalloutHandle& handle) { // This may throw if the lease couldn't be deleted for // any reason, but we still want to proceed with other // leases. - if (LeaseMgrFactory::instance().deleteLease(lease)) { - ++success_count; + bool deleted = false; + try { + if (LeaseMgrFactory::instance().deleteLease(lease)) { + ++success_count; + deleted = true; + } + } catch (NoSuchLease &ex) { + // catch exceptions related to concurrent deletes + } - } else { + if (!deleted) { // Lazy creation of the list of leases which failed to delete. if (!failed_deleted_list) { failed_deleted_list = Element::createList(); @@ -1318,9 +1329,13 @@ LeaseCmdsImpl::lease6DelHandler(CalloutHandle& handle) { } } - if (LeaseMgrFactory::instance().deleteLease(lease6)) { - setSuccessResponse(handle, "IPv6 lease deleted."); - } else { + try { + if (LeaseMgrFactory::instance().deleteLease(lease)) { + setSuccessResponse(handle, "IPv6 lease deleted."); + } else { + setErrorResponse (handle, "IPv6 lease not found.", CONTROL_RESULT_EMPTY); + } + } catch (NoSuchLease &ex) { setErrorResponse (handle, "IPv6 lease not found.", CONTROL_RESULT_EMPTY); } } catch (const std::exception& ex) { -- GitLab From 4d18eb7789a9dabde665b2b6125f36d5ec755921 Mon Sep 17 00:00:00 2001 From: Razvan Becheriu Date: Fri, 10 Jan 2020 18:04:17 +0200 Subject: [PATCH 6/9] [#1040] fixed exception messages --- src/lib/dhcpsrv/memfile_lease_mgr.cc | 4 ++-- src/lib/dhcpsrv/mysql_lease_mgr.cc | 8 ++++---- src/lib/dhcpsrv/pgsql_lease_mgr.cc | 8 ++++---- 3 files changed, 10 insertions(+), 10 deletions(-) diff --git a/src/lib/dhcpsrv/memfile_lease_mgr.cc b/src/lib/dhcpsrv/memfile_lease_mgr.cc index 0398f419ca..5b5282713f 100644 --- a/src/lib/dhcpsrv/memfile_lease_mgr.cc +++ b/src/lib/dhcpsrv/memfile_lease_mgr.cc @@ -1486,7 +1486,7 @@ Memfile_LeaseMgr::deleteLeaseInternal(const Lease4Ptr& lease) { Lease4Storage::iterator l = storage4_.find(addr); if (l == storage4_.end()) { // No such lease - isc_throw(NoSuchLease, "unable to update lease for address " << + isc_throw(NoSuchLease, "unable to delete lease for address " << lease->addr_.toText() << " as it does not exist"); } else { if (persistLeases(V4)) { @@ -1526,7 +1526,7 @@ Memfile_LeaseMgr::deleteLeaseInternal(const Lease6Ptr& lease) { Lease6Storage::iterator l = storage6_.find(addr); if (l == storage6_.end()) { // No such lease - isc_throw(NoSuchLease, "unable to update lease for address " << + isc_throw(NoSuchLease, "unable to delete lease for address " << lease->addr_.toText() << " as it does not exist"); } else { if (persistLeases(V6)) { diff --git a/src/lib/dhcpsrv/mysql_lease_mgr.cc b/src/lib/dhcpsrv/mysql_lease_mgr.cc index 6dea85aca6..5e11da6fc8 100644 --- a/src/lib/dhcpsrv/mysql_lease_mgr.cc +++ b/src/lib/dhcpsrv/mysql_lease_mgr.cc @@ -2873,13 +2873,13 @@ MySqlLeaseMgr::deleteLease(const Lease4Ptr& lease) { // If no rows affected, lease doesn't exist. if (affected_rows == 0) { - isc_throw(NoSuchLease, "unable to update lease for address " << + isc_throw(NoSuchLease, "unable to delete lease for address " << lease->addr_.toText() << " as it does not exist"); } // Should not happen - primary key constraint should only have selected // one row. - isc_throw(DbOperationError, "apparently updated more than one lease " + isc_throw(DbOperationError, "apparently deleted more than one lease " "that had the address " << lease->addr_.toText()); } @@ -2920,13 +2920,13 @@ MySqlLeaseMgr::deleteLease(const Lease6Ptr& lease) { // If no rows affected, lease doesn't exist. if (affected_rows == 0) { - isc_throw(NoSuchLease, "unable to update lease for address " << + isc_throw(NoSuchLease, "unable to delete lease for address " << lease->addr_.toText() << " as it does not exist"); } // Should not happen - primary key constraint should only have selected // one row. - isc_throw(DbOperationError, "apparently updated more than one lease " + isc_throw(DbOperationError, "apparently deleted more than one lease " "that had the address " << lease->addr_.toText()); } diff --git a/src/lib/dhcpsrv/pgsql_lease_mgr.cc b/src/lib/dhcpsrv/pgsql_lease_mgr.cc index c43c451ba4..f30587c236 100644 --- a/src/lib/dhcpsrv/pgsql_lease_mgr.cc +++ b/src/lib/dhcpsrv/pgsql_lease_mgr.cc @@ -2046,13 +2046,13 @@ PgSqlLeaseMgr::deleteLease(const Lease4Ptr& lease) { // If no rows affected, lease doesn't exist. if (affected_rows == 0) { - isc_throw(NoSuchLease, "unable to update lease for address " << + isc_throw(NoSuchLease, "unable to delete lease for address " << lease->addr_.toText() << " as it does not exist"); } // Should not happen - primary key constraint should only have selected // one row. - isc_throw(DbOperationError, "apparently updated more than one lease " + isc_throw(DbOperationError, "apparently deleted more than one lease " "that had the address " << lease->addr_.toText()); } @@ -2082,13 +2082,13 @@ PgSqlLeaseMgr::deleteLease(const Lease6Ptr& lease) { // If no rows affected, lease doesn't exist. if (affected_rows == 0) { - isc_throw(NoSuchLease, "unable to update lease for address " << + isc_throw(NoSuchLease, "unable to delete lease for address " << lease->addr_.toText() << " as it does not exist"); } // Should not happen - primary key constraint should only have selected // one row. - isc_throw(DbOperationError, "apparently updated more than one lease " + isc_throw(DbOperationError, "apparently deleted more than one lease " "that had the address " << lease->addr_.toText()); } -- GitLab From 7df0151c8df06e4b54657ab467c7fa56bc4fae79 Mon Sep 17 00:00:00 2001 From: Razvan Becheriu Date: Tue, 21 Jan 2020 01:00:44 +0200 Subject: [PATCH 7/9] [#1040] fixed rebase --- src/lib/dhcpsrv/mysql_lease_mgr.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/lib/dhcpsrv/mysql_lease_mgr.cc b/src/lib/dhcpsrv/mysql_lease_mgr.cc index 5e11da6fc8..c770401dc9 100644 --- a/src/lib/dhcpsrv/mysql_lease_mgr.cc +++ b/src/lib/dhcpsrv/mysql_lease_mgr.cc @@ -2911,7 +2911,7 @@ MySqlLeaseMgr::deleteLease(const Lease6Ptr& lease) { inbind[1].buffer = reinterpret_cast(&expire); inbind[1].buffer_length = sizeof(expire); - auto affected_rows = deleteLeaseCommon(DELETE_LEASE6, inbind); + auto affected_rows = deleteLeaseCommon(DELETE_LEASE6, inbind); // Check success case first as it is the most likely outcome. if (affected_rows == 1) { -- GitLab From b724a342f44726cdaed615f8dc29cf16f29a127f Mon Sep 17 00:00:00 2001 From: Razvan Becheriu Date: Tue, 21 Jan 2020 01:16:45 +0200 Subject: [PATCH 8/9] [#1040] fixed rebase --- src/hooks/dhcp/lease_cmds/lease_cmds.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/hooks/dhcp/lease_cmds/lease_cmds.cc b/src/hooks/dhcp/lease_cmds/lease_cmds.cc index 88b5887629..701958b264 100644 --- a/src/hooks/dhcp/lease_cmds/lease_cmds.cc +++ b/src/hooks/dhcp/lease_cmds/lease_cmds.cc @@ -1072,7 +1072,7 @@ LeaseCmdsImpl::lease4DelHandler(CalloutHandle& handle) { } try { - if (LeaseMgrFactory::instance().deleteLease(lease)) { + if (LeaseMgrFactory::instance().deleteLease(lease4)) { setSuccessResponse(handle, "IPv4 lease deleted."); } else { setErrorResponse (handle, "IPv4 lease not found.", CONTROL_RESULT_EMPTY); @@ -1330,7 +1330,7 @@ LeaseCmdsImpl::lease6DelHandler(CalloutHandle& handle) { } try { - if (LeaseMgrFactory::instance().deleteLease(lease)) { + if (LeaseMgrFactory::instance().deleteLease(lease6)) { setSuccessResponse(handle, "IPv6 lease deleted."); } else { setErrorResponse (handle, "IPv6 lease not found.", CONTROL_RESULT_EMPTY); -- GitLab From f03386bef2a68f2a8eeefb373abcb8a3f6c24345 Mon Sep 17 00:00:00 2001 From: Razvan Becheriu Date: Thu, 23 Jan 2020 15:18:21 +0200 Subject: [PATCH 9/9] [#1040] properly update stats after update function calls --- src/bin/dhcp4/dhcp4_srv.cc | 18 ++++++------ src/bin/dhcp6/dhcp6_srv.cc | 15 ++++++---- src/lib/dhcpsrv/alloc_engine.cc | 49 ++++++++++++++++----------------- 3 files changed, 41 insertions(+), 41 deletions(-) diff --git a/src/bin/dhcp4/dhcp4_srv.cc b/src/bin/dhcp4/dhcp4_srv.cc index 668940c96b..05a0514aec 100644 --- a/src/bin/dhcp4/dhcp4_srv.cc +++ b/src/bin/dhcp4/dhcp4_srv.cc @@ -3043,6 +3043,15 @@ Dhcpv4Srv::declineLease(const Lease4Ptr& lease, const Pkt4Ptr& decline, // queueNCR will do the necessary checks and will skip the update, if not needed. queueNCR(CHG_REMOVE, lease); + // @todo: Call hooks. + + // We need to disassociate the lease from the client. Once we move a lease + // to declined state, it is no longer associated with the client in any + // way. + lease->decline(CfgMgr::instance().getCurrentCfg()->getDeclinePeriod()); + + LeaseMgrFactory::instance().updateLease4(lease); + // Bump up the statistics. // Per subnet declined addresses counter. @@ -3062,15 +3071,6 @@ Dhcpv4Srv::declineLease(const Lease4Ptr& lease, const Pkt4Ptr& decline, // immediately after receiving DHCPDECLINE, rather than later when we recover // the address. - // @todo: Call hooks. - - // We need to disassociate the lease from the client. Once we move a lease - // to declined state, it is no longer associated with the client in any - // way. - lease->decline(CfgMgr::instance().getCurrentCfg()->getDeclinePeriod()); - - LeaseMgrFactory::instance().updateLease4(lease); - context.reset(new AllocEngine::ClientContext4()); context->new_lease_ = lease; diff --git a/src/bin/dhcp6/dhcp6_srv.cc b/src/bin/dhcp6/dhcp6_srv.cc index 1b2fa97cb0..9f6f9e192c 100644 --- a/src/bin/dhcp6/dhcp6_srv.cc +++ b/src/bin/dhcp6/dhcp6_srv.cc @@ -3400,6 +3400,15 @@ Dhcpv6Srv::declineLease(const Pkt6Ptr& decline, const Lease6Ptr lease, // the entries. This method does all necessary checks. queueNCR(CHG_REMOVE, lease); + // @todo: Call hooks. + + // We need to disassociate the lease from the client. Once we move a lease + // to declined state, it is no longer associated with the client in any + // way. + lease->decline(CfgMgr::instance().getCurrentCfg()->getDeclinePeriod()); + + LeaseMgrFactory::instance().updateLease6(lease); + // Bump up the subnet-specific statistic. StatsMgr::instance().addValue( StatsMgr::generateName("subnet", lease->subnet_id_, "declined-addresses"), @@ -3408,12 +3417,6 @@ Dhcpv6Srv::declineLease(const Pkt6Ptr& decline, const Lease6Ptr lease, // Global declined addresses counter. StatsMgr::instance().addValue("declined-addresses", static_cast(1)); - // We need to disassociate the lease from the client. Once we move a lease - // to declined state, it is no longer associated with the client in any - // way. - lease->decline(CfgMgr::instance().getCurrentCfg()->getDeclinePeriod()); - LeaseMgrFactory::instance().updateLease6(lease); - LOG_INFO(lease6_logger, DHCP6_DECLINE_LEASE).arg(decline->getLabel()) .arg(lease->addr_.toText()).arg(lease->valid_lft_); diff --git a/src/lib/dhcpsrv/alloc_engine.cc b/src/lib/dhcpsrv/alloc_engine.cc index fd9ab7f1be..fe4a6c6d71 100644 --- a/src/lib/dhcpsrv/alloc_engine.cc +++ b/src/lib/dhcpsrv/alloc_engine.cc @@ -291,7 +291,6 @@ AllocEngine::HashedAllocator::HashedAllocator(Lease::Type lease_type) isc_throw(NotImplemented, "Hashed allocator is not implemented"); } - isc::asiolink::IOAddress AllocEngine::HashedAllocator::pickAddress(const SubnetPtr&, const ClientClasses&, @@ -305,7 +304,6 @@ AllocEngine::RandomAllocator::RandomAllocator(Lease::Type lease_type) isc_throw(NotImplemented, "Random allocator is not implemented"); } - isc::asiolink::IOAddress AllocEngine::RandomAllocator::pickAddress(const SubnetPtr&, const ClientClasses&, @@ -314,7 +312,6 @@ AllocEngine::RandomAllocator::pickAddress(const SubnetPtr&, isc_throw(NotImplemented, "Random allocator is not implemented"); } - AllocEngine::AllocEngine(AllocType engine_type, uint64_t attempts, bool ipv6) : attempts_(attempts), incomplete_v4_reclamations_(0), @@ -420,7 +417,6 @@ inAllowedPool(AllocEngine::ClientContext6& ctx, const Lease::Type& lease_type, } - // ########################################################################## // # DHCPv6 lease allocation code starts here. // ########################################################################## @@ -665,7 +661,6 @@ AllocEngine::findGlobalReservation(ClientContext6& ctx) { return (host); } - Lease6Collection AllocEngine::allocateLeases6(ClientContext6& ctx) { @@ -835,7 +830,6 @@ AllocEngine::allocateLeases6(ClientContext6& ctx) { return (leases); } - } catch (const isc::Exception& e) { // Some other error, return an empty lease. @@ -1265,7 +1259,6 @@ AllocEngine::allocateReservedLeases6(ClientContext6& ctx, // ... and add it to the existing leases list. existing_leases.push_back(lease); - if (ctx.currentIA().type_ == Lease::TYPE_NA) { LOG_INFO(alloc_engine_logger, ALLOC_ENGINE_V6_HR_ADDR_GRANTED) .arg(addr.toText()) @@ -1889,7 +1882,6 @@ AllocEngine::renewLeases6(ClientContext6& ctx) { subnet = subnet->getNextSubnet(ctx.subnet_); } - if (!leases.empty()) { LOG_DEBUG(alloc_engine_logger, ALLOC_ENGINE_DBG_TRACE, ALLOC_ENGINE_V6_RENEW_REMOVE_RESERVED) @@ -2090,19 +2082,17 @@ AllocEngine::extendLease6(ClientContext6& ctx, Lease6Ptr lease) { } if (!skip) { + bool update_stats = false; + // If the lease we're renewing has expired, we need to reclaim this // lease before we can renew it. if (old_data->expired()) { reclaimExpiredLease(old_data, ctx.callout_handle_); // If the lease is in the current subnet we need to account - // for the re-assignment of The lease. + // for the re-assignment of the lease. if (ctx.subnet_->inPool(ctx.currentIA().type_, old_data->addr_)) { - StatsMgr::instance().addValue( - StatsMgr::generateName("subnet", ctx.subnet_->getID(), - ctx.currentIA().type_ == Lease::TYPE_NA ? - "assigned-nas" : "assigned-pds"), - static_cast(1)); + update_stats = true; } } @@ -2110,6 +2100,14 @@ AllocEngine::extendLease6(ClientContext6& ctx, Lease6Ptr lease) { // in the lease database. LeaseMgrFactory::instance().updateLease6(lease); + if (update_stats) { + StatsMgr::instance().addValue( + StatsMgr::generateName("subnet", ctx.subnet_->getID(), + ctx.currentIA().type_ == Lease::TYPE_NA ? + "assigned-nas" : "assigned-pds"), + static_cast(1)); + } + } else { // Copy back the original date to the lease. For MySQL it doesn't make // much sense, but for memfile, the Lease6Ptr points to the actual lease @@ -2123,7 +2121,6 @@ AllocEngine::extendLease6(ClientContext6& ctx, Lease6Ptr lease) { ctx.currentIA().changed_leases_.push_back(old_data); } - Lease6Collection AllocEngine::updateLeaseData(ClientContext6& ctx, const Lease6Collection& leases) { Lease6Collection updated_leases; @@ -2134,20 +2131,17 @@ AllocEngine::updateLeaseData(ClientContext6& ctx, const Lease6Collection& leases lease->fqdn_rev_ = ctx.rev_dns_update_; lease->hostname_ = ctx.hostname_; if (!ctx.fake_allocation_) { + bool update_stats = false; if (lease->state_ == Lease::STATE_EXPIRED_RECLAIMED) { // Transition lease state to default (aka assigned) lease->state_ = Lease::STATE_DEFAULT; // If the lease is in the current subnet we need to account - // for the re-assignment of The lease. + // for the re-assignment of the lease. if (inAllowedPool(ctx, ctx.currentIA().type_, lease->addr_, true)) { - StatsMgr::instance().addValue( - StatsMgr::generateName("subnet", lease->subnet_id_, - ctx.currentIA().type_ == Lease::TYPE_NA ? - "assigned-nas" : "assigned-pds"), - static_cast(1)); + update_stats = true; } } @@ -2158,6 +2152,14 @@ AllocEngine::updateLeaseData(ClientContext6& ctx, const Lease6Collection& leases ctx.currentIA().changed_leases_.push_back(*lease_it); LeaseMgrFactory::instance().updateLease6(lease); } + + if (update_stats) { + StatsMgr::instance().addValue( + StatsMgr::generateName("subnet", lease->subnet_id_, + ctx.currentIA().type_ == Lease::TYPE_NA ? + "assigned-nas" : "assigned-pds"), + static_cast(1)); + } } updated_leases.push_back(lease); @@ -2304,7 +2306,6 @@ AllocEngine::deleteExpiredReclaimedLeases6(const uint32_t secs) { .arg(deleted_leases); } - void AllocEngine::reclaimExpiredLeases4(const size_t max_leases, const uint16_t timeout, const bool remove_lease, @@ -2347,7 +2348,6 @@ AllocEngine::reclaimExpiredLeases4(const size_t max_leases, const uint16_t timeo lease_mgr.getExpiredLeases4(leases, max_leases); } - // Do not initialize the callout handle until we know if there are any // lease4_expire callouts installed. CalloutHandlePtr callout_handle; @@ -2770,7 +2770,6 @@ AllocEngine::reclaimDeclined(const Lease6Ptr& lease) { return (true); } - template void AllocEngine::reclaimLeaseInDatabase(const LeasePtrType& lease, const bool remove_lease, @@ -2802,7 +2801,6 @@ void AllocEngine::reclaimLeaseInDatabase(const LeasePtrType& lease, .arg(lease->addr_.toText()); } - } // namespace dhcp } // namespace isc @@ -3242,7 +3240,6 @@ AllocEngine::findGlobalReservation(ClientContext4& ctx) { return (host); } - Lease4Ptr AllocEngine::discoverLease4(AllocEngine::ClientContext4& ctx) { // Find an existing lease for this client. This function will return true -- GitLab