]> git.ipfire.org Git - thirdparty/kea.git/commitdiff
[4281] Updated recently modified files with proper commentary.
authorMarcin Siodelski <marcin@isc.org>
Tue, 17 May 2016 13:23:27 +0000 (15:23 +0200)
committerMarcin Siodelski <marcin@isc.org>
Tue, 17 May 2016 13:23:27 +0000 (15:23 +0200)
Also, fixed a couple of minor issues.

src/lib/dhcp/libdhcp++.cc
src/lib/dhcpsrv/cfg_option.cc
src/lib/dhcpsrv/cfg_option.h
src/lib/dhcpsrv/mysql_connection.cc
src/lib/dhcpsrv/mysql_connection.h
src/lib/dhcpsrv/mysql_host_data_source.cc
src/lib/dhcpsrv/tests/generic_host_data_source_unittest.cc
src/lib/dhcpsrv/tests/generic_host_data_source_unittest.h
src/lib/dhcpsrv/tests/mysql_host_data_source_unittest.cc

index fdbc1437c18da7ebeddb6a88d353e6533268838b..3b746ef9b8db76a279752bd7036d1bf4bf611de3 100644 (file)
@@ -863,21 +863,12 @@ LibDHCP::optionSpaceToVendorId(const std::string& option_space) {
     try {
         check = boost::lexical_cast<int64_t>(x);
     } catch (const boost::bad_lexical_cast &) {
-        /// @todo: Should we throw here?
-        // isc_throw(BadValue, "Failed to parse vendor-X value (" << x
-        //           << ") as unsigned 32-bit integer.");
         return (0);
     }
     if (check > std::numeric_limits<uint32_t>::max()) {
-        /// @todo: Should we throw here?
-        //isc_throw(BadValue, "Value " << x << "is too large"
-        //          << " for unsigned 32-bit integer.");
         return (0);
     }
     if (check < 0) {
-        /// @todo: Should we throw here?
-        // isc_throw(BadValue, "Value " << x << "is negative."
-        //       << " Only 0 or larger are allowed for unsigned 32-bit integer.");
         return (0);
     }
 
index 4532e5a217849393177f902a02755f148a0afe58..b901d6030d0eb5a4b2ab47069c3aec405e38f078 100644 (file)
@@ -65,6 +65,8 @@ CfgOption::getVendorIdsSpaceNames() const {
     for (std::list<uint32_t>::const_iterator id = ids.begin();
          id != ids.end(); ++id) {
         std::ostringstream s;
+        // Vendor space name is constructed as "vendor-XYZ" where XYZ is an
+        // uint32_t value, without leading zeros.
         s << "vendor-" << *id;
         names.push_back(s.str());
     }
index 52f85f96bdb35df08d2d7c3d4cc52031d7262eae..065316fa6a59489e3766dd234e0e080b2f6d8095 100644 (file)
@@ -18,7 +18,6 @@
 #include <boost/shared_ptr.hpp>
 #include <stdint.h>
 #include <string>
-#include <set>
 #include <list>
 
 namespace isc {
@@ -34,10 +33,10 @@ struct OptionDescriptor {
     /// @brief Option instance.
     OptionPtr option_;
 
-    /// @briefPersistence flag.
+    /// @brief Persistence flag.
     ///
-    /// If true option is always sent to the client, if false option is
-    /// sent to the client on request.
+    /// If true, option is always sent to the client. If false, option is
+    /// sent to the client when requested using ORO or PRL option.
     bool persistent_;
 
     /// @brief Option value in textual (CSV) format.
@@ -48,9 +47,10 @@ struct OptionDescriptor {
     /// database instead of the binary format.
     ///
     /// Note that this value is carried in the option descriptor, rather than
-    /// @c Option instance because it is a server specific value.
+    /// @c Option instance because it is a server specific value (same as
+    /// persistence flag).
     ///
-    /// An example of the formatted value is: 2001:db8:1::1, 23, some text
+    /// An example of the formatted value is: "2001:db8:1::1, 23, some text"
     /// for the option which carries IPv6 address, a number and a text.
     std::string formatted_value_;
 
@@ -276,14 +276,11 @@ public:
     void add(const OptionPtr& option, const bool persistent,
              const std::string& option_space);
 
-    /// @brief A variant of the method which takes option descriptor as an
-    /// argument.
-    ///
-    /// This method works exactly the same as the other variant, but it takes
-    /// option descriptor as an argument.
+    /// @brief A variant of the @ref CfgOption::add method which takes option
+    /// descriptor as an argument.
     ///
     /// @param desc Option descriptor holding option instance and other
-    /// parameters pertaining to this option.
+    /// parameters pertaining to the option.
     /// @param option_space Option space name.
     ///
     /// @throw isc::BadValue if the option space is invalid.
@@ -372,7 +369,13 @@ public:
         return (*od_itr);
     }
 
-    /// @brief Returns a list of all configured option space names.
+    /// @brief Returns a list of configured option space names.
+    ///
+    /// The returned option space names exclude vendor option spaces,
+    /// such as "vendor-1234". These are returned by the
+    /// @ref getVendorIdsSpaceNames.
+    ///
+    /// @return List comprising option space names.
     std::list<std::string> getOptionSpaceNames() const {
         return (options_.getOptionSpaceNames());
     }
@@ -385,7 +388,10 @@ public:
     /// @brief Returns a list of option space names for configured vendor ids.
     ///
     /// For each vendor-id the option space name returned is constructed
-    /// as "vendor-<vendor-id>".
+    /// as "vendor-XYZ" where XYZ is a @c uint32_t value without leading
+    /// zeros.
+    ///
+    /// @return List comprising option space names for vendor options.
     std::list<std::string> getVendorIdsSpaceNames() const;
 
 private:
index 8096cfd7aacb9011ea4ae0b85ce8339d56f9e990..6da2dae458dce4f5c39a31fecc51c65603801aca 100755 (executable)
@@ -30,6 +30,8 @@ const int MLM_MYSQL_FETCH_FAILURE = 1;
 
 MySqlTransaction::MySqlTransaction(MySqlConnection& conn)
     : conn_(conn), committed_(false) {
+    // We create prepared statements for all other queries, but MySQL
+    // don't support prepared statements for START TRANSACTION.
     int status = mysql_query(conn_.mysql_, "START TRANSACTION");
     if (status != 0) {
         isc_throw(DbOperationError, "unable to start transaction, "
@@ -38,6 +40,8 @@ MySqlTransaction::MySqlTransaction(MySqlConnection& conn)
 }
 
 MySqlTransaction::~MySqlTransaction() {
+    // Rollback if the MySqlTransaction::commit wasn't explicitly
+    // called.
     if (!committed_) {
         conn_.rollback();
     }
index 866bf6472af543e0ec0656aa528b4369304fbf5d..09dfe7230385b8bc88a57887cca79ab4c4b91a46 100755 (executable)
@@ -140,23 +140,60 @@ private:
     MYSQL* mysql_;      ///< Initialization context
 };
 
+/// @brief Forward declaration to @ref MySqlConnection.
 class MySqlConnection;
 
+/// @brief RAII object representing MySQL 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 MySqlTransaction::commit is
+/// explicitly called or when the instance of this class is destroyed.
+/// The @ref MySqlTransaction::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 MySqlTransaction::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 default MySQL 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.
 class MySqlTransaction : public boost::noncopyable {
 public:
 
+    /// @brief Constructor.
+    ///
+    /// Starts transaction by making a "START TRANSACTION" query.
+    ///
+    /// @param conn MySQL connection to use for the transaction. This
+    /// connection will be later used to commit or rollback changes.
+    ///
+    /// @throw DbOperationError if "START TRANSACTION" query fails.
     MySqlTransaction(MySqlConnection& conn);
 
+    /// @brief Destructor.
+    ///
+    /// Rolls back the transaction if changes haven't been committed.
     ~MySqlTransaction();
 
+    /// @brief Commits transaction.
     void commit();
 
 private:
 
+    /// @brief Holds reference to the MySQL database connection.
     MySqlConnection& conn_;
 
+    /// @brief Boolean flag indicating if the transaction has been committed.
+    ///
+    /// This flag is used in the class destructor to assess if the
+    /// transaction should be rolled back.
     bool committed_;
-
 };
 
 
index 3bd484dd4516b763d8bc28b975a24b19e146790d..4539c617cdf8b2d5542bff457687ac0a8c0d1218 100644 (file)
@@ -95,11 +95,10 @@ TaggedStatement tagged_statements[] = {
             "persistent, dhcp_client_class, dhcp6_subnet_id, host_id) "
          " VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?)"},
 
-    // Retrieves host information along with IPv6 reservations associated
-    // with this host. If the host exists in multiple subnets, all hosts
-    // having a specified identifier will be returned from those subnets.
-    // Because LEFT JOIN clause is used, the number of rows returned for
-    // a single host depends on the number of reservations.
+    // Retrieves host information, IPv6 reservations and both DHCPv4 and
+    // DHCPv6 options associated with the host. The LEFT JOIN clause is used
+    // to retrieve information from 4 different tables using a single query.
+    // Hence, this query returns multiple rows for a single host.
     {MySqlHostDataSource::GET_HOST_DHCPID,
             "SELECT DISTINCT h.host_id, h.dhcp_identifier, h.dhcp_identifier_type, "
                 "h.dhcp4_subnet_id, h.dhcp6_subnet_id, h.ipv4_address, "
@@ -120,9 +119,9 @@ TaggedStatement tagged_statements[] = {
             "WHERE dhcp_identifier = ? AND dhcp_identifier_type = ? "
             "ORDER BY h.host_id, o4.option_id, o6.option_id, r.reservation_id"},
 
-    // Retrieves host information by IPv4 address. This should typically
-    // return a single host, but if we ever allow for defining subnets
-    // with overlapping address pools, multiple hosts may be returned.
+    // Retrieves host information along with the DHCPv4 options associated with
+    // it. Left joining the dhcp4_options table results in multiple rows being
+    // returned for the same host. The host is retrieved by IPv4 address.
     {MySqlHostDataSource::GET_HOST_ADDR,
             "SELECT DISTINCT h.host_id, h.dhcp_identifier, h.dhcp_identifier_type, "
                 "h.dhcp4_subnet_id, h.dhcp6_subnet_id, h.ipv4_address, h.hostname, "
@@ -135,8 +134,9 @@ TaggedStatement tagged_statements[] = {
             "WHERE ipv4_address = ? "
             "ORDER BY h.host_id, o.option_id"},
 
-    // Retrieves host information by subnet identifier and unique
-    // identifier of a client. This is expected to return a single host.
+    // Retrieves host information and DHCPv4 options using subnet identifier
+    // and client's identifier. Left joining the dhcp4_options table results in
+    // multiple rows being returned for the same host.
     {MySqlHostDataSource::GET_HOST_SUBID4_DHCPID,
             "SELECT DISTINCT h.host_id, h.dhcp_identifier, h.dhcp_identifier_type, "
                 "h.dhcp4_subnet_id, h.dhcp6_subnet_id, h.ipv4_address, h.hostname, "
@@ -150,12 +150,9 @@ TaggedStatement tagged_statements[] = {
             "   AND h.dhcp_identifier = ? "
             "ORDER BY h.host_id, o.option_id"},
 
-    // Retrieves host information by subnet identifier and unique
-    // identifier of a client. This query should return information
-    // for a single host but multiple rows are returned due to
-    // use of LEFT JOIN clause. The number of rows returned for a single
-    // host dpeneds on the number of IPv6 reservations existing for
-    // this client.
+    // Retrieves host information, IPv6 reservations and DHCPv6 options
+    // associated with a host. The number of rows returned is a multiplication
+    // of number of IPv6 reservations and DHCPv6 options.
     {MySqlHostDataSource::GET_HOST_SUBID6_DHCPID,
             "SELECT DISTINCT h.host_id, h.dhcp_identifier, "
                 "h.dhcp_identifier_type, h.dhcp4_subnet_id, "
@@ -174,9 +171,10 @@ TaggedStatement tagged_statements[] = {
                 "AND h.dhcp_identifier = ? "
             "ORDER BY h.host_id, o.option_id, r.reservation_id"},
 
-    // Retrieves host information using subnet identifier and the
-    // IPv4 address reservation. This should return inforamation for
-    // a single host.
+    // Retrieves host information and DHCPv4 options for the host using subnet
+    // identifier and IPv4 reservation. Left joining the dhcp4_options table
+    // results in multiple rows being returned for the host. The number of
+    // rows depends on the number of options defined for the host.
     {MySqlHostDataSource::GET_HOST_SUBID_ADDR,
             "SELECT DISTINCT h.host_id, h.dhcp_identifier, h.dhcp_identifier_type, "
                 "h.dhcp4_subnet_id, h.dhcp6_subnet_id, h.ipv4_address, h.hostname, "
@@ -189,12 +187,12 @@ TaggedStatement tagged_statements[] = {
             "WHERE h.dhcp4_subnet_id = ? AND h.ipv4_address = ? "
             "ORDER BY h.host_id, o.option_id"},
 
-    // Retrieves host information using IPv6 prefix and prefix length
-    // or IPv6 address. This query returns host information for a
-    // single host. However, multiple rows are returned by this
-    // query due to use of LEFT JOIN clause with 'ipv6_reservations'
-    // table. The number of rows returned depends on the number of
-    // reservations for a particular host.
+    // Retrieves host information, IPv6 reservations and DHCPv6 options
+    // associated with a host using prefix and prefix length. This query
+    // returns host information for a single host. However, multiple rows
+    // are returned due to left joining IPv6 reservations and DHCPv6 options.
+    // The number of rows returned is multiplication of number of existing
+    // IPv6 reservations and DHCPv6 options.
     {MySqlHostDataSource::GET_HOST_PREFIX,
             "SELECT DISTINCT h.host_id, h.dhcp_identifier, "
                 "h.dhcp_identifier_type, h.dhcp4_subnet_id, "
@@ -225,29 +223,25 @@ TaggedStatement tagged_statements[] = {
 /// @brief This class provides mechanisms for sending and retrieving
 /// information from the 'hosts' table.
 ///
-/// This class should be used to create new entries in the 'hosts'
-/// table and to retrieve DHCPv4 reservations from this table. The
-/// queries used with this class do not retrieve IPv6 reservations for
-/// the hosts to minimize negative impact on performance.
-///
-/// The derived class MySqlHostIPv6Exchange extends this class to facilitate
-/// retrieving IPv6 reservations along with the host information.
+/// This class is used to insert and retrieve entries from the 'hosts' table.
+/// The queries used with this class do not retrieve IPv6 reservations or
+/// options associated with a host to minimize impact on performance. Other
+/// classes derived from @ref MySqlHostExchange should be used to retrieve
+/// information about IPv6 reservations and options.
 class MySqlHostExchange {
 private:
 
-    /// @brief Number of columns returned for queries used with this class.
+    /// @brief Number of columns returned for SELECT queries send by this class.
     static const size_t HOST_COLUMNS = 9;
 
 public:
 
     /// @brief Constructor
     ///
-    /// The initialization of the variables here is only to satisfy cppcheck -
-    /// all variables are initialized/set in the methods before they are used.
-    ///
-    /// @param additional_columns_num Additional number of columns to standard
-    /// set of columns used by this class, for which resources should be
-    /// allocated.
+    /// @param additional_columns_num This value is set by the derived classes
+    /// to indicate how many additional columns will be returned by SELECT
+    /// queries performed by the derived class. This constructor will allocate
+    /// resources for these columns, e.g. binding table, error indicators.
     MySqlHostExchange(const size_t additional_columns_num = 0)
         : columns_num_(HOST_COLUMNS + additional_columns_num),
           bind_(columns_num_), columns_(columns_num_),
@@ -268,7 +262,9 @@ public:
         memset(dhcp4_client_classes_, 0, sizeof(dhcp4_client_classes_));
         memset(dhcp6_client_classes_, 0, sizeof(dhcp6_client_classes_));
 
-        // Set the column names (for error messages)
+        // Set the column names for use by this class. This only comprises
+        // names used by the MySqlHostExchange class. Derived classes will
+        // need to set names for the columns they use.
         columns_[0] = "host_id";
         columns_[1] = "dhcp_identifier";
         columns_[2] = "dhcp_identifier_type";
@@ -286,6 +282,20 @@ public:
     virtual ~MySqlHostExchange() {
     }
 
+    /// @brief Returns index of the first uninitialized column name.
+    ///
+    /// This method is called by the derived classes to determine which
+    /// column indexes are available for the derived classes within a
+    /// binding array, error array and column names. This method
+    /// determines the first available index by searching the first
+    /// empty value within the columns_ vector. Previously we relied on
+    /// the fixed values set for each class, but this was hard to maintain
+    /// when new columns were added to the SELECT queries. It required
+    /// modifying indexes in all derived classes.
+    ///
+    /// Derived classes must call this method in their constructors and
+    /// use returned value as an index for the first column used by the
+    /// derived class and increment this value for each subsequent column.
     size_t findAvailColumn() const {
         std::vector<std::string>::const_iterator empty_column =
             std::find(columns_.begin(), columns_.end(), std::string());
@@ -627,8 +637,8 @@ public:
     /// adding duplicated hosts to the collection, assuming that processed
     /// rows are primarily ordered by host id column.
     ///
-    /// @todo This method will need to be extended to process options
-    /// associated with hosts.
+    /// This method must be overriden in the derived classes to also
+    /// retrieve IPv6 reservations and DHCP options associated with a host.
     ///
     /// @param [out] hosts Collection of hosts to which a new host created
     ///        from the processed data should be inserted.
@@ -749,14 +759,44 @@ private:
 
 };
 
+/// @brief Extends base exchange class with ability to retrieve DHCP options
+/// from the 'dhcp4_options' and 'dhcp6_options' tables.
+///
+/// This class provides means to retrieve both DHCPv4 and DHCPv6 options
+/// along with the host information. It is not used to retrieve IPv6
+/// reservations. The following types of queries are supported:
+/// - SELECT ? FROM hosts LEFT JOIN dhcp4_options LEFT JOIN dhcp6_options ...
+/// - SELECT ? FROM hosts LEFT JOIN dhcp4_options ...
+/// - SELECT ? FROM hosts LEFT JOIN dhcp6_options ...
 class MySqlHostExchangeOpts : public MySqlHostExchange {
 private:
 
-    /// @brief Number of columns holding option information.
+    /// @brief Number of columns holding DHCPv4  or DHCPv6 option information.
     static const size_t OPTION_COLUMNS = 6;
 
+    /// @brief Receives DHCPv4 or DHCPv6 options information from the
+    /// dhcp4_options or dhcp6_options tables respectively.
+    ///
+    /// The MySqlHostExchangeOpts class holds two respective instances of this
+    /// class, one for receiving DHCPv4 options, one for receiving DHCPv6
+    /// options.
+    ///
+    /// The following are the basic functions of this class:
+    /// - bind class members to specific columns in MySQL binding tables,
+    /// - set DHCP options specific column names,
+    /// - create instances of options retrieved from the database.
+    ///
+    /// The reason for isolating those functions in a separate C++ class is
+    /// to prevent code duplication for handling DHCPv4 and DHCPv6 options.
     class OptionProcessor {
     public:
+
+        /// @brief Constructor.
+        ///
+        /// @param universe V4 or V6. The type of the options' instances
+        /// created by this class depends on this parameter.
+        /// @param start_column Index of the first column to be used by this
+        /// class.
         OptionProcessor(const Option::Universe& universe,
                         const size_t start_column)
         : universe_(universe), start_column_(start_column), option_id_(0),
@@ -776,6 +816,7 @@ private:
             memset(space_, 0, sizeof(space_));
         }
 
+        /// @brief Returns identifier of the currently processed option.
         uint64_t getOptionId() const {
             if (option_id_null_ == MLM_FALSE) {
                 return (option_id_);
@@ -783,67 +824,112 @@ private:
             return (0);
         }
 
+        /// @brief Creates instance of the currently processed option.
+        ///
+        /// This method detects if the currently processed option is a new
+        /// instance. It makes it determination by comparing the identifier
+        /// of the currently processed option, with the most recently processed
+        /// option. If the current value is greater than the id of the recently
+        /// processed option it is assumed that the processed row holds new
+        /// option information. In such case the option instance is created and
+        /// inserted into the configuration passed as argument.
+        ///
+        /// @param cfg Pointer to the configuration object into which new
+        /// option instances should be inserted.
         void retrieveOption(const CfgOptionPtr& cfg) {
-            if (option_id_ == 0) {
+            // option_id may be NULL 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_null_ == MLM_TRUE) ||
+                (most_recent_option_id_ >= option_id_)) {
                 return;
             }
 
-            if (most_recent_option_id_ < option_id_) {
-                most_recent_option_id_ = option_id_;
-
-                std::string space;
-                if (space_null_ == MLM_FALSE) {
-                    space_[space_length_] = '\0';
-                    space.assign(space_);
-                }
-
-                std::string formatted_value;
-                if (formatted_value_null_ == MLM_FALSE) {
-                    formatted_value_[formatted_value_length_] = '\0';
-                    formatted_value.assign(formatted_value_);
-                }
+            // Remember current option id as the most recent processed one. We
+            // will be comparing it with option ids in subsequent rows.
+            most_recent_option_id_ = option_id_;
+
+            // Convert it to string object for easier comparison.
+            std::string space;
+            if (space_null_ == MLM_FALSE) {
+                // Typically, the string values returned by the database are not
+                // NULL terminated.
+                space_[space_length_] = '\0';
+                space.assign(space_);
+            }
 
-                OptionDefinitionPtr def;
-                if ((space == DHCP4_OPTION_SPACE) || (space == DHCP6_OPTION_SPACE)) {
-                    def = LibDHCP::getOptionDef(universe_, code_);
-                }
+            // Convert formatted_value to string as well.
+            std::string formatted_value;
+            if (formatted_value_null_ == MLM_FALSE) {
+                formatted_value_[formatted_value_length_] = '\0';
+                formatted_value.assign(formatted_value_);
+            }
 
-                if (!def && (space != DHCP4_OPTION_SPACE) &&
-                    (space != DHCP6_OPTION_SPACE)) {
-                    uint32_t vendor_id = LibDHCP::optionSpaceToVendorId(space);
-                    if (vendor_id > 0) {
-                        def = LibDHCP::getVendorOptionDef(universe_, vendor_id, code_);
-                    }
-                }
+            // Options are held in a binary or textual format in the database.
+            // This is similar to having an option specified in a server
+            // configuration file. Such option is converted to appropriate C++
+            // class, using option definition. Thus, we need to find the
+            // option definition for this option code and option space.
+
+            // If the option space is a standard DHCPv4 or DHCPv6 option space,
+            // this is most likely a standard option, for which we have a
+            // definition created within libdhcp++.
+            OptionDefinitionPtr def;
+            if ((space == DHCP4_OPTION_SPACE) || (space == DHCP6_OPTION_SPACE)) {
+                def = LibDHCP::getOptionDef(universe_, code_);
+            }
 
-                if (!def) {
-                    def = LibDHCP::getRuntimeOptionDef(space, code_);
+            // Otherwise, we may check if this an option encapsulated within the
+            // vendor space.
+            if (!def && (space != DHCP4_OPTION_SPACE) &&
+                (space != DHCP6_OPTION_SPACE)) {
+                uint32_t vendor_id = LibDHCP::optionSpaceToVendorId(space);
+                if (vendor_id > 0) {
+                    def = LibDHCP::getVendorOptionDef(universe_, vendor_id, code_);
                 }
+            }
 
+            // In all other cases, we use runtime option definitions, which
+            // should be also registered within the libdhcp++.
+            if (!def) {
+                def = LibDHCP::getRuntimeOptionDef(space, code_);
+            }
 
-                OptionPtr option;
+            OptionPtr option;
 
-                if (!def) {
+            if (!def) {
+                // If no definition found, we use generic option type.
+                OptionBuffer buf(value_, value_ + value_length_);
+                option.reset(new Option(universe_, code_, buf.begin(),
+                                        buf.end()));
+            } else {
+                // The option value may be specified in textual or binary format
+                // in the database. If formatted_value is empty, the binary
+                // format is used. Depending on the format we use a different
+                // variant of the optionFactory function.
+                if (formatted_value.empty()) {
                     OptionBuffer buf(value_, value_ + value_length_);
-                    option.reset(new Option(universe_, code_, buf.begin(),
-                                            buf.end()));
+                    option = def->optionFactory(universe_, code_, buf.begin(),
+                                                buf.end());
                 } else {
-                    if (formatted_value.empty()) {
-                        OptionBuffer buf(value_, value_ + value_length_);
-                        option = def->optionFactory(universe_, code_, buf.begin(),
-                                                    buf.end());
-                    } else {
-                        std::vector<std::string> split_vec;
-                        boost::split(split_vec, formatted_value, boost::is_any_of(","));
-                        option = def->optionFactory(universe_, code_, split_vec);
-                    }
+                    // Spit the value specified in comma separated values
+                    // format.
+                    std::vector<std::string> split_vec;
+                    boost::split(split_vec, formatted_value, boost::is_any_of(","));
+                    option = def->optionFactory(universe_, code_, split_vec);
                 }
-
-                OptionDescriptor desc(option, persistent_, formatted_value);
-                cfg->add(desc, space);
             }
+
+            OptionDescriptor desc(option, persistent_, formatted_value);
+            cfg->add(desc, space);
         }
 
+        /// @brief Specify column names.
+        ///
+        /// @param [out] columns Reference to a vector holding names of option
+        /// specific columns.
         void setColumnNames(std::vector<std::string>& columns) {
             columns[option_id_index_] = "option_id";
             columns[code_index_] = "code";
@@ -853,7 +939,15 @@ private:
             columns[persistent_index_] = "persistent";
         }
 
+        /// @brief Initialize binding table fields for options.
+        ///
+        /// Resets most_recent_option_id_ value to 0.
+        ///
+        /// @param [out] bind Binding table.
         void setBindFields(std::vector<MYSQL_BIND>& bind) {
+            // This method is called just before making a new query, so we
+            // reset the most_recent_option_id_ to start over with options
+            // processing.
             most_recent_option_id_ = 0;
 
             // option_id : INT UNSIGNED NOT NULL AUTO_INCREMENT,
@@ -899,76 +993,79 @@ private:
 
     private:
 
+        /// @brief Universe: V4 or V6.
         Option::Universe universe_;
 
+        /// @brief Index of first column used by this class.
         size_t start_column_;
 
-        /// Option id.
+        /// @brief Option id.
         uint64_t option_id_;
 
-        /// Option code.
+        /// @brief Option code.
         uint16_t code_;
 
-        /// Buffer holding binary value of an option.
+        /// @brief Buffer holding binary value of an option.
         uint8_t value_[OPTION_VALUE_MAX_LEN];
 
-        /// Option value length.
+        /// @brief Option value length.
         unsigned long value_length_;
 
-        /// Buffer holding textual value of an option.
+        /// @brief Buffer holding textual value of an option.
         char formatted_value_[OPTION_FORMATTED_VALUE_MAX_LEN];
 
-        /// Formatted option value length.
+        /// @brief Formatted option value length.
         unsigned long formatted_value_length_;
 
-        /// Buffer holding option space name.
+        /// @brief Buffer holding option space name.
         char space_[OPTION_SPACE_MAX_LEN];
 
-        /// Option space length.
+        /// @brief Option space length.
         unsigned long space_length_;
 
-        /// Flag indicating if option is always sent or only if requested.
+        /// @brief Flag indicating if option is always sent or only if
+        /// requested.
         bool persistent_;
 
         /// @name Boolean values indicating if values of specific columns in
         /// the database are NULL.
         //@{
-        /// Boolean flag indicating if the DHCPv4 option id is NULL.
+        /// @brief Boolean flag indicating if the DHCPv4 option id is NULL.
         my_bool option_id_null_;
 
-        /// Boolean flag indicating if the DHCPv4 option code is NULL.
+        /// @brief Boolean flag indicating if the DHCPv4 option code is NULL.
         my_bool code_null_;
 
-        /// Boolean flag indicating if the DHCPv4 option value is NULL.
+        /// @brief Boolean flag indicating if the DHCPv4 option value is NULL.
         my_bool value_null_;
 
-        /// Boolean flag indicating if the DHCPv4 formatted option value
+        /// @brief Boolean flag indicating if the DHCPv4 formatted option value
         /// is NULL.
         my_bool formatted_value_null_;
 
-        /// Boolean flag indicating if the DHCPv4 option space is NULL.
+        /// @brief Boolean flag indicating if the DHCPv4 option space is NULL.
         my_bool space_null_;
 
         //@}
 
         /// @name Indexes of the specific columns
         //@{
-        /// Option id
+        /// @brief Option id
         size_t option_id_index_;
 
-        /// Code
+        /// @brief Code
         size_t code_index_;
 
-        /// Value
+        /// @brief Value
         size_t value_index_;
 
-        /// Formatted value
+        /// @brief Formatted value
         size_t formatted_value_index_;
 
-        /// Space
+        /// @brief Space
         size_t space_index_;
 
-        /// Persistent
+        /// @brief Persistent
         size_t persistent_index_;
         //@}
 
@@ -976,22 +1073,38 @@ private:
         uint64_t most_recent_option_id_;
     };
 
+    /// @brief Pointer to the @ref OptionProcessor class.
     typedef boost::shared_ptr<OptionProcessor> OptionProcessorPtr;
 
 public:
 
+    /// @brief DHCP option types to be fetched from the database.
+    ///
+    /// Supported types are:
+    /// - Only DHCPv4 options,
+    /// - Only DHCPv6 options,
+    /// - Both DHCPv4 and DHCPv6 options.
     enum FetchedOptions {
         DHCP4_ONLY,
         DHCP6_ONLY,
         DHCP4_AND_DHCP6
     };
 
+    /// @brief Constructor.
+    ///
+    /// @param fetched_options Specifies if DHCPv4, DHCPv6 or both should
+    /// be fetched from the database for a host.
+    /// @param additional_columns_num Number of additional columns for which
+    /// resources should be allocated, e.g. binding table, column names etc.
+    /// This parameter should be set to a non zero value by derived classes to
+    /// allocate resources for the columns supported by derived classes.
     MySqlHostExchangeOpts(const FetchedOptions& fetched_options,
                           const size_t additional_columns_num = 0)
         : MySqlHostExchange(getRequiredColumnsNum(fetched_options)
                             + additional_columns_num),
           opt_proc4_(), opt_proc6_() {
 
+        // Create option processor for DHCPv4 options, if required.
         if ((fetched_options == DHCP4_ONLY) ||
             (fetched_options == DHCP4_AND_DHCP6)) {
             opt_proc4_.reset(new OptionProcessor(Option::V4,
@@ -999,6 +1112,7 @@ public:
             opt_proc4_->setColumnNames(columns_);
         }
 
+        // Create option processor for DHCPv6 options, if required.
         if ((fetched_options == DHCP6_ONLY) ||
             (fetched_options == DHCP4_AND_DHCP6)) {
             opt_proc6_.reset(new OptionProcessor(Option::V6,
@@ -1007,10 +1121,17 @@ public:
         }
     }
 
+    /// @brief Processes the current row.
+    ///
+    /// The processed row includes both host information and DHCP option
+    /// information. Because used SELECT query use LEFT JOIN clause, the
+    /// some rows contain duplicated host or options entries. This method
+    /// detects duplicated information and discards such entries.
+    ///
+    /// @param [out] hosts Container holding parsed hosts and options.
     virtual void processFetchedData(ConstHostCollection& hosts) {
-        HostPtr host;
+        // Holds pointer to the previously parsed host.
         HostPtr most_recent_host;
-
         if (!hosts.empty()) {
             // Const cast is not very elegant way to deal with it, but
             // there is a good reason to use it here. This method is called
@@ -1021,34 +1142,43 @@ public:
             // most recently added host in a class member but this would
             // make the code less readable.
             most_recent_host = boost::const_pointer_cast<Host>(hosts.back());
-
         }
 
-        if (!most_recent_host || (most_recent_host->getHostId() != getHostId())) {
-            host = retrieveHost();
+        // If no host has been parsed yet or we're at the row holding next
+        // host, we create a new host object and put it at the end of the
+        // list.
+        if (!most_recent_host || (most_recent_host->getHostId() < getHostId())) {
+            HostPtr host = retrieveHost();
             hosts.push_back(host);
             most_recent_host = host;
         }
 
+        // Parse DHCPv4 options if required to do so.
         if (opt_proc4_) {
             CfgOptionPtr cfg = most_recent_host->getCfgOption4();
             opt_proc4_->retrieveOption(cfg);
         }
 
+        // Parse DHCPv6 options if required to do so.
         if (opt_proc6_) {
             CfgOptionPtr cfg = most_recent_host->getCfgOption6();
             opt_proc6_->retrieveOption(cfg);
         }
     }
 
+    /// @brief Bind variables for receiving option data.
+    ///
+    /// @return Vector of MYSQL_BIND object representing data to be retrieved.
     virtual std::vector<MYSQL_BIND> createBindForReceive() {
         // The following call sets bind_ values between 0 and 8.
         static_cast<void>(MySqlHostExchange::createBindForReceive());
 
+        // Bind variables for DHCPv4 options.
         if (opt_proc4_) {
             opt_proc4_->setBindFields(bind_);
         }
 
+        // Bind variables for DHCPv6 options.
         if (opt_proc6_) {
             opt_proc6_->setBindFields(bind_);
         }
@@ -1056,13 +1186,22 @@ public:
         // Add the error flags
         setErrorIndicators(bind_, error_);
 
-        // Add the data to the vector.  Note the end element is one after the
-        // end of the array.
         return (bind_);
     };
 
 private:
 
+    /// @brief Returns a number of columns required to retrieve option data.
+    ///
+    /// Depending if we need DHCPv4/DHCPv6 options only, or both DHCPv4 and
+    /// DHCPv6 a different number of columns is required in the binding array.
+    /// This method returns the number of required columns, according to the
+    /// value of @c fetched_columns passed in the constructor.
+    ///
+    /// @param fetched_columns A value which specifies whether DHCPv4, DHCPv6 or
+    /// both types of options should be retrieved.
+    ///
+    /// @return Number of required columns.
     static size_t getRequiredColumnsNum(const FetchedOptions& fetched_options) {
         return (fetched_options == DHCP4_AND_DHCP6 ? 2 * OPTION_COLUMNS :
                 OPTION_COLUMNS);
@@ -1079,18 +1218,18 @@ private:
     OptionProcessorPtr opt_proc6_;
 };
 
-/// @Brief This class provides mechanisms for sending and retrieving
-/// host information and associated IPv6 reservations.
+/// @brief This class provides mechanisms for sending and retrieving
+/// host information, DHCPv4 options, DHCPv6 options and IPv6 reservations.
 ///
-/// This class extends the @ref MySqlHostExchange class with the
-/// mechanisms to retrieve IPv6 reservations along with host
-/// information. It is assumed that both host data and IPv6
-/// reservations are retrieved with a single query (using LEFT JOIN
-/// MySQL clause). Because the host to IPv6 reservation is a 1-to-many
-/// relation, the same row from the Host table is returned many times
-/// (for each IPv6 reservation). This class is responsible for
-/// converting those multiple host instances into a single Host
-/// object with multiple IPv6 reservations.
+/// This class extends the @ref MySqlHostExchangeOpts class with the
+/// mechanisms to retrieve IPv6 reservations. This class is used in sitations
+/// when it is desired to retrieve DHCPv6 specific information about the host
+/// (DHCPv6 options and reservations), or entire information about the host
+/// (DHCPv4 options, DHCPv6 options and reservations). The following are the
+/// queries used with this class:
+/// - SELECT ? FROM hosts LEFT JOIN dhcp4_options LEFT JOIN dhcp6_options
+///   LEFT JOIN ipv6_reservations ...
+/// - SELECT ? FROM hosts LEFT JOIN dhcp6_options LEFT JOIN ipv6_reservations ..
 class MySqlHostIPv6Exchange : public MySqlHostExchangeOpts {
 private:
 
@@ -1117,7 +1256,7 @@ public:
 
         memset(ipv6_address_buffer_, 0, sizeof(ipv6_address_buffer_));
 
-        // Append additional columns returned by the queries.
+        // Provide names of additional columns returned by the queries.
         columns_[reservation_id_index_] = "reservation_id";
         columns_[address_index_] = "address";
         columns_[prefix_len_index_] = "prefix_len";
@@ -1135,7 +1274,7 @@ public:
         return (0);
     };
 
-    /// @brief Create IPv6 reservation from the data contained in the
+    /// @brief Creates IPv6 reservation from the data contained in the
     /// currently processed row.
     ///
     /// Called after the MYSQL_BIND array created by createBindForReceive().
@@ -1175,12 +1314,14 @@ public:
     /// adding duplicated hosts to the collection, assuming that processed
     /// rows are primarily ordered by host id column.
     ///
-    /// For any returned row which contains IPv6 reservation information it
-    /// creates a @ref IPv6Resrv and appends it to the collection of the
-    /// IPv6 reservations in a Host object.
+    /// Depending on the value of the @c fetched_options specified in the
+    /// constructor, this method also parses options returned as a result
+    /// of SELECT queries.
     ///
-    /// @todo This method will need to be extended to process DHCPv6 options
-    /// associated with hosts.
+    /// For any returned row which contains IPv6 reservation information it
+    /// checks if the reservation is not a duplicate of previously parsed
+    /// reservation and appends the IPv6Resrv object into the host object
+    /// if the parsed row contains new reservation information.
     ///
     /// @param [out] hosts Collection of hosts to which a new host created
     ///        from the processed data should be inserted.
@@ -1223,7 +1364,7 @@ public:
         // a new SELECT query.
         most_recent_reservation_id_ = 0;
 
-        // The following call sets bind_ values between 0 and 8.
+        // Bind values supported by parent classes.
         static_cast<void>(MySqlHostExchangeOpts::createBindForReceive());
 
         // reservation_id : INT UNSIGNED NOT NULL AUTO_INCREMENT
@@ -1258,8 +1399,6 @@ public:
         // Add the error flags
         setErrorIndicators(bind_, error_);
 
-        // Add the data to the vector.  Note the end element is one after the
-        // end of the array.
         return (bind_);
     };
 
@@ -1459,13 +1598,18 @@ private:
     my_bool error_[RESRV_COLUMNS];
 };
 
+/// @brief This class is used for inserting options into a database.
+///
+/// This class supports inserting both DHCPv4 and DHCPv6 options.
 class MySqlOptionExchange {
 private:
 
+    /// @brief Number of columns in the tables holding options.
     static const size_t OPTION_COLUMNS = 9;
 
 public:
 
+    /// @brief Constructor.
     MySqlOptionExchange()
         : type_(0), value_len_(0), formatted_value_len_(0), space_(), space_len_(0),
           persistent_(false), client_class_(), client_class_len_(0),
@@ -1474,7 +1618,9 @@ public:
         BOOST_STATIC_ASSERT(8 < OPTION_COLUMNS);
     }
 
-
+    /// @brief Creates binding array to insert option data into database.
+    ///
+    /// @return Vector of MYSQL_BIND object representing an option.
     std::vector<MYSQL_BIND>
     createBindForSend(const OptionDescriptor& opt_desc,
                       const std::string& opt_space,
@@ -1482,7 +1628,7 @@ public:
                       const HostID& host_id) {
 
         // Hold pointer to the option to make sure it remains valid until
-        // we make a query.
+        // we complete a query.
         option_ = opt_desc.option_;
 
         memset(bind_, 0, sizeof(bind_));
@@ -1494,16 +1640,19 @@ public:
             bind_[0].buffer_type = MYSQL_TYPE_NULL;
 
             // code: SMALLINT UNSIGNED NOT NULL
-            type_ = option_->getType();
+            type_  = option_->getType();
             bind_[1].buffer_type = MYSQL_TYPE_SHORT;
             bind_[1].buffer = reinterpret_cast<char*>(&type_);
             bind_[1].is_unsigned = MLM_TRUE;
 
             // value: BLOB NULL
-            OutputBuffer buf(opt_desc.option_->len());
-            opt_desc.option_->pack(buf);
-            if ((buf.getLength() > opt_desc.option_->getHeaderLen()) &&
-                 opt_desc.formatted_value_.empty()) {
+            if (opt_desc.formatted_value_.empty() &&
+                (opt_desc.option_->len() > opt_desc.option_->getHeaderLen())) {
+                // The formatted_value is empty and the option value is
+                // non-empty so we need to prepare on-wire format for the
+                // option and store it in the database as a blob.
+                OutputBuffer buf(opt_desc.option_->len());
+                opt_desc.option_->pack(buf);
                 const char* buf_ptr = static_cast<const char*>(buf.getData());
                 value_.assign(buf_ptr + opt_desc.option_->getHeaderLen(),
                               buf_ptr + buf.getLength());
@@ -1514,6 +1663,8 @@ public:
                 bind_[2].length = &value_len_;
 
             } else {
+                // No value or formatted_value specified. In this case, the
+                // value blob is NULL.
                 value_.clear();
                 bind_[2].buffer_type = MYSQL_TYPE_NULL;
             }
@@ -1581,33 +1732,45 @@ public:
 
 private:
 
+    /// @brief Option type.
     uint16_t type_;
 
+    /// @brief Option value as binary.
     std::vector<uint8_t> value_;
 
+    /// @brief Option value length.
     size_t value_len_;
 
-    /// Formatted option value length.
+    /// @brief Formatted option value length.
     unsigned long formatted_value_len_;
 
+    /// @brief Option space name.
     std::string space_;
 
+    /// @brief Option space name length.
     size_t space_len_;
 
+    /// @brief Boolean flag indicating if the option is always returned to
+    /// a client or only when requested.
     bool persistent_;
 
+    /// @brief Client classes for the option.
     std::string client_class_;
 
+    /// @brief Length of the string holding client classes for the option.
     size_t client_class_len_;
 
+    /// @brief Subnet identifier.
     uint32_t subnet_id_;
 
+    /// @brief Host identifier.
     uint32_t host_id_;
 
+    /// @brief Pointer to currently parsed option.
     OptionPtr option_;
 
+    /// @brief Array of MYSQL_BIND elements representing inserted data.
     MYSQL_BIND bind_[OPTION_COLUMNS];
-
 };
 
 } // end of anonymous namespace
@@ -1644,12 +1807,29 @@ public:
     /// @param id ID of a host owning this reservation
     void addResv(const IPv6Resrv& resv, const HostID& id);
 
+    /// @brief Inserts a single DHCP option into the database.
+    ///
+    /// @param stindex Index of a statement being executed.
+    /// @param opt_desc Option descriptor holding information about an option
+    /// to be inserted into the database.
+    /// @param opt_space Option space name.
+    /// @param subnet_id Subnet identifier.
+    /// @param host_id Host identifier.
     void addOption(const MySqlHostDataSource::StatementIndex& stindex,
                    const OptionDescriptor& opt_desc,
                    const std::string& opt_space,
                    const OptionalValue<SubnetID>& subnet_id,
-                   const HostID& id);
+                   const HostID& host_id);
 
+    /// @brief Inserts multiple options into the database.
+    ///
+    /// @param stindex Index of a statement being executed.
+    /// @param options_cfg An object holding a collection of options to be
+    /// inserted into the database.
+    /// @param host_id Host identifier reference to a value to which
+    /// host identifier will be assigned if the current host_id value is 0.
+    /// The host identifier is retrieve from the database using the
+    /// mysql_insert_id function.
     void addOptions(const MySqlHostDataSource::StatementIndex& stindex,
                     const ConstCfgOptionPtr& options_cfg,
                     uint64_t& host_id);
@@ -1671,13 +1851,14 @@ public:
                     const char* what) const;
 
     /// @brief Creates collection of @ref Host objects with associated
-    /// information such as IPv6 reservations.
+    /// information such as IPv6 reservations and/or DHCP options..
     ///
     /// This method performs a query which returns host information from
     /// the 'hosts' table. The query may also use LEFT JOIN clause to
-    /// retrieve information from other tables, e.g. ipv6_reservations.
-    /// Whether IPv6 reservations are assigned to the @ref Host objects
-    /// depends on the type of the exchange object.
+    /// retrieve information from other tables, e.g. ipv6_reservations,
+    /// dhcp4_options and dhcp6_options.
+    /// Whether IPv6 reservations and/or options are assigned to the
+    /// @ref Host objects depends on the type of the exchange object.
     ///
     /// @param stindex Statement index.
     /// @param bind Pointer to an array of MySQL bindings.
@@ -1715,16 +1896,16 @@ public:
                          boost::shared_ptr<MySqlHostExchange> exchange) const;
 
     /// @brief Pointer to the object representing an exchange which
-    /// can be used to retrieve DHCPv4 reservation.
+    /// can be used to retrieve hosts and DHCPv4 options.
     boost::shared_ptr<MySqlHostExchangeOpts> host_exchange_;
 
     /// @brief Pointer to an object representing an exchange which can
-    /// be used to retrieve DHCPv6 reservations.
+    /// be used to retrieve hosts, DHCPv6 options and IPv6 reservations.
     boost::shared_ptr<MySqlHostIPv6Exchange> host_ipv6_exchange_;
 
     /// @brief Pointer to an object representing an exchange which can
-    /// be used to retrieve DHCPv4 and DHCPv6 reservations using a
-    /// single query.
+    /// be used to retrieve hosts, DHCPv4 and DHCPv6 options, and
+    /// IPv6 reservations using a single query.
     boost::shared_ptr<MySqlHostIPv6Exchange> host_ipv46_exchange_;
 
     /// @brief Pointer to an object representing an exchange which can
@@ -1732,8 +1913,9 @@ public:
     boost::shared_ptr<MySqlIPv6ReservationExchange> host_ipv6_reservation_exchange_;
 
     /// @brief Pointer to an object representing an exchange which can
-    /// be used to insert DHCPv4 option into 'dhcp4_options' table.
-    boost::shared_ptr<MySqlOptionExchange> host_ipv4_option_exchange_;
+    /// be used to insert DHCPv4 or DHCPv6 option into dhcp4_options
+    /// or dhcp6_options table.
+    boost::shared_ptr<MySqlOptionExchange> host_option_exchange_;
 
     /// @brief MySQL connection
     MySqlConnection conn_;
@@ -1747,7 +1929,7 @@ MySqlHostDataSourceImpl(const MySqlConnection::ParameterMap& parameters)
       host_ipv46_exchange_(new MySqlHostIPv6Exchange(MySqlHostExchangeOpts::
                                                      DHCP4_AND_DHCP6)),
       host_ipv6_reservation_exchange_(new MySqlIPv6ReservationExchange()),
-      host_ipv4_option_exchange_(new MySqlOptionExchange()),
+      host_option_exchange_(new MySqlOptionExchange()),
       conn_(parameters) {
 
     // Open the database.
@@ -1819,8 +2001,8 @@ MySqlHostDataSourceImpl::addOption(const MySqlHostDataSource::StatementIndex& st
                                    const OptionalValue<SubnetID>& subnet_id,
                                    const HostID& id) {
     std::vector<MYSQL_BIND> bind =
-        host_ipv4_option_exchange_->createBindForSend(opt_desc, opt_space,
-                                                      subnet_id, id);
+        host_option_exchange_->createBindForSend(opt_desc, opt_space,
+                                                 subnet_id, id);
 
     addQuery(stindex, bind);
 }
@@ -1829,6 +2011,8 @@ void
 MySqlHostDataSourceImpl::addOptions(const MySqlHostDataSource::StatementIndex& stindex,
                                     const ConstCfgOptionPtr& options_cfg,
                                     uint64_t& host_id) {
+    // Get option space names and vendor space names and combine them within a
+    // single list.
     std::list<std::string> option_spaces = options_cfg->getOptionSpaceNames();
     std::list<std::string> vendor_spaces = options_cfg->getVendorIdsSpaceNames();
     option_spaces.insert(option_spaces.end(), vendor_spaces.begin(),
@@ -1839,6 +2023,9 @@ MySqlHostDataSourceImpl::addOptions(const MySqlHostDataSource::StatementIndex& s
     if ((host_id == 0) && !option_spaces.empty()) {
         host_id = mysql_insert_id(conn_.mysql_);
     }
+
+    // For each option space retrieve all options and insert them into the
+    // database.
     for (std::list<std::string>::const_iterator space = option_spaces.begin();
          space != option_spaces.end(); ++space) {
         OptionContainerPtr options = options_cfg->getAll(*space);
@@ -1982,6 +2169,10 @@ MySqlHostDataSource::~MySqlHostDataSource() {
 
 void
 MySqlHostDataSource::add(const HostPtr& host) {
+    // Initiate MySQL transaction as we will have to make multiple queries
+    // to insert host information into multiple tables. If that fails on
+    // any stage, the transaction will be rolled back by the destructor of
+    // the MySqlTransaction class.
     MySqlTransaction transaction(impl_->conn_);
 
     // Create the MYSQL_BIND array for the host
@@ -1993,6 +2184,7 @@ MySqlHostDataSource::add(const HostPtr& host) {
     // Gets the last inserted hosts id
     uint64_t host_id = 0;
 
+    // Insert IPv6 reservations.
     IPv6ResrvRange v6resv = host->getIPv6Reservations();
     if (std::distance(v6resv.first, v6resv.second) > 0) {
         host_id = mysql_insert_id(impl_->conn_.mysql_);
@@ -2002,16 +2194,19 @@ MySqlHostDataSource::add(const HostPtr& host) {
         }
     }
 
+    // Insert DHCPv4 options.
     ConstCfgOptionPtr cfg_option4 = host->getCfgOption4();
     if (cfg_option4) {
         impl_->addOptions(INSERT_V4_OPTION, cfg_option4, host_id);
     }
 
+    // Insert DHCPv6 options.
     ConstCfgOptionPtr cfg_option6 = host->getCfgOption6();
     if (cfg_option6) {
         impl_->addOptions(INSERT_V6_OPTION, cfg_option6, host_id);
     }
 
+    // Everything went fine, so explicitly commit the transaction.
     transaction.commit();
 }
 
index 7c0d8c363e164dbda88b5e41d5c3451440549835..415cb7a4a226ab9a6e8ad2c3611a5363655b9812 100644 (file)
@@ -253,6 +253,7 @@ void GenericHostDataSourceTest::compareHosts(const ConstHostPtr& host1,
     compareClientClasses(host1->getClientClasses6(),
                          host2->getClientClasses6());
 
+    // Compare DHCPv4 and DHCPv6 options.
     compareOptions(host1->getCfgOption4(), host2->getCfgOption4());
     compareOptions(host1->getCfgOption6(), host2->getCfgOption6());
 }
@@ -339,27 +340,41 @@ GenericHostDataSourceTest::compareOptions(const ConstCfgOptionPtr& cfg1,
     ASSERT_TRUE(cfg1);
     ASSERT_TRUE(cfg2);
 
+    // Combine option space names with vendor space names in a single list.
     std::list<std::string> option_spaces = cfg2->getOptionSpaceNames();
     std::list<std::string> vendor_spaces = cfg2->getVendorIdsSpaceNames();
     option_spaces.insert(option_spaces.end(), vendor_spaces.begin(),
                          vendor_spaces.end());
 
+    // Iterate over all option spaces existing in cfg2.
     BOOST_FOREACH(std::string space, option_spaces) {
+        // Retrieve options belonging to the current option space.
         OptionContainerPtr options1 = cfg1->getAll(space);
         OptionContainerPtr options2 = cfg2->getAll(space);
-        ASSERT_TRUE(options1);
-        ASSERT_TRUE(options2);
+        ASSERT_TRUE(options1) << "failed for option space " << space;
+        ASSERT_TRUE(options2) << "failed for option space " << space;
 
-        ASSERT_EQ(options1->size(), options2->size());
+        // If number of options doesn't match, the test fails.
+        ASSERT_EQ(options1->size(), options2->size())
+            << "failed for option space " << space;
 
+        // Iterate over all options within this option space.
         BOOST_FOREACH(OptionDescriptor desc1, *options1) {
             OptionDescriptor desc2 = cfg2->get(space, desc1.option_->getType());
-            ASSERT_EQ(desc1.persistent_, desc2.persistent_);
-            ASSERT_EQ(desc1.formatted_value_, desc2.formatted_value_);
+            // Compare persistent flag.
+            EXPECT_EQ(desc1.persistent_, desc2.persistent_)
+                << "failed for option " << space << "." << desc1.option_->getType();
+            // Compare formatted value.
+            EXPECT_EQ(desc1.formatted_value_, desc2.formatted_value_)
+                << "failed for option " << space << "." << desc1.option_->getType();
+
+            // Retrieve options.
             Option* option1 = desc1.option_.get();
             Option* option2 = desc2.option_.get();
 
-            ASSERT_TRUE(typeid(*option1) == typeid(*option2))
+            // Options must be represented by the same C++ class derived from
+            // the Option class.
+            EXPECT_TRUE(typeid(*option1) == typeid(*option2))
                 << "Comapared DHCP options, having option code "
                 << desc1.option_->getType() << " and belonging to the "
                 << space << " option space, are represented "
@@ -367,13 +382,18 @@ GenericHostDataSourceTest::compareOptions(const ConstCfgOptionPtr& cfg1,
                 << typeid(*option1).name() << " vs "
                 << typeid(*option2).name();
 
+            // Because we use different C++ classes to represent different
+            // options, the simplest way to make sure that the options are
+            // equal is to simply compare them in wire format.
             OutputBuffer buf1(option1->len());
             ASSERT_NO_THROW(option1->pack(buf1));
             OutputBuffer buf2(option2->len());
             ASSERT_NO_THROW(option2->pack(buf2));
 
-            ASSERT_EQ(buf1.getLength(), buf2.getLength());
-            ASSERT_EQ(0, memcmp(buf1.getData(), buf2.getData(), buf1.getLength()));
+            ASSERT_EQ(buf1.getLength(), buf2.getLength())
+                 << "failed for option " << space << "." << desc1.option_->getType();
+            EXPECT_EQ(0, memcmp(buf1.getData(), buf2.getData(), buf1.getLength()))
+                << "failed for option " << space << "." << desc1.option_->getType();
         }
     }
 }
@@ -397,6 +417,8 @@ GenericHostDataSourceTest::createVendorOption(const Option::Universe& universe,
 
     std::ostringstream s;
     if (formatted) {
+        // Vendor id comprises vendor-id field, for which we need to
+        // assign a value in the textual (formatted) format.
         s << vendor_id;
     }
 
@@ -467,6 +489,9 @@ GenericHostDataSourceTest::addTestOptions(const HostPtr& host,
                                                           "ipv6-address", true)),
                  "isc2");
 
+    // Register created "runtime" option definitions. They will be used by a
+    // host data source to convert option data into the appropriate option
+    // classes when the options are retrieved.
     LibDHCP::setRuntimeOptionDefs(defs);
 }
 
@@ -968,37 +993,6 @@ void GenericHostDataSourceTest::testAddDuplicate4() {
     EXPECT_NO_THROW(hdsptr_->add(host));
 }
 
-void GenericHostDataSourceTest::testAddRollback() {
-    // Make sure we have the pointer to the host data source.
-    ASSERT_TRUE(hdsptr_);
-
-    MySqlConnection::ParameterMap params;
-    params["name"] = "keatest";
-    params["user"] = "keatest";
-    params["password"] = "keatest";
-    MySqlConnection conn(params);
-    ASSERT_NO_THROW(conn.openDatabase());
-    int status = mysql_query(conn.mysql_, "DROP TABLE IF EXISTS ipv6_reservations");
-    ASSERT_EQ(0, status) << mysql_error(conn.mysql_);
-
-    // Create a host reservations.
-    HostPtr host = initializeHost6("2001:db8:1::1", Host::IDENT_HWADDR, false);
-
-    host->setIPv4SubnetID(SubnetID(4));
-
-    // Create IPv6 reservation (for an address) and add it to the host
-    IPv6Resrv resv(IPv6Resrv::TYPE_NA, IOAddress("2001:db8::2"), 128);
-    host->addReservation(resv);
-
-    ASSERT_THROW(hdsptr_->add(host), DbOperationError);
-
-    ConstHostPtr from_hds = hdsptr_->get4(host->getIPv4SubnetID(),
-                                          host->getIdentifierType(),
-                                          &host->getIdentifier()[0],
-                                          host->getIdentifier().size());
-    ASSERT_FALSE(from_hds);
-}
-
 void GenericHostDataSourceTest::testAddr6AndPrefix(){
     // Make sure we have the pointer to the host data source.
     ASSERT_TRUE(hdsptr_);
index 14a68e6071f612e8fbbf6beb18e942824ac164b2..1e8bb758451550e4496df154835b860ede6a35f2 100644 (file)
@@ -131,24 +131,44 @@ public:
 
     /// @brief Compares options within two configurations.
     ///
+    /// This method uses gtest macros to signal errors.
+    ///
     /// @param cfg1 First configuration.
     /// @param cfg2 Second configuration.
     void compareOptions(const ConstCfgOptionPtr& cfg1,
                         const ConstCfgOptionPtr& cfg2) const;
 
+    /// @brief Creates an opton descriptor holding an empty option.
+    ///
+    /// @param universe V4 or V6.
+    /// @param option_type Option type.
+    /// @param persist A boolean flag indicating if the option is always
+    /// returned to the client or only when requested.
+    ///
+    /// @return Descriptor holding an empty option.
     OptionDescriptor createEmptyOption(const Option::Universe& universe,
                                        const uint16_t option_type,
                                        const bool persist) const;
 
     /// @brief Creates an instance of the option for which it is possible to
-    /// specify universe, option type and value in the constructor.
+    /// specify universe, option type, persistence flag  and value in
+    /// the constructor.
     ///
     /// Examples of options that can be created using this function are:
     /// - @ref OptionString
     /// - different variants of @ref OptionInt.
     ///
-    /// @param encapsulated_space 
-    /// @return Instance of the option created.
+    /// @param universe V4 or V6.
+    /// @param option_type Option type.
+    /// @param persist A boolean flag indicating if the option is always
+    /// returned to the client or only when requested.
+    /// @param formatted A boolean value selecting if the formatted option
+    /// value should be used (if true), or binary value (if false).
+    /// @param value Option value to be assigned to the option.
+    /// @tparam OptionType Class encapsulating the option.
+    /// @tparam DataType Option value data type.
+    ///
+    /// @return Descriptor holding an instance of the option created.
     template<typename OptionType, typename DataType>
     OptionDescriptor createOption(const Option::Universe& universe,
                                   const uint16_t option_type,
@@ -159,12 +179,31 @@ public:
                                                             value));
         std::ostringstream s;
         if (formatted) {
+            // Using formatted option value. Convert option value to a
+            // textual format.
             s << value;
         }
         OptionDescriptor desc(option, persist, s.str());
         return (desc);
     }
 
+    /// @brief Creates an instance of the option for which it is possible to
+    /// specify option type, persistence flag  and value in the constructor.
+    ///
+    /// Examples of options that can be created using this function are:
+    /// - @ref Option4AddrLst
+    /// - @ref Option6AddrLst
+    ///
+    /// @param option_type Option type.
+    /// @param persist A boolean flag indicating if the option is always
+    /// returned to the client or only when requested.
+    /// @param formatted A boolean value selecting if the formatted option
+    /// value should be used (if true), or binary value (if false).
+    /// @param value Option value to be assigned to the option.
+    /// @tparam OptionType Class encapsulating the option.
+    /// @tparam DataType Option value data type.
+    ///
+    /// @return Descriptor holding an instance of the option created.
     template<typename OptionType, typename DataType>
     OptionDescriptor createOption(const uint16_t option_type,
                                   const bool persist,
@@ -174,6 +213,8 @@ public:
 
         std::ostringstream s;
         if (formatted) {
+            // Using formatted option value. Convert option value to a
+            // textual format.
             s << value;
         }
 
@@ -181,6 +222,22 @@ public:
         return (desc);
     }
 
+    /// @brief Creates an instance of the option holding list of IP addresses.
+    ///
+    /// @param option_type Option type.
+    /// @param persist A boolean flag indicating if the option is always
+    /// returned to the client or only when requested.
+    /// @param formatted A boolean value selecting if the formatted option
+    /// value should be used (if true), or binary value (if false).
+    /// @param address1 First address to be included. If address is empty, it is
+    /// not included.
+    /// @param address2 Second address to be included. If address is empty, it
+    /// is not included.
+    /// @param address3 Third address to be included. If address is empty, it
+    /// is not included.
+    /// @tparam OptionType Class encapsulating the option.
+    ///
+    /// @return Descriptor holding an instance of the option created.
     template<typename OptionType>
     OptionDescriptor
     createAddressOption(const uint16_t option_type,
@@ -190,6 +247,7 @@ public:
                         const std::string& address2 = "",
                         const std::string& address3 = "") const {
         std::ostringstream s;
+        // First address.
         typename OptionType::AddressContainer addresses;
         if (!address1.empty()) {
             addresses.push_back(asiolink::IOAddress(address1));
@@ -197,6 +255,7 @@ public:
                 s << address1;
             }
         }
+        // Second address.
         if (!address2.empty()) {
             addresses.push_back(asiolink::IOAddress(address2));
             if (formatted) {
@@ -206,6 +265,7 @@ public:
                 s << address2;
             }
         }
+        // Third address.
         if (!address3.empty()) {
             addresses.push_back(asiolink::IOAddress(address3));
             if (formatted) {
@@ -216,16 +276,49 @@ public:
             }
         }
 
-        boost::shared_ptr<OptionType> option(new OptionType(option_type, addresses));
+        boost::shared_ptr<OptionType> option(new OptionType(option_type,
+                                                            addresses));
         OptionDescriptor desc(option, persist, s.str());
         return (desc);
     }
 
+    /// @brief Creates an instance of the vendor option.
+    ///
+    /// @param universe V4 or V6.
+    /// @param persist A boolean flag indicating if the option is always
+    /// returned to the client or only when requested.
+    /// @param formatted A boolean value selecting if the formatted option
+    /// value should be used (if true), or binary value (if false).
+    /// @param vendor_id Vendor identifier.
+    ///
+    /// @return Descriptor holding an instance of the option created.
     OptionDescriptor createVendorOption(const Option::Universe& universe,
                                         const bool persist,
                                         const bool formatted,
                                         const uint32_t vendor_id) const;
 
+    /// @brief Adds multiple options into the host.
+    ///
+    /// This method creates the following options into the host object:
+    /// - DHCPv4 boot file name option,
+    /// - DHCPv4 default ip ttl option,
+    /// - DHCPv4 option 1 within vendor-encapsulated-options space,
+    /// - DHCPv4 option 254 with a single IPv4 address,
+    /// - DHCPv4 option 1 within isc option space,
+    /// - DHCPv6 boot file url option,
+    /// - DHCPv6 information refresh time option,
+    /// - DHCPv6 vendor option with vendor id 2495,
+    /// - DHCPv6 option 1024, with a sigle IPv6 address,
+    /// - DHCPv6 empty option 1, within isc2 option space,
+    /// - DHCPv6 option 2, within isc2 option space with 3 IPv6 addresses,
+    ///
+    /// This method also creates option definitions for the non-standard
+    /// options and registers them in the LibDHCP as runtime option
+    /// definitions.
+    ///
+    /// @param host Host object into which options should be added.
+    /// @param formatted A boolean value selecting if the formatted option
+    /// value should be used (if true), or binary value (if false).
     void addTestOptions(const HostPtr& host, const bool formatted) const;
 
     /// @brief Pointer to the host data source
@@ -342,8 +435,6 @@ public:
     /// Uses gtest macros to report failures.
     void testAddDuplicate4();
 
-    void testAddRollback();
-
     /// @brief Test that DHCPv4 options can be inserted and retrieved from
     /// the database.
     ///
index 64801a6c6d5c9a583d76660b1e4943f47188a689..7f93e3149f8607e74c51a990e7ee69a92d313b5b 100644 (file)
@@ -437,13 +437,56 @@ TEST_F(MySqlHostDataSourceTest, formattedOptionsReservations6) {
 }
 
 // This test verifies that DHCPv4 and DHCPv6 options can be inserted in a
-/// textual format and retrieved with a single query to the database.
+// textual format and retrieved with a single query to the database.
 TEST_F(MySqlHostDataSourceTest, formattedOptionsReservations46) {
     testOptionsReservations46(true);
 }
 
+// This test checks transactional insertion of the host information
+// into the database. The failure to insert host information at
+// any stage should cause the whole transaction to be rolled back.
 TEST_F(MySqlHostDataSourceTest, testAddRollback) {
-    testAddRollback();
+    // Make sure we have the pointer to the host data source.
+    ASSERT_TRUE(hdsptr_);
+
+    // To test the transaction rollback mechanism we need to cause the
+    // insertion of host information to fail at some stage. The 'hosts'
+    // table should be updated correctly but the failure should occur
+    // when inserting reservations or options. The simplest way to
+    // achieve that is to simply drop one of the tables. To do so, we
+    // connect to the database and issue a DROP query.
+    MySqlConnection::ParameterMap params;
+    params["name"] = "keatest";
+    params["user"] = "keatest";
+    params["password"] = "keatest";
+    MySqlConnection conn(params);
+    ASSERT_NO_THROW(conn.openDatabase());
+    int status = mysql_query(conn.mysql_,
+                             "DROP TABLE IF EXISTS ipv6_reservations");
+    ASSERT_EQ(0, status) << mysql_error(conn.mysql_);
+
+    // Create a host with a reservation.
+    HostPtr host = initializeHost6("2001:db8:1::1", Host::IDENT_HWADDR, false);
+    // Let's assign some DHCPv4 subnet to the host, because we will use the
+    // DHCPv4 subnet to try to retrieve the host after failed insertion.
+    host->setIPv4SubnetID(SubnetID(4));
+
+    // There is no ipv6_reservations table, so the insertion should fail.
+    ASSERT_THROW(hdsptr_->add(host), DbOperationError);
+
+    // Even though we have created a DHCPv6 host, we can't use get6()
+    // method to retrieve the host from the database, because the
+    // query would expect that the ipv6_reservations table is present.
+    // Therefore, the query would fail. Instead, we use the get4 method
+    // which uses the same client identifier, but doesn't attempt to
+    // retrieve the data from ipv6_reservations table. The query should
+    // pass but return no host because the (insert) transaction is expected
+    // to be rolled back.
+    ConstHostPtr from_hds = hdsptr_->get4(host->getIPv4SubnetID(),
+                                          host->getIdentifierType(),
+                                          &host->getIdentifier()[0],
+                                          host->getIdentifier().size());
+    EXPECT_FALSE(from_hds);
 }
 
 }; // Of anonymous namespace