[5477] Addressed review comments

src/bin/dhcp4/ctrl_dhcp4_srv.*
src/bin/dhcp6/ctrl_dhcp6_srv.*
    Changed dbReconnect() to accept ReconnectCtlPtr
    Added commentary for dbReconnect and dbLostCallback

src/lib/dhcpsrv/database_connection.h
    Removed extraneous typedef

many files:
    Changed DatabaseConnection::Callback to ::DbLostCallback

src/lib/dhcpsrv/tests/database_connection_unittest.cc
    Added commentary to text fixture and tests

diff --git a/src/bin/dhcp4/ctrl_dhcp4_srv.cc b/src/bin/dhcp4/ctrl_dhcp4_srv.cc
index db76346b030ae3a1edc737b4f308c733eb5c4bd0..6767602acb962a10ddb095577245f5a9e74ba69d 100644 (file)
--- a/src/bin/dhcp4/ctrl_dhcp4_srv.cc
+++ b/src/bin/dhcp4/ctrl_dhcp4_srv.cc
@@ -650,8 +650,7 @@ ControlledDhcpv4Srv::checkConfig(isc::data::ConstElementPtr config) {
 }
 
 ControlledDhcpv4Srv::ControlledDhcpv4Srv(uint16_t port /*= DHCP4_SERVER_PORT*/)
-    : Dhcpv4Srv(port), io_service_(), timer_mgr_(TimerMgr::instance()),
-      db_reconnect_ctl_(NULL) {
+    : Dhcpv4Srv(port), io_service_(), timer_mgr_(TimerMgr::instance()) {
     if (getInstance()) {
         isc_throw(InvalidOperation,
                   "There is another Dhcpv4Srv instance already.");
@@ -795,7 +794,7 @@ ControlledDhcpv4Srv::deleteExpiredReclaimedLeases(const uint32_t secs) {
 }
 
 void
-ControlledDhcpv4Srv::dbReconnect() {
+ControlledDhcpv4Srv::dbReconnect(ReconnectCtlPtr db_reconnect_ctl) {
     bool reopened = false;
 
     // Re-open lease and host database with new parameters.
@@ -817,24 +816,25 @@ ControlledDhcpv4Srv::dbReconnect() {
         network_state_.enableService();
 
         // Toss the reconnect control, we're done with it
-        db_reconnect_ctl_.reset();
+        db_reconnect_ctl.reset();
     } else {
-        if (!db_reconnect_ctl_->checkRetries()) {
+        if (!db_reconnect_ctl->checkRetries()) {
             LOG_ERROR(dhcp4_logger, DHCP4_DB_RECONNECT_RETRIES_EXHAUSTED)
-            .arg(db_reconnect_ctl_->maxRetries());
+            .arg(db_reconnect_ctl->maxRetries());
             shutdown();
             return;
         }
 
         LOG_INFO(dhcp4_logger, DHCP4_DB_RECONNECT_ATTEMPT_SCHEDULE)
-                .arg(db_reconnect_ctl_->maxRetries() - db_reconnect_ctl_->retriesLeft() + 1)
-                .arg(db_reconnect_ctl_->maxRetries())
-                .arg(db_reconnect_ctl_->retryInterval());
+                .arg(db_reconnect_ctl->maxRetries() - db_reconnect_ctl->retriesLeft() + 1)
+                .arg(db_reconnect_ctl->maxRetries())
+                .arg(db_reconnect_ctl->retryInterval());
 
         if (!TimerMgr::instance()->isTimerRegistered("Dhcp4DbReconnectTimer")) {
             TimerMgr::instance()->registerTimer("Dhcp4DbReconnectTimer",
-                            boost::bind(&ControlledDhcpv4Srv::dbReconnect, this),
-                            db_reconnect_ctl_->retryInterval() * 1000,
+                            boost::bind(&ControlledDhcpv4Srv::dbReconnect, this,
+                                        db_reconnect_ctl),
+                            db_reconnect_ctl->retryInterval() * 1000,
                             asiolink::IntervalTimer::ONE_SHOT);
         }
 
@@ -862,11 +862,8 @@ ControlledDhcpv4Srv::dbLostCallback(ReconnectCtlPtr db_reconnect_ctl) {
         return(false);
     }
 
-    // Save the reconnect control
-    db_reconnect_ctl_ = db_reconnect_ctl;
-
     // Invoke reconnect method
-    dbReconnect();
+    dbReconnect(db_reconnect_ctl);
 
     return(true);
 }

diff --git a/src/bin/dhcp4/ctrl_dhcp4_srv.h b/src/bin/dhcp4/ctrl_dhcp4_srv.h
index 740a7dcfe977a696f9616fb06cd634d5356f6738..06d5ee6472c6791bb7c6ffb8d657d88f5ddc186d 100644 (file)
--- a/src/bin/dhcp4/ctrl_dhcp4_srv.h
+++ b/src/bin/dhcp4/ctrl_dhcp4_srv.h
@@ -319,9 +319,43 @@ private:
     void deleteExpiredReclaimedLeases(const uint32_t secs);
 
     /// @brief Attempts to reconnect the server to the DB backend managers
-    void dbReconnect();
+    ///
+    /// This is a self-rescheduling function that attempts to reconnect to the
+    /// server's DB backends after connectivity to one or more have been
+    /// lost.  Upon entry it will attempt to reconnect via @ref CfgDdbAccess::
+    /// createManagers.  If this is succesful, DHCP servicing is re-enabled and
+    /// server returns to normal operation.
+    ///
+    /// If reconnection fails and the maximum number of retries has not been
+    /// exhausted, it will schedule a call to itself to occur at the
+    /// configured retry interval. DHCP service remains disabled.
+    ///
+    /// If the maximum number of retries has been exhausted an error is logged
+    /// and the server shuts down.
+    ///
+    /// @param db_reconnect_ctl pointer to the ReconnectCtl containing the
+    /// configured reconnect parameters
+    ///
+    void dbReconnect(ReconnectCtlPtr db_reconnect_ctl);
 
     /// @brief Callback DB backends should invoke upon loss of connectivity
+    ///
+    /// This function is invoked by DB backends when they detect a loss of
+    /// connectivity.  The parameter, db_reconnect_ctl, conveys the configured
+    /// maximum number of reconnect retries as well as the interval to wait
+    /// between retry attempts.
+    ///
+    /// If either value is zero, reconnect is presumed to be disabled and
+    /// the function will returns false.  This instructs the DB backend
+    /// layer (the caller) to treat the connectivity loss as fatal.
+    ///
+    /// Otherwise, the function saves db_reconnect_ctl and invokes
+    /// dbReconnect to initiate the reconnect process.
+    ///
+    /// @param db_reconnect_ctl pointer to the ReconnectCtl containing the
+    /// configured reconnect parameters
+    ///
+    /// @return false if reconnect is not configured, true otherwise
     bool dbLostCallback(ReconnectCtlPtr db_reconnect_ctl);
 
     /// @brief Static pointer to the sole instance of the DHCP server.
@@ -338,9 +372,6 @@ private:
     /// Shared pointer to the instance of timer @c TimerMgr is held here to
     /// make sure that the @c TimerMgr outlives instance of this class.
     TimerMgrPtr timer_mgr_;
-
-    /// @brief Pointer the current DB reconnect control values
-    ReconnectCtlPtr db_reconnect_ctl_;
 };
 
 }; // namespace isc::dhcp

diff --git a/src/bin/dhcp6/ctrl_dhcp6_srv.cc b/src/bin/dhcp6/ctrl_dhcp6_srv.cc
index bfa8348d2af15b2721baf1a2bbad9a73c1faedb4..46a524c8542844118ae046e6c2a8a12c383be491 100644 (file)
--- a/src/bin/dhcp6/ctrl_dhcp6_srv.cc
+++ b/src/bin/dhcp6/ctrl_dhcp6_srv.cc
@@ -673,8 +673,7 @@ ControlledDhcpv6Srv::checkConfig(isc::data::ConstElementPtr config) {
 }
 
 ControlledDhcpv6Srv::ControlledDhcpv6Srv(uint16_t port)
-    : Dhcpv6Srv(port), io_service_(), timer_mgr_(TimerMgr::instance()),
-      db_reconnect_ctl_(NULL) {
+    : Dhcpv6Srv(port), io_service_(), timer_mgr_(TimerMgr::instance()) {
     if (server_) {
         isc_throw(InvalidOperation,
                   "There is another Dhcpv6Srv instance already.");
@@ -817,7 +816,7 @@ ControlledDhcpv6Srv::deleteExpiredReclaimedLeases(const uint32_t secs) {
 }
 
 void
-ControlledDhcpv6Srv::dbReconnect() {
+ControlledDhcpv6Srv::dbReconnect(ReconnectCtlPtr db_reconnect_ctl) {
     bool reopened = false;
 
     // Re-open lease and host database with new parameters.
@@ -838,24 +837,25 @@ ControlledDhcpv6Srv::dbReconnect() {
         network_state_.enableService();
 
         // Toss the reconnct control, we're done with it
-        db_reconnect_ctl_.reset();
+        db_reconnect_ctl.reset();
     } else {
-        if (!db_reconnect_ctl_->checkRetries()) {
+        if (!db_reconnect_ctl->checkRetries()) {
             LOG_ERROR(dhcp6_logger, DHCP6_DB_RECONNECT_RETRIES_EXHAUSTED)
-            .arg(db_reconnect_ctl_->maxRetries());
+            .arg(db_reconnect_ctl->maxRetries());
             shutdown();
             return;
         }
 
         LOG_INFO(dhcp6_logger, DHCP6_DB_RECONNECT_ATTEMPT_SCHEDULE)
-                .arg(db_reconnect_ctl_->maxRetries() - db_reconnect_ctl_->retriesLeft() + 1)
-                .arg(db_reconnect_ctl_->maxRetries())
-                .arg(db_reconnect_ctl_->retryInterval());
+                .arg(db_reconnect_ctl->maxRetries() - db_reconnect_ctl->retriesLeft() + 1)
+                .arg(db_reconnect_ctl->maxRetries())
+                .arg(db_reconnect_ctl->retryInterval());
 
         if (!TimerMgr::instance()->isTimerRegistered("Dhcp6DbReconnectTimer")) {
             TimerMgr::instance()->registerTimer("Dhcp6DbReconnectTimer",
-                            boost::bind(&ControlledDhcpv6Srv::dbReconnect, this),
-                            db_reconnect_ctl_->retryInterval() * 1000,
+                            boost::bind(&ControlledDhcpv6Srv::dbReconnect, this,
+                            db_reconnect_ctl),
+                            db_reconnect_ctl->retryInterval() * 1000,
                             asiolink::IntervalTimer::ONE_SHOT);
         }
 
@@ -883,11 +883,8 @@ ControlledDhcpv6Srv::dbLostCallback(ReconnectCtlPtr db_reconnect_ctl) {
         return(false);
     }
 
-    // Save the reconnect control
-    db_reconnect_ctl_ = db_reconnect_ctl;
-
     // Invoke reconnect method
-    dbReconnect();
+    dbReconnect(db_reconnect_ctl);
 
     return(true);
 }

diff --git a/src/bin/dhcp6/ctrl_dhcp6_srv.h b/src/bin/dhcp6/ctrl_dhcp6_srv.h
index 49201b87da643c35a335a828f4c381c45fd84148..31e738a91ed170f0765942f5b2a9fc655044c862 100644 (file)
--- a/src/bin/dhcp6/ctrl_dhcp6_srv.h
+++ b/src/bin/dhcp6/ctrl_dhcp6_srv.h
@@ -319,9 +319,40 @@ private:
     void deleteExpiredReclaimedLeases(const uint32_t secs);
 
     /// @brief Attempts to reconnect the server to the DB backend managers
-    void dbReconnect();
+    ///
+    /// This is a self-rescheduling function that attempts to reconnect to the
+    /// server's DB backends after connectivity to one or more have been
+    /// lost.  Upon entry it will attempt to reconnect via @ref CfgDdbAccess::
+    /// createManagers.  If this is succesful, DHCP servicing is re-enabled and
+    /// server returns to normal operation.
+    ///
+    /// If reconnection fails and the maximum number of retries has not been
+    /// exhausted, it will schedule a call to itself to occur at the
+    /// configured retry interval. DHCP service remains disabled.
+    ///
+    /// If the maximum number of retries has been exhausted an error is logged
+    /// and the server shuts down.
+    /// @param db_reconnect_ctl pointer to the ReconnectCtl containing the
+    /// configured reconnect parameters
+    ///
+    void dbReconnect(ReconnectCtlPtr db_reconnect_ctl);
 
     /// @brief Callback DB backends should invoke upon loss of connectivity
+    ///
+    /// This function is invoked by DB backends when they detect a loss of
+    /// connectivity.  The parameter, db_reconnect_ctl, conveys the configured
+    /// maximum number of reconnect retries as well as the interval to wait
+    /// between retry attempts.
+    ///
+    /// If either value is zero, reconnect is presumed to be disabled and
+    /// the function will returns false.  This instructs the DB backend
+    /// layer (the caller) to treat the connectivity loss as fatal.
+    ///
+    /// Otherwise, the function saves db_reconnect_ctl and invokes
+    /// dbReconnect to initiate the reconnect process.
+    ///
+    /// @param db_reconnect_ctl pointer to the ReconnectCtl containing the
+    /// configured reconnect parameters
     bool dbLostCallback(ReconnectCtlPtr db_reconnect_ctl);
 
     /// @brief Static pointer to the sole instance of the DHCP server.
@@ -338,9 +369,6 @@ private:
     /// Shared pointer to the instance of timer @c TimerMgr is held here to
     /// make sure that the @c TimerMgr outlives instance of this class.
     TimerMgrPtr timer_mgr_;
-
-    /// @brief Pointer the current DB reconnect control values
-    ReconnectCtlPtr db_reconnect_ctl_;
 };
 
 }; // namespace isc::dhcp

diff --git a/src/lib/dhcpsrv/cfg_db_access.cc b/src/lib/dhcpsrv/cfg_db_access.cc
index 942b944646ec25f7a8facc38ad30631f4f5c9355..72ebc8ab1974e86a04377aa5f1e6162bae02fad4 100644 (file)
--- a/src/lib/dhcpsrv/cfg_db_access.cc
+++ b/src/lib/dhcpsrv/cfg_db_access.cc
@@ -38,7 +38,7 @@ CfgDbAccess::getHostDbAccessString() const {
 
 
 void
-CfgDbAccess::createManagers(DatabaseConnection::Callback db_lost_callback) const {
+CfgDbAccess::createManagers(DatabaseConnection::DbLostCallback db_lost_callback) const {
     // Recreate lease manager.
     LeaseMgrFactory::destroy();
     LeaseMgrFactory::create(getLeaseDbAccessString(), db_lost_callback);

diff --git a/src/lib/dhcpsrv/cfg_db_access.h b/src/lib/dhcpsrv/cfg_db_access.h
index 3459edf1e8a1de20044d3bde91c96e297ef704b2..845535d646ce1d68e2fac2b9cda0f9087ddf1966 100644 (file)
--- a/src/lib/dhcpsrv/cfg_db_access.h
+++ b/src/lib/dhcpsrv/cfg_db_access.h
@@ -68,7 +68,7 @@ public:
     /// @param db_lost_callback function to invoke if connectivity to
     /// either the lease or host managers, once established, is subsequently
     /// lost.
-    void createManagers(DatabaseConnection::Callback db_lost_callback = NULL) const;
+    void createManagers(DatabaseConnection::DbLostCallback db_lost_callback = NULL) const;
 
     /// @brief Unparse an access string
     ///

diff --git a/src/lib/dhcpsrv/database_connection.h b/src/lib/dhcpsrv/database_connection.h
index d4d9e484119e65e3e667370bfbf13c2197693460..7f89ad2edb17c47cb8e4adb27ccfd30b6a77259b 100644 (file)
--- a/src/lib/dhcpsrv/database_connection.h
+++ b/src/lib/dhcpsrv/database_connection.h
@@ -141,8 +141,7 @@ public:
     typedef std::map<std::string, std::string> ParameterMap;
 
     /// @brief Defines a callback prototype for propogating events upward
-    /// typedef boost::function<bool (const ParameterMap& parameters)> Callback;
-    typedef boost::function<bool (ReconnectCtlPtr db_retry)> Callback;
+    typedef boost::function<bool (ReconnectCtlPtr db_retry)> DbLostCallback;
 
     /// @brief Constructor
     ///
@@ -151,7 +150,7 @@ public:
     /// @param db_lost_callback  Optional call back function to invoke if a
     ///        successfully open connection subsequently fails
     DatabaseConnection(const ParameterMap& parameters,
-        Callback db_lost_callback = NULL)
+        DbLostCallback db_lost_callback = NULL)
         :parameters_(parameters), db_lost_callback_(db_lost_callback) {
     }
 
@@ -221,7 +220,7 @@ private:
 protected:
 
     /// @brief Optional function to invoke if the connectivity is lost
-    Callback db_lost_callback_;
+    DbLostCallback db_lost_callback_;
 };
 
 }; // end of isc::dhcp namespace

diff --git a/src/lib/dhcpsrv/host_data_source_factory.cc b/src/lib/dhcpsrv/host_data_source_factory.cc
index 0f1e15372581f669dfabf73596948d7091f39541..2216ff768bafcc6536b36e7d52c775bd90c780e3 100644 (file)
--- a/src/lib/dhcpsrv/host_data_source_factory.cc
+++ b/src/lib/dhcpsrv/host_data_source_factory.cc
@@ -46,7 +46,7 @@ HostDataSourceFactory::getHostDataSourcePtr() {
 
 void
 HostDataSourceFactory::create(const std::string& dbaccess,
-                              DatabaseConnection::Callback db_lost_callback) {
+                              DatabaseConnection::DbLostCallback db_lost_callback) {
     // Parse the access string and create a redacted string for logging.
     DatabaseConnection::ParameterMap parameters =
             DatabaseConnection::parse(dbaccess);

diff --git a/src/lib/dhcpsrv/host_data_source_factory.h b/src/lib/dhcpsrv/host_data_source_factory.h
index 151f46ffc69221f421ffb61028d7c68beb340d24..b941aa730d6c7150b589440df8913d40154acc78 100644 (file)
--- a/src/lib/dhcpsrv/host_data_source_factory.h
+++ b/src/lib/dhcpsrv/host_data_source_factory.h
@@ -67,7 +67,7 @@ public:
     /// @throw isc::dhcp::InvalidType The "type" keyword in dbaccess does not
     ///        identify a supported backend.
     static void create(const std::string& dbaccess,
-                       DatabaseConnection::Callback db_lost_callback = NULL);
+                       DatabaseConnection::DbLostCallback db_lost_callback = NULL);
 
     /// @brief Destroy host data source
     ///

diff --git a/src/lib/dhcpsrv/host_mgr.cc b/src/lib/dhcpsrv/host_mgr.cc
index 75cb6ecdecc8affac5f5133d614449c469114f28..e977eae7d636d6ce7eb7c7776b72177fee17e45d 100644 (file)
--- a/src/lib/dhcpsrv/host_mgr.cc
+++ b/src/lib/dhcpsrv/host_mgr.cc
@@ -38,7 +38,7 @@ HostMgr::getHostMgrPtr() {
 
 void
 HostMgr::create(const std::string& access,
-                DatabaseConnection::Callback db_lost_callback) {
+                DatabaseConnection::DbLostCallback db_lost_callback) {
     getHostMgrPtr().reset(new HostMgr());
 
     if (!access.empty()) {

diff --git a/src/lib/dhcpsrv/host_mgr.h b/src/lib/dhcpsrv/host_mgr.h
index a0e88a36a99d2cf70007b06ca96747913a405364..7cef82e7b8227ccbcfc0eeb67de619df926c9390 100644 (file)
--- a/src/lib/dhcpsrv/host_mgr.h
+++ b/src/lib/dhcpsrv/host_mgr.h
@@ -73,7 +73,7 @@ public:
     /// @param db_lost_callback function to invoke if connectivity to
     /// the host database is lost.
     static void create(const std::string& access = "",
-                       DatabaseConnection::Callback db_lost_callback = NULL);
+                       DatabaseConnection::DbLostCallback db_lost_callback = NULL);
 
     /// @brief Returns a sole instance of the @c HostMgr.
     ///

diff --git a/src/lib/dhcpsrv/lease_mgr_factory.cc b/src/lib/dhcpsrv/lease_mgr_factory.cc
index 54ee05771590c0ee72f29f131c9d1e43ccf71f36..261ece96b37816cdf5343c81ce4fb3fde946b458 100644 (file)
--- a/src/lib/dhcpsrv/lease_mgr_factory.cc
+++ b/src/lib/dhcpsrv/lease_mgr_factory.cc
@@ -42,7 +42,7 @@ LeaseMgrFactory::getLeaseMgrPtr() {
 
 void
 LeaseMgrFactory::create(const std::string& dbaccess,
-                        DatabaseConnection::Callback db_lost_callback) {
+                        DatabaseConnection::DbLostCallback db_lost_callback) {
     const std::string type = "type";
 
     // Parse the access string and create a redacted string for logging.

diff --git a/src/lib/dhcpsrv/lease_mgr_factory.h b/src/lib/dhcpsrv/lease_mgr_factory.h
index 54c57c3d9c15280e13a0d7a5e4164059f8ce683e..83fd6017fd35e6469bdd5eac16b7a090947128ff 100644 (file)
--- a/src/lib/dhcpsrv/lease_mgr_factory.h
+++ b/src/lib/dhcpsrv/lease_mgr_factory.h
@@ -71,7 +71,7 @@ public:
     /// @throw isc::dhcp::InvalidType The "type" keyword in dbaccess does not
     ///        identify a supported backend.
     static void create(const std::string& dbaccess, 
-                       DatabaseConnection::Callback db_lost_callback = NULL);
+                       DatabaseConnection::DbLostCallback db_lost_callback = NULL);
 
     /// @brief Destroy lease manager
     ///

diff --git a/src/lib/dhcpsrv/pgsql_connection.h b/src/lib/dhcpsrv/pgsql_connection.h
index 23d07a3ce12524c1fffa1457c99769905be32164..fc8e9de64821a4711f6fcb2fe5380a8b52711fd4 100644 (file)
--- a/src/lib/dhcpsrv/pgsql_connection.h
+++ b/src/lib/dhcpsrv/pgsql_connection.h
@@ -304,7 +304,7 @@ public:
     }
 
     PgSqlConnection(const ParameterMap& parameters, 
-                    Callback db_lost_callback)
+                    DbLostCallback db_lost_callback)
         : DatabaseConnection(parameters, db_lost_callback) {
     }
 

diff --git a/src/lib/dhcpsrv/pgsql_host_data_source.cc b/src/lib/dhcpsrv/pgsql_host_data_source.cc
index bd1e330da7290c908fb690f089b5a4ce7bc18e15..fa04926ef6e30bf91807064297b57af0911b8089 100644 (file)
--- a/src/lib/dhcpsrv/pgsql_host_data_source.cc
+++ b/src/lib/dhcpsrv/pgsql_host_data_source.cc
@@ -1285,7 +1285,7 @@ public:
     /// This constructor opens database connection and initializes prepared
     /// statements used in the queries.
     PgSqlHostDataSourceImpl(const PgSqlConnection::ParameterMap& parameters,
-                            DatabaseConnection::Callback db_lost_callback);
+                            DatabaseConnection::DbLostCallback db_lost_callback);
 
     /// @brief Destructor.
     ~PgSqlHostDataSourceImpl();
@@ -1701,7 +1701,7 @@ TaggedStatementArray tagged_statements = { {
 
 PgSqlHostDataSourceImpl::
 PgSqlHostDataSourceImpl(const PgSqlConnection::ParameterMap& parameters,
-                        DatabaseConnection::Callback db_lost_callback)
+                        DatabaseConnection::DbLostCallback db_lost_callback)
     : host_exchange_(new PgSqlHostWithOptionsExchange(PgSqlHostWithOptionsExchange::DHCP4_ONLY)),
       host_ipv6_exchange_(new PgSqlHostIPv6Exchange(PgSqlHostWithOptionsExchange::DHCP6_ONLY)),
       host_ipv46_exchange_(new PgSqlHostIPv6Exchange(PgSqlHostWithOptionsExchange::
@@ -1929,7 +1929,7 @@ PgSqlHostDataSourceImpl::checkReadOnly() const {
 
 PgSqlHostDataSource::
 PgSqlHostDataSource(const PgSqlConnection::ParameterMap& parameters,
-                    DatabaseConnection::Callback db_lost_callback)
+                    DatabaseConnection::DbLostCallback db_lost_callback)
     : impl_(new PgSqlHostDataSourceImpl(parameters, db_lost_callback)) {
 }
 

diff --git a/src/lib/dhcpsrv/pgsql_host_data_source.h b/src/lib/dhcpsrv/pgsql_host_data_source.h
index 91ffd7643e001fc42c451ba0e0dcb105d8aa4230..0c15f873932359f4ae3faf1984666f6c03261796 100644 (file)
--- a/src/lib/dhcpsrv/pgsql_host_data_source.h
+++ b/src/lib/dhcpsrv/pgsql_host_data_source.h
@@ -61,7 +61,7 @@ public:
     /// @throw isc::dhcp::DbOperationError An operation on the open database has
     ///        failed.
     PgSqlHostDataSource(const DatabaseConnection::ParameterMap& parameters,
-                        DatabaseConnection::Callback db_lost_callback = NULL); 
+                        DatabaseConnection::DbLostCallback db_lost_callback = NULL); 
 
     /// @brief Virtual destructor.
     /// Frees database resources and closes the database connection through

diff --git a/src/lib/dhcpsrv/pgsql_lease_mgr.cc b/src/lib/dhcpsrv/pgsql_lease_mgr.cc
index 049533acf13299a086d550899bf573bd0516a753..6c016333b6de34eb12320302c0bf8413a05cd354 100644 (file)
--- a/src/lib/dhcpsrv/pgsql_lease_mgr.cc
+++ b/src/lib/dhcpsrv/pgsql_lease_mgr.cc
@@ -821,7 +821,7 @@ protected:
 };
 
 PgSqlLeaseMgr::PgSqlLeaseMgr(const DatabaseConnection::ParameterMap& parameters,
-    DatabaseConnection::Callback db_lost_callback)
+    DatabaseConnection::DbLostCallback db_lost_callback)
     : LeaseMgr(), exchange4_(new PgSqlLease4Exchange()),
     exchange6_(new PgSqlLease6Exchange()), conn_(parameters, db_lost_callback) {
     conn_.openDatabase();

diff --git a/src/lib/dhcpsrv/pgsql_lease_mgr.h b/src/lib/dhcpsrv/pgsql_lease_mgr.h
index fb891ae349cc75af3e63ff19e0d526822a56968f..5c211b602b92d393e4031ee6be26396a64ec3f52 100644 (file)
--- a/src/lib/dhcpsrv/pgsql_lease_mgr.h
+++ b/src/lib/dhcpsrv/pgsql_lease_mgr.h
@@ -59,7 +59,7 @@ public:
     /// @throw isc::dhcp::DbOperationError An operation on the open database has
     ///        failed.
     PgSqlLeaseMgr(const DatabaseConnection::ParameterMap& parameters,
-                  DatabaseConnection::Callback db_lost_callback = NULL);
+                  DatabaseConnection::DbLostCallback db_lost_callback = NULL);
 
     /// @brief Destructor (closes database)
     virtual ~PgSqlLeaseMgr();

diff --git a/src/lib/dhcpsrv/tests/database_connection_unittest.cc b/src/lib/dhcpsrv/tests/database_connection_unittest.cc
index d5edcc38caa43ad373b301b20a368282198d6d9e..37fe123530cd8dd27e941adc98a14adac95b16a6 100644 (file)
--- a/src/lib/dhcpsrv/tests/database_connection_unittest.cc
+++ b/src/lib/dhcpsrv/tests/database_connection_unittest.cc
@@ -13,21 +13,28 @@
 
 using namespace isc::dhcp;
 
+/// @brief Test fixture for excersizing DbLostCallback invocation
 class DatabaseConnectionCallbackTest : public ::testing::Test {
 public:
+    /// Constructor
     DatabaseConnectionCallbackTest()
         : db_reconnect_ctl_(0) {
     }
 
-    bool dbLostCallback(ReconnectCtlPtr db_conn_retry) {
-        if (!db_conn_retry) {
-            isc_throw(isc::BadValue, "db_conn_retry should not null");
+    /// @brief Callback to register with a DatabaseConnection
+    ///
+    /// @param db_reconnect_ctl ReconnectCtl containing reconnect
+    /// parameters
+    bool dbLostCallback(ReconnectCtlPtr db_reconnect_ctl) {
+        if (!db_reconnect_ctl) {
+            isc_throw(isc::BadValue, "db_reconnect_ctl should not null");
         }
 
-        db_reconnect_ctl_ = db_conn_retry;
+        db_reconnect_ctl_ = db_reconnect_ctl;
         return (true);
     }
 
+    /// @brief Retainer for the control passed into the callback
     ReconnectCtlPtr db_reconnect_ctl_;
 };
 
@@ -49,6 +56,11 @@ TEST(DatabaseConnectionTest, getParameter) {
     EXPECT_THROW(datasrc.getParameter("param3"), isc::BadValue);
 }
 
+/// @brief NoDbLostCallback
+///
+/// This test verifies that DatabaseConnection::invokeDbLostCallback
+/// returns a false if there is connection has no registered
+/// DbLostCallback.
 TEST_F(DatabaseConnectionCallbackTest, NoDbLostCallback) {
     DatabaseConnection::ParameterMap pmap;
     pmap[std::string("max-reconnect-tries")] = std::string("3");
@@ -61,12 +73,25 @@ TEST_F(DatabaseConnectionCallbackTest, NoDbLostCallback) {
     EXPECT_FALSE(db_reconnect_ctl_);
 }
 
+
+/// @brief NoDbLostCallback
+///
+/// This test verifies that DatabaseConnection::invokeDbLostCallback
+/// safely invokes the registered DbLostCallback.  It also tests
+/// operation of DbReconnectCtl retry accounting methods.
 TEST_F(DatabaseConnectionCallbackTest, dbLostCallback) {
+    /// Create a Database configuration that includes the reconnect
+    /// control parameters.
     DatabaseConnection::ParameterMap pmap;
     pmap[std::string("max-reconnect-tries")] = std::string("3");
     pmap[std::string("reconnect-wait-time")] = std::string("60");
+
+    /// Create the connection with a DbLostCallback.
     DatabaseConnection datasrc(pmap, boost::bind(&DatabaseConnectionCallbackTest
                                                  ::dbLostCallback, this, _1));
+
+    /// We should be able to invoke the callback and glean
+    /// the correct reconnect contorl parameters from it.
     bool ret;
     ASSERT_NO_THROW(ret = datasrc.invokeDbLostCallback());
     EXPECT_TRUE(ret);
@@ -75,11 +100,15 @@ TEST_F(DatabaseConnectionCallbackTest, dbLostCallback) {
     ASSERT_EQ(3, db_reconnect_ctl_->retriesLeft());
     EXPECT_EQ(60, db_reconnect_ctl_->retryInterval());
 
+    /// Verify that checkRetries() correctly decrements
+    /// down to zero, and that retriesLeft() returns
+    /// the correct value.
     for (int i = 3; i > 1 ; --i) {
         ASSERT_EQ(i, db_reconnect_ctl_->retriesLeft());
         ASSERT_TRUE(db_reconnect_ctl_->checkRetries());
     }
 
+    /// Retries are exhausted, verify that's reflected.
     EXPECT_FALSE(db_reconnect_ctl_->checkRetries());
     EXPECT_EQ(0, db_reconnect_ctl_->retriesLeft());
     EXPECT_EQ(3, db_reconnect_ctl_->maxRetries());