From: Stephen Morris Date: Tue, 20 Mar 2012 18:59:23 +0000 (+0000) Subject: [1688] Updated as a result of review X-Git-Tag: trac2351_base~226^2~116^2~66^2~10^2~13 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=b09579f0af1644cd441ceca5deb2b6e1c40119bc;p=thirdparty%2Fkea.git [1688] Updated as a result of review --- diff --git a/src/bin/auth/query.cc b/src/bin/auth/query.cc index 4bc6190d3b..a6a666256d 100644 --- a/src/bin/auth/query.cc +++ b/src/bin/auth/query.cc @@ -26,8 +26,9 @@ #include #include +#include #include // for std::max -#include +#include #include using namespace std; @@ -35,70 +36,9 @@ using namespace isc::dns; using namespace isc::datasrc; using namespace isc::dns::rdata; -// Commonly used helper callback object for vector to -// insert them to (the specified section of) a message. -// -// One feature is that it maintains an internal set of raw pointers to the -// RRsets as they are added (this is safe - the object is only in scope in -// the createResponse() method and during this time, all RRsets referred to -// remain in existence due to the presence of the ConstRRsetPtr objects -// from which the raw objects were derived). The set is used to detect -// and discard duplicates. -namespace { -class RRsetInserter { -private: - // \brief RRset comparison functor. - struct RRsetLthan { - bool operator()(const AbstractRRset* r1, const AbstractRRset* r2) { - return (r1->lthan(*r2)); - } - }; - - typedef std::set AddedRRsets; - -public: - RRsetInserter(Message& msg, const Message::Section section, - const bool dnssec) : - msg_(msg), section_(section), dnssec_(dnssec) - {} - - // \brief Set Target Section - // - // Sets the section into which the information added by addRRset will be - // inserted. - // - // \param section New section number - void setSection(Message::Section section) { - section_ = section; - } - - // Insertion operation - // - // \param rrset Pointer to RRset to be added to the message - void addRRset(const ConstRRsetPtr& rrset) { - // Has the RRset already been added to this message? - std::pair result = - rrsets_added_.insert(rrset.get()); - if (result.second) { - // Were able to add the pointer to the RRset to rrsets_added_, so - // the RRset has not already been seen. Add it to the message. - // The const-cast is wrong, but the Message interface seems - // to insist. - msg_.addRRset(section_, - boost::const_pointer_cast(rrset), - dnssec_); - } - } - -private: - Message& msg_; - Message::Section section_; - const bool dnssec_; - AddedRRsets rrsets_added_; -}; - // This is a "constant" vector storing desired RR types for the additional // section. The vector is filled first time it's used. +namespace { const vector& A_AND_AAAA() { static vector needed_types; @@ -108,12 +48,32 @@ A_AND_AAAA() { } return (needed_types); } - } namespace isc { namespace auth { +void +Query::RRsetInserter::addRRset(isc::dns::Message& message, + const isc::dns::Message::Section section, + const ConstRRsetPtr& rrset, const bool dnssec) +{ + /// Is this RRset already in the list of RRsets added to the message? + std::vector::iterator i = + std::find_if(added_.begin(), added_.end(), + std::bind1st(Query::RRsetInserter::isSameKind(), + rrset.get())); + if (i == added_.end()) { + // No - add it to both the message and the list of RRsets processed. + // The const-cast is wrong, but the message interface seems to insist. + message.addRRset(section, + boost::const_pointer_cast(rrset), + dnssec); + added_.push_back(rrset.get()); + } +} + + void Query::addSOA(ZoneFinder& finder) { ZoneFinderContextPtr soa_ctx = finder.find(finder.getOrigin(), @@ -571,25 +531,26 @@ Query::initialize(datasrc::DataSourceClient& datasrc_client, void Query::createResponse() { + // Inserter should be reset each time the query is reset, so should be + // empty at this point. + assert(inserter_.empty()); + // Add the RRsets to the message. The order of sections is important, // as the RRsetInserter remembers RRsets added and will not add // duplicates. Adding in the order answer, authory, additional will // guarantee that if there are duplicates, the single RRset added will // appear in the most important section. std::vector::const_iterator i; - RRsetInserter inserter(*response_, Message::SECTION_ANSWER, dnssec_); for (i = answers_.begin(); i != answers_.end(); ++i) { - inserter.addRRset(*i); + inserter_.addRRset(*response_, Message::SECTION_ANSWER, *i, dnssec_); } - inserter.setSection(Message::SECTION_AUTHORITY); for (i = authorities_.begin(); i != authorities_.end(); ++i) { - inserter.addRRset(*i); + inserter_.addRRset(*response_, Message::SECTION_AUTHORITY, *i, dnssec_); } - inserter.setSection(Message::SECTION_ADDITIONAL); for (i = additionals_.begin(); i != additionals_.end(); ++i) { - inserter.addRRset(*i); + inserter_.addRRset(*response_, Message::SECTION_ADDITIONAL, *i, dnssec_); } } @@ -602,6 +563,7 @@ Query::reset() { answers_.clear(); authorities_.clear(); additionals_.clear(); + inserter_.clear(); } bool diff --git a/src/bin/auth/query.h b/src/bin/auth/query.h index 5a112889f0..44092d0832 100644 --- a/src/bin/auth/query.h +++ b/src/bin/auth/query.h @@ -256,6 +256,70 @@ private: /// Called by the QueryCleaner object upon its destruction void reset(); + /// \brief Inserter Class + /// + /// Used during the construction of the response message, this performs + /// the duplicate RRset detection check. It keeps a list of RRsets added + /// to the message and does not add an RRset if it is the same as one + /// already added. + class RRsetInserter { + public: + // \brief RRset comparison functor. + struct isSameKind : public std::binary_function< + const isc::dns::AbstractRRset*, + const isc::dns::AbstractRRset*, + bool> { + bool operator()(const isc::dns::AbstractRRset* r1, + const isc::dns::AbstractRRset* r2) const { + return (r1->isSameKind(*r2)); + } + }; + + /// \brief Constructor + /// + /// Reserves space for the list of RRsets. Although the RRInserter + /// will be used to create a message from the contents of the Query + /// object's answers_, authorities_ and additionals_ elements, and + /// each of these are sized to RESERVE_RRSETS, it is _extremely_ + /// unlikely that all three will be filled to capacity. So we reserve + /// more elements than in each of these components, but not three + /// times the amount. + /// + /// As with the answers_, authorities_ and additionals_ elements, the + /// reservation is made in the constructor to avoid dynamic allocation + /// of memory. The RRsetInserter is a member variable of the Query + /// object so is constructed once and lasts as long as that object. + /// Internal state is cleared through the clear() method. + RRsetInserter() { + added_.reserve(2 * RESERVE_RRSETS); + } + + /// \brief Reset internal state + void clear() { + added_.clear(); + } + + /// \brief Return true if empty + bool empty() const { + return (added_.empty()); + } + + /// Insertion operation + /// + /// \param message Message to which the RRset is to be added + /// \param section Section of the message in which the RRset is put + /// \param rrset Pointer to RRset to be added to the message + /// \param dnssec Whether RRSIG records should be added as well + void addRRset(isc::dns::Message& message, + const isc::dns::Message::Section section, + const isc::dns::ConstRRsetPtr& rrset, const bool dnssec); + + private: + /// List of RRsets already added to the message + std::vector added_; + }; + + /// \brief Internal class used for cleanup of Query members /// /// The process() call creates an object of this class, which @@ -421,6 +485,7 @@ protected: std::vector answers_; std::vector authorities_; std::vector additionals_; + RRsetInserter inserter_; }; } diff --git a/src/lib/datasrc/rbnode_rrset.h b/src/lib/datasrc/rbnode_rrset.h index 43814b481b..d711f71e77 100644 --- a/src/lib/datasrc/rbnode_rrset.h +++ b/src/lib/datasrc/rbnode_rrset.h @@ -141,24 +141,6 @@ public: } } - virtual bool lthan(const AbstractRRset& other) const { - // Like "isSameKind", this method is an optimisation for the case where - // there will only ever be one object containing a particular RRset, - // and if the objects are different, the RRset they represent are - // different. - // - // lthan() is only used as an ordering for objects like std::set. - // Therefore the only criteria is that two objects are consistently - // ordered in the same way. The quickest way to do this is to use - // a comparison based on the address of the objects. - const RBNodeRRset* rb = dynamic_cast(&other); - if (rb != NULL) { - return (this < rb); - } else { - return (AbstractRRset::lthan(other)); - } - } - virtual unsigned int toWire( isc::dns::AbstractMessageRenderer& renderer) const; diff --git a/src/lib/datasrc/tests/rbnode_rrset_unittest.cc b/src/lib/datasrc/tests/rbnode_rrset_unittest.cc index 069307059b..d6fa32e049 100644 --- a/src/lib/datasrc/tests/rbnode_rrset_unittest.cc +++ b/src/lib/datasrc/tests/rbnode_rrset_unittest.cc @@ -169,72 +169,6 @@ addRRset(std::vector& vec, const RRType& rrtype, RRTTL(3600)))); } -TEST_F(RBNodeRRsetTest, lthan) { - // Check values of type codes: this effectively documents the expected - // order of the rrsets created. - ASSERT_EQ(1, RRType::A().getCode()); - ASSERT_EQ(2, RRType::NS().getCode()); - - ASSERT_EQ(1, RRClass::IN().getCode()); - ASSERT_EQ(3, RRClass::CH().getCode()); - - // Create a vector of RRsets in ascending (conventional) sort order. - std::vector rrsets; - addRRset(rrsets, RRType::A(), RRClass::IN(), "alpha.com"); - addRRset(rrsets, RRType::A(), RRClass::IN(), "beta.com"); - addRRset(rrsets, RRType::A(), RRClass::CH(), "alpha.com"); - addRRset(rrsets, RRType::A(), RRClass::CH(), "beta.com"); - - // ... and create eight RBNodeRRsets for the underlying objects (two - // sets of four - the reason becomes apparent below). - std::vector rbrrsets; - for (int i = 0; i < 2; ++i) { - for (int j = 0; j < rrsets.size(); ++j) { - rbrrsets.push_back(ConstRRsetPtr(new RBNodeRRset(rrsets[j]))); - } - } - - // Using the first four of the RBNodeRRsets, check that they order - // correctly when compared with the standard RRsets. - for (int i = 0; i < rrsets.size(); ++i) { - for (int j = 0; j < rrsets.size(); ++j) { - stringstream ss; - ss << "Comparing RbNodeRRset[" << i << "] against RRset[" - << j << "]"; - SCOPED_TRACE(ss.str()); - if (i < j) { - EXPECT_TRUE(rbrrsets[i]->lthan(*rrsets[j])); - } else { - EXPECT_FALSE(rbrrsets[i]->lthan(*rrsets[j])); - } - } - } - - // Put the raw pointers of the eight RBNodeRRsets into a vector and - // sort in ascending order. - std::vector rbptrs; - for (int i = 0; i < rbrrsets.size(); ++i) { - rbptrs.push_back(rbrrsets[i].get()); - } - sort(rbptrs.begin(), rbptrs.end()); - - // Now iterate through and check the relationships. Addresses with lower - // indexes should sort lower than addresses with higher indexes, regardless - // of what they point to. - for (int i = 0; i < rbptrs.size(); ++i) { - for (int j = 0; j < rbptrs.size(); ++j) { - stringstream ss; - ss << "Comparing address[" << i << "] against address[" << j << "]"; - SCOPED_TRACE(ss.str()); - if (i < j) { - EXPECT_TRUE(rbptrs[i]->lthan(*rbptrs[j])); - } else { - EXPECT_FALSE(rbptrs[i]->lthan(*rbptrs[j])); - } - } - } -} - // Note: although the next two tests are essentially the same and used common // test code, they use different test data: the MessageRenderer produces // compressed wire data whereas the OutputBuffer does not. diff --git a/src/lib/dns/rrset.cc b/src/lib/dns/rrset.cc index c517feab2e..9a55f5f55c 100644 --- a/src/lib/dns/rrset.cc +++ b/src/lib/dns/rrset.cc @@ -123,35 +123,6 @@ AbstractRRset::isSameKind(const AbstractRRset& other) const { getClass() == other.getClass()); } -bool -AbstractRRset::lthan(const AbstractRRset& other) const { - - // Check on type first... - const uint16_t my_type = getType().getCode(); - const uint16_t other_type = other.getType().getCode(); - if (my_type < other_type) { - return (true); - - } else if (my_type == other_type) { - // Types equal, so check class - const uint16_t my_class = getClass().getCode(); - const uint16_t other_class = other.getClass().getCode(); - if (my_class < other_class) { - return (true); - - } else if (my_class == other_class) { - // Class equal, so check name - return (getName().lthan(other.getName())); - - } else { - return (false); - } - - } else { - return (false); - } -} - ostream& operator<<(ostream& os, const AbstractRRset& rrset) { os << rrset.toText(); diff --git a/src/lib/dns/rrset.h b/src/lib/dns/rrset.h index 7e137a3332..43ade58063 100644 --- a/src/lib/dns/rrset.h +++ b/src/lib/dns/rrset.h @@ -482,28 +482,6 @@ public: /// \param other Pointer to another AbstractRRset to compare /// against. virtual bool isSameKind(const AbstractRRset& other) const; - - /// \brief Check if one RRset is "less" than another - /// - /// This method is needed for storing RRsets in STL containers such - /// as multisets. It applies an ordering based on - /// - Type - /// - Class - /// - Name - /// (Type and Class are ordered by the values associated with those - /// constants. Name is ordered according to case-insensitive comparison.) - /// - /// Note that unlike isSameKind, type and class are checked before name. - /// This is because with ordering based on A, B and C (in that order), the - /// algorithm needs to do two checks on A and B - a "less than" check and a - /// check for equality. It only needs to do a "less than" check on C. - /// equality. It only needs to do one check on C, - /// - /// \param other The other AbstractRRset to compare against. - /// - /// \return true if "this" is less than the given RRset according to - /// the criteria given. - virtual bool lthan(const AbstractRRset& other) const; //@} }; diff --git a/src/lib/dns/tests/rrset_unittest.cc b/src/lib/dns/tests/rrset_unittest.cc index 9e0d5e91c6..a0ea532b09 100644 --- a/src/lib/dns/tests/rrset_unittest.cc +++ b/src/lib/dns/tests/rrset_unittest.cc @@ -127,66 +127,6 @@ TEST_F(RRsetTest, isSameKind) { EXPECT_FALSE(rrset_w.isSameKind(rrset_p)); } -// Utility function to create an add an RRset to a vector of RRsets for the -// "less" test. It's only purpose is to allow the RRset creation to be -// written with arguments in an order that reflects the RRset ordering. -void -addRRset(std::vector& vec, const RRType& rrtype, - const RRClass& rrclass, const char* rrname) -{ - vec.push_back(ConstRRsetPtr(new RRset(Name(rrname), rrclass, rrtype, - RRTTL(3600)))); -} - -TEST_F(RRsetTest, lthan) { - // Check values of type codes: this effectively documents the expected - // order of the rrsets created. - ASSERT_EQ(1, RRType::A().getCode()); - ASSERT_EQ(2, RRType::NS().getCode()); - - ASSERT_EQ(1, RRClass::IN().getCode()); - ASSERT_EQ(3, RRClass::CH().getCode()); - - // Create a vector of RRsets in ascending sort order. - std::vector rrsets; - addRRset(rrsets, RRType::A(), RRClass::IN(), "alpha.com"); - addRRset(rrsets, RRType::A(), RRClass::IN(), "beta.com"); - addRRset(rrsets, RRType::A(), RRClass::CH(), "alpha.com"); - addRRset(rrsets, RRType::A(), RRClass::CH(), "beta.com"); - addRRset(rrsets, RRType::NS(), RRClass::IN(), "alpha.com"); - addRRset(rrsets, RRType::NS(), RRClass::IN(), "beta.com"); - addRRset(rrsets, RRType::NS(), RRClass::CH(), "alpha.com"); - addRRset(rrsets, RRType::NS(), RRClass::CH(), "beta.com"); - - // ... and do the checks. The ASSERT_ form is used to avoid a plethora - // of messages if there is an error. And if there is an error, supply - // a more informative message. - for (int i = 0; i < rrsets.size(); ++i) { - // Check that an RRset is not less than itself - ostringstream ossi; - ossi << "i = (" - << rrsets[i]->getType().toText() << ", " - << rrsets[i]->getClass().toText() << ", " - << rrsets[i]->getName().toText() - << ")"; - ASSERT_FALSE(rrsets[i]->lthan(*rrsets[i])) << ossi.str(); - for (int j = i + 1; j < rrsets.size(); ++j) { - // Check it against the remaining RRsets. - ostringstream ossj; - ossj << ", j = (" - << rrsets[j]->getType().toText() << ", " - << rrsets[j]->getClass().toText() << ", " - << rrsets[j]->getName().toText() - << ")"; - ASSERT_TRUE(rrsets[i]->lthan(*rrsets[j])) - << ossi.str() << ossj.str(); - ASSERT_FALSE(rrsets[j]->lthan(*rrsets[i])) - << ossi.str() << ossj.str(); - } - } -} - - void addRdataTestCommon(const RRset& rrset) { EXPECT_EQ(2, rrset.getRdataCount());