[4281] Addressed further review comments.

- Always invoke mysql_insert_id to retrieve host_id value
- exit if connection with  MySql database is lost

src/lib/dhcpsrv/mysql_connection.h

@@ -8,8 +8,12 @@
 #define MYSQL_CONNECTION_H
 
 #include <dhcpsrv/database_connection.h>
+#include <dhcpsrv/dhcpsrv_log.h>
+#include <exceptions/exceptions.h>
 #include <boost/scoped_ptr.hpp>
 #include <mysql.h>
+#include <mysqld_error.h>
+#include <errmsg.h>
 #include <vector>
 #include <stdint.h>
 
@@ -335,6 +339,69 @@ public:
     /// @throw DbOperationError If the rollback failed.
     void rollback();
 
+    /// @brief Check Error and Throw Exception
+    ///
+    /// Virtually all MySQL functions return a status which, if non-zero,
+    /// indicates an error.  This function centralizes the error checking
+    /// code.
+    ///
+    /// It is used to determine whether or not the function succeeded, and
+    /// in the event of failures, decide whether or not those failures are
+    /// recoverable.
+    ///
+    /// If the error is recoverable, the method will throw a DbOperationError.
+    /// In the error is deemed unrecoverable, such as a loss of connectivity
+    /// with the server, this method will log the error and call exit(-1);
+    ///
+    /// @todo Calling exit() is viewed as a short term solution for Kea 1.0.
+    /// Two tickets are likely to alter this behavior, first is #3639, which
+    /// calls for the ability to attempt to reconnect to the database. The
+    /// second ticket, #4087 which calls for the implementation of a generic,
+    /// FatalException class which will propagate outward.
+    ///
+    /// @param status Status code: non-zero implies an error
+    /// @param index Index of statement that caused the error
+    /// @param what High-level description of the error
+    ///
+    /// @tparam Enumeration representing index of a statement to which an
+    /// error pertains.
+    ///
+    /// @throw isc::dhcp::DbOperationError An operation on the open database has
+    ///        failed.
+    template<typename StatementIndex>
+    void checkError(const int status, const StatementIndex& index,
+                    const char* what) const {
+        if (status != 0) {
+            switch(mysql_errno(mysql_)) {
+                // These are the ones we consider fatal. Remember this method is
+                // used to check errors of API calls made subsequent to successfully
+                // connecting.  Errors occuring while attempting to connect are
+                // checked in the connection code. An alternative would be to call
+                // mysql_ping() - assuming autoreconnect is off. If that fails
+                // then we know connection is toast.
+            case CR_SERVER_GONE_ERROR:
+            case CR_SERVER_LOST:
+            case CR_OUT_OF_MEMORY:
+            case CR_CONNECTION_ERROR:
+                // We're exiting on fatal
+                LOG_ERROR(dhcpsrv_logger, DHCPSRV_MYSQL_FATAL_ERROR)
+                         .arg(what)
+                         .arg(text_statements_[static_cast<int>(index)])
+                         .arg(mysql_error(mysql_))
+                         .arg(mysql_errno(mysql_));
+                exit (-1);
+
+            default:
+                // Connection is ok, so it must be an SQL error
+                isc_throw(DbOperationError, what << " for <"
+                          << text_statements_[static_cast<int>(index)]
+                          << ">, reason: "
+                          << mysql_error(mysql_) << " (error code "
+                          << mysql_errno(mysql_) << ")");
+            }
+        }
+    }
+
     /// @brief Prepared statements
     ///
     /// This field is public, because it is used heavily from MySqlConnection

src/lib/dhcpsrv/mysql_host_data_source.cc

@@ -1691,25 +1691,20 @@ public:
     /// @param stindex Index of a statement being executed.
     /// @param options_cfg An object holding a collection of options to be
     /// inserted into the database.
-    /// @param host_id Host identifier reference to a value to which
-    /// host identifier will be assigned if the current host_id value is 0.
-    /// The host identifier is retrieve from the database using the
-    /// mysql_insert_id function.
+    /// @param host_id Host identifier retrieved using @c mysql_insert_id.
     void addOptions(const StatementIndex& stindex, const ConstCfgOptionPtr& options_cfg,
-                    uint64_t& host_id);
+                    const uint64_t host_id);
 
     /// @brief Check Error and Throw Exception
     ///
-    /// Virtually all MySQL functions return a status which, if non-zero,
-    /// indicates an error.  This inline function conceals a lot of error
-    /// checking/exception-throwing code.
+    /// This method invokes @ref MySqlConnection::checkError.
     ///
     /// @param status Status code: non-zero implies an error
     /// @param index Index of statement that caused the error
     /// @param what High-level description of the error
     ///
-    /// @throw isc::dhcp::DbOperationError An operation on the open database
-    ///        has failed.
+    /// @throw isc::dhcp::DbOperationError An operation on the open database has
+    ///        failed.
     void checkError(const int status, const StatementIndex index,
                     const char* what) const;
 
@@ -2026,7 +2021,7 @@ MySqlHostDataSourceImpl::addOption(const StatementIndex& stindex,
 void
 MySqlHostDataSourceImpl::addOptions(const StatementIndex& stindex,
                                     const ConstCfgOptionPtr& options_cfg,
-                                    uint64_t& host_id) {
+                                    const uint64_t host_id) {
     // Get option space names and vendor space names and combine them within a
     // single list.
     std::list<std::string> option_spaces = options_cfg->getOptionSpaceNames();
@@ -2034,12 +2029,6 @@ MySqlHostDataSourceImpl::addOptions(const StatementIndex& stindex,
     option_spaces.insert(option_spaces.end(), vendor_spaces.begin(),
                          vendor_spaces.end());
 
-    // Retrieve host id only if there are any options to be added and the
-    // host id hasn't been retrieved yet.
-    if ((host_id == 0) && !option_spaces.empty()) {
-        host_id = mysql_insert_id(conn_.mysql_);
-    }
-
     // For each option space retrieve all options and insert them into the
     // database.
     for (std::list<std::string>::const_iterator space = option_spaces.begin();
@@ -2059,12 +2048,7 @@ void
 MySqlHostDataSourceImpl::
 checkError(const int status, const StatementIndex index,
            const char* what) const {
-    if (status != 0) {
-        isc_throw(DbOperationError, what << " for <"
-                  << conn_.text_statements_[index] << ">, reason: "
-                  << mysql_error(conn_.mysql_) << " (error code "
-                  << mysql_errno(conn_.mysql_) << ")");
-    }
+    conn_.checkError(status, index, what);
 }
 
 void
@@ -2198,7 +2182,7 @@ MySqlHostDataSource::add(const HostPtr& host) {
     impl_->addStatement(MySqlHostDataSourceImpl::INSERT_HOST, bind);
 
     // Gets the last inserted hosts id
-    uint64_t host_id = 0;
+    uint64_t host_id = mysql_insert_id(impl_->conn_.mysql_);
 
     // Insert DHCPv4 options.
     ConstCfgOptionPtr cfg_option4 = host->getCfgOption4();
@@ -2217,10 +2201,6 @@ MySqlHostDataSource::add(const HostPtr& host) {
     // Insert IPv6 reservations.
     IPv6ResrvRange v6resv = host->getIPv6Reservations();
     if (std::distance(v6resv.first, v6resv.second) > 0) {
-        // Set host_id if it wasn't set when we inserted options.
-        if (host_id == 0) {
-            host_id = mysql_insert_id(impl_->conn_.mysql_);
-        }
         for (IPv6ResrvIterator resv = v6resv.first; resv != v6resv.second;
              ++resv) {
             impl_->addResv(resv->second, host_id);

src/lib/dhcpsrv/mysql_lease_mgr.cc

@@ -15,7 +15,6 @@
 
 #include <boost/static_assert.hpp>
 #include <mysqld_error.h>
-#include <errmsg.h>
 
 #include <iostream>
 #include <iomanip>
@@ -2047,34 +2046,7 @@ MySqlLeaseMgr::rollback() {
 void
 MySqlLeaseMgr::checkError(int status, StatementIndex index,
                            const char* what) const {
-    if (status != 0) {
-        switch(mysql_errno(conn_.mysql_)) {
-            // These are the ones we consider fatal. Remember this method is
-            // used to check errors of API calls made subsequent to successfully
-            // connecting.  Errors occuring while attempting to connect are
-            // checked in the connection code. An alternative would be to call
-            // mysql_ping() - assuming autoreconnect is off. If that fails
-            // then we know connection is toast.
-            case CR_SERVER_GONE_ERROR:
-            case CR_SERVER_LOST:
-            case CR_OUT_OF_MEMORY:
-            case CR_CONNECTION_ERROR:
-                // We're exiting on fatal
-                LOG_ERROR(dhcpsrv_logger, DHCPSRV_MYSQL_FATAL_ERROR)
-                         .arg(what)
-                         .arg(conn_.text_statements_[index])
-                         .arg(mysql_error(conn_.mysql_))
-                         .arg(mysql_errno(conn_.mysql_));
-                exit (-1);
-
-            default:
-                // Connection is ok, so it must be an SQL error
-                isc_throw(DbOperationError, what << " for <"
-                          << conn_.text_statements_[index] << ">, reason: "
-                          << mysql_error(conn_.mysql_) << " (error code "
-                          << mysql_errno(conn_.mysql_) << ")");
-        }
-    }
+    conn_.checkError(status, index, what);
 }
 
 }; // end of isc::dhcp namespace

src/lib/dhcpsrv/mysql_lease_mgr.h

@@ -593,23 +593,7 @@ private:
 
     /// @brief Check Error and Throw Exception
     ///
-    /// Virtually all MySQL functions return a status which, if non-zero,
-    /// indicates an error.  This function centralizes the error checking
-    /// code.
-    ///
-    /// It is used to determine whether or not the function succeeded, and
-    /// in the event of failures, decide whether or not those failures are
-    /// recoverable.
-    ///
-    /// If the error is recoverable, the method will throw a DbOperationError.
-    /// In the error is deemed unrecoverable, such as a loss of connectivity
-    /// with the server, this method will log the error and call exit(-1);
-    ///
-    /// @todo Calling exit() is viewed as a short term solution for Kea 1.0.
-    /// Two tickets are likely to alter this behavior, first is #3639, which
-    /// calls for the ability to attempt to reconnect to the database. The
-    /// second ticket, #4087 which calls for the implementation of a generic,
-    /// FatalException class which will propagate outward.
+    /// This method invokes @ref MySqlConnection::checkError.
     ///
     /// @param status Status code: non-zero implies an error
     /// @param index Index of statement that caused the error