[4281] Updated recently modified files with proper commentary.

Also, fixed a couple of minor issues.

src/lib/dhcp/libdhcp++.cc

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

src/lib/dhcpsrv/cfg_option.cc

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

src/lib/dhcpsrv/cfg_option.h

@@ -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:

src/lib/dhcpsrv/mysql_connection.cc

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

src/lib/dhcpsrv/mysql_connection.h

@@ -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_;
-
 };
 
 

src/lib/dhcpsrv/tests/generic_host_data_source_unittest.cc

@@ -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_);

src/lib/dhcpsrv/tests/generic_host_data_source_unittest.h

@@ -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.
     ///

src/lib/dhcpsrv/tests/mysql_host_data_source_unittest.cc

@@ -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