[1688] Updated as a result of review

src/bin/auth/query.cc

@@ -26,8 +26,9 @@
 #include <boost/bind.hpp>
 #include <boost/function.hpp>
 
+#include <cassert>
 #include <algorithm>            // for std::max
-#include <set>
+#include <functional>
 #include <vector>
 
 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<ConstRRsetPtr> 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<const AbstractRRset*, RRsetLthan> 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<AddedRRsets::iterator, bool> 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<AbstractRRset>(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<RRType>&
 A_AND_AAAA() {
     static vector<RRType> needed_types;
@@ -108,13 +48,33 @@ 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<const AbstractRRset*>::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<AbstractRRset>(rrset),
+                         dnssec);
+        added_.push_back(rrset.get());
+    }
+}
+
+
+void
 Query::addSOA(ZoneFinder& finder) {
     ZoneFinderContextPtr soa_ctx = finder.find(finder.getOrigin(),
                                                RRType::SOA(), dnssec_opt_);
@@ -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<isc::dns::ConstRRsetPtr>::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

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<const isc::dns::AbstractRRset*> 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<isc::dns::ConstRRsetPtr> answers_;
     std::vector<isc::dns::ConstRRsetPtr> authorities_;
     std::vector<isc::dns::ConstRRsetPtr> additionals_;
+    RRsetInserter inserter_;
 };
 
 }

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<const RBNodeRRset*>(&other);
-        if (rb != NULL) {
-            return (this < rb);
-        } else {
-            return (AbstractRRset::lthan(other));
-        }
-    }
-
 
     virtual unsigned int toWire(
         isc::dns::AbstractMessageRenderer& renderer) const;

src/lib/datasrc/tests/rbnode_rrset_unittest.cc

@@ -169,72 +169,6 @@ addRRset(std::vector<ConstRRsetPtr>& 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<ConstRRsetPtr> 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<ConstRRsetPtr> 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<const AbstractRRset*> 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.

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();

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;
     //@}
 
 };

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<ConstRRsetPtr>& 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<ConstRRsetPtr> 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());