From: JINMEI Tatuya Date: Wed, 12 Dec 2012 22:01:37 +0000 (-0800) Subject: [2470] added a framework to report different TTLs via an optional callback. X-Git-Tag: bind10-1.0.0-beta-release~15^2~5 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=2c50eecd72f4e59aaea65cab950ae058179ae1bd;p=thirdparty%2Fkea.git [2470] added a framework to report different TTLs via an optional callback. the constructor was extended to have the additional callbacks. --- diff --git a/src/lib/dns/rrcollator.cc b/src/lib/dns/rrcollator.cc index 7090e6c4a4..5ac7e1fc42 100644 --- a/src/lib/dns/rrcollator.cc +++ b/src/lib/dns/rrcollator.cc @@ -31,7 +31,10 @@ 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) + {} void addRR(const Name& name, const RRClass& rrclass, const RRType& rrtype, const RRTTL& rrttl, @@ -39,6 +42,8 @@ public: RRsetPtr current_rrset_; AddRRsetCallback callback_; +private: + MasterLoaderCallbacks issue_callback_; }; namespace { @@ -75,13 +80,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("", 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() { diff --git a/src/lib/dns/rrcollator.h b/src/lib/dns/rrcollator.h index 3a9e0aa019..92fe1ed14a 100644 --- a/src/lib/dns/rrcollator.h +++ b/src/lib/dns/rrcollator.h @@ -63,12 +63,27 @@ public: /// \brief Constructor. /// + /// 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 + /// "" for the source name and the line number of 0 via + /// the callback. + /// /// \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. /// @@ -85,7 +100,7 @@ public: /// \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 diff --git a/src/lib/dns/tests/rrcollator_unittest.cc b/src/lib/dns/tests/rrcollator_unittest.cc index e66f87c03a..0b0452e1f8 100644 --- a/src/lib/dns/tests/rrcollator_unittest.cc +++ b/src/lib/dns/tests/rrcollator_unittest.cc @@ -25,12 +25,16 @@ #include +#include #include +#include #include #include +using std::string; using std::vector; +using boost::lexical_cast; using namespace isc::dns; using namespace isc::dns::rdata; @@ -50,9 +54,14 @@ addRRset(const RRsetPtr& rrset, vector* 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_)), + 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")), @@ -65,6 +74,12 @@ protected: "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, const RRType& expected_type, const RRTTL& expected_ttl, const vector& expected_rdatas) { @@ -92,6 +107,20 @@ 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(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_; @@ -101,6 +130,7 @@ protected: AddRRCallback rr_callback_; const RdataPtr a_rdata1_, a_rdata2_, txt_rdata_, sig_rdata1_, sig_rdata2_; vector rdatas_; // placeholder for expected data + vector warnings_, errors_; }; TEST_F(RRCollatorTest, basicCases) { @@ -134,6 +164,9 @@ 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_.flush(); checkRRset(Name("txt.example.com"), RRClass::CH(), RRType::TXT(), rrttl_, @@ -147,12 +180,16 @@ TEST_F(RRCollatorTest, basicCases) { 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_.flush(); checkRRset(origin_, rrclass_, RRType::A(), RRTTL(10), rdatas_); + EXPECT_EQ(1, warnings_.size()); + EXPECT_EQ(":0: Different TTLs for the same RRset: " + "example.com/IN/A, set to 10", warnings_.at(0)); } TEST_F(RRCollatorTest, maxTTLFirst) { @@ -164,6 +201,9 @@ TEST_F(RRCollatorTest, maxTTLFirst) { rdatas_.push_back(a_rdata2_); collator_.flush(); checkRRset(origin_, rrclass_, RRType::A(), RRTTL(10), rdatas_); + EXPECT_EQ(1, warnings_.size()); + EXPECT_EQ(":0: Different TTLs for the same RRset: " + "example.com/IN/A, set to 10", warnings_.at(0)); } TEST_F(RRCollatorTest, addRRSIGs) { @@ -200,13 +240,17 @@ TEST_F(RRCollatorTest, throwFromCallback) { 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_.flush(); + collator.flush(); rdatas_.push_back(a_rdata1_); checkRRset(origin_, rrclass_, RRType::A(), rrttl_, rdatas_); }