BIND 10 master, updated. e906f116b8125b58fefb62a14281bbf36b2d7941 [master] Merge branch 'trac2575'
BIND 10 source code commits
bind10-changes at lists.isc.org
Fri Jan 11 17:07:22 UTC 2013
The branch, master has been updated
via e906f116b8125b58fefb62a14281bbf36b2d7941 (commit)
via c9d46273b40b9be9902deaa081be0f1216d287c6 (commit)
via 84bc0b71b8c31013d702c206a3fb1b4d1b93e514 (commit)
via e567eee7d939fa75ac7ccee625178e179e47ad98 (commit)
via 3072c500b4c563cb22b7cdae805cdb6c1e99a408 (commit)
from 8e5e30b800b786cbcf8ff12db820bb1a7c75d69f (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 e906f116b8125b58fefb62a14281bbf36b2d7941
Merge: 8e5e30b c9d4627
Author: JINMEI Tatuya <jinmei at isc.org>
Date: Fri Jan 11 08:58:19 2013 -0800
[master] Merge branch 'trac2575'
-----------------------------------------------------------------------
Summary of changes:
src/lib/datasrc/client.cc | 14 +-
src/lib/datasrc/client.h | 36 ++++-
src/lib/datasrc/database.cc | 18 ++-
src/lib/datasrc/database.h | 33 ++++-
src/lib/datasrc/sqlite3_accessor.cc | 20 ++-
src/lib/datasrc/sqlite3_accessor.h | 4 +
src/lib/datasrc/tests/database_unittest.cc | 153 ++++++++++++++------
src/lib/datasrc/tests/sqlite3_accessor_unittest.cc | 48 ++++++
8 files changed, 264 insertions(+), 62 deletions(-)
-----------------------------------------------------------------------
diff --git a/src/lib/datasrc/client.cc b/src/lib/datasrc/client.cc
index 919e9ab..c69b23c 100644
--- a/src/lib/datasrc/client.cc
+++ b/src/lib/datasrc/client.cc
@@ -28,20 +28,22 @@ namespace datasrc {
ZoneIteratorPtr
DataSourceClient::getIterator(const isc::dns::Name&, bool) const {
- isc_throw(isc::NotImplemented,
- "Data source doesn't support iteration");
+ isc_throw(isc::NotImplemented, "Data source doesn't support iteration");
}
unsigned int
DataSourceClient::getZoneCount() const {
- isc_throw(isc::NotImplemented,
- "Data source doesn't support getZoneCount");
+ isc_throw(isc::NotImplemented, "Data source doesn't support getZoneCount");
}
bool
DataSourceClient::createZone(const dns::Name&) {
- isc_throw(isc::NotImplemented,
- "Data source doesn't support addZone");
+ isc_throw(isc::NotImplemented, "Data source doesn't support createZone");
+}
+
+bool
+DataSourceClient::deleteZone(const dns::Name&) {
+ isc_throw(isc::NotImplemented, "Data source doesn't support deleteZone");
}
} // end namespace datasrc
diff --git a/src/lib/datasrc/client.h b/src/lib/datasrc/client.h
index 8739489..607af05 100644
--- a/src/lib/datasrc/client.h
+++ b/src/lib/datasrc/client.h
@@ -385,9 +385,41 @@ public:
/// direct zone creation.
/// \throw DataSourceError If something goes wrong in the data source
/// while creating the zone.
- /// \param name The (fully qualified) name of the zone to create
+ /// \param zone_name The (fully qualified) name of the zone to create
/// \return True if the zone was added, false if it already existed
- virtual bool createZone(const dns::Name& name);
+ virtual bool createZone(const dns::Name& zone_name);
+
+ /// \brief Delete a zone from the data source
+ ///
+ /// This method also checks if the specified zone exists in the data
+ /// source, and returns true/false depending on whether the zone
+ /// existed/not existed, respectively. In either case, on successful
+ /// return it ensures the data source does not contain the specified
+ /// name of the zone.
+ ///
+ /// \note This is a tentative API, and this method is likely to change
+ /// or be removed in the near future. For that reason, it currently
+ /// provides a default implementation that throws NotImplemented.
+ /// Note also that this method does not delete other database records
+ /// related to the zone, such as zone's resource records or differences
+ /// corresponding to updates made in the zone. This is primarily for
+ /// implementation simplicity (in the currently intended usage there
+ /// wouldn't be such other data at the time of this call anyway) and due
+ /// to the fact that details of managing zones is still in flux. Once
+ /// the design in this area is fixed we may revisit the behavior.
+ ///
+ /// Apart from the two exceptions mentioned below, in theory this
+ /// call can throw anything, depending on the implementation of
+ /// the datasource backend.
+ ///
+ /// \throw NotImplemented If the datasource backend does not support
+ /// direct zone deletion.
+ /// \throw DataSourceError If something goes wrong in the data source
+ /// while deleting the zone.
+ /// \param zone_name The (fully qualified) name of the zone to be deleted
+ /// \return true if the zone previously existed and has been deleted by
+ /// this method; false if the zone didn't exist.
+ virtual bool deleteZone(const dns::Name& zone_name);
};
}
}
diff --git a/src/lib/datasrc/database.cc b/src/lib/datasrc/database.cc
index f377ffe..b5117e0 100644
--- a/src/lib/datasrc/database.cc
+++ b/src/lib/datasrc/database.cc
@@ -117,13 +117,25 @@ DatabaseClient::findZone(const Name& name) const {
}
bool
-DatabaseClient::createZone(const Name& name) {
+DatabaseClient::createZone(const Name& zone_name) {
TransactionHolder transaction(*accessor_);
- std::pair<bool, int> zone(accessor_->getZone(name.toText()));
+ const std::pair<bool, int> zone(accessor_->getZone(zone_name.toText()));
if (zone.first) {
return (false);
}
- accessor_->addZone(name.toText());
+ accessor_->addZone(zone_name.toText());
+ transaction.commit();
+ return (true);
+}
+
+bool
+DatabaseClient::deleteZone(const Name& zone_name) {
+ TransactionHolder transaction(*accessor_);
+ const std::pair<bool, int> zinfo(accessor_->getZone(zone_name.toText()));
+ if (!zinfo.first) { // if it doesn't exist just return false
+ return (false);
+ }
+ accessor_->deleteZone(zinfo.second);
transaction.commit();
return (true);
}
diff --git a/src/lib/datasrc/database.h b/src/lib/datasrc/database.h
index 9db8a8f..3302adf 100644
--- a/src/lib/datasrc/database.h
+++ b/src/lib/datasrc/database.h
@@ -155,7 +155,7 @@ public:
///
/// It is empty, but needs a virtual one, since we will use the derived
/// classes in polymorphic way.
- virtual ~DatabaseAccessor() { }
+ virtual ~DatabaseAccessor() {}
/// \brief Retrieve a zone identifier
///
@@ -164,8 +164,8 @@ public:
/// apex), as the DatabaseClient will loop trough the labels itself and
/// find the most suitable zone.
///
- /// It is not specified if and what implementation of this method may throw,
- /// so code should expect anything.
+ /// It is not specified if and what implementation of this method may
+ /// throw, so code should expect anything.
///
/// \param name The (fully qualified) domain name of the zone's apex to be
/// looked up.
@@ -195,6 +195,23 @@ public:
/// or was created by this call).
virtual int addZone(const std::string& name) = 0;
+ /// \brief Delete a zone from the database
+ ///
+ /// Like for deleteRecordToZone, implementations are not required to
+ /// check for the existence of the given zone name, it is the
+ /// responsibility of the caller to do so.
+ ///
+ /// Callers must also start a transaction before calling this method.
+ /// Implementations should throw InvalidOperation if this has not been
+ /// done. Callers should also expect DataSourceError for other potential
+ /// problems specific to the database.
+ ///
+ /// \note This method does not delete other database records related to
+ /// the zone. See \c DataSourceClient::deleteZone for the rationale.
+ ///
+ /// \param zone_id The ID of the zone, that would be returned by getZone().
+ virtual void deleteZone(int zone_id) = 0;
+
/// \brief This holds the internal context of ZoneIterator for databases
///
/// While the ZoneIterator implementation from DatabaseClient does all the
@@ -212,15 +229,15 @@ public:
/// \brief Destructor
///
/// Virtual destructor, so any descendand class is destroyed correctly.
- virtual ~IteratorContext() { }
+ virtual ~IteratorContext() {}
/// \brief Function to provide next resource record
///
/// This function should provide data about the next resource record
/// from the data that is searched. The data is not converted yet.
///
- /// Depending on how the iterator was constructed, there is a difference
- /// in behaviour; for a 'full zone iterator', created with
+ /// Depending on how the iterator was constructed, there is a
+ /// difference in behaviour; for a 'full zone iterator', created with
/// getAllRecords(), all COLUMN_COUNT elements of the array are
/// overwritten.
/// For a 'name iterator', created with getRecords(), the column
@@ -1399,7 +1416,9 @@ public:
/// does not, creates it, commits, and returns true. If the zone
/// does exist already, it does nothing (except abort the transaction)
/// and returns false.
- virtual bool createZone(const isc::dns::Name& name);
+ virtual bool createZone(const isc::dns::Name& zone_name);
+
+ virtual bool deleteZone(const isc::dns::Name& zone_name);
/// \brief Get the zone iterator
///
diff --git a/src/lib/datasrc/sqlite3_accessor.cc b/src/lib/datasrc/sqlite3_accessor.cc
index 4ef49d0..bd71544 100644
--- a/src/lib/datasrc/sqlite3_accessor.cc
+++ b/src/lib/datasrc/sqlite3_accessor.cc
@@ -79,7 +79,8 @@ enum StatementID {
DEL_ZONE_NSEC3_RECORDS = 20,
DEL_NSEC3_RECORD = 21,
ADD_ZONE = 22,
- NUM_STATEMENTS = 23
+ DELETE_ZONE = 23,
+ NUM_STATEMENTS = 24
};
const char* const text_statements[NUM_STATEMENTS] = {
@@ -165,7 +166,9 @@ const char* const text_statements[NUM_STATEMENTS] = {
"AND rdtype=?3 AND rdata=?4",
// ADD_ZONE: add a zone to the zones table
- "INSERT INTO zones (name, rdclass) VALUES (?1, ?2)" // ADD_ZONE
+ "INSERT INTO zones (name, rdclass) VALUES (?1, ?2)", // ADD_ZONE
+ // DELETE_ZONE: delete a zone from the zones table
+ "DELETE FROM zones WHERE id=?1" // DELETE_ZONE
};
struct SQLite3Parameters {
@@ -643,6 +646,19 @@ SQLite3Accessor::addZone(const std::string& name) {
return (getzone_result.second);
}
+void
+SQLite3Accessor::deleteZone(int zone_id) {
+ // Transaction should have been started by the caller
+ if (!dbparameters_->in_transaction) {
+ isc_throw(InvalidOperation, "performing deleteZone on SQLite3 "
+ "data source without transaction");
+ }
+
+ StatementProcessor proc(*dbparameters_, DELETE_ZONE, "delete zone");
+ proc.bindInt(1, zone_id);
+ proc.exec();
+}
+
namespace {
// Conversion to plain char
diff --git a/src/lib/datasrc/sqlite3_accessor.h b/src/lib/datasrc/sqlite3_accessor.h
index c5773d0..d014193 100644
--- a/src/lib/datasrc/sqlite3_accessor.h
+++ b/src/lib/datasrc/sqlite3_accessor.h
@@ -139,6 +139,10 @@ public:
/// \return the id of the zone that has been added
virtual int addZone(const std::string& name);
+ // Nothing special to add for this implementation (the base class
+ // description is sufficient).
+ virtual void deleteZone(int zone_id);
+
/// \brief Look up all resource records for a name
///
/// This implements the getRecords() method from DatabaseAccessor
diff --git a/src/lib/datasrc/tests/database_unittest.cc b/src/lib/datasrc/tests/database_unittest.cc
index 174da04..06b3874 100644
--- a/src/lib/datasrc/tests/database_unittest.cc
+++ b/src/lib/datasrc/tests/database_unittest.cc
@@ -55,6 +55,7 @@ namespace {
// Imaginary zone IDs used in the mock accessor below.
const int READONLY_ZONE_ID = 42;
+const int NEW_ZONE_ID = 420;
const int WRITABLE_ZONE_ID = 4200;
// Commonly used test data
@@ -257,26 +258,43 @@ const char* TEST_NSEC3_RECORDS[][5] = {
*/
class NopAccessor : public DatabaseAccessor {
public:
- NopAccessor() : database_name_("mock_database")
- { }
+ NopAccessor() : database_name_("mock_database") {
+ zones_["example.org."] = READONLY_ZONE_ID;
+ zones_["null.example.org."] = 13;
+ zones_["empty.example.org."] = 0;
+ zones_["bad.example.org."] = -1;
+ }
virtual std::pair<bool, int> getZone(const std::string& name) const {
- if (name == "example.org.") {
- return (std::pair<bool, int>(true, READONLY_ZONE_ID));
- } else if (name == "null.example.org.") {
- return (std::pair<bool, int>(true, 13));
- } else if (name == "empty.example.org.") {
- return (std::pair<bool, int>(true, 0));
- } else if (name == "bad.example.org.") {
- return (std::pair<bool, int>(true, -1));
+ std::map<std::string, int>::const_iterator found = zones_.find(name);
+ if (found != zones_.end()) {
+ return (std::pair<bool, int>(true, found->second));
} else {
return (std::pair<bool, int>(false, 0));
}
}
- virtual int addZone(const std::string&) {
- isc_throw(isc::NotImplemented,
- "This database datasource can't add zones");
+ // A simple implementation of addZone.
+ virtual int addZone(const std::string& zone_name) {
+ if (zone_name == "example.com.") {
+ zones_[zone_name] = NEW_ZONE_ID;
+ }
+
+ // for simplicity we assume zone_name is in zones_ at this point
+ return (zones_[zone_name]);
+ }
+
+ // A simple implementation of deleteZone.
+ virtual void deleteZone(int zone_id) {
+ std::map<std::string, int>::iterator it = zones_.begin();
+ std::map<std::string, int>::iterator end = zones_.end();
+ while (it != end) {
+ if (it->second == zone_id) {
+ zones_.erase(it);
+ return;
+ }
+ ++it;
+ }
}
virtual boost::shared_ptr<DatabaseAccessor> clone() {
@@ -337,7 +355,7 @@ public:
private:
const std::string database_name_;
-
+ std::map<std::string, int> zones_;
};
/**
@@ -438,7 +456,7 @@ public:
// Check any attempt of multiple transactions
if (did_transaction_) {
- isc_throw(isc::Unexpected, "MockAccessor::startTransaction() "
+ isc_throw(DataSourceError, "MockAccessor::startTransaction() "
"called multiple times - likely a bug in the test");
}
@@ -447,6 +465,14 @@ public:
did_transaction_ = true;
}
+ // If the test needs multiple calls to startTransaction() and knows it's
+ // safe, it can use this method to disable the safeguard check in
+ // startTransaction(); the test can also use this method by emulating a
+ // lock conflict by setting is_allowed to false.
+ void allowMoreTransaction(bool is_allowed) {
+ did_transaction_ = !is_allowed;
+ }
+
private:
class DomainIterator : public IteratorContext {
public:
@@ -1293,6 +1319,14 @@ public:
}
}
+ // Mock-only; control whether to allow subsequent transaction.
+ void allowMoreTransaction(bool is_allowed) {
+ if (is_mock_) {
+ dynamic_cast<MockAccessor&>(*current_accessor_).
+ allowMoreTransaction(is_allowed);
+ }
+ }
+
// Some tests only work for MockAccessor. We remember whether our accessor
// is of that type.
bool is_mock_;
@@ -4104,51 +4138,86 @@ TYPED_TEST(DatabaseClientTest, createZone) {
zone(this->client_->findZone(new_name));
ASSERT_EQ(result::NOTFOUND, zone.code);
- // The mock implementation does not do createZone,
- // in which case it should throw NotImplemented (from
- // the base class)
- if (this->is_mock_) {
- ASSERT_THROW(this->client_->createZone(new_name), isc::NotImplemented);
- } else {
- // But in the real case, it should work and return true
- ASSERT_TRUE(this->client_->createZone(new_name));
- const DataSourceClient::FindResult
- zone2(this->client_->findZone(new_name));
- ASSERT_EQ(result::SUCCESS, zone2.code);
- // And the second call should return false since
- // it already exists
- ASSERT_FALSE(this->client_->createZone(new_name));
- }
+ // Adding a new zone; it should work and return true
+ ASSERT_TRUE(this->client_->createZone(new_name));
+ const DataSourceClient::FindResult
+ zone2(this->client_->findZone(new_name));
+ ASSERT_EQ(result::SUCCESS, zone2.code);
+ // And the second call should return false since
+ // it already exists
+ this->allowMoreTransaction(true);
+ ASSERT_FALSE(this->client_->createZone(new_name));
}
TYPED_TEST(DatabaseClientTest, createZoneRollbackOnLocked) {
- // skip test for mock
- if (this->is_mock_) {
- return;
- }
-
const Name new_name("example.com");
isc::datasrc::ZoneUpdaterPtr updater =
this->client_->getUpdater(this->zname_, true);
+ this->allowMoreTransaction(false);
ASSERT_THROW(this->client_->createZone(new_name), DataSourceError);
// createZone started a transaction as well, but since it failed,
// it should have been rolled back. Roll back the other one as
// well, and the next attempt should succeed
updater.reset();
+ this->allowMoreTransaction(true);
ASSERT_TRUE(this->client_->createZone(new_name));
}
TYPED_TEST(DatabaseClientTest, createZoneRollbackOnExists) {
- // skip test for mock
- if (this->is_mock_) {
- return;
- }
-
const Name new_name("example.com");
ASSERT_FALSE(this->client_->createZone(this->zname_));
- // createZone started a transaction, but since it failed,
- // it should have been rolled back, and the next attempt should succeed
+
+ // deleteZone started a transaction, but since the zone didn't even exist
+ // the transaction was not committed but should have been rolled back.
+ // The first transaction shouldn't leave any state, lock, etc, that
+ // would hinder the second attempt.
+ this->allowMoreTransaction(true);
ASSERT_TRUE(this->client_->createZone(new_name));
}
+TYPED_TEST(DatabaseClientTest, deleteZone) {
+ // Check the zone currently exists.
+ EXPECT_EQ(result::SUCCESS, this->client_->findZone(this->zname_).code);
+
+ // Deleting an existing zone; it should work and return true (previously
+ // existed and is now deleted)
+ EXPECT_TRUE(this->client_->deleteZone(this->zname_));
+
+ // Now it's not found by findZone
+ EXPECT_EQ(result::NOTFOUND, this->client_->findZone(this->zname_).code);
+
+ // And the second call should return false since it doesn't exist any more
+ this->allowMoreTransaction(true);
+ EXPECT_FALSE(this->client_->deleteZone(this->zname_));
+}
+
+TYPED_TEST(DatabaseClientTest, deleteZoneRollbackOnLocked) {
+ isc::datasrc::ZoneUpdaterPtr updater =
+ this->client_->getUpdater(this->zname_, true);
+
+ // updater locks the DB so deleteZone() will fail.
+ this->allowMoreTransaction(false);
+ EXPECT_THROW(this->client_->deleteZone(this->zname_), DataSourceError);
+
+ // deleteZone started a transaction as well, but since it failed,
+ // it should have been rolled back. Roll back the other one as
+ // well, and the next attempt should succeed
+ updater.reset();
+ this->allowMoreTransaction(true);
+ EXPECT_TRUE(this->client_->deleteZone(this->zname_));
+}
+
+TYPED_TEST(DatabaseClientTest, deleteZoneRollbackOnNotFind) {
+ // attempt of deleting non-existent zone. result in false
+ const Name new_name("example.com");
+ EXPECT_FALSE(this->client_->deleteZone(new_name));
+
+ // deleteZone started a transaction, but since the zone didn't even exist
+ // the transaction was not committed but should have been rolled back.
+ // The first transaction shouldn't leave any state, lock, etc, that
+ // would hinder the second attempt.
+ this->allowMoreTransaction(true);
+ EXPECT_TRUE(this->client_->deleteZone(this->zname_));
+}
+
}
diff --git a/src/lib/datasrc/tests/sqlite3_accessor_unittest.cc b/src/lib/datasrc/tests/sqlite3_accessor_unittest.cc
index 8dff6d5..c263304 100644
--- a/src/lib/datasrc/tests/sqlite3_accessor_unittest.cc
+++ b/src/lib/datasrc/tests/sqlite3_accessor_unittest.cc
@@ -20,6 +20,8 @@
#include <dns/rrclass.h>
+#include <exceptions/exceptions.h>
+
#include <sqlite3.h>
#include <gtest/gtest.h>
@@ -1615,4 +1617,50 @@ TEST_F(SQLite3Update, addZoneWhileLocked) {
EXPECT_FALSE(accessor->getZone(new_zone).first);
}
+//
+// Tests for deleteZone() follow.
+//
+TEST_F(SQLite3Update, deleteZone) {
+ const std::pair<bool, int> zone_info(accessor->getZone("example.com."));
+ ASSERT_TRUE(zone_info.first);
+ zone_id = zone_info.second;
+
+ // Calling deleteZone without transaction should fail
+ EXPECT_THROW(accessor->deleteZone(zone_info.first), isc::InvalidOperation);
+
+ // Delete the zone. Then confirm it, both before and after commit.
+ accessor->startTransaction();
+ accessor->deleteZone(zone_info.second);
+ EXPECT_FALSE(accessor->getZone("example.com.").first);
+ accessor->commit();
+ EXPECT_FALSE(accessor->getZone("example.com.").first);
+
+ // Records are not deleted.
+ std::string data[DatabaseAccessor::COLUMN_COUNT];
+ EXPECT_TRUE(accessor->getRecords("example.com.", zone_id, false)
+ ->getNext(data));
+}
+
+TEST_F(SQLite3Update, deleteZoneWhileLocked) {
+ const std::pair<bool, int> zone_info(accessor->getZone("example.com."));
+ ASSERT_TRUE(zone_info.first);
+ zone_id = zone_info.second;
+
+ // Adding another (not commit yet), it should lock the db
+ const std::string new_zone = "new.example.com.";
+ accessor->startTransaction();
+ zone_id = accessor->addZone(new_zone);
+
+ // deleteZone should throw an exception that it is locked
+ another_accessor->startTransaction();
+ EXPECT_THROW(another_accessor->deleteZone(zone_id), DataSourceError);
+ // Commit should do nothing, but not fail
+ another_accessor->commit();
+
+ accessor->rollback();
+
+ // The zone should still exist.
+ EXPECT_TRUE(accessor->getZone("example.com.").first);
+}
+
} // end anonymous namespace
More information about the bind10-changes
mailing list