BIND 10 trac1068review1, updated. df047c5ccb5c81f9a3d36f7fc38a19bc7c8f2ac2 [1068] use fixed sized arrays for addRecordToZone() and deleteRecordInZone() instead of vector, mainly to be consistent with other interfaces.
BIND 10 source code commits
bind10-changes at lists.isc.org
Thu Aug 25 00:41:40 UTC 2011
The branch, trac1068review1 has been updated
via df047c5ccb5c81f9a3d36f7fc38a19bc7c8f2ac2 (commit)
via a7346d50ae5389ce37e35a7131f0f218663b8c68 (commit)
via ad91831c938430b6d4a8fd7bfae517a0f1e327c1 (commit)
from ad36c799ff07d47ebd5c861c63e9feef50408e34 (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 df047c5ccb5c81f9a3d36f7fc38a19bc7c8f2ac2
Author: JINMEI Tatuya <jinmei at isc.org>
Date: Wed Aug 24 17:09:00 2011 -0700
[1068] use fixed sized arrays for addRecordToZone() and deleteRecordInZone()
instead of vector, mainly to be consistent with other interfaces.
commit a7346d50ae5389ce37e35a7131f0f218663b8c68
Author: JINMEI Tatuya <jinmei at isc.org>
Date: Wed Aug 24 15:24:42 2011 -0700
[1068] updated doxygen doc for ADD_TTL enum to be (hopefully) more intuitive
commit ad91831c938430b6d4a8fd7bfae517a0f1e327c1
Author: JINMEI Tatuya <jinmei at isc.org>
Date: Wed Aug 24 15:13:57 2011 -0700
[1068] avoid using ASSERT_EQ in a constructor. ASSERT_EQ is not appropriate
since it would have to have a return value, and even any xxx_EQ doesn't
make much sense because this is not actually what we are trying to test.
I choise to simply let it fail (should it ever happens); we should be able
to know by seeing some of the subsequent tests fail.
-----------------------------------------------------------------------
Summary of changes:
src/lib/datasrc/database.h | 17 ++--
src/lib/datasrc/sqlite3_accessor.cc | 29 +++----
src/lib/datasrc/sqlite3_accessor.h | 6 +-
src/lib/datasrc/tests/database_unittest.cc | 4 +-
src/lib/datasrc/tests/sqlite3_accessor_unittest.cc | 91 ++++++++------------
5 files changed, 62 insertions(+), 85 deletions(-)
-----------------------------------------------------------------------
diff --git a/src/lib/datasrc/database.h b/src/lib/datasrc/database.h
index f2aa9ed..efa3706 100644
--- a/src/lib/datasrc/database.h
+++ b/src/lib/datasrc/database.h
@@ -81,7 +81,7 @@ public:
enum AddRecordColumns {
ADD_NAME = 0, ///< The owner name of the record (a domain name)
ADD_REV_NAME = 1, ///< Reversed name of NAME (used for DNSSEC)
- ADD_TTL = 2, ///< The TTL of the record (an integer)
+ ADD_TTL = 2, ///< The TTL of the record (in numeric form)
ADD_TYPE = 3, ///< The RRType of the record (A/NS/TXT etc.)
ADD_SIGTYPE = 4, ///< For RRSIG records, this contains the RRTYPE
///< the RRSIG covers.
@@ -340,12 +340,12 @@ public:
/// in the actual derived method.
///
/// \exception DataSourceError Invalid call without starting a transaction,
- /// the columns parameter has an invalid number of elements, or other
- /// internal database error.
+ /// or other internal database error.
///
- /// \param columns A vector of strings that defines a record to be added
+ /// \param columns An array of strings that defines a record to be added
/// to the zone.
- virtual void addRecordToZone(const std::vector<std::string>& columns) = 0;
+ virtual void addRecordToZone(
+ const std::string (&columns)[ADD_COLUMN_COUNT]) = 0;
/// Delete a single record from the zone to be updated.
///
@@ -377,13 +377,12 @@ public:
/// it simply ignores the result.
///
/// \exception DataSourceError Invalid call without starting a transaction,
- /// the columns parameter has an invalid number of elements, or other
- /// internal database error.
+ /// or other internal database error.
///
- /// \param params A vector of strings that defines a record to be deleted
+ /// \param params An array of strings that defines a record to be deleted
/// from the zone.
virtual void deleteRecordInZone(
- const std::vector<std::string>& params) = 0;
+ const std::string (¶ms)[DEL_PARAM_COUNT]) = 0;
/// Commit updates to the zone.
///
diff --git a/src/lib/datasrc/sqlite3_accessor.cc b/src/lib/datasrc/sqlite3_accessor.cc
index ab34c3b..0e6ffdc 100644
--- a/src/lib/datasrc/sqlite3_accessor.cc
+++ b/src/lib/datasrc/sqlite3_accessor.cc
@@ -516,9 +516,10 @@ SQLite3Accessor::rollbackUpdateZone() {
namespace {
// Commonly used code sequence for adding/deleting record
+template <typename COLUMNS_TYPE>
void
doUpdate(SQLite3Parameters& dbparams, StatementID stmt_id,
- const vector<string>& update_params, const char* exec_desc)
+ COLUMNS_TYPE update_params, const char* exec_desc)
{
sqlite3_stmt* const stmt = dbparams.statements_[stmt_id];
StatementExecuter executer(dbparams, stmt_id, exec_desc);
@@ -529,8 +530,10 @@ doUpdate(SQLite3Parameters& dbparams, StatementID stmt_id,
isc_throw(DataSourceError, "failed to bind SQLite3 parameter: " <<
sqlite3_errmsg(dbparams.db_));
}
- BOOST_FOREACH(const string& column, update_params) {
- if (sqlite3_bind_text(stmt, ++param_id, column.c_str(), -1,
+ const size_t column_count =
+ sizeof(update_params) / sizeof(update_params[0]);
+ for (int i = 0; i < column_count; ++i) {
+ if (sqlite3_bind_text(stmt, ++param_id, update_params[i].c_str(), -1,
SQLITE_TRANSIENT) != SQLITE_OK) {
isc_throw(DataSourceError, "failed to bind SQLite3 parameter: " <<
sqlite3_errmsg(dbparams.db_));
@@ -541,31 +544,23 @@ doUpdate(SQLite3Parameters& dbparams, StatementID stmt_id,
}
void
-SQLite3Accessor::addRecordToZone(const vector<string>& columns) {
+SQLite3Accessor::addRecordToZone(const string (&columns)[ADD_COLUMN_COUNT]) {
if (!dbparameters_->updating_zone) {
isc_throw(DataSourceError, "adding record to SQLite3 "
"data source without transaction");
}
- if (columns.size() != ADD_COLUMN_COUNT) {
- isc_throw(DataSourceError, "adding incompatible number of columns "
- "to SQLite3 data source: " << columns.size());
- }
-
- doUpdate(*dbparameters_, ADD_RECORD, columns, "add record to zone");
+ doUpdate<const string (&)[DatabaseAccessor::ADD_COLUMN_COUNT]>(
+ *dbparameters_, ADD_RECORD, columns, "add record to zone");
}
void
-SQLite3Accessor::deleteRecordInZone(const vector<string>& params) {
+SQLite3Accessor::deleteRecordInZone(const string (¶ms)[DEL_PARAM_COUNT]) {
if (!dbparameters_->updating_zone) {
isc_throw(DataSourceError, "deleting record in SQLite3 "
"data source without transaction");
}
- if (params.size() != DEL_PARAM_COUNT) {
- isc_throw(DataSourceError, "incompatible # of parameters for "
- "deleting in SQLite3 data source: " << params.size());
- }
-
- doUpdate(*dbparameters_, DEL_RECORD, params, "delete record from zone");
+ doUpdate<const string (&)[DatabaseAccessor::DEL_PARAM_COUNT]>(
+ *dbparameters_, DEL_RECORD, params, "delete record from zone");
}
}
diff --git a/src/lib/datasrc/sqlite3_accessor.h b/src/lib/datasrc/sqlite3_accessor.h
index 471f6a1..fd3cdfd 100644
--- a/src/lib/datasrc/sqlite3_accessor.h
+++ b/src/lib/datasrc/sqlite3_accessor.h
@@ -140,9 +140,11 @@ public:
/// considered a bug of the higher level application program.
virtual void rollbackUpdateZone();
- virtual void addRecordToZone(const std::vector<std::string>& columns);
+ virtual void addRecordToZone(
+ const std::string (&columns)[ADD_COLUMN_COUNT]);
- virtual void deleteRecordInZone(const std::vector<std::string>& params);
+ virtual void deleteRecordInZone(
+ const std::string (¶ms)[DEL_PARAM_COUNT]);
/// The SQLite3 implementation of this method returns a string starting
/// with a fixed prefix of "sqlite3_" followed by the DB file name
diff --git a/src/lib/datasrc/tests/database_unittest.cc b/src/lib/datasrc/tests/database_unittest.cc
index f832b1d..c3d7b12 100644
--- a/src/lib/datasrc/tests/database_unittest.cc
+++ b/src/lib/datasrc/tests/database_unittest.cc
@@ -64,8 +64,8 @@ public:
}
virtual void commitUpdateZone() {}
virtual void rollbackUpdateZone() {}
- virtual void addRecordToZone(const std::vector<std::string>&) {}
- virtual void deleteRecordInZone(const std::vector<std::string>&) {}
+ virtual void addRecordToZone(const string (&)[ADD_COLUMN_COUNT]) {}
+ virtual void deleteRecordInZone(const string (&)[DEL_PARAM_COUNT]) {}
virtual const std::string& getDBName() const {
return (database_name_);
diff --git a/src/lib/datasrc/tests/sqlite3_accessor_unittest.cc b/src/lib/datasrc/tests/sqlite3_accessor_unittest.cc
index c4ff357..5725c81 100644
--- a/src/lib/datasrc/tests/sqlite3_accessor_unittest.cc
+++ b/src/lib/datasrc/tests/sqlite3_accessor_unittest.cc
@@ -12,6 +12,7 @@
// OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR
// PERFORMANCE OF THIS SOFTWARE.
+#include <algorithm>
#include <vector>
#include <datasrc/sqlite3_accessor.h>
@@ -299,9 +300,10 @@ const char* const deleted_data[] = {
class SQLite3Update : public SQLite3AccessorTest {
protected:
SQLite3Update() {
- ASSERT_EQ(0, system(INSTALL_PROG " " TEST_DATA_DIR
- "/test.sqlite3 "
- TEST_DATA_BUILDDIR "/test.sqlite3.copied"));
+ // Note: if "installing" the test file fails some of the subsequent
+ // tests will fail and we should be able to notice that.
+ system(INSTALL_PROG " " TEST_DATA_DIR
+ "/test.sqlite3 " TEST_DATA_BUILDDIR "/test.sqlite3.copied");
initAccessor(TEST_DATA_BUILDDIR "/test.sqlite3.copied", RRClass::IN());
zone_id = accessor->getZone(Name("example.com")).second;
another_accessor.reset(new SQLite3Accessor(
@@ -312,7 +314,8 @@ protected:
int zone_id;
std::string get_columns[DatabaseAccessor::COLUMN_COUNT];
- std::vector<std::string> update_columns;
+ std::string add_columns[DatabaseAccessor::ADD_COLUMN_COUNT];
+ std::string del_params[DatabaseAccessor::DEL_PARAM_COUNT];
vector<const char* const*> expected_stored; // placeholder for checkRecords
vector<const char* const*> empty_stored; // indicate no corresponding data
@@ -450,9 +453,9 @@ TEST_F(SQLite3Update, addRecord) {
checkRecords(*accessor, zone_id, "newdata.example.com.", empty_stored);
zone_id = accessor->startUpdateZone("example.com.", false).second;
- update_columns.assign(new_data,
- new_data + DatabaseAccessor::ADD_COLUMN_COUNT);
- accessor->addRecordToZone(update_columns);
+ copy(new_data, new_data + DatabaseAccessor::ADD_COLUMN_COUNT,
+ add_columns);
+ accessor->addRecordToZone(add_columns);
expected_stored.clear();
expected_stored.push_back(new_data);
@@ -465,9 +468,9 @@ TEST_F(SQLite3Update, addRecord) {
TEST_F(SQLite3Update, addThenRollback) {
zone_id = accessor->startUpdateZone("example.com.", false).second;
- update_columns.assign(new_data,
- new_data + DatabaseAccessor::ADD_COLUMN_COUNT);
- accessor->addRecordToZone(update_columns);
+ copy(new_data, new_data + DatabaseAccessor::ADD_COLUMN_COUNT,
+ add_columns);
+ accessor->addRecordToZone(add_columns);
expected_stored.clear();
expected_stored.push_back(new_data);
@@ -488,28 +491,17 @@ TEST_F(SQLite3Update, duplicateAdd) {
// Adding exactly the same data. As this backend is "dumb", another
// row of the same content will be inserted.
- update_columns.assign(dup_data,
- dup_data + DatabaseAccessor::ADD_COLUMN_COUNT);
+ copy(dup_data, dup_data + DatabaseAccessor::ADD_COLUMN_COUNT,
+ add_columns);
zone_id = accessor->startUpdateZone("example.com.", false).second;
- accessor->addRecordToZone(update_columns);
+ accessor->addRecordToZone(add_columns);
expected_stored.push_back(dup_data);
checkRecords(*accessor, zone_id, "foo.bar.example.com.", expected_stored);
}
TEST_F(SQLite3Update, invalidAdd) {
// An attempt of add before an explicit start of transaction
- EXPECT_THROW(accessor->addRecordToZone(update_columns), DataSourceError);
-
- // Short column vector
- update_columns.clear();
- zone_id = accessor->startUpdateZone("example.com.", false).second;
- EXPECT_THROW(accessor->addRecordToZone(update_columns), DataSourceError);
-
- // Too many columns
- for (int i = 0; i < DatabaseAccessor::ADD_COLUMN_COUNT + 1; ++i) {
- update_columns.push_back("");
- }
- EXPECT_THROW(accessor->addRecordToZone(update_columns), DataSourceError);
+ EXPECT_THROW(accessor->addRecordToZone(add_columns), DataSourceError);
}
TEST_F(SQLite3Update, deleteRecord) {
@@ -517,9 +509,9 @@ TEST_F(SQLite3Update, deleteRecord) {
checkRecords(*accessor, zone_id, "foo.bar.example.com.", expected_stored);
- update_columns.assign(deleted_data, deleted_data +
- DatabaseAccessor::DEL_PARAM_COUNT);
- accessor->deleteRecordInZone(update_columns);
+ copy(deleted_data, deleted_data + DatabaseAccessor::DEL_PARAM_COUNT,
+ del_params);
+ accessor->deleteRecordInZone(del_params);
checkRecords(*accessor, zone_id, "foo.bar.example.com.", empty_stored);
// Commit the change, and confirm the deleted data still isn't there.
@@ -530,9 +522,9 @@ TEST_F(SQLite3Update, deleteRecord) {
TEST_F(SQLite3Update, deleteThenRollback) {
zone_id = accessor->startUpdateZone("example.com.", false).second;
- update_columns.assign(deleted_data, deleted_data +
- DatabaseAccessor::DEL_PARAM_COUNT);
- accessor->deleteRecordInZone(update_columns);
+ copy(deleted_data, deleted_data + DatabaseAccessor::DEL_PARAM_COUNT,
+ del_params);
+ accessor->deleteRecordInZone(del_params);
checkRecords(*accessor, zone_id, "foo.bar.example.com.", empty_stored);
// Rollback the change, and confirm the data still exists.
@@ -542,47 +534,36 @@ TEST_F(SQLite3Update, deleteThenRollback) {
TEST_F(SQLite3Update, deleteNonexistent) {
zone_id = accessor->startUpdateZone("example.com.", false).second;
- update_columns.assign(deleted_data, deleted_data +
- DatabaseAccessor::DEL_PARAM_COUNT);
+ copy(deleted_data, deleted_data + DatabaseAccessor::DEL_PARAM_COUNT,
+ del_params);
// Replace the name with a non existent one, then try to delete it.
// nothing should happen.
- update_columns[0] = "no-such-name.example.com.";
+ del_params[DatabaseAccessor::DEL_NAME] = "no-such-name.example.com.";
checkRecords(*accessor, zone_id, "no-such-name.example.com.",
empty_stored);
- accessor->deleteRecordInZone(update_columns);
+ accessor->deleteRecordInZone(del_params);
checkRecords(*accessor, zone_id, "no-such-name.example.com.",
empty_stored);
// Name exists but the RR type is different. Delete attempt shouldn't
// delete only by name.
- update_columns.assign(deleted_data, deleted_data +
- DatabaseAccessor::DEL_PARAM_COUNT);
- update_columns[1] = "AAAA";
- accessor->deleteRecordInZone(update_columns);
+ copy(deleted_data, deleted_data + DatabaseAccessor::DEL_PARAM_COUNT,
+ del_params);
+ del_params[DatabaseAccessor::DEL_TYPE] = "AAAA";
+ accessor->deleteRecordInZone(del_params);
checkRecords(*accessor, zone_id, "foo.bar.example.com.", expected_stored);
// Similar to the previous case, but RDATA is different.
- update_columns.assign(deleted_data, deleted_data +
- DatabaseAccessor::DEL_PARAM_COUNT);
- update_columns[2] = "192.0.2.2";
- accessor->deleteRecordInZone(update_columns);
+ copy(deleted_data, deleted_data + DatabaseAccessor::DEL_PARAM_COUNT,
+ del_params);
+ del_params[DatabaseAccessor::DEL_RDATA] = "192.0.2.2";
+ accessor->deleteRecordInZone(del_params);
checkRecords(*accessor, zone_id, "foo.bar.example.com.", expected_stored);
}
TEST_F(SQLite3Update, invalidDelete) {
// An attempt of delete before an explicit start of transaction
- EXPECT_THROW(accessor->deleteRecordInZone(update_columns), DataSourceError);
-
- // Short column vector
- update_columns.clear();
- zone_id = accessor->startUpdateZone("example.com.", false).second;
- EXPECT_THROW(accessor->deleteRecordInZone(update_columns), DataSourceError);
-
- // Too many parameters
- for (int i = 0; i < DatabaseAccessor::DEL_PARAM_COUNT + 1; ++i) {
- update_columns.push_back("");
- }
- EXPECT_THROW(accessor->deleteRecordInZone(update_columns), DataSourceError);
+ EXPECT_THROW(accessor->deleteRecordInZone(del_params), DataSourceError);
}
} // end anonymous namespace
More information about the bind10-changes
mailing list