[trac3665] More tests and cleanup

Add more tests to verity the cleanup of leases files

Cleanup the code for comments, typos, spaces and the like.

src/bin/lfc/lfc_controller.cc

@@ -22,8 +22,6 @@
 #include <dhcpsrv/lease_file_loader.h>
 #include <config.h>
 
-#include <boost/shared_ptr.hpp>
-
 #include <iostream>
 #include <sstream>
 #include <unistd.h>
@@ -44,7 +42,7 @@ 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 read the leases from the lease file.
+/// @brief Maximum number of errors to allow when reading leases from the file.
 const uint32_t MAX_LEASE_ERRORS = 100;
 
 LFCController::LFCController()
@@ -57,6 +55,8 @@ LFCController::~LFCController() {
 
 void
 LFCController::launch(int argc, char* argv[]) {
+    bool do_clean = true;
+
     try {
         parseArgs(argc, argv);
     } catch (const InvalidUsage& ex) {
@@ -64,7 +64,8 @@ LFCController::launch(int argc, char* argv[]) {
         throw;  // rethrow it
     }
 
-    std::cerr << "Starting lease file cleanup" << std::endl;
+    if (verbose_ == true)
+        std::cerr << "Starting lease file cleanup" << std::endl;
 
     // verify we are the only instance
     PIDFile pid_file(pid_file_);
@@ -88,26 +89,36 @@ LFCController::launch(int argc, char* argv[]) {
         return;
     }
 
-    // do other work (TBD)
     // If we don't have a finish file do the processing
     if (access(finish_file_.c_str(), F_OK) == -1) {
-        std::cerr << "LFC Processing files" << std::endl;
-
-	if (protocol_version_ == 4) {
-	    processLeases<Lease4, CSVLeaseFile4, Lease4Storage>();
-	} else {
-	    processLeases<Lease6, CSVLeaseFile6, Lease6Storage>();
-	}
+        if (verbose_ == true)
+            std::cerr << "LFC Processing files" << std::endl;
+
+        try {
+            if (protocol_version_ == 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;
+            std::cerr << "Processing failed: " << proc_ex.what() << std::endl;
+        }
     }
 
-    // We either already had a finish file or just created one, do the
-    // file cleanup, we don't want to return after the catch as we
+    // If do_clean 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
-    try {
-        std::cerr << "LFC cleaning files" << std::endl;
-        fileCleanup();
-    } catch (const RunTimeFail& run_ex) {
-        std::cerr << run_ex.what() << std::endl;
+    if (do_clean == true) {
+        if (verbose_ == true)
+            std::cerr << "LFC cleaning files" << std::endl;
+        try {
+            fileCleanup();
+        } catch (const RunTimeFail& run_ex) {
+            std::cerr << run_ex.what() << std::endl;
+        }
     }
 
     // delete the pid file for this instance
@@ -115,10 +126,10 @@ LFCController::launch(int argc, char* argv[]) {
         pid_file.deleteFile();
     } catch (const PIDFileError& pid_ex) {
         std::cerr << pid_ex.what() << std::endl;
-        return;
     }
 
-    std::cerr << "LFC complete" << std::endl;
+    if (verbose_ == true)
+        std::cerr << "LFC complete" << std::endl;
 }
 
 void
@@ -256,13 +267,14 @@ LFCController::parseArgs(int argc, char* argv[]) {
 
     // If verbose is set echo the input information
     if (verbose_ == true) {
-      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
-                << "Output lease file:         " << output_file_ << std::endl
-                << "Finish file:               " << finish_file_ << std::endl
-                << "Config file:               " << config_file_ << std::endl
-                << "PID file:                  " << pid_file_ << std::endl;
+        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
+                  << "Output lease file:         " << output_file_ << std::endl
+                  << "Finish file:               " << finish_file_ << std::endl
+                  << "Config file:               " << config_file_ << std::endl
+                  << "PID file:                  " << pid_file_ << std::endl
+                  << std::endl;
     }
 }
 
@@ -303,26 +315,26 @@ LFCController::getVersion(const bool extended) const{
 template<typename LeaseObjectType, typename LeaseFileType, typename StorageType>
 void
 LFCController::processLeases() const {
-    LeaseFileType lfPrev(previous_file_.c_str());
-    LeaseFileType lfCopy(copy_file_.c_str());
-    LeaseFileType lfOutput(output_file_.c_str());
+    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
-    if (lfPrev.exists()) {
-        LeaseFileLoader::load<LeaseObjectType>(lfPrev, storage,
-					       MAX_LEASE_ERRORS);
+    if (lf_prev.exists()) {
+        LeaseFileLoader::load<LeaseObjectType>(lf_prev, storage,
+                                               MAX_LEASE_ERRORS);
     }
 
-    // Follow that with the copy of the current lease file 
-    if (lfCopy.exists()) {
-        LeaseFileLoader::load<LeaseObjectType>(lfCopy, storage,
-					       MAX_LEASE_ERRORS);
+    // Follow that with the copy of the current lease file
+    if (lf_copy.exists()) {
+        LeaseFileLoader::load<LeaseObjectType>(lf_copy, storage,
+                                               MAX_LEASE_ERRORS);
     }
 
     // Write the result out to the output file
-    LeaseFileLoader::write<LeaseObjectType>(lfOutput, storage);
+    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)

src/bin/lfc/lfc_controller.h

@@ -41,10 +41,6 @@ public:
 /// manage the command line, check for already running instances,
 /// invoke the code to process the lease files and finally to rename
 /// the lease files as necessary.
-///
-/// @todo The current code simply processes the command line we still need to
-/// -# invoke the code to read, process and write the lease files
-/// -# rename and delete the shell files as required
 class LFCController {
 public:
     /// @brief Defines the application name, it may be used to locate
@@ -67,10 +63,10 @@ public:
     /// -# parse command line arguments
     /// -# verify that it is the only instance
     /// -# create pid file
-    /// -# read leases files TBD
-    /// -# write lease file TBD
-    /// -# move leases files TBD
-    /// -# cleanup artifacts TBD
+    /// -# read leases files
+    /// -# write lease file
+    /// -# move leases files
+    /// -# cleanup artifacts
     /// -# remove pid file
     /// -# exit to the caller
     ///
@@ -158,6 +154,8 @@ public:
     /// @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;
 
@@ -166,7 +164,7 @@ public:
     /// delete the work files (previous & copy) and move the finish file
     /// to be the new previous file.
     ///
-    /// @throw RunTimeFail if the command line parameters are invalid.
+    /// @throw RunTimeFail if we can't manipulate the files.
     void fileCleanup() const;
     //@}
 

src/bin/lfc/tests/lfc_controller_unittests.cc

@@ -13,6 +13,7 @@
 // PERFORMANCE OF THIS SOFTWARE.
 
 #include <lfc/lfc_controller.h>
+#include <util/csv_file.h>
 #include <gtest/gtest.h>
 #include <fstream>
 #include <cerrno>
@@ -24,7 +25,6 @@ namespace {
 
 class LFCControllerTest : public ::testing::Test {
 public:
-    
     string pstr_; ///< String for name for pid file
     string xstr_; ///< String for name for previous file
     string istr_; ///< String for name for copy file
@@ -32,30 +32,36 @@ public:
     string fstr_; ///< String for name for finish file
     string cstr_; ///< String for name for config file
 
-    /// @brief Create a file and write the filename into it.
-    void touchFile(const std::string& filename, int);
+    string v4_hdr_; ///< String for the header of the v4 csv test file
+    string v6_hdr_; ///< String for the header of the v6 csv test file
 
     /// @brief Create a file and write the given string into it.
     void writeFile(const std::string& filename, const std::string& contents) const;
 
-    /// @brief Read a string from a file 
+    /// @brief Read a string from a file
     std::string readFile(const std::string& contents) const;
 
-    /// @brief check the file to see if i matches what was written to it.
-    bool checkFile(const std::string& filename, int);
-
 protected:
-    /// @brief Sets up the file names and Removes any old test
-    /// files before the test
+    /// @brief Sets up the file names and header string and removes 
+    /// any old test files before the test
     virtual void SetUp() {
         // set up the test files we need
-        string baseDir = TEST_DATA_BUILDDIR;
-	pstr_ = baseDir + "/" + "lease_file." + "pid";        // pid
-	xstr_ = baseDir + "/" + "lease_file." + "2";          // previous
-	istr_ = baseDir + "/" + "lease_file." + "1";          // copy
-	ostr_ = baseDir + "/" + "lease_file." + "output";     // output
-	fstr_ = baseDir + "/" + "lease_file." + "completed";  // finish
-	cstr_ = baseDir + "/" + "config_file";                // config
+        string base_dir = TEST_DATA_BUILDDIR;
+        string lf = "lease_file.";
+
+        pstr_ = base_dir + "/" + lf + "pid";        // pid
+        xstr_ = base_dir + "/" + lf + "2";          // previous
+        istr_ = base_dir + "/" + lf + "1";          // copy
+        ostr_ = base_dir + "/" + lf + "output";     // output
+        fstr_ = base_dir + "/" + lf + "completed";  // finish
+        cstr_ = base_dir + "/" + "config_file";     // config
+
+        v4_hdr_ = "address,hwaddr,client_id,valid_lifetime,expire,subnet_id,"
+                  "fqdn_fwd,fqdn_rev,hostname\n";
+
+        v6_hdr_ = "address,duid,valid_lifetime,expire,subnet_id,"
+                  "pref_lifetime,lease_type,iaid,prefix_len,fqdn_fwd,"
+                  "fqdn_rev,hostname,hwaddr\n";
 
         // and remove any outstanding test files
         removeTestFile();
@@ -67,7 +73,6 @@ protected:
     }
 
 private:
-
     /// @brief Removes any remaining test files
     void removeTestFile() const {
         remove(pstr_.c_str());
@@ -79,53 +84,28 @@ private:
 
 };
 
-void
-LFCControllerTest::touchFile(const std::string& filename, int i) {
-    std::ofstream fs;
-
-    fs.open(filename, std::ofstream::out);
-    fs << i << std::endl;
-    fs.close();
-    
-}
-
 std::string
 LFCControllerTest::readFile(const std::string& filename) const {
     std::ifstream fs;
 
     fs.open(filename, std::ifstream::in);
     std::string contents((std::istreambuf_iterator<char>(fs)),
-			 std::istreambuf_iterator<char>());
+                         std::istreambuf_iterator<char>());
     fs.close();
     return (contents);
 }
 
 void
 LFCControllerTest::writeFile(const std::string& filename,
-			     const std::string& contents) const {
+                             const std::string& contents) const {
     std::ofstream fs(filename, std::ofstream::out);
 
     if (fs.is_open()) {
         fs << contents;
-	fs.close();
+        fs.close();
     }
 }
 
-bool
-LFCControllerTest::checkFile(const std::string& filename, int i) {
-    std::ifstream fs;
-    int j;
-
-    fs.open(filename, std::ifstream::in);
-    fs >> j;
-    fs.close();
-
-    if (i == j)
-        return (true);
-    return (false);
-}
-
-
 /// @brief Verify initial state of LFC controller.
 /// Create an instance of the controller and see that
 /// all of the initial values are empty as expected.
@@ -301,74 +281,79 @@ TEST_F(LFCControllerTest, fileCleanup) {
 
     // 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.
-    touchFile(xstr_.c_str(), 1);
-    touchFile(istr_.c_str(), 2);
-    touchFile(fstr_.c_str(), 3);
+    writeFile(xstr_.c_str(), "1");
+    writeFile(istr_.c_str(), "2");
+    writeFile(fstr_.c_str(), "3");
 
     lfc_controller.fileCleanup();
 
     // verify finish is now previous and copy and finish are gone
-    EXPECT_TRUE(checkFile(xstr_.c_str(), 3));
+    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(pstr_.c_str());
+    remove(xstr_.c_str());
 
 
     // Test 3: Create a file for previous and finish but not copy.
-    touchFile(xstr_.c_str(), 4);
-    touchFile(fstr_.c_str(), 6);
+    writeFile(xstr_.c_str(), "4");
+    writeFile(fstr_.c_str(), "6");
 
     lfc_controller.fileCleanup();
 
     // verify finish is now previous and copy and finish are gone
-    EXPECT_TRUE(checkFile(xstr_.c_str(), 6));
+    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(pstr_.c_str());
+    remove(xstr_.c_str());
 
 
     // Test 4: Create a file for copy and finish but not previous.
-    touchFile(istr_.c_str(), 8);
-    touchFile(fstr_.c_str(), 9);
+    writeFile(istr_.c_str(), "8");
+    writeFile(fstr_.c_str(), "9");
 
     lfc_controller.fileCleanup();
 
     // verify finish is now previous and copy and finish are gone
-    EXPECT_TRUE(checkFile(xstr_.c_str(), 9));
+    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(pstr_.c_str());
+    remove(xstr_.c_str());
 
 
     // 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
-    touchFile(xstr_.c_str(), 10);
-    touchFile(istr_.c_str(), 11);
-    touchFile(fstr_.c_str(), 12);
+    writeFile(xstr_.c_str(), "10");
+    writeFile(istr_.c_str(), "11");
+    writeFile(fstr_.c_str(), "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_TRUE(checkFile(xstr_.c_str(), 12));
+    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(pstr_.c_str());
+    remove(xstr_.c_str());
 }
 
 /// @brief Verify that we properly combine and clean up files
-/// 
+///
 /// This is mostly a retest as we already test that the loader and
 /// writer functions work in their own tests but we combine it all
-/// here.  This is the v4 version
+/// here.  We check: both files available, only previous, only copy
+/// neither and one of them having many lease errors.  This is the
+/// v4 version.
 
-TEST_F(LFCControllerTest, programLaunch4) {
+TEST_F(LFCControllerTest, launch4) {
     LFCController lfc_controller;
 
-    // We can use the same arguments and controller for all of the tests
-    // as the files get redone for each subtest.
+    // The arg list for our test.  We generally use the
+    // same file names to make it easy.  We include the -d
+    // in the argv list in case we want to enable verbose
+    // for debugging purposes.  To enable it change argc from
+    // 14 to 15
     char* argv[] = { const_cast<char*>("progName"),
                      const_cast<char*>("-4"),
                      const_cast<char*>("-x"),
@@ -386,64 +371,135 @@ TEST_F(LFCControllerTest, programLaunch4) {
                      const_cast<char*>("-d")
     };
     int argc = 14;
-    lfc_controller.parseArgs(argc, argv);
+    string test_str, astr;
+
+    // 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,,"
+                 "200,200,8,1,1,host.example.com\n";
+    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,,"
+                 "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,"
+                 "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,"
+                 "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,"
+                 "100,150,7,0,0,\n";
+
+    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,,"
+                 "200,200,8,1,1,host.example.com\n";
+    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);
+
+    // Create the test copy file
+    test_str = v4_hdr_ + A_3 + B_3 + D_2;
+    writeFile(istr_.c_str(), 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());
+
 
+    // Subtest 2: only previous available
     // Create the test previous file
-    writeFile(xstr_.c_str(),
-	      "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,"
-	      "host.example.com\n"
-	      "192.0.3.15,dd:de:ba:0d:1b:2e:3e:4f,0a:00:01:04,100,100,7,"
-	      "0,0,\n"
-	      "192.0.2.3,,a:11:01:04,200,200,8,1,1,host.example.com\n"
-	      "192.0.3.15,dd:de:ba:0d:1b:2e:3e:4f,0a:00:01:04,100,135,7,"
-	      "0,0,\n"
-	      "192.0.2.1,06:07:08:09:0a:bc,,200,500,8,1,1,"
-	      "host.example.com\n"
-	      "192.0.2.5,06:07:08:09:0a:bc,,200,200,8,1,1,"
-	      "host.example.com\n");
+    test_str = v4_hdr_ + A_1 + B_1 + C_1 + B_2 + A_2 + D_1;
+    writeFile(xstr_.c_str(), 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 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());
+
+
+    // Subtest 3: only copy available
+    // No previous file
 
     // Create the test copy file
-    writeFile(istr_.c_str(),
-	      "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,800,8,1,1,"
-	      "host.example.com\n"
-	      "192.0.3.15,dd:de:ba:0d:1b:2e:3e:4f,0a:00:01:04,100,150,7,"
-	      "0,0,\n"
-	      "192.0.2.5,06:07:08:09:0a:bc,,200,0,8,1,1,"
-	      "host.example.com\n");
+    test_str = v4_hdr_ + D_1 + A_1 + B_1 + B_3 + D_2 + A_3;
+    writeFile(istr_.c_str(), test_str);
 
     // Run the cleanup
     lfc_controller.launch(argc, argv);
 
-    // Compare the results
-    EXPECT_EQ(readFile(xstr_.c_str()),
-	      "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,800,8,1,1,"
-	      "host.example.com\n"
-	      "192.0.3.15,dd:de:ba:0d:1b:2e:3e:4f,0a:00:01:04,100,150,7,"
-	      "0,0,\n");
+    // 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());
+
+
+    // Subtest 4: neither available
+    // No previous file
+
+    // No copy file
+
+    // Run the cleanup
+    lfc_controller.launch(argc, argv);
 
-	      
+    // Compare the results, we expect a header and no leaes
+    test_str = v4_hdr_;
+    EXPECT_EQ(readFile(xstr_.c_str()), test_str);
+    remove(xstr_.c_str());
 
+
+    // Subtest 5: a 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 = v4_hdr_ + astr + astr + astr + astr + astr +
+               astr + astr + astr + astr + astr + astr;
+    writeFile(xstr_.c_str(), test_str);
+
+    // No copy file
+
+    // Run the cleanup, the file should fail but we should
+    // 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));
 }
 
 /// @brief Verify that we properly combine and clean up files
-/// 
+///
 /// This is mostly a retest as we already test that the loader and
 /// writer functions work in their own tests but we combine it all
-/// here.  This is the v6 version
+/// here.  We check: both files available, only previous, only copy
+/// neither and one of them having many lease errors.  This is the
+/// v6 version.
 
-TEST_F(LFCControllerTest, programLaunch6) {
+TEST_F(LFCControllerTest, launch6) {
     LFCController lfc_controller;
 
-    // We can use the same arguments and controller for all of the tests
-    // as the files get redone for each subtest.
+    // The arg list for our test.  We generally use the
+    // same file names to make it easy.  We include the -d
+    // in the argv list in case we want to enable verbose
+    // for debugging purposes.  To enable it change argc from
+    // 14 to 15
     char* argv[] = { const_cast<char*>("progName"),
                      const_cast<char*>("-6"),
                      const_cast<char*>("-x"),
@@ -461,56 +517,119 @@ TEST_F(LFCControllerTest, programLaunch6) {
                      const_cast<char*>("-d")
     };
     int argc = 14;
-    lfc_controller.parseArgs(argc, argv);
+    string test_str, astr;
+
+    // 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,"
+                 "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,"
+                 "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,"
+                 "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,"
+                 "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,"
+                 "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,"
+                 "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,"
+                 "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,"
+                 "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,"
+                 "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);
 
+    // Create the test copy file
+    test_str = v6_hdr_ + B_3 + A_4 + D_1 + C_2;
+    writeFile(istr_.c_str(), 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());
+
+
+    // Subtest 2: only previous available
     // Create the test previous file
-    writeFile(xstr_.c_str(),
-	      "address,duid,valid_lifetime,expire,subnet_id,"
-	      "pref_lifetime,lease_type,iaid,prefix_len,fqdn_fwd,"
-	      "fqdn_rev,hostname,hwaddr\n"
-	      "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"
-	      "2001:db8:1::1,,200,200,8,100,0,7,0,1,1,host.example.com,\n"
-	      "2001:db8:2::10,01:01:01:01:0a:01:02:03:04:05,300,300,6,150,"
-	      "0,8,0,0,0,,\n"
-	      "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"
-	      "2001:db8:2::10,01:01:01:01:0a:01:02:03:04:05,300,800,6,150,"
-	      "0,8,0,0,0,,\n"
-	      "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"
-	      );
+    test_str = v6_hdr_ + A_1 + B_1 + A_2 + C_1 + A_3 + B_2;
+    writeFile(xstr_.c_str(), 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());
+
+
+    // Subtest 3: only copy available
+    // No previous file
 
     // Create the test copy file
-    writeFile(istr_.c_str(),
-	      "address,duid,valid_lifetime,expire,subnet_id,"
-	      "pref_lifetime,lease_type,iaid,prefix_len,fqdn_fwd,"
-	      "fqdn_rev,hostname,hwaddr\n"
-	      "2001:db8:2::10,01:01:01:01:0a:01:02:03:04:05,300,1000,6,150,"
-	      "0,8,0,0,0,,\n"
-	      "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"
-	      "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"
-	      "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"
-	      );
+    test_str = v6_hdr_ + A_1 + B_2 + B_3 + A_4 + D_1 + C_2;
+    writeFile(istr_.c_str(), test_str);
 
     // Run the cleanup
     lfc_controller.launch(argc, argv);
 
-    // Compare the results
-    EXPECT_EQ(readFile(xstr_.c_str()),
-	      "address,duid,valid_lifetime,expire,subnet_id,"
-	      "pref_lifetime,lease_type,iaid,prefix_len,fqdn_fwd,"
-	      "fqdn_rev,hostname,hwaddr\n"
-	      "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"
-	      "2001:db8:2::10,01:01:01:01:0a:01:02:03:04:05,300,1000,6,150,"
-	      "0,8,0,0,0,,\n"
-	      "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"
-	      );
+    // 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());
+
+
+    // Subtest 4: neither available
+    // No previous file
+
+    // No copy file
+
+    // Run the cleanup
+    lfc_controller.launch(argc, argv);
+
+    // Compare the results, we expect a header and no leases
+    test_str = v6_hdr_;
+    EXPECT_EQ(readFile(xstr_.c_str()), test_str);
+    remove(xstr_.c_str());
+
+
+    // Subtest 5: a 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);
+
+    // No copy file
+
+    // Run the cleanup, the file should fail but we should
+    // 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));
+
 }
 
 } // end of anonymous namespace

src/lib/dhcpsrv/lease_file_loader.h

@@ -161,20 +161,20 @@ public:
         }
     }
 
-    /// @brief Write leaes from the storage into a lease file
+    /// @brief Write leases from the storage into a lease file
     ///
     /// 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 storege to the file, it does
-    /// not perform any checks for expriation or duplication.
+    /// This method writes all entries in the storage to the file, it does
+    /// not perform any checks for expiration or duplication.
     ///
     /// @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.
     /// @param storage A reference to the container from which leases
-    /// should be written..
+    /// should be written.
     /// @tparam LeasePtrType A @c Lease4 or @c Lease6.
     /// @tparam LeaseFileType A @c CSVLeaseFile4 or @c CSVLeaseFile6.
     /// @tparam StorageType A @c Lease4Storage or @c Lease6Storage.
@@ -188,14 +188,14 @@ public:
         lease_file.close();
         lease_file.open();
 
-	// Iterate over the storage area writing out the leases
+        // Iterate over the storage area writing out the leases
         for (typename StorageType::const_iterator lease = storage.begin();
              lease != storage.end();
              ++lease) {
             lease_file.append(**lease);
         }
 
-	// Close the file 
+        // Close the file
         lease_file.close();
     }
 };

src/lib/dhcpsrv/tests/lease_file_loader_unittest.cc

@@ -82,31 +82,31 @@ public:
     /// @brief Tests the write function.
     ///
     /// This method writes the leases from the storage container to the lease file
-    /// then compares the output to the string provided in the aguments to verify
-    /// the write was correct.  The order of the leases in the output will dpend
+    /// then compares the output to the string provided in the arguments to verify
+    /// the write was correct.  The order of the leases in the output will depend
     /// on the order in which the container provides the leases.
     ///
     /// @param storage A reference to the container to be written to the file
-    /// @param compStr The string to compare to what was read from the file
-    /// 
+    /// @param compare The string to compare to what was read from the file
+    ///
     /// @tparam LeaseStorage Type of the container: @c Lease4Container
     /// @c Lease6Container.
     ///
     template<typename LeaseObjectType, typename LeaseFileType,
-	     typename StorageType>
+             typename StorageType>
     void writeLeases(LeaseFileType lease_file,
-		     const StorageType& storage,
-		     const std::string& compare) {
-      // Prepare for a new file, close and remove the old
-      lease_file.close();
-      io_.removeFile();
-
-      // Write the current leases to the file
-      LeaseFileLoader::write<LeaseObjectType, LeaseFileType, StorageType>
-	(lease_file, storage);
-
-      // Compare to see if we got what we exepcted.
-      EXPECT_EQ(compare, io_.readFile());
+                     const StorageType& storage,
+                     const std::string& compare) {
+        // Prepare for a new file, close and remove the old
+        lease_file.close();
+        io_.removeFile();
+
+        // Write the current leases to the file
+        LeaseFileLoader::write<LeaseObjectType, LeaseFileType, StorageType>
+            (lease_file, storage);
+
+        // Compare to see if we got what we exepcted.
+        EXPECT_EQ(compare, io_.readFile());
     }
 
 
@@ -131,6 +131,9 @@ LeaseFileLoaderTest::absolutePath(const std::string& filename) {
 // This test verifies that the DHCPv4 leases can be loaded from the lease
 // file and that only the most recent entry for each lease is loaded and
 // the previous entries are discarded.
+//
+// It also tests the write function by writing the storage to a file
+// and comparing that with the expected value.
 TEST_F(LeaseFileLoaderTest, load4) {
     // 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
@@ -177,17 +180,20 @@ TEST_F(LeaseFileLoaderTest, load4) {
 
     writeLeases<Lease4, CSVLeaseFile4, Lease4Storage>
         (*lf, storage,
-	 "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,500,8,1,1,"
-	 "host.example.com\n"
-	 "192.0.3.15,dd:de:ba:0d:1b:2e:3e:4f,0a:00:01:04,100,135,7,"
-	 "0,0,\n");
+         "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,500,8,1,1,"
+         "host.example.com\n"
+         "192.0.3.15,dd:de:ba:0d:1b:2e:3e:4f,0a:00:01:04,100,135,7,"
+         "0,0,\n");
 }
 
 // This test verifies that the lease with a valid lifetime of 0
 // is removed from the storage. The valid lifetime of 0 is set
 // for the released leases.
+//
+// It also tests the write function by writing the storage to a file
+// and comparing that with the expected value.
 TEST_F(LeaseFileLoaderTest, load4LeaseRemove) {
     // 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
@@ -219,15 +225,18 @@ TEST_F(LeaseFileLoaderTest, load4LeaseRemove) {
 
     writeLeases<Lease4, CSVLeaseFile4, Lease4Storage>
         (*lf, storage,
-	 "address,hwaddr,client_id,valid_lifetime,expire,subnet_id,"
-	 "fqdn_fwd,fqdn_rev,hostname\n"
-	 "192.0.3.15,dd:de:ba:0d:1b:2e:3e:4f,0a:00:01:04,100,135,7,"
-	 "0,0,\n");
+         "address,hwaddr,client_id,valid_lifetime,expire,subnet_id,"
+         "fqdn_fwd,fqdn_rev,hostname\n"
+         "192.0.3.15,dd:de:ba:0d:1b:2e:3e:4f,0a:00:01:04,100,135,7,"
+         "0,0,\n");
 }
 
 // This test verifies that the DHCPv6 leases can be loaded from the lease
 // file and that only the most recent entry for each lease is loaded and
 // the previous entries are discarded.
+//
+// It also tests the write function by writing the storage to a file
+// and comparing that with the expected value.
 TEST_F(LeaseFileLoaderTest, load6) {
     // Create a lease file with three valid leases: 2001:db8:1::1,
     // 3000:1:: and 2001:db8:2::10.
@@ -277,20 +286,23 @@ TEST_F(LeaseFileLoaderTest, load6) {
 
     writeLeases<Lease6, CSVLeaseFile6, Lease6Storage>
         (*lf, storage,
-	 "address,duid,valid_lifetime,expire,subnet_id,"
-	 "pref_lifetime,lease_type,iaid,prefix_len,fqdn_fwd,"
-	 "fqdn_rev,hostname,hwaddr\n"
-	 "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"
-	 "2001:db8:2::10,01:01:01:01:0a:01:02:03:04:05,300,800,6,150,"
-	 "0,8,0,0,0,,\n"
-	 "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");
+         "address,duid,valid_lifetime,expire,subnet_id,"
+         "pref_lifetime,lease_type,iaid,prefix_len,fqdn_fwd,"
+         "fqdn_rev,hostname,hwaddr\n"
+         "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"
+         "2001:db8:2::10,01:01:01:01:0a:01:02:03:04:05,300,800,6,150,"
+         "0,8,0,0,0,,\n"
+         "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");
 }
 
 // This test verifies that the lease with a valid lifetime of 0
 // is removed from the storage. The valid lifetime of 0 set set
 // for the released leases.
+//
+// It also tests the write function by writing the storage to a file
+// and comparing that with the expected value.
 TEST_F(LeaseFileLoaderTest, load6LeaseRemove) {
     // 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
@@ -324,11 +336,11 @@ TEST_F(LeaseFileLoaderTest, load6LeaseRemove) {
 
     writeLeases<Lease6, CSVLeaseFile6, Lease6Storage>
         (*lf, storage,
-	 "address,duid,valid_lifetime,expire,subnet_id,"
-	 "pref_lifetime,lease_type,iaid,prefix_len,fqdn_fwd,"
-	 "fqdn_rev,hostname,hwaddr\n"
-	 "2001:db8:2::10,01:01:01:01:0a:01:02:03:04:05,300,800,6,150,"
-	 "0,8,0,0,0,,\n");
+         "address,duid,valid_lifetime,expire,subnet_id,"
+         "pref_lifetime,lease_type,iaid,prefix_len,fqdn_fwd,"
+         "fqdn_rev,hostname,hwaddr\n"
+         "2001:db8:2::10,01:01:01:01:0a:01:02:03:04:05,300,800,6,150,"
+         "0,8,0,0,0,,\n");
 }
 
 // This test verifies that the exception is thrown when the specific
@@ -380,6 +392,9 @@ TEST_F(LeaseFileLoaderTest, loadMaxErrors) {
 // This test verifies that the lease with a valid lifetime set to 0 is
 // not loaded if there are no previous entries for this lease in the
 // lease file.
+//
+// It also tests the write function by writing the storage to a file
+// and comparing that with the expected value.
 TEST_F(LeaseFileLoaderTest, loadLeaseWithZeroLifetime) {
     // Create lease file. The second lease has a valid lifetime of 0.
     io_.writeFile("address,hwaddr,client_id,valid_lifetime,expire,subnet_id,"
@@ -388,7 +403,7 @@ TEST_F(LeaseFileLoaderTest, loadLeaseWithZeroLifetime) {
                   "192.0.2.3,06:07:08:09:0a:bd,,0,200,8,1,1,,\n");
 
     boost::scoped_ptr<CSVLeaseFile4> lf(new CSVLeaseFile4(filename_));
-    ASSERT_NO_THROW(lf->open()); 
+    ASSERT_NO_THROW(lf->open());
 
     // Set the error count to 0 to make sure that lease with a zero
     // lifetime doesn't cause an error.
@@ -405,11 +420,11 @@ TEST_F(LeaseFileLoaderTest, loadLeaseWithZeroLifetime) {
 
     writeLeases<Lease4, CSVLeaseFile4, Lease4Storage>
         (*lf, storage,
-	 "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");
+         "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");
 
-}   
+}
 
 
 } // end of anonymous namespace

src/lib/util/pid_file.cc

@@ -97,4 +97,3 @@ PIDFile::deleteFile() const {
 
 } // namespace isc::util
 } // namespace isc
-