From: Razvan Becheriu Date: Wed, 17 Mar 2021 15:32:54 +0000 (+0200) Subject: [#1621] addressed comments X-Git-Tag: Kea-1.9.6~24 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=f37892a82b3743b1f501c3e4d3ef2bd1979f357c;p=thirdparty%2Fkea.git [#1621] addressed comments --- diff --git a/src/hooks/dhcp/mysql_cb/mysql_cb_dhcp4.cc b/src/hooks/dhcp/mysql_cb/mysql_cb_dhcp4.cc index b201ed711e..309291da94 100644 --- a/src/hooks/dhcp/mysql_cb/mysql_cb_dhcp4.cc +++ b/src/hooks/dhcp/mysql_cb/mysql_cb_dhcp4.cc @@ -3009,10 +3009,12 @@ MySqlConfigBackendDHCPv4Impl::MySqlConfigBackendDHCPv4Impl(const DatabaseConnect tagged_statements.end()); // tagged_statements.begin() + WRITE_STMTS_BEGIN); + // Create unique timer name per instance. timer_name_ = "MySqlConfigBackend4["; timer_name_ += boost::lexical_cast(reinterpret_cast(this)); timer_name_ += "]DbReconnectTimer"; + // Create ReconnectCtl for this connection. conn_.makeReconnectCtl(timer_name_); } diff --git a/src/hooks/dhcp/mysql_cb/mysql_cb_dhcp6.cc b/src/hooks/dhcp/mysql_cb/mysql_cb_dhcp6.cc index 9425763e77..21036525f9 100644 --- a/src/hooks/dhcp/mysql_cb/mysql_cb_dhcp6.cc +++ b/src/hooks/dhcp/mysql_cb/mysql_cb_dhcp6.cc @@ -3482,10 +3482,12 @@ MySqlConfigBackendDHCPv6Impl::MySqlConfigBackendDHCPv6Impl(const DatabaseConnect tagged_statements.end()); // tagged_statements.begin() + WRITE_STMTS_BEGIN); + // Create unique timer name per instance. timer_name_ = "MySqlConfigBackend6["; timer_name_ += boost::lexical_cast(reinterpret_cast(this)); timer_name_ += "]DbReconnectTimer"; + // Create ReconnectCtl for this connection. conn_.makeReconnectCtl(timer_name_); } diff --git a/src/hooks/dhcp/mysql_cb/tests/mysql_cb_dhcp4_unittest.cc b/src/hooks/dhcp/mysql_cb/tests/mysql_cb_dhcp4_unittest.cc index 3a61b8564e..e4f18dbcab 100644 --- a/src/hooks/dhcp/mysql_cb/tests/mysql_cb_dhcp4_unittest.cc +++ b/src/hooks/dhcp/mysql_cb/tests/mysql_cb_dhcp4_unittest.cc @@ -4176,7 +4176,7 @@ public: isc::dhcp::CfgMgr::instance().clear(); } - ~MySqlConfigBackendDHCPv4DbLostCallbackTest() { + virtual ~MySqlConfigBackendDHCPv4DbLostCallbackTest() { isc::db::DatabaseConnection::db_lost_callback_ = 0; isc::db::DatabaseConnection::db_recovered_callback_ = 0; isc::db::DatabaseConnection::db_failed_callback_ = 0; @@ -4191,10 +4191,6 @@ public: /// appropriate schema and create a basic DB manager to /// wipe out any prior instance virtual void SetUp() { - isc::dhcp::MySqlConfigBackendImpl::setIOService(io_service_); - isc::db::DatabaseConnection::db_lost_callback_ = 0; - isc::db::DatabaseConnection::db_recovered_callback_ = 0; - isc::db::DatabaseConnection::db_failed_callback_ = 0; // Ensure we have the proper schema with no transient data. createMySQLSchema(); isc::dhcp::CfgMgr::instance().clear(); @@ -4206,10 +4202,6 @@ public: /// Invoked by gtest upon test exit, we destroy the schema /// we created. virtual void TearDown() { - isc::dhcp::MySqlConfigBackendImpl::setIOService(isc::asiolink::IOServicePtr()); - isc::db::DatabaseConnection::db_lost_callback_ = 0; - isc::db::DatabaseConnection::db_recovered_callback_ = 0; - isc::db::DatabaseConnection::db_failed_callback_ = 0; // If data wipe enabled, delete transient data otherwise destroy the schema destroyMySQLSchema(); isc::dhcp::CfgMgr::instance().clear(); @@ -4239,10 +4231,10 @@ public: /// @brief Verifies the CB manager's behavior if DB connection is lost /// /// This function creates a CB manager with a back end that supports - /// connectivity lost callback (currently only MySQL and PostgreSQL). It - /// verifies connectivity by issuing a known valid query. Next it simulates - /// connectivity lost by identifying and closing the socket connection to - /// the CB backend. It then reissues the query and verifies that: + /// connectivity lost callback. It verifies connectivity by issuing a known + /// valid query. Next it simulates connectivity lost by identifying and + /// closing the socket connection to the CB backend. It then reissues the + /// query and verifies that: /// -# The Query throws DbOperationError (rather than exiting) /// -# The registered DbLostCallback was invoked /// -# The registered DbRecoveredCallback was invoked @@ -4251,10 +4243,10 @@ public: /// @brief Verifies the CB manager's behavior if DB connection is lost /// /// This function creates a CB manager with a back end that supports - /// connectivity lost callback (currently only MySQL and PostgreSQL). It - /// verifies connectivity by issuing a known valid query. Next it simulates - /// connectivity lost by identifying and closing the socket connection to - /// the CB backend. It then reissues the query and verifies that: + /// connectivity lost callback. It verifies connectivity by issuing a known + /// valid query. Next it simulates connectivity lost by identifying and + /// closing the socket connection to the CB backend. It then reissues the + /// query and verifies that: /// -# The Query throws DbOperationError (rather than exiting) /// -# The registered DbLostCallback was invoked /// -# The registered DbFailedCallback was invoked @@ -4263,10 +4255,10 @@ public: /// @brief Verifies the CB manager's behavior if DB connection is lost /// /// This function creates a CB manager with a back end that supports - /// connectivity lost callback (currently only MySQL and PostgreSQL). It - /// verifies connectivity by issuing a known valid query. Next it simulates - /// connectivity lost by identifying and closing the socket connection to - /// the CB backend. It then reissues the query and verifies that: + /// connectivity lost callback. It verifies connectivity by issuing a known + /// valid query. Next it simulates connectivity lost by identifying and + /// closing the socket connection to the CB backend. It then reissues the + /// query and verifies that: /// -# The Query throws DbOperationError (rather than exiting) /// -# The registered DbLostCallback was invoked /// -# The registered DbRecoveredCallback was invoked after two reconnect @@ -4276,10 +4268,10 @@ public: /// @brief Verifies the CB manager's behavior if DB connection is lost /// /// This function creates a CB manager with a back end that supports - /// connectivity lost callback (currently only MySQL and PostgreSQL). It - /// verifies connectivity by issuing a known valid query. Next it simulates - /// connectivity lost by identifying and closing the socket connection to - /// the CB backend. It then reissues the query and verifies that: + /// connectivity lost callback. It verifies connectivity by issuing a known + /// valid query. Next it simulates connectivity lost by identifying and + /// closing the socket connection to the CB backend. It then reissues the + /// query and verifies that: /// -# The Query throws DbOperationError (rather than exiting) /// -# The registered DbLostCallback was invoked /// -# The registered DbFailedCallback was invoked after two reconnect diff --git a/src/hooks/dhcp/mysql_cb/tests/mysql_cb_dhcp6_unittest.cc b/src/hooks/dhcp/mysql_cb/tests/mysql_cb_dhcp6_unittest.cc index b2df370343..f07bf48ee7 100644 --- a/src/hooks/dhcp/mysql_cb/tests/mysql_cb_dhcp6_unittest.cc +++ b/src/hooks/dhcp/mysql_cb/tests/mysql_cb_dhcp6_unittest.cc @@ -4351,7 +4351,7 @@ public: isc::dhcp::CfgMgr::instance().clear(); } - ~MySqlConfigBackendDHCPv6DbLostCallbackTest() { + virtual ~MySqlConfigBackendDHCPv6DbLostCallbackTest() { isc::db::DatabaseConnection::db_lost_callback_ = 0; isc::db::DatabaseConnection::db_recovered_callback_ = 0; isc::db::DatabaseConnection::db_failed_callback_ = 0; @@ -4366,10 +4366,6 @@ public: /// appropriate schema and create a basic DB manager to /// wipe out any prior instance virtual void SetUp() { - isc::dhcp::MySqlConfigBackendImpl::setIOService(io_service_); - isc::db::DatabaseConnection::db_lost_callback_ = 0; - isc::db::DatabaseConnection::db_recovered_callback_ = 0; - isc::db::DatabaseConnection::db_failed_callback_ = 0; // Ensure we have the proper schema with no transient data. createMySQLSchema(); isc::dhcp::CfgMgr::instance().clear(); @@ -4381,10 +4377,6 @@ public: /// Invoked by gtest upon test exit, we destroy the schema /// we created. virtual void TearDown() { - isc::dhcp::MySqlConfigBackendImpl::setIOService(isc::asiolink::IOServicePtr()); - isc::db::DatabaseConnection::db_lost_callback_ = 0; - isc::db::DatabaseConnection::db_recovered_callback_ = 0; - isc::db::DatabaseConnection::db_failed_callback_ = 0; // If data wipe enabled, delete transient data otherwise destroy the schema destroyMySQLSchema(); isc::dhcp::CfgMgr::instance().clear(); @@ -4414,10 +4406,10 @@ public: /// @brief Verifies the CB manager's behavior if DB connection is lost /// /// This function creates a CB manager with a back end that supports - /// connectivity lost callback (currently only MySQL and PostgreSQL). It - /// verifies connectivity by issuing a known valid query. Next it simulates - /// connectivity lost by identifying and closing the socket connection to - /// the CB backend. It then reissues the query and verifies that: + /// connectivity lost callback. It verifies connectivity by issuing a known + /// valid query. Next it simulates connectivity lost by identifying and + /// closing the socket connection to the CB backend. It then reissues the + /// query and verifies that: /// -# The Query throws DbOperationError (rather than exiting) /// -# The registered DbLostCallback was invoked /// -# The registered DbRecoveredCallback was invoked @@ -4426,10 +4418,10 @@ public: /// @brief Verifies the CB manager's behavior if DB connection is lost /// /// This function creates a CB manager with a back end that supports - /// connectivity lost callback (currently only MySQL and PostgreSQL). It - /// verifies connectivity by issuing a known valid query. Next it simulates - /// connectivity lost by identifying and closing the socket connection to - /// the CB backend. It then reissues the query and verifies that: + /// connectivity lost callback. It verifies connectivity by issuing a known + /// valid query. Next it simulates connectivity lost by identifying and + /// closing the socket connection to the CB backend. It then reissues the + /// query and verifies that: /// -# The Query throws DbOperationError (rather than exiting) /// -# The registered DbLostCallback was invoked /// -# The registered DbFailedCallback was invoked @@ -4438,10 +4430,10 @@ public: /// @brief Verifies the CB manager's behavior if DB connection is lost /// /// This function creates a CB manager with a back end that supports - /// connectivity lost callback (currently only MySQL and PostgreSQL). It - /// verifies connectivity by issuing a known valid query. Next it simulates - /// connectivity lost by identifying and closing the socket connection to - /// the CB backend. It then reissues the query and verifies that: + /// connectivity lost callback. It verifies connectivity by issuing a known + /// valid query. Next it simulates connectivity lost by identifying and + /// closing the socket connection to the CB backend. It then reissues the + /// query and verifies that: /// -# The Query throws DbOperationError (rather than exiting) /// -# The registered DbLostCallback was invoked /// -# The registered DbRecoveredCallback was invoked after two reconnect @@ -4451,10 +4443,10 @@ public: /// @brief Verifies the CB manager's behavior if DB connection is lost /// /// This function creates a CB manager with a back end that supports - /// connectivity lost callback (currently only MySQL and PostgreSQL). It - /// verifies connectivity by issuing a known valid query. Next it simulates - /// connectivity lost by identifying and closing the socket connection to - /// the CB backend. It then reissues the query and verifies that: + /// connectivity lost callback. It verifies connectivity by issuing a known + /// valid query. Next it simulates connectivity lost by identifying and + /// closing the socket connection to the CB backend. It then reissues the + /// query and verifies that: /// -# The Query throws DbOperationError (rather than exiting) /// -# The registered DbLostCallback was invoked /// -# The registered DbFailedCallback was invoked after two reconnect diff --git a/src/lib/dhcpsrv/mysql_host_data_source.cc b/src/lib/dhcpsrv/mysql_host_data_source.cc index f4818dcdf2..e2e482213d 100644 --- a/src/lib/dhcpsrv/mysql_host_data_source.cc +++ b/src/lib/dhcpsrv/mysql_host_data_source.cc @@ -2781,6 +2781,7 @@ MySqlHostDataSourceImpl::MySqlHostDataSourceImpl(const DatabaseConnection::Param : parameters_(parameters), ip_reservations_unique_(true), unusable_(false), timer_name_("") { + // Create unique timer name per instance. timer_name_ = "MySqlHostMgr["; timer_name_ += boost::lexical_cast(reinterpret_cast(this)); timer_name_ += "]DbReconnectTimer"; @@ -2841,6 +2842,7 @@ MySqlHostDataSourceImpl::createContext() const { ctx->host_ipv6_reservation_exchange_.reset(new MySqlIPv6ReservationExchange()); ctx->host_option_exchange_.reset(new MySqlOptionExchange()); + // Create ReconnectCtl for this connection. ctx->conn_.makeReconnectCtl(timer_name_); return (ctx); @@ -2872,7 +2874,6 @@ MySqlHostDataSourceImpl::dbReconnect(ReconnectCtlPtr db_reconnect_ctl) { HostMgr::addBackend(hds); } } - reopened = true; } catch (const std::exception& ex) { LOG_ERROR(dhcpsrv_logger, DHCPSRV_MYSQL_HOST_DB_RECONNECT_ATTEMPT_FAILED) @@ -2902,7 +2903,6 @@ MySqlHostDataSourceImpl::dbReconnect(ReconnectCtlPtr db_reconnect_ctl) { // Invoke application layer connection failed callback. DatabaseConnection::invokeDbFailedCallback(db_reconnect_ctl); - return (false); } diff --git a/src/lib/dhcpsrv/mysql_lease_mgr.cc b/src/lib/dhcpsrv/mysql_lease_mgr.cc index f1065c0797..2c9abe76a5 100644 --- a/src/lib/dhcpsrv/mysql_lease_mgr.cc +++ b/src/lib/dhcpsrv/mysql_lease_mgr.cc @@ -1783,6 +1783,7 @@ MySqlLeaseMgr::MySqlLeaseContextAlloc::~MySqlLeaseContextAlloc() { MySqlLeaseMgr::MySqlLeaseMgr(const DatabaseConnection::ParameterMap& parameters) : parameters_(parameters), timer_name_("") { + // Create unique timer name per instance. timer_name_ = "MySqlLeaseMgr["; timer_name_ += boost::lexical_cast(reinterpret_cast(this)); timer_name_ += "]DbReconnectTimer"; @@ -1825,7 +1826,6 @@ MySqlLeaseMgr::dbReconnect(ReconnectCtlPtr db_reconnect_ctl) { CfgDbAccessPtr cfg_db = CfgMgr::instance().getCurrentCfg()->getCfgDbAccess(); LeaseMgrFactory::destroy(); LeaseMgrFactory::create(cfg_db->getLeaseDbAccessString()); - reopened = true; } catch (const std::exception& ex) { LOG_ERROR(dhcpsrv_logger, DHCPSRV_MYSQL_LEASE_DB_RECONNECT_ATTEMPT_FAILED) @@ -1855,7 +1855,6 @@ MySqlLeaseMgr::dbReconnect(ReconnectCtlPtr db_reconnect_ctl) { // Invoke application layer connection failed callback. DatabaseConnection::invokeDbFailedCallback(db_reconnect_ctl); - return (false); } @@ -1897,6 +1896,7 @@ MySqlLeaseMgr::createContext() const { ctx->exchange4_.reset(new MySqlLease4Exchange()); ctx->exchange6_.reset(new MySqlLease6Exchange()); + // Create ReconnectCtl for this connection. ctx->conn_.makeReconnectCtl(timer_name_); return (ctx); diff --git a/src/lib/dhcpsrv/pgsql_host_data_source.cc b/src/lib/dhcpsrv/pgsql_host_data_source.cc index d4dba22f0b..6fec6459a9 100644 --- a/src/lib/dhcpsrv/pgsql_host_data_source.cc +++ b/src/lib/dhcpsrv/pgsql_host_data_source.cc @@ -2222,6 +2222,7 @@ PgSqlHostDataSourceImpl::PgSqlHostDataSourceImpl(const DatabaseConnection::Param : parameters_(parameters), ip_reservations_unique_(true), unusable_(false), timer_name_("") { + // Create unique timer name per instance. timer_name_ = "PgSqlHostMgr["; timer_name_ += boost::lexical_cast(reinterpret_cast(this)); timer_name_ += "]DbReconnectTimer"; @@ -2277,6 +2278,7 @@ PgSqlHostDataSourceImpl::createContext() const { ctx->host_ipv6_reservation_exchange_.reset(new PgSqlIPv6ReservationExchange()); ctx->host_option_exchange_.reset(new PgSqlOptionExchange()); + // Create ReconnectCtl for this connection. ctx->conn_.makeReconnectCtl(timer_name_); return (ctx); @@ -2308,7 +2310,6 @@ PgSqlHostDataSourceImpl::dbReconnect(ReconnectCtlPtr db_reconnect_ctl) { HostMgr::addBackend(hds); } } - reopened = true; } catch (const std::exception& ex) { LOG_ERROR(dhcpsrv_logger, DHCPSRV_PGSQL_HOST_DB_RECONNECT_ATTEMPT_FAILED) @@ -2338,7 +2339,6 @@ PgSqlHostDataSourceImpl::dbReconnect(ReconnectCtlPtr db_reconnect_ctl) { // Invoke application layer connection failed callback. DatabaseConnection::invokeDbFailedCallback(db_reconnect_ctl); - return (false); } diff --git a/src/lib/dhcpsrv/pgsql_lease_mgr.cc b/src/lib/dhcpsrv/pgsql_lease_mgr.cc index da39f2d973..eed79d2eec 100644 --- a/src/lib/dhcpsrv/pgsql_lease_mgr.cc +++ b/src/lib/dhcpsrv/pgsql_lease_mgr.cc @@ -1217,6 +1217,7 @@ PgSqlLeaseMgr::PgSqlLeaseContextAlloc::~PgSqlLeaseContextAlloc() { PgSqlLeaseMgr::PgSqlLeaseMgr(const DatabaseConnection::ParameterMap& parameters) : parameters_(parameters), timer_name_("") { + // Create unique timer name per instance. timer_name_ = "PgSqlLeaseMgr["; timer_name_ += boost::lexical_cast(reinterpret_cast(this)); timer_name_ += "]DbReconnectTimer"; @@ -1259,7 +1260,6 @@ PgSqlLeaseMgr::dbReconnect(ReconnectCtlPtr db_reconnect_ctl) { CfgDbAccessPtr cfg_db = CfgMgr::instance().getCurrentCfg()->getCfgDbAccess(); LeaseMgrFactory::destroy(); LeaseMgrFactory::create(cfg_db->getLeaseDbAccessString()); - reopened = true; } catch (const std::exception& ex) { LOG_ERROR(dhcpsrv_logger, DHCPSRV_PGSQL_LEASE_DB_RECONNECT_ATTEMPT_FAILED) @@ -1289,7 +1289,6 @@ PgSqlLeaseMgr::dbReconnect(ReconnectCtlPtr db_reconnect_ctl) { // Invoke application layer connection failed callback. DatabaseConnection::invokeDbFailedCallback(db_reconnect_ctl); - return (false); } @@ -1339,6 +1338,7 @@ PgSqlLeaseMgr::createContext() const { ctx->exchange4_.reset(new PgSqlLease4Exchange()); ctx->exchange6_.reset(new PgSqlLease6Exchange()); + // Create ReconnectCtl for this connection. ctx->conn_.makeReconnectCtl(timer_name_); return (ctx); diff --git a/src/lib/dhcpsrv/testutils/generic_host_data_source_unittest.h b/src/lib/dhcpsrv/testutils/generic_host_data_source_unittest.h index 18baa31ec6..8444f93704 100644 --- a/src/lib/dhcpsrv/testutils/generic_host_data_source_unittest.h +++ b/src/lib/dhcpsrv/testutils/generic_host_data_source_unittest.h @@ -549,7 +549,7 @@ public: isc::dhcp::CfgMgr::instance().clear(); } - ~HostMgrDbLostCallbackTest() { + virtual ~HostMgrDbLostCallbackTest() { isc::db::DatabaseConnection::db_lost_callback_ = 0; isc::db::DatabaseConnection::db_recovered_callback_ = 0; isc::db::DatabaseConnection::db_failed_callback_ = 0; @@ -564,10 +564,6 @@ public: /// appropriate schema and create a basic host manager to /// wipe out any prior instance virtual void SetUp() { - isc::dhcp::HostMgr::setIOService(io_service_); - isc::db::DatabaseConnection::db_lost_callback_ = 0; - isc::db::DatabaseConnection::db_recovered_callback_ = 0; - isc::db::DatabaseConnection::db_failed_callback_ = 0; // Ensure we have the proper schema with no transient data. createSchema(); // Wipe out any pre-existing mgr @@ -580,10 +576,6 @@ public: /// Invoked by gtest upon test exit, we destroy the schema /// we created. virtual void TearDown() { - isc::dhcp::HostMgr::setIOService(isc::asiolink::IOServicePtr()); - isc::db::DatabaseConnection::db_lost_callback_ = 0; - isc::db::DatabaseConnection::db_recovered_callback_ = 0; - isc::db::DatabaseConnection::db_failed_callback_ = 0; // If data wipe enabled, delete transient data otherwise destroy the schema destroySchema(); isc::dhcp::CfgMgr::instance().clear(); diff --git a/src/lib/mysql/mysql_connection.h b/src/lib/mysql/mysql_connection.h index 8700289818..43e804eb57 100644 --- a/src/lib/mysql/mysql_connection.h +++ b/src/lib/mysql/mysql_connection.h @@ -658,11 +658,15 @@ public: /// /// @note The recover function must be run on the IO Service thread. void startRecoverDbConnection() { - if (!io_service_ && io_service_access_callback_) { - io_service_ = (*io_service_access_callback_)(); - } - if (callback_ && io_service_) { - io_service_->post(std::bind(callback_, reconnectCtl())); + if (callback_) { + if (!io_service_ && io_service_access_callback_) { + io_service_ = (*io_service_access_callback_)(); + io_service_access_callback_.reset(); + } + + if (io_service_) { + io_service_->post(std::bind(callback_, reconnectCtl())); + } } } diff --git a/src/lib/pgsql/pgsql_connection.h b/src/lib/pgsql/pgsql_connection.h index da69a7a99b..c2503b77ea 100644 --- a/src/lib/pgsql/pgsql_connection.h +++ b/src/lib/pgsql/pgsql_connection.h @@ -426,11 +426,15 @@ public: /// /// @note The recover function must be run on the IO Service thread. void startRecoverDbConnection() { - if (!io_service_ && io_service_access_callback_) { - io_service_ = (*io_service_access_callback_)(); - } - if (callback_ && io_service_) { - io_service_->post(std::bind(callback_, reconnectCtl())); + if (callback_) { + if (!io_service_ && io_service_access_callback_) { + io_service_ = (*io_service_access_callback_)(); + io_service_access_callback_.reset(); + } + + if (io_service_) { + io_service_->post(std::bind(callback_, reconnectCtl())); + } } }