BIND 10 trac3083, updated. 231fed6c34f32ce87f15be0f79bcc258f900a783 [3083] Return a copy of the lease when getLease is called.
BIND 10 source code commits
bind10-changes at lists.isc.org
Wed Aug 21 11:02:50 UTC 2013
The branch, trac3083 has been updated
via 231fed6c34f32ce87f15be0f79bcc258f900a783 (commit)
via bf6d6bcddd460b7fa2c77314bccb4e14ab856609 (commit)
from 3bc0e24e4d1692c73124fe9727141cc2552e5aa3 (commit)
Those revisions listed above that are new to this repository have
not appeared on any other notification email; so we list those
revisions in full, below.
- Log -----------------------------------------------------------------
commit 231fed6c34f32ce87f15be0f79bcc258f900a783
Author: Marcin Siodelski <marcin at isc.org>
Date: Wed Aug 21 13:02:25 2013 +0200
[3083] Return a copy of the lease when getLease is called.
Also, updateLease4 and updateLease6 are implemented.
commit bf6d6bcddd460b7fa2c77314bccb4e14ab856609
Author: Marcin Siodelski <marcin at isc.org>
Date: Wed Aug 21 12:15:21 2013 +0200
[3083] Allocation engine returns a copy of the previous lease instance.
-----------------------------------------------------------------------
Summary of changes:
src/lib/dhcpsrv/alloc_engine.cc | 8 ++++
src/lib/dhcpsrv/alloc_engine.h | 33 ++++++++++++--
src/lib/dhcpsrv/memfile_lease_mgr.cc | 27 +++++++----
src/lib/dhcpsrv/memfile_lease_mgr.h | 35 +++++++++-----
src/lib/dhcpsrv/tests/alloc_engine_unittest.cc | 58 ++++++++++++++++++++++--
5 files changed, 131 insertions(+), 30 deletions(-)
-----------------------------------------------------------------------
diff --git a/src/lib/dhcpsrv/alloc_engine.cc b/src/lib/dhcpsrv/alloc_engine.cc
index 531fff9..ac5e616 100644
--- a/src/lib/dhcpsrv/alloc_engine.cc
+++ b/src/lib/dhcpsrv/alloc_engine.cc
@@ -332,6 +332,9 @@ AllocEngine::allocateAddress4(const SubnetPtr& subnet,
const isc::hooks::CalloutHandlePtr& callout_handle,
Lease4Ptr& old_lease) {
+ // The NULL pointer indicates that the old lease didn't exist. It may
+ // be later set to non NULL value if existing lease is found in the
+ // database.
old_lease.reset();
try {
@@ -352,6 +355,7 @@ AllocEngine::allocateAddress4(const SubnetPtr& subnet,
// Check if there's existing lease for that subnet/clientid/hwaddr combination.
Lease4Ptr existing = LeaseMgrFactory::instance().getLease4(*hwaddr, subnet->getID());
if (existing) {
+ // Save the old lease, before renewal.
old_lease.reset(new Lease4(*existing));
// We have a lease already. This is a returning client, probably after
// its reboot.
@@ -367,6 +371,7 @@ AllocEngine::allocateAddress4(const SubnetPtr& subnet,
if (clientid) {
existing = LeaseMgrFactory::instance().getLease4(*clientid, subnet->getID());
if (existing) {
+ // Save the old lease before renewal.
old_lease.reset(new Lease4(*existing));
// we have a lease already. This is a returning client, probably after
// its reboot.
@@ -398,6 +403,7 @@ AllocEngine::allocateAddress4(const SubnetPtr& subnet,
}
} else {
if (existing->expired()) {
+ // Save the old lease, before reusing it.
old_lease.reset(new Lease4(*existing));
return (reuseExpiredLease(existing, subnet, clientid, hwaddr,
callout_handle, fake_allocation));
@@ -444,6 +450,8 @@ AllocEngine::allocateAddress4(const SubnetPtr& subnet,
// allocation attempts.
} else {
if (existing->expired()) {
+ // Save old lease before reusing it.
+ old_lease.reset(new Lease4(*existing));
return (reuseExpiredLease(existing, subnet, clientid, hwaddr,
callout_handle, fake_allocation));
}
diff --git a/src/lib/dhcpsrv/alloc_engine.h b/src/lib/dhcpsrv/alloc_engine.h
index 9a919dc..fd966dc 100644
--- a/src/lib/dhcpsrv/alloc_engine.h
+++ b/src/lib/dhcpsrv/alloc_engine.h
@@ -182,11 +182,33 @@ protected:
/// we give up (0 means unlimited)
AllocEngine(AllocType engine_type, unsigned int attempts);
- /// @brief Allocates an IPv4 lease
+ /// @brief Returns IPv4 lease.
///
- /// This method uses currently selected allocator to pick an address from
- /// specified subnet, creates a lease for that address and then inserts
- /// it into LeaseMgr (if this allocation is not fake).
+ /// This method finds the appropriate lease for the client using the
+ /// following algorithm:
+ /// - If lease exists for the combination of the HW address, client id and
+ /// subnet, try to renew a lease and return it.
+ /// - If lease exists for the combination of the client id and subnet, try
+ /// to renew the lease and return it.
+ /// - If client supplied an address hint and this address is available,
+ /// allocate the new lease with this address.
+ /// - If client supplied an address hint and the lease for this address
+ /// exists in the database, return this lease if it is expired.
+ /// - Pick new address from the pool and try to allocate it for the client,
+ /// if expired lease exists for the picked address, try to reuse this lease.
+ ///
+ /// When a server should do DNS updates, it is required that allocation
+ /// returns the information how the lease was obtained by the allocation
+ /// engine. In particular, the DHCP server should be able to check whether
+ /// existing lease was returned, or new lease was allocated. When existing
+ /// lease was returned, server should check whether the FQDN has changed
+ /// between the allocation of the old and new lease. If so, server should
+ /// perform appropriate DNS update. If not, server may choose to not
+ /// perform the update. The information about the old lease is returned via
+ /// @c old_lease parameter. If NULL value is returned, it is an indication
+ /// that new lease was allocated for the client. If non-NULL value is
+ /// returned, it is an indication that allocation engine reused/renewed an
+ /// existing lease.
///
/// @param subnet subnet the allocation should come from
/// @param clientid Client identifier
@@ -337,7 +359,8 @@ private:
/// an address for DISCOVER that is not really allocated (true)
/// @return refreshed lease
/// @throw BadValue if trying to recycle lease that is still valid
- Lease4Ptr reuseExpiredLease(Lease4Ptr& expired, const SubnetPtr& subnet,
+ Lease4Ptr reuseExpiredLease(Lease4Ptr& expired,
+ const SubnetPtr& subnet,
const ClientIdPtr& clientid,
const HWAddrPtr& hwaddr,
const isc::hooks::CalloutHandlePtr& callout_handle,
diff --git a/src/lib/dhcpsrv/memfile_lease_mgr.cc b/src/lib/dhcpsrv/memfile_lease_mgr.cc
index 713d625..804eef8 100644
--- a/src/lib/dhcpsrv/memfile_lease_mgr.cc
+++ b/src/lib/dhcpsrv/memfile_lease_mgr.cc
@@ -62,7 +62,7 @@ Lease4Ptr Memfile_LeaseMgr::getLease4(const isc::asiolink::IOAddress& addr) cons
if (l == storage4_.end()) {
return (Lease4Ptr());
} else {
- return (*l);
+ return (Lease4Ptr(new Lease4(**l)));
}
}
@@ -94,7 +94,7 @@ Lease4Ptr Memfile_LeaseMgr::getLease4(const HWAddr& hwaddr,
}
// Lease was found. Return it to the caller.
- return (*lease);
+ return (Lease4Ptr(new Lease4(**lease)));
}
Lease4Collection Memfile_LeaseMgr::getLease4(const ClientId& clientid) const {
@@ -123,11 +123,11 @@ Lease4Ptr Memfile_LeaseMgr::getLease4(const ClientId& client_id,
return Lease4Ptr();
}
// Lease was found. Return it to the caller.
- return (*lease);
+ return (Lease4Ptr(new Lease4(**lease)));
}
-Lease6Ptr Memfile_LeaseMgr::getLease6(
- const isc::asiolink::IOAddress& addr) const {
+Lease6Ptr
+Memfile_LeaseMgr::getLease6(const isc::asiolink::IOAddress& addr) const {
LOG_DEBUG(dhcpsrv_logger, DHCPSRV_DBG_TRACE_DETAIL,
DHCPSRV_MEMFILE_GET_ADDR6).arg(addr.toText());
@@ -135,7 +135,7 @@ Lease6Ptr Memfile_LeaseMgr::getLease6(
if (l == storage6_.end()) {
return (Lease6Ptr());
} else {
- return (*l);
+ return (Lease6Ptr(new Lease6(**l)));
}
}
@@ -167,20 +167,31 @@ Lease6Ptr Memfile_LeaseMgr::getLease6(const DUID& duid, uint32_t iaid,
return (Lease6Ptr());
}
// Lease was found, return it to the caller.
- return (*lease);
+ return (Lease6Ptr(new Lease6(**lease)));
}
void Memfile_LeaseMgr::updateLease4(const Lease4Ptr& lease) {
LOG_DEBUG(dhcpsrv_logger, DHCPSRV_DBG_TRACE_DETAIL,
DHCPSRV_MEMFILE_UPDATE_ADDR4).arg(lease->addr_.toText());
+ Lease4Storage::iterator lease_it = storage4_.find(lease->addr_);
+ if (lease_it == storage4_.end()) {
+ isc_throw(NoSuchLease, "failed to update the lease with address "
+ << lease->addr_.toText() << " - no such lease");
+ }
+ **lease_it = *lease;
}
void Memfile_LeaseMgr::updateLease6(const Lease6Ptr& lease) {
LOG_DEBUG(dhcpsrv_logger, DHCPSRV_DBG_TRACE_DETAIL,
DHCPSRV_MEMFILE_UPDATE_ADDR6).arg(lease->addr_.toText());
-
+ Lease6Storage::iterator lease_it = storage6_.find(lease->addr_);
+ if (lease_it == storage6_.end()) {
+ isc_throw(NoSuchLease, "failed to update the lease with address "
+ << lease->addr_.toText() << " - no such lease");
+ }
+ **lease_it = *lease;
}
bool Memfile_LeaseMgr::deleteLease(const isc::asiolink::IOAddress& addr) {
diff --git a/src/lib/dhcpsrv/memfile_lease_mgr.h b/src/lib/dhcpsrv/memfile_lease_mgr.h
index 4ac7536..2f75a98 100644
--- a/src/lib/dhcpsrv/memfile_lease_mgr.h
+++ b/src/lib/dhcpsrv/memfile_lease_mgr.h
@@ -65,8 +65,10 @@ public:
/// @brief Returns existing IPv4 lease for specified IPv4 address.
///
- /// @todo Not implemented yet
- /// @param addr address of the searched lease
+ /// This function returns a copy of the lease. The modification in the
+ /// return lease does not affect the instance held in the lease storage.
+ ///
+ /// @param addr An address of the searched lease.
///
/// @return a collection of leases
virtual Lease4Ptr getLease4(const isc::asiolink::IOAddress& addr) const;
@@ -85,10 +87,11 @@ public:
/// @return lease collection
virtual Lease4Collection getLease4(const isc::dhcp::HWAddr& hwaddr) const;
- /// @brief Returns existing IPv4 leases for specified hardware address
+ /// @brief Returns existing IPv4 lease for specified hardware address
/// and a subnet
///
- /// @todo Not implemented yet
+ /// This function returns a copy of the lease. The modification in the
+ /// return lease does not affect the instance held in the lease storage.
///
/// There can be at most one lease for a given HW address in a single
/// pool, so this method with either return a single lease or NULL.
@@ -109,11 +112,12 @@ public:
/// @brief Returns existing IPv4 lease for specified client-id
///
+ /// This function returns a copy of the lease. The modification in the
+ /// return lease does not affect the instance held in the lease storage.
+ ///
/// There can be at most one lease for a given HW address in a single
/// pool, so this method with either return a single lease or NULL.
///
- /// @todo Not implemented yet
- ///
/// @param clientid client identifier
/// @param subnet_id identifier of the subnet that lease must belong to
///
@@ -123,7 +127,10 @@ public:
/// @brief Returns existing IPv6 lease for a given IPv6 address.
///
- /// @param addr address of the searched lease
+ /// This function returns a copy of the lease. The modification in the
+ /// return lease does not affect the instance held in the lease storage.
+ ///
+ /// @param addr An address of the searched lease.
///
/// @return smart pointer to the lease (or NULL if a lease is not found)
virtual Lease6Ptr getLease6(const isc::asiolink::IOAddress& addr) const;
@@ -140,27 +147,31 @@ public:
/// @brief Returns existing IPv6 lease for a given DUID+IA combination
///
- /// @todo Not implemented yet
+ /// This function returns a copy of the lease. The modification in the
+ /// return lease does not affect the instance held in the lease storage.
///
/// @param duid client DUID
/// @param iaid IA identifier
/// @param subnet_id identifier of the subnet the lease must belong to
///
/// @return smart pointer to the lease (or NULL if a lease is not found)
- virtual Lease6Ptr getLease6(const DUID& duid, uint32_t iaid, SubnetID subnet_id) const;
+ virtual Lease6Ptr getLease6(const DUID& duid, uint32_t iaid,
+ SubnetID subnet_id) const;
/// @brief Updates IPv4 lease.
///
- /// @todo Not implemented yet
+ /// @warning This function does not validate the pointer to the lease.
+ /// It is caller's responsibility to pass the valid pointer.
///
/// @param lease4 The lease to be updated.
///
/// If no such lease is present, an exception will be thrown.
virtual void updateLease4(const Lease4Ptr& lease4);
- /// @brief Updates IPv4 lease.
+ /// @brief Updates IPv6 lease.
///
- /// @todo Not implemented yet
+ /// @warning This function does not validate the pointer to the lease.
+ /// It is caller's responsibility to pass the valid pointer.
///
/// @param lease6 The lease to be updated.
///
diff --git a/src/lib/dhcpsrv/tests/alloc_engine_unittest.cc b/src/lib/dhcpsrv/tests/alloc_engine_unittest.cc
index 6c6abdd..622e564 100644
--- a/src/lib/dhcpsrv/tests/alloc_engine_unittest.cc
+++ b/src/lib/dhcpsrv/tests/alloc_engine_unittest.cc
@@ -616,6 +616,8 @@ TEST_F(AllocEngine4Test, simpleAlloc4) {
IOAddress("0.0.0.0"), false,
CalloutHandlePtr(),
old_lease_);
+ // The new lease has been allocated, so the old lease should not exist.
+ EXPECT_FALSE(old_lease_);
// Check that we got a lease
ASSERT_TRUE(lease);
@@ -642,6 +644,9 @@ TEST_F(AllocEngine4Test, fakeAlloc4) {
CalloutHandlePtr(),
old_lease_);
+ // The new lease has been allocated, so the old lease should not exist.
+ EXPECT_FALSE(old_lease_);
+
// Check that we got a lease
ASSERT_TRUE(lease);
@@ -665,10 +670,12 @@ TEST_F(AllocEngine4Test, allocWithValidHint4) {
IOAddress("192.0.2.105"),
false, CalloutHandlePtr(),
old_lease_);
-
// Check that we got a lease
ASSERT_TRUE(lease);
+ // We have allocated the new lease, so the old lease should not exist.
+ EXPECT_FALSE(old_lease_);
+
// We should get what we asked for
EXPECT_EQ(lease->addr_.toText(), "192.0.2.105");
@@ -706,6 +713,10 @@ TEST_F(AllocEngine4Test, allocWithUsedHint4) {
IOAddress("192.0.2.106"),
false, CalloutHandlePtr(),
old_lease_);
+
+ // New lease has been allocated, so the old lease should not exist.
+ EXPECT_FALSE(old_lease_);
+
// Check that we got a lease
ASSERT_TRUE(lease);
@@ -744,6 +755,9 @@ TEST_F(AllocEngine4Test, allocBogusHint4) {
// Check that we got a lease
ASSERT_TRUE(lease);
+ // We have allocated a new lease, so the old lease should not exist.
+ EXPECT_FALSE(old_lease_);
+
// We should NOT get what we asked for, because it is used already
EXPECT_TRUE(lease->addr_.toText() != "10.1.1.1");
@@ -777,6 +791,7 @@ TEST_F(AllocEngine4Test, allocateAddress4Nulls) {
CalloutHandlePtr(),
old_lease_);
EXPECT_FALSE(lease);
+ EXPECT_FALSE(old_lease_);
// Allocations without client-id are allowed
clientid_ = ClientIdPtr();
@@ -786,6 +801,8 @@ TEST_F(AllocEngine4Test, allocateAddress4Nulls) {
old_lease_);
// Check that we got a lease
ASSERT_TRUE(lease);
+ // New lease has been allocated, so the old lease should not exist.
+ EXPECT_FALSE(old_lease_);
// Do all checks on the lease
checkLease4(lease);
@@ -893,6 +910,9 @@ TEST_F(AllocEngine4Test, smallPool4) {
// Check that we got that single lease
ASSERT_TRUE(lease);
+ // We have allocated new lease, so the old lease should not exist.
+ EXPECT_FALSE(old_lease_);
+
EXPECT_EQ("192.0.2.17", lease->addr_.toText());
// Do all checks on the lease
@@ -940,6 +960,7 @@ TEST_F(AllocEngine4Test, outOfAddresses4) {
CalloutHandlePtr(),
old_lease_);
EXPECT_FALSE(lease2);
+ EXPECT_FALSE(old_lease_);
}
// This test checks if an expired lease can be reused in DISCOVER (fake allocation)
@@ -962,21 +983,33 @@ TEST_F(AllocEngine4Test, discoverReuseExpiredLease4) {
uint8_t hwaddr2[] = { 0, 0xfe, 0xfe, 0xfe, 0xfe, 0xfe};
uint8_t clientid2[] = { 8, 7, 6, 5, 4, 3, 2, 1 };
time_t now = time(NULL) - 500; // Allocated 500 seconds ago
- Lease4Ptr lease(new Lease4(addr, clientid2, sizeof(clientid2), hwaddr2, sizeof(hwaddr2),
+ Lease4Ptr lease(new Lease4(addr, clientid2, sizeof(clientid2),
+ hwaddr2, sizeof(hwaddr2),
495, 100, 200, now, subnet_->getID()));
+ // Copy the lease, so as it can be compared with the old lease returned
+ // by the allocation engine.
+ Lease4 original_lease(*lease);
+
// Lease was assigned 500 seconds ago, but its valid lifetime is 495, so it
// is expired already
ASSERT_TRUE(lease->expired());
ASSERT_TRUE(LeaseMgrFactory::instance().addLease(lease));
// CASE 1: Asking for any address
- lease = engine->allocateAddress4(subnet_, clientid_, hwaddr_, IOAddress("0.0.0.0"),
+ lease = engine->allocateAddress4(subnet_, clientid_, hwaddr_,
+ IOAddress("0.0.0.0"),
true, CalloutHandlePtr(),
old_lease_);
// Check that we got that single lease
ASSERT_TRUE(lease);
EXPECT_EQ(addr.toText(), lease->addr_.toText());
+ // We are reusing expired lease, the old (expired) instance should be
+ // returned. The returned instance should be the same as the original
+ // lease.
+ ASSERT_TRUE(old_lease_);
+ EXPECT_TRUE(original_lease == *old_lease_);
+
// Do all checks on the lease (if subnet-id, preferred/valid times are ok etc.)
checkLease4(lease);
@@ -987,6 +1020,11 @@ TEST_F(AllocEngine4Test, discoverReuseExpiredLease4) {
// Check that we got that single lease
ASSERT_TRUE(lease);
EXPECT_EQ(addr.toText(), lease->addr_.toText());
+
+ // We are updating expired lease. The copy of the old lease should be
+ // returned and it should be equal to the original lease.
+ ASSERT_TRUE(old_lease_);
+ EXPECT_TRUE(*old_lease_ == original_lease);
}
// This test checks if an expired lease can be reused in REQUEST (actual allocation)
@@ -1001,8 +1039,13 @@ TEST_F(AllocEngine4Test, requestReuseExpiredLease4) {
uint8_t hwaddr2[] = { 0, 0xfe, 0xfe, 0xfe, 0xfe, 0xfe};
uint8_t clientid2[] = { 8, 7, 6, 5, 4, 3, 2, 1 };
time_t now = time(NULL) - 500; // Allocated 500 seconds ago
- Lease4Ptr lease(new Lease4(addr, clientid2, sizeof(clientid2), hwaddr2, sizeof(hwaddr2),
- 495, 100, 200, now, subnet_->getID()));
+ Lease4Ptr lease(new Lease4(addr, clientid2, sizeof(clientid2), hwaddr2,
+ sizeof(hwaddr2), 495, 100, 200, now,
+ subnet_->getID()));
+ // Make a copy of the lease, so as we can comapre that with the old lease
+ // instance returned by the allocation engine.
+ Lease4 original_lease(*lease);
+
// Lease was assigned 500 seconds ago, but its valid lifetime is 495, so it
// is expired already
ASSERT_TRUE(lease->expired());
@@ -1024,6 +1067,11 @@ TEST_F(AllocEngine4Test, requestReuseExpiredLease4) {
// Now check that the lease in LeaseMgr has the same parameters
detailCompareLease(lease, from_mgr);
+
+ // The allocation engine should return a copy of the old lease. This
+ // lease should be equal to the original lease.
+ ASSERT_TRUE(old_lease_);
+ EXPECT_TRUE(*old_lease_ == original_lease);
}
/// @todo write renewLease6
More information about the bind10-changes
mailing list