[5437] Changes after review

 - some comments added
 - unit-tests renamed
 - using C++11 syntax to simplify things.

diff --git a/src/bin/dhcp4/dhcp4_srv.cc b/src/bin/dhcp4/dhcp4_srv.cc
index d6f003603e2db324ed2799a11315fa10f8893a61..543649c9e646a8711ec4c08db00e0cd5b9807f91 100644 (file)
--- a/src/bin/dhcp4/dhcp4_srv.cc
+++ b/src/bin/dhcp4/dhcp4_srv.cc
@@ -1800,11 +1800,23 @@ Dhcpv4Srv::assignLease(Dhcpv4Exchange& ex) {
         Lease4Ptr lease;
         Subnet4Ptr original_subnet = subnet;
 
+        // We used to issue a separate query (two actually: one for client-id
+        // and another one for hw-addr for) each subnet in the shared network.
+        // That was horribly inefficient if the client didn't have any lease
+        // (or there were many subnets and the client happended to be in one
+        // of the last subnets).
+        //
+        // We now issue at most two queries: get all the leases for specific
+        // client-id and then get all leases for specific hw-address.
         if (client_id) {
+
+            // Get all the leases for this client-id
             Lease4Collection leases_client_id = LeaseMgrFactory::instance().getLease4(*client_id);
             if (!leases_client_id.empty()) {
                 Subnet4Ptr s = original_subnet;
 
+                // Among those returned try to find a lease that belongs to
+                // current shared network.
                 while (s) {
                     for (auto l = leases_client_id.begin(); l != leases_client_id.end(); ++l) {
                         if ((*l)->subnet_id_ == s->getID()) {
@@ -1823,11 +1835,16 @@ Dhcpv4Srv::assignLease(Dhcpv4Exchange& ex) {
             }
         }
 
+        // If we haven't found a lease yet, try again by hardware-address.
+        // The logic is the same.
         if (!lease && hwaddr) {
+
+            // Get all leases for this particular hw-address.
             Lease4Collection leases_hwaddr = LeaseMgrFactory::instance().getLease4(*hwaddr);
             if (!leases_hwaddr.empty()) {
                 Subnet4Ptr s = original_subnet;
 
+                // Pick one that belongs to a subnet in this shared network.
                 while (s) {
                     for (auto l = leases_hwaddr.begin(); l != leases_hwaddr.end(); ++l) {
                         if ((*l)->subnet_id_ == s->getID()) {

diff --git a/src/bin/dhcp4/tests/dora_unittest.cc b/src/bin/dhcp4/tests/dora_unittest.cc
index 1ecd72944cb339256a44820beefff1f8f3acdf93..3df230d2149cf9ba82c4452df08ea740ca06afcd 100644 (file)
--- a/src/bin/dhcp4/tests/dora_unittest.cc
+++ b/src/bin/dhcp4/tests/dora_unittest.cc
@@ -1569,7 +1569,7 @@ TEST_F(DORATest, reservationModeDisabled) {
 // cases in the real deployments, but this is just a test that the allocation
 // engine skips checking if the reservation exists when it allocates an
 // address. In the real deployment the reservation simply wouldn't exist.
-TEST_F(DORATest, reservationModeDisabledAddressHijacking) {
+TEST_F(DORATest, reservationIgnoredInDisabledMode) {
     // Client has a reservation.
     Dhcp4Client client(Dhcp4Client::SELECTING);
     // Set MAC address which doesn't match the reservation configured.
@@ -1633,7 +1633,7 @@ TEST_F(DORATest, reservationModeOutOfPool) {
 // This test verifies that the in-pool reservation can be assigned to
 // the client not owning this reservation when the reservation mode is
 // set to "out-of-pool".
-TEST_F(DORATest, reservationModeOutOfPoolAddressHijacking) {
+TEST_F(DORATest, reservationIgnoredInOutOfPoolMode) {
     // Create the first client for which we have a reservation out of the
     // dynamic pool.
     Dhcp4Client client(Dhcp4Client::SELECTING);

diff --git a/src/lib/dhcpsrv/alloc_engine.cc b/src/lib/dhcpsrv/alloc_engine.cc
index a66a8c16f6bd1e4973aed74aa3dc2f8b3d7c3eec..f25057f2bd00567e0cb4f78e7d61e655f28dda38 100644 (file)
--- a/src/lib/dhcpsrv/alloc_engine.cc
+++ b/src/lib/dhcpsrv/alloc_engine.cc
@@ -515,12 +515,10 @@ void AllocEngine::findReservation(ClientContext6& ctx) {
         (network->getAllSubnets()->size() > ctx.host_identifiers_.size());
 
     if (use_single_query) {
-        for (auto id_pair = ctx.host_identifiers_.begin();
-             id_pair != ctx.host_identifiers_.end();
-             ++id_pair) {
-            ConstHostCollection hosts = HostMgr::instance().getAll(id_pair->first,
-                                                                   &id_pair->second[0],
-                                                                   id_pair->second.size());
+        for (auto id_pair : ctx.host_identifiers_) {
+            ConstHostCollection hosts = HostMgr::instance().getAll(id_pair.first,
+                                                                   &id_pair.second[0],
+                                                                   id_pair.second.size());
             // Store the hosts in the temporary map, because some hosts may
             // belong to subnets outside of the shared network. We'll need
             // to eliminate them.
@@ -541,7 +539,7 @@ void AllocEngine::findReservation(ClientContext6& ctx) {
             (subnet->getHostReservationMode() != Network::HR_DISABLED)) {
             // Iterate over configured identifiers in the order of preference
             // and try to use each of them to search for the reservations.
-            BOOST_FOREACH(const IdentifierPair& id_pair, ctx.host_identifiers_) {
+            for (auto id_pair : ctx.host_identifiers_) {
                 if (use_single_query) {
                     if (host_map.count(subnet->getID()) > 0) {
                         ctx.hosts_[subnet->getID()] = host_map[subnet->getID()];
@@ -593,9 +591,9 @@ AllocEngine::allocateLeases6(ClientContext6& ctx) {
         // our shared network.
         Lease6Collection leases;
         while (subnet) {
-            for (auto l = all_leases.begin(); l != all_leases.end(); ++l) {
-                if ((*l)->subnet_id_ == subnet->getID()) {
-                    leases.push_back(*l);
+            for (auto l : all_leases) {
+                if ((l)->subnet_id_ == subnet->getID()) {
+                    leases.push_back(l);
                 }
             }
 
@@ -3506,7 +3504,7 @@ AllocEngine::allocateUnreservedLease4(ClientContext4& ctx) {
     // the entire subnet (or more subnets) to discover that the address
     // pools have been exhausted. Using a subnet from which an address
     // was assigned most recently is an optimization which increases
-    // the likelyhood of starting from the subnet which address pools
+    // the likelihood of starting from the subnet which address pools
     // are not exhausted.
     SharedNetwork4Ptr network;
     ctx.subnet_->getSharedNetwork(network);