[#1040] handle update and delete

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;
 

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

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(
+                update_stats = true;
-                    StatsMgr::generateName("subnet", ctx.subnet_->getID(),
-                                       ctx.currentIA().type_ == Lease::TYPE_NA ?
-                                       "assigned-nas" : "assigned-pds"),
-                    static_cast<int64_t>(1));
             }
         }
 
@@ -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<int64_t>(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(
+                    update_stats = true;
-                        StatsMgr::generateName("subnet", lease->subnet_id_,
-                                               ctx.currentIA().type_ == Lease::TYPE_NA ?
-                                               "assigned-nas" : "assigned-pds"),
-                        static_cast<int64_t>(1));
                 }
             }
 
@@ -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<int64_t>(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<typename LeasePtrType>
 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

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<Lease6StorageDuidIaidTypeIndex::const_iterator,
               Lease6StorageDuidIaidTypeIndex::const_iterator> 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<Lease6StorageDuidIaidTypeIndex::const_iterator,
               Lease6StorageDuidIaidTypeIndex::const_iterator> 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)));

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<uint64_t>(mysql_stmt_affected_rows(ctx->conn_.statements_[stindex])));
+    int affected_rows = mysql_stmt_affected_rows(ctx->conn_.statements_[stindex]);
+
+    return (static_cast<uint64_t>(affected_rows));
 }
 
 bool
@@ -2862,7 +2864,22 @@ MySqlLeaseMgr::deleteLease(const Lease4Ptr& lease) {
     inbind[1].buffer = reinterpret_cast<char*>(&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<char*>(&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

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

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.