BIND 10 trac1331, updated. 0d94cca23a4f22d1bb953d62d38358a8b0e49f01 [1331] Code cleanups
BIND 10 source code commits
bind10-changes at lists.isc.org
Fri Nov 11 14:40:32 UTC 2011
The branch, trac1331 has been updated
via 0d94cca23a4f22d1bb953d62d38358a8b0e49f01 (commit)
via 4215dabae27f7b9b089ff8fafef2ba5425062fc5 (commit)
via 219879a5c8d6cb361d6d6f91d88c199930560994 (commit)
via 7003eecf6f7792d140e74bac444fb00eb7b8415b (commit)
from 7c6c725225eb89d9911b28aff0c6d80152e26aaf (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 0d94cca23a4f22d1bb953d62d38358a8b0e49f01
Author: Michal 'vorner' Vaner <michal.vaner at nic.cz>
Date: Fri Nov 11 15:40:19 2011 +0100
[1331] Code cleanups
commit 4215dabae27f7b9b089ff8fafef2ba5425062fc5
Author: Michal 'vorner' Vaner <michal.vaner at nic.cz>
Date: Fri Nov 11 15:30:26 2011 +0100
[1331] Don't impose too much flexibility
commit 219879a5c8d6cb361d6d6f91d88c199930560994
Author: Michal 'vorner' Vaner <michal.vaner at nic.cz>
Date: Fri Nov 11 15:17:56 2011 +0100
[1331] Propagate exception
commit 7003eecf6f7792d140e74bac444fb00eb7b8415b
Author: JINMEI Tatuya <jinmei at isc.org>
Date: Fri Nov 11 15:13:21 2011 +0100
[1331] Put common code to a function
-----------------------------------------------------------------------
Summary of changes:
src/lib/datasrc/client.h | 9 +-
src/lib/datasrc/database.cc | 169 ++++++++++++---------------
src/lib/datasrc/tests/database_unittest.cc | 14 ++-
src/lib/datasrc/zone.h | 8 +-
4 files changed, 92 insertions(+), 108 deletions(-)
-----------------------------------------------------------------------
diff --git a/src/lib/datasrc/client.h b/src/lib/datasrc/client.h
index 8fa2ca1..7a5f4f0 100644
--- a/src/lib/datasrc/client.h
+++ b/src/lib/datasrc/client.h
@@ -270,11 +270,12 @@ public:
/// However, the parameter is a hint only. It might be unable to store
/// them and they would be silently discarded. Or it might need to
/// store them no matter what (for example a git-based data source would
- /// store journal implicitly). When the \c journaling is true, it might
- /// require that the following update be formatted as IXFR transfer
+ /// store journal implicitly). When the \c journaling is true, it
+ /// requires that the following update be formatted as IXFR transfer
/// (SOA to be removed, bunch of RRs to be removed, SOA to be added,
- /// bunch of RRs to be added, and possibly repeated). If it is false, it
- /// must not require so.
+ /// bunch of RRs to be added, and possibly repeated). However, it is not
+ /// required that the updater checks that. If it is false, it must not
+ /// require so and must accept any order of changes.
///
/// We don't support erasing the whole zone (by replace being true) and
/// saving a journal at the same time. In such situation, BadValue is
diff --git a/src/lib/datasrc/database.cc b/src/lib/datasrc/database.cc
index 8d4685e..c745725 100644
--- a/src/lib/datasrc/database.cc
+++ b/src/lib/datasrc/database.cc
@@ -868,6 +868,9 @@ public:
virtual void commit();
private:
+ // A short cut typedef only for making the code shorter.
+ typedef DatabaseAccessor Accessor;
+
bool committed_;
shared_ptr<DatabaseAccessor> accessor_;
const int zone_id_;
@@ -884,63 +887,75 @@ private:
DiffPhase diff_phase_;
uint32_t serial_;
boost::scoped_ptr<DatabaseClient::Finder> finder_;
+
+ // This is a set of validation checks commonly used for addRRset() and
+ // deleteRRset to minimize duplicate code logic and to make the main
+ // code concise.
+ void validateAddOrDelete(const char* const op_str, const RRset& rrset,
+ DiffPhase prev_phase,
+ DiffPhase current_phase) const;
};
void
-DatabaseUpdater::addRRset(const RRset& rrset) {
+DatabaseUpdater::validateAddOrDelete(const char* const op_str,
+ const RRset& rrset,
+ DiffPhase prev_phase,
+ DiffPhase current_phase) const
+{
if (committed_) {
- isc_throw(DataSourceError, "Add attempt after commit to zone: "
+ isc_throw(DataSourceError, op_str << " attempt after commit to zone: "
<< zone_name_ << "/" << zone_class_);
}
+ if (rrset.getRdataCount() == 0) {
+ isc_throw(DataSourceError, op_str << " attempt with an empty RRset: "
+ << rrset.getName() << "/" << zone_class_ << "/"
+ << rrset.getType());
+ }
if (rrset.getClass() != zone_class_) {
- isc_throw(DataSourceError, "An RRset of a different class is being "
- << "added to " << zone_name_ << "/" << zone_class_ << ": "
+ isc_throw(DataSourceError, op_str << " attempt for a different class "
+ << zone_name_ << "/" << zone_class_ << ": "
<< rrset.toText());
}
if (rrset.getRRsig()) {
- isc_throw(DataSourceError, "An RRset with RRSIG is being added to "
+ isc_throw(DataSourceError, op_str << " attempt for RRset with RRSIG "
<< zone_name_ << "/" << zone_class_ << ": "
<< rrset.toText());
}
- if (rrset.getType() == RRType::SOA() && diff_phase_ == ADD &&
- journaling_) {
- isc_throw(isc::BadValue, "Another SOA added inside an add sequence");
- }
- if (rrset.getType() != RRType::SOA() && diff_phase_ != ADD &&
- journaling_) {
- isc_throw(isc::BadValue, "Adding sequence not started by SOA");
- }
- if (rrset.getType() == RRType::SOA() && diff_phase_ != DELETE &&
- journaling_) {
- isc_throw(isc::BadValue,
- "Adding sequence can follow only after delete");
+ if (journaling_) {
+ const RRType rrtype(rrset.getType());
+ if (rrtype == RRType::SOA() && diff_phase_ != prev_phase) {
+ isc_throw(isc::BadValue, op_str << " attempt in an invalid "
+ << "diff phase: " << diff_phase_ << ", rrset: " <<
+ rrset.toText());
+ }
+ if (rrtype != RRType::SOA() && diff_phase_ != current_phase) {
+ isc_throw(isc::BadValue, "diff state change by non SOA: "
+ << rrset.toText());
+ }
}
+}
+
+void
+DatabaseUpdater::addRRset(const RRset& rrset) {
+ validateAddOrDelete("add", rrset, DELETE, ADD);
+ // It's guaranteed rrset has at least one RDATA at this point.
RdataIteratorPtr it = rrset.getRdataIterator();
- if (it->isLast()) {
- isc_throw(DataSourceError, "An empty RRset is being added for "
- << rrset.getName() << "/" << zone_class_ << "/"
- << rrset.getType());
- }
- string columns[DatabaseAccessor::ADD_COLUMN_COUNT]; // initialized with ""
- columns[DatabaseAccessor::ADD_NAME] = rrset.getName().toText();
- columns[DatabaseAccessor::ADD_REV_NAME] =
- rrset.getName().reverse().toText();
- columns[DatabaseAccessor::ADD_TTL] = rrset.getTTL().toText();
- columns[DatabaseAccessor::ADD_TYPE] = rrset.getType().toText();
- string journal[DatabaseAccessor::DIFF_PARAM_COUNT];
+ string columns[Accessor::ADD_COLUMN_COUNT]; // initialized with ""
+ columns[Accessor::ADD_NAME] = rrset.getName().toText();
+ columns[Accessor::ADD_REV_NAME] = rrset.getName().reverse().toText();
+ columns[Accessor::ADD_TTL] = rrset.getTTL().toText();
+ columns[Accessor::ADD_TYPE] = rrset.getType().toText();
+ string journal[Accessor::DIFF_PARAM_COUNT];
if (journaling_) {
- journal[DatabaseAccessor::DIFF_NAME] =
- columns[DatabaseAccessor::ADD_NAME];
- journal[DatabaseAccessor::DIFF_TYPE] =
- columns[DatabaseAccessor::ADD_TYPE];
- journal[DatabaseAccessor::DIFF_TTL] =
- columns[DatabaseAccessor::ADD_TTL];
+ journal[Accessor::DIFF_NAME] = columns[Accessor::ADD_NAME];
+ journal[Accessor::DIFF_TYPE] = columns[Accessor::ADD_TYPE];
+ journal[Accessor::DIFF_TTL] = columns[Accessor::ADD_TTL];
diff_phase_ = ADD;
if (rrset.getType() == RRType::SOA()) {
serial_ =
- dynamic_cast<const rdata::generic::SOA&>(it->getCurrent()).
+ dynamic_cast<const generic::SOA&>(it->getCurrent()).
getSerial();
}
}
@@ -953,19 +968,14 @@ 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());
- columns[DatabaseAccessor::ADD_SIGTYPE] =
+ columns[Accessor::ADD_SIGTYPE] =
rrsig_rdata.typeCovered().toText();
}
- columns[DatabaseAccessor::ADD_RDATA] = it->getCurrent().toText();
+ columns[Accessor::ADD_RDATA] = it->getCurrent().toText();
if (journaling_) {
- journal[DatabaseAccessor::DIFF_RDATA] =
- columns[DatabaseAccessor::ADD_RDATA];
- try {
- accessor_->addRecordDiff(zone_id_, serial_,
- DatabaseAccessor::DIFF_ADD, journal);
- }
- // We ignore not implemented
- catch (const isc::NotImplemented&) {}
+ journal[Accessor::DIFF_RDATA] = columns[Accessor::ADD_RDATA];
+ accessor_->addRecordDiff(zone_id_, serial_, Accessor::DIFF_ADD,
+ journal);
}
accessor_->addRecordToZone(columns);
}
@@ -973,66 +983,37 @@ DatabaseUpdater::addRRset(const RRset& rrset) {
void
DatabaseUpdater::deleteRRset(const RRset& rrset) {
- if (committed_) {
- isc_throw(DataSourceError, "Delete attempt after commit on zone: "
- << zone_name_ << "/" << zone_class_);
- }
- if (rrset.getClass() != zone_class_) {
- isc_throw(DataSourceError, "An RRset of a different class is being "
- << "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());
- }
- if (rrset.getType() == RRType::SOA() && diff_phase_ == DELETE &&
- journaling_) {
- isc_throw(isc::BadValue,
- "Another SOA delete inside a delete sequence");
- }
- if (rrset.getType() != RRType::SOA() && diff_phase_ != DELETE &&
- journaling_) {
- isc_throw(isc::BadValue, "Delete sequence not started by SOA");
+ // If this is the first operation, pretend we are starting a new delete
+ // sequence after adds. This will simplify the validation below.
+ if (diff_phase_ == NOT_STARTED) {
+ diff_phase_ = ADD;
}
+ validateAddOrDelete("delete", rrset, ADD, DELETE);
+
RdataIteratorPtr it = rrset.getRdataIterator();
- if (it->isLast()) {
- isc_throw(DataSourceError, "An empty RRset is being deleted for "
- << rrset.getName() << "/" << zone_class_ << "/"
- << rrset.getType());
- }
- string params[DatabaseAccessor::DEL_PARAM_COUNT]; // initialized with ""
- params[DatabaseAccessor::DEL_NAME] = rrset.getName().toText();
- params[DatabaseAccessor::DEL_TYPE] = rrset.getType().toText();
- string journal[DatabaseAccessor::DIFF_PARAM_COUNT];
+ string params[Accessor::DEL_PARAM_COUNT]; // initialized with ""
+ params[Accessor::DEL_NAME] = rrset.getName().toText();
+ params[Accessor::DEL_TYPE] = rrset.getType().toText();
+ string journal[Accessor::DIFF_PARAM_COUNT];
if (journaling_) {
- journal[DatabaseAccessor::DIFF_NAME] =
- params[DatabaseAccessor::DEL_NAME];
- journal[DatabaseAccessor::DIFF_TYPE] =
- params[DatabaseAccessor::DEL_TYPE];
- journal[DatabaseAccessor::DIFF_TTL] = rrset.getTTL().toText();
+ journal[Accessor::DIFF_NAME] = params[Accessor::DEL_NAME];
+ journal[Accessor::DIFF_TYPE] = params[Accessor::DEL_TYPE];
+ journal[Accessor::DIFF_TTL] = rrset.getTTL().toText();
diff_phase_ = DELETE;
if (rrset.getType() == RRType::SOA()) {
serial_ =
- dynamic_cast<const rdata::generic::SOA&>(it->getCurrent()).
+ dynamic_cast<const generic::SOA&>(it->getCurrent()).
getSerial();
}
}
for (; !it->isLast(); it->next()) {
- params[DatabaseAccessor::DEL_RDATA] = it->getCurrent().toText();
+ params[Accessor::DEL_RDATA] = it->getCurrent().toText();
if (journaling_) {
- journal[DatabaseAccessor::DIFF_RDATA] =
- params[DatabaseAccessor::DEL_RDATA];
- try {
- accessor_->addRecordDiff(zone_id_, serial_,
- DatabaseAccessor::DIFF_DELETE,
- journal);
- }
- // Don't fail if the backend can't store them
- catch(const isc::NotImplemented&) {}
+ journal[Accessor::DIFF_RDATA] = params[Accessor::DEL_RDATA];
+ accessor_->addRecordDiff(zone_id_, serial_, Accessor::DIFF_DELETE,
+ journal);
}
accessor_->deleteRecordInZone(params);
}
@@ -1046,7 +1027,7 @@ DatabaseUpdater::commit() {
<< db_name_);
}
if (journaling_ && diff_phase_ == DELETE) {
- isc_throw(isc::Unexpected, "Update sequence not complete");
+ isc_throw(isc::BatValue, "Update sequence not complete");
}
accessor_->commit();
committed_ = true; // make sure the destructor won't trigger rollback
diff --git a/src/lib/datasrc/tests/database_unittest.cc b/src/lib/datasrc/tests/database_unittest.cc
index 6ac7a61..a384a41 100644
--- a/src/lib/datasrc/tests/database_unittest.cc
+++ b/src/lib/datasrc/tests/database_unittest.cc
@@ -301,12 +301,14 @@ struct JournalEntry {
DatabaseAccessor::DiffOperation operation_;
std::string data_[DatabaseAccessor::DIFF_PARAM_COUNT];
bool operator==(const JournalEntry& other) const {
- bool data_equal(true);
for (size_t i(0); i < DatabaseAccessor::DIFF_PARAM_COUNT; ++ i) {
- data_equal = data_equal && (data_[i] == other.data_[i]);
+ if (data_[i] != other.data_[i]) {
+ return false;
+ }
}
+ // No need to check data here, checked above
return (id_ == other.id_ && serial_ == other.serial_ &&
- operation_ == other.operation_ && data_equal);
+ operation_ == other.operation_);
}
};
@@ -2871,16 +2873,16 @@ TEST_F(MockDatabaseClientTest, journalOnErase) {
}
/*
- * Check that the exception isn't raised when the journal is not implemented.
+ * Check that exception is propagated when the journal is not implemented.
*/
TEST_F(MockDatabaseClientTest, journalNotImplemented) {
updater_ = client_->getUpdater(Name("null.example.org"), false, true);
- EXPECT_NO_THROW(updater_->deleteRRset(*soa_));
+ EXPECT_THROW(updater_->deleteRRset(*soa_), isc::NotImplemented);
soa_.reset(new RRset(zname_, qclass_, RRType::SOA(), rrttl_));
soa_->addRdata(rdata::createRdata(soa_->getType(), soa_->getClass(),
"ns1.example.org. admin.example.org. "
"1234 3600 1800 2419201 7200"));
- EXPECT_NO_THROW(updater_->addRRset(*soa_));
+ EXPECT_THROW(updater_->addRRset(*soa_), isc::NotImplemented);
}
/*
diff --git a/src/lib/datasrc/zone.h b/src/lib/datasrc/zone.h
index 65cec7e..94a95e1 100644
--- a/src/lib/datasrc/zone.h
+++ b/src/lib/datasrc/zone.h
@@ -439,8 +439,8 @@ public:
/// \c DataSourceError exception.
///
/// If journaling was requested when getting this updater, it might reject
- /// to add the RRset if the squence doesn't look like and IXFR. In such
- /// case isc::BadValue is thrown.
+ /// to add the RRset if the squence doesn't look like and IXFR (see
+ /// DataSourceClient::getUpdater). In such case isc::BadValue is thrown.
///
/// \todo As noted above we may have to revisit the design details as we
/// gain experiences:
@@ -510,8 +510,8 @@ public:
/// \c DataSourceError exception.
///
/// If journaling was requested when getting this updater, it might reject
- /// to add the RRset if the squence doesn't look like and IXFR. In such
- /// case isc::BadValue is thrown.
+ /// to add the RRset if the squence doesn't look like and IXFR (see
+ /// DataSourceClient::getUpdater). In such case isc::BadValue is thrown.
///
/// \todo As noted above we may have to revisit the design details as we
/// gain experiences:
More information about the bind10-changes
mailing list