From 61ca0234985904bbc175cf906fa1920c144115a0 Mon Sep 17 00:00:00 2001 From: Marcin Siodelski Date: Mon, 30 May 2016 11:30:26 +0200 Subject: [PATCH] [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 | 67 +++++++++++++++++++++++ src/lib/dhcpsrv/mysql_host_data_source.cc | 36 +++--------- src/lib/dhcpsrv/mysql_lease_mgr.cc | 30 +--------- src/lib/dhcpsrv/mysql_lease_mgr.h | 18 +----- 4 files changed, 77 insertions(+), 74 deletions(-) diff --git a/src/lib/dhcpsrv/mysql_connection.h b/src/lib/dhcpsrv/mysql_connection.h index 636ae700f4..f5e7ab022b 100755 --- a/src/lib/dhcpsrv/mysql_connection.h +++ b/src/lib/dhcpsrv/mysql_connection.h @@ -8,8 +8,12 @@ #define MYSQL_CONNECTION_H #include +#include +#include #include #include +#include +#include #include #include @@ -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 + 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(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(index)] + << ">, reason: " + << mysql_error(mysql_) << " (error code " + << mysql_errno(mysql_) << ")"); + } + } + } + /// @brief Prepared statements /// /// This field is public, because it is used heavily from MySqlConnection diff --git a/src/lib/dhcpsrv/mysql_host_data_source.cc b/src/lib/dhcpsrv/mysql_host_data_source.cc index 0b2dbd4235..21c9d981e7 100644 --- a/src/lib/dhcpsrv/mysql_host_data_source.cc +++ b/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 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::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); diff --git a/src/lib/dhcpsrv/mysql_lease_mgr.cc b/src/lib/dhcpsrv/mysql_lease_mgr.cc index ed3ef03d61..3d60dd6f49 100755 --- a/src/lib/dhcpsrv/mysql_lease_mgr.cc +++ b/src/lib/dhcpsrv/mysql_lease_mgr.cc @@ -15,7 +15,6 @@ #include #include -#include #include #include @@ -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 diff --git a/src/lib/dhcpsrv/mysql_lease_mgr.h b/src/lib/dhcpsrv/mysql_lease_mgr.h index a8b2e80274..e328711b6b 100755 --- a/src/lib/dhcpsrv/mysql_lease_mgr.h +++ b/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 -- 2.47.2