[master] Merge branch 'trac3692'

src/lib/dhcpsrv/alloc_engine.cc

@@ -1,4 +1,4 @@
-// Copyright (C) 2012-2014 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2012-2015 Internet Systems Consortium, Inc. ("ISC")
 //
 // Permission to use, copy, modify, and/or distribute this software for any
 // purpose with or without fee is hereby granted, provided that the above
@@ -21,6 +21,7 @@
 #include <hooks/hooks_manager.h>
 
 #include <cstring>
+#include <limits>
 #include <vector>
 #include <string.h>
 
@@ -405,8 +406,13 @@ AllocEngine::allocateLeases6(const Subnet6Ptr& subnet, const DuidPtr& duid,
         // left), but this has one major problem. We exactly control allocation
         // moment, but we currently do not control expiration time at all
 
-        unsigned int i = attempts_;
-        do {
+        // Initialize the maximum number of attempts to pick and allocate an
+        // address. The value of 0 means "infinite", which is maximum uint32_t
+        // value.
+        uint32_t max_attempts = (attempts_ == 0) ?
+            std::numeric_limits<uint32_t>::max() : attempts_;
+
+        for (uint32_t i = 0; i < max_attempts; ++i) {
             IOAddress candidate = allocator->pickAddress(subnet, duid, hint);
 
             /// @todo: check if the address is reserved once we have host support
@@ -461,11 +467,7 @@ AllocEngine::allocateLeases6(const Subnet6Ptr& subnet, const DuidPtr& duid,
                     return (collection);
                 }
             }
-
-            // Continue trying allocation until we run out of attempts
-            // (or attempts are set to 0, which means infinite)
-            --i;
-        } while ((i > 0) || !attempts_);
+        }
 
         // Unable to allocate an address, return an empty lease.
         LOG_WARN(dhcpsrv_logger, DHCPSRV_ADDRESS6_ALLOC_FAIL).arg(attempts_);
@@ -674,6 +676,9 @@ AllocEngine::allocateLease4(const SubnetPtr& subnet, const ClientIdPtr& clientid
         /// moment, but we currently do not control expiration time at all
         unsigned int i = attempts_;
         do {
+            // Decrease the number of remaining attempts here so as we guarantee
+            // that it is decreased when the code below uses "continue".
+            --i;
             IOAddress candidate = allocator->pickAddress(subnet, clientid,
                                                          ctx.requested_address_);
 
@@ -710,7 +715,6 @@ AllocEngine::allocateLease4(const SubnetPtr& subnet, const ClientIdPtr& clientid
 
             // Continue trying allocation until we run out of attempts
             // (or attempts are set to 0, which means infinite)
-            --i;
         } while ((i > 0) || !attempts_);
 
         // Unable to allocate an address, return an empty lease.

src/lib/dhcpsrv/tests/alloc_engine_unittest.cc

@@ -1,4 +1,4 @@
-// Copyright (C) 2012-2014 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2012-2015 Internet Systems Consortium, Inc. ("ISC")
 //
 // Permission to use, copy, modify, and/or distribute this software for any
 // purpose with or without fee is hereby granted, provided that the above
@@ -339,11 +339,11 @@ public:
     /// @brief checks if bogus hint can be ignored and the allocation succeeds
     ///
     /// This test checks if the allocation with a hing that is out of the blue
-    /// can succeed. The invalid hint should be ingored completely.
+    /// can succeed. The invalid hint should be ignored completely.
     ///
     /// @param type Lease type
     /// @param hint hint (as send by a client)
-    /// @param expectd_pd_len (used in validation)
+    /// @param expected_pd_len (used in validation)
     void allocBogusHint6(Lease::Type type, IOAddress hint,
                          uint8_t expected_pd_len) {
         boost::scoped_ptr<AllocEngine> engine;
@@ -433,11 +433,7 @@ public:
         // instantiate cfg_mgr
         CfgMgr& cfg_mgr = CfgMgr::instance();
 
-        subnet_ = Subnet4Ptr(new Subnet4(IOAddress("192.0.2.0"), 24, 1, 2, 3));
-        pool_ = Pool4Ptr(new Pool4(IOAddress("192.0.2.100"),
-                                   IOAddress("192.0.2.109")));
-        subnet_->addPool(pool_);
-        cfg_mgr.getStagingCfg()->getCfgSubnets4()->add(subnet_);
+        initSubnet(IOAddress("192.0.2.100"), IOAddress("192.0.2.109"));
         cfg_mgr.commit();
 
         factory_.create("type=memfile universe=4 persist=false");
@@ -476,6 +472,20 @@ public:
         /// @todo: check cltt
      }
 
+    /// @brief Create a subnet with a specified pool of addresses.
+    ///
+    /// @param pool_start First address in the pool.
+    /// @param pool_end Last address in the pool.
+    void initSubnet(const IOAddress& pool_start, const IOAddress& pool_end) {
+        CfgMgr& cfg_mgr = CfgMgr::instance();
+
+        subnet_ = Subnet4Ptr(new Subnet4(IOAddress("192.0.2.0"), 24, 1, 2, 3));
+        pool_ = Pool4Ptr(new Pool4(pool_start, pool_end));
+        subnet_->addPool(pool_);
+
+        cfg_mgr.getStagingCfg()->getCfgSubnets4()->add(subnet_);
+    }
+
     virtual ~AllocEngine4Test() {
         factory_.destroy();
     }
@@ -687,7 +697,7 @@ TEST_F(AllocEngine6Test, IterativeAllocatorPrefixStep) {
     // Second pool (just one lease here)
     EXPECT_EQ("2001:db8:1::", alloc.pickAddress(subnet_, duid_, IOAddress("::")).toText());
 
-    // Third pool (256 leases, let's check first and last explictly and the
+    // Third pool (256 leases, let's check first and last explicitly and the
     // rest over in a pool
     EXPECT_EQ("2001:db8:2::", alloc.pickAddress(subnet_, duid_, IOAddress("::")).toText());
     for (int i = 1; i < 255; i++) {
@@ -2263,6 +2273,44 @@ TEST_F(AllocEngine4Test, reservedAddressVsDynamicPool) {
     EXPECT_NE(allocated_lease->addr_.toText(), "192.0.2.100");
 }
 
+// This test checks that the allocation engine refuses to allocate an
+// address when the pool is exhausted, and the only one available
+// address is reserved for a different client.
+TEST_F(AllocEngine4Test, reservedAddressShortPool) {
+    AllocEngine engine(AllocEngine::ALLOC_ITERATIVE, 100, false);
+
+    // Create short pool with only one address.
+    initSubnet(IOAddress("192.0.2.100"), IOAddress("192.0.2.100"));
+    // Reserve the address for a different client.
+    HostPtr host(new Host(&hwaddr2_->hwaddr_[0], hwaddr_->hwaddr_.size(),
+                          Host::IDENT_HWADDR, subnet_->getID(),
+                          SubnetID(0), IOAddress("192.0.2.100")));
+    CfgMgr::instance().getStagingCfg()->getCfgHosts()->add(host);
+    CfgMgr::instance().commit();
+
+    // Allocation engine should determine that the available address is
+    // reserved for someone else and not allocate it.
+    Lease4Ptr allocated_lease = engine.allocateLease4(subnet_, ClientIdPtr(),
+                                                      hwaddr_,
+                                                      IOAddress("0.0.0.0"),
+                                                      false, false, "", false,
+                                                      CalloutHandlePtr(),
+                                                      old_lease_);
+    EXPECT_FALSE(allocated_lease);
+
+    // Now, let's remove the reservation.
+    initSubnet(IOAddress("192.0.2.100"), IOAddress("192.0.2.100"));
+    CfgMgr::instance().commit();
+
+    // Address should be successfully allocated.
+    allocated_lease = engine.allocateLease4(subnet_, ClientIdPtr(), hwaddr_,
+                                            IOAddress("0.0.0.0"), false, false,
+                                            "", false, CalloutHandlePtr(),
+                                            old_lease_);
+    ASSERT_TRUE(allocated_lease);
+    EXPECT_EQ("192.0.2.100", allocated_lease->addr_.toText());
+}
+
 /// @brief helper class used in Hooks testing in AllocEngine6
 ///
 /// It features a couple of callout functions and buffers to store