[1061] Address review comments

Mostly comments and cleanups, some simplification of interface and
change from auto_ptr to shared_ptr.

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)
 { }

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_;
 };
 
 }

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);
 }
 

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

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);
 }
 

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;