From: Marcin Siodelski Date: Wed, 17 Aug 2016 09:22:24 +0000 (+0200) Subject: [4489] Addressed review comments. X-Git-Tag: trac4631_base~9^2~6 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=4a8efcb45a81052889ae1a7258bec5d49db0bc47;p=thirdparty%2Fkea.git [4489] Addressed review comments. The only review item not addressed with this commit is the implementation of unit test that operates on the read only database, i.e. the database containing tables on which the given user only has SELECT privileges. --- diff --git a/src/lib/dhcpsrv/database_connection.cc b/src/lib/dhcpsrv/database_connection.cc index 983a752d96..0aa886fa88 100644 --- a/src/lib/dhcpsrv/database_connection.cc +++ b/src/lib/dhcpsrv/database_connection.cc @@ -1,10 +1,11 @@ -// Copyright (C) 2015 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2015-2016 Internet Systems Consortium, Inc. ("ISC") // // This Source Code Form is subject to the terms of the Mozilla Public // License, v. 2.0. If a copy of the MPL was not distributed with this // file, You can obtain one at http://mozilla.org/MPL/2.0/. #include +#include #include #include @@ -86,5 +87,24 @@ DatabaseConnection::redactedAccessString(const ParameterMap& parameters) { return (access); } +bool +DatabaseConnection::configuredReadOnly() const { + std::string readonly_value = "false"; + try { + readonly_value = getParameter("readonly"); + boost::algorithm::to_lower(readonly_value); + } catch (...) { + // Parameter "readonly" hasn't been specified so we simply use + // the default value of "false". + } + + if ((readonly_value != "false") && (readonly_value != "true")) { + isc_throw(DbInvalidReadOnly, "invalid value '" << readonly_value + << "' specified for boolean parameter 'readonly'"); + } + + return (readonly_value == "true"); +} + }; }; diff --git a/src/lib/dhcpsrv/database_connection.h b/src/lib/dhcpsrv/database_connection.h index c18c16f28c..e6a902babf 100644 --- a/src/lib/dhcpsrv/database_connection.h +++ b/src/lib/dhcpsrv/database_connection.h @@ -121,6 +121,14 @@ public: /// @return Redacted database access string. static std::string redactedAccessString(const ParameterMap& parameters); + /// @brief Convenience method checking if database should be opened with + /// read only access. + /// + /// @return true if "readonly" parameter is specified and set to true; + /// false if "readonly" parameter is not specified or it is specified + /// and set to false. + bool configuredReadOnly() const; + private: /// @brief List of parameters passed in dbconfig diff --git a/src/lib/dhcpsrv/dhcpsrv_messages.mes b/src/lib/dhcpsrv/dhcpsrv_messages.mes index 7bf0ba22ad..bb8abaee1f 100644 --- a/src/lib/dhcpsrv/dhcpsrv_messages.mes +++ b/src/lib/dhcpsrv/dhcpsrv_messages.mes @@ -551,7 +551,7 @@ connection including database name and username needed to access it A debug message issued when the server is about to obtain schema version information from the MySQL hosts database. -% DHCPSRV_MYSQL_HOST_DB_READONLY MySQL host database operates in read-only mode +% DHCPSRV_MYSQL_HOST_DB_READONLY MySQL host database opened for read access only This informational message is issued when the user has configured the PostgreSQL database in read-only mode. Kea will not be able to insert or modify host reservations but will be able to retrieve existing ones and @@ -702,7 +702,7 @@ V6) is about to open a PostgreSQL hosts database. The parameters of the connection including database name and username needed to access it (but not the password if any) are logged. -% DHCPSRV_PGSQL_HOST_DB_READONLY PostgreSQL host database operates in read-only mode +% DHCPSRV_PGSQL_HOST_DB_READONLY PostgreSQL host database opened for read access only This informational message is issued when the user has configured the PostgreSQL database in read-only mode. Kea will not be able to insert or modify host reservations but will be able to retrieve existing ones and diff --git a/src/lib/dhcpsrv/mysql_connection.cc b/src/lib/dhcpsrv/mysql_connection.cc index b12bdd5e19..9569b4e8d3 100644 --- a/src/lib/dhcpsrv/mysql_connection.cc +++ b/src/lib/dhcpsrv/mysql_connection.cc @@ -214,12 +214,15 @@ MySqlConnection::prepareStatement(uint32_t index, const char* text) { void MySqlConnection::prepareStatements(const TaggedStatement* start_statement, - const TaggedStatement* end_statement, - size_t num_statements) { - // Allocate space for all statements - statements_.resize(num_statements, NULL); + const TaggedStatement* end_statement) { + size_t num_statements = std::distance(start_statement, end_statement); + if (num_statements == 0) { + return; + } - text_statements_.resize(num_statements, std::string("")); + // Expand vectors of allocated statements to hold additional statements. + statements_.resize(statements_.size() + num_statements, NULL); + text_statements_.resize(text_statements_.size() + num_statements, std::string("")); // Created the MySQL prepared statements for each DML statement. for (const TaggedStatement* tagged_statement = start_statement; diff --git a/src/lib/dhcpsrv/mysql_connection.h b/src/lib/dhcpsrv/mysql_connection.h index 06fb34d466..89ad0ec593 100644 --- a/src/lib/dhcpsrv/mysql_connection.h +++ b/src/lib/dhcpsrv/mysql_connection.h @@ -241,15 +241,13 @@ public: /// Creates the prepared statements for all of the SQL statements used /// by the MySQL backend. /// @param tagged_statements an array of statements to be compiled - /// @param num_statements number of statements in tagged_statements /// /// @throw isc::dhcp::DbOperationError An operation on the open database has /// failed. /// @throw isc::InvalidParameter 'index' is not valid for the vector. This /// represents an internal error within the code. void prepareStatements(const TaggedStatement* start_statement, - const TaggedStatement* end_statement, - size_t num_statements); + const TaggedStatement* end_statement); /// @brief Clears prepared statements and text statements. void clearStatements(); diff --git a/src/lib/dhcpsrv/mysql_host_data_source.cc b/src/lib/dhcpsrv/mysql_host_data_source.cc index 96ef41f9f0..becad67abe 100644 --- a/src/lib/dhcpsrv/mysql_host_data_source.cc +++ b/src/lib/dhcpsrv/mysql_host_data_source.cc @@ -849,7 +849,7 @@ private: size_t start_column_; /// @brief Option id. - uint64_t option_id_; + uint32_t option_id_; /// @brief Option code. uint16_t code_; @@ -919,7 +919,7 @@ private: //@} /// @brief Option id for last processed row. - uint64_t most_recent_option_id_; + uint32_t most_recent_option_id_; }; /// @brief Pointer to the @ref OptionProcessor class. @@ -1116,7 +1116,7 @@ public: /// @brief Returns last fetched reservation id. /// /// @return Reservation id or 0 if no reservation data is fetched. - uint64_t getReservationId() const { + uint32_t getReservationId() const { if (reserv_type_null_ == MLM_FALSE) { return (reservation_id_); } @@ -1254,7 +1254,7 @@ public: private: /// @brief IPv6 reservation id. - uint64_t reservation_id_; + uint32_t reservation_id_; /// @brief IPv6 reservation type. uint8_t reserv_type_; @@ -1297,7 +1297,7 @@ private: //@} /// @brief Reservation id for last processed row. - uint64_t most_recent_reservation_id_; + uint32_t most_recent_reservation_id_; }; @@ -1633,7 +1633,10 @@ public: /// @brief Statement Tags /// - /// The contents of the enum are indexes into the list of SQL statements + /// The contents of the enum are indexes into the list of SQL statements. + /// It is assumed that the order is such that the indicies of statements + /// reading the database are less than those of statements modifying the + /// database. enum StatementIndex { GET_HOST_DHCPID, // Gets hosts by host identifier GET_HOST_ADDR, // Gets hosts by IPv4 address @@ -1986,33 +1989,18 @@ MySqlHostDataSourceImpl(const MySqlConnection::ParameterMap& parameters) // information from the database, so they can be used even if the // database is read only for the current user. conn_.prepareStatements(tagged_statements.begin(), - tagged_statements.begin() + WRITE_STMTS_BEGIN, - WRITE_STMTS_BEGIN); + tagged_statements.begin() + WRITE_STMTS_BEGIN); - std::string readonly_value = "false"; - try { - readonly_value = conn_.getParameter("readonly"); - boost::algorithm::to_lower(readonly_value); - } catch (...) { - // Parameter "readonly" hasn't been specified so we simply use - // the default value of "false". - } - - if (readonly_value == "true") { - is_readonly_ = true; - - } else if (readonly_value != "false") { - isc_throw(DbInvalidReadOnly, "invalid value '" << readonly_value - << "' specified for boolean parameter 'readonly'"); - } + // Check if the backend is explicitly configured to operate with + // read only access to the database. + is_readonly_ = conn_.configuredReadOnly(); // If we are using read-write mode for the database we also prepare // statements for INSERTS etc. if (!is_readonly_) { // Prepare statements for writing to the database, e.g. INSERT. conn_.prepareStatements(tagged_statements.begin() + WRITE_STMTS_BEGIN, - tagged_statements.end(), - tagged_statements.size()); + tagged_statements.end()); } else { LOG_INFO(dhcpsrv_logger, DHCPSRV_MYSQL_HOST_DB_READONLY); } diff --git a/src/lib/dhcpsrv/mysql_lease_mgr.cc b/src/lib/dhcpsrv/mysql_lease_mgr.cc index 7e448428f2..8edf4dfc5c 100644 --- a/src/lib/dhcpsrv/mysql_lease_mgr.cc +++ b/src/lib/dhcpsrv/mysql_lease_mgr.cc @@ -1237,8 +1237,7 @@ MySqlLeaseMgr::MySqlLeaseMgr(const MySqlConnection::ParameterMap& parameters) } // Prepare all statements likely to be used. - conn_.prepareStatements(tagged_statements.begin(), tagged_statements.end(), - tagged_statements.size()); + conn_.prepareStatements(tagged_statements.begin(), tagged_statements.end()); // Create the exchange objects for use in exchanging data between the // program and the database. diff --git a/src/lib/dhcpsrv/pgsql_host_data_source.cc b/src/lib/dhcpsrv/pgsql_host_data_source.cc index d134d3c766..2ab5280923 100644 --- a/src/lib/dhcpsrv/pgsql_host_data_source.cc +++ b/src/lib/dhcpsrv/pgsql_host_data_source.cc @@ -1105,7 +1105,10 @@ public: /// @brief Statement Tags /// - /// The contents of the enum are indexes into the list of SQL statements + /// The contents of the enum are indexes into the list of SQL statements. + /// It is assumed that the order is such that the indicies of statements + /// reading the database are less than those of statements modifying the + /// database. enum StatementIndex { GET_HOST_DHCPID, // Gets hosts by host identifier GET_HOST_ADDR, // Gets hosts by IPv4 address @@ -1489,22 +1492,9 @@ PgSqlHostDataSourceImpl(const PgSqlConnection::ParameterMap& parameters) conn_.prepareStatements(tagged_statements.begin(), tagged_statements.begin() + WRITE_STMTS_BEGIN); - std::string readonly_value = "false"; - try { - readonly_value = conn_.getParameter("readonly"); - boost::algorithm::to_lower(readonly_value); - } catch (...) { - // Parameter "readonly" hasn't been specified so we simply use - // the default value of "false". - } - - if (readonly_value == "true") { - is_readonly_ = true; - - } else if (readonly_value != "false") { - isc_throw(DbInvalidReadOnly, "invalid value '" << readonly_value - << "' specified for boolean parameter 'readonly'"); - } + // Check if the backend is explicitly configured to operate with + // read only access to the database. + is_readonly_ = conn_.configuredReadOnly(); // If we are using read-write mode for the database we also prepare // statements for INSERTS etc. diff --git a/src/share/database/scripts/mysql/dhcpdb_create.mysql b/src/share/database/scripts/mysql/dhcpdb_create.mysql index 5d6a321200..80aaf082c2 100644 --- a/src/share/database/scripts/mysql/dhcpdb_create.mysql +++ b/src/share/database/scripts/mysql/dhcpdb_create.mysql @@ -465,6 +465,11 @@ ALTER TABLE dhcp6_options ADD CONSTRAINT fk_dhcp6_option_scope FOREIGN KEY (scope_id) REFERENCES dhcp_option_scope (scope_id); +# Add UNSIGNED to reservation_id +ALTER TABLE ipv6_reservations + MODIFY reservation_id INT UNSIGNED NOT NULL AUTO_INCREMENT; + + # Update the schema version number UPDATE schema_version SET version = '4', minor = '2';