diff --git a/src/bin/dhcp4/dhcp4_srv.cc b/src/bin/dhcp4/dhcp4_srv.cc index 37a1028653..01773340e8 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 diff --git a/src/lib/dhcpsrv/memfile_lease_mgr.cc b/src/lib/dhcpsrv/memfile_lease_mgr.cc index eec7b2d758..278fb671b9 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))); diff --git a/src/lib/dhcpsrv/mysql_lease_mgr.cc b/src/lib/dhcpsrv/mysql_lease_mgr.cc index 465201ca30..2b1a8227d6 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 (static_cast(affected_rows)); } bool @@ -2862,7 +2864,22 @@ 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) { + return (false); + } + + // Should not happen - primary key constraint should only have selected + // one row. + isc_throw(DbOperationError, "apparently deleted more than one lease " + "that had the address " << lease->addr_.toText()); } bool @@ -2893,7 +2910,22 @@ 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) { + return (false); + } + + // Should not happen - primary key constraint should only have selected + // one row. + isc_throw(DbOperationError, "apparently deleted 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..127db4d73e 100644 --- a/src/lib/dhcpsrv/pgsql_lease_mgr.cc +++ b/src/lib/dhcpsrv/pgsql_lease_mgr.cc @@ -2037,7 +2037,22 @@ 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) { + return (false); + } + + // Should not happen - primary key constraint should only have selected + // one row. + isc_throw(DbOperationError, "apparently deleted more than one lease " + "that had the address " << lease->addr_.toText()); } bool @@ -2057,7 +2072,22 @@ 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) { + return (false); + } + + // Should not happen - primary key constraint should only have selected + // one row. + isc_throw(DbOperationError, "apparently deleted more than one lease " + "that had the address " << lease->addr_.toText()); } uint64_t diff --git a/src/lib/pgsql/pgsql_connection.cc b/src/lib/pgsql/pgsql_connection.cc index 7391ae098e..9901096520 100644 --- a/src/lib/pgsql/pgsql_connection.cc +++ b/src/lib/pgsql/pgsql_connection.cc @@ -344,7 +344,7 @@ PgSqlConnection::checkStatementError(const PgSqlResult& r, // We still need to throw so caller can error out of the current // processing. isc_throw(DbOperationError, - "fatal database errror or connectivity lost"); + "fatal database error or connectivity lost"); } // Apparently it wasn't fatal, so we throw with a helpful message.