From: Tomek Mrugalski Date: Tue, 3 Feb 2015 15:29:07 +0000 (+0100) Subject: [3563] Changes after review: X-Git-Tag: trac3712_base~21^2~4 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=e767ad16875e06fc33544c559cd96ad314819d4b;p=thirdparty%2Fkea.git [3563] Changes after review: - general improvements in AllocEngine - doc updated --- diff --git a/src/lib/dhcpsrv/alloc_engine.cc b/src/lib/dhcpsrv/alloc_engine.cc index 30de403160..360809766f 100644 --- a/src/lib/dhcpsrv/alloc_engine.cc +++ b/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 @@ -299,11 +299,10 @@ AllocEngine::allocateLeases6(ClientContext6& ctx) { try { if (!ctx.subnet_) { - isc_throw(InvalidOperation, "Subnet is required for allocation"); - } - + isc_throw(InvalidOperation, "Subnet is required for IPv6 lease allocation"); + } else if (!ctx.duid_) { - isc_throw(InvalidOperation, "DUID is mandatory for allocation"); + isc_throw(InvalidOperation, "DUID is mandatory for IPv6 lease allocation"); } // Check which host reservation mode is supported in this subnet. @@ -315,6 +314,9 @@ AllocEngine::allocateLeases6(ClientContext6& ctx) { ctx.host_ = HostMgr::instance().get6(ctx.subnet_->getID(), ctx.duid_, ctx.hwaddr_); + } else { + // Let's explicitly set it to NULL if reservations are disabled. + ctx.host_.reset(); } // Check if there are existing leases for that subnet/duid/iaid @@ -336,7 +338,7 @@ AllocEngine::allocateLeases6(ClientContext6& ctx) { // 3.1 are the leases matching reservations? // yes: renew them => done // no: release existing leases, assign new ones based on reservations - // Case 4. if there are no leases and no reservations... + // Case 4/catch-all. if there are no leases and no reservations... // assign new leases // // We could implement those checks as nested ifs, but the performance @@ -344,7 +346,7 @@ AllocEngine::allocateLeases6(ClientContext6& ctx) { // Hence independent checks. // Case 1: There are no leases and there's a reservation for this host. - if ((hr_mode != Subnet::HR_DISABLED) && leases.empty() && ctx.host_) { + if (leases.empty() && ctx.host_) { // Try to allocate leases that match reservations. Typically this will // succeed, except cases where the reserved addresses are used by @@ -377,13 +379,13 @@ AllocEngine::allocateLeases6(ClientContext6& ctx) { removeNonmatchingReservedLeases6(ctx, leases); if (!leases.empty()) { - // Return old leases so the server can see what has changed. + // Return old leases so the server can see what has changed. return (updateFqdnData(leases, ctx.fwd_dns_update_, ctx.rev_dns_update_, ctx.hostname_, ctx.fake_allocation_)); } - // If leases is empty at this stage, it means that we used to have + // If leases are empty at this stage, it means that we used to have // leases for this client, but we checked and those leases are reserved // for someone else, so we lost them. We will need to continue and // will finally end up in case 4 (no leases, no reservations), so we'll @@ -401,7 +403,12 @@ AllocEngine::allocateLeases6(ClientContext6& ctx) { // from reservations. // Second, let's remove leases that are reserved for someone else. - // This applies to any existing leases. + // This applies to any existing leases. This will not happen frequently, + // but it may happen with the following chain of events: + // 1. client A gets address X; + // 2. reservation for client B for address X is made by a administrator; + // 3. client A reboots + // 4. client A requests the address (X) he got previously removeNonmatchingReservedLeases6(ctx, leases); // leases now contain existing and new leases, but we removed those @@ -413,7 +420,7 @@ AllocEngine::allocateLeases6(ClientContext6& ctx) { // Caveat: do this only if we have at least one reserved address. removeNonreservedLeases6(ctx, leases); - // All checks are done. Let's hope we do have any leases left. + // All checks are done. Let's hope we have some leases left. // If we have any leases left, let's return them and we're done. if (!leases.empty()) { @@ -431,7 +438,7 @@ AllocEngine::allocateLeases6(ClientContext6& ctx) { // someone else, so we released it. } - // Case 4: One of the following is true: + // Case 4/catch-all: One of the following is true: // - we don't have leases and there are no reservations // - we used to have leases, but we lost them, because they are now // reserved for someone else @@ -439,11 +446,14 @@ AllocEngine::allocateLeases6(ClientContext6& ctx) { // is still used by someone else // // In any case, we need to go through normal lease assignment process - // for now. + // for now. This is also a catch-all or last resort attempt, when we + // couldn't find any reservations (or couldn't use them). leases = allocateUnreservedLeases6(ctx); - return (leases); + if (!leases.empty()) { + return (leases); + } // Unable to allocate an address, return an empty lease. LOG_WARN(dhcpsrv_logger, DHCPSRV_ADDRESS6_ALLOC_FAIL).arg(attempts_); @@ -508,7 +518,7 @@ AllocEngine::allocateUnreservedLeases6(ClientContext6& ctx) { // It can happen that the lease allocation failed (we could // have lost the race condition. That means that the hint is - // lo longer usable and we need to continue the regular + // no longer usable and we need to continue the regular // allocation path. if (lease) { @@ -547,8 +557,9 @@ AllocEngine::allocateUnreservedLeases6(ClientContext6& ctx) { } } - // Hint is in the pool but is not available. Search the pool until first of - // the following occurs: + // The hint was useless (it was not provided at all, was used by someone else, + // was out of pool or reserved for someone else). 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 @@ -599,9 +610,8 @@ AllocEngine::allocateUnreservedLeases6(ClientContext6& ctx) { // old lease should be NULL. ctx.old_leases_.clear(); - Lease6Collection collection; - collection.push_back(lease); - return (collection); + leases.push_back(lease); + return (leases); } // Although the address was free just microseconds ago, it may have @@ -617,9 +627,9 @@ AllocEngine::allocateUnreservedLeases6(ClientContext6& ctx) { existing = reuseExpiredLease(existing, ctx, prefix_len); - Lease6Collection collection; - collection.push_back(existing); - return (collection); + + leases.push_back(existing); + return (leases); } } @@ -717,15 +727,20 @@ AllocEngine::removeNonmatchingReservedLeases6(ClientContext6& ctx, bool AllocEngine::removeLeases(Lease6Collection& container, const asiolink::IOAddress& addr) { + bool removed = false; for (Lease6Collection::iterator lease = container.begin(); lease != container.end(); ++lease) { if ((*lease)->addr_ == addr) { - container.erase(lease); - return (true); + lease->reset(); + removed = true; } } - return (false); + // Remove all elements that have NULL value + container.erase(std::remove(container.begin(), container.end(), Lease6Ptr()), + container.end()); + + return (removed); } void @@ -740,15 +755,13 @@ AllocEngine::removeNonreservedLeases6(ClientContext6& ctx, // This is the total number of leases. We should not remove the last one. int total = existing_leases.size(); - // This is tricky/scary code. It iterates and possibly deletes at the same time. + // This is officially not scary code anymore. iterates and marks specified + // leases for deletion, by setting appropriate pointers to NULL. for (Lease6Collection::iterator lease = existing_leases.begin(); - lease != existing_leases.end();) { + lease != existing_leases.end(); ++lease) { IPv6Resrv resv(ctx.type_ == Lease::TYPE_NA ? IPv6Resrv::TYPE_NA : IPv6Resrv::TYPE_PD, (*lease)->addr_, (*lease)->prefixlen_); - if (ctx.host_->hasReservation(resv)) { - // This is a lease for reserved address. Let's keep it. - ++lease; - } else { + if (!ctx.host_->hasReservation(resv)) { // We have reservations, but not for this lease. Release it. // Remove this lease from LeaseMgr @@ -759,17 +772,23 @@ AllocEngine::removeNonreservedLeases6(ClientContext6& ctx, // Add this to the list of removed leases. ctx.old_leases_.push_back(*lease); - // This is tricky part. We move lease to the next element and - // then pass the old value to erase. - existing_leases.erase(lease++); + // Set this pointer to NULL. The pointer is still valid. We're just + // setting the Lease6Ptr to NULL value. We'll remove all NULL + // pointers once the loop is finished. + lease->reset(); if (--total == 1) { - // If there's only one lease left, return. - return; + // If there's only one lease left, break the loop. + break; } } } + + // Remove all elements that we previously marked for deletion (those that + // have NULL value). + existing_leases.erase(std::remove(existing_leases.begin(), + existing_leases.end(), Lease6Ptr()), existing_leases.end()); } Lease4Ptr diff --git a/src/lib/dhcpsrv/alloc_engine.h b/src/lib/dhcpsrv/alloc_engine.h index 7f035c6a70..8735498c2d 100644 --- a/src/lib/dhcpsrv/alloc_engine.h +++ b/src/lib/dhcpsrv/alloc_engine.h @@ -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 @@ -389,8 +389,8 @@ protected: /// @brief Default constructor. ClientContext6() : subnet_(), duid_(), iaid_(0), type_(Lease::TYPE_NA), hwaddr_(), - fwd_dns_update_(false), rev_dns_update_(false), hostname_(""), - callout_handle_(), fake_allocation_(false), host_() { + hints_(), fwd_dns_update_(false), rev_dns_update_(false), hostname_(""), + callout_handle_(), fake_allocation_(false), old_leases_(), host_() { } /// @brief Constructor with parameters. @@ -418,9 +418,10 @@ protected: const Lease::Type type, const bool fwd_dns, const bool rev_dns, const std::string& hostname, const bool fake_allocation): - subnet_(subnet), duid_(duid), iaid_(iaid), type_(type), - fwd_dns_update_(fwd_dns), rev_dns_update_(rev_dns), - hostname_(hostname), fake_allocation_(fake_allocation) { + subnet_(subnet), duid_(duid), iaid_(iaid), type_(type), hwaddr_(), + hints_(), fwd_dns_update_(fwd_dns), rev_dns_update_(rev_dns), + hostname_(hostname), fake_allocation_(fake_allocation), + old_leases_(), host_() { hints_.push_back(hint); // callout_handle, host pointers initiated to NULL by their // respective constructors. @@ -636,6 +637,8 @@ protected: /// collection as old leases.
/// @ref ClientContext6::hwaddr_ Hardware address (optional, may be null if /// not available)
+ /// @ref ClientContext6::host_ Host reservation. allocateLeases6 will set + /// this field, if appropriate reservation is found. /// /// @return Allocated IPv6 leases (may be empty if allocation failed) Lease6Collection @@ -779,7 +782,7 @@ private: /// not reserved for this client. It will leave at least one lease on the list, /// if possible. The reason to run this method is that if there is a reservation /// for address A for client X and client X already has a lease for a - /// diffierent address B, we should assign A and release B. However, + /// different address B, we should assign A and release B. However, /// if for some reason we can't assign A, keeping B would be better than /// not having a lease at all. Hence we may keep B if that's the only lease /// left. diff --git a/src/lib/dhcpsrv/tests/alloc_engine_unittest.cc b/src/lib/dhcpsrv/tests/alloc_engine_unittest.cc index 89d4b7d481..be919b5743 100644 --- a/src/lib/dhcpsrv/tests/alloc_engine_unittest.cc +++ b/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