[4303] Address second round of review comments.

- No additional check for HR mode in findReservations
- Additional commentary in the loop collecting host ids.

diff --git a/src/bin/dhcp4/dhcp4_srv.cc b/src/bin/dhcp4/dhcp4_srv.cc
index 0cc74195f8459b58bfb4be003c2a3f5c30ccaabb..49352b256741e63c5fa7ec35b97ac6d8970762fc 100644 (file)
--- a/src/bin/dhcp4/dhcp4_srv.cc
+++ b/src/bin/dhcp4/dhcp4_srv.cc
@@ -245,6 +245,8 @@ void
 Dhcpv4Exchange::setHostIdentifiers() {
     const ConstCfgHostOperationsPtr cfg =
         CfgMgr::instance().getCurrentCfg()->getCfgHostOperations4();
+    // Collect host identifiers. The identifiers are stored in order of preference.
+    // The server will use them in that order to search for host reservations.
     BOOST_FOREACH(const Host::IdentifierType& id_type,
                   cfg->getIdentifierTypes()) {
         switch (id_type) {

diff --git a/src/bin/dhcp6/dhcp6_srv.cc b/src/bin/dhcp6/dhcp6_srv.cc
index 0fa5a8ea04ae3f671e8077032dc76af417834536..704d63aac86802d9f2675b3429e86c094c5fe287 100644 (file)
--- a/src/bin/dhcp6/dhcp6_srv.cc
+++ b/src/bin/dhcp6/dhcp6_srv.cc
@@ -281,7 +281,9 @@ Dhcpv6Srv::createContext(const Pkt6Ptr& pkt) {
     ctx.duid_ = pkt->getClientId();
     ctx.hwaddr_ = getMAC(pkt);
 
-    // Collect host identifiers if host reservations enabled.
+    // Collect host identifiers if host reservations enabled. The identifiers
+    // are stored in order of preference. The server will use them in that
+    // order to search for host reservations.
     if (ctx.subnet_ &&
         (ctx.subnet_->getHostReservationMode() != Subnet::HR_DISABLED)) {
         const ConstCfgHostOperationsPtr cfg =

diff --git a/src/lib/dhcpsrv/alloc_engine.cc b/src/lib/dhcpsrv/alloc_engine.cc
index fdf298bce827f0f5315838ddcc0b0028fb476ee8..489d328bb7e8ee0acd9de7d27b273d8491ec822d 100644 (file)
--- a/src/lib/dhcpsrv/alloc_engine.cc
+++ b/src/lib/dhcpsrv/alloc_engine.cc
@@ -314,19 +314,15 @@ AllocEngine::findReservationInternal(ContextType& ctx,
         // Check which host reservation mode is supported in this subnet.
         Subnet::HRMode hr_mode = ctx.subnet_->getHostReservationMode();
 
-        // Check if there is a host reseravtion for this client. Attempt to
-        // get host information
-        if (hr_mode != Subnet::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_) {
-                // Attempt to find a host using a specified identifier.
-                ctx.host_ = host_get(ctx.subnet_->getID(), id_pair.first,
-                                     &id_pair.second[0], id_pair.second.size());
-                // If we found matching host, return.
-                if (ctx.host_) {
-                    return;
-                }
+        // 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_) {
+            // Attempt to find a host using a specified identifier.
+            ctx.host_ = host_get(ctx.subnet_->getID(), id_pair.first,
+                                 &id_pair.second[0], id_pair.second.size());
+            // If we found matching host, return.
+            if (ctx.host_) {
+                return;
             }
         }
     }

diff --git a/src/lib/dhcpsrv/tests/alloc_engine4_unittest.cc b/src/lib/dhcpsrv/tests/alloc_engine4_unittest.cc
index c5068500b296fa36f9ba16ca2c37961336730123..af61867eedff572f006a37a74000f00e2c6a3de2 100644 (file)
--- a/src/lib/dhcpsrv/tests/alloc_engine4_unittest.cc
+++ b/src/lib/dhcpsrv/tests/alloc_engine4_unittest.cc
@@ -1669,12 +1669,12 @@ TEST_F(AllocEngine4Test, findReservation) {
     EXPECT_TRUE(ctx.host_);
     EXPECT_EQ(ctx.host_->getIPv4Reservation(), host->getIPv4Reservation());
 
-    // If the host reservation mode for the subnet is disabled, the
-    // host should not be returned, even though it exists in the
-    // host database.
+    // Regardless of the host reservation mode, the host should be
+    // always returned when findReservation() is called.
     subnet_->setHostReservationMode(Subnet::HR_DISABLED);
     ASSERT_NO_THROW(engine.findReservation(ctx));
-    EXPECT_FALSE(ctx.host_);
+    EXPECT_TRUE(ctx.host_);
+    EXPECT_EQ(ctx.host_->getIPv4Reservation(), host->getIPv4Reservation());
 
     // Check the third possible reservation mode.
     subnet_->setHostReservationMode(Subnet::HR_OUT_OF_POOL);