BIND 10 trac2470, updated. 15d0b77ba78bc2507b65072a74294a85ee7e5440 [2470] reordered dns/ header files to check independence of rrcollator.h
BIND 10 source code commits
bind10-changes at lists.isc.org
Wed Dec 12 22:17:26 UTC 2012
The branch, trac2470 has been updated
via 15d0b77ba78bc2507b65072a74294a85ee7e5440 (commit)
via 0e2ef3c72360da9d0fcc4a36470bcb6b02591581 (commit)
via 822fa31524fc37af1ffdd265f8554fb9d85471f1 (commit)
via 2c50eecd72f4e59aaea65cab950ae058179ae1bd (commit)
via 103c817670b1d4f839e8e82ae01fea0fe3aa1298 (commit)
via a709670c0cb9ca9f1b8ea4f13a7191fcba0ab8cd (commit)
via 2d8efd0e48d097a54753ea6504e8e69bf65f41ba (commit)
via e0eda94e0b5f31efae914bf00deedf0955590c62 (commit)
via 04447bd12e293df0f40f46e16c87a77f49cffe25 (commit)
from ae7e56576e33265085f6542787d11d28efa097b3 (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 15d0b77ba78bc2507b65072a74294a85ee7e5440
Author: JINMEI Tatuya <jinmei at isc.org>
Date: Wed Dec 12 14:16:24 2012 -0800
[2470] reordered dns/ header files to check independence of rrcollator.h
as suggested in the review.
commit 0e2ef3c72360da9d0fcc4a36470bcb6b02591581
Author: JINMEI Tatuya <jinmei at isc.org>
Date: Wed Dec 12 14:13:24 2012 -0800
[2470] check and reject ampty add-RRset callback.
commit 822fa31524fc37af1ffdd265f8554fb9d85471f1
Author: JINMEI Tatuya <jinmei at isc.org>
Date: Wed Dec 12 14:04:54 2012 -0800
[2470] provide RR collator with issue callbacks
commit 2c50eecd72f4e59aaea65cab950ae058179ae1bd
Author: JINMEI Tatuya <jinmei at isc.org>
Date: Wed Dec 12 14:01:37 2012 -0800
[2470] added a framework to report different TTLs via an optional callback.
the constructor was extended to have the additional callbacks.
commit 103c817670b1d4f839e8e82ae01fea0fe3aa1298
Author: JINMEI Tatuya <jinmei at isc.org>
Date: Wed Dec 12 11:00:11 2012 -0800
[2470] initialize test Rdata for RRCollatorTest in member init list.
as a bonus we can make them const.
commit a709670c0cb9ca9f1b8ea4f13a7191fcba0ab8cd
Author: JINMEI Tatuya <jinmei at isc.org>
Date: Wed Dec 12 10:55:27 2012 -0800
[2470] renamed RRCollator::finish() to flush() as suggested in review.
commit 2d8efd0e48d097a54753ea6504e8e69bf65f41ba
Author: JINMEI Tatuya <jinmei at isc.org>
Date: Wed Dec 12 10:52:37 2012 -0800
[2470] completed doc for RRCollator::AddRRsetCallback.
commit e0eda94e0b5f31efae914bf00deedf0955590c62
Author: JINMEI Tatuya <jinmei at isc.org>
Date: Wed Dec 12 10:47:36 2012 -0800
[2470] make RRCollator noncopyable, add some doc about the rationale.
commit 04447bd12e293df0f40f46e16c87a77f49cffe25
Author: JINMEI Tatuya <jinmei at isc.org>
Date: Wed Dec 12 10:35:20 2012 -0800
[2470] removed redundant #include
-----------------------------------------------------------------------
Summary of changes:
src/lib/datasrc/memory/zone_data_loader.cc | 11 ++--
src/lib/dns/rrcollator.cc | 32 +++++++--
src/lib/dns/rrcollator.h | 42 ++++++++++--
src/lib/dns/tests/rrcollator_unittest.cc | 98 ++++++++++++++++++++--------
4 files changed, 139 insertions(+), 44 deletions(-)
-----------------------------------------------------------------------
diff --git a/src/lib/datasrc/memory/zone_data_loader.cc b/src/lib/datasrc/memory/zone_data_loader.cc
index 36b2920..0238093 100644
--- a/src/lib/datasrc/memory/zone_data_loader.cc
+++ b/src/lib/datasrc/memory/zone_data_loader.cc
@@ -23,7 +23,6 @@
#include <dns/rrcollator.h>
#include <dns/rdataclass.h>
#include <dns/rrset.h>
-#include <dns/rrcollator.h>
#include <boost/foreach.hpp>
#include <boost/bind.hpp>
@@ -194,14 +193,14 @@ masterLoaderWrapper(const char* const filename, const Name& origin,
const RRClass& zone_class, LoadCallback callback)
{
bool load_ok = false; // (we don't use it)
- dns::RRCollator collator(boost::bind(callback, _1));
+ const MasterLoaderCallbacks issue_callbacks =
+ createMasterLoaderCallbacks(origin, zone_class, &load_ok);
+ dns::RRCollator collator(boost::bind(callback, _1), issue_callbacks);
try {
- dns::MasterLoader(filename, origin, zone_class,
- createMasterLoaderCallbacks(origin, zone_class,
- &load_ok),
+ dns::MasterLoader(filename, origin, zone_class, issue_callbacks,
collator.getCallback()).load();
- collator.finish();
+ collator.flush();
} catch (const dns::MasterLoaderError& e) {
isc_throw(ZoneLoaderException, e.what());
}
diff --git a/src/lib/dns/rrcollator.cc b/src/lib/dns/rrcollator.cc
index 1d30ce6..d8cc55b 100644
--- a/src/lib/dns/rrcollator.cc
+++ b/src/lib/dns/rrcollator.cc
@@ -12,9 +12,13 @@
// OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR
// PERFORMANCE OF THIS SOFTWARE.
+#include <exceptions/exceptions.h>
+
+// include this first to check the header is self-contained.
+#include <dns/rrcollator.h>
+
#include <dns/name.h>
#include <dns/rdataclass.h>
-#include <dns/rrcollator.h>
#include <dns/rrclass.h>
#include <dns/rrtype.h>
#include <dns/rrttl.h>
@@ -31,7 +35,14 @@ using namespace rdata;
class RRCollator::Impl {
public:
- Impl(const AddRRsetCallback& callback) : callback_(callback) {}
+ Impl(const AddRRsetCallback& callback,
+ const MasterLoaderCallbacks& issue_callback) :
+ callback_(callback), issue_callback_(issue_callback)
+ {
+ if (!callback_) {
+ isc_throw(InvalidParameter, "Empty add RRset callback");
+ }
+ }
void addRR(const Name& name, const RRClass& rrclass,
const RRType& rrtype, const RRTTL& rrttl,
@@ -39,6 +50,8 @@ public:
RRsetPtr current_rrset_;
AddRRsetCallback callback_;
+private:
+ MasterLoaderCallbacks issue_callback_;
};
namespace {
@@ -75,13 +88,20 @@ RRCollator::Impl::addRR(const Name& name, const RRClass& rrclass,
current_rrset_ = RRsetPtr(new RRset(name, rrclass, rrtype, rrttl));
} else if (current_rrset_->getTTL() != rrttl) {
// RRs with different TTLs are given. Smaller TTL should win.
- current_rrset_->setTTL(std::min(current_rrset_->getTTL(), rrttl));
+ const RRTTL min_ttl(std::min(current_rrset_->getTTL(), rrttl));
+ issue_callback_.warning("<unknown source>", 0,
+ "Different TTLs for the same RRset: " +
+ name.toText(true) + "/" +
+ rrclass.toText() + "/" + rrtype.toText() +
+ ", set to " + min_ttl.toText());
+ current_rrset_->setTTL(min_ttl);
}
current_rrset_->addRdata(rdata);
}
-RRCollator::RRCollator(const AddRRsetCallback& callback) :
- impl_(new Impl(callback))
+RRCollator::RRCollator(const AddRRsetCallback& callback,
+ const MasterLoaderCallbacks& issue_callback) :
+ impl_(new Impl(callback, issue_callback))
{}
RRCollator::~RRCollator() {
@@ -95,7 +115,7 @@ RRCollator::getCallback() {
}
void
-RRCollator::finish() {
+RRCollator::flush() {
if (impl_->current_rrset_) {
impl_->callback_(impl_->current_rrset_);
impl_->current_rrset_.reset();
diff --git a/src/lib/dns/rrcollator.h b/src/lib/dns/rrcollator.h
index df8a397..97b7642 100644
--- a/src/lib/dns/rrcollator.h
+++ b/src/lib/dns/rrcollator.h
@@ -18,6 +18,7 @@
#include <dns/master_loader_callbacks.h>
#include <dns/rrset.h>
+#include <boost/noncopyable.hpp>
#include <boost/function.hpp>
namespace isc {
@@ -44,19 +45,48 @@ namespace dns {
/// single RRset. In fact, it can only collate RRs that are consecutive
/// in the original stream; once it encounters an RR of a different RRset,
/// any subsequent RRs of the previous RRset will form a separate RRset object.
-class RRCollator {
+///
+/// This class is non-copyable; it's partially for the convenience of internal
+/// implementation details, but it actually doesn't make sense to copy
+/// an object of this class, if not harmful, for the intended usage of
+/// the class.
+class RRCollator : boost::noncopyable {
public:
- /// \brief
+ /// \brief Callback functor type for \c RRCollator.
+ ///
+ /// This type of callback is given to an \c RRCollator object on its
+ /// construction, and will be called for each collated RRset built in
+ /// the \c RRCollator.
+ ///
+ /// \param rrset The collated RRset.
typedef boost::function<void(const RRsetPtr& rrset)> AddRRsetCallback;
/// \brief Constructor.
///
+ /// \c callback must not be an empty functor.
+ ///
+ /// If the optional issue_callback parameter is given, it will be used
+ /// to report any errors and non fatal warnings found in the collator's
+ /// operation. By default special callbacks that do nothing are used.
+ ///
+ /// \note Since the \c RRCollator does not have any information on the
+ /// source of the given RRs (which is normally a DNS master file in the
+ /// intended usage) it cannot provide the actual source name or the line
+ /// number via the callback. Instead, it passes a special string of
+ /// "<unknown source>" for the source name and the line number of 0 via
+ /// the callback.
+ ///
+ /// \throw isc::InvalidParameter Empty RRset callback is given.
/// \throw std::bad_alloc Internal memory allocation fails. This should
/// be very rare.
///
/// \param callback The callback functor to be called for each collated
/// RRset.
- RRCollator(const AddRRsetCallback& callback);
+ /// \param issue_callback The callbacks to be called on any issue found
+ /// in the collator.
+ RRCollator(const AddRRsetCallback& callback,
+ const MasterLoaderCallbacks& issue_callback =
+ MasterLoaderCallbacks::getNullCallbacks());
/// \brief Destructor.
///
@@ -66,14 +96,14 @@ public:
/// impossible to predict how this class is used (to see if it's a very
/// rare case where propagating an exception from a destructor is
/// justified). Instead, the application needs to make sure that
- /// \c finish() is called before the object of this class is destroyed.
+ /// \c flush() is called before the object of this class is destroyed.
///
/// \throw None
~RRCollator();
/// \brief Call the callback on the remaining RRset, if any.
///
- /// This method is expected to be called that it's supposed all RRs have
+ /// This method is expected to be called when it's supposed all RRs have
/// been passed to this class object. Since there is no explicit
/// indicator of the end of the stream, the user of this class needs to
/// explicitly call this method to call the callback for the last buffered
@@ -84,7 +114,7 @@ public:
///
/// It propagates any exception thrown from the callback; otherwise it
/// doesn't throw anything.
- void finish();
+ void flush();
/// \brief Return \c MasterLoader compatible callback.
///
diff --git a/src/lib/dns/tests/rrcollator_unittest.cc b/src/lib/dns/tests/rrcollator_unittest.cc
index 68b4fe3..155edb9 100644
--- a/src/lib/dns/tests/rrcollator_unittest.cc
+++ b/src/lib/dns/tests/rrcollator_unittest.cc
@@ -25,12 +25,16 @@
#include <gtest/gtest.h>
+#include <boost/lexical_cast.hpp>
#include <boost/bind.hpp>
+#include <string>
#include <sstream>
#include <vector>
+using std::string;
using std::vector;
+using boost::lexical_cast;
using namespace isc::dns;
using namespace isc::dns::rdata;
@@ -50,22 +54,30 @@ addRRset(const RRsetPtr& rrset, vector<ConstRRsetPtr>* to_append,
class RRCollatorTest : public ::testing::Test {
protected:
RRCollatorTest() :
+ issue_callbacks_(boost::bind(&RRCollatorTest::issueCallback, this,
+ _1, _2, _3, true),
+ boost::bind(&RRCollatorTest::issueCallback, this,
+ _1, _2, _3, false)),
origin_("example.com"), rrclass_(RRClass::IN()), rrttl_(3600),
throw_from_callback_(false),
- collator_(boost::bind(addRRset, _1, &rrsets_, &throw_from_callback_)),
- rr_callback_(collator_.getCallback())
- {
- a_rdata1_ = createRdata(RRType::A(), rrclass_, "192.0.2.1");
- a_rdata2_ = createRdata(RRType::A(), rrclass_, "192.0.2.2");
-
- txt_rdata_ = createRdata(RRType::TXT(), rrclass_, "test");
-
- sig_rdata1_ = createRdata(RRType::RRSIG(), rrclass_,
- "A 5 3 3600 20000101000000 20000201000000 "
- "12345 example.com. FAKE\n");
- sig_rdata2_ = createRdata(RRType::RRSIG(), rrclass_,
- "NS 5 3 3600 20000101000000 20000201000000 "
- "12345 example.com. FAKE\n");
+ collator_(boost::bind(addRRset, _1, &rrsets_, &throw_from_callback_),
+ issue_callbacks_),
+ rr_callback_(collator_.getCallback()),
+ a_rdata1_(createRdata(RRType::A(), rrclass_, "192.0.2.1")),
+ a_rdata2_(createRdata(RRType::A(), rrclass_, "192.0.2.2")),
+ txt_rdata_(createRdata(RRType::TXT(), rrclass_, "test")),
+ sig_rdata1_(createRdata(RRType::RRSIG(), rrclass_,
+ "A 5 3 3600 20000101000000 20000201000000 "
+ "12345 example.com. FAKE\n")),
+ sig_rdata2_(createRdata(RRType::RRSIG(), rrclass_,
+ "NS 5 3 3600 20000101000000 20000201000000 "
+ "12345 example.com. FAKE\n"))
+ {}
+
+ virtual void TearDown() {
+ // (The current implementation of) RRCollator should never report an
+ // error.
+ EXPECT_TRUE(errors_.empty());
}
void checkRRset(const Name& expected_name, const RRClass& expected_class,
@@ -95,15 +107,30 @@ protected:
rrsets_.clear();
}
+ void issueCallback(const string& src_name, size_t src_line,
+ const string& reason, bool is_error) {
+ const string msg(src_name + ":" + lexical_cast<string>(src_line) +
+ ": " + reason);
+ if (is_error) {
+ errors_.push_back(msg);
+ } else {
+ warnings_.push_back(msg);
+ }
+ }
+
+private:
+ MasterLoaderCallbacks issue_callbacks_;
+protected:
const Name origin_;
const RRClass rrclass_;
const RRTTL rrttl_;
vector<ConstRRsetPtr> rrsets_;
- RdataPtr a_rdata1_, a_rdata2_, txt_rdata_, sig_rdata1_, sig_rdata2_;
bool throw_from_callback_;
RRCollator collator_;
AddRRCallback rr_callback_;
+ const RdataPtr a_rdata1_, a_rdata2_, txt_rdata_, sig_rdata1_, sig_rdata2_;
vector<ConstRdataPtr> rdatas_; // placeholder for expected data
+ vector<string> warnings_, errors_;
};
TEST_F(RRCollatorTest, basicCases) {
@@ -137,25 +164,32 @@ TEST_F(RRCollatorTest, basicCases) {
checkRRset(Name("txt.example.com"), rrclass_, RRType::TXT(), rrttl_,
rdatas_);
+ // There should have been no warnings.
+ EXPECT_EQ(0, warnings_.size());
+
// Tell the collator we are done, then we'll see the last RR as an RRset.
- collator_.finish();
+ collator_.flush();
checkRRset(Name("txt.example.com"), RRClass::CH(), RRType::TXT(), rrttl_,
rdatas_);
- // Redundant finish() will be no-op.
- collator_.finish();
+ // Redundant flush() will be no-op.
+ collator_.flush();
EXPECT_TRUE(rrsets_.empty());
}
TEST_F(RRCollatorTest, minTTLFirst) {
// RRs of the same RRset but has different TTLs. The first RR has
// the smaller TTL, which should be used for the TTL of the RRset.
+ // There should be a warning callback about this.
rr_callback_(origin_, rrclass_, RRType::A(), RRTTL(10), a_rdata1_);
rr_callback_(origin_, rrclass_, RRType::A(), RRTTL(20), a_rdata2_);
rdatas_.push_back(a_rdata1_);
rdatas_.push_back(a_rdata2_);
- collator_.finish();
+ collator_.flush();
checkRRset(origin_, rrclass_, RRType::A(), RRTTL(10), rdatas_);
+ EXPECT_EQ(1, warnings_.size());
+ EXPECT_EQ("<unknown source>:0: Different TTLs for the same RRset: "
+ "example.com/IN/A, set to 10", warnings_.at(0));
}
TEST_F(RRCollatorTest, maxTTLFirst) {
@@ -165,8 +199,11 @@ TEST_F(RRCollatorTest, maxTTLFirst) {
rr_callback_(origin_, rrclass_, RRType::A(), RRTTL(10), a_rdata2_);
rdatas_.push_back(a_rdata1_);
rdatas_.push_back(a_rdata2_);
- collator_.finish();
+ collator_.flush();
checkRRset(origin_, rrclass_, RRType::A(), RRTTL(10), rdatas_);
+ EXPECT_EQ(1, warnings_.size());
+ EXPECT_EQ("<unknown source>:0: Different TTLs for the same RRset: "
+ "example.com/IN/A, set to 10", warnings_.at(0));
}
TEST_F(RRCollatorTest, addRRSIGs) {
@@ -178,8 +215,8 @@ TEST_F(RRCollatorTest, addRRSIGs) {
checkRRset(origin_, rrclass_, RRType::RRSIG(), rrttl_, rdatas_);
}
-TEST_F(RRCollatorTest, emptyFinish) {
- collator_.finish();
+TEST_F(RRCollatorTest, emptyFlush) {
+ collator_.flush();
EXPECT_TRUE(rrsets_.empty());
}
@@ -195,21 +232,30 @@ TEST_F(RRCollatorTest, throwFromCallback) {
// We'll only see the A RR.
throw_from_callback_ = false;
- collator_.finish();
+ collator_.flush();
rdatas_.push_back(a_rdata1_);
checkRRset(origin_, rrclass_, RRType::A(), rrttl_, rdatas_);
}
+TEST_F(RRCollatorTest, emptyCallback) {
+ const AddRRsetCallback empty_callback;
+ EXPECT_THROW(RRCollator collator(empty_callback), isc::InvalidParameter);
+}
+
TEST_F(RRCollatorTest, withMasterLoader) {
// Test a simple case with MasterLoader. There shouldn't be anything
// special, but that's the mainly intended usage of the collator, so we
- // check it explicitly.
+ // check it explicitly. Also, this test uses a different local collator,
+ // just for checking it works with omitting the issue callback (using
+ // the default).
+ RRCollator collator(boost::bind(addRRset, _1, &rrsets_,
+ &throw_from_callback_));
std::istringstream ss("example.com. 3600 IN A 192.0.2.1\n");
MasterLoader loader(ss, origin_, rrclass_,
MasterLoaderCallbacks::getNullCallbacks(),
- collator_.getCallback());
+ collator.getCallback());
loader.load();
- collator_.finish();
+ collator.flush();
rdatas_.push_back(a_rdata1_);
checkRRset(origin_, rrclass_, RRType::A(), rrttl_, rdatas_);
}
More information about the bind10-changes
mailing list