From: Marcin Siodelski <marcin@isc.org>
Date: Wed, 27 Apr 2016 13:34:33 +0000 (+0200)
Subject: [4303] Address second round of review comments.
X-Git-Tag: trac4106_update_base~36^2~1
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=aec8c6360491ebd85acec8bbe7a7b0e628f3d5a3;p=thirdparty%2Fkea.git

[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 0cc74195f8..49352b2567 100644
--- 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 0fa5a8ea04..704d63aac8 100644
--- 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 fdf298bce8..489d328bb7 100644
--- 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 c5068500b2..af61867eed 100644
--- 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);