[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.

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 <dhcpsrv/database_connection.h>
+#include <dhcpsrv/db_exceptions.h>
 #include <dhcpsrv/dhcpsrv_log.h>
 #include <exceptions/exceptions.h>
 
@@ -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");
+}
+
 };
 };

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

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

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;

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();

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);
     }

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.

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.

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';