From: Tomek Mrugalski Date: Fri, 2 Mar 2018 15:06:13 +0000 (+0100) Subject: [5437] Changes after review X-Git-Tag: ha_checkpoints12~10^2~1 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=e7d1bd6af4be4d2a23555363c0bc5497c3e67954;p=thirdparty%2Fkea.git [5437] Changes after review - some comments added - unit-tests renamed - using C++11 syntax to simplify things. --- diff --git a/src/bin/dhcp4/dhcp4_srv.cc b/src/bin/dhcp4/dhcp4_srv.cc index d6f003603e..543649c9e6 100644 --- a/src/bin/dhcp4/dhcp4_srv.cc +++ b/src/bin/dhcp4/dhcp4_srv.cc @@ -1800,11 +1800,23 @@ Dhcpv4Srv::assignLease(Dhcpv4Exchange& ex) { Lease4Ptr lease; Subnet4Ptr original_subnet = subnet; + // We used to issue a separate query (two actually: one for client-id + // and another one for hw-addr for) each subnet in the shared network. + // That was horribly inefficient if the client didn't have any lease + // (or there were many subnets and the client happended to be in one + // of the last subnets). + // + // We now issue at most two queries: get all the leases for specific + // client-id and then get all leases for specific hw-address. if (client_id) { + + // Get all the leases for this client-id Lease4Collection leases_client_id = LeaseMgrFactory::instance().getLease4(*client_id); if (!leases_client_id.empty()) { Subnet4Ptr s = original_subnet; + // Among those returned try to find a lease that belongs to + // current shared network. while (s) { for (auto l = leases_client_id.begin(); l != leases_client_id.end(); ++l) { if ((*l)->subnet_id_ == s->getID()) { @@ -1823,11 +1835,16 @@ Dhcpv4Srv::assignLease(Dhcpv4Exchange& ex) { } } + // If we haven't found a lease yet, try again by hardware-address. + // The logic is the same. if (!lease && hwaddr) { + + // Get all leases for this particular hw-address. Lease4Collection leases_hwaddr = LeaseMgrFactory::instance().getLease4(*hwaddr); if (!leases_hwaddr.empty()) { Subnet4Ptr s = original_subnet; + // Pick one that belongs to a subnet in this shared network. while (s) { for (auto l = leases_hwaddr.begin(); l != leases_hwaddr.end(); ++l) { if ((*l)->subnet_id_ == s->getID()) { diff --git a/src/bin/dhcp4/tests/dora_unittest.cc b/src/bin/dhcp4/tests/dora_unittest.cc index 1ecd72944c..3df230d214 100644 --- a/src/bin/dhcp4/tests/dora_unittest.cc +++ b/src/bin/dhcp4/tests/dora_unittest.cc @@ -1569,7 +1569,7 @@ TEST_F(DORATest, reservationModeDisabled) { // cases in the real deployments, but this is just a test that the allocation // engine skips checking if the reservation exists when it allocates an // address. In the real deployment the reservation simply wouldn't exist. -TEST_F(DORATest, reservationModeDisabledAddressHijacking) { +TEST_F(DORATest, reservationIgnoredInDisabledMode) { // Client has a reservation. Dhcp4Client client(Dhcp4Client::SELECTING); // Set MAC address which doesn't match the reservation configured. @@ -1633,7 +1633,7 @@ TEST_F(DORATest, reservationModeOutOfPool) { // This test verifies that the in-pool reservation can be assigned to // the client not owning this reservation when the reservation mode is // set to "out-of-pool". -TEST_F(DORATest, reservationModeOutOfPoolAddressHijacking) { +TEST_F(DORATest, reservationIgnoredInOutOfPoolMode) { // Create the first client for which we have a reservation out of the // dynamic pool. Dhcp4Client client(Dhcp4Client::SELECTING); diff --git a/src/lib/dhcpsrv/alloc_engine.cc b/src/lib/dhcpsrv/alloc_engine.cc index a66a8c16f6..f25057f2bd 100644 --- a/src/lib/dhcpsrv/alloc_engine.cc +++ b/src/lib/dhcpsrv/alloc_engine.cc @@ -515,12 +515,10 @@ void AllocEngine::findReservation(ClientContext6& ctx) { (network->getAllSubnets()->size() > ctx.host_identifiers_.size()); if (use_single_query) { - for (auto id_pair = ctx.host_identifiers_.begin(); - id_pair != ctx.host_identifiers_.end(); - ++id_pair) { - ConstHostCollection hosts = HostMgr::instance().getAll(id_pair->first, - &id_pair->second[0], - id_pair->second.size()); + for (auto id_pair : ctx.host_identifiers_) { + ConstHostCollection hosts = HostMgr::instance().getAll(id_pair.first, + &id_pair.second[0], + id_pair.second.size()); // Store the hosts in the temporary map, because some hosts may // belong to subnets outside of the shared network. We'll need // to eliminate them. @@ -541,7 +539,7 @@ void AllocEngine::findReservation(ClientContext6& ctx) { (subnet->getHostReservationMode() != Network::HR_DISABLED)) { // Iterate over configured identifiers in the order of preference // and try to use each of them to search for the reservations. - BOOST_FOREACH(const IdentifierPair& id_pair, ctx.host_identifiers_) { + for (auto id_pair : ctx.host_identifiers_) { if (use_single_query) { if (host_map.count(subnet->getID()) > 0) { ctx.hosts_[subnet->getID()] = host_map[subnet->getID()]; @@ -593,9 +591,9 @@ AllocEngine::allocateLeases6(ClientContext6& ctx) { // our shared network. Lease6Collection leases; while (subnet) { - for (auto l = all_leases.begin(); l != all_leases.end(); ++l) { - if ((*l)->subnet_id_ == subnet->getID()) { - leases.push_back(*l); + for (auto l : all_leases) { + if ((l)->subnet_id_ == subnet->getID()) { + leases.push_back(l); } } @@ -3506,7 +3504,7 @@ AllocEngine::allocateUnreservedLease4(ClientContext4& ctx) { // the entire subnet (or more subnets) to discover that the address // pools have been exhausted. Using a subnet from which an address // was assigned most recently is an optimization which increases - // the likelyhood of starting from the subnet which address pools + // the likelihood of starting from the subnet which address pools // are not exhausted. SharedNetwork4Ptr network; ctx.subnet_->getSharedNetwork(network);