From: Thomas Markwalder Date: Mon, 27 Jun 2016 20:00:38 +0000 (-0400) Subject: [4277] Cleanup and commentary X-Git-Tag: trac4551_base~12^2~5 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=3d405228678b08ad1ee473e1e2b65adea062b1f0;p=thirdparty%2Fkea.git [4277] Cleanup and commentary --- diff --git a/src/lib/dhcpsrv/pgsql_connection.cc b/src/lib/dhcpsrv/pgsql_connection.cc index 0f6f69c8f6..7af6576689 100644 --- a/src/lib/dhcpsrv/pgsql_connection.cc +++ b/src/lib/dhcpsrv/pgsql_connection.cc @@ -41,8 +41,7 @@ PgSqlTransaction::PgSqlTransaction(PgSqlConnection& conn) } PgSqlTransaction::~PgSqlTransaction() { - // Rollback if the PgSqlTransaction::commit wasn't explicitly - // called. + // If commit() wasn't explicitly called, rollback. if (!committed_) { conn_.rollback(); } @@ -218,7 +217,7 @@ PgSqlConnection::startTransaction() { PgSqlResult r(PQexec(conn_, "START TRANSACTION")); if (PQresultStatus(r) != PGRES_COMMAND_OK) { const char* error_message = PQerrorMessage(conn_); - isc_throw(DbOperationError, "unable to start transaction" + isc_throw(DbOperationError, "unable to start transaction" << error_message); } } diff --git a/src/lib/dhcpsrv/pgsql_connection.h b/src/lib/dhcpsrv/pgsql_connection.h index e1ec93bae9..664b8fdb3b 100755 --- a/src/lib/dhcpsrv/pgsql_connection.h +++ b/src/lib/dhcpsrv/pgsql_connection.h @@ -82,10 +82,17 @@ class PgSqlResult : public boost::noncopyable { public: /// @brief Constructor /// - /// Store the pointer to the result set to being fetched. + /// Store the pointer to the result set to being fetched. Set row + /// and column counts for convenience. /// - PgSqlResult(PGresult *result) : result_(result) - {} + PgSqlResult(PGresult *result) : result_(result), rows_(0), cols_(0) { + if (!result) { + isc_throw (BadValue, "PgSqlResult result pointer cannot be null"); + } + + rows_ = PQntuples(result); + cols_ = PQnfields(result); + } /// @brief Destructor /// @@ -96,6 +103,50 @@ public: } } + /// @brief Returns the number of rows in the result set. + int getRows() const { + return (rows_); + } + + /// @brief Returns the number of columns in the result set. + int getCols() const { + return (cols_); + } + + /// @brief Determines if a row index is valid + /// + /// @param row index to range check + /// + /// @throw throws DbOperationError if the row index is out of range + void rowCheck(int row) const { + if (row >= rows_) { + isc_throw (DbOperationError, "row: " << row << ", out of range: 0.." << rows_); + } + } + + /// @brief Determines if a column index is valid + /// + /// @param col index to range check + /// + /// @throw throws DbOperationError if the column index is out of range + void colCheck(int col) const { + if (col >= cols_) { + isc_throw (DbOperationError, "col: " << col << ", out of range: 0.." << cols_); + } + } + + /// @brief Determines if both a row and column index are valid + /// + /// @param row index to range check + /// @param col index to range check + /// + /// @throw throws DbOperationError if either the row or column index + /// is out of range + void rowColCheck(int row, int col) const { + rowCheck(row); + colCheck(col); + } + /// @brief Conversion Operator /// /// Allows the PgSqlResult object to be passed as the result set argument to @@ -113,6 +164,8 @@ public: private: PGresult* result_; ///< Result set to be freed + int rows_; ///< Number of rows in the result set + int cols_; ///< Number of columns in the result set }; @@ -181,45 +234,50 @@ private: /// @brief Forward declaration to @ref PgSqlConnection. class PgSqlConnection; -/// @brief RAII object representing PostgreSQL transaction. +/// @brief RAII object representing a PostgreSQL transaction. /// /// An instance of this class should be created in a scope where multiple /// INSERT statements should be executed within a single transaction. The /// transaction is started when the constructor of this class is invoked. /// The transaction is ended when the @ref PgSqlTransaction::commit is /// explicitly called or when the instance of this class is destroyed. -/// The @ref PgSqlTransaction::commit commits changes to the database -/// and the changes remain in the database when the instance of the -/// class is destroyed. If the class instance is destroyed before the -/// @ref PgSqlTransaction::commit is called, the transaction is rolled -/// back. The rollback on destruction guarantees that partial data is -/// not stored in the database when there is an error during any -/// of the operations belonging to a transaction. +/// The @ref PgSqlTransaction::commit commits changes to the database. +/// If the class instance is destroyed before @ref PgSqlTransaction::commit +/// has been called, the transaction is rolled back. The rollback on +/// destruction guarantees that partial data is not stored in the database +/// when an error occurs during any of the operations within a transaction. /// -/// The default PostgreSQL backend configuration enables 'autocommit'. -/// Starting a transaction overrides 'autocommit' setting for this -/// particular transaction only. It does not affect the global 'autocommit' -/// setting for the database connection, i.e. all modifications to the -/// database which don't use transactions will still be auto committed. +/// By default PostgreSQL performs a commit following each statement which +/// alters the database (i.e. "autocommit"). Starting a transaction +/// stops autocommit for the connection until the transaction is ended by +/// either commit or rollback. Other connections are unaffected. class PgSqlTransaction : public boost::noncopyable { public: /// @brief Constructor. /// - /// Starts transaction by making a "START TRANSACTION" query. + /// Starts transaction by executing the SQL statement: "START TRANSACTION" /// /// @param conn PostgreSQL connection to use for the transaction. This /// connection will be later used to commit or rollback changes. /// - /// @throw DbOperationError if "START TRANSACTION" query fails. + /// @throw DbOperationError if statement execution fails PgSqlTransaction(PgSqlConnection& conn); /// @brief Destructor. /// - /// Rolls back the transaction if changes haven't been committed. + /// If the transaction has not been committed, it is rolled back + /// by executing the SQL statement: "ROLLBACK" + /// + /// @throw DbOperationError if statement execution fails ~PgSqlTransaction(); /// @brief Commits transaction. + /// + /// Commits all changes made during the transaction by executing the + /// SQL statement: "COMMIT"> + /// + /// @throw DbOperationError if statement execution fails void commit(); private: diff --git a/src/lib/dhcpsrv/pgsql_exchange.cc b/src/lib/dhcpsrv/pgsql_exchange.cc index 70685a62e5..00f7813acc 100644 --- a/src/lib/dhcpsrv/pgsql_exchange.cc +++ b/src/lib/dhcpsrv/pgsql_exchange.cc @@ -51,16 +51,16 @@ void PsqlBindArray::add(const bool& value) { void PsqlBindArray::add(const uint8_t& byte) { // We static_cast to an unsigned int, otherwise lexcial_cast may to // treat byte as a character, which yields "" for unprintable values - bindString(boost::lexical_cast + addTempString(boost::lexical_cast (static_cast(byte))); } void PsqlBindArray::add(const isc::asiolink::IOAddress& addr) { if (addr.isV4()) { - bindString(boost::lexical_cast + addTempString(boost::lexical_cast (static_cast(addr))); } else { - bindString(addr.toText()); + addTempString(addr.toText()); } } @@ -70,9 +70,9 @@ void PsqlBindArray::addNull(const int format) { formats_.push_back(format); } -// eventually this should replace add(std::string) -void PsqlBindArray::bindString(const std::string& str) { - bound_strs_.push_back(StringPtr(new std::string(str))); +// Eventually this could replace add(std::string&) ? +void PsqlBindArray::addTempString(const std::string& str) { + bound_strs_.push_back(ConstStringPtr(new std::string(str))); PsqlBindArray::add((bound_strs_.back())->c_str()); } @@ -94,6 +94,7 @@ std::string PsqlBindArray::toText() const { << static_cast(data[x]); } stream << std::endl; + stream << std::setbase(10); } } } @@ -146,6 +147,7 @@ PgSqlExchange::convertFromDatabaseTime(const std::string& db_time_val) { const char* PgSqlExchange::getRawColumnValue(const PgSqlResult& r, const int row, const size_t col) { + r.rowColCheck(row,col); const char* value = PQgetvalue(r, row, col); if (!value) { isc_throw(DbOperationError, "getRawColumnValue no data for :" @@ -157,6 +159,7 @@ PgSqlExchange::getRawColumnValue(const PgSqlResult& r, const int row, bool PgSqlExchange::isColumnNull(const PgSqlResult& r, const int row, const size_t col) { + r.rowColCheck(row,col); return (PQgetisnull(r, row, col)); } @@ -241,21 +244,9 @@ PgSqlExchange::convertFromBytea(const PgSqlResult& r, const int row, PQfreemem(bytes); } -#if 0 -std::string -PgSqlExchange::getColumnLabel(const size_t column) const { - if (column > columns_.size()) { - std::ostringstream os; - os << "Unknown column:" << column; - return (os.str()); - } - - return (columns_[column]); -} -#endif - std::string PgSqlExchange::getColumnLabel(const PgSqlResult& r, const size_t column) { + r.colCheck(column); const char* label = PQfname(r, column); if (!label) { std::ostringstream os; @@ -268,8 +259,9 @@ PgSqlExchange::getColumnLabel(const PgSqlResult& r, const size_t column) { std::string PgSqlExchange::dumpRow(const PgSqlResult& r, int row) { + r.rowCheck(row); std::ostringstream stream; - int columns = PQnfields(r); + int columns = r.getCols(); for (int col = 0; col < columns; ++col) { const char* val = getRawColumnValue(r, row, col); std::string name = getColumnLabel(r, col); diff --git a/src/lib/dhcpsrv/pgsql_exchange.h b/src/lib/dhcpsrv/pgsql_exchange.h index 8a7e04f335..00cb6f6e3d 100644 --- a/src/lib/dhcpsrv/pgsql_exchange.h +++ b/src/lib/dhcpsrv/pgsql_exchange.h @@ -30,10 +30,16 @@ namespace dhcp { /// be valid for the duration of the PostgreSQL statement execution. In other /// words populating them with pointers to values that go out of scope before /// statement is executed is a bad idea. - -/// @brief smart pointer to strings used by PsqlBindArray to ensure scope -/// of strings supplying exchange values -typedef boost::shared_ptr StringPtr; +/// +/// Other than vectors or buffers of binary data, all other values are currently +/// converted to their string representation prior to sending them to PostgreSQL. +/// All of the add() method variants which accept a non-string value internally +/// create the conversion string which is then retained in the bind array to ensure +/// scope. +/// +/// @brief smart pointer to const std::strings used by PsqlBindArray to ensure scope +/// of strings supplying exchange values +typedef boost::shared_ptr ConstStringPtr; struct PsqlBindArray { /// @brief Vector of pointers to the data values. @@ -71,8 +77,9 @@ struct PsqlBindArray { /// @brief Adds a char array to bind array based /// /// Adds a TEXT_FMT value to the end of the bind array, using the given - /// char* as the data source. Note that value is expected to be NULL - /// terminated. + /// char* as the data source. The value is expected to be NULL + /// terminated. The caller is responsible for ensuring that value + /// remains in scope until the bind array has been discarded. /// /// @param value char array containing the null-terminated text to add. void add(const char* value); @@ -80,7 +87,9 @@ struct PsqlBindArray { /// @brief Adds an string value to the bind array /// /// Adds a TEXT formatted value to the end of the bind array using the - /// given string as the data source. + /// given string as the data source. The caller is responsible for + /// ensuring that string parameter remains in scope until the bind + /// array has been discarded. /// /// @param value std::string containing the value to add. void add(const std::string& value); @@ -103,54 +112,59 @@ struct PsqlBindArray { /// is destroyed. /// /// @param data buffer of binary data. - /// @param len number of bytes of data in buffer + /// @param len number of bytes of data in buffer void add(const uint8_t* data, const size_t len); /// @brief Adds a boolean value to the bind array. /// /// Converts the given boolean value to its corresponding to PostgreSQL /// string value and adds it as a TEXT_FMT value to the bind array. + /// This creates an internally scoped string. /// - /// @param value bool value to add. + /// @param value the boolean value to add. void add(const bool& value); /// @brief Adds a uint8_t value to the bind array. /// /// Converts the given uint8_t value to its corresponding numeric string /// literal and adds it as a TEXT_FMT value to the bind array. + /// This creates an internally scoped string. /// - /// @param value bool value to add. + /// @param byte the one byte value to add. void add(const uint8_t& byte); /// @brief Adds a the given IOAddress value to the bind array. /// - /// Converts the IOAddress, based on its protocol family, to the - /// corresponding string literal and adds it as a TEXT_FMT value to + /// Converts the IOAddress, based on its protocol family, to the + /// corresponding string literal and adds it as a TEXT_FMT value to /// the bind array. + /// This creates an internally scoped string. /// - /// @param value bool value to add. + /// @param addr IP address value to add. void add(const isc::asiolink::IOAddress& addr); /// @brief Adds a the given value to the bind array. /// - /// Converts the given value its corresponding string literal + /// Converts the given value to its corresponding string literal /// boost::lexical_cast and adds it as a TEXT_FMT value to the bind array. + /// This is intended primarily for numeric types. + /// This creates an internally scoped string. /// - /// @param value bool value to add. + /// @param value data value to add. template - void add(const T& numeric) { - bindString(boost::lexical_cast(numeric)); + void add(const T& value) { + addTempString(boost::lexical_cast(value)); } /// @brief Binds a the given string to the bind array. /// /// Prior to added the The given string the vector of exchange values, - /// it duplicated as a StringPtr and saved internally. This garauntees + /// it duplicated as a ConstStringPtr and saved internally. This garauntees /// the string remains in scope until the PsqlBindArray is destroyed, - /// without the caller maintaining the string values. + /// without the caller maintaining the string values. /// - /// @param value bool value to add. - void bindString(const std::string& str); + /// @param str string value to add. + void addTempString(const std::string& str); /// @brief Adds a NULL value to the bind array /// @@ -159,7 +173,7 @@ struct PsqlBindArray { void addNull(const int format = PsqlBindArray::TEXT_FMT); //std::vector getBoundStrs() { - std::vector getBoundStrs() { + std::vector getBoundStrs() { return (bound_strs_); } @@ -169,7 +183,7 @@ struct PsqlBindArray { private: /// @brief vector of strings which supplied the values - std::vector bound_strs_; + std::vector bound_strs_; }; @@ -260,7 +274,7 @@ public: /// /// @throw DbOperationError if the value cannot be fetched or is /// invalid. - static void getColumnValue(const PgSqlResult& r, const int row, + static void getColumnValue(const PgSqlResult& r, const int row, const size_t col, std::string& value); /// @brief Fetches boolean text ('t' or 'f') as a bool. @@ -272,7 +286,7 @@ public: /// /// @throw DbOperationError if the value cannot be fetched or is /// invalid. - static void getColumnValue(const PgSqlResult& r, const int row, + static void getColumnValue(const PgSqlResult& r, const int row, const size_t col, bool &value); /// @brief Fetches an integer text column as a uint8_t. @@ -284,20 +298,34 @@ public: /// /// @throw DbOperationError if the value cannot be fetched or is /// invalid. - static void getColumnValue(const PgSqlResult& r, const int row, + static void getColumnValue(const PgSqlResult& r, const int row, const size_t col, uint8_t &value); + /// @brief Converts a column in a row in a result set into IPv6 address. + /// + /// @param r the result set containing the query results + /// @param row the row number within the result set + /// @param col the column number within the row + /// + /// @return isc::asiolink::IOAddress containing the IPv6 address. + /// @throw DbOperationError if the value cannot be fetched or is + /// invalid. static isc::asiolink::IOAddress getIPv6Value(const PgSqlResult& r, const int row, const size_t col); - static bool isColumnNull(const PgSqlResult& r, const int row, + /// @brief Returns true if a column within a row is null + /// + /// @param r the result set containing the query results + /// @param row the row number within the result set + /// @param col the column number within the row + static bool isColumnNull(const PgSqlResult& r, const int row, const size_t col); /// @brief Fetches a text column as the given value type /// /// Uses boost::lexicalcast to convert the text column value into - /// a value of type T. + /// a value of type T. /// /// @param r the result set containing the query results /// @param row the row number within the result set @@ -307,14 +335,14 @@ public: /// @throw DbOperationError if the value cannot be fetched or is /// invalid. template - static void getColumnValue(const PgSqlResult& r, const int row, + static void getColumnValue(const PgSqlResult& r, const int row, const size_t col, T& value) { const char* data = getRawColumnValue(r, row, col); try { value = boost::lexical_cast(data); } catch (const std::exception& ex) { isc_throw(DbOperationError, "Invalid data:[" << data - << "] for row: " << row << " col: " << col << "," + << "] for row: " << row << " col: " << col << "," << getColumnLabel(r, col) << " : " << ex.what()); } } @@ -335,12 +363,15 @@ public: /// /// @throw DbOperationError if the value cannot be fetched or is /// invalid. - static void convertFromBytea(const PgSqlResult& r, const int row, - const size_t col, uint8_t* buffer, - const size_t buffer_size, + static void convertFromBytea(const PgSqlResult& r, const int row, + const size_t col, uint8_t* buffer, + const size_t buffer_size, size_t &bytes_converted); - + /// @brief Diagnostic tool which dumps the Result row contents as a string + /// + /// @param r the result set containing the query results + /// @param row the row number within the result set static std::string dumpRow(const PgSqlResult& r, int row); protected: diff --git a/src/lib/dhcpsrv/pgsql_host_data_source.cc b/src/lib/dhcpsrv/pgsql_host_data_source.cc index ccf3397a5b..0cc63e1ea8 100644 --- a/src/lib/dhcpsrv/pgsql_host_data_source.cc +++ b/src/lib/dhcpsrv/pgsql_host_data_source.cc @@ -104,10 +104,10 @@ public: } /// @brief Reinitializes state information - /// + /// /// This function should be called in between statement executions. - /// Deriving classes should be to sure to reinitialize any stateful - /// values that control statement result processing. + /// Deriving classes should inovke this method as well as be reset + /// all of their own stateful values. virtual void clear() { host_.reset(); }; @@ -135,8 +135,8 @@ public: /// @brief Returns value of host id in the given row. /// /// This method is used to "look ahead" at the host_id in a row - /// without having to call retrieveHost() - HostID getHostId(const PgSqlResult& r, int row) { + /// without having to call retrieveHost() + HostID getHostId(const PgSqlResult& r, int row) { HostID host_id; getColumnValue(r, row, HOST_ID_COL, host_id); return (host_id); @@ -185,14 +185,14 @@ public: bind_array->add(host->getIPv4Reservation()); // hostname : VARCHAR(255) NULL - bind_array->bindString(host->getHostname()); + bind_array->add(host->getHostname()); // dhcp4_client_classes : VARCHAR(255) NULL // Override default separator to not include space after comma. - bind_array->bindString(host->getClientClasses4().toText(",")); + bind_array->addTempString(host->getClientClasses4().toText(",")); // dhcp6_client_classes : VARCHAR(255) NULL - bind_array->bindString(host->getClientClasses6().toText(",")); + bind_array->addTempString(host->getClientClasses6().toText(",")); } catch (const std::exception& ex) { host_.reset(); isc_throw(DbOperationError, @@ -216,46 +216,38 @@ public: /// /// @param [out] hosts Collection of hosts to which a new host created /// from the processed data should be inserted. - virtual void processRowData(ConstHostCollection& hosts, + virtual void processRowData(ConstHostCollection& hosts, const PgSqlResult& r, int row) { // Peek at the host id , so we can skip it if we already have it // This lets us avoid constructing a copy of host for each // of its sub-rows (options, etc...) - HostID host_id; - getColumnValue(r, row, HOST_ID_COL, host_id); + HostID row_host_id = getHostId(r, row); // Add new host only if there are no hosts or the host id of the // most recently added host is different than the host id of the // currently processed host. - if (hosts.empty() || host_id != hosts.back()->getHostId()) { - HostPtr host = retrieveHost(r, row); + if (hosts.empty() || row_host_id != hosts.back()->getHostId()) { + HostPtr host = retrieveHost(r, row, row_host_id); hosts.push_back(host); } } /// @brief Creates a Host object from a given row in a result set. /// - /// @param r result set containing one or rows from the hosts table - /// @param row row number within the result set from to create the Lease4 - /// object. + /// @param r result set containing one or more rows from the hosts table + /// @param row index within the result set of the row to process /// @param peeked_host_id if the caller has peeked ahead at the row's - /// host_id, it can be passed in here to avoid fetching from the row + /// host_id, it can be passed in here to avoid fetching it from the row /// a second time. /// /// @return HostPtr to the newly created Host object /// @throw DbOperationError if the host cannot be created. - HostPtr retrieveHost(const PgSqlResult& r, int row, + HostPtr retrieveHost(const PgSqlResult& r, int row, const HostID& peeked_host_id = 0) { - + // If the caller peeked ahead at the host_id use that, otherwise - // read it from the row. - HostID host_id = peeked_host_id; - if (peeked_host_id) { - host_id = peeked_host_id; - } - else { - getColumnValue(r, row, HOST_ID_COL, host_id); - } + // read it from the row. + HostID host_id = (peeked_host_id ? peeked_host_id : getHostId(r,row)); // dhcp_identifier : BYTEA NOT NULL uint8_t identifier_value[DHCP_IDENTIFIER_MAX_LEN]; @@ -300,6 +292,7 @@ public: std::string dhcp6_client_classes; getColumnValue(r, row, DHCP6_CLIENT_CLASSES_COL, dhcp6_client_classes); + // Finally, attempt to create the new host. HostPtr host; try { host.reset(new Host(identifier_value, identifier_len, @@ -362,7 +355,7 @@ private: /// class. OptionProcessor(const Option::Universe& universe, const size_t start_column) - : universe_(universe), start_column_(start_column), + : universe_(universe), start_column_(start_column), option_id_index_(start_column), code_index_(start_column_ + 1), value_index_(start_column_ + 2), formatted_value_index_(start_column_ + 3), @@ -391,22 +384,25 @@ private: /// /// @param cfg Pointer to the configuration object into which new /// option instances should be inserted. - void retrieveOption(const CfgOptionPtr& cfg, const PgSqlResult& r, + /// @param r result set containing one or more rows from a dhcp + /// options table. + /// @param row index within the result set of the row to process + void retrieveOption(const CfgOptionPtr& cfg, const PgSqlResult& r, int row) { + // If the option id on this row is NULL, then there's no + // option of this type (4/6) on this row to fetch, so bail. if (PgSqlExchange::isColumnNull(r, row, option_id_index_)) { return; } - // option_id: INT NOT NULL + // option_id: INT uint64_t option_id; PgSqlExchange::getColumnValue(r, row, option_id_index_, option_id); - // option_id may be zero if dhcp4_options or dhcp6_options table - // doesn't contain any options for the particular host. Also, the - // current option id must be greater than id if the most recent - // option because options are ordered by option id. Otherwise - // we assume that this is already processed option. - if (!option_id || (most_recent_option_id_ >= option_id)) { + // The row option id must be greater than id if the most recent + // option because they are ordered by option id. Otherwise + // we assume that we have already processed this option. + if (most_recent_option_id_ >= option_id) { return; } @@ -418,15 +414,15 @@ private: uint16_t code; PgSqlExchange::getColumnValue(r, row, code_index_, code); - // value: BYTEA NOT NULL + // value: BYTEA uint8_t value[OPTION_VALUE_MAX_LEN]; size_t value_len; - PgSqlExchange::convertFromBytea(r, row, value_index_, value, + PgSqlExchange::convertFromBytea(r, row, value_index_, value, sizeof(value), value_len); // formatted_value: TEXT // @todo Should we attempt to enforce max value of 8K? - // If so, we should declare this VARCHAR[8K] in the table + // If so, we should declare this VARCHAR[8K] in the table std::string formatted_value; PgSqlExchange::getColumnValue(r, row, formatted_value_index_, formatted_value); @@ -492,7 +488,7 @@ private: // Spit the value specified in comma separated values // format. std::vector split_vec; - boost::split(split_vec, formatted_value, + boost::split(split_vec, formatted_value, boost::is_any_of(",")); option = def->optionFactory(universe_, code, split_vec); } @@ -597,11 +593,11 @@ public: } } - /// @brief Reinitializes state information - /// + /// @brief Clears state information + /// /// This function should be called in between statement executions. - /// Deriving classes should be to sure to reinitialize any stateful - /// values that control statement result processing. + /// Deriving classes should inovke this method as well as be reset + /// all of their own stateful values. virtual void clear() { PgSqlHostExchange::clear(); if (opt_proc4_) { @@ -621,25 +617,28 @@ public: /// detects duplicated information and discards such entries. /// /// @param [out] hosts Container holding parsed hosts and options. - virtual void processRowData(ConstHostCollection& hosts, + virtual void processRowData(ConstHostCollection& hosts, const PgSqlResult& r, int row) { HostPtr current_host; if (hosts.empty()) { + // Must be the first one, fetch it. current_host = retrieveHost(r, row); hosts.push_back(current_host); } else { // Peek at the host id so we can skip it if we already have // this host. This lets us avoid retrieving the host needlessly - // for each of its sub-rows (options, etc...) + // for each of its sub-rows (options, etc...). HostID row_host_id = getHostId(r, row); current_host = boost::const_pointer_cast(hosts.back()); + + // if the row's host id is greater than the one we've been + // working on we're starting a new host, so fetch it. if (row_host_id > current_host->getHostId()) { current_host = retrieveHost(r, row, row_host_id); hosts.push_back(current_host); } } - // Parse DHCPv4 options if required to do so. if (opt_proc4_) { CfgOptionPtr cfg = current_host->getCfgOption4(); @@ -722,14 +721,14 @@ public: columns_[type_index_] = "type"; columns_[iaid_index_] = "dhcp6_iaid"; - // BOOST_STATIC_ASSERT(5 < RESERVATION_COLUMNS); + BOOST_STATIC_ASSERT(4 < RESERVATION_COLUMNS); } /// @brief Reinitializes state information - /// + /// /// This function should be called in between statement executions. - /// Deriving classes should be to sure to reinitialize any stateful - /// values that control statement result processing. + /// Deriving classes should inovke this method as well as be reset + /// all of their own stateful values. void clear() { PgSqlHostWithOptionsExchange::clear(); most_recent_reservation_id_ = 0; @@ -743,7 +742,7 @@ public: if (!isColumnNull(r, row, reservation_id_index_)) { getColumnValue(r, row, reservation_id_index_, resv_id); } - + return (resv_id); }; @@ -754,11 +753,13 @@ public: /// /// @return IPv6Resrv object (containing IPv6 address or prefix reservation) IPv6Resrv retrieveReservation(const PgSqlResult& r, int row) { - // Set the IPv6 Reservation type (0 = IA_NA, 2 = IA_PD) - IPv6Resrv::Type resv_type; + + // type: SMALLINT NOT NULL uint16_t tmp; getColumnValue(r, row, type_index_, tmp); + // Convert it to IPv6 Reservation type (0 = IA_NA, 2 = IA_PD) + IPv6Resrv::Type resv_type; switch (tmp) { case 0: resv_type = IPv6Resrv::TYPE_NA; @@ -774,11 +775,19 @@ public: << tmp << ". Only 0 or 2 are allowed."); } + // address VARCHAR(39) NOT NULL isc::asiolink::IOAddress address(getIPv6Value(r, row, address_index_)); - uint16_t prefix_len; + // prefix_len: SMALLINT NOT NULL + uint16_t prefix_len; getColumnValue(r, row, prefix_len_index_, prefix_len); + // @todo once we support populating iaid + // iaid: INT + // int iaid; + // getColumnValue(r, row, iaid_index_, iaid); + + // Create the reservation. IPv6Resrv reservation(resv_type, IOAddress(address), prefix_len); return (reservation); }; @@ -802,30 +811,26 @@ public: /// /// @param [out] hosts Collection of hosts to which a new host created /// from the processed data should be inserted. + /// @param r result set containing one or more rows of fetched data + /// @param row index within the result set of the row to process virtual void processRowData(ConstHostCollection& hosts, const PgSqlResult& r, int row) { // Call parent class to fetch host information and options. PgSqlHostWithOptionsExchange::processRowData(hosts, r, row); - uint64_t reservation_id = getReservationId(r, row); - if (!reservation_id) { - return; - } - + // Shouldn't happen but just in case if (hosts.empty()) { isc_throw(Unexpected, "no host information while retrieving" " IPv6 reservation"); } - HostPtr host = boost::const_pointer_cast(hosts.back()); - // If we're dealing with a new reservation, let's add it to the - // host. - if (reservation_id > most_recent_reservation_id_) { + // If we have reservation id we havent' seen yet, retrive the + // the reservation, adding it to the current host + uint64_t reservation_id = getReservationId(r, row); + if (reservation_id && (reservation_id > most_recent_reservation_id_)) { + HostPtr host = boost::const_pointer_cast(hosts.back()); + host->addReservation(retrieveReservation(r, row)); most_recent_reservation_id_ = reservation_id; - - if (most_recent_reservation_id_ > 0) { - host->addReservation(retrieveReservation(r, row)); - } } } @@ -876,7 +881,7 @@ public: /// /// Initialize class members representing a single IPv6 reservation. PgSqlIPv6ReservationExchange() - : PgSqlExchange(RESRV_COLUMNS), + : PgSqlExchange(RESRV_COLUMNS), resv_(IPv6Resrv::TYPE_NA, asiolink::IOAddress("::"), 128) { // Set the column names (for error messages) columns_[0] = "host_id"; @@ -884,7 +889,7 @@ public: columns_[2] = "prefix_len"; columns_[3] = "type"; columns_[4] = "dhcp6_iaid"; - BOOST_STATIC_ASSERT(4 < RESRV_COLUMNS); + BOOST_STATIC_ASSERT(5 < RESRV_COLUMNS); } /// @brief Populate a bind array representing an IPv6 reservation @@ -899,7 +904,7 @@ public: /// bound values extracted the IPv6 reservation /// /// @throw DbOperationError if bind_array cannot be populated. - PsqlBindArrayPtr createBindForSend(const IPv6Resrv& resv, + PsqlBindArrayPtr createBindForSend(const IPv6Resrv& resv, const HostID& host_id) { // Store the values to ensure they remain valid. // Technically we don't need this, as currently all the values @@ -943,7 +948,6 @@ private: /// @brief This class is used for inserting options into a database. /// /// This class supports inserting both DHCPv4 and DHCPv6 options. - class PgSqlOptionExchange : public PgSqlExchange { private: @@ -977,22 +981,26 @@ public: columns_[HOST_ID_COL] = "host_id"; columns_[SCOPE_ID_COL] = "scope_id"; - BOOST_STATIC_ASSERT(8 < OPTION_COLUMNS); + BOOST_STATIC_ASSERT(9 < OPTION_COLUMNS); } /// @brief Creates binding array to insert option data into database. /// - /// @return Vector of MYSQL_BIND object representing an option. + /// @param opt_desc option descriptor of the option to write + /// @param opt_space name of the option space to which the option belongs + /// @param host_id host id of the host to which the option belongs + /// + /// @return pointer to newly constructed bind_array containing the + /// bound values extracted from host PsqlBindArrayPtr createBindForSend(const OptionDescriptor& opt_desc, const std::string& opt_space, - const OptionalValue& subnet_id, const HostID& host_id) { - // Hold pointer to the option to make sure it remains valid until // we complete a query. option_ = opt_desc.option_; + // Create the bind-array PsqlBindArrayPtr bind_array(new PsqlBindArray()); try { @@ -1021,41 +1029,32 @@ public: } // formatted_value: TEXT NULL, - if (opt_desc.formatted_value_.empty()) { + if (!opt_desc.formatted_value_.empty()) { + bind_array->addTempString(opt_desc.formatted_value_); + } else { bind_array->addNull(); - } else { - bind_array->bindString(opt_desc.formatted_value_); } // space: VARCHAR(128) NULL - if (opt_space.empty()) { - bind_array->addNull(); + if (!opt_space.empty()) { + bind_array->addTempString(opt_space); } else { - bind_array->bindString(opt_space); + bind_array->addNull(); } // persistent: BOOLEAN DEFAULT false bind_array->add(opt_desc.persistent_); - // dhcp_client_class: VARCHAR(128) NULL - /// @todo Assign actual value to client class string. - /// once option level client class is supported - bind_array->addNull(); - - // dhcp_subnet_id: INT NULL - if (subnet_id.isSpecified()) { - uint32_t subnet_id = subnet_id; - bind_array->add(subnet_id); - } else { - bind_array->addNull(); + // host_id: INT NULL + if (!host_id) { + isc_throw(BadValue, "host_id cannot be null"); } - - // host_id: INT - in this case it should never be 0 bind_array->add(host_id); + } catch (const std::exception& ex) { isc_throw(DbOperationError, "Could not create bind array for inserting DHCP " - "option: " << option_->toText() << ", reason: " + "host option: " << option_->toText() << ", reason: " << ex.what()); } @@ -1090,8 +1089,8 @@ public: enum StatementIndex { INSERT_HOST, // Insert new host to collection INSERT_V6_RESRV, // Insert v6 reservation - INSERT_V4_OPTION, // Insert DHCPv4 option - INSERT_V6_OPTION, // Insert DHCPv6 option + INSERT_V4_HOST_OPTION, // Insert DHCPv4 option + INSERT_V6_HOST_OPTION, // Insert DHCPv6 option GET_HOST_DHCPID, // Gets hosts by host identifier GET_HOST_ADDR, // Gets hosts by IPv4 address GET_HOST_SUBID4_DHCPID, // Gets host by IPv4 SubnetID, HW address/DUID @@ -1157,7 +1156,7 @@ public: /// @param options_cfg An object holding a collection of options to be /// inserted into the database. /// @param host_id Host identifier retrieved using @c mysql_insert_id. - void addOptions(const StatementIndex& stindex, + void addOptions(const StatementIndex& stindex, const ConstCfgOptionPtr& options_cfg, const uint64_t host_id); @@ -1250,7 +1249,7 @@ PgSqlTaggedStatement tagged_statements[] = { {8, // PgSqlHostDataSourceImpl::INSERT_HOST, { OID_BYTEA, OID_INT2, OID_INT4, OID_INT4, OID_INT8, OID_VARCHAR, - OID_VARCHAR, OID_VARCHAR }, + OID_VARCHAR, OID_VARCHAR }, "insert_host", "INSERT INTO hosts(dhcp_identifier, dhcp_identifier_type, " " dhcp4_subnet_id, dhcp6_subnet_id, ipv4_address, hostname, " @@ -1269,24 +1268,24 @@ PgSqlTaggedStatement tagged_statements[] = { // Inserts a single DHCPv4 option into 'dhcp4_options' table. // Using fixed scope_id = 3, which associates an option with host. - {8, // PgSqlHostDataSourceImpl::INSERT_V4_OPTION, - { OID_INT2, OID_BYTEA, OID_TEXT, OID_VARCHAR, - OID_BOOL, OID_VARCHAR, OID_INT4, OID_INT8, OID_INT2}, - "insert_v4_option", + {6, // PgSqlHostDataSourceImpl::INSERT_V4_HOST_OPTION, + { OID_INT2, OID_BYTEA, OID_TEXT, + OID_VARCHAR, OID_BOOL, OID_INT8}, + "insert_v4_host_option", "INSERT INTO dhcp4_options(code, value, formatted_value, space, " - " persistent, dhcp_client_class, dhcp4_subnet_id, host_id, scope_id) " - "VALUES ($1, $2, $3, $4, $5, $6, $7, $8, 3)" + " persistent, host_id, scope_id) " + "VALUES ($1, $2, $3, $4, $5, $6, 3)" }, // Inserts a single DHCPv6 option into 'dhcp6_options' table. // Using fixed scope_id = 3, which associates an option with host. - {8, // PgSqlHostDataSourceImpl::INSERT_V6_OPTION, - { OID_INT2, OID_BYTEA, OID_TEXT, OID_VARCHAR, - OID_BOOL, OID_VARCHAR, OID_INT4, OID_INT8, OID_INT2}, - "insert_v6_option", + {6, // PgSqlHostDataSourceImpl::INSERT_V6_HOST_OPTION, + { OID_INT2, OID_BYTEA, OID_TEXT, + OID_VARCHAR, OID_BOOL, OID_INT8}, + "insert_v6_host_option", "INSERT INTO dhcp6_options(code, value, formatted_value, space, " - " persistent, dhcp_client_class, dhcp6_subnet_id, host_id, scope_id) " - "VALUES ($1, $2, $3, $4, $5, $6, $7, $8, 3)" + " persistent, host_id, scope_id) " + "VALUES ($1, $2, $3, $4, $5, $6, 3)" }, // Retrieves host information, IPv6 reservations and both DHCPv4 and @@ -1331,11 +1330,11 @@ PgSqlTaggedStatement tagged_statements[] = { // and client's identifier. Left joining the dhcp4_options table results in // multiple rows being returned for the same host. { 3, //PgSqlHostDataSourceImpl::GET_HOST_SUBID4_DHCPID, - { OID_INT4, OID_INT2, OID_BYTEA }, + { OID_INT4, OID_INT2, OID_BYTEA }, "get_host_subid4_dhcpid", "SELECT h.host_id, h.dhcp_identifier, h.dhcp_identifier_type, " " h.dhcp4_subnet_id, h.dhcp6_subnet_id, h.ipv4_address, h.hostname, " - " h.dhcp4_client_classes, h.dhcp6_client_classes, o.option_id, o.code, " + " h.dhcp4_client_classes, h.dhcp6_client_classes, o.option_id, o.code, " " o.value, o.formatted_value, o.space, o.persistent " "FROM hosts AS h " "LEFT JOIN dhcp4_options AS o ON h.host_id = o.host_id " @@ -1491,11 +1490,11 @@ void PgSqlHostDataSourceImpl::addOption(const StatementIndex& stindex, const OptionDescriptor& opt_desc, const std::string& opt_space, - const OptionalValue& subnet_id, + const OptionalValue&, const HostID& id) { PsqlBindArrayPtr bind_array; bind_array = host_option_exchange_->createBindForSend(opt_desc, opt_space, - subnet_id, id); + id); addStatement(stindex, bind_array); } @@ -1540,7 +1539,7 @@ getHostCollection(StatementIndex stindex, PsqlBindArrayPtr bind_array, conn_.checkStatementError(r, tagged_statements[stindex]); - int rows = PQntuples(r); + int rows = r.getRows(); for(int row = 0; row < rows; ++row) { exchange->processRowData(result, r, row); @@ -1591,16 +1590,11 @@ std::pair PgSqlHostDataSourceImpl::getVersion() const { PgSqlResult r(PQexecPrepared(conn_, "get_version", 0, NULL, NULL, NULL, 0)); conn_.checkStatementError(r, tagged_statements[GET_VERSION]); - istringstream tmp; uint32_t version; - tmp.str(PQgetvalue(r, 0, 0)); - tmp >> version; - tmp.str(""); - tmp.clear(); + PgSqlExchange::getColumnValue(r, 0, 0, version); uint32_t minor; - tmp.str(PQgetvalue(r, 0, 1)); - tmp >> minor; + PgSqlExchange::getColumnValue(r, 0, 0, minor); return (std::make_pair(version, minor)); } @@ -1635,14 +1629,14 @@ PgSqlHostDataSource::add(const HostPtr& host) { // Insert DHCPv4 options. ConstCfgOptionPtr cfg_option4 = host->getCfgOption4(); if (cfg_option4) { - impl_->addOptions(PgSqlHostDataSourceImpl::INSERT_V4_OPTION, + impl_->addOptions(PgSqlHostDataSourceImpl::INSERT_V4_HOST_OPTION, cfg_option4, host_id); } // Insert DHCPv6 options. ConstCfgOptionPtr cfg_option6 = host->getCfgOption6(); if (cfg_option6) { - impl_->addOptions(PgSqlHostDataSourceImpl::INSERT_V6_OPTION, + impl_->addOptions(PgSqlHostDataSourceImpl::INSERT_V6_HOST_OPTION, cfg_option6, host_id); } @@ -1702,7 +1696,7 @@ PgSqlHostDataSource::getAll4(const asiolink::IOAddress& address) const { // Set up the WHERE clause value PsqlBindArrayPtr bind_array(new PsqlBindArray()); - // v4 Reservation address + // v4 Reservation address bind_array->add(address); ConstHostCollection result; @@ -1819,7 +1813,7 @@ PgSqlHostDataSource::get6(const asiolink::IOAddress& prefix, // Set up the WHERE clause value PsqlBindArrayPtr bind_array(new PsqlBindArray()); - // Add the prefix + // Add the prefix bind_array->add(prefix); // Add the prefix length diff --git a/src/lib/dhcpsrv/pgsql_host_data_source.h b/src/lib/dhcpsrv/pgsql_host_data_source.h index d5fde34aae..3f0de33f18 100644 --- a/src/lib/dhcpsrv/pgsql_host_data_source.h +++ b/src/lib/dhcpsrv/pgsql_host_data_source.h @@ -50,8 +50,6 @@ public: PgSqlHostDataSource(const DatabaseConnection::ParameterMap& parameters); /// @brief Virtual destructor. - /// - /// Releases prepared MySQL statements used by the backend. virtual ~PgSqlHostDataSource(); /// @brief Return all hosts for the specified HW address or DUID. @@ -216,7 +214,7 @@ public: /// /// @return Type of the backend. virtual std::string getType() const { - return (std::string("mysql")); + return (std::string("postgresql")); } /// @brief Returns backend name. diff --git a/src/lib/dhcpsrv/tests/pgsql_exchange_unittest.cc b/src/lib/dhcpsrv/tests/pgsql_exchange_unittest.cc index d97a7d59d9..ba672fe2c4 100755 --- a/src/lib/dhcpsrv/tests/pgsql_exchange_unittest.cc +++ b/src/lib/dhcpsrv/tests/pgsql_exchange_unittest.cc @@ -69,45 +69,71 @@ TEST(PgSqlExchangeTest, convertTimeTest) { EXPECT_EQ(ref_time, from_time); } -TEST(PsqlBindArray, basicOperation) { - +/// @brief Verifies the ability to add various data types to +/// the bind array. +TEST(PsqlBindArray, addDataTest) { + PsqlBindArray b; - uint8_t small_int = 25; - b.add(small_int); + // Declare a vector to add. Vectors are not currently duplicated + // So they will go out of scope, unless caller ensures it. + std::vector bytes; + for (int i = 0; i < 10; i++) { + bytes.push_back(i+1); + } - int reg_int = 376; - b.add(reg_int); + // Declare a string + std::string not_temp_str("just a string"); - uint64_t big_int = 86749032; - b.add(big_int); + // Now add all the items within a different scope. Everything should + // still be valid once we exit this scope. + { + // Add a const char* + b.add("booya!"); - b.add((bool)(1)); - b.add((bool)(0)); + // Add the non temporary string + b.add(not_temp_str); - b.add(isc::asiolink::IOAddress("192.2.15.34")); - b.add(isc::asiolink::IOAddress("3001::1")); + // Add a temporary string + b.addTempString("walah walah washington"); - std::string str("just a string"); - b.add(str); + // Add a one byte int + uint8_t small_int = 25; + b.add(small_int); - std::vector bytes; - for (int i = 0; i < 10; i++) { - bytes.push_back(i+1); + // Add a four byte int + int reg_int = 376; + b.add(reg_int); + + // Add a eight byte unsigned int + uint64_t big_int = 48786749032; + b.add(big_int); + + // Add boolean true and false + b.add((bool)(1)); + b.add((bool)(0)); + + // Add IP addresses + b.add(isc::asiolink::IOAddress("192.2.15.34")); + b.add(isc::asiolink::IOAddress("3001::1")); + + // Add the vector + b.add(bytes); } - b.add(bytes); - - std::string expected = - "0 : \"25\"\n" - "1 : \"376\"\n" - "2 : \"86749032\"\n" - "3 : \"TRUE\"\n" - "4 : \"FALSE\"\n" - "5 : \"3221360418\"\n" - "6 : \"3001::1\"\n" - "7 : \"just a string\"\n" - "8 : 0x0102030405060708090a\n"; + // We've left bind scope, everything should be intact. + std::string expected = + "0 : \"booya!\"\n" + "1 : \"just a string\"\n" + "2 : \"walah walah washington\"\n" + "3 : \"25\"\n" + "4 : \"376\"\n" + "5 : \"48786749032\"\n" + "6 : \"TRUE\"\n" + "7 : \"FALSE\"\n" + "8 : \"3221360418\"\n" + "9 : \"3001::1\"\n" + "10 : 0x0102030405060708090a\n"; EXPECT_EQ(expected, b.toText()); }