[4212] Addressed review comments.

- Introduced constants for MySQL fetch results
- Simplified loops in generic SQL unit tests
- Removed some typos
- Removed unused variables

diff --git a/src/lib/dhcpsrv/mysql_connection.cc b/src/lib/dhcpsrv/mysql_connection.cc
index 10f841905d1dca2f0c687b27a7d98286a2ef74d6..fbfb345f25edd5b542f6baa83ebf056406fd7c49 100755 (executable)
--- a/src/lib/dhcpsrv/mysql_connection.cc
+++ b/src/lib/dhcpsrv/mysql_connection.cc
@@ -30,21 +30,10 @@ using namespace std;
 namespace isc {
 namespace dhcp {
 
-/// @brief Maximum size of database fields
-///
-/// The following constants define buffer sizes for variable length database
-/// fields.  The values should be greater than or equal to the length set in
-/// the schema definition.
-///
-/// The exception is the length of any VARCHAR fields: buffers for these should
-/// be set greater than or equal to the length of the field plus 1: this allows
-/// for the insertion of a trailing null whatever data is returned.
-
-const my_bool MLM_FALSE = 0;                ///< False value
-const my_bool MLM_TRUE = 1;                 ///< True value
-
-///@}
-
+const my_bool MLM_FALSE = 0;
+const my_bool MLM_TRUE = 1;
+const int MLM_MYSQL_FETCH_SUCCESS = 0;
+const int MLM_MYSQL_FETCH_FAILURE = 1;
 
 // Open the database using the parameters passed to the constructor.
 

diff --git a/src/lib/dhcpsrv/mysql_connection.h b/src/lib/dhcpsrv/mysql_connection.h
index 8d52c57a306a4ecd8c18009d7e0548b3cc4c1774..8d208234ff34ecf7c7198c40d663fb7afa10e922 100755 (executable)
--- a/src/lib/dhcpsrv/mysql_connection.h
+++ b/src/lib/dhcpsrv/mysql_connection.h
@@ -24,20 +24,31 @@
 namespace isc {
 namespace dhcp {
 
-/// @brief MySQL True/False constants
+/// @name MySQL constants.
 ///
-/// Declare typed values so as to avoid problems of data conversion.  These
-/// are local to the file but are given the prefix MLM (MySql Lease Manager) to
-/// avoid any likely conflicts with variables in header files named TRUE or
-/// FALSE.
+//@{
+
+/// @brief MySQL false value.
 extern const my_bool MLM_FALSE;
+
+/// @brief MySQL true value.
 extern const my_bool MLM_TRUE;
 
-// Define the current database schema values
+/// @brief MySQL fetch success code.
+extern const int MLM_MYSQL_FETCH_SUCCESS;
+
+/// @brief MySQL fetch failure code.
+extern const int MLM_MYSQL_FETCH_FAILURE;
 
+//@}
+
+/// @name Current database schema version values.
+//@{
 const uint32_t CURRENT_VERSION_VERSION = 3;
 const uint32_t CURRENT_VERSION_MINOR = 0;
 
+//@}
+
 /// @brief Fetch and Release MySQL Results
 ///
 /// When a MySQL statement is expected, to fetch the results the function

diff --git a/src/lib/dhcpsrv/mysql_host_data_source.cc b/src/lib/dhcpsrv/mysql_host_data_source.cc
index 17e6ed2ca3d57ec912ea5a490aa6df29b8c910b7..10599bdefc76224b048bf4f5df52e7316132417c 100644 (file)
--- a/src/lib/dhcpsrv/mysql_host_data_source.cc
+++ b/src/lib/dhcpsrv/mysql_host_data_source.cc
@@ -842,21 +842,12 @@ public:
 private:
     uint64_t   host_id_;        /// Host unique identifier
     uint64_t    reservation_id_; /// Reservation unique identifier
-    size_t      host_id_length_; /// Length of the host unique ID
     std::string address_;        ///< Address (or prefix)
     size_t      address_len_;    ///< Length of the textual address representation
     uint8_t     prefix_len_;     ///< Length of the prefix (128 for addresses)
     uint8_t     type_;
     uint8_t     iaid_;
 
-    uint64_t    host_id_len_;
-
-    // NULL flags for subnets id, ipv4 address, hostname and client classes
-    my_bool     host_id_null_;
-    my_bool     address_null_;
-    my_bool     prefix_len_null_;
-    my_bool     iaid_null_;
-
     IPv6Resrv   resv_;
 
     MYSQL_BIND  bind_[RESRV_COLUMNS];
@@ -999,7 +990,8 @@ MySqlHostDataSource::getIPv6ReservationCollection(StatementIndex stindex,
     // retrieve the data. mysql_stmt_fetch return value equal to 0 represents
     // successful data fetch.
     MySqlFreeResult fetch_release(conn_.statements_[stindex]);
-    while ((status = mysql_stmt_fetch(conn_.statements_[stindex])) == 0) {
+    while ((status = mysql_stmt_fetch(conn_.statements_[stindex])) ==
+           MLM_MYSQL_FETCH_SUCCESS) {
         try {
             result.insert(IPv6ResrvTuple(exchange->getIPv6ReservData().getType(),
                                             exchange->getIPv6ReservData()));
@@ -1013,9 +1005,10 @@ MySqlHostDataSource::getIPv6ReservationCollection(StatementIndex stindex,
 
     // How did the fetch end?
     // If mysql_stmt_fetch return value is equal to 1 an error occurred.
-    if (status == 1) {
+    if (status == MLM_MYSQL_FETCH_FAILURE) {
         // Error - unable to fetch results
         checkError(status, stindex, "unable to fetch results");
+
     } else if (status == MYSQL_DATA_TRUNCATED) {
         // Data truncated - throw an exception indicating what was at fault
         isc_throw(DataTruncated, conn_.text_statements_[stindex]
@@ -1106,9 +1099,10 @@ MySqlHostDataSource::getHostCollection(StatementIndex stindex, MYSQL_BIND* bind,
 
     // How did the fetch end?
     // If mysql_stmt_fetch return value is equal to 1 an error occurred.
-    if (status == 1) {
+    if (status == MLM_MYSQL_FETCH_FAILURE) {
         // Error - unable to fetch results
         checkError(status, stindex, "unable to fetch results");
+
     } else if (status == MYSQL_DATA_TRUNCATED) {
         // Data truncated - throw an exception indicating what was at fault
         isc_throw(DataTruncated, conn_.text_statements_[stindex]

diff --git a/src/lib/dhcpsrv/tests/generic_host_data_source_unittest.cc b/src/lib/dhcpsrv/tests/generic_host_data_source_unittest.cc
index 6220873c1fbc7f1c756498d01680e0830cd21a0f..a178b63c0d5af709e0221b364a2b3a66a3cc8188 100644 (file)
--- a/src/lib/dhcpsrv/tests/generic_host_data_source_unittest.cc
+++ b/src/lib/dhcpsrv/tests/generic_host_data_source_unittest.cc
@@ -250,8 +250,7 @@ void GenericHostDataSourceTest::compareHosts(const ConstHostPtr& host1,
 
     // Compare IPv6 reservations
     compareReservations6(host1->getIPv6Reservations(),
-                         host2->getIPv6Reservations(),
-                         true);
+                         host2->getIPv6Reservations());
 
     // And compare client classification details
     compareClientClasses(host1->getClientClasses4(),
@@ -282,21 +281,34 @@ GenericHostDataSourceTest::DuidToHWAddr(const DuidPtr& duid) {
 
 void
 GenericHostDataSourceTest::compareReservations6(IPv6ResrvRange resrv1,
-                                                IPv6ResrvRange resrv2,
-                                                bool expect_match) {
+                                                IPv6ResrvRange resrv2) {
 
     // Compare number of reservations for both hosts
     if (std::distance(resrv1.first, resrv1.second) !=
             std::distance(resrv2.first, resrv2.second)){
-        // Number of reservations is not equal.
-        // Let's see if it's a problem.
-        if (expect_match) {
-            ADD_FAILURE()<< "Reservation comparison failed, "
-                    "hosts got different number of reservations.";
-        }
+        ADD_FAILURE()<< "Reservation comparison failed, "
+            "hosts got different number of reservations.";
         return;
     }
 
+    // Iterate over the range of reservations to find a match in the
+    // reference range.
+    for (IPv6ResrvIterator r1 = resrv1.first; r1 != resrv1.second; ++r1) {
+        IPv6ResrvIterator r2 = resrv2.first;
+        for (; r2 != resrv2.second; ++r2) {
+            // IPv6Resrv object implements equality operator.
+            if (r1->second == r2->second) {
+                break;
+            }
+        }
+        // If r2 iterator reached the end of the range it means that there
+        // is no match.
+        if (r2 == resrv2.second) {
+            ADD_FAILURE() << "No match found for reservation: "
+                          << resrv1.first->second.getPrefix().toText();
+        }
+    }
+
     if (std::distance(resrv1.first, resrv1.second) > 0) {
         for (; resrv1.first != resrv1.second; resrv1.first++) {
             IPv6ResrvIterator iter = resrv2.first;
@@ -307,7 +319,7 @@ GenericHostDataSourceTest::compareReservations6(IPv6ResrvRange resrv1,
                     break;
                 }
                 iter++;
-                if (iter == resrv2.second && expect_match) {
+                if (iter == resrv2.second) {
                     ADD_FAILURE()<< "Reservation comparison failed, "
                     "no match for reservation: "
                     << resrv1.first->second.getPrefix().toText();
@@ -824,10 +836,10 @@ void GenericHostDataSourceTest::testAddr6AndPrefix(){
     ASSERT_TRUE(from_hds);
 
     // Check if reservations are the same
-    compareReservations6(host->getIPv6Reservations(), from_hds->getIPv6Reservations(), true);
+    compareReservations6(host->getIPv6Reservations(), from_hds->getIPv6Reservations());
 }
 
-void GenericHostDataSourceTest::testMultipletReservations(){
+void GenericHostDataSourceTest::testMultipleReservations(){
     // Make sure we have the pointer to the host data source.
     ASSERT_TRUE(hdsptr_);
     uint8_t len = 128;
@@ -857,7 +869,7 @@ void GenericHostDataSourceTest::testMultipletReservations(){
     compareHosts(host, from_hds);
 }
 
-void GenericHostDataSourceTest::testMultipletReservationsDifferentOrder(){
+void GenericHostDataSourceTest::testMultipleReservationsDifferentOrder(){
     // Make sure we have the pointer to the host data source.
     ASSERT_TRUE(hdsptr_);
     uint8_t len = 128;
@@ -882,7 +894,7 @@ void GenericHostDataSourceTest::testMultipletReservationsDifferentOrder(){
     host2->addReservation(resv1);
 
     // Check if reservations are the same
-    compareReservations6(host1->getIPv6Reservations(), host2->getIPv6Reservations(), true);
+    compareReservations6(host1->getIPv6Reservations(), host2->getIPv6Reservations());
 
 }
 

diff --git a/src/lib/dhcpsrv/tests/generic_host_data_source_unittest.h b/src/lib/dhcpsrv/tests/generic_host_data_source_unittest.h
index 1f4a66a32441b61f5f3292881871daa4689174dd..ed136c5be0f53c838710cb794221f66f35499fdc 100644 (file)
--- a/src/lib/dhcpsrv/tests/generic_host_data_source_unittest.h
+++ b/src/lib/dhcpsrv/tests/generic_host_data_source_unittest.h
@@ -112,8 +112,7 @@ public:
     ///
     /// @param resv1 first IPv6 reservations list
     /// @param resv2 second IPv6 reservations list
-    void compareReservations6(IPv6ResrvRange resv1, IPv6ResrvRange resv2,
-                              bool expect_match);
+    void compareReservations6(IPv6ResrvRange resv1, IPv6ResrvRange resv2);
 
     /// @brief Compares two client classes
     ///
@@ -212,12 +211,12 @@ public:
     void testAddr6AndPrefix();
 
     /// @brief Tests if host with multiple IPv6 reservations can be added and then
-    ///         retrieved correctly.
-    void testMultipletReservations();
+    ///        retrieved correctly.
+    void testMultipleReservations();
 
     /// @brief Tests if compareIPv6Reservations() method treats same pool of
-    ///         reservations but added in different order as equal.
-    void testMultipletReservationsDifferentOrder();
+    ///        reservations but added in different order as equal.
+    void testMultipleReservationsDifferentOrder();
 
     /// @brief Test if host reservations made for different IPv6 subnets
     ///        are handled correctly.

diff --git a/src/lib/dhcpsrv/tests/mysql_host_data_source_unittest.cc b/src/lib/dhcpsrv/tests/mysql_host_data_source_unittest.cc
index 5b06e8f7723dcdecae56ccaeea15fc0c95ac3484..307c2bbd058001955d8a4baa708e7d3668291292 100644 (file)
--- a/src/lib/dhcpsrv/tests/mysql_host_data_source_unittest.cc
+++ b/src/lib/dhcpsrv/tests/mysql_host_data_source_unittest.cc
@@ -322,13 +322,13 @@ TEST_F(MySqlHostDataSourceTest, addr6AndPrefix) {
 // Tests if host with multiple IPv6 reservations can be added and then
 // retrieved correctly. Test checks reservations comparing.
 TEST_F(MySqlHostDataSourceTest, multipleReservations){
-    testMultipletReservations();
+    testMultipleReservations();
 }
 
 // Tests if compareIPv6Reservations() method treats same pool of reservations
 // but added in different order as equal.
 TEST_F(MySqlHostDataSourceTest, multipleReservationsDifferentOrder){
-    testMultipletReservationsDifferentOrder();
+    testMultipleReservationsDifferentOrder();
 }
 
 // Test verifies if multiple client classes for IPv4 can be stored.