BIND 10 trac1068review2, updated. 83a58b817e5c0432d543b66208f502b059fdbe13 [1068] added a new test case that performs a bit more complicated update operations with multiple adds and deletes.
BIND 10 source code commits
bind10-changes at lists.isc.org
Wed Sep 7 01:06:27 UTC 2011
The branch, trac1068review2 has been updated
via 83a58b817e5c0432d543b66208f502b059fdbe13 (commit)
via 40126733cc69634035b0cca3a0c90ee3a606ea3b (commit)
via bcafb8b98d5df77108a83a6bd8b7746f7c2616d7 (commit)
via 4ef59f25a452f934408a9ba837cea9b7fab0be48 (commit)
via 3d069e2745070bc23f14c845cb7d8116d919f0da (commit)
via 230df584722d08705f2cb3b99940b764b1cb7865 (commit)
via fda403b09887a24403c3a90d7ad6c95288f2d641 (commit)
from 748c3e1aeb833012a19b651af7d98757a8ffc50f (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 83a58b817e5c0432d543b66208f502b059fdbe13
Author: JINMEI Tatuya <jinmei at isc.org>
Date: Tue Sep 6 17:45:29 2011 -0700
[1068] added a new test case that performs a bit more complicated update
operations with multiple adds and deletes.
commit 40126733cc69634035b0cca3a0c90ee3a606ea3b
Author: JINMEI Tatuya <jinmei at isc.org>
Date: Tue Sep 6 17:19:46 2011 -0700
[1068] some editorial fixes that happened to be found
commit bcafb8b98d5df77108a83a6bd8b7746f7c2616d7
Author: JINMEI Tatuya <jinmei at isc.org>
Date: Tue Sep 6 16:43:51 2011 -0700
[1068] added a note about possible future extensions to getUpdater()
of the in-memory data source client.
commit 4ef59f25a452f934408a9ba837cea9b7fab0be48
Author: JINMEI Tatuya <jinmei at isc.org>
Date: Tue Sep 6 16:22:40 2011 -0700
[1068] made sure Updater::committed_ is updated immediately after commit.
commit 3d069e2745070bc23f14c845cb7d8116d919f0da
Author: JINMEI Tatuya <jinmei at isc.org>
Date: Tue Sep 6 12:39:22 2011 -0700
[1068] used local arrays for columns/parameters of add/delete RRset instead
of class member variables (which are now removed). also added a test to
detect a bug in the previous code where the sigtype field for addRRset
was incorrectly carried over after adding an RRSIG.
commit 230df584722d08705f2cb3b99940b764b1cb7865
Author: JINMEI Tatuya <jinmei at isc.org>
Date: Tue Sep 6 12:10:42 2011 -0700
[1068] Throw an exception if an RRset is being added/deleted with RRSIG RRset.
Also updated doc and tests accordingly.
commit fda403b09887a24403c3a90d7ad6c95288f2d641
Author: JINMEI Tatuya <jinmei at isc.org>
Date: Tue Sep 6 11:47:19 2011 -0700
[1068] fixed a typo
-----------------------------------------------------------------------
Summary of changes:
src/lib/datasrc/database.cc | 40 +++++++-----
src/lib/datasrc/memory_datasrc.h | 8 ++-
src/lib/datasrc/tests/database_unittest.cc | 98 +++++++++++++++++++++++++++-
src/lib/datasrc/zone.h | 12 +++-
4 files changed, 138 insertions(+), 20 deletions(-)
-----------------------------------------------------------------------
diff --git a/src/lib/datasrc/database.cc b/src/lib/datasrc/database.cc
index f27c2ce..2c5aaeb 100644
--- a/src/lib/datasrc/database.cc
+++ b/src/lib/datasrc/database.cc
@@ -660,8 +660,6 @@ private:
const string zone_name_;
const RRClass zone_class_;
boost::scoped_ptr<DatabaseClient::Finder> finder_;
- string add_columns_[DatabaseAccessor::ADD_COLUMN_COUNT];
- string del_params_[DatabaseAccessor::DEL_PARAM_COUNT];
};
void
@@ -675,6 +673,11 @@ DatabaseUpdater::addRRset(const RRset& rrset) {
<< "added to " << zone_name_ << "/" << zone_class_ << ": "
<< rrset.toText());
}
+ if (rrset.getRRsig()) {
+ isc_throw(DataSourceError, "An RRset with RRSIG is being added to "
+ << zone_name_ << "/" << zone_class_ << ": "
+ << rrset.toText());
+ }
RdataIteratorPtr it = rrset.getRdataIterator();
if (it->isLast()) {
@@ -683,11 +686,12 @@ DatabaseUpdater::addRRset(const RRset& rrset) {
<< rrset.getType());
}
- add_columns_[DatabaseAccessor::ADD_NAME] = rrset.getName().toText();
- add_columns_[DatabaseAccessor::ADD_REV_NAME] =
+ string columns[DatabaseAccessor::ADD_COLUMN_COUNT]; // initialized with ""
+ columns[DatabaseAccessor::ADD_NAME] = rrset.getName().toText();
+ columns[DatabaseAccessor::ADD_REV_NAME] =
rrset.getName().reverse().toText();
- add_columns_[DatabaseAccessor::ADD_TTL] = rrset.getTTL().toText();
- add_columns_[DatabaseAccessor::ADD_TYPE] = rrset.getType().toText();
+ columns[DatabaseAccessor::ADD_TTL] = rrset.getTTL().toText();
+ columns[DatabaseAccessor::ADD_TYPE] = rrset.getType().toText();
for (; !it->isLast(); it->next()) {
if (rrset.getType() == RRType::RRSIG()) {
// XXX: the current interface (based on the current sqlite3
@@ -697,11 +701,11 @@ DatabaseUpdater::addRRset(const RRset& rrset) {
// the interface, but until then we have to conform to the schema.
const generic::RRSIG& rrsig_rdata =
dynamic_cast<const generic::RRSIG&>(it->getCurrent());
- add_columns_[DatabaseAccessor::ADD_SIGTYPE] =
+ columns[DatabaseAccessor::ADD_SIGTYPE] =
rrsig_rdata.typeCovered().toText();
}
- add_columns_[DatabaseAccessor::ADD_RDATA] = it->getCurrent().toText();
- accessor_->addRecordToZone(add_columns_);
+ columns[DatabaseAccessor::ADD_RDATA] = it->getCurrent().toText();
+ accessor_->addRecordToZone(columns);
}
}
@@ -716,6 +720,11 @@ DatabaseUpdater::deleteRRset(const RRset& rrset) {
<< "deleted from " << zone_name_ << "/" << zone_class_
<< ": " << rrset.toText());
}
+ if (rrset.getRRsig()) {
+ isc_throw(DataSourceError, "An RRset with RRSIG is being deleted from "
+ << zone_name_ << "/" << zone_class_ << ": "
+ << rrset.toText());
+ }
RdataIteratorPtr it = rrset.getRdataIterator();
if (it->isLast()) {
@@ -724,12 +733,12 @@ DatabaseUpdater::deleteRRset(const RRset& rrset) {
<< rrset.getType());
}
- del_params_[DatabaseAccessor::DEL_NAME] = rrset.getName().toText();
- del_params_[DatabaseAccessor::DEL_TYPE] = rrset.getType().toText();
+ string params[DatabaseAccessor::DEL_PARAM_COUNT]; // initialized with ""
+ params[DatabaseAccessor::DEL_NAME] = rrset.getName().toText();
+ params[DatabaseAccessor::DEL_TYPE] = rrset.getType().toText();
for (; !it->isLast(); it->next()) {
- del_params_[DatabaseAccessor::DEL_RDATA] =
- it->getCurrent().toText();
- accessor_->deleteRecordInZone(del_params_);
+ params[DatabaseAccessor::DEL_RDATA] = it->getCurrent().toText();
+ accessor_->deleteRecordInZone(params);
}
}
@@ -741,13 +750,12 @@ DatabaseUpdater::commit() {
<< db_name_);
}
accessor_->commitUpdateZone();
+ committed_ = true; // make sure the destructor won't trigger rollback
// We release the accessor immediately after commit is completed so that
// we don't hold the possible internal resource any longer.
accessor_.reset();
- committed_ = true;
-
logger.debug(DBG_TRACE_DATA, DATASRC_DATABASE_UPDATER_COMMIT)
.arg(zone_name_).arg(zone_class_).arg(db_name_);
}
diff --git a/src/lib/datasrc/memory_datasrc.h b/src/lib/datasrc/memory_datasrc.h
index be8cb8f..c569548 100644
--- a/src/lib/datasrc/memory_datasrc.h
+++ b/src/lib/datasrc/memory_datasrc.h
@@ -267,7 +267,13 @@ public:
virtual ZoneIteratorPtr getIterator(const isc::dns::Name& name) const;
/// In-memory data source is read-only, so this derived method will
- /// result in a NotImplemented (once merged) exception.
+ /// result in a NotImplemented exception.
+ ///
+ /// \note We plan to use a database-based data source as a backend
+ /// persistent storage for an in-memory data source. When it's
+ /// implemented we may also want to allow the user of the in-memory client
+ /// to update via its updater (this may or may not be a good idea and
+ /// is subject to further discussions).
virtual ZoneUpdaterPtr getUpdater(const isc::dns::Name& name,
bool replace) const;
diff --git a/src/lib/datasrc/tests/database_unittest.cc b/src/lib/datasrc/tests/database_unittest.cc
index 980b7f0..0e1ee36 100644
--- a/src/lib/datasrc/tests/database_unittest.cc
+++ b/src/lib/datasrc/tests/database_unittest.cc
@@ -402,7 +402,7 @@ public:
private:
// The following member variables are storage and/or update work space
// of the test zone. The "master"s are the real objects that contain
- // the data, and they are shared among by all accessors cloned from
+ // the data, and they are shared among all accessors cloned from
// an initially created one. The pointer members allow the sharing.
// "readonly" is for normal lookups. "update" is the workspace for
// updates. When update starts it will be initialized either as an
@@ -416,7 +416,6 @@ private:
const Domains* empty_records_;
// used as temporary storage during the building of the fake data
- //Domains records;
// used as temporary storage after searchForRecord() and during
// getNextRecord() calls, as well as during the building of the
@@ -1633,6 +1632,15 @@ TEST_F(DatabaseClientTest, addRRsetToNewZone) {
qtype_, rrttl_, ZoneFinder::SUCCESS,
expected_rdatas_, expected_sig_rdatas_);
}
+
+ // Add the non RRSIG RRset again, to see the attempt of adding RRSIG
+ // causes any unexpected effect, in particular, whether the SIGTYPE
+ // field might remain.
+ updater_->addRRset(*rrset_);
+ const char* const rrset_added[] = {
+ "www.example.org.", "org.example.www.", "3600", "A", "", "192.0.2.2"
+ };
+ checkLastAdded(rrset_added);
}
TEST_F(DatabaseClientTest, addRRsetToCurrentZone) {
@@ -1772,6 +1780,12 @@ TEST_F(DatabaseClientTest, addEmptyRRset) {
EXPECT_THROW(updater_->addRRset(*rrset_), DataSourceError);
}
+TEST_F(DatabaseClientTest, addRRsetWithRRSIG) {
+ updater_ = client_->getUpdater(zname_, false);
+ rrset_->addRRsig(*rrsigset_);
+ EXPECT_THROW(updater_->addRRset(*rrset_), DataSourceError);
+}
+
TEST_F(DatabaseClientTest, addAfterCommit) {
updater_ = client_->getUpdater(zname_, false);
updater_->commit();
@@ -1951,4 +1965,84 @@ TEST_F(DatabaseClientTest, deleteEmptyRRset) {
rrset_.reset(new RRset(qname_, qclass_, qtype_, rrttl_));
EXPECT_THROW(updater_->deleteRRset(*rrset_), DataSourceError);
}
+
+TEST_F(DatabaseClientTest, deleteRRsetWithRRSIG) {
+ updater_ = client_->getUpdater(zname_, false);
+ rrset_->addRRsig(*rrsigset_);
+ EXPECT_THROW(updater_->deleteRRset(*rrset_), DataSourceError);
+}
+
+TEST_F(DatabaseClientTest, compoundUpdate) {
+ // This test case performs an arbitrary chosen add/delete operations
+ // in a single update transaction. Essentially there is nothing new to
+ // test here, but there may be some bugs that was overlooked and can
+ // only happen in the compound update scenario, so we test it.
+
+ updater_ = client_->getUpdater(zname_, false);
+
+ // add a new RR to an existing RRset
+ updater_->addRRset(*rrset_);
+ expected_rdatas_.clear();
+ expected_rdatas_.push_back("192.0.2.1");
+ expected_rdatas_.push_back("192.0.2.2");
+ doFindTest(updater_->getFinder(), qname_, qtype_, qtype_, rrttl_,
+ ZoneFinder::SUCCESS, expected_rdatas_, empty_rdatas_);
+
+ // delete an existing RR
+ rrset_.reset(new RRset(Name("www.example.org"), qclass_, qtype_, rrttl_));
+ rrset_->addRdata(rdata::createRdata(rrset_->getType(),
+ rrset_->getClass(), "192.0.2.1"));
+ updater_->deleteRRset(*rrset_);
+ expected_rdatas_.clear();
+ expected_rdatas_.push_back("192.0.2.2");
+ doFindTest(updater_->getFinder(), qname_, qtype_, qtype_, rrttl_,
+ ZoneFinder::SUCCESS, expected_rdatas_, empty_rdatas_);
+
+ // re-add it
+ updater_->addRRset(*rrset_);
+ expected_rdatas_.push_back("192.0.2.1");
+ doFindTest(updater_->getFinder(), qname_, qtype_, qtype_, rrttl_,
+ ZoneFinder::SUCCESS, expected_rdatas_, empty_rdatas_);
+
+ // add a new RR with a new name
+ const Name newname("newname.example.org");
+ const RRType newtype(RRType::AAAA());
+ doFindTest(updater_->getFinder(), newname, newtype, newtype, rrttl_,
+ ZoneFinder::NXDOMAIN, empty_rdatas_, empty_rdatas_);
+ rrset_.reset(new RRset(newname, qclass_, newtype, rrttl_));
+ rrset_->addRdata(rdata::createRdata(rrset_->getType(),
+ rrset_->getClass(), "2001:db8::10"));
+ rrset_->addRdata(rdata::createRdata(rrset_->getType(),
+ rrset_->getClass(), "2001:db8::11"));
+ updater_->addRRset(*rrset_);
+ expected_rdatas_.clear();
+ expected_rdatas_.push_back("2001:db8::10");
+ expected_rdatas_.push_back("2001:db8::11");
+ doFindTest(updater_->getFinder(), newname, newtype, newtype, rrttl_,
+ ZoneFinder::SUCCESS, expected_rdatas_, empty_rdatas_);
+
+ // delete one RR from the previous set
+ rrset_.reset(new RRset(newname, qclass_, newtype, rrttl_));
+ rrset_->addRdata(rdata::createRdata(rrset_->getType(),
+ rrset_->getClass(), "2001:db8::11"));
+ updater_->deleteRRset(*rrset_);
+ expected_rdatas_.clear();
+ expected_rdatas_.push_back("2001:db8::10");
+ doFindTest(updater_->getFinder(), newname, newtype, newtype, rrttl_,
+ ZoneFinder::SUCCESS, expected_rdatas_, empty_rdatas_);
+
+ // Commit the changes, confirm the entire changes applied.
+ updater_->commit();
+ shared_ptr<DatabaseClient::Finder> finder(getFinder());
+ expected_rdatas_.clear();
+ expected_rdatas_.push_back("192.0.2.2");
+ expected_rdatas_.push_back("192.0.2.1");
+ doFindTest(*finder, qname_, qtype_, qtype_, rrttl_,
+ ZoneFinder::SUCCESS, expected_rdatas_, empty_rdatas_);
+
+ expected_rdatas_.clear();
+ expected_rdatas_.push_back("2001:db8::10");
+ doFindTest(*finder, newname, newtype, newtype, rrttl_,
+ ZoneFinder::SUCCESS, expected_rdatas_, empty_rdatas_);
+}
}
diff --git a/src/lib/datasrc/zone.h b/src/lib/datasrc/zone.h
index acbb4b5..b8d9fae 100644
--- a/src/lib/datasrc/zone.h
+++ b/src/lib/datasrc/zone.h
@@ -214,7 +214,7 @@ typedef boost::shared_ptr<const ZoneFinder> ConstZoneFinderPtr;
/// On construction, each derived class object will start a "transaction"
/// for making updates to a specific zone (this means a constructor of
/// a derived class would normally take parameters to identify the zone
-/// to be updated). The underlying realization of a "transaction" will defer
+/// to be updated). The underlying realization of a "transaction" will differ
/// for different derived classes; if it uses a general purpose database
/// as a backend, it will involve performing some form of "begin transaction"
/// statement for the database.
@@ -279,6 +279,8 @@ public:
/// It performs minimum level of validation on the specified RRset:
/// - Whether the RR class is identical to that for the zone to be updated
/// - Whether the RRset is not empty, i.e., it has at least one RDATA
+ /// - Whether the RRset is not associated with an RRSIG, i.e.,
+ /// whether \c getRRsig() on the RRset returns a NULL pointer.
///
/// and otherwise does not check any oddity. For example, it doesn't
/// check whether the owner name of the specified RRset is a subdomain
@@ -289,6 +291,12 @@ public:
/// the existing data beforehand using the \c ZoneFinder returned by
/// \c getFinder().
///
+ /// The validation requirement on the associated RRSIG is temporary.
+ /// If we find it more reasonable and useful to allow adding a pair of
+ /// RRset and its RRSIG RRset as we gain experiences with the interface,
+ /// we may remove this restriction. Until then we explicitly check it
+ /// to prevent accidental misuse.
+ ///
/// Conceptually, on successful call to this method, the zone will have
/// the specified RRset, and if there is already an RRset of the same
/// name and RR type, these two sets will be "merged". "Merged" means
@@ -368,6 +376,8 @@ public:
/// RRset:
/// - Whether the RR class is identical to that for the zone to be updated
/// - Whether the RRset is not empty, i.e., it has at least one RDATA
+ /// - Whether the RRset is not associated with an RRSIG, i.e.,
+ /// whether \c getRRsig() on the RRset returns a NULL pointer.
///
/// This method must not be called once commit() is performed. If it
/// calls after \c commit() the implementation must throw a
More information about the bind10-changes
mailing list