[2327] Changes after review

- detailedLeaseCompare unified (alloc_engine and mysql_lease_mgr)
- comments clarified

src/lib/dhcpsrv/alloc_engine.cc

@@ -208,6 +208,22 @@ AllocEngine::allocateAddress6(const Subnet6Ptr& subnet,
         }
     }
 
+    // Hint is in the pool but is not available. Search the pool until first of
+    // the following occurs:
+    // - we find a free address
+    // - we find an address for which the lease has expired
+    // - we exhaust number of tries
+    //
+    // @todo: Current code does not handle pool exhaustion well. It will be
+    // improved. Current problems:
+    // 1. with attempts set to too large value (e.g. 1000) and a small pool (e.g.
+    // 10 addresses), we will iterate over it 100 times before giving up
+    // 2. attempts 0 mean unlimited (this is really UINT_MAX, not infinite)
+    // 3. the whole concept of infinite attempts is just asking for infinite loop
+    // We may consider some form or reference counting (this pool has X addresses
+    // 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 {
         IOAddress candidate = allocator_->pickAddress(subnet, duid, hint);
@@ -216,9 +232,9 @@ AllocEngine::allocateAddress6(const Subnet6Ptr& subnet,
         /// implemented
 
         Lease6Ptr existing = LeaseMgrFactory::instance().getLease6(candidate);
-        // there's no existing lease for selected candidate, so it is
-        // free. Let's allocate it.
         if (!existing) {
+            // there's no existing lease for selected candidate, so it is
+            // free. Let's allocate it.
             Lease6Ptr lease = createLease(subnet, duid, iaid, candidate,
                                           fake_allocation);
             if (lease) {
@@ -274,7 +290,7 @@ Lease6Ptr AllocEngine::reuseExpiredLease(Lease6Ptr& expired,
     if (!fake_allocation) {
         // for REQUEST we do update the lease
         LeaseMgrFactory::instance().updateLease6(expired);
-    } 
+    }
 
     // We do nothing for SOLICIT. We'll just update database when
     // the client gets back to us with REQUEST message.

src/lib/dhcpsrv/tests/Makefile.am

@@ -40,6 +40,7 @@ libdhcpsrv_unittests_SOURCES += pool_unittest.cc
 libdhcpsrv_unittests_SOURCES += schema_copy.h
 libdhcpsrv_unittests_SOURCES += subnet_unittest.cc
 libdhcpsrv_unittests_SOURCES += triplet_unittest.cc
+libdhcpsrv_unittests_SOURCES += test_utils.cc test_utils.h
 
 libdhcpsrv_unittests_CPPFLAGS = $(AM_CPPFLAGS) $(GTEST_INCLUDES) $(LOG4CPLUS_INCLUDES)
 if HAVE_MYSQL

src/lib/dhcpsrv/tests/alloc_engine_unittest.cc

@@ -22,6 +22,8 @@
 #include <dhcpsrv/lease_mgr_factory.h>
 #include <dhcpsrv/memfile_lease_mgr.h>
 
+#include <dhcpsrv/tests/test_utils.h>
+
 #include <boost/shared_ptr.hpp>
 #include <boost/scoped_ptr.hpp>
 #include <gtest/gtest.h>
@@ -35,6 +37,7 @@ using namespace std;
 using namespace isc;
 using namespace isc::asiolink;
 using namespace isc::dhcp;
+using namespace isc::dhcp::test;
 
 namespace {
 
@@ -108,26 +111,6 @@ TEST_F(AllocEngineTest, constructor) {
     ASSERT_NO_THROW(x.reset(new AllocEngine(AllocEngine::ALLOC_ITERATIVE, 100)));
 }
 
-/// @todo: This method is taken from mysql_lease_mgr_utilities.cc from ticket
-/// #2342. Get rid of one instance once the code is merged
-void
-detailCompareLease6(const Lease6Ptr& first, const Lease6Ptr& second) {
-    EXPECT_EQ(first->type_, second->type_);
-
-    // Compare address strings - odd things happen when they are different
-    // as the EXPECT_EQ appears to call the operator uint32_t() function,
-    // which causes an exception to be thrown for IPv6 addresses.
-    EXPECT_EQ(first->addr_.toText(), second->addr_.toText());
-    EXPECT_EQ(first->prefixlen_, second->prefixlen_);
-    EXPECT_EQ(first->iaid_, second->iaid_);
-    EXPECT_TRUE(*first->duid_ == *second->duid_);
-    EXPECT_EQ(first->preferred_lft_, second->preferred_lft_);
-    EXPECT_EQ(first->valid_lft_, second->valid_lft_);
-    EXPECT_EQ(first->cltt_, second->cltt_);
-    EXPECT_EQ(first->subnet_id_, second->subnet_id_);
-}
-
-
 // This test checks if the simple allocation can succeed
 TEST_F(AllocEngineTest, simpleAlloc) {
     boost::scoped_ptr<AllocEngine> engine;
@@ -148,7 +131,7 @@ TEST_F(AllocEngineTest, simpleAlloc) {
     ASSERT_TRUE(from_mgr);
 
     // Now check that the lease in LeaseMgr has the same parameters
-    detailCompareLease6(lease, from_mgr);
+    detailCompareLease(lease, from_mgr);
 }
 
 // This test checks if the fake allocation (for SOLICIT) can succeed
@@ -196,7 +179,7 @@ TEST_F(AllocEngineTest, allocWithValidHint) {
     ASSERT_TRUE(from_mgr);
 
     // Now check that the lease in LeaseMgr has the same parameters
-    detailCompareLease6(lease, from_mgr);
+    detailCompareLease(lease, from_mgr);
 }
 
 // This test checks if the allocation with a hint that is in range,
@@ -235,7 +218,7 @@ TEST_F(AllocEngineTest, allocWithUsedHint) {
     ASSERT_TRUE(from_mgr);
 
     // Now check that the lease in LeaseMgr has the same parameters
-    detailCompareLease6(lease, from_mgr);
+    detailCompareLease(lease, from_mgr);
 }
 
 // This test checks if the allocation with a hint that is out the blue
@@ -265,7 +248,7 @@ TEST_F(AllocEngineTest, allocBogusHint) {
     ASSERT_TRUE(from_mgr);
 
     // Now check that the lease in LeaseMgr has the same parameters
-    detailCompareLease6(lease, from_mgr);
+    detailCompareLease(lease, from_mgr);
 }
 
 // This test verifies that the allocator picks addresses that belong to the
@@ -370,7 +353,7 @@ TEST_F(AllocEngineTest, smallPool) {
     ASSERT_TRUE(from_mgr);
 
     // Now check that the lease in LeaseMgr has the same parameters
-    detailCompareLease6(lease, from_mgr);
+    detailCompareLease(lease, from_mgr);
 }
 
 // This test checks if all addresses in a pool are currently used, the attempt
@@ -487,7 +470,7 @@ TEST_F(AllocEngineTest, requestReuseExpiredLease) {
     ASSERT_TRUE(from_mgr);
 
     // Now check that the lease in LeaseMgr has the same parameters
-    detailCompareLease6(lease, from_mgr);
+    detailCompareLease(lease, from_mgr);
 }
 
 }; // end of anonymous namespace

src/lib/dhcpsrv/tests/mysql_lease_mgr_unittest.cc

@@ -17,6 +17,7 @@
 #include <asiolink/io_address.h>
 #include <dhcpsrv/lease_mgr_factory.h>
 #include <dhcpsrv/mysql_lease_mgr.h>
+#include <dhcpsrv/tests/test_utils.h>
 
 #include <gtest/gtest.h>
 
@@ -29,6 +30,7 @@
 using namespace isc;
 using namespace isc::asiolink;
 using namespace isc::dhcp;
+using namespace isc::dhcp::test;
 using namespace std;
 
 namespace {
@@ -537,48 +539,6 @@ public:
     vector<IOAddress> ioaddress6_;  ///< IOAddress forms of IPv6 addresses
 };
 
-///@{
-/// @brief Test Utilities
-///
-/// The follow are a set of functions used during the tests.
-
-/// @brief Compare two Lease4 structures for equality
-void
-detailCompareLease(const Lease4Ptr& first, const Lease4Ptr& second) {
-    // Compare address strings.  Comparison of address objects is not used, as
-    // odd things happen when they are different: the EXPECT_EQ macro appears to
-    // call the operator uint32_t() function, which causes an exception to be
-    // thrown for IPv6 addresses.
-    EXPECT_EQ(first->addr_.toText(), second->addr_.toText());
-    EXPECT_TRUE(first->hwaddr_ == second->hwaddr_);
-    EXPECT_TRUE(*first->client_id_ == *second->client_id_);
-    EXPECT_EQ(first->valid_lft_, second->valid_lft_);
-    EXPECT_EQ(first->cltt_, second->cltt_);
-    EXPECT_EQ(first->subnet_id_, second->subnet_id_);
-}
-
-/// @brief Compare two Lease6 structures for equality
-void
-detailCompareLease(const Lease6Ptr& first, const Lease6Ptr& second) {
-    EXPECT_EQ(first->type_, second->type_);
-
-    // Compare address strings.  Comparison of address objects is not used, as
-    // odd things happen when they are different: the EXPECT_EQ macro appears to
-    // call the operator uint32_t() function, which causes an exception to be
-    // thrown for IPv6 addresses.
-    EXPECT_EQ(first->addr_.toText(), second->addr_.toText());
-    EXPECT_EQ(first->prefixlen_, second->prefixlen_);
-    EXPECT_EQ(first->iaid_, second->iaid_);
-    EXPECT_TRUE(*first->duid_ == *second->duid_);
-    EXPECT_EQ(first->preferred_lft_, second->preferred_lft_);
-    EXPECT_EQ(first->valid_lft_, second->valid_lft_);
-    EXPECT_EQ(first->cltt_, second->cltt_);
-    EXPECT_EQ(first->subnet_id_, second->subnet_id_);
-}
-
-///@}
-
-
 /// @brief Check that database can be opened
 ///
 /// This test checks if the MySqlLeaseMgr can be instantiated.  This happens

src/lib/dhcpsrv/tests/test_utils.cc

@@ -0,0 +1,58 @@
+// Copyright (C) 2012 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
+// copyright notice and this permission notice appear in all copies.
+//
+// THE SOFTWARE IS PROVIDED "AS IS" AND ISC DISCLAIMS ALL WARRANTIES WITH
+// REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY
+// AND FITNESS.  IN NO EVENT SHALL ISC BE LIABLE FOR ANY SPECIAL, DIRECT,
+// INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM
+// LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE
+// OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR
+// PERFORMANCE OF THIS SOFTWARE.
+
+#include "test_utils.h"
+#include <gtest/gtest.h>
+
+namespace isc {
+namespace dhcp {
+namespace test {
+
+void
+detailCompareLease(const Lease4Ptr& first, const Lease4Ptr& second) {
+    // Compare address strings.  Comparison of address objects is not used, as
+    // odd things happen when they are different: the EXPECT_EQ macro appears to
+    // call the operator uint32_t() function, which causes an exception to be
+    // thrown for IPv6 addresses.
+    EXPECT_EQ(first->addr_.toText(), second->addr_.toText());
+    EXPECT_TRUE(first->hwaddr_ == second->hwaddr_);
+    EXPECT_TRUE(*first->client_id_ == *second->client_id_);
+    EXPECT_EQ(first->valid_lft_, second->valid_lft_);
+    EXPECT_EQ(first->cltt_, second->cltt_);
+    EXPECT_EQ(first->subnet_id_, second->subnet_id_);
+}
+
+void
+detailCompareLease(const Lease6Ptr& first, const Lease6Ptr& second) {
+    EXPECT_EQ(first->type_, second->type_);
+
+    // Compare address strings.  Comparison of address objects is not used, as
+    // odd things happen when they are different: the EXPECT_EQ macro appears to
+    // call the operator uint32_t() function, which causes an exception to be
+    // thrown for IPv6 addresses.
+    EXPECT_EQ(first->addr_.toText(), second->addr_.toText());
+    EXPECT_EQ(first->prefixlen_, second->prefixlen_);
+    EXPECT_EQ(first->iaid_, second->iaid_);
+    ASSERT_TRUE(first->duid_);
+    ASSERT_TRUE(second->duid_);
+    EXPECT_TRUE(*first->duid_ == *second->duid_);
+    EXPECT_EQ(first->preferred_lft_, second->preferred_lft_);
+    EXPECT_EQ(first->valid_lft_, second->valid_lft_);
+    EXPECT_EQ(first->cltt_, second->cltt_);
+    EXPECT_EQ(first->subnet_id_, second->subnet_id_);
+}
+
+};
+};
+};

src/lib/dhcpsrv/tests/test_utils.h

@@ -0,0 +1,49 @@
+// Copyright (C) 2012 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
+// copyright notice and this permission notice appear in all copies.
+//
+// THE SOFTWARE IS PROVIDED "AS IS" AND ISC DISCLAIMS ALL WARRANTIES WITH
+// REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY
+// AND FITNESS.  IN NO EVENT SHALL ISC BE LIABLE FOR ANY SPECIAL, DIRECT,
+// INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM
+// LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE
+// OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR
+// PERFORMANCE OF THIS SOFTWARE.
+
+#ifndef LIBDHCPSRV_TEST_UTILS_H
+#define LIBDHCPSRV_TEST_UTILS_H
+
+#include <dhcpsrv/lease_mgr.h>
+
+namespace isc {
+namespace dhcp {
+namespace test {
+
+// @brief performs details comparison between two IPv6 leases
+//
+// @param first first lease to compare
+// @param second second lease to compare
+//
+// This method is intended to be run from gtest tests as it
+// uses gtest macros and possibly reports gtest failures.
+void
+detailCompareLease(const Lease6Ptr& first, const Lease6Ptr& second);
+
+// @brief performs details comparison between two IPv4 leases
+//
+// @param first first lease to compare
+// @param second second lease to compare
+//
+// This method is intended to be run from gtest tests as it
+// uses gtest macros and possibly reports gtest failures.
+void
+detailCompareLease(const Lease4Ptr& first, const Lease4Ptr& second);
+
+
+};
+};
+};
+
+#endif