From deed9e6f1dec820a502b247b59ba05b35df112c8 Mon Sep 17 00:00:00 2001 From: Francis Dupont Date: Tue, 19 Mar 2019 19:24:09 +0100 Subject: [PATCH] [474-optionally-disable-collection-host-lookups] Addressed comments --- src/lib/dhcpsrv/alloc_engine.cc | 8 +++--- src/lib/dhcpsrv/host_mgr.h | 25 ++++++++++--------- .../dhcpsrv/tests/alloc_engine4_unittest.cc | 17 +++++++++---- .../dhcpsrv/tests/alloc_engine6_unittest.cc | 16 +++++++++--- 4 files changed, 41 insertions(+), 25 deletions(-) diff --git a/src/lib/dhcpsrv/alloc_engine.cc b/src/lib/dhcpsrv/alloc_engine.cc index a39f2ee592..df3d1b085f 100644 --- a/src/lib/dhcpsrv/alloc_engine.cc +++ b/src/lib/dhcpsrv/alloc_engine.cc @@ -544,10 +544,10 @@ void AllocEngine::findReservation(ClientContext6& ctx) { // client rather than a query for each subnet within this shared network. // The only case when it is going to be less efficient is when there are // more host identifier types in use than subnets within a shared network. - // As it breaks RADIUS and host caching this can be disabled by the + // As it breaks RADIUS use of host caching this can be disabled by the // host manager. const bool use_single_query = network && - !HostMgr::instance().getPreventCollection() && + !HostMgr::instance().getDisableSingleQuery() && (network->getAllSubnets()->size() > ctx.host_identifiers_.size()); if (use_single_query) { @@ -3073,10 +3073,10 @@ AllocEngine::findReservation(ClientContext4& ctx) { // client rather than a query for each subnet within this shared network. // The only case when it is going to be less efficient is when there are // more host identifier types in use than subnets within a shared network. - // As it breaks RADIUS and host caching this can be disabled by the + // As it breaks RADIUS use of host caching this can be disabled by the // host manager. const bool use_single_query = network && - !HostMgr::instance().getPreventCollection() && + !HostMgr::instance().getDisableSingleQuery() && (network->getAllSubnets()->size() > ctx.host_identifiers_.size()); if (use_single_query) { diff --git a/src/lib/dhcpsrv/host_mgr.h b/src/lib/dhcpsrv/host_mgr.h index 501a1f5c07..caedff49b6 100644 --- a/src/lib/dhcpsrv/host_mgr.h +++ b/src/lib/dhcpsrv/host_mgr.h @@ -427,17 +427,17 @@ public: negative_caching_ = negative_caching; } - /// @brief Returns the prevent collection flag. + /// @brief Returns the disable single query flag. /// - /// @return the prevent collection flag. - bool getPreventCollection() const { - return (prevent_collection_); + /// @return the disable single query flag. + bool getDisableSingleQuery() const { + return (disable_single_query_); } - /// @brief Sets the prevent collection flag. + /// @brief Sets the disable single query flag. /// - void setPreventCollection(bool prevent_collection) { - prevent_collection_ = prevent_collection; + void setDisableSingleQuery(bool disable_single_query) { + disable_single_query_ = disable_single_query; } protected: @@ -448,11 +448,12 @@ protected: /// This works for get[46] for a subnet and an identifier. bool negative_caching_; - /// @brief The prevent collection flag. + /// @brief The disable single query flag. /// - /// When true prevent the use of lookup methods returning a - /// collection when methods returning a single object are usable instead. - bool prevent_collection_; + /// When true prevent the use of lookup methods returning a collection + /// aka single queries when methods returning a host object are usable + /// instead. + bool disable_single_query_; /// @brief Cache an answer. /// @@ -475,7 +476,7 @@ protected: private: /// @brief Private default constructor. - HostMgr() : negative_caching_(false), prevent_collection_(false) { } + HostMgr() : negative_caching_(false), disable_single_query_(false) { } /// @brief List of alternate host data sources. HostDataSourceList alternate_sources_; diff --git a/src/lib/dhcpsrv/tests/alloc_engine4_unittest.cc b/src/lib/dhcpsrv/tests/alloc_engine4_unittest.cc index 4cd5212655..da2d620ad4 100644 --- a/src/lib/dhcpsrv/tests/alloc_engine4_unittest.cc +++ b/src/lib/dhcpsrv/tests/alloc_engine4_unittest.cc @@ -808,6 +808,8 @@ TEST_F(SharedNetworkAlloc4Test, discoverSharedNetworkPoolClassification) { // reservations belong. TEST_F(SharedNetworkAlloc4Test, discoverSharedNetworkReservations) { + EXPECT_FALSE(HostMgr::instance().getDisableSingleQuery()); + // Create reservation for the client. HostPtr host(new Host(&hwaddr_->hwaddr_[0], hwaddr_->hwaddr_.size(), Host::IDENT_HWADDR, subnet2_->getID(), @@ -843,11 +845,13 @@ TEST_F(SharedNetworkAlloc4Test, discoverSharedNetworkReservations) { // Test that reservations within shared network take precedence over the // existing leases regardless in which subnet belonging to a shared network // reservations belong. Host lookups returning a collection are disabled. +// As it is only an optimization the behavior (so the test) must stay +// unchanged. TEST_F(SharedNetworkAlloc4Test, discoverSharedNetworkReservationsNoColl) { // Disable host lookups returning a collection. - ASSERT_FALSE(HostMgr::instance().getPreventCollection()); - HostMgr::instance().setPreventCollection(true); + ASSERT_FALSE(HostMgr::instance().getDisableSingleQuery()); + HostMgr::instance().setDisableSingleQuery(true); // Create reservation for the client. HostPtr host(new Host(&hwaddr_->hwaddr_[0], hwaddr_->hwaddr_.size(), @@ -1105,6 +1109,8 @@ TEST_F(SharedNetworkAlloc4Test, requestSharedNetworkPoolClassification) { // reservations belong (DHCPREQUEST case). TEST_F(SharedNetworkAlloc4Test, requestSharedNetworkReservations) { + EXPECT_FALSE(HostMgr::instance().getDisableSingleQuery()); + // Create reservation for the client. HostPtr host(new Host(&hwaddr_->hwaddr_[0], hwaddr_->hwaddr_.size(), Host::IDENT_HWADDR, subnet2_->getID(), @@ -1143,12 +1149,13 @@ TEST_F(SharedNetworkAlloc4Test, requestSharedNetworkReservations) { // Test that reservations within shared network take precedence over the // existing leases regardless in which subnet belonging to a shared network // reservations belong (DHCPREQUEST case). Host lookups returning a collection -// are disabled. +// are disabled. As it is only an optimization the behavior (so the test) +// must stay unchanged. TEST_F(SharedNetworkAlloc4Test, requestSharedNetworkReservationsNoColl) { // Disable host lookups returning a collection. - ASSERT_FALSE(HostMgr::instance().getPreventCollection()); - HostMgr::instance().setPreventCollection(true); + ASSERT_FALSE(HostMgr::instance().getDisableSingleQuery()); + HostMgr::instance().setDisableSingleQuery(true); // Create reservation for the client. HostPtr host(new Host(&hwaddr_->hwaddr_[0], hwaddr_->hwaddr_.size(), diff --git a/src/lib/dhcpsrv/tests/alloc_engine6_unittest.cc b/src/lib/dhcpsrv/tests/alloc_engine6_unittest.cc index 011fb3da89..698a5a3700 100644 --- a/src/lib/dhcpsrv/tests/alloc_engine6_unittest.cc +++ b/src/lib/dhcpsrv/tests/alloc_engine6_unittest.cc @@ -2581,6 +2581,8 @@ TEST_F(SharedNetworkAlloc6Test, solicitSharedNetworkPoolClassification) { // even if this address belongs to another subnet within the same // shared network. TEST_F(SharedNetworkAlloc6Test, solicitSharedNetworkReservations) { + EXPECT_FALSE(HostMgr::instance().getDisableSingleQuery()); + // Create reservation for the client in the second subnet. subnet_ = subnet2_; createHost6(true, IPv6Resrv::TYPE_NA, IOAddress("2001:db8:2::15"), 128); @@ -2605,10 +2607,12 @@ TEST_F(SharedNetworkAlloc6Test, solicitSharedNetworkReservations) { // This test verifies that the client is offerred a reserved address // even if this address belongs to another subnet within the same // shared network. Host lookups returning a collection are disabled. +// As it is only an optimization the behavior (so the test) must stay +// unchanged. TEST_F(SharedNetworkAlloc6Test, solicitSharedNetworkReservationsNoColl) { // Disable host lookups returning a collection. - ASSERT_FALSE(HostMgr::instance().getPreventCollection()); - HostMgr::instance().setPreventCollection(true); + ASSERT_FALSE(HostMgr::instance().getDisableSingleQuery()); + HostMgr::instance().setDisableSingleQuery(true); // Create reservation for the client in the second subnet. subnet_ = subnet2_; @@ -2635,6 +2639,8 @@ TEST_F(SharedNetworkAlloc6Test, solicitSharedNetworkReservationsNoColl) { // even if this address belongs to another subnet within the same // shared network. TEST_F(SharedNetworkAlloc6Test, requestSharedNetworkReservations) { + EXPECT_FALSE(HostMgr::instance().getDisableSingleQuery()); + // Create reservation for the client in the second subnet. subnet_ = subnet2_; createHost6(true, IPv6Resrv::TYPE_NA, IOAddress("2001:db8:2::15"), 128); @@ -2659,10 +2665,12 @@ TEST_F(SharedNetworkAlloc6Test, requestSharedNetworkReservations) { // This test verifies that the client is allocated a reserved address // even if this address belongs to another subnet within the same // shared network. Host lookups returning a collection are disabled. +// As it is only an optimization the behavior (so the test) must stay +// unchanged. TEST_F(SharedNetworkAlloc6Test, requestSharedNetworkReservationsNoColl) { // Disable host lookups returning a collection. - ASSERT_FALSE(HostMgr::instance().getPreventCollection()); - HostMgr::instance().setPreventCollection(true); + ASSERT_FALSE(HostMgr::instance().getDisableSingleQuery()); + HostMgr::instance().setDisableSingleQuery(true); // Create reservation for the client in the second subnet. subnet_ = subnet2_; -- 2.47.2