From: Francis Dupont Date: Fri, 15 Dec 2017 17:30:59 +0000 (+0100) Subject: [5443] Moved to extra argument vs deconst X-Git-Tag: trac5457_base~2^2~6 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=2aaedcdebe6db11186aab2b50b092c80c0b6d2d4;p=thirdparty%2Fkea.git [5443] Moved to extra argument vs deconst --- diff --git a/src/bin/dhcp4/dhcp4_srv.cc b/src/bin/dhcp4/dhcp4_srv.cc index 9a12a33b6a..84f1db40bf 100644 --- a/src/bin/dhcp4/dhcp4_srv.cc +++ b/src/bin/dhcp4/dhcp4_srv.cc @@ -483,11 +483,11 @@ Dhcpv4Srv::shutdown() { } isc::dhcp::Subnet4Ptr -Dhcpv4Srv::selectSubnet(Pkt4Ptr& query) { +Dhcpv4Srv::selectSubnet(const Pkt4Ptr& query, bool& drop) const { // DHCPv4-over-DHCPv6 is a special (and complex) case if (query->isDhcp4o6()) { - return (selectSubnet4o6(query)); + return (selectSubnet4o6(query, drop)); } Subnet4Ptr subnet; @@ -570,13 +570,12 @@ Dhcpv4Srv::selectSubnet(Pkt4Ptr& query) { } // Callouts decided to drop the packet. It is a superset of the - // skip case so no subnet will be selected. For the caller to - // know it has to drop the packet the query pointer is cleared. + // skip case so no subnet will be selected. if (callout_handle->getStatus() == CalloutHandle::NEXT_STEP_DROP) { LOG_DEBUG(hooks_logger, DBG_DHCP4_HOOKS, DHCP4_HOOK_SUBNET4_SELECT_DROP) .arg(query->getLabel()); - query.reset(); + drop = true; return (Subnet4Ptr()); } @@ -605,7 +604,7 @@ Dhcpv4Srv::selectSubnet(Pkt4Ptr& query) { } isc::dhcp::Subnet4Ptr -Dhcpv4Srv::selectSubnet4o6(Pkt4Ptr& query) { +Dhcpv4Srv::selectSubnet4o6(const Pkt4Ptr& query, bool& drop) const { Subnet4Ptr subnet; @@ -683,13 +682,12 @@ Dhcpv4Srv::selectSubnet4o6(Pkt4Ptr& query) { } // Callouts decided to drop the packet. It is a superset of the - // skip case so no subnet will be selected. For the caller to - // know it has to drop the packet the query pointer is cleared. + // skip case so no subnet will be selected. if (callout_handle->getStatus() == CalloutHandle::NEXT_STEP_DROP) { LOG_DEBUG(hooks_logger, DBG_DHCP4_HOOKS, DHCP4_HOOK_SUBNET4_SELECT_DROP) .arg(query->getLabel()); - query.reset(); + drop = true; return (Subnet4Ptr()); } @@ -2341,10 +2339,11 @@ Pkt4Ptr Dhcpv4Srv::processDiscover(Pkt4Ptr& discover) { sanityCheck(discover, FORBIDDEN); - Dhcpv4Exchange ex(alloc_engine_, discover, selectSubnet(discover)); + bool drop = false; + Dhcpv4Exchange ex(alloc_engine_, discover, selectSubnet(discover, drop)); // Stop here if selectSubnet decided to drop the packet - if (!discover) { + if (drop) { return (Pkt4Ptr()); } @@ -2398,10 +2397,11 @@ Dhcpv4Srv::processRequest(Pkt4Ptr& request) { /// @todo Uncomment this (see ticket #3116) /// sanityCheck(request, MANDATORY); - Dhcpv4Exchange ex(alloc_engine_, request, selectSubnet(request)); + bool drop = false; + Dhcpv4Exchange ex(alloc_engine_, request, selectSubnet(request, drop)); // Stop here if selectSubnet decided to drop the packet - if (!request) { + if (drop) { return (Pkt4Ptr()); } @@ -2704,10 +2704,11 @@ Dhcpv4Srv::processInform(Pkt4Ptr& inform) { // DHCPINFORM MUST not include server identifier. sanityCheck(inform, FORBIDDEN); - Dhcpv4Exchange ex(alloc_engine_, inform, selectSubnet(inform)); + bool drop = false; + Dhcpv4Exchange ex(alloc_engine_, inform, selectSubnet(inform, drop)); // Stop here if selectSubnet decided to drop the packet - if (!inform) { + if (drop) { return (Pkt4Ptr()); } @@ -2747,7 +2748,7 @@ Dhcpv4Srv::processInform(Pkt4Ptr& inform) { } bool -Dhcpv4Srv::accept(Pkt4Ptr& query) { +Dhcpv4Srv::accept(const Pkt4Ptr& query) const { // Check that the message type is accepted by the server. We rely on the // function called to log a message if needed. if (!acceptMessageType(query)) { @@ -2775,7 +2776,7 @@ Dhcpv4Srv::accept(Pkt4Ptr& query) { } bool -Dhcpv4Srv::acceptDirectRequest(Pkt4Ptr& pkt) { +Dhcpv4Srv::acceptDirectRequest(const Pkt4Ptr& pkt) const { // Accept all relayed messages. if (pkt->isRelayed()) { return (true); @@ -2802,8 +2803,9 @@ Dhcpv4Srv::acceptDirectRequest(Pkt4Ptr& pkt) { // we validate the message type prior to calling this function. return (false); } - bool result = (!pkt->getLocalAddr().isV4Bcast() || selectSubnet(pkt)); - if (!pkt) { + bool drop = false; + bool result = (!pkt->getLocalAddr().isV4Bcast() || selectSubnet(pkt, drop)); + if (drop) { // The packet must be dropped. return (false); } diff --git a/src/bin/dhcp4/dhcp4_srv.h b/src/bin/dhcp4/dhcp4_srv.h index d601ddddbe..f6b673cf0f 100644 --- a/src/bin/dhcp4/dhcp4_srv.h +++ b/src/bin/dhcp4/dhcp4_srv.h @@ -348,7 +348,7 @@ protected: /// /// @return true if the message should be further processed, or false if /// the message should be discarded. - bool accept(Pkt4Ptr& query); + bool accept(const Pkt4Ptr& query) const; /// @brief Check if a message sent by directly connected client should be /// accepted or discarded. @@ -377,7 +377,7 @@ protected: /// /// @return true if message is accepted for further processing, false /// otherwise. - bool acceptDirectRequest(Pkt4Ptr& query); + bool acceptDirectRequest(const Pkt4Ptr& query) const; /// @brief Check if received message type is valid for the server to /// process. @@ -766,19 +766,19 @@ protected: /// @brief Selects a subnet for a given client's packet. /// - /// When the packet has to be dropped the query pointer is cleared - /// /// @param query client's message + /// @param drop if it is true the packet will be dropped /// @return selected subnet (or NULL if no suitable subnet was found) - isc::dhcp::Subnet4Ptr selectSubnet(Pkt4Ptr& query); + isc::dhcp::Subnet4Ptr selectSubnet(const Pkt4Ptr& query, + bool& drop) const; /// @brief Selects a subnet for a given client's DHCP4o6 packet. /// - /// When the packet has to be dropped the query pointer is cleared - /// /// @param query client's message + /// @param drop if it is true the packet will be dropped /// @return selected subnet (or NULL if no suitable subnet was found) - isc::dhcp::Subnet4Ptr selectSubnet4o6(Pkt4Ptr& query); + isc::dhcp::Subnet4Ptr selectSubnet4o6(const Pkt4Ptr& query, + bool& drop) const; /// indicates if shutdown is in progress. Setting it to true will /// initiate server shutdown procedure. diff --git a/src/bin/dhcp4/tests/dhcp4_srv_unittest.cc b/src/bin/dhcp4/tests/dhcp4_srv_unittest.cc index df1ae36fb9..bd9238f7fc 100644 --- a/src/bin/dhcp4/tests/dhcp4_srv_unittest.cc +++ b/src/bin/dhcp4/tests/dhcp4_srv_unittest.cc @@ -2303,19 +2303,23 @@ TEST_F(Dhcpv4SrvTest, clientClassify) { // This discover does not belong to foo class, so it will not // be serviced - EXPECT_FALSE(srv_.selectSubnet(dis)); + bool drop = false; + EXPECT_FALSE(srv_.selectSubnet(dis, drop)); + EXPECT_FALSE(drop); // Let's add the packet to bar class and try again. dis->addClass("bar"); // Still not supported, because it belongs to wrong class. - EXPECT_FALSE(srv_.selectSubnet(dis)); + EXPECT_FALSE(srv_.selectSubnet(dis, drop)); + EXPECT_FALSE(drop); // Let's add it to matching class. dis->addClass("foo"); // This time it should work - EXPECT_TRUE(srv_.selectSubnet(dis)); + EXPECT_TRUE(srv_.selectSubnet(dis, drop)); + EXPECT_FALSE(drop); } // Verifies last resort option 43 is backward compatible @@ -3493,31 +3497,38 @@ TEST_F(Dhcpv4SrvTest, relayOverride) { // This is just a sanity check, we're using regular method: ciaddr 192.0.2.1 // belongs to the first subnet, so it is selected dis->setGiaddr(IOAddress("192.0.2.1")); - EXPECT_TRUE(subnet1 == srv_.selectSubnet(dis)); + bool drop = false; + EXPECT_TRUE(subnet1 == srv_.selectSubnet(dis, drop)); + EXPECT_FALSE(drop); // Relay belongs to the second subnet, so it should be selected. dis->setGiaddr(IOAddress("192.0.3.1")); - EXPECT_TRUE(subnet2 == srv_.selectSubnet(dis)); + EXPECT_TRUE(subnet2 == srv_.selectSubnet(dis, drop)); + EXPECT_FALSE(drop); // Now let's check if the relay override for the first subnets works dis->setGiaddr(IOAddress("192.0.5.1")); - EXPECT_TRUE(subnet1 == srv_.selectSubnet(dis)); + EXPECT_TRUE(subnet1 == srv_.selectSubnet(dis, drop)); + EXPECT_FALSE(drop); // The same check for the second subnet... dis->setGiaddr(IOAddress("192.0.5.2")); - EXPECT_TRUE(subnet2 == srv_.selectSubnet(dis)); + EXPECT_TRUE(subnet2 == srv_.selectSubnet(dis, drop)); + EXPECT_FALSE(drop); // And finally, let's check if mis-matched relay address will end up // in not selecting a subnet at all dis->setGiaddr(IOAddress("192.0.5.3")); - EXPECT_FALSE(srv_.selectSubnet(dis)); + EXPECT_FALSE(srv_.selectSubnet(dis, drop)); + EXPECT_FALSE(drop); // Finally, check that the relay override works only with relay address // (GIADDR) and does not affect client address (CIADDR) dis->setGiaddr(IOAddress("0.0.0.0")); dis->setHops(0); dis->setCiaddr(IOAddress("192.0.5.1")); - EXPECT_FALSE(srv_.selectSubnet(dis)); + EXPECT_FALSE(srv_.selectSubnet(dis, drop)); + EXPECT_FALSE(drop); } // Checks if relay IP address specified in the relay-info structure can be @@ -3573,12 +3584,15 @@ TEST_F(Dhcpv4SrvTest, relayOverrideAndClientClass) { // subnet[0], even though the relay-ip matches. It should be accepted in // subnet[1], because the subnet matches and there are no class // requirements. - EXPECT_TRUE(subnet2 == srv_.selectSubnet(dis)); + bool drop = false; + EXPECT_TRUE(subnet2 == srv_.selectSubnet(dis, drop)); + EXPECT_FALSE(drop); // Now let's add this packet to class foo and recheck. This time it should // be accepted in the first subnet, because both class and relay-ip match. dis->addClass("foo"); - EXPECT_TRUE(subnet1 == srv_.selectSubnet(dis)); + EXPECT_TRUE(subnet1 == srv_.selectSubnet(dis, drop)); + EXPECT_FALSE(drop); } // Checks if a RAI link selection sub-option works as expected @@ -3644,16 +3658,20 @@ TEST_F(Dhcpv4SrvTest, relayLinkSelect) { // This is just a sanity check, we're using regular method: ciaddr 192.0.3.1 // belongs to the second subnet, so it is selected dis->setGiaddr(IOAddress("192.0.3.1")); - EXPECT_TRUE(subnet2 == srv_.selectSubnet(dis)); + bool drop = false; + EXPECT_TRUE(subnet2 == srv_.selectSubnet(dis, drop)); + EXPECT_FALSE(drop); // Setup a relay override for the first subnet as it has a high precedence dis->setGiaddr(IOAddress("192.0.5.1")); - EXPECT_TRUE(subnet1 == srv_.selectSubnet(dis)); + EXPECT_TRUE(subnet1 == srv_.selectSubnet(dis, drop)); + EXPECT_FALSE(drop); // Put a RAI to select back the second subnet as it has // the highest precedence dis->addOption(rai); - EXPECT_TRUE(subnet2 == srv_.selectSubnet(dis)); + EXPECT_TRUE(subnet2 == srv_.selectSubnet(dis, drop)); + EXPECT_FALSE(drop); // Subnet select option has a lower precedence OptionDefinitionPtr sbnsel_def = LibDHCP::getOptionDef(DHCP4_OPTION_SPACE, @@ -3663,7 +3681,8 @@ TEST_F(Dhcpv4SrvTest, relayLinkSelect) { ASSERT_TRUE(sbnsel); sbnsel->writeAddress(IOAddress("192.0.2.3")); dis->addOption(sbnsel); - EXPECT_TRUE(subnet2 == srv_.selectSubnet(dis)); + EXPECT_TRUE(subnet2 == srv_.selectSubnet(dis, drop)); + EXPECT_FALSE(drop); dis->delOption(DHO_SUBNET_SELECTION); // Check client-classification still applies @@ -3675,10 +3694,12 @@ TEST_F(Dhcpv4SrvTest, relayLinkSelect) { rai->addOption(ols); dis->addOption(rai); // Note it shall fail (vs. try the next criterion). - EXPECT_FALSE(srv_.selectSubnet(dis)); + EXPECT_FALSE(srv_.selectSubnet(dis, drop)); + EXPECT_FALSE(drop); // Add the packet to the class and check again: now it shall succeed dis->addClass("foo"); - EXPECT_TRUE(subnet3 == srv_.selectSubnet(dis)); + EXPECT_TRUE(subnet3 == srv_.selectSubnet(dis, drop)); + EXPECT_FALSE(drop); // Check it fails with a bad address in the sub-option IOAddress addr_bad("10.0.0.1"); @@ -3688,7 +3709,8 @@ TEST_F(Dhcpv4SrvTest, relayLinkSelect) { dis->delOption(DHO_DHCP_AGENT_OPTIONS); rai->addOption(ols); dis->addOption(rai); - EXPECT_FALSE(srv_.selectSubnet(dis)); + EXPECT_FALSE(srv_.selectSubnet(dis, drop)); + EXPECT_FALSE(drop); } // Checks if a subnet selection option works as expected @@ -3749,28 +3771,35 @@ TEST_F(Dhcpv4SrvTest, subnetSelect) { // This is just a sanity check, we're using regular method: ciaddr 192.0.3.1 // belongs to the second subnet, so it is selected dis->setGiaddr(IOAddress("192.0.3.1")); - EXPECT_TRUE(subnet2 == srv_.selectSubnet(dis)); + bool drop = false; + EXPECT_TRUE(subnet2 == srv_.selectSubnet(dis, drop)); + EXPECT_FALSE(drop); // Setup a relay override for the first subnet as it has a high precedence dis->setGiaddr(IOAddress("192.0.5.1")); - EXPECT_TRUE(subnet1 == srv_.selectSubnet(dis)); + EXPECT_TRUE(subnet1 == srv_.selectSubnet(dis, drop)); + EXPECT_FALSE(drop); // Put a subnet select option to select back the second subnet as // it has the second highest precedence dis->addOption(sbnsel); - EXPECT_TRUE(subnet2 == srv_.selectSubnet(dis)); + EXPECT_TRUE(subnet2 == srv_.selectSubnet(dis, drop)); + EXPECT_FALSE(drop); // Check client-classification still applies sbnsel->writeAddress(IOAddress("192.0.4.2")); // Note it shall fail (vs. try the next criterion). - EXPECT_FALSE(srv_.selectSubnet(dis)); + EXPECT_FALSE(srv_.selectSubnet(dis, drop)); + EXPECT_FALSE(drop); // Add the packet to the class and check again: now it shall succeed dis->addClass("foo"); - EXPECT_TRUE(subnet3 == srv_.selectSubnet(dis)); + EXPECT_TRUE(subnet3 == srv_.selectSubnet(dis, drop)); + EXPECT_FALSE(drop); // Check it fails with a bad address in the sub-option sbnsel->writeAddress(IOAddress("10.0.0.1")); - EXPECT_FALSE(srv_.selectSubnet(dis)); + EXPECT_FALSE(srv_.selectSubnet(dis, drop)); + EXPECT_FALSE(drop); } // This test verifies that the direct message is dropped when it has been diff --git a/src/bin/dhcp4/tests/dhcp4_test_utils.cc b/src/bin/dhcp4/tests/dhcp4_test_utils.cc index bd02d5f6fd..75c31f3f67 100644 --- a/src/bin/dhcp4/tests/dhcp4_test_utils.cc +++ b/src/bin/dhcp4/tests/dhcp4_test_utils.cc @@ -631,8 +631,12 @@ Dhcpv4SrvTest::configure(const std::string& config, NakedDhcpv4Srv& srv, } Dhcpv4Exchange -Dhcpv4SrvTest::createExchange(Pkt4Ptr& query) { - return (Dhcpv4Exchange(srv_.alloc_engine_, query, srv_.selectSubnet(query))); +Dhcpv4SrvTest::createExchange(const Pkt4Ptr& query) { + bool drop = false; + Dhcpv4Exchange ex(srv_.alloc_engine_, query, + srv_.selectSubnet(query, drop)); + EXPECT_FALSE(drop); + return (ex); } void diff --git a/src/bin/dhcp4/tests/dhcp4_test_utils.h b/src/bin/dhcp4/tests/dhcp4_test_utils.h index dd4c1a795b..48e571a447 100644 --- a/src/bin/dhcp4/tests/dhcp4_test_utils.h +++ b/src/bin/dhcp4/tests/dhcp4_test_utils.h @@ -434,7 +434,7 @@ public: uint8_t pkt_type, const std::string& stat_name); /// @brief Create @c Dhcpv4Exchange from client's query. - Dhcpv4Exchange createExchange(Pkt4Ptr& query); + Dhcpv4Exchange createExchange(const Pkt4Ptr& query); /// @brief Performs 4-way exchange to obtain new lease. /// diff --git a/src/bin/dhcp4/tests/fqdn_unittest.cc b/src/bin/dhcp4/tests/fqdn_unittest.cc index 2afa40955d..264ec433b7 100644 --- a/src/bin/dhcp4/tests/fqdn_unittest.cc +++ b/src/bin/dhcp4/tests/fqdn_unittest.cc @@ -396,7 +396,7 @@ public: // Test that server generates the appropriate FQDN option in response to // client's FQDN option. - void testProcessFqdn(Pkt4Ptr& query, const uint8_t exp_flags, + void testProcessFqdn(const Pkt4Ptr& query, const uint8_t exp_flags, const std::string& exp_domain_name, Option4ClientFqdn::DomainNameType exp_domain_type = Option4ClientFqdn::FULL) { @@ -524,7 +524,7 @@ public: /// packet must contain the hostname option /// /// @return a pointer to the hostname option constructed by the server - OptionStringPtr processHostname(Pkt4Ptr& query, + OptionStringPtr processHostname(const Pkt4Ptr& query, bool must_have_host = true) { if (!getHostnameOption(query) && must_have_host) { ADD_FAILURE() << "Hostname option not carried in the query";