[#4249] Make leasesX-write more resilient

/src/lib/dhcpsrv/memfile_lease_mgr.cc
    Memfile_LeaseMgr::writeLeases4Internal()
    Memfile_LeaseMgr::writeLeases6Internal() - rewritten to clean up
    on failure

/src/lib/dhcpsrv/tests/memfile_lease_mgr_unittest.cc
    Updated UTs

diff --git a/src/lib/dhcpsrv/memfile_lease_mgr.cc b/src/lib/dhcpsrv/memfile_lease_mgr.cc
index 0670ba24ae5ff1cbdadd38eaf723d0f9bfe738c1..e6c5931acc34402331beeced80d8a44733d65da9 100644 (file)
--- a/src/lib/dhcpsrv/memfile_lease_mgr.cc
+++ b/src/lib/dhcpsrv/memfile_lease_mgr.cc
@@ -3732,29 +3732,48 @@ Memfile_LeaseMgr::writeLeases4(const std::string& filename) {
 
 void
 Memfile_LeaseMgr::writeLeases4Internal(const std::string& filename) {
-    bool overwrite = (lease_file4_ && lease_file4_->getFilename() == filename);
+    // Create the temp file name and remove it (if it exists).
+    std::ostringstream tmp;
+    tmp << filename << ".tmp" << getpid();
+    auto tmpname = tmp.str();
+    ::remove(tmpname.c_str());
+
+    // Dump in memory leasses to temp file.
     try {
-        if (overwrite) {
-            lease_file4_->close();
-        }
-        std::ostringstream old;
-        old << filename << ".bak" << getpid();
-        ::rename(filename.c_str(), old.str().c_str());
-        CSVLeaseFile4 backup(filename);
-        backup.open();
+        CSVLeaseFile4 tmpfile(tmpname);
+        tmpfile.open();
         for (auto const& lease : storage4_) {
-            backup.append(*lease);
-        }
-        backup.close();
-        if (overwrite) {
-            lease_file4_->open(true);
+            tmpfile.append(*lease);
         }
+        tmpfile.close();
     } catch (const std::exception& ex) {
-        if (overwrite) {
-            lease_file4_->open(true);
-        }
+        // Failed writing the temp file, remove it.
+        ::remove(tmpname.c_str());
         throw;
     }
+
+    // Create the backup file name.
+    std::ostringstream bak;
+    bak << filename << ".bak" << getpid();
+    auto bakname = bak.str();
+
+    if (lease_file4_ && lease_file4_->getFilename() == filename) {
+        // Overwriting the existing lease file.
+        // Close the existing lease file and move it to back up.
+        lease_file4_->close();
+        ::rename(filename.c_str(), bakname.c_str());
+
+        // Rename the temp file and open it as the lease file.
+        ::rename(tmpname.c_str(), filename.c_str());
+        lease_file4_->open(true);
+    } else {
+        // Dumping to a new file.
+        // Rename the previous dump file (if one) to back up.
+        ::rename(filename.c_str(), bakname.c_str());
+
+        // Rename tmp file to dump file.
+        ::rename(tmpname.c_str(), filename.c_str());
+    }
 }
 
 void
@@ -3769,29 +3788,48 @@ Memfile_LeaseMgr::writeLeases6(const std::string& filename) {
 
 void
 Memfile_LeaseMgr::writeLeases6Internal(const std::string& filename) {
-    bool overwrite = (lease_file6_ && lease_file6_->getFilename() == filename);
+    // Create the temp file name and remove it (if it exists).
+    std::ostringstream tmp;
+    tmp << filename << ".tmp" << getpid();
+    auto tmpname = tmp.str();
+    ::remove(tmpname.c_str());
+
+    // Dump in memory leasses to temp file.
     try {
-        if (overwrite) {
-            lease_file6_->close();
-        }
-        std::ostringstream old;
-        old << filename << ".bak" << getpid();
-        ::rename(filename.c_str(), old.str().c_str());
-        CSVLeaseFile6 backup(filename);
-        backup.open();
+        CSVLeaseFile6 tmpfile(tmpname);
+        tmpfile.open();
         for (auto const& lease : storage6_) {
-            backup.append(*lease);
-        }
-        backup.close();
-        if (overwrite) {
-            lease_file6_->open(true);
-        }
-    } catch (const std::exception&) {
-        if (overwrite) {
-            lease_file6_->open(true);
+            tmpfile.append(*lease);
         }
+        tmpfile.close();
+    } catch (const std::exception& ex) {
+        // Failed writing the temp file, remove it.
+        ::remove(tmpname.c_str());
         throw;
     }
+
+    // Create the backup file name.
+    std::ostringstream bak;
+    bak << filename << ".bak" << getpid();
+    auto bakname = bak.str();
+
+    if (lease_file6_ && lease_file6_->getFilename() == filename) {
+        // Overwriting the existing lease file.
+        // Close the existing lease file and move it to back up.
+        lease_file6_->close();
+        ::rename(filename.c_str(), bakname.c_str());
+
+        // Rename the temp file and open it as the lease file.
+        ::rename(tmpname.c_str(), filename.c_str());
+        lease_file6_->open(true);
+    } else {
+        // Dumping to a new file.
+        // Rename the previous dump file (if one) to back up.
+        ::rename(filename.c_str(), bakname.c_str());
+
+        // Rename tmp file to dump file.
+        ::rename(tmpname.c_str(), filename.c_str());
+    }
 }
 
 TrackingLeaseMgrPtr

diff --git a/src/lib/dhcpsrv/tests/memfile_lease_mgr_unittest.cc b/src/lib/dhcpsrv/tests/memfile_lease_mgr_unittest.cc
index adde1c3b9e645e44b754fb2b926f70aada7d4281..912a1855fdaa2efc47a28593c78a41b6afe4e6c5 100644 (file)
--- a/src/lib/dhcpsrv/tests/memfile_lease_mgr_unittest.cc
+++ b/src/lib/dhcpsrv/tests/memfile_lease_mgr_unittest.cc
@@ -3152,35 +3152,39 @@ public:
 /// @brief Check if writeLease fails on bad file name (v4).
 TEST_F(MemfileLeaseMgrTest, badWriteLease4) {
     startBackend(V4);
-    string expected = "unable to open '/this/does/not/exist'";
+    std::ostringstream oss;
+    oss << "unable to open '/this/does/not/exist.tmp" << getpid() << "'";
     EXPECT_THROW_MSG(lmptr_->writeLeases4("/this/does/not/exist"),
-                     CSVFileError, expected);
+                     CSVFileError, oss.str());
 }
 
 /// @brief Check if writeLease fails on bad file name (v4+MT).
 TEST_F(MemfileLeaseMgrTest, badWriteLease4MultiThread) {
     startBackend(V4);
     MultiThreadingMgr::instance().setMode(true);
-    string expected = "unable to open '/this/does/not/exist'";
+    std::ostringstream oss;
+    oss << "unable to open '/this/does/not/exist.tmp" << getpid() << "'";
     EXPECT_THROW_MSG(lmptr_->writeLeases4("/this/does/not/exist"),
-                     CSVFileError, expected);
+                     CSVFileError, oss.str());
 }
 
 /// @brief Check if writeLease fails on bad file name (v6).
 TEST_F(MemfileLeaseMgrTest, badWriteLease6) {
     startBackend(V6);
-    string expected = "unable to open '/this/does/not/exist'";
+    std::ostringstream oss;
+    oss << "unable to open '/this/does/not/exist.tmp" << getpid() << "'";
     EXPECT_THROW_MSG(lmptr_->writeLeases6("/this/does/not/exist"),
-                     CSVFileError, expected);
+                     CSVFileError, oss.str());
 }
 
 /// @brief Check if writeLease fails on bad file name (v6+MT).
 TEST_F(MemfileLeaseMgrTest, badWriteLease6MultiThread) {
     startBackend(V6);
     MultiThreadingMgr::instance().setMode(true);
-    string expected = "unable to open '/this/does/not/exist'";
+    std::ostringstream oss;
+    oss << "unable to open '/this/does/not/exist.tmp" << getpid() << "'";
     EXPECT_THROW_MSG(lmptr_->writeLeases6("/this/does/not/exist"),
-                     CSVFileError, expected);
+                     CSVFileError, oss.str());
 }
 
 /// @brief Check writeLease basic scenario (v4).
@@ -3433,7 +3437,7 @@ TEST_F(MemfileLeaseMgrTest, overWriteLease4) {
     EXPECT_GT(content.size(), content1.size());
 
     // Backup should have the previous database image.
-    ASSERT_TRUE(backup.exists());
+    ASSERT_TRUE(backup.exists()) << "looking for: " << b.str();
     EXPECT_EQ(content, backup.readFile());
 }