[trac3665] Updates per review comments

One of the large items was to rearrange the
lfc_controller test code to:
- not use .c_str()
- check the non-existance of the temporary files after processing
- remove any files after each subtest
- Use a_1 instead of A_1 for variable names

There were also some other coding standards issues as well
as additional comments and such.

ChangeLog

@@ -1,3 +1,13 @@
+xxx.    [func]          sar
+	A new process, lfc, has been added.  When invoked this
+	process will load leases from a previous lease file and
+	a copy of the current lease file.  It removes duplicate
+	and expired leases and writes the result to an output
+	file.  If all goes well the old files are removed
+	and the new file is renamed to the name of the previous
+	file.  See http://kea.isc.org/wiki/LFCDesign#no1 for
+	the design.
+
 882.	[func]		sar
 	A utility class has been added which handles writing and
 	deleting pid files as well as checking if the process with

src/bin/lfc/lfc_controller.cc

@@ -32,6 +32,11 @@ using namespace std;
 using namespace isc::util;
 using namespace isc::dhcp;
 
+namespace {
+/// @brief Maximum number of errors to allow when reading leases from the file.
+const uint32_t MAX_LEASE_ERRORS = 100;
+}; // namespace anonymous
+
 namespace isc {
 namespace lfc {
 
@@ -42,9 +47,6 @@ const char* LFCController::lfc_app_name_ = "DhcpLFC";
 /// @brief Defines the executable name.
 const char* LFCController::lfc_bin_name_ = "kea-lfc";
 
-/// @brief Maximum number of errors to allow when reading leases from the file.
-const uint32_t MAX_LEASE_ERRORS = 100;
-
 LFCController::LFCController()
     : protocol_version_(0), verbose_(false), config_file_(""), previous_file_(""),
       copy_file_(""), output_file_(""), finish_file_(""), pid_file_("") {
@@ -55,7 +57,7 @@ LFCController::~LFCController() {
 
 void
 LFCController::launch(int argc, char* argv[]) {
-    bool do_clean = true;
+    bool do_rotate = true;
 
     try {
         parseArgs(argc, argv);
@@ -64,58 +66,60 @@ LFCController::launch(int argc, char* argv[]) {
         throw;  // rethrow it
     }
 
-    if (verbose_ == true)
+    if (verbose_) {
         std::cerr << "Starting lease file cleanup" << std::endl;
+    }
 
     // verify we are the only instance
     PIDFile pid_file(pid_file_);
 
     try {
-        if (pid_file.check() == true) {
+        if (pid_file.check()) {
             // Already running instance, bail out
             std::cerr << "LFC instance already running" <<  std::endl;
             return;
         }
-    } catch (const PIDFileError& pid_ex) {
-        std::cerr << pid_ex.what() << std::endl;
-        return;
-    }
 
-    // create the pid file for this instance
-    try {
+        // create the pid file for this instance
         pid_file.write();
     } catch (const PIDFileError& pid_ex) {
         std::cerr << pid_ex.what() << std::endl;
         return;
     }
 
-    // If we don't have a finish file do the processing
-    if (access(finish_file_.c_str(), F_OK) == -1) {
-        if (verbose_ == true)
+    // If we don't have a finish file do the processing.  We
+    // don't know the exact type of the finish file here but
+    // all we care about is if it exists so that's okay
+    CSVFile lf_finish(getFinishFile());
+    if (!lf_finish.exists()) {
+        if (verbose_) {
             std::cerr << "LFC Processing files" << std::endl;
+        }
 
         try {
-            if (protocol_version_ == 4) {
+            if (getProtocolVersion() == 4) {
                 processLeases<Lease4, CSVLeaseFile4, Lease4Storage>();
             } else {
                 processLeases<Lease6, CSVLeaseFile6, Lease6Storage>();
             }
         } catch (const isc::Exception& proc_ex) {
             // We don't want to do the cleanup but do want to get rid of the pid
-            do_clean = false;
+            do_rotate = false;
             std::cerr << "Processing failed: " << proc_ex.what() << std::endl;
         }
     }
 
-    // If do_clean is true We either already had a finish file or
+    // If do_rotate is true We either already had a finish file or
     // were able to create one.  We now want to do the file cleanup,
     // we don't want to return after the catch as we
     // still need to cleanup the pid file
-    if (do_clean == true) {
-        if (verbose_ == true)
+    if (do_rotate) {
+        if (verbose_) {
             std::cerr << "LFC cleaning files" << std::endl;
+        }
+
         try {
-            fileCleanup();
+            fileRotate();
         } catch (const RunTimeFail& run_ex) {
             std::cerr << run_ex.what() << std::endl;
         }
@@ -128,8 +132,9 @@ LFCController::launch(int argc, char* argv[]) {
         std::cerr << pid_ex.what() << std::endl;
     }
 
-    if (verbose_ == true)
+    if (verbose_) {
         std::cerr << "LFC complete" << std::endl;
+    }
 }
 
 void
@@ -266,7 +271,7 @@ LFCController::parseArgs(int argc, char* argv[]) {
     }
 
     // If verbose is set echo the input information
-    if (verbose_ == true) {
+    if (verbose_) {
         std::cerr << "Protocol version:    DHCPv" << protocol_version_ << std::endl
                   << "Previous or ex lease file: " << previous_file_ << std::endl
                   << "Copy lease file:           " << copy_file_ << std::endl
@@ -280,7 +285,7 @@ LFCController::parseArgs(int argc, char* argv[]) {
 
 void
 LFCController::usage(const std::string& text) {
-    if (text != "") {
+    if (!text.empty()) {
         std::cerr << "Usage error: " << text << std::endl;
     }
 
@@ -315,55 +320,56 @@ LFCController::getVersion(const bool extended) const{
 template<typename LeaseObjectType, typename LeaseFileType, typename StorageType>
 void
 LFCController::processLeases() const {
-    LeaseFileType lf_prev(previous_file_.c_str());
-    LeaseFileType lf_copy(copy_file_.c_str());
-    LeaseFileType lf_output(output_file_.c_str());
     StorageType storage;
-    storage.clear();
 
     // If a previous file exists read the entries into storage
+    LeaseFileType lf_prev(getPreviousFile());
     if (lf_prev.exists()) {
         LeaseFileLoader::load<LeaseObjectType>(lf_prev, storage,
                                                MAX_LEASE_ERRORS);
     }
 
     // Follow that with the copy of the current lease file
+    LeaseFileType lf_copy(getCopyFile());
     if (lf_copy.exists()) {
         LeaseFileLoader::load<LeaseObjectType>(lf_copy, storage,
                                                MAX_LEASE_ERRORS);
     }
 
     // Write the result out to the output file
+    LeaseFileType lf_output(getOutputFile());
     LeaseFileLoader::write<LeaseObjectType>(lf_output, storage);
 
     // Once we've finished the output file move it to the complete file
-    if (rename(output_file_.c_str(), finish_file_.c_str()) != 0)
+    if (rename(getOutputFile().c_str(), getFinishFile().c_str()) != 0) {
         isc_throw(RunTimeFail, "Unable to move output (" << output_file_
                   << ") to complete (" << finish_file_
                   << ") error: " << strerror(errno));
+    }
 }
 
 void
-LFCController::fileCleanup() const {
+LFCController::fileRotate() const {
     // Remove the old previous file
-    if ((remove(previous_file_.c_str()) != 0) &&
+    if ((remove(getPreviousFile().c_str()) != 0) &&
         (errno != ENOENT)) {
         isc_throw(RunTimeFail, "Unable to delete previous file '"
                   << previous_file_ << "' error: " << strerror(errno));
     }
 
     // Remove the copy file
-    if ((remove(copy_file_.c_str()) != 0) &&
+    if ((remove(getCopyFile().c_str()) != 0) &&
         (errno != ENOENT)) {
         isc_throw(RunTimeFail, "Unable to delete copy file '"
                   << copy_file_ << "' error: " << strerror(errno));
     }
 
     // Rename the finish file to be the previous file
-    if (rename(finish_file_.c_str(), previous_file_.c_str()) != 0)
+    if (rename(finish_file_.c_str(), previous_file_.c_str()) != 0) {
         isc_throw(RunTimeFail, "Unable to move finish (" << finish_file_
                   << ") to previous (" << previous_file_
                   << ") error: " << strerror(errno));
+    }
 }
 }; // namespace isc::lfc
 }; // namespace isc

src/bin/lfc/lfc_controller.h

@@ -28,7 +28,8 @@ public:
         isc::Exception(file, line, what) { };
 };
 
-/// @brief Exceptions thrown when the processing fails
+/// @brief Exceptions thrown when a method is unable to manipulate
+/// (remove or rename) a file.
 class RunTimeFail : public isc::Exception {
 public:
     RunTimeFail(const char* file, size_t line, const char* what) :
@@ -58,7 +59,10 @@ public:
     ~LFCController();
 
     /// @brief Acts as the primary entry point to start execution
-    /// of the process.  Provides the control logic:
+    /// of the process.  Provides the control logic to combine
+    /// two lease files and weed out duplicate and expired leases.
+    /// A description of the design can be found at
+    /// http://kea.isc.org/wiki/LFCDesign#no1
     ///
     /// -# parse command line arguments
     /// -# verify that it is the only instance
@@ -85,17 +89,13 @@ public:
     /// @throw InvalidUsage if the command line parameters are invalid.
     void parseArgs(int argc, char* argv[]);
 
-    /// @brief Prints the program usage text to std error.
-    ///
-    /// @param text is a string message which will preceded the usage text.
-    /// This is intended to be used for specific usage violation messages.
-    void usage(const std::string& text);
-
-    /// @brief Gets the Kea version number for printing
+    /// @brief Rotate files.  After we have a finish file, either from
+    /// doing the cleanup or because a previous instance was interrupted,
+    /// delete the work files (previous & copy) and move the finish file
+    /// to be the new previous file.
     ///
-    /// @param extended is a boolean indicating if the version string
-    /// should be short (false) or extended (true)
-    std::string getVersion(const bool extended) const;
+    /// @throw RunTimeFail if we can't manipulate the files.
+    void fileRotate() const;
 
     /// @name Accessor methods mainly used for testing purposes
     //@{
@@ -150,22 +150,6 @@ public:
     std::string getPidFile() const {
         return (pid_file_);
     }
-
-    /// @brief Process files.  Read in the leases from any previous & copy
-    /// files we have and write the results out to the output file.  Upon
-    /// completion of the write move the file to the finish file.
-    ///
-    /// @throw RunTimeFail if we can't move the file.
-    template<typename LeaseObjectType, typename LeaseFileType, typename StorageType>
-    void processLeases() const;
-
-    /// @brief Cleanup files.  After we have a finish file, either from
-    /// doing the cleanup or because a previous instance was interrupted,
-    /// delete the work files (previous & copy) and move the finish file
-    /// to be the new previous file.
-    ///
-    /// @throw RunTimeFail if we can't manipulate the files.
-    void fileCleanup() const;
     //@}
 
 private:
@@ -179,6 +163,30 @@ private:
     std::string output_file_;   ///< The path to the output file
     std::string finish_file_;   ///< The path to the finished output file
     std::string pid_file_;      ///< The path to the pid file
+
+    /// @brief Prints the program usage text to std error.
+    ///
+    /// @param text is a string message which will preceded the usage text.
+    /// This is intended to be used for specific usage violation messages.
+    void usage(const std::string& text);
+
+    /// @brief Gets the Kea version number for printing
+    ///
+    /// @param extended is a boolean indicating if the version string
+    /// should be short (false) or extended (true)
+    std::string getVersion(const bool extended) const;
+
+    /// @brief Process files.  Read in the leases from any previous & copy
+    /// files we have and write the results out to the output file.  Upon
+    /// completion of the write move the file to the finish file.
+    ///
+    /// @tparam LeaseObjectType A @c Lease4 or @c Lease6.
+    /// @tparam LeaseFileType A @c CSVLeaseFile4 or @c CSVLeaseFile6.
+    /// @tparam StorageType A @c Lease4Storage or @c Lease6Storage.
+    ///
+    /// @throw RunTimeFail if we can't move the file.
+    template<typename LeaseObjectType, typename LeaseFileType, typename StorageType>
+    void processLeases() const;
 };
 
 }; // namespace isc::lfc

src/bin/lfc/tests/lfc_controller_unittests.cc

@@ -39,7 +39,49 @@ public:
     void writeFile(const std::string& filename, const std::string& contents) const;
 
     /// @brief Read a string from a file
-    std::string readFile(const std::string& contents) const;
+    std::string readFile(const std::string& filename) const;
+
+    /// @brief Test if a file doesn't exist
+    ///
+    /// @returns true if the file doesn't exist, false if it does
+    bool noExist(const std::string& filename) const {
+        if ((remove(filename.c_str()) != 0) && (errno == ENOENT)) {
+            return (true);
+        }
+        return (false);
+    }
+
+    /// @brief Test if any of the temporary (copy, output or finish)
+    /// files exist
+    ///
+    /// @returns true if no files exist, and false if any do.
+    bool noExistIOF() const {
+        if (noExist(istr_) && noExist(ostr_) && noExist(fstr_)) {
+            return (true);
+        }
+        return (false);
+    }
+
+    /// @brief Test if any of the temporary (copy, output or finish)
+    /// files and the pid file exist
+    ///
+    /// @returns true if no files exist, and false if any do.
+    bool noExistIOFP() const {
+        if (noExist(istr_) && noExist(ostr_) &&
+            noExist(fstr_) && noExist(pstr_)) {
+            return (true);
+        }
+        return (false);
+    }
+
+    /// @brief Remove any files we may have created
+    void removeTestFile() const {
+        remove(pstr_.c_str());
+        remove(xstr_.c_str());
+        remove(istr_.c_str());
+        remove(ostr_.c_str());
+        remove(fstr_.c_str());
+    }
 
 protected:
     /// @brief Sets up the file names and header string and removes 
@@ -73,15 +115,6 @@ protected:
     }
 
 private:
-    /// @brief Removes any remaining test files
-    void removeTestFile() const {
-        remove(pstr_.c_str());
-        remove(xstr_.c_str());
-        remove(istr_.c_str());
-        remove(ostr_.c_str());
-        remove(fstr_.c_str());
-    }
-
 };
 
 std::string
@@ -246,9 +279,9 @@ TEST_F(LFCControllerTest, someBadData) {
     EXPECT_THROW(lfc_controller.parseArgs(argc, argv), InvalidUsage);
 }
 
-/// @brief Verify that we do file_cleanup correctly.  We create different
+/// @brief Verify that we do file rotation correctly.  We create different
 /// files and see if we properly delete and move them.
-TEST_F(LFCControllerTest, fileCleanup) {
+TEST_F(LFCControllerTest, fileRotate) {
     LFCController lfc_controller, lfc_controller_launch;
 
     // We can use the same arguments and controller for all of the tests
@@ -276,66 +309,60 @@ TEST_F(LFCControllerTest, fileCleanup) {
 
     // Test 1: Start with no files - we expect an execption as there
     // is no file to copy.
-    EXPECT_THROW(lfc_controller.fileCleanup(), RunTimeFail);
-
+    EXPECT_THROW(lfc_controller.fileRotate(), RunTimeFail);
+    removeTestFile();
 
     // Test 2: Create a file for each of previous, copy and finish.  We should
     // delete the previous and copy files then move finish to previous.
-    writeFile(xstr_.c_str(), "1");
-    writeFile(istr_.c_str(), "2");
-    writeFile(fstr_.c_str(), "3");
+    writeFile(xstr_, "1");
+    writeFile(istr_, "2");
+    writeFile(fstr_, "3");
 
-    lfc_controller.fileCleanup();
+    lfc_controller.fileRotate();
 
-    // verify finish is now previous and copy and finish are gone
-    EXPECT_EQ(readFile(xstr_.c_str()), "3");
-    EXPECT_TRUE((remove(istr_.c_str()) != 0) && (errno == ENOENT));
-    EXPECT_TRUE((remove(fstr_.c_str()) != 0) && (errno == ENOENT));
-    remove(xstr_.c_str());
+    // verify finish is now previous and no temp files remain.
+    EXPECT_EQ(readFile(xstr_), "3");
+    EXPECT_TRUE(noExistIOF());
+    removeTestFile();
 
 
     // Test 3: Create a file for previous and finish but not copy.
-    writeFile(xstr_.c_str(), "4");
-    writeFile(fstr_.c_str(), "6");
+    writeFile(xstr_, "4");
+    writeFile(fstr_, "6");
 
-    lfc_controller.fileCleanup();
+    lfc_controller.fileRotate();
 
-    // verify finish is now previous and copy and finish are gone
-    EXPECT_EQ(readFile(xstr_.c_str()), "6");
-    EXPECT_TRUE((remove(istr_.c_str()) != 0) && (errno == ENOENT));
-    EXPECT_TRUE((remove(fstr_.c_str()) != 0) && (errno == ENOENT));
-    remove(xstr_.c_str());
+    // verify finish is now previous and no temp files remain.
+    EXPECT_EQ(readFile(xstr_), "6");
+    EXPECT_TRUE(noExistIOF());
+    removeTestFile();
 
 
     // Test 4: Create a file for copy and finish but not previous.
-    writeFile(istr_.c_str(), "8");
-    writeFile(fstr_.c_str(), "9");
+    writeFile(istr_, "8");
+    writeFile(fstr_, "9");
 
-    lfc_controller.fileCleanup();
+    lfc_controller.fileRotate();
 
-    // verify finish is now previous and copy and finish are gone
-    EXPECT_EQ(readFile(xstr_.c_str()), "9");
-    EXPECT_TRUE((remove(istr_.c_str()) != 0) && (errno == ENOENT));
-    EXPECT_TRUE((remove(fstr_.c_str()) != 0) && (errno == ENOENT));
-    remove(xstr_.c_str());
+    // verify finish is now previous and no temp files remain.
+    EXPECT_EQ(readFile(xstr_), "9");
+    EXPECT_TRUE(noExistIOF());
+    removeTestFile();
 
 
     // Test 5: rerun test 2 but using launch instead of cleanup
     // as we already have a finish file we shouldn't do any extra
     // processing
-    writeFile(xstr_.c_str(), "10");
-    writeFile(istr_.c_str(), "11");
-    writeFile(fstr_.c_str(), "12");
+    writeFile(xstr_, "10");
+    writeFile(istr_, "11");
+    writeFile(fstr_, "12");
 
     lfc_controller_launch.launch(argc, argv);
 
-    // verify finish is now previous and copy and finish are gone
-    // as we ran launch we also check to see if the pid is gone.
-    EXPECT_EQ(readFile(xstr_.c_str()), "12");
-    EXPECT_TRUE((remove(istr_.c_str()) != 0) && (errno == ENOENT));
-    EXPECT_TRUE((remove(fstr_.c_str()) != 0) && (errno == ENOENT));
-    EXPECT_TRUE((remove(pstr_.c_str()) != 0) && (errno == ENOENT));
-    remove(xstr_.c_str());
+    // verify finish is now previous and no temp files or pid remain.
+    EXPECT_EQ(readFile(xstr_), "12");
+    EXPECT_TRUE(noExistIOFP());
+    removeTestFile();
 }
 
 /// @brief Verify that we properly combine and clean up files
@@ -376,51 +403,53 @@ TEST_F(LFCControllerTest, launch4) {
     // Create the various strings we want to use, the header is predefined.
     // We have several entries for different leases, the naming is:
     // <lease letter>_<version#>
-    string A_1 = "192.0.2.1,06:07:08:09:0a:bc,,"
+    string a_1 = "192.0.2.1,06:07:08:09:0a:bc,,"
                  "200,200,8,1,1,host.example.com\n";
-    string A_2 = "192.0.2.1,06:07:08:09:0a:bc,,"
+    string a_2 = "192.0.2.1,06:07:08:09:0a:bc,,"
                  "200,500,8,1,1,host.example.com\n";
-    string A_3 = "192.0.2.1,06:07:08:09:0a:bc,,"
+    string a_3 = "192.0.2.1,06:07:08:09:0a:bc,,"
                  "200,800,8,1,1,host.example.com\n";
 
-    string B_1 = "192.0.3.15,dd:de:ba:0d:1b:2e:3e:4f,0a:00:01:04,"
+    string b_1 = "192.0.3.15,dd:de:ba:0d:1b:2e:3e:4f,0a:00:01:04,"
                  "100,100,7,0,0,\n";
-    string B_2 = "192.0.3.15,dd:de:ba:0d:1b:2e:3e:4f,0a:00:01:04,"
+    string b_2 = "192.0.3.15,dd:de:ba:0d:1b:2e:3e:4f,0a:00:01:04,"
                  "100,135,7,0,0,\n";
-    string B_3 = "192.0.3.15,dd:de:ba:0d:1b:2e:3e:4f,0a:00:01:04,"
+    string b_3 = "192.0.3.15,dd:de:ba:0d:1b:2e:3e:4f,0a:00:01:04,"
                  "100,150,7,0,0,\n";
 
-    string C_1 = "192.0.2.3,,a:11:01:04,"
+    string c_1 = "192.0.2.3,,a:11:01:04,"
                  "200,200,8,1,1,host.example.com\n";
 
-    string D_1 = "192.0.2.5,16:17:18:19:1a:bc,,"
+    string d_1 = "192.0.2.5,16:17:18:19:1a:bc,,"
                  "200,200,8,1,1,host.example.com\n";
-    string D_2 = "192.0.2.5,16:17:18:19:1a:bc,,"
+    string d_2 = "192.0.2.5,16:17:18:19:1a:bc,,"
                  "0,200,8,1,1,host.example.com\n";
 
     // Subtest 1: both previous and copy available.
     // Create the test previous file
-    test_str = v4_hdr_ + A_1 + B_1 + C_1 + B_2 + A_2 + D_1;
-    writeFile(xstr_.c_str(), test_str);
+    test_str = v4_hdr_ + a_1 + b_1 + c_1 + b_2 + a_2 + d_1;
+    writeFile(xstr_, test_str);
 
     // Create the test copy file
-    test_str = v4_hdr_ + A_3 + B_3 + D_2;
-    writeFile(istr_.c_str(), test_str);
+    test_str = v4_hdr_ + a_3 + b_3 + d_2;
+    writeFile(istr_, test_str);
 
     // Run the cleanup
     lfc_controller.launch(argc, argv);
 
     // Compare the results, we expect the last lease for each ip
-    // except for C which was invalid and D which has expired
-    test_str = v4_hdr_ + A_3 + B_3;
-    EXPECT_EQ(readFile(xstr_.c_str()), test_str);
-    remove(xstr_.c_str());
+    // except for C which was invalid and D which has expired.
+    // We also verify none of the temp or pid files remain.
+    test_str = v4_hdr_ + a_3 + b_3;
+    EXPECT_EQ(readFile(xstr_), test_str);
+    EXPECT_TRUE(noExistIOFP());
+    removeTestFile();
 
 
     // Subtest 2: only previous available
     // Create the test previous file
-    test_str = v4_hdr_ + A_1 + B_1 + C_1 + B_2 + A_2 + D_1;
-    writeFile(xstr_.c_str(), test_str);
+    test_str = v4_hdr_ + a_1 + b_1 + c_1 + b_2 + a_2 + d_1;
+    writeFile(xstr_, test_str);
 
     // No copy file
 
@@ -428,27 +457,31 @@ TEST_F(LFCControllerTest, launch4) {
     lfc_controller.launch(argc, argv);
 
     // Compare the results, we expect the last lease for each ip
-    // except for C which was invalid and D which has expired
-    test_str = v4_hdr_ + A_2 + D_1 + B_2;
-    EXPECT_EQ(readFile(xstr_.c_str()), test_str);
-    remove(xstr_.c_str());
+    // except for C which was invalid and D which has expired.
+    // We also verify none of the temp or pid files remain.
+    test_str = v4_hdr_ + a_2 + d_1 + b_2;
+    EXPECT_EQ(readFile(xstr_), test_str);
+    EXPECT_TRUE(noExistIOFP());
+    removeTestFile();
 
 
     // Subtest 3: only copy available
     // No previous file
 
     // Create the test copy file
-    test_str = v4_hdr_ + D_1 + A_1 + B_1 + B_3 + D_2 + A_3;
-    writeFile(istr_.c_str(), test_str);
+    test_str = v4_hdr_ + d_1 + a_1 + b_1 + b_3 + d_2 + a_3;
+    writeFile(istr_, test_str);
 
     // Run the cleanup
     lfc_controller.launch(argc, argv);
 
     // Compare the results, we expect the last lease for each ip
-    // except for C which was invalid and D which has expired
-    test_str = v4_hdr_ + A_3 + B_3;
-    EXPECT_EQ(readFile(xstr_.c_str()), test_str);
-    remove(xstr_.c_str());
+    // except for C which was invalid and D which has expired.
+    // We also verify none of the temp or pid files remain.
+    test_str = v4_hdr_ + a_3 + b_3;
+    EXPECT_EQ(readFile(xstr_), test_str);
+    EXPECT_TRUE(noExistIOFP());
+    removeTestFile();
 
 
     // Subtest 4: neither available
@@ -459,10 +492,12 @@ TEST_F(LFCControllerTest, launch4) {
     // Run the cleanup
     lfc_controller.launch(argc, argv);
 
-    // Compare the results, we expect a header and no leaes
+    // Compare the results, we expect a header and no leaes.
+    // We also verify none of the temp or pid files remain.
     test_str = v4_hdr_;
-    EXPECT_EQ(readFile(xstr_.c_str()), test_str);
-    remove(xstr_.c_str());
+    EXPECT_EQ(readFile(xstr_), test_str);
+    EXPECT_TRUE(noExistIOFP());
+    removeTestFile();
 
 
     // Subtest 5: a file with a lot of errors
@@ -470,7 +505,7 @@ TEST_F(LFCControllerTest, launch4) {
     astr = "1,\n2,\n3,\n4,\n5,\n6,\n7,\n7,\n8,\n9,\n10,\n";
     test_str = v4_hdr_ + astr + astr + astr + astr + astr +
                astr + astr + astr + astr + astr + astr;
-    writeFile(xstr_.c_str(), test_str);
+    writeFile(xstr_, test_str);
 
     // No copy file
 
@@ -478,10 +513,10 @@ TEST_F(LFCControllerTest, launch4) {
     // catch the error and properly cleanup.
     lfc_controller.launch(argc, argv);
 
-    // And we shouldn't have deleted the previous file, but should
-    // have deleted the pid file
-    EXPECT_EQ(readFile(xstr_.c_str()), test_str);
-    EXPECT_TRUE((remove(pstr_.c_str()) != 0) && (errno == ENOENT));
+    // And we shouldn't have deleted the previous file.
+    // We also verify none of the temp or pid files remain.
+    EXPECT_EQ(readFile(xstr_), test_str);
+    EXPECT_TRUE(noExistIOFP());
 }
 
 /// @brief Verify that we properly combine and clean up files
@@ -522,80 +557,84 @@ TEST_F(LFCControllerTest, launch6) {
     // Create the various strings we want to use, the header is predefined.
     // We have several entries for different leases, the naming is:
     // <lease letter>_<version#>.
-    string A_1 = "2001:db8:1::1,00:01:02:03:04:05:06:0a:0b:0c:0d:0e:0f,"
+    string a_1 = "2001:db8:1::1,00:01:02:03:04:05:06:0a:0b:0c:0d:0e:0f,"
                  "200,200,8,100,0,7,0,1,1,host.example.com,\n";
-    string A_2 = "2001:db8:1::1,,200,200,8,100,0,7,0,1,1,host.example.com,\n";
-    string A_3 = "2001:db8:1::1,00:01:02:03:04:05:06:0a:0b:0c:0d:0e:0f,"
+    string a_2 = "2001:db8:1::1,,200,200,8,100,0,7,0,1,1,host.example.com,\n";
+    string a_3 = "2001:db8:1::1,00:01:02:03:04:05:06:0a:0b:0c:0d:0e:0f,"
                  "200,400,8,100,0,7,0,1,1,host.example.com,\n";
-    string A_4 = "2001:db8:1::1,00:01:02:03:04:05:06:0a:0b:0c:0d:0e:0f,"
+    string a_4 = "2001:db8:1::1,00:01:02:03:04:05:06:0a:0b:0c:0d:0e:0f,"
                  "0,200,8,100,0,7,0,1,1,host.example.com,\n";
 
-    string B_1 = "2001:db8:2::10,01:01:01:01:0a:01:02:03:04:05,"
+    string b_1 = "2001:db8:2::10,01:01:01:01:0a:01:02:03:04:05,"
                  "300,300,6,150,0,8,0,0,0,,\n";
-    string B_2 = "2001:db8:2::10,01:01:01:01:0a:01:02:03:04:05,"
+    string b_2 = "2001:db8:2::10,01:01:01:01:0a:01:02:03:04:05,"
                  "300,800,6,150,0,8,0,0,0,,\n";
-    string B_3 = "2001:db8:2::10,01:01:01:01:0a:01:02:03:04:05,"
+    string b_3 = "2001:db8:2::10,01:01:01:01:0a:01:02:03:04:05,"
                  "300,1000,6,150,0,8,0,0,0,,\n";
 
-    string C_1 = "3000:1::,00:01:02:03:04:05:06:0a:0b:0c:0d:0e:0f,100,200,8,0,2,"
+    string c_1 = "3000:1::,00:01:02:03:04:05:06:0a:0b:0c:0d:0e:0f,100,200,8,0,2,"
                  "16,64,0,0,,\n";
-    string C_2 = "3000:1::,00:01:02:03:04:05:06:0a:0b:0c:0d:0e:0f,100,400,8,0,2,"
+    string c_2 = "3000:1::,00:01:02:03:04:05:06:0a:0b:0c:0d:0e:0f,100,400,8,0,2,"
                  "16,64,0,0,,\n";
 
-    string D_1 = "2001:db8:1::3,00:01:02:03:04:05:06:0a:0b:0c:0d:0e:0f,"
+    string d_1 = "2001:db8:1::3,00:01:02:03:04:05:06:0a:0b:0c:0d:0e:0f,"
                  "200,600,8,100,0,7,0,1,1,host.example.com,\n";
 
     // Subtest 1: bot previous and copy available
     // Create the test previous file
-    test_str = v6_hdr_ + A_1 + B_1 + A_2 + C_1 + A_3 + B_2;
-    writeFile(xstr_.c_str(), test_str);
+    test_str = v6_hdr_ + a_1 + b_1 + a_2 + c_1 + a_3 + b_2;
+    writeFile(xstr_, test_str);
 
     // Create the test copy file
-    test_str = v6_hdr_ + B_3 + A_4 + D_1 + C_2;
-    writeFile(istr_.c_str(), test_str);
+    test_str = v6_hdr_ + b_3 + a_4 + d_1 + c_2;
+    writeFile(istr_, test_str);
 
     // Run the cleanup
     lfc_controller.launch(argc, argv);
 
     // Compare the results, we expect the last lease for each ip
-    // except for A which has expired
-    test_str = v6_hdr_ + D_1 + B_3 + C_2;
-    EXPECT_EQ(readFile(xstr_.c_str()), test_str);
-    remove(xstr_.c_str());
+    // except for A which has expired.
+    // We also verify none of the temp or pid files remain.
+    test_str = v6_hdr_ + d_1 + b_3 + c_2;
+    EXPECT_EQ(readFile(xstr_), test_str);
+    EXPECT_TRUE(noExistIOFP());
+    removeTestFile();
 
 
     // Subtest 2: only previous available
     // Create the test previous file
-    test_str = v6_hdr_ + A_1 + B_1 + A_2 + C_1 + A_3 + B_2;
-    writeFile(xstr_.c_str(), test_str);
+    test_str = v6_hdr_ + a_1 + b_1 + a_2 + c_1 + a_3 + b_2;
+    writeFile(xstr_, test_str);
 
     // No copy file
 
     // Run the cleanup
     lfc_controller.launch(argc, argv);
 
-    // Compare the results, we expect the last lease for each ip
-    // except for A which has expired
-    test_str = v6_hdr_ + A_3 + B_2 + C_1;
-    EXPECT_EQ(readFile(xstr_.c_str()), test_str);
-    remove(xstr_.c_str());
+    // Compare the results, we expect the last lease for each ip.
+    // We also verify none of the temp or pid files remain.
+    test_str = v6_hdr_ + a_3 + b_2 + c_1;
+    EXPECT_EQ(readFile(xstr_), test_str);
+    EXPECT_TRUE(noExistIOFP());
+    removeTestFile();
 
 
     // Subtest 3: only copy available
     // No previous file
 
     // Create the test copy file
-    test_str = v6_hdr_ + A_1 + B_2 + B_3 + A_4 + D_1 + C_2;
-    writeFile(istr_.c_str(), test_str);
+    test_str = v6_hdr_ + a_1 + b_2 + b_3 + a_4 + d_1 + c_2;
+    writeFile(istr_, test_str);
 
     // Run the cleanup
     lfc_controller.launch(argc, argv);
 
-    // Compare the results, we expect the last lease for each ip
-    // except for A which has expired
-    test_str = v6_hdr_ + D_1 + B_3 + C_2;
-    EXPECT_EQ(readFile(xstr_.c_str()), test_str);
-    remove(xstr_.c_str());
+    // Compare the results, we expect the last lease for each ip.
+    // We also verify none of the temp or pid files remain.
+    test_str = v6_hdr_ + d_1 + b_3 + c_2;
+    EXPECT_EQ(readFile(xstr_), test_str);
+    EXPECT_TRUE(noExistIOFP());
+    removeTestFile();
 
 
     // Subtest 4: neither available
@@ -606,18 +645,20 @@ TEST_F(LFCControllerTest, launch6) {
     // Run the cleanup
     lfc_controller.launch(argc, argv);
 
-    // Compare the results, we expect a header and no leases
+    // Compare the results, we expect a header and no leases.
+    // We also verify none of the temp or pid files remain.
     test_str = v6_hdr_;
-    EXPECT_EQ(readFile(xstr_.c_str()), test_str);
-    remove(xstr_.c_str());
+    EXPECT_EQ(readFile(xstr_), test_str);
+    EXPECT_TRUE(noExistIOFP());
+    removeTestFile();
 
 
     // Subtest 5: a file with a lot of errors
-    // A previous file with a lot of errors
+    // A previous file with a lot of errors.
     astr = "1,\n2,\n3,\n4,\n5,\n6,\n7,\n7,\n8,\n9,\n10,\n";
     test_str = v6_hdr_ + astr + astr + astr + astr + astr +
                astr + astr + astr + astr + astr + astr;
-    writeFile(xstr_.c_str(), test_str);
+    writeFile(xstr_, test_str);
 
     // No copy file
 
@@ -625,11 +666,10 @@ TEST_F(LFCControllerTest, launch6) {
     // catch the error and properly cleanup.
     lfc_controller.launch(argc, argv);
 
-    // And we shouldn't have deleted the previous file, but should
-    // have deleted the pid file
-    EXPECT_EQ(readFile(xstr_.c_str()), test_str);
-    EXPECT_TRUE((remove(pstr_.c_str()) != 0) && (errno == ENOENT));
-
+    // And we shouldn't have deleted the previous file.
+    // We also verify none of the temp or pid files remain.
+    EXPECT_EQ(readFile(xstr_), test_str);
+    EXPECT_TRUE(noExistIOFP());
 }
 
 } // end of anonymous namespace

src/lib/dhcpsrv/lease_file_loader.h

@@ -166,10 +166,18 @@ public:
     /// This method iterates over the @c Lease4 or @c Lease6 object in the
     /// storage specified in the arguments and writes them to the file
     /// specified in the arguments.
-    /// 
+    ///
     /// This method writes all entries in the storage to the file, it does
     /// not perform any checks for expiration or duplication.
     ///
+    /// The order in which the entries will be written to the file depends
+    /// on the first index in the multi-index container.  Currently that
+    /// is the v4 or v6 IP address and they are written from lowest to highest.
+    ///
+    /// Before writing the method will close the file if it is open
+    /// and reopen it for writing.  After completion it will close
+    /// the file.
+    ///
     /// @param lease_file A reference to the @c CSVLeaseFile4 or
     /// @c CSVLeaseFile6 object representing the lease file. The file
     /// doesn't need to be open because the method re-opens the file.
@@ -192,7 +200,13 @@ public:
         for (typename StorageType::const_iterator lease = storage.begin();
              lease != storage.end();
              ++lease) {
-            lease_file.append(**lease);
+            try {
+                lease_file.append(**lease);
+            } catch (const isc::Exception& ex) {
+            // Close the file
+            lease_file.close();
+            throw;
+            }
         }
 
         // Close the file

src/lib/dhcpsrv/tests/lease_file_loader_unittest.cc

@@ -134,7 +134,7 @@ LeaseFileLoaderTest::absolutePath(const std::string& filename) {
 //
 // It also tests the write function by writing the storage to a file
 // and comparing that with the expected value.
-TEST_F(LeaseFileLoaderTest, load4) {
+TEST_F(LeaseFileLoaderTest, loadWrite4) {
     // Create lease file with leases for 192.0.2.1, 192.0.3.15. The lease
     // entry for the 192.0.2.3 is invalid (lacks HW address) and should
     // be discarded.
@@ -194,7 +194,7 @@ TEST_F(LeaseFileLoaderTest, load4) {
 //
 // It also tests the write function by writing the storage to a file
 // and comparing that with the expected value.
-TEST_F(LeaseFileLoaderTest, load4LeaseRemove) {
+TEST_F(LeaseFileLoaderTest, loadWrite4LeaseRemove) {
     // Create lease file in which one of the entries for 192.0.2.1
     // has a valid_lifetime of 0 and results in the deletion of the
     // lease.
@@ -237,7 +237,7 @@ TEST_F(LeaseFileLoaderTest, load4LeaseRemove) {
 //
 // It also tests the write function by writing the storage to a file
 // and comparing that with the expected value.
-TEST_F(LeaseFileLoaderTest, load6) {
+TEST_F(LeaseFileLoaderTest, loadWrite6) {
     // Create a lease file with three valid leases: 2001:db8:1::1,
     // 3000:1:: and 2001:db8:2::10.
     io_.writeFile("address,duid,valid_lifetime,expire,subnet_id,"
@@ -283,7 +283,6 @@ TEST_F(LeaseFileLoaderTest, load6) {
     ASSERT_TRUE(lease);
     EXPECT_EQ(500, lease->cltt_);
 
-
     writeLeases<Lease6, CSVLeaseFile6, Lease6Storage>
         (*lf, storage,
          "address,duid,valid_lifetime,expire,subnet_id,"
@@ -303,7 +302,7 @@ TEST_F(LeaseFileLoaderTest, load6) {
 //
 // It also tests the write function by writing the storage to a file
 // and comparing that with the expected value.
-TEST_F(LeaseFileLoaderTest, load6LeaseRemove) {
+TEST_F(LeaseFileLoaderTest, loadWrite6LeaseRemove) {
     // Create lease file in which one of the entries for the 2001:db8:1::1
     // has valid lifetime set to 0, in which case the lease should be
     // deleted.
@@ -395,7 +394,7 @@ TEST_F(LeaseFileLoaderTest, loadMaxErrors) {
 //
 // It also tests the write function by writing the storage to a file
 // and comparing that with the expected value.
-TEST_F(LeaseFileLoaderTest, loadLeaseWithZeroLifetime) {
+TEST_F(LeaseFileLoaderTest, loadWriteLeaseWithZeroLifetime) {
     // Create lease file. The second lease has a valid lifetime of 0.
     io_.writeFile("address,hwaddr,client_id,valid_lifetime,expire,subnet_id,"
                   "fqdn_fwd,fqdn_rev,hostname\n"
@@ -423,7 +422,6 @@ TEST_F(LeaseFileLoaderTest, loadLeaseWithZeroLifetime) {
          "address,hwaddr,client_id,valid_lifetime,expire,subnet_id,"
          "fqdn_fwd,fqdn_rev,hostname\n"
          "192.0.2.1,06:07:08:09:0a:bc,,200,200,8,1,1,\n");
-
 }