BIND 10 trac1688, updated. b09579f0af1644cd441ceca5deb2b6e1c40119bc [1688] Updated as a result of review
BIND 10 source code commits
bind10-changes at lists.isc.org
Tue Mar 20 19:00:33 UTC 2012
The branch, trac1688 has been updated
via b09579f0af1644cd441ceca5deb2b6e1c40119bc (commit)
from ff926e066594778c68772d77c3ecfe271fe3079c (commit)
Those revisions listed above that are new to this repository have
not appeared on any other notification email; so we list those
revisions in full, below.
- Log -----------------------------------------------------------------
commit b09579f0af1644cd441ceca5deb2b6e1c40119bc
Author: Stephen Morris <stephen at isc.org>
Date: Tue Mar 20 18:59:23 2012 +0000
[1688] Updated as a result of review
-----------------------------------------------------------------------
Summary of changes:
src/bin/auth/query.cc | 102 ++++++++----------------
src/bin/auth/query.h | 65 +++++++++++++++
src/lib/datasrc/rbnode_rrset.h | 18 ----
src/lib/datasrc/tests/rbnode_rrset_unittest.cc | 66 ---------------
src/lib/dns/rrset.cc | 29 -------
src/lib/dns/rrset.h | 22 -----
src/lib/dns/tests/rrset_unittest.cc | 60 --------------
7 files changed, 97 insertions(+), 265 deletions(-)
-----------------------------------------------------------------------
diff --git a/src/bin/auth/query.cc b/src/bin/auth/query.cc
index 4bc6190..a6a6662 100644
--- a/src/bin/auth/query.cc
+++ b/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
diff --git a/src/bin/auth/query.h b/src/bin/auth/query.h
index 5a11288..44092d0 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<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_;
};
}
diff --git a/src/lib/datasrc/rbnode_rrset.h b/src/lib/datasrc/rbnode_rrset.h
index 43814b4..d711f71 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<const RBNodeRRset*>(&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 0693070..d6fa32e 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<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.
diff --git a/src/lib/dns/rrset.cc b/src/lib/dns/rrset.cc
index c517fea..9a55f5f 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 7e137a3..43ade58 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 9e0d5e9..a0ea532 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<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());
More information about the bind10-changes
mailing list