[2723] Test for lease4 with null client-id added, some fixes in MySQL.

src/lib/dhcpsrv/lease_mgr.h

@@ -249,8 +249,10 @@ struct Lease4 : public Lease {
            const uint8_t* clientid, size_t clientid_len, uint32_t valid_lft,
            uint32_t t1, uint32_t t2, time_t cltt, uint32_t subnet_id)
         : Lease(addr, t1, t2, valid_lft, subnet_id, cltt),
-        ext_(0), hwaddr_(hwaddr, hwaddr + hwaddr_len),
-        client_id_(new ClientId(clientid, clientid_len)) {
+        ext_(0), hwaddr_(hwaddr, hwaddr + hwaddr_len) {
+        if (clientid_len) {
+            client_id_.reset(new ClientId(clientid, clientid_len));
+        }
     }
 
     /// @brief Default constructor

src/lib/dhcpsrv/mysql_lease_mgr.cc

@@ -289,7 +289,7 @@ public:
         memset(hwaddr_buffer_, 0, sizeof(hwaddr_buffer_));
         memset(client_id_buffer_, 0, sizeof(client_id_buffer_));
         std::fill(&error_[0], &error_[LEASE_COLUMNS], MLM_FALSE);
- 
+
         // Set the column names (for error messages)
         columns_[0] = "address";
         columns_[1] = "hwaddr";
@@ -353,9 +353,9 @@ public:
             // fields doesn't matter if type is set to MYSQL_TYPE_NULL,
             // but let's set them to some sane values in case earlier versions
             // didn't have that assumption.
-            static my_bool no_clientid = MLM_TRUE;
+            client_id_null_ = MLM_TRUE;
             bind_[2].buffer = NULL;
-            bind_[2].is_null = &no_clientid;
+            bind_[2].is_null = &client_id_null_;
         }
 
         // valid lifetime: unsigned int
@@ -424,6 +424,7 @@ public:
         bind_[2].buffer = reinterpret_cast<char*>(client_id_buffer_);
         bind_[2].buffer_length = client_id_length_;
         bind_[2].length = &client_id_length_;
+        bind_[2].is_null = &client_id_null_;
 
         // lease_time: unsigned int
         bind_[3].buffer_type = MYSQL_TYPE_LONG;
@@ -465,6 +466,11 @@ public:
         time_t cltt = 0;
         MySqlLeaseMgr::convertFromDatabaseTime(expire_, valid_lifetime_, cltt);
 
+        if (client_id_null_==MLM_TRUE) {
+            // There's no client-id, so we pass client-id_length_ set to 0
+            client_id_length_ = 0;
+        }
+
         // note that T1 and T2 are not stored
         return (Lease4Ptr(new Lease4(addr4_, hwaddr_buffer_, hwaddr_length_,
                                      client_id_buffer_, client_id_length_,
@@ -502,6 +508,8 @@ private:
     uint8_t         client_id_buffer_[ClientId::MAX_CLIENT_ID_LEN];
                                         ///< Client ID buffer
     unsigned long   client_id_length_;  ///< Client ID address length
+    my_bool         client_id_null_;    ///< Is Client ID null?
+
     MYSQL_TIME      expire_;            ///< Lease expiry time
     Lease4Ptr       lease_;             ///< Pointer to lease object
     uint32_t        subnet_id_;         ///< Subnet identification
@@ -536,7 +544,7 @@ public:
         memset(addr6_buffer_, 0, sizeof(addr6_buffer_));
         memset(duid_buffer_, 0, sizeof(duid_buffer_));
         std::fill(&error_[0], &error_[LEASE_COLUMNS], MLM_FALSE);
- 
+
         // Set the column names (for error messages)
         columns_[0] = "address";
         columns_[1] = "duid";
@@ -809,7 +817,7 @@ private:
     // schema.
     // Note: arrays are declared fixed length for speed of creation
     std::string     addr6_;             ///< String form of address
-    char            addr6_buffer_[ADDRESS6_TEXT_MAX_LEN + 1];  ///< Character 
+    char            addr6_buffer_[ADDRESS6_TEXT_MAX_LEN + 1];  ///< Character
                                         ///< array form of V6 address
     unsigned long   addr6_length_;      ///< Length of the address
     MYSQL_BIND      bind_[LEASE_COLUMNS]; ///< Bind array
@@ -873,7 +881,7 @@ private:
 
 // MySqlLeaseMgr Constructor and Destructor
 
-MySqlLeaseMgr::MySqlLeaseMgr(const LeaseMgr::ParameterMap& parameters) 
+MySqlLeaseMgr::MySqlLeaseMgr(const LeaseMgr::ParameterMap& parameters)
     : LeaseMgr(parameters), mysql_(NULL) {
 
     // Allocate context for MySQL - it is destroyed in the destructor.
@@ -970,7 +978,7 @@ MySqlLeaseMgr::convertFromDatabaseTime(const MYSQL_TIME& expire,
     expire_tm.tm_hour = expire.hour;
     expire_tm.tm_min = expire.minute;
     expire_tm.tm_sec = expire.second;
-    expire_tm.tm_isdst = -1;    // Let the system work out about DST 
+    expire_tm.tm_isdst = -1;    // Let the system work out about DST
 
     // Convert to local time
     cltt = mktime(&expire_tm) - valid_lifetime;
@@ -1022,7 +1030,7 @@ MySqlLeaseMgr::openDatabase() {
     }
 
     // Set options for the connection:
-    // 
+    //
     // Automatic reconnection: after a period of inactivity, the client will
     // disconnect from the database.  This option causes it to automatically
     // reconnect when another operation is about to be done.
@@ -1088,7 +1096,7 @@ MySqlLeaseMgr::prepareStatements() {
     // Allocate space for all statements
     statements_.clear();
     statements_.resize(NUM_STATEMENTS, NULL);
-    
+
     text_statements_.clear();
     text_statements_.resize(NUM_STATEMENTS, std::string(""));
 

src/lib/dhcpsrv/tests/mysql_lease_mgr_unittest.cc

@@ -712,6 +712,84 @@ TEST_F(MySqlLeaseMgrTest, basicLease4) {
     detailCompareLease(leases[2], l_returned);
 }
 
+/// @brief Basic Lease4 Checks
+///
+/// Checks that the addLease, getLease4(by address), getLease4(hwaddr,subnet_id),
+/// updateLease4() and deleteLease (IPv4 address) can handle NULL client-id.
+/// (client-id is optional and may not be present)
+TEST_F(MySqlLeaseMgrTest, lease4NullClientId) {
+    // Get the leases to be used for the test.
+    vector<Lease4Ptr> leases = createLeases4();
+
+    // Let's clear client-id pointers
+    leases[1]->client_id_ = ClientIdPtr();
+    leases[2]->client_id_ = ClientIdPtr();
+    leases[3]->client_id_ = ClientIdPtr();
+
+    // Start the tests.  Add three leases to the database, read them back and
+    // check they are what we think they are.
+    EXPECT_TRUE(lmptr_->addLease(leases[1]));
+    EXPECT_TRUE(lmptr_->addLease(leases[2]));
+    EXPECT_TRUE(lmptr_->addLease(leases[3]));
+    lmptr_->commit();
+
+    // Reopen the database to ensure that they actually got stored.
+    reopen();
+
+    Lease4Ptr l_returned = lmptr_->getLease4(ioaddress4_[1]);
+    ASSERT_TRUE(l_returned);
+    detailCompareLease(leases[1], l_returned);
+
+    l_returned = lmptr_->getLease4(ioaddress4_[2]);
+    ASSERT_TRUE(l_returned);
+    detailCompareLease(leases[2], l_returned);
+
+    l_returned = lmptr_->getLease4(ioaddress4_[3]);
+    ASSERT_TRUE(l_returned);
+    detailCompareLease(leases[3], l_returned);
+
+    // Check that we can't add a second lease with the same address
+    EXPECT_FALSE(lmptr_->addLease(leases[1]));
+
+
+    // Check that we can get the lease by HWAddr
+    Lease4Collection returned = lmptr_->getLease4(leases[2]->hwaddr_);
+
+    ASSERT_EQ(1, returned.size());
+    detailCompareLease(leases[2], *returned.begin());
+
+    l_returned = lmptr_->getLease4(leases[2]->hwaddr_, leases[2]->subnet_id_);
+    ASSERT_TRUE(l_returned);
+    detailCompareLease(leases[2], l_returned);
+
+
+    // Check that we can update the lease
+    // Modify some fields in lease 1 (not the address) and update it.
+    ++leases[1]->subnet_id_;
+    leases[1]->valid_lft_ *= 2;
+    lmptr_->updateLease4(leases[1]);
+
+    // ... and check that the lease is indeed updated
+    l_returned = lmptr_->getLease4(ioaddress4_[1]);
+    ASSERT_TRUE(l_returned);
+    detailCompareLease(leases[1], l_returned);
+
+
+
+    // Delete a lease, check that it's gone, and that we can't delete it
+    // a second time.
+    EXPECT_TRUE(lmptr_->deleteLease(ioaddress4_[1]));
+    l_returned = lmptr_->getLease4(ioaddress4_[1]);
+    EXPECT_FALSE(l_returned);
+    EXPECT_FALSE(lmptr_->deleteLease(ioaddress4_[1]));
+
+    // Check that the second address is still there.
+    l_returned = lmptr_->getLease4(ioaddress4_[2]);
+    ASSERT_TRUE(l_returned);
+    detailCompareLease(leases[2], l_returned);
+
+}
+
 /// @brief Basic Lease6 Checks
 ///
 /// Checks that the addLease, getLease6 (by address) and deleteLease (with an
@@ -791,14 +869,14 @@ TEST_F(MySqlLeaseMgrTest, getLease4Hwaddr) {
     // Repeat test with just one expected match
     // @todo: Simply use HWAddr directly once 2589 is implemented
     returned = lmptr_->getLease4(HWAddr(leases[2]->hwaddr_, HTYPE_ETHER));
-    EXPECT_EQ(1, returned.size());
+    ASSERT_EQ(1, returned.size());
     detailCompareLease(leases[2], *returned.begin());
 
     // Check that an empty vector is valid
     EXPECT_TRUE(leases[7]->hwaddr_.empty());
     // @todo: Simply use HWAddr directly once 2589 is implemented
     returned = lmptr_->getLease4(HWAddr(leases[7]->hwaddr_, HTYPE_ETHER));
-    EXPECT_EQ(1, returned.size());
+    ASSERT_EQ(1, returned.size());
     detailCompareLease(leases[7], *returned.begin());
 
     // Try to get something with invalid hardware address
@@ -968,13 +1046,13 @@ TEST_F(MySqlLeaseMgrTest, getLease4ClientId) {
 
     // Repeat test with just one expected match
     returned = lmptr_->getLease4(*leases[3]->client_id_);
-    EXPECT_EQ(1, returned.size());
+    ASSERT_EQ(1, returned.size());
     detailCompareLease(leases[3], *returned.begin());
 
     // Check that an empty vector is valid
     EXPECT_TRUE(leases[7]->client_id_->getClientId().empty());
     returned = lmptr_->getLease4(leases[7]->hwaddr_);
-    EXPECT_EQ(1, returned.size());
+    ASSERT_EQ(1, returned.size());
     detailCompareLease(leases[7], *returned.begin());
 
     // Try to get something with invalid client ID
@@ -1113,7 +1191,7 @@ TEST_F(MySqlLeaseMgrTest, getLease6DuidIaidSize) {
         EXPECT_TRUE(lmptr_->addLease(leases[1]));
         Lease6Collection returned = lmptr_->getLease6(*leases[1]->duid_,
                                                       leases[1]->iaid_);
-        EXPECT_EQ(1, returned.size());
+        ASSERT_EQ(1, returned.size());
         detailCompareLease(leases[1], *returned.begin());
         (void) lmptr_->deleteLease(leases[1]->addr_);
     }

src/lib/dhcpsrv/tests/test_utils.cc

@@ -27,7 +27,23 @@ detailCompareLease(const Lease4Ptr& first, const Lease4Ptr& second) {
     // thrown for IPv6 addresses.
     EXPECT_EQ(first->addr_.toText(), second->addr_.toText());
     EXPECT_TRUE(first->hwaddr_ == second->hwaddr_);
-    EXPECT_TRUE(*first->client_id_ == *second->client_id_);
+    if (first->client_id_ && second->client_id_) {
+        EXPECT_TRUE(*first->client_id_ == *second->client_id_);
+    } else {
+        if (first->client_id_ && !second->client_id_) {
+
+            ADD_FAILURE() << "Client-id present in first lease ("
+                          << first->client_id_->getClientId().size()
+                          << " bytes), but missing in second.";
+        }
+        if (!first->client_id_ && second->client_id_) {
+            ADD_FAILURE() << "Client-id missing in first lease, but present in second ("
+                          << second->client_id_->getClientId().size()
+                          << " bytes).";
+        }
+        // else here would mean that both leases do not have client_id_
+        // which makes them equal in that regard. It is ok.
+    }
     EXPECT_EQ(first->valid_lft_, second->valid_lft_);
     EXPECT_EQ(first->cltt_, second->cltt_);
     EXPECT_EQ(first->subnet_id_, second->subnet_id_);