From: Thomas Markwalder Date: Thu, 31 May 2018 13:41:46 +0000 (-0400) Subject: [5629] MySQL Lease back end now validates schema after connecting X-Git-Tag: trac5631_base~7^2~4 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=7614eb8d2b6206cef284dbd566524d56d9562e82;p=thirdparty%2Fkea.git [5629] MySQL Lease back end now validates schema after connecting src/lib/dhcpsrv/mysql_lease_mgr.* MySqlLeaseMgr::MySqlLeaseMgr() - now validates schema after connecting MySqlLeaseMgr::getVersion() - no longer relies on pre-prepared statement or formal statement execution error handling src/lib/dhcpsrv/tests/generic_lease_mgr_unittest.cc LeaseMgrDbLostCallbackTest::testDbLostCallback() - replaced use of getVersion() with getLease4() for testing DB usability --- diff --git a/src/lib/dhcpsrv/mysql_lease_mgr.cc b/src/lib/dhcpsrv/mysql_lease_mgr.cc index 59d105ee42..4367a389e5 100644 --- a/src/lib/dhcpsrv/mysql_lease_mgr.cc +++ b/src/lib/dhcpsrv/mysql_lease_mgr.cc @@ -209,8 +209,6 @@ tagged_statements = { { "WHERE state != ? AND expire < ? " "ORDER BY expire ASC " "LIMIT ?"}, - {MySqlLeaseMgr::GET_VERSION, - "SELECT version, minor FROM schema_version"}, {MySqlLeaseMgr::INSERT_LEASE4, "INSERT INTO lease4(address, hwaddr, client_id, " "valid_lifetime, expire, subnet_id, " @@ -1501,6 +1499,16 @@ MySqlLeaseMgr::MySqlLeaseMgr(const MySqlConnection::ParameterMap& parameters) // Open the database. conn_.openDatabase(); + // Test schema version before we try to prepare statements. + std::pair version = getVersion(); + if (version.first != MYSQL_SCHEMA_VERSION_MAJOR || + version.second != MYSQL_SCHEMA_VERSION_MINOR) { + isc_throw(DbOpenError, "MySQL schema version is: " + << version.first << "." << version.second << ", need version: " + << MYSQL_SCHEMA_VERSION_MAJOR << "." + << MYSQL_SCHEMA_VERSION_MINOR); + } + // Enable autocommit. To avoid a flush to disk on every commit, the global // parameter innodb_flush_log_at_trx_commit should be set to 2. This will // cause the changes to be written to the log, but flushed to disk in the @@ -2372,40 +2380,60 @@ MySqlLeaseMgr::getDescription() const { std::pair MySqlLeaseMgr::getVersion() const { - const StatementIndex stindex = GET_VERSION; - LOG_DEBUG(dhcpsrv_logger, DHCPSRV_DBG_TRACE_DETAIL, DHCPSRV_MYSQL_GET_VERSION); - uint32_t major; // Major version number - uint32_t minor; // Minor version number + // Allocate a new statement. + MYSQL_STMT *stmt = mysql_stmt_init(conn_.mysql_); + if (stmt == NULL) { + isc_throw(DbOperationError, "unable to allocate MySQL prepared " + "statement structure, reason: " << mysql_error(conn_.mysql_)); + } + + // Prepare the statement from SQL text. + const char* version_sql = "SELECT version, minor FROM schema_version"; + int status = mysql_stmt_prepare(stmt, version_sql, strlen(version_sql)); + if (status != 0) { + isc_throw(DbOperationError, "unable to prepare MySQL statement <" + << version_sql << ">, reason: " << mysql_error(conn_.mysql_)); + } - // Execute the prepared statement - int status = mysql_stmt_execute(conn_.statements_[stindex]); - checkError(status, stindex, "unable to execute statement"); + // Execute the prepared statement. + if (mysql_stmt_execute(stmt) != 0) { + isc_throw(DbOperationError, "cannot execute schema version query:" + << version_sql << ">, reason: " << mysql_errno(conn_.mysql_) << ")"); + } // Bind the output of the statement to the appropriate variables. MYSQL_BIND bind[2]; memset(bind, 0, sizeof(bind)); + uint32_t major; bind[0].buffer_type = MYSQL_TYPE_LONG; bind[0].is_unsigned = 1; bind[0].buffer = &major; bind[0].buffer_length = sizeof(major); + uint32_t minor; bind[1].buffer_type = MYSQL_TYPE_LONG; bind[1].is_unsigned = 1; bind[1].buffer = &minor; bind[1].buffer_length = sizeof(minor); - status = mysql_stmt_bind_result(conn_.statements_[stindex], bind); - checkError(status, stindex, "unable to bind result set"); + if (mysql_stmt_bind_result(stmt, bind)) { + isc_throw(DbOperationError, "unable to bind result set for:" + << version_sql << ">, reason: " << mysql_errno(conn_.mysql_) << ")"); + } + + // Fetch the data. + if (mysql_stmt_fetch(stmt)) { + mysql_stmt_close(stmt); + isc_throw(DbOperationError, "unable to bind result set for:" + << version_sql << ">, reason: " << mysql_errno(conn_.mysql_) << ")"); + } - // Fetch the data and set up the "release" object to release associated - // resources when this method exits then retrieve the data. - MySqlFreeResult fetch_release(conn_.statements_[stindex]); - status = mysql_stmt_fetch(conn_.statements_[stindex]); - checkError(status, stindex, "unable to fetch result set"); + // Discard the statement and its resources + mysql_stmt_close(stmt); return (std::make_pair(major, minor)); } diff --git a/src/lib/dhcpsrv/mysql_lease_mgr.h b/src/lib/dhcpsrv/mysql_lease_mgr.h index fb2a427659..497ed2104c 100644 --- a/src/lib/dhcpsrv/mysql_lease_mgr.h +++ b/src/lib/dhcpsrv/mysql_lease_mgr.h @@ -53,7 +53,8 @@ public: /// concerned with the database. /// /// @throw isc::dhcp::NoDatabaseName Mandatory database name not given - /// @throw isc::dhcp::DbOpenError Error opening the database + /// @throw isc::dhcp::DbOpenError Error opening the database or the schema + /// version is incorrect. /// @throw isc::dhcp::DbOperationError An operation on the open database has /// failed. MySqlLeaseMgr(const DatabaseConnection::ParameterMap& parameters); @@ -472,6 +473,11 @@ public: /// @brief Returns backend version. /// + /// The method is called by the constructor after opening the database + /// but prior to preparing SQL statements, to verify that the schema version + /// is correct. Thus it does not rely on a pre-prepared statement or + /// formal statement execution error checking. + /// /// @return Version number as a pair of unsigned integers. "first" is the /// major version number, "second" the minor number. /// @@ -517,7 +523,6 @@ public: GET_LEASE6_DUID_IAID_SUBID, // Get lease6 by DUID, IAID and subnet ID GET_LEASE6_SUBID, // Get IPv6 leases by subnet ID GET_LEASE6_EXPIRE, // Get lease6 by expiration. - GET_VERSION, // Obtain version number INSERT_LEASE4, // Add entry to lease4 table INSERT_LEASE6, // Add entry to lease6 table UPDATE_LEASE4, // Update a Lease4 entry diff --git a/src/lib/dhcpsrv/tests/generic_lease_mgr_unittest.cc b/src/lib/dhcpsrv/tests/generic_lease_mgr_unittest.cc index 90a7f28e7a..ed1208fe21 100644 --- a/src/lib/dhcpsrv/tests/generic_lease_mgr_unittest.cc +++ b/src/lib/dhcpsrv/tests/generic_lease_mgr_unittest.cc @@ -2842,16 +2842,19 @@ LeaseMgrDbLostCallbackTest::testDbLostCallback() { // Clear the callback invocation marker. callback_called_ = false; - // Verify we can execute a query. + // Verify we can execute a query. We do not care if + // we find a lease or not. LeaseMgr& lm = LeaseMgrFactory::instance(); - pair version; - ASSERT_NO_THROW(version = lm.getVersion()); + + Lease4Ptr lease; + ASSERT_NO_THROW(lease = lm.getLease4(IOAddress("192.0.1.0"))); // Now close the sql socket out from under backend client ASSERT_EQ(0, close(sql_socket)); // A query should fail with DbOperationError. - ASSERT_THROW(version = lm.getVersion(), DbOperationError); + ASSERT_THROW(lease = lm.getLease4(IOAddress("192.0.1.0")), + DbOperationError); // Our lost connectivity callback should have been invoked. EXPECT_TRUE(callback_called_);