BIND 10 trac1061, updated. fea1f88cd0bb5bdeefc6048b122da4328635163d [1061] Address review comments
BIND 10 source code commits
bind10-changes at lists.isc.org
Thu Aug 4 15:23:27 UTC 2011
The branch, trac1061 has been updated
via fea1f88cd0bb5bdeefc6048b122da4328635163d (commit)
from 3702df52de21023d90052afdc54732d9ad285b39 (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 fea1f88cd0bb5bdeefc6048b122da4328635163d
Author: Michal 'vorner' Vaner <michal.vaner at nic.cz>
Date: Thu Aug 4 17:18:54 2011 +0200
[1061] Address review comments
Mostly comments and cleanups, some simplification of interface and
change from auto_ptr to shared_ptr.
-----------------------------------------------------------------------
Summary of changes:
src/lib/datasrc/database.cc | 10 ++++---
src/lib/datasrc/database.h | 24 +++++-----------
src/lib/datasrc/sqlite3_connection.cc | 16 +++++-----
src/lib/datasrc/sqlite3_connection.h | 18 ++++--------
src/lib/datasrc/tests/database_unittest.cc | 6 ++--
.../datasrc/tests/sqlite3_connection_unittest.cc | 29 +++++--------------
6 files changed, 38 insertions(+), 65 deletions(-)
-----------------------------------------------------------------------
diff --git a/src/lib/datasrc/database.cc b/src/lib/datasrc/database.cc
index 5fe9f7b..2264f2c 100644
--- a/src/lib/datasrc/database.cc
+++ b/src/lib/datasrc/database.cc
@@ -22,7 +22,8 @@ using isc::dns::Name;
namespace isc {
namespace datasrc {
-DatabaseClient::DatabaseClient(std::auto_ptr<DatabaseConnection> connection) :
+DatabaseClient::DatabaseClient(boost::shared_ptr<DatabaseConnection>
+ connection) :
connection_(connection)
{
if (connection_.get() == NULL) {
@@ -37,7 +38,7 @@ DatabaseClient::findZone(const Name& name) const {
// Try exact first
if (zone.first) {
return (FindResult(result::SUCCESS,
- ZoneFinderPtr(new Finder(*connection_,
+ ZoneFinderPtr(new Finder(connection_,
zone.second))));
}
// Than super domains
@@ -46,7 +47,7 @@ DatabaseClient::findZone(const Name& name) const {
zone = connection_->getZone(name.split(i));
if (zone.first) {
return (FindResult(result::PARTIALMATCH,
- ZoneFinderPtr(new Finder(*connection_,
+ ZoneFinderPtr(new Finder(connection_,
zone.second))));
}
}
@@ -54,7 +55,8 @@ DatabaseClient::findZone(const Name& name) const {
return (FindResult(result::NOTFOUND, ZoneFinderPtr()));
}
-DatabaseClient::Finder::Finder(DatabaseConnection& connection, int zone_id) :
+DatabaseClient::Finder::Finder(boost::shared_ptr<DatabaseConnection>
+ connection, int zone_id) :
connection_(connection),
zone_id_(zone_id)
{ }
diff --git a/src/lib/datasrc/database.h b/src/lib/datasrc/database.h
index 5693479..8e5c156 100644
--- a/src/lib/datasrc/database.h
+++ b/src/lib/datasrc/database.h
@@ -50,7 +50,7 @@ public:
* It is empty, but needs a virtual one, since we will use the derived
* classes in polymorphic way.
*/
- virtual ~ DatabaseConnection() { }
+ virtual ~DatabaseConnection() { }
/**
* \brief Retrieve a zone identifier
*
@@ -94,24 +94,14 @@ public:
*
* It initializes the client with a connection.
*
- * It throws isc::InvalidParameter if connection is NULL. It might throw
+ * \exception isc::InvalidParameter if connection is NULL. It might throw
* standard allocation exception as well, but doesn't throw anything else.
*
- * \note Some objects returned from methods of this class (like ZoneFinder)
- * hold references to the connection. As the lifetime of the connection
- * is bound to this object, the returned objects must not be used after
- * descruction of the DatabaseClient.
- *
- * \todo Should we use shared_ptr instead? On one side, we would get rid of
- * the restriction and maybe could easy up some shutdown scenarios with
- * multi-threaded applications, on the other hand it is more expensive
- * and looks generally unneeded.
- *
* \param connection The connection to use to get data. As the parameter
* suggests, the client takes ownership of the connection and will
* delete it when itself deleted.
*/
- DatabaseClient(std::auto_ptr<DatabaseConnection> connection);
+ DatabaseClient(boost::shared_ptr<DatabaseConnection> connection);
/**
* \brief Corresponding ZoneFinder implementation
*
@@ -138,7 +128,7 @@ public:
* DatabaseConnection::getZone and which will be passed to further
* calls to the connection.
*/
- Finder(DatabaseConnection& connection, int zone_id);
+ Finder(boost::shared_ptr<DatabaseConnection> connection, int zone_id);
virtual isc::dns::Name getOrigin() const;
virtual isc::dns::RRClass getClass() const;
virtual FindResult find(const isc::dns::Name& name,
@@ -162,10 +152,10 @@ public:
* normal applications shouldn't need it.
*/
const DatabaseConnection& connection() const {
- return (connection_);
+ return (*connection_);
}
private:
- DatabaseConnection& connection_;
+ boost::shared_ptr<DatabaseConnection> connection_;
const int zone_id_;
};
/**
@@ -183,7 +173,7 @@ public:
virtual FindResult findZone(const isc::dns::Name& name) const;
private:
/// \brief Our connection.
- const std::auto_ptr<DatabaseConnection> connection_;
+ const boost::shared_ptr<DatabaseConnection> connection_;
};
}
diff --git a/src/lib/datasrc/sqlite3_connection.cc b/src/lib/datasrc/sqlite3_connection.cc
index e850db4..35db446 100644
--- a/src/lib/datasrc/sqlite3_connection.cc
+++ b/src/lib/datasrc/sqlite3_connection.cc
@@ -44,19 +44,14 @@ struct SQLite3Parameters {
*/
};
-SQLite3Connection::SQLite3Connection(const isc::data::ConstElementPtr&
- config,
+SQLite3Connection::SQLite3Connection(const std::string& filename,
const isc::dns::RRClass& rrclass) :
dbparameters_(new SQLite3Parameters),
class_(rrclass.toText())
{
LOG_DEBUG(logger, DBG_TRACE_BASIC, DATASRC_SQLITE_NEWCONN);
- if (config && config->contains("database_file")) {
- open(config->get("database_file")->stringValue());
- } else {
- isc_throw(DataSourceError, "No SQLite database file specified");
- }
+ open(filename);
}
namespace {
@@ -237,7 +232,7 @@ SQLite3Connection::open(const std::string& name) {
initializer.move(dbparameters_);
}
-SQLite3Connection::~ SQLite3Connection() {
+SQLite3Connection::~SQLite3Connection() {
LOG_DEBUG(logger, DBG_TRACE_BASIC, DATASRC_SQLITE_DROPCONN);
if (dbparameters_->db_ != NULL) {
close();
@@ -291,6 +286,8 @@ std::pair<bool, int>
SQLite3Connection::getZone(const isc::dns::Name& name) const {
int rc;
+ // Take the statement (simple SELECT id FROM zones WHERE...)
+ // and prepare it (bind the parameters to it)
sqlite3_reset(dbparameters_->q_zone_);
rc = sqlite3_bind_text(dbparameters_->q_zone_, 1, name.toText().c_str(),
-1, SQLITE_STATIC);
@@ -305,6 +302,7 @@ SQLite3Connection::getZone(const isc::dns::Name& name) const {
" to SQL statement (zone)");
}
+ // Get the data there and see if it found anything
rc = sqlite3_step(dbparameters_->q_zone_);
std::pair<bool, int> result;
if (rc == SQLITE_ROW) {
@@ -314,7 +312,9 @@ SQLite3Connection::getZone(const isc::dns::Name& name) const {
} else {
result = std::pair<bool, int>(false, 0);
}
+ // Free resources
sqlite3_reset(dbparameters_->q_zone_);
+
return (result);
}
diff --git a/src/lib/datasrc/sqlite3_connection.h b/src/lib/datasrc/sqlite3_connection.h
index 266dd05..4845715 100644
--- a/src/lib/datasrc/sqlite3_connection.h
+++ b/src/lib/datasrc/sqlite3_connection.h
@@ -19,7 +19,6 @@
#include <datasrc/database.h>
#include <exceptions/exceptions.h>
-#include <cc/data.h>
#include <string>
@@ -59,27 +58,22 @@ public:
*
* This opens the database and becomes ready to serve data from there.
*
- * This might throw SQLite3Error if the given database file doesn't work
- * (it is broken, doesn't exist and can't be created, etc). It might throw
- * DataSourceError if the provided config is invalid (it is missing the
- * database_file element).
+ * \exception SQLite3Error will be thrown if the given database file
+ * doesn't work (it is broken, doesn't exist and can't be created, etc).
*
- * \param config The part of config describing which database file should
- * be used.
+ * \param filename The database file to be used.
* \param rrclass Which class of data it should serve (while the database
* can contain multiple classes of data, single connection can provide
* only one class).
- * \todo Should we pass the database filename instead of the config? It
- * might be cleaner if this class doesn't know anything about configs.
*/
- SQLite3Connection(const isc::data::ConstElementPtr& config,
+ SQLite3Connection(const std::string& filename,
const isc::dns::RRClass& rrclass);
/**
* \brief Destructor
*
* Closes the database.
*/
- ~ SQLite3Connection();
+ ~SQLite3Connection();
/**
* \brief Look up a zone
*
@@ -87,7 +81,7 @@ public:
* in the data. It looks for a zone with the exact given origin and class
* passed to the constructor.
*
- * It may throw SQLite3Error if something about the database is broken.
+ * \exception SQLite3Error if something about the database is broken.
*
* \param name The name of zone to look up
* \return The pair contains if the lookup was successful in the first
diff --git a/src/lib/datasrc/tests/database_unittest.cc b/src/lib/datasrc/tests/database_unittest.cc
index b60d5c0..c271a76 100644
--- a/src/lib/datasrc/tests/database_unittest.cc
+++ b/src/lib/datasrc/tests/database_unittest.cc
@@ -52,12 +52,12 @@ public:
*/
void createClient() {
current_connection_ = new MockConnection();
- client_.reset(new DatabaseClient(auto_ptr<DatabaseConnection>(
+ client_.reset(new DatabaseClient(shared_ptr<DatabaseConnection>(
current_connection_)));
}
// Will be deleted by client_, just keep the current value for comparison.
MockConnection* current_connection_;
- auto_ptr<DatabaseClient> client_;
+ shared_ptr<DatabaseClient> client_;
/**
* Check the zone finder is a valid one and references the zone ID and
* connection available here.
@@ -92,7 +92,7 @@ TEST_F(DatabaseClientTest, superZone) {
}
TEST_F(DatabaseClientTest, noConnException) {
- EXPECT_THROW(DatabaseClient(auto_ptr<DatabaseConnection>()),
+ EXPECT_THROW(DatabaseClient(shared_ptr<DatabaseConnection>()),
isc::InvalidParameter);
}
diff --git a/src/lib/datasrc/tests/sqlite3_connection_unittest.cc b/src/lib/datasrc/tests/sqlite3_connection_unittest.cc
index 3065dfe..6d2a945 100644
--- a/src/lib/datasrc/tests/sqlite3_connection_unittest.cc
+++ b/src/lib/datasrc/tests/sqlite3_connection_unittest.cc
@@ -27,23 +27,17 @@ using isc::dns::Name;
namespace {
// Some test data
-ConstElementPtr SQLITE_DBFILE_EXAMPLE = Element::fromJSON(
- "{ \"database_file\": \"" TEST_DATA_DIR "/test.sqlite3\"}");
-ConstElementPtr SQLITE_DBFILE_EXAMPLE2 = Element::fromJSON(
- "{ \"database_file\": \"" TEST_DATA_DIR "/example2.com.sqlite3\"}");
-ConstElementPtr SQLITE_DBFILE_EXAMPLE_ROOT = Element::fromJSON(
- "{ \"database_file\": \"" TEST_DATA_DIR "/test-root.sqlite3\"}");
-ConstElementPtr SQLITE_DBFILE_BROKENDB = Element::fromJSON(
- "{ \"database_file\": \"" TEST_DATA_DIR "/brokendb.sqlite3\"}");
-ConstElementPtr SQLITE_DBFILE_MEMORY = Element::fromJSON(
- "{ \"database_file\": \":memory:\"}");
+std::string SQLITE_DBFILE_EXAMPLE = TEST_DATA_DIR "/test.sqlite3";
+std::string SQLITE_DBFILE_EXAMPLE2 = TEST_DATA_DIR "/example2.com.sqlite3";
+std::string SQLITE_DBFILE_EXAMPLE_ROOT = TEST_DATA_DIR "/test-root.sqlite3";
+std::string SQLITE_DBFILE_BROKENDB = TEST_DATA_DIR "/brokendb.sqlite3";
+std::string SQLITE_DBFILE_MEMORY = "memory";
// The following file must be non existent and must be non"creatable";
// the sqlite3 library will try to create a new DB file if it doesn't exist,
// so to test a failure case the create operation should also fail.
// The "nodir", a non existent directory, is inserted for this purpose.
-ConstElementPtr SQLITE_DBFILE_NOTEXIST = Element::fromJSON(
- "{ \"database_file\": \"" TEST_DATA_DIR "/nodir/notexist\"}");
+std::string SQLITE_DBFILE_NOTEXIST = TEST_DATA_DIR "/nodir/notexist";
// Opening works (the content is tested in different tests)
TEST(SQLite3Open, common) {
@@ -51,13 +45,6 @@ TEST(SQLite3Open, common) {
RRClass::IN()));
}
-// Missing config
-TEST(SQLite3Open, noConfig) {
- EXPECT_THROW(SQLite3Connection conn(Element::fromJSON("{}"),
- RRClass::IN()),
- DataSourceError);
-}
-
// The file can't be opened
TEST(SQLite3Open, notExist) {
EXPECT_THROW(SQLite3Connection conn(SQLITE_DBFILE_NOTEXIST,
@@ -83,8 +70,8 @@ public:
initConn(SQLITE_DBFILE_EXAMPLE, RRClass::IN());
}
// So it can be re-created with different data
- void initConn(const ConstElementPtr& config, const RRClass& rrclass) {
- conn.reset(new SQLite3Connection(config, rrclass));
+ void initConn(const std::string& filename, const RRClass& rrclass) {
+ conn.reset(new SQLite3Connection(filename, rrclass));
}
// The tested connection
std::auto_ptr<SQLite3Connection> conn;
More information about the bind10-changes
mailing list