Commit 92ac02b8 authored by JINMEI Tatuya's avatar JINMEI Tatuya

[2470] Revert "[2470] added a framework to report different TTLs via an optional callback."

This reverts commit 2c50eecd.

Conflicts:
	src/lib/dns/rrcollator.cc
	src/lib/dns/rrcollator.h
parent 2a8f04d9
...@@ -35,14 +35,7 @@ using namespace rdata; ...@@ -35,14 +35,7 @@ using namespace rdata;
class RRCollator::Impl { class RRCollator::Impl {
public: public:
Impl(const AddRRsetCallback& callback, Impl(const AddRRsetCallback& callback) : callback_(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, void addRR(const Name& name, const RRClass& rrclass,
const RRType& rrtype, const RRTTL& rrttl, const RRType& rrtype, const RRTTL& rrttl,
...@@ -50,8 +43,6 @@ public: ...@@ -50,8 +43,6 @@ public:
RRsetPtr current_rrset_; RRsetPtr current_rrset_;
AddRRsetCallback callback_; AddRRsetCallback callback_;
private:
MasterLoaderCallbacks issue_callback_;
}; };
namespace { namespace {
...@@ -88,20 +79,13 @@ RRCollator::Impl::addRR(const Name& name, const RRClass& rrclass, ...@@ -88,20 +79,13 @@ RRCollator::Impl::addRR(const Name& name, const RRClass& rrclass,
current_rrset_ = RRsetPtr(new RRset(name, rrclass, rrtype, rrttl)); current_rrset_ = RRsetPtr(new RRset(name, rrclass, rrtype, rrttl));
} else if (current_rrset_->getTTL() != rrttl) { } else if (current_rrset_->getTTL() != rrttl) {
// RRs with different TTLs are given. Smaller TTL should win. // RRs with different TTLs are given. Smaller TTL should win.
const RRTTL min_ttl(std::min(current_rrset_->getTTL(), rrttl)); current_rrset_->setTTL(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); current_rrset_->addRdata(rdata);
} }
RRCollator::RRCollator(const AddRRsetCallback& callback, RRCollator::RRCollator(const AddRRsetCallback& callback) :
const MasterLoaderCallbacks& issue_callback) : impl_(new Impl(callback))
impl_(new Impl(callback, issue_callback))
{} {}
RRCollator::~RRCollator() { RRCollator::~RRCollator() {
......
...@@ -63,30 +63,12 @@ public: ...@@ -63,30 +63,12 @@ public:
/// \brief Constructor. /// \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 /// \throw std::bad_alloc Internal memory allocation fails. This should
/// be very rare. /// be very rare.
/// ///
/// \param callback The callback functor to be called for each collated /// \param callback The callback functor to be called for each collated
/// RRset. /// RRset.
/// \param issue_callback The callbacks to be called on any issue found RRCollator(const AddRRsetCallback& callback);
/// in the collator.
RRCollator(const AddRRsetCallback& callback,
const MasterLoaderCallbacks& issue_callback =
MasterLoaderCallbacks::getNullCallbacks());
/// \brief Destructor. /// \brief Destructor.
/// ///
...@@ -103,7 +85,7 @@ public: ...@@ -103,7 +85,7 @@ public:
/// \brief Call the callback on the remaining RRset, if any. /// \brief Call the callback on the remaining RRset, if any.
/// ///
/// This method is expected to be called when it's supposed all RRs have /// This method is expected to be called that it's supposed all RRs have
/// been passed to this class object. Since there is no explicit /// 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 /// 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 /// explicitly call this method to call the callback for the last buffered
......
...@@ -25,16 +25,12 @@ ...@@ -25,16 +25,12 @@
#include <gtest/gtest.h> #include <gtest/gtest.h>
#include <boost/lexical_cast.hpp>
#include <boost/bind.hpp> #include <boost/bind.hpp>
#include <string>
#include <sstream> #include <sstream>
#include <vector> #include <vector>
using std::string;
using std::vector; using std::vector;
using boost::lexical_cast;
using namespace isc::dns; using namespace isc::dns;
using namespace isc::dns::rdata; using namespace isc::dns::rdata;
...@@ -54,14 +50,9 @@ addRRset(const RRsetPtr& rrset, vector<ConstRRsetPtr>* to_append, ...@@ -54,14 +50,9 @@ addRRset(const RRsetPtr& rrset, vector<ConstRRsetPtr>* to_append,
class RRCollatorTest : public ::testing::Test { class RRCollatorTest : public ::testing::Test {
protected: protected:
RRCollatorTest() : 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), origin_("example.com"), rrclass_(RRClass::IN()), rrttl_(3600),
throw_from_callback_(false), 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()), rr_callback_(collator_.getCallback()),
a_rdata1_(createRdata(RRType::A(), rrclass_, "192.0.2.1")), a_rdata1_(createRdata(RRType::A(), rrclass_, "192.0.2.1")),
a_rdata2_(createRdata(RRType::A(), rrclass_, "192.0.2.2")), a_rdata2_(createRdata(RRType::A(), rrclass_, "192.0.2.2")),
...@@ -74,12 +65,6 @@ protected: ...@@ -74,12 +65,6 @@ protected:
"12345 example.com. FAKE\n")) "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, void checkRRset(const Name& expected_name, const RRClass& expected_class,
const RRType& expected_type, const RRTTL& expected_ttl, const RRType& expected_type, const RRTTL& expected_ttl,
const vector<ConstRdataPtr>& expected_rdatas) { const vector<ConstRdataPtr>& expected_rdatas) {
...@@ -107,20 +92,6 @@ protected: ...@@ -107,20 +92,6 @@ protected:
rrsets_.clear(); 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 Name origin_;
const RRClass rrclass_; const RRClass rrclass_;
const RRTTL rrttl_; const RRTTL rrttl_;
...@@ -130,7 +101,6 @@ protected: ...@@ -130,7 +101,6 @@ protected:
AddRRCallback rr_callback_; AddRRCallback rr_callback_;
const RdataPtr a_rdata1_, a_rdata2_, txt_rdata_, sig_rdata1_, sig_rdata2_; const RdataPtr a_rdata1_, a_rdata2_, txt_rdata_, sig_rdata1_, sig_rdata2_;
vector<ConstRdataPtr> rdatas_; // placeholder for expected data vector<ConstRdataPtr> rdatas_; // placeholder for expected data
vector<string> warnings_, errors_;
}; };
TEST_F(RRCollatorTest, basicCases) { TEST_F(RRCollatorTest, basicCases) {
...@@ -164,9 +134,6 @@ TEST_F(RRCollatorTest, basicCases) { ...@@ -164,9 +134,6 @@ TEST_F(RRCollatorTest, basicCases) {
checkRRset(Name("txt.example.com"), rrclass_, RRType::TXT(), rrttl_, checkRRset(Name("txt.example.com"), rrclass_, RRType::TXT(), rrttl_,
rdatas_); 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. // Tell the collator we are done, then we'll see the last RR as an RRset.
collator_.flush(); collator_.flush();
checkRRset(Name("txt.example.com"), RRClass::CH(), RRType::TXT(), rrttl_, checkRRset(Name("txt.example.com"), RRClass::CH(), RRType::TXT(), rrttl_,
...@@ -180,16 +147,12 @@ TEST_F(RRCollatorTest, basicCases) { ...@@ -180,16 +147,12 @@ TEST_F(RRCollatorTest, basicCases) {
TEST_F(RRCollatorTest, minTTLFirst) { TEST_F(RRCollatorTest, minTTLFirst) {
// RRs of the same RRset but has different TTLs. The first RR has // 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. // 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(10), a_rdata1_);
rr_callback_(origin_, rrclass_, RRType::A(), RRTTL(20), a_rdata2_); rr_callback_(origin_, rrclass_, RRType::A(), RRTTL(20), a_rdata2_);
rdatas_.push_back(a_rdata1_); rdatas_.push_back(a_rdata1_);
rdatas_.push_back(a_rdata2_); rdatas_.push_back(a_rdata2_);
collator_.flush(); collator_.flush();
checkRRset(origin_, rrclass_, RRType::A(), RRTTL(10), rdatas_); 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) { TEST_F(RRCollatorTest, maxTTLFirst) {
...@@ -201,9 +164,6 @@ TEST_F(RRCollatorTest, maxTTLFirst) { ...@@ -201,9 +164,6 @@ TEST_F(RRCollatorTest, maxTTLFirst) {
rdatas_.push_back(a_rdata2_); rdatas_.push_back(a_rdata2_);
collator_.flush(); collator_.flush();
checkRRset(origin_, rrclass_, RRType::A(), RRTTL(10), rdatas_); 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) { TEST_F(RRCollatorTest, addRRSIGs) {
...@@ -237,25 +197,16 @@ TEST_F(RRCollatorTest, throwFromCallback) { ...@@ -237,25 +197,16 @@ TEST_F(RRCollatorTest, throwFromCallback) {
checkRRset(origin_, rrclass_, RRType::A(), rrttl_, rdatas_); 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_F(RRCollatorTest, withMasterLoader) {
// Test a simple case with MasterLoader. There shouldn't be anything // Test a simple case with MasterLoader. There shouldn't be anything
// special, but that's the mainly intended usage of the collator, so we // special, but that's the mainly intended usage of the collator, so we
// check it explicitly. Also, this test uses a different local collator, // check it explicitly.
// 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"); std::istringstream ss("example.com. 3600 IN A 192.0.2.1\n");
MasterLoader loader(ss, origin_, rrclass_, MasterLoader loader(ss, origin_, rrclass_,
MasterLoaderCallbacks::getNullCallbacks(), MasterLoaderCallbacks::getNullCallbacks(),
collator.getCallback()); collator_.getCallback());
loader.load(); loader.load();
collator.flush(); collator_.flush();
rdatas_.push_back(a_rdata1_); rdatas_.push_back(a_rdata1_);
checkRRset(origin_, rrclass_, RRType::A(), rrttl_, rdatas_); checkRRset(origin_, rrclass_, RRType::A(), rrttl_, rdatas_);
} }
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment