From: Francis Dupont Date: Mon, 26 Dec 2022 18:22:02 +0000 (+0100) Subject: [#2694] Done X-Git-Tag: Kea-2.3.4~81 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=6e08dfb799eff12b28144e783fa15e71bb470cae;p=thirdparty%2Fkea.git [#2694] Done --- diff --git a/src/lib/dhcp/pkt.cc b/src/lib/dhcp/pkt.cc index 8a63cda0c1..fdd1e8977e 100644 --- a/src/lib/dhcp/pkt.cc +++ b/src/lib/dhcp/pkt.cc @@ -64,6 +64,33 @@ Pkt::getOption(const uint16_t type) { return (OptionPtr()); // NULL } +OptionCollection +Pkt::getNonCopiedOptions(const uint16_t opt_type) const { + std::pair range = options_.equal_range(opt_type); + return (OptionCollection(range.first, range.second)); +} + +OptionCollection +Pkt::getOptions(const uint16_t opt_type) { + OptionCollection options_copy; + + std::pair range = options_.equal_range(opt_type); + // If options should be copied on retrieval, we should now iterate over + // matching options, copy them and replace the original ones with new + // instances. + if (copy_retrieved_options_) { + for (OptionCollection::iterator opt_it = range.first; + opt_it != range.second; ++opt_it) { + OptionPtr option_copy = opt_it->second->clone(); + opt_it->second = option_copy; + } + } + // Finally, return updated options. This can also be empty in some cases. + return (OptionCollection(range.first, range.second)); +} + bool Pkt::delOption(uint16_t type) { const auto& x = options_.find(type); diff --git a/src/lib/dhcp/pkt.h b/src/lib/dhcp/pkt.h index 77e9a2b0f1..441fd39598 100644 --- a/src/lib/dhcp/pkt.h +++ b/src/lib/dhcp/pkt.h @@ -348,13 +348,26 @@ protected: /// if such option is not present. OptionPtr getNonCopiedOption(const uint16_t type) const; + /// @brief Returns all option instances of specified type without + /// copying. + /// + /// This is a variant of @ref getOptions method, which returns a collection + /// of options without copying them. This method should be only used by + /// the @ref Pkt6 class and derived classes. Any external callers should + /// use @ref getOptions which copies option instances before returning them + /// when the @ref Pkt::copy_retrieved_options_ flag is set to true. + /// + /// @param opt_type Option code. + /// + /// @return Collection of options found. + OptionCollection getNonCopiedOptions(const uint16_t opt_type) const; + public: /// @brief Returns the first option of specified type. /// /// Returns the first option of specified type. Note that in DHCPv6 several /// instances of the same option are allowed (and frequently used). - /// Also see @ref Pkt6::getOptions(). /// /// The options will be only returned after unpack() is called. /// @@ -363,6 +376,15 @@ public: /// @return pointer to found option (or NULL) OptionPtr getOption(const uint16_t type); + /// @brief Returns all instances of specified type. + /// + /// Returns all instances of options of the specified type. DHCPv6 protocol + /// allows (and uses frequently) multiple instances. + /// + /// @param type option type we are looking for + /// @return instance of option collection with requested options + isc::dhcp::OptionCollection getOptions(const uint16_t type); + /// @brief Controls whether the option retrieved by the @ref Pkt::getOption /// should be copied before being returned. /// diff --git a/src/lib/dhcp/pkt4.h b/src/lib/dhcp/pkt4.h index 398f05d135..dd36168033 100644 --- a/src/lib/dhcp/pkt4.h +++ b/src/lib/dhcp/pkt4.h @@ -325,6 +325,9 @@ public: /// @brief Add an option. /// + /// @note: to avoid throwing when adding multiple options + /// with the same type use @ref Pkt::addOption. + /// /// @throw BadValue if option with that type is already present. /// /// @param opt option to be added diff --git a/src/lib/dhcp/pkt6.cc b/src/lib/dhcp/pkt6.cc index 0154873b9f..b02bf39e11 100644 --- a/src/lib/dhcp/pkt6.cc +++ b/src/lib/dhcp/pkt6.cc @@ -102,7 +102,6 @@ Pkt6::prepareGetAnyRelayOption(const RelaySearchOrder& order, } } - OptionPtr Pkt6::getNonCopiedAnyRelayOption(const uint16_t option_code, const RelaySearchOrder& order) const { @@ -777,33 +776,6 @@ Pkt6::getClientId() const { return (DuidPtr()); } -isc::dhcp::OptionCollection -Pkt6::getNonCopiedOptions(const uint16_t opt_type) const { - std::pair range = options_.equal_range(opt_type); - return (OptionCollection(range.first, range.second)); -} - -isc::dhcp::OptionCollection -Pkt6::getOptions(const uint16_t opt_type) { - OptionCollection options_copy; - - std::pair range = options_.equal_range(opt_type); - // If options should be copied on retrieval, we should now iterate over - // matching options, copy them and replace the original ones with new - // instances. - if (copy_retrieved_options_) { - for (OptionCollection::iterator opt_it = range.first; - opt_it != range.second; ++opt_it) { - OptionPtr option_copy = opt_it->second->clone(); - opt_it->second = option_copy; - } - } - // Finally, return updated options. This can also be empty in some cases. - return (OptionCollection(range.first, range.second)); -} - const char* Pkt6::getName(const uint8_t type) { static const char* ADVERTISE = "ADVERTISE"; diff --git a/src/lib/dhcp/pkt6.h b/src/lib/dhcp/pkt6.h index e0b68239f2..1ae23be3de 100644 --- a/src/lib/dhcp/pkt6.h +++ b/src/lib/dhcp/pkt6.h @@ -418,33 +418,6 @@ public: const isc::asiolink::IOAddress& getRelay6PeerAddress(uint8_t relay_level) const; -protected: - - /// @brief Returns all option instances of specified type without - /// copying. - /// - /// This is a variant of @ref getOptions method, which returns a collection - /// of options without copying them. This method should be only used by - /// the @ref Pkt6 class and derived classes. Any external callers should - /// use @ref getOptions which copies option instances before returning them - /// when the @ref Pkt::copy_retrieved_options_ flag is set to true. - /// - /// @param opt_type Option code. - /// - /// @return Collection of options found. - OptionCollection getNonCopiedOptions(const uint16_t opt_type) const; - -public: - - /// @brief Returns all instances of specified type. - /// - /// Returns all instances of options of the specified type. DHCPv6 protocol - /// allows (and uses frequently) multiple instances. - /// - /// @param type option type we are looking for - /// @return instance of option collection with requested options - isc::dhcp::OptionCollection getOptions(const uint16_t type); - /// @brief add information about one traversed relay /// /// This adds information about one traversed relay, i.e. diff --git a/src/lib/dhcp/tests/pkt4_unittest.cc b/src/lib/dhcp/tests/pkt4_unittest.cc index edbb65bd7c..efb1617ecf 100644 --- a/src/lib/dhcp/tests/pkt4_unittest.cc +++ b/src/lib/dhcp/tests/pkt4_unittest.cc @@ -609,6 +609,92 @@ TEST_F(Pkt4Test, options) { EXPECT_NO_THROW(pkt.reset()); } +// Check that multiple options of the same type may be retrieved by +// using getOptions, Also check that retrieved options are copied when +// setCopyRetrievedOptions is enabled. +TEST_F(Pkt4Test, getOptions) { + scoped_ptr pkt(new Pkt4(DHCPOFFER, 0)); + OptionPtr opt1(new Option(Option::V4, 1)); + OptionPtr opt2(new Option(Option::V4, 1)); + OptionPtr opt3(new Option(Option::V4, 2)); + OptionPtr opt4(new Option(Option::V4, 2)); + + pkt->addOption(opt1); + pkt->Pkt::addOption(opt2); + pkt->Pkt::addOption(opt3); + pkt->Pkt::addOption(opt4); + + // Retrieve options with option code 1. + OptionCollection options = pkt->getOptions(1); + ASSERT_EQ(2, options.size()); + + OptionCollection::const_iterator opt_it; + + // Make sure that the first option is returned. We're using the pointer + // to opt1 to find the option. + opt_it = std::find(options.begin(), options.end(), + std::pair(1, opt1)); + EXPECT_TRUE(opt_it != options.end()); + + // Make sure that the second option is returned. + opt_it = std::find(options.begin(), options.end(), + std::pair(1, opt2)); + EXPECT_TRUE(opt_it != options.end()); + + // Retrieve options with option code 2. + options = pkt->getOptions(2); + + // opt3 and opt4 should exist. + opt_it = std::find(options.begin(), options.end(), + std::pair(2, opt3)); + EXPECT_TRUE(opt_it != options.end()); + + opt_it = std::find(options.begin(), options.end(), + std::pair(2, opt4)); + EXPECT_TRUE(opt_it != options.end()); + + // Enable copying options when they are retrieved. + pkt->setCopyRetrievedOptions(true); + + options = pkt->getOptions(1); + ASSERT_EQ(2, options.size()); + + // Both retrieved options should be copied so an attempt to find them + // using option pointer should fail. Original pointers should have + // been replaced with new instances. + opt_it = std::find(options.begin(), options.end(), + std::pair(1, opt1)); + EXPECT_TRUE(opt_it == options.end()); + + opt_it = std::find(options.begin(), options.end(), + std::pair(1, opt2)); + EXPECT_TRUE(opt_it == options.end()); + + // Return instances of options with the option code 1 and make sure + // that copies of the options were used to replace original options + // in the packet. + pkt->setCopyRetrievedOptions(false); + OptionCollection options_modified = pkt->getOptions(1); + for (OptionCollection::const_iterator opt_it_modified = options_modified.begin(); + opt_it_modified != options_modified.end(); ++opt_it_modified) { + opt_it = std::find(options.begin(), options.end(), *opt_it_modified); + ASSERT_TRUE(opt_it != options.end()); + } + + // Let's check that remaining two options haven't been affected by + // retrieving the options with option code 1. + options = pkt->getOptions(2); + ASSERT_EQ(2, options.size()); + + opt_it = std::find(options.begin(), options.end(), + std::pair(2, opt3)); + EXPECT_TRUE(opt_it != options.end()); + + opt_it = std::find(options.begin(), options.end(), + std::pair(2, opt4)); + EXPECT_TRUE(opt_it != options.end()); +} + // This test verifies that it is possible to control whether a pointer // to an option or a pointer to a copy of an option is returned by the // packet object. diff --git a/src/lib/dhcp/tests/pkt6_unittest.cc b/src/lib/dhcp/tests/pkt6_unittest.cc index d829c569b4..616b89482e 100644 --- a/src/lib/dhcp/tests/pkt6_unittest.cc +++ b/src/lib/dhcp/tests/pkt6_unittest.cc @@ -71,7 +71,7 @@ public: : Pkt6(buf, len, proto) { } - using Pkt6::getNonCopiedOptions; + using Pkt::getNonCopiedOptions; using Pkt6::getNonCopiedRelayOption; using Pkt6::getNonCopiedRelayOptions; using Pkt6::getNonCopiedAnyRelayOption; @@ -487,9 +487,9 @@ TEST_F(Pkt6Test, addGetDelOptions) { } // Check that multiple options of the same type may be retrieved by using -// Pkt6::getOptions or Pkt6::getNonCopiedOptions. In the former case, also -// check that retrieved options are copied when Pkt6::setCopyRetrievedOptions -// is enabled. +// getOptions or getNonCopiedOptions. In the former case, also check +// that retrieved options are copied when setCopyRetrievedOptions is +// enabled. TEST_F(Pkt6Test, getOptions) { NakedPkt6 pkt(DHCPV6_SOLICIT, 1234); OptionPtr opt1(new Option(Option::V6, 1));