[3689] Removed call to findReservation from AllocateEngine::allocateLeases6()

src/lib/dhcpsrv/alloc_engine.h
src/lib/dhcpsrv/alloc_engine.cc
    allocateLeases6()
        - removed find_reservation parameter from allocateLeases6
        - removed logic that called findReservation(), this eliminates
        inadvertanly stomping an already populated host in the context.
        Places burden of doing the reservation lookup always on the caller.

src/bin/dhcp6/dhcp6_srv.cc
    removed obsolete boolean parameter from allocateLeases6 calls

src/lib/dhcpsrv/tests/alloc_engine_utils.h
src/lib/dhcpsrv/tests/alloc_engine_utils.cc
   AllocEngine6Test::findReservation() - new method which calls
   engine's findReservation() and fills in context hostname accordingly.

src/lib/dhcpsrv/tests/alloc_engine6_unittest.cc
    added calls to AlloceEnginetTest::findReservation where needed

diff --git a/src/bin/dhcp6/dhcp6_srv.cc b/src/bin/dhcp6/dhcp6_srv.cc
index c0dc4a78e3e95e264c248e73fbe5b0093cbc1a88..7c757b78d7cfd094bf853d58babed9c7764b0496 100644 (file)
--- a/src/bin/dhcp6/dhcp6_srv.cc
+++ b/src/bin/dhcp6/dhcp6_srv.cc
@@ -1327,7 +1327,7 @@ Dhcpv6Srv::assignIA_NA(const Pkt6Ptr& query, const Pkt6Ptr& answer,
     ctx.hwaddr_ = orig_ctx.hwaddr_;
     ctx.host_ = orig_ctx.host_;
 
-    Lease6Collection leases = alloc_engine_->allocateLeases6(ctx, false);
+    Lease6Collection leases = alloc_engine_->allocateLeases6(ctx);
 
     /// @todo: Handle more than one lease
     Lease6Ptr lease;
@@ -1472,7 +1472,7 @@ Dhcpv6Srv::assignIA_PD(const Pkt6Ptr& query,
     ctx.hwaddr_ = orig_ctx.hwaddr_;
     ctx.host_ = orig_ctx.host_;
 
-    Lease6Collection leases = alloc_engine_->allocateLeases6(ctx, false);
+    Lease6Collection leases = alloc_engine_->allocateLeases6(ctx);
 
     if (!leases.empty()) {
 

diff --git a/src/lib/dhcpsrv/alloc_engine.cc b/src/lib/dhcpsrv/alloc_engine.cc
index 1d7ff41e61d31783c7a1af561c29908483761c5e..dc5f7a70f1f28fdbdb91496c33649b7d2ab32ee3 100644 (file)
--- a/src/lib/dhcpsrv/alloc_engine.cc
+++ b/src/lib/dhcpsrv/alloc_engine.cc
@@ -339,7 +339,7 @@ void AllocEngine::findReservation(ClientContext6& ctx) const {
 }
 
 Lease6Collection
-AllocEngine::allocateLeases6(ClientContext6& ctx, bool find_reservation) {
+AllocEngine::allocateLeases6(ClientContext6& ctx) {
 
     try {
         if (!ctx.subnet_) {
@@ -349,23 +349,6 @@ AllocEngine::allocateLeases6(ClientContext6& ctx, bool find_reservation) {
             isc_throw(InvalidOperation, "DUID is mandatory for IPv6 lease allocation");
         }
 
-        /// @todo This is ONLY used for unit tests and really should be taken out
-        /// Currently production code passes in find_reservation as false
-        if (find_reservation) {
-            std::cout << "***** TKM findReservation being called from allocateLeases6"
-                        << std::endl;
-            findReservation(ctx);
-
-            // Let's check whether there's a hostname specified in the reservation
-            if (ctx.host_) {
-                std::string hostname = ctx.host_->getHostname();
-                // If there is, let's use it
-                if (!hostname.empty()) {
-                    ctx.hostname_ = hostname;
-                }
-            }
-        }
-
         // Check if there are existing leases for that subnet/duid/iaid
         // combination.
         Lease6Collection leases =

diff --git a/src/lib/dhcpsrv/alloc_engine.h b/src/lib/dhcpsrv/alloc_engine.h
index 33363147b2115d1b218f41596cdb27b627c45cc0..bb6835b57525a6c432a167b1b06a894a80c41e56 100644 (file)
--- a/src/lib/dhcpsrv/alloc_engine.h
+++ b/src/lib/dhcpsrv/alloc_engine.h
@@ -445,12 +445,6 @@ public:
     ///
     /// @param ctx client context that passes all necessary information. See
     ///        @ref ClientContext6 for details.
-    /// @param find_resrv specifies whether the code should search for host
-    ///   reservation. true means that the code will consult HostMgr, false means
-    ///   to skip this check. That is easier to use, but is redundant if the
-    ///   ctx.host_ field is already set. We can't use ctx.host_ == NULL as
-    ///   check, because for cases whithout reservations, the reservation
-    ///   search would be repeated.
     ///
     /// The following fields of ClientContext6 are used:
     ///
@@ -484,7 +478,7 @@ public:
     ///
     /// @return Allocated IPv6 leases (may be empty if allocation failed)
     Lease6Collection
-    allocateLeases6(ClientContext6& ctx, bool find_resrv = true);
+    allocateLeases6(ClientContext6& ctx);
 
     /// @brief Renews existing DHCPv6 leases for a given IA.
     ///

diff --git a/src/lib/dhcpsrv/tests/alloc_engine6_unittest.cc b/src/lib/dhcpsrv/tests/alloc_engine6_unittest.cc
index 6206dad51e5dc75cc2fd1899f8aa01c776d60b5e..3363c3649bee76bb2f9cfac383986c0be3328447 100644 (file)
--- a/src/lib/dhcpsrv/tests/alloc_engine6_unittest.cc
+++ b/src/lib/dhcpsrv/tests/alloc_engine6_unittest.cc
@@ -968,6 +968,7 @@ TEST_F(AllocEngine6Test, reservedAddress) {
     for (int i = 0; i < 30; i++) {
         AllocEngine::ClientContext6 ctx(subnet_, clients[i], iaid_, IOAddress("::"),
                                         Lease::TYPE_NA,  false, false, "", false);
+        findReservation(engine, ctx);
         Lease6Collection leases = engine.allocateLeases6(ctx);
         if (leases.empty()) {
             failure++;
@@ -990,6 +991,7 @@ TEST_F(AllocEngine6Test, reservedAddress) {
     // address reserved, will get it (despite the pool being depleted).
     AllocEngine::ClientContext6 ctx(subnet_, duid_, iaid_, IOAddress("::"),
                                     Lease::TYPE_NA,  false, false, "", false);
+    findReservation(engine, ctx);
     Lease6Collection leases = engine.allocateLeases6(ctx);
     ASSERT_EQ(1, leases.size());
     EXPECT_EQ("2001:db8:1::12", leases[0]->addr_.toText());
@@ -1097,6 +1099,7 @@ TEST_F(AllocEngine6Test, DISABLED_reserved2AddressesSolicit) {
     AllocEngine::ClientContext6 ctx1(subnet_, duid_, iaid_, IOAddress("::"),
                                     pool_->getType(), false, false, "", true);
     Lease6Collection leases1;
+    findReservation(engine, ctx1);
     EXPECT_NO_THROW(leases1 = engine.allocateLeases6(ctx1));
     ASSERT_EQ(1, leases1.size());
     EXPECT_EQ("2001:db8:1::babe", leases1[0]->addr_.toText());
@@ -1106,6 +1109,7 @@ TEST_F(AllocEngine6Test, DISABLED_reserved2AddressesSolicit) {
     AllocEngine::ClientContext6 ctx2(subnet_, duid_, iaid_, IOAddress("::"),
                                     pool_->getType(), false, false, "", true);
     Lease6Collection leases2;
+    findReservation(engine, ctx2);
     EXPECT_NO_THROW(leases2 = engine.allocateLeases6(ctx2));
     EXPECT_EQ(1, leases2.size());
     EXPECT_EQ("2001:db8:1::babe", leases2[0]->addr_.toText());
@@ -1115,6 +1119,7 @@ TEST_F(AllocEngine6Test, DISABLED_reserved2AddressesSolicit) {
     AllocEngine::ClientContext6 ctx3(subnet_, duid_, iaid_ + 1, IOAddress("::"),
                                     pool_->getType(), false, false, "", true);
     Lease6Collection leases3;
+    findReservation(engine, ctx3);
     EXPECT_NO_THROW(leases3 = engine.allocateLeases6(ctx3));
     ASSERT_EQ(1, leases3.size());
     EXPECT_EQ("2001:db8:1::cafe", leases3[0]->addr_.toText());
@@ -1138,6 +1143,7 @@ TEST_F(AllocEngine6Test, reserved2Addresses) {
     AllocEngine::ClientContext6 ctx1(subnet_, duid_, iaid_, IOAddress("::"),
                                     pool_->getType(), false, false, "", false);
     Lease6Collection leases1;
+    findReservation(engine, ctx1);
     EXPECT_NO_THROW(leases1 = engine.allocateLeases6(ctx1));
     ASSERT_EQ(1, leases1.size());
     EXPECT_EQ("2001:db8:1::babe", leases1[0]->addr_.toText());
@@ -1147,6 +1153,7 @@ TEST_F(AllocEngine6Test, reserved2Addresses) {
     AllocEngine::ClientContext6 ctx2(subnet_, duid_, iaid_, IOAddress("::"),
                                     pool_->getType(), false, false, "", false);
     Lease6Collection leases2;
+    findReservation(engine, ctx2);
     EXPECT_NO_THROW(leases2 = engine.allocateLeases6(ctx2));
     EXPECT_EQ(1, leases2.size());
     EXPECT_EQ("2001:db8:1::babe", leases2[0]->addr_.toText());
@@ -1156,6 +1163,7 @@ TEST_F(AllocEngine6Test, reserved2Addresses) {
     AllocEngine::ClientContext6 ctx3(subnet_, duid_, iaid_ + 1, IOAddress("::"),
                                     pool_->getType(), false, false, "", false);
     Lease6Collection leases3;
+    findReservation(engine, ctx3);
     EXPECT_NO_THROW(leases3 = engine.allocateLeases6(ctx3));
     ASSERT_EQ(1, leases3.size());
     EXPECT_EQ("2001:db8:1::cafe", leases3[0]->addr_.toText());

diff --git a/src/lib/dhcpsrv/tests/alloc_engine_utils.cc b/src/lib/dhcpsrv/tests/alloc_engine_utils.cc
index 55a8315f78aec5e8f02391d191864b7cbdc845c0..38b07611a2ffb5f1a7dc2fd219dda70c6ab07e96 100644 (file)
--- a/src/lib/dhcpsrv/tests/alloc_engine_utils.cc
+++ b/src/lib/dhcpsrv/tests/alloc_engine_utils.cc
@@ -82,6 +82,20 @@ AllocEngine6Test::initSubnet(const asiolink::IOAddress& subnet,
     cfg_mgr.commit();
 }
 
+void
+AllocEngine6Test::findReservation(AllocEngine& engine,
+    AllocEngine::ClientContext6& ctx) {
+    engine.findReservation(ctx);
+    // Let's check whether there's a hostname specified in the reservation
+    if (ctx.host_) {
+        std::string hostname = ctx.host_->getHostname();
+        // If there is, let's use it
+        if (!hostname.empty()) {
+            ctx.hostname_ = hostname;
+        }
+    }
+}
+
 HostPtr
 AllocEngine6Test::createHost6HWAddr(bool add_to_host_mgr, IPv6Resrv::Type type,
                                     HWAddrPtr& hwaddr, const asiolink::IOAddress& addr,
@@ -110,6 +124,8 @@ AllocEngine6Test::allocateTest(AllocEngine& engine, const Pool6Ptr& pool,
                                     false, false, "", fake);
 
     Lease6Collection leases;
+
+    findReservation(engine, ctx);
     EXPECT_NO_THROW(leases = engine.allocateLeases6(ctx));
 
     for (Lease6Collection::iterator it = leases.begin(); it != leases.end(); ++it) {
@@ -165,7 +181,7 @@ AllocEngine6Test::simpleAlloc6Test(const Pool6Ptr& pool, const IOAddress& hint,
     AllocEngine::ClientContext6 ctx(subnet_, duid_, iaid_, hint, type,
                                     false, false, "", fake);
     ctx.hwaddr_ = hwaddr_;
-
+    findReservation(*engine, ctx);
     EXPECT_NO_THROW(lease = expectOneLease(engine->allocateLeases6(ctx)));
 
     // Check that we got a lease

diff --git a/src/lib/dhcpsrv/tests/alloc_engine_utils.h b/src/lib/dhcpsrv/tests/alloc_engine_utils.h
index 9e51ae1289c4478c20178061d61135ecb6051b85..0c2a6f82aad66bb4d9b9ba847049f0c7eeec4e9b 100644 (file)
--- a/src/lib/dhcpsrv/tests/alloc_engine_utils.h
+++ b/src/lib/dhcpsrv/tests/alloc_engine_utils.h
@@ -111,6 +111,15 @@ public:
         fqdn_rev_ = fqdn_rev;
     }
 
+    /// @brief Wrapper around call to AllocEngine6::findRervation
+    ///
+    /// If a reservation is found bg the engine, the function sets
+    /// ctx.hostname_ accordingly.
+    ///
+    /// @param engine allocation engine to use
+    /// @param ctx client context to pass into engine's findReservation method
+    void findReservation(AllocEngine& engine, AllocEngine::ClientContext6& ctx);
+
     /// @brief attempts to convert leases collection to a single lease
     ///
     /// This operation makes sense if there is at most one lease in the