From a339db38d46b517d2f153d00476c826740b271a7 Mon Sep 17 00:00:00 2001 From: Tomek Mrugalski Date: Fri, 13 Feb 2015 21:43:09 +0100 Subject: [PATCH] [3677] Changes after review. --- src/bin/dhcp6/dhcp6_srv.cc | 94 +++++++++++----------- src/bin/dhcp6/dhcp6_srv.h | 2 +- src/bin/dhcp6/tests/dhcp6_client.h | 2 +- src/bin/dhcp6/tests/fqdn_unittest.cc | 2 +- src/lib/dhcpsrv/alloc_engine.cc | 2 +- src/lib/dhcpsrv/alloc_engine.h | 14 +--- src/lib/dhcpsrv/dhcpsrv_messages.mes | 1 + src/lib/dhcpsrv/lease.h | 7 -- src/lib/dhcpsrv/tests/alloc_engine_utils.h | 5 +- 9 files changed, 57 insertions(+), 72 deletions(-) diff --git a/src/bin/dhcp6/dhcp6_srv.cc b/src/bin/dhcp6/dhcp6_srv.cc index 37ac9a0f56..5dbec66b68 100644 --- a/src/bin/dhcp6/dhcp6_srv.cc +++ b/src/bin/dhcp6/dhcp6_srv.cc @@ -1268,8 +1268,6 @@ Dhcpv6Srv::assignIA_NA(const Subnet6Ptr& subnet, const DuidPtr& duid, fake_allocation = true; } - CalloutHandlePtr callout_handle = getCalloutHandle(query); - // At this point, we have to make make some decisions with respect to the // FQDN option that we have generated as a result of receiving client's // FQDN. In particular, we have to get to know if the DNS update will be @@ -1297,7 +1295,7 @@ Dhcpv6Srv::assignIA_NA(const Subnet6Ptr& subnet, const DuidPtr& duid, AllocEngine::ClientContext6 ctx(subnet, duid, ia->getIAID(), hint, Lease::TYPE_NA, do_fwd, do_rev, hostname, fake_allocation); - ctx.callout_handle_ = callout_handle; + ctx.callout_handle_ = getCalloutHandle(query); // Attempt to get MAC address using configured mechanisms. // It's ok if there response is NULL. Hardware address is optional in Lease6. @@ -1339,28 +1337,25 @@ Dhcpv6Srv::assignIA_NA(const Subnet6Ptr& subnet, const DuidPtr& duid, // but this is considered waste of bandwidth as absence of status // code is considered a success. - Lease6Ptr old_lease; - if (!ctx.changed_leases_.empty()) { - old_lease = *ctx.changed_leases_.begin(); - } - // Allocation engine may have returned an existing lease. If so, we - // have to check that the FQDN settings we provided are the same - // that were set. If they aren't, we will have to remove existing - // DNS records and update the lease with the new settings. - if (!fake_allocation && old_lease) { - conditionalNCRRemoval(old_lease, lease, hostname, do_fwd, do_rev); - } - old_lease.reset(); + if (!fake_allocation) { + Lease6Ptr old_lease; + if (!ctx.changed_leases_.empty()) { + old_lease = *ctx.changed_leases_.begin(); - // We need to repeat that check for leases that used to be used, but - // are no longer valid. - if (!ctx.old_leases_.empty()) { - old_lease = *ctx.old_leases_.begin(); - } - if (!fake_allocation && old_lease) { - conditionalNCRRemoval(old_lease, lease, hostname, do_fwd, do_rev); - } + // Allocation engine has returned an existing lease. If so, we + // have to check that the FQDN settings we provided are the same + // that were set. If they aren't, we will have to remove existing + // DNS records and update the lease with the new settings. + conditionalNCRRemoval(old_lease, lease, hostname, do_fwd, do_rev); + } + // We need to repeat that check for leases that used to be used, but + // are no longer valid. + if (!ctx.old_leases_.empty()) { + old_lease = *ctx.old_leases_.begin(); + conditionalNCRRemoval(old_lease, lease, hostname, do_fwd, do_rev); + } + } } else { // Allocation engine did not allocate a lease. The engine logged // cause of that failure. The only thing left is to insert @@ -1381,11 +1376,7 @@ void Dhcpv6Srv::conditionalNCRRemoval(Lease6Ptr& old_lease, Lease6Ptr& new_lease, const std::string& hostname, bool do_fwd, bool do_rev) { - if (!old_lease) { - return; - } - - if (!new_lease->hasIdenticalFqdn(*old_lease)) { + if (old_lease && !new_lease->hasIdenticalFqdn(*old_lease)) { LOG_DEBUG(dhcp6_logger, DBG_DHCP6_DETAIL, DHCP6_DDNS_LEASE_ASSIGN_FQDN_CHANGE) .arg(old_lease->toText()) .arg(hostname) @@ -1440,8 +1431,6 @@ Dhcpv6Srv::assignIA_PD(const Subnet6Ptr& subnet, const DuidPtr& duid, // allocation. bool fake_allocation = (query->getType() == DHCPV6_SOLICIT); - CalloutHandlePtr callout_handle = getCalloutHandle(query); - // Use allocation engine to pick a lease for this client. Allocation engine // will try to honour the hint, but it is just a hint - some other address // may be used instead. If fake_allocation is set to false, the lease will @@ -1449,7 +1438,7 @@ Dhcpv6Srv::assignIA_PD(const Subnet6Ptr& subnet, const DuidPtr& duid, Lease6Collection old_leases; AllocEngine::ClientContext6 ctx(subnet, duid, ia->getIAID(), hint, Lease::TYPE_PD, false, false, string(), fake_allocation); - ctx.callout_handle_ = callout_handle; + ctx.callout_handle_ = getCalloutHandle(query); // Attempt to get MAC address using any of available mechanisms. // It's ok if there response is NULL. Hardware address is optional in Lease6 @@ -1542,15 +1531,15 @@ Dhcpv6Srv::extendIA_NA(const Subnet6Ptr& subnet, const DuidPtr& duid, // server didn't force the update. bool do_fwd = false; bool do_rev = false; + std::string hostname; Option6ClientFqdnPtr fqdn = boost::dynamic_pointer_cast< Option6ClientFqdn>(answer->getOption(D6O_CLIENT_FQDN)); if (fqdn) { CfgMgr::instance().getD2ClientMgr().getUpdateDirections(*fqdn, do_fwd, do_rev); - } - std::string hostname; - if (fqdn && (do_fwd || do_rev)) { - hostname = fqdn->getDomainName(); + if (do_fwd || do_rev) { + hostname = fqdn->getDomainName(); + } } // Create client context for this renewal @@ -1568,7 +1557,6 @@ Dhcpv6Srv::extendIA_NA(const Subnet6Ptr& subnet, const DuidPtr& duid, ctx.hwaddr_ = getMAC(query); // Extract the addresses that the client is trying to obtain. - int hints_count = 0; OptionCollection addrs = ia->getOptions(); for (OptionCollection::const_iterator it = addrs.begin(); it != addrs.end(); ++it) { @@ -1578,15 +1566,20 @@ Dhcpv6Srv::extendIA_NA(const Subnet6Ptr& subnet, const DuidPtr& duid, Option6IAAddrPtr iaaddr = boost::dynamic_pointer_cast(it->second); if (!iaaddr) { // That's weird. Option code was ok, but the object type was not. + // As we use Dhcpv6Srv::unpackOptions() that is guaranteed to use + // Option7IAAddr for D6O_IAADDR, this should never happen. The only + // case would be with badly mis-implemented hook libraries that + // insert invalid option objects. There's no way to protect against + // this. continue; } ctx.hints_.push_back(make_pair(iaaddr->getAddress(), 128)); - - // We need to remember it as we'll be removing hints from this list as - // we extend, cancel or otherwise deal with the leases. - hints_count++; } + // We need to remember it as we'll be removing hints from this list as + // we extend, cancel or otherwise deal with the leases. + bool hints_present = !ctx.hints_.empty(); + Lease6Collection leases = alloc_engine_->renewLeases6(ctx); // Ok, now we have the leases extended. We have: @@ -1635,7 +1628,7 @@ Dhcpv6Srv::extendIA_NA(const Subnet6Ptr& subnet, const DuidPtr& duid, // Finally, if there are any addresses requested that we haven't dealt with // already, inform the client that he can't have them. - for (AllocEngine::HintContainerConstIter hint = ctx.hints_.begin(); + for (AllocEngine::HintContainer::const_iterator hint = ctx.hints_.begin(); hint != ctx.hints_.end(); ++hint) { Option6IAAddrPtr iaaddr(new Option6IAAddr(D6O_IAADDR, hint->first, 0, 0)); @@ -1647,7 +1640,7 @@ Dhcpv6Srv::extendIA_NA(const Subnet6Ptr& subnet, const DuidPtr& duid, // We did not assign anything. If client has sent something, then // the status code is NoBinding, if he sent an empty IA_NA, then it's // NoAddrsAvailable - if (hints_count) { + if (hints_present) { // Insert status code NoBinding to indicate that the lease does not // exist for this client. ia_rsp->addOption(createStatusCode(STATUS_NoBinding, @@ -1730,7 +1723,6 @@ Dhcpv6Srv::extendIA_PD(const Subnet6Ptr& subnet, const DuidPtr& duid, ctx.hwaddr_ = getMAC(query); // Extract prefixes that the client is trying to renew. - int hints_count = 0; OptionCollection addrs = ia->getOptions(); for (OptionCollection::const_iterator it = addrs.begin(); it != addrs.end(); ++it) { @@ -1740,16 +1732,20 @@ Dhcpv6Srv::extendIA_PD(const Subnet6Ptr& subnet, const DuidPtr& duid, Option6IAPrefixPtr prf = boost::dynamic_pointer_cast(it->second); if (!prf) { // That's weird. Option code was ok, but the object type was not. + // As we use Dhcpv6Srv::unpackOptions() that is guaranteed to use + // Option7IAAddr for D6O_IAADDR, this should never happen. The only + // case would be with badly mis-implemented hook libraries that + // insert invalid option objects. There's no way to protect against + // this. continue; } // Put the client's prefix into the hints list. ctx.hints_.push_back(make_pair(prf->getAddress(), prf->getLength())); - - // We need to remember it as we'll be removing hints from this list as - // we extend, cancel or otherwise deal with the leases. - hints_count++; } + // We need to remember it as we'll be removing hints from this list as + // we extend, cancel or otherwise deal with the leases. + bool hints_present = !ctx.hints_.empty(); // Call Allocation Engine and attempt to renew leases. Number of things // may happen. Leases may be extended, revoked (if the lease is no longer @@ -1781,7 +1777,7 @@ Dhcpv6Srv::extendIA_PD(const Subnet6Ptr& subnet, const DuidPtr& duid, // zero lifetimes // Finally, if there are any addresses requested that we haven't dealt with // already, inform the client that he can't have them. - for (AllocEngine::HintContainerIter prefix = ctx.hints_.begin(); + for (AllocEngine::HintContainer::const_iterator prefix = ctx.hints_.begin(); prefix != ctx.hints_.end(); ++prefix) { OptionPtr prefix_opt(new Option6IAPrefix(D6O_IAPREFIX, prefix->first, prefix->second, 0, 0)); @@ -1795,7 +1791,7 @@ Dhcpv6Srv::extendIA_PD(const Subnet6Ptr& subnet, const DuidPtr& duid, // We did not assign anything. If client has sent something, then // the status code is NoBinding, if he sent an empty IA_NA, then it's // NoAddrsAvailable - if (hints_count) { + if (hints_present) { // Insert status code NoBinding to indicate that the lease does not // exist for this client. ia_rsp->addOption(createStatusCode(STATUS_NoBinding, diff --git a/src/bin/dhcp6/dhcp6_srv.h b/src/bin/dhcp6/dhcp6_srv.h index f7a57e6f8a..2f294de5bd 100644 --- a/src/bin/dhcp6/dhcp6_srv.h +++ b/src/bin/dhcp6/dhcp6_srv.h @@ -1,4 +1,4 @@ -// Copyright (C) 2011-2014 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2011-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 diff --git a/src/bin/dhcp6/tests/dhcp6_client.h b/src/bin/dhcp6/tests/dhcp6_client.h index 6bade76fdb..bb63220647 100644 --- a/src/bin/dhcp6/tests/dhcp6_client.h +++ b/src/bin/dhcp6/tests/dhcp6_client.h @@ -408,7 +408,7 @@ public: /// @brief returns client-id /// @return client-id - DuidPtr getDuid() { + DuidPtr getDuid() const { return (duid_); } diff --git a/src/bin/dhcp6/tests/fqdn_unittest.cc b/src/bin/dhcp6/tests/fqdn_unittest.cc index 60d69ede32..c87f7a7577 100644 --- a/src/bin/dhcp6/tests/fqdn_unittest.cc +++ b/src/bin/dhcp6/tests/fqdn_unittest.cc @@ -1,4 +1,4 @@ -// Copyright (C) 2013-2014 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2013-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 diff --git a/src/lib/dhcpsrv/alloc_engine.cc b/src/lib/dhcpsrv/alloc_engine.cc index 4b54d19a60..8e7396b806 100644 --- a/src/lib/dhcpsrv/alloc_engine.cc +++ b/src/lib/dhcpsrv/alloc_engine.cc @@ -671,7 +671,7 @@ AllocEngine::allocateReservedLeases6(ClientContext6& ctx, Lease6Collection& exis uint8_t prefix_len = resv->second.getPrefixLen(); // Check if already have this lease on the existing_leases list. - for (Lease6CollectionConstIter l = existing_leases.begin(); + for (Lease6Collection::const_iterator l = existing_leases.begin(); l != existing_leases.end(); ++l) { // Ok, we already have a lease for this reservation and it's usable diff --git a/src/lib/dhcpsrv/alloc_engine.h b/src/lib/dhcpsrv/alloc_engine.h index c9a65055b7..418fe8cb70 100644 --- a/src/lib/dhcpsrv/alloc_engine.h +++ b/src/lib/dhcpsrv/alloc_engine.h @@ -311,13 +311,7 @@ protected: typedef std::pair HintType; /// @brief Container for client's hints. - typedef std::vector< HintType > HintContainer; - - /// @brief Non-const iterator for hint container. - typedef HintContainer::iterator HintContainerIter; - - /// @brief Const iterator for hint container. - typedef HintContainer::const_iterator HintContainerConstIter; + typedef std::vector HintContainer; /// @brief Context information for the DHCPv6 leases allocation. /// @@ -422,8 +416,8 @@ protected: /// @brief Specifies whether new leases in Renew/Rebind are allowed /// /// This field controls what to do when renewing or rebinding client - /// does not have any leases. RFC3315 and stateful-issues draft does not - /// specify it and it is left up to the server configuration policy. + /// does not have any leases. RFC3315 and the stateful-issues draft does + /// not specify it and it is left up to the server configuration policy. /// False (the default) means that the client will not get any new /// unreserved leases if his existing leases are no longer suitable. /// True means that the allocation engine will do its best to assign @@ -982,7 +976,7 @@ private: /// This method attempts to extend the lease. It will call the lease6_renew /// or lease6_rebind hooks (depending on the client's message specified in /// ctx.query). The lease will be extended in LeaseMgr, unless the hooks - /// library will set skip flag. + /// library will set the skip flag. /// /// @param ctx client context that passes all necessary information. See /// @ref ClientContext6 for details. diff --git a/src/lib/dhcpsrv/dhcpsrv_messages.mes b/src/lib/dhcpsrv/dhcpsrv_messages.mes index 2145a5f631..85444479c7 100644 --- a/src/lib/dhcpsrv/dhcpsrv_messages.mes +++ b/src/lib/dhcpsrv/dhcpsrv_messages.mes @@ -203,6 +203,7 @@ no lease4 should be assigned. The server will not put that lease in its database and the client will get a NAK packet. % DHCPSRV_HOOK_LEASE6_EXTEND_SKIP DHCPv6 lease lifetime was not extended because a callout set the skip flag for message %1 +This debug message is printed when a callout installed on lease6_renew or the lease6_rebind hook point set the skip flag. For this particular hook point, the setting of the flag by a callout instructs the server to not extend the lifetime for a lease. If the client requested renewal of multiple diff --git a/src/lib/dhcpsrv/lease.h b/src/lib/dhcpsrv/lease.h index 0c536fb5a2..bea3715277 100644 --- a/src/lib/dhcpsrv/lease.h +++ b/src/lib/dhcpsrv/lease.h @@ -415,13 +415,6 @@ typedef boost::shared_ptr ConstLease6Ptr; /// @brief A collection of IPv6 leases. typedef std::vector Lease6Collection; -/// @brief A non-const iterator for IPv6 leases collection -typedef std::vector::iterator Lease6CollectionIter; - -/// @brief A const iterator for IPv6 leases collection -typedef std::vector::const_iterator Lease6CollectionConstIter; - - /// @brief Stream output operator. /// /// Dumps the output of Lease::toText to the given stream. diff --git a/src/lib/dhcpsrv/tests/alloc_engine_utils.h b/src/lib/dhcpsrv/tests/alloc_engine_utils.h index ba556a63fe..16de8c6a52 100644 --- a/src/lib/dhcpsrv/tests/alloc_engine_utils.h +++ b/src/lib/dhcpsrv/tests/alloc_engine_utils.h @@ -236,8 +236,9 @@ public: /// /// @param engine a reference to Allocation Engine /// @param pool pool from which the lease will be allocated from - /// @param hint address to be used as a hint - /// @param fake true - this is fake allocation (SOLICIT) + /// @param hints address to be used as a hint + /// @param allow_new_leases_in_renewal - specifies how to set the + /// allow_new_leases_in_renewal flag in ClientContext6 /// @param in_pool specifies whether the lease is expected to be in pool /// @return allocated lease(s) (may be empty) Lease6Collection renewTest(AllocEngine& engine, const Pool6Ptr& pool, -- 2.47.3