Merge branch 'trac3665' Add processing for LFC

This branch adds the main processing functions for
lease file cleanup.  We added

code in lease_file_loader.h to write leases from storage to a file

code in lfc_controller to read leaes into storage from a file
and then write the storage out to a file (using lease_file_loader)
and then to move the files around to get the names correct.

Conflicts:
	ChangeLog

ChangeLog

@@ -1,3 +1,9 @@
+887.	[func]		sar
+	A new process, kea-lfc, has been added. It is meant to
+	be periodically executed by the DHCPv4 and DHCPv6 servers
+	to remove redundant information from the lease files.
+	See http://kea.isc.org/wiki/LFCDesign#no1 for the design.
+
 886.	[func]		tomek
 	libdhcpsrv: Allocation Engine now uses statically assigned
 	addresses when it allocates leases for the DHCPv6 clients.

src/bin/lfc/lfc_controller.cc

@@ -15,14 +15,27 @@
 #include <lfc/lfc_controller.h>
 #include <util/pid_file.h>
 #include <exceptions/exceptions.h>
+#include <dhcpsrv/csv_lease_file4.h>
+#include <dhcpsrv/csv_lease_file6.h>
+#include <dhcpsrv/memfile_lease_storage.h>
+#include <dhcpsrv/lease_mgr.h>
+#include <dhcpsrv/lease_file_loader.h>
 #include <config.h>
+
 #include <iostream>
 #include <sstream>
 #include <unistd.h>
 #include <stdlib.h>
+#include <cerrno>
 
 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 {
@@ -44,6 +57,8 @@ LFCController::~LFCController() {
 
 void
 LFCController::launch(int argc, char* argv[]) {
+    bool do_rotate = true;
+
     try {
         parseArgs(argc, argv);
     } catch (const InvalidUsage& ex) {
@@ -51,42 +66,75 @@ LFCController::launch(int argc, char* argv[]) {
         throw;  // rethrow it
     }
 
-    std::cerr << "Starting lease file cleanup" << std::endl;
+    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;
     }
 
-    // do other work (TBD)
-    std::cerr << "Add code to perform lease cleanup" << std::endl;
+    // 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 (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_rotate = false;
+            std::cerr << "Processing failed: " << proc_ex.what() << std::endl;
+        }
+    }
+
+    // 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_rotate) {
+        if (verbose_) {
+            std::cerr << "LFC cleaning files" << std::endl;
+        }
+
+        try {
+            fileRotate();
+        } catch (const RunTimeFail& run_ex) {
+            std::cerr << run_ex.what() << std::endl;
+        }
+    }
 
     // delete the pid file for this instance
     try {
         pid_file.deleteFile();
     } catch (const PIDFileError& pid_ex) {
         std::cerr << pid_ex.what() << std::endl;
-        return;
     }
 
-    std::cerr << "LFC complete" << std::endl;
+    if (verbose_) {
+        std::cerr << "LFC complete" << std::endl;
+    }
 }
 
 void
@@ -223,20 +271,21 @@ 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
-                << "Finishn file:              " << finish_file_ << std::endl
-                << "Config file:               " << config_file_ << std::endl
-                << "PID file:                  " << pid_file_ << std::endl;
+    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
+                  << "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;
     }
 }
 
 void
 LFCController::usage(const std::string& text) {
-    if (text != "") {
+    if (!text.empty()) {
         std::cerr << "Usage error: " << text << std::endl;
     }
 
@@ -268,5 +317,59 @@ LFCController::getVersion(const bool extended) const{
     return (version_stream.str());
 }
 
+template<typename LeaseObjectType, typename LeaseFileType, typename StorageType>
+void
+LFCController::processLeases() const {
+    StorageType storage;
+
+    // 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(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::fileRotate() const {
+    // Remove the old previous file
+    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(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) {
+        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,17 +28,20 @@ public:
         isc::Exception(file, line, what) { };
 };
 
+/// @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) :
+        isc::Exception(file, line, what) { };
+};
+
 /// @brief Process controller for LFC process
 ///
 /// This class provides the LFC process functions. These are used to:
 /// 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
-/// -# handle PID file manipulation
-/// -# 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
@@ -56,12 +59,19 @@ 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
     ///
     /// -# parse command line arguments
     /// -# verify that it is the only instance
     /// -# create pid file
-    /// -# .... TBD
+    /// -# read leases files
+    /// -# write lease file
+    /// -# move leases files
+    /// -# cleanup artifacts
     /// -# remove pid file
     /// -# exit to the caller
     ///
@@ -71,8 +81,9 @@ public:
     /// @throw InvalidUsage if the command line parameters are invalid.
     void launch(int argc, char* argv[]);
 
-    /// @brief Process the command line arguments.  It is the first
-    /// step taken after the process has been launched.
+    /// @brief Process the command line arguments.
+    ///
+    /// It is the first step taken after the process has been launched.
     ///
     /// @param argc Number of strings in the @c argv array.
     /// @param argv Array of arguments passed in via the program's main function.
@@ -80,17 +91,14 @@ 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.
+    /// @brief Rotate files.
     ///
-    /// @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
+    /// 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
     //@{
@@ -158,6 +166,32 @@ 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

@@ -13,17 +13,127 @@
 // PERFORMANCE OF THIS SOFTWARE.
 
 #include <lfc/lfc_controller.h>
+#include <util/csv_file.h>
 #include <gtest/gtest.h>
+#include <fstream>
+#include <cerrno>
 
 using namespace isc::lfc;
 using namespace std;
 
 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
+    string ostr_; ///< String for name for output file
+    string fstr_; ///< String for name for finish file
+    string cstr_; ///< String for name for config file
+
+    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
+    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 {
+        return ((remove(filename.c_str()) != 0) && (errno == ENOENT));
+    }
+
+    /// @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 {
+        return (noExist(istr_) && noExist(ostr_) && noExist(fstr_));
+    }
+
+    /// @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 {
+        return ((noExist(istr_) && noExist(ostr_) &&
+                 noExist(fstr_) && noExist(pstr_)));
+    }
+
+    /// @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 
+    /// any old test files before the test
+    virtual void SetUp() {
+        // set up the test files we need
+        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();
+    }
+
+    /// @brief Removes any remaining test files after the test
+    virtual void TearDown() {
+        removeTestFile();
+    }
+
+private:
+};
+
+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>());
+    fs.close();
+    return (contents);
+}
+
+void
+LFCControllerTest::writeFile(const std::string& filename,
+                             const std::string& contents) const {
+    std::ofstream fs(filename, std::ofstream::out);
+
+    if (fs.is_open()) {
+        fs << contents;
+        fs.close();
+    }
+}
+
 /// @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.
-TEST(LFCControllerTest, initialValues) {
+TEST_F(LFCControllerTest, initialValues) {
     LFCController lfc_controller;
 
     // Verify that we start with all the private variables empty
@@ -39,7 +149,7 @@ TEST(LFCControllerTest, initialValues) {
 /// @brief Verify that parsing a full command line works.
 /// Parse a complete command line then verify the parsed
 /// and saved data matches our expectations.
-TEST(LFCControllerTest, fullCommandLine) {
+TEST_F(LFCControllerTest, fullCommandLine) {
     LFCController lfc_controller;
 
     // Verify that standard options can be parsed without error
@@ -75,7 +185,7 @@ TEST(LFCControllerTest, fullCommandLine) {
 /// Parse a command line that is correctly formatted but isn't complete
 /// (doesn't include some options or an some option arguments).  We
 /// expect that the parse will fail with an InvalidUsage exception.
-TEST(LFCControllerTest, notEnoughData) {
+TEST_F(LFCControllerTest, notEnoughData) {
     LFCController lfc_controller;
 
     // Test the results if we don't include all of the required arguments
@@ -112,7 +222,7 @@ TEST(LFCControllerTest, notEnoughData) {
 /// to verify that we don't stop parsing when we find all of the
 /// required arguments.  We exepct the parse to fail with an
 /// InvalidUsage exception.
-TEST(LFCControllerTest, tooMuchData) {
+TEST_F(LFCControllerTest, tooMuchData) {
     LFCController lfc_controller;
 
     // The standard options plus some others
@@ -144,7 +254,7 @@ TEST(LFCControllerTest, tooMuchData) {
 /// @brief Verify that unknown arguments cause the parse to fail.
 /// Parse some unknown arguments to verify that we generate the
 /// proper InvalidUsage exception.
-TEST(LFCControllerTest, someBadData) {
+TEST_F(LFCControllerTest, someBadData) {
     LFCController lfc_controller;
 
     // Some random arguments
@@ -160,4 +270,397 @@ TEST(LFCControllerTest, someBadData) {
     EXPECT_THROW(lfc_controller.parseArgs(argc, argv), InvalidUsage);
 }
 
+/// @brief Verify that we do file rotation correctly.  We create different
+/// files and see if we properly delete and move them.
+TEST_F(LFCControllerTest, fileRotate) {
+    LFCController lfc_controller, lfc_controller_launch;
+
+    // We can use the same arguments and controller for all of the tests
+    // as the files get redone for each subtest.  We leave "-d" in the arg
+    // list but don't pass it as we use 14 as the argument count.  This
+    // makes it easy to turn it on by simply increasing argc below to 15
+    char* argv[] = { const_cast<char*>("progName"),
+                     const_cast<char*>("-4"),
+                     const_cast<char*>("-x"),
+                     const_cast<char*>(xstr_.c_str()),
+                     const_cast<char*>("-i"),
+                     const_cast<char*>(istr_.c_str()),
+                     const_cast<char*>("-o"),
+                     const_cast<char*>(ostr_.c_str()),
+                     const_cast<char*>("-c"),
+                     const_cast<char*>(cstr_.c_str()),
+                     const_cast<char*>("-f"),
+                     const_cast<char*>(fstr_.c_str()),
+                     const_cast<char*>("-p"),
+                     const_cast<char*>(pstr_.c_str()),
+                     const_cast<char*>("-d")
+    };
+    int argc = 14;
+    lfc_controller.parseArgs(argc, argv);
+
+    // Test 1: Start with no files - we expect an execption as there
+    // is no file to copy.
+    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_, "1");
+    writeFile(istr_, "2");
+    writeFile(fstr_, "3");
+
+    lfc_controller.fileRotate();
+
+    // 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_, "4");
+    writeFile(fstr_, "6");
+
+    lfc_controller.fileRotate();
+
+    // 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_, "8");
+    writeFile(fstr_, "9");
+
+    lfc_controller.fileRotate();
+
+    // 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_, "10");
+    writeFile(istr_, "11");
+    writeFile(fstr_, "12");
+
+    lfc_controller_launch.launch(argc, argv);
+
+    // 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
+///
+/// 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.  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, launch4) {
+    LFCController lfc_controller;
+
+    // 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"),
+                     const_cast<char*>(xstr_.c_str()),
+                     const_cast<char*>("-i"),
+                     const_cast<char*>(istr_.c_str()),
+                     const_cast<char*>("-o"),
+                     const_cast<char*>(ostr_.c_str()),
+                     const_cast<char*>("-c"),
+                     const_cast<char*>(cstr_.c_str()),
+                     const_cast<char*>("-f"),
+                     const_cast<char*>(fstr_.c_str()),
+                     const_cast<char*>("-p"),
+                     const_cast<char*>(pstr_.c_str()),
+                     const_cast<char*>("-d")
+    };
+    int argc = 14;
+    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_, test_str);
+
+    // Create the test copy file
+    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.
+    // 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_, 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.
+    // 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_, 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.
+    // 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
+    // No previous file
+
+    // No copy file
+
+    // Run the cleanup
+    lfc_controller.launch(argc, argv);
+
+    // 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_), test_str);
+    EXPECT_TRUE(noExistIOFP());
+    removeTestFile();
+
+
+    // 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_, 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.
+    // 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
+///
+/// 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.  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, launch6) {
+    LFCController lfc_controller;
+
+    // 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"),
+                     const_cast<char*>(xstr_.c_str()),
+                     const_cast<char*>("-i"),
+                     const_cast<char*>(istr_.c_str()),
+                     const_cast<char*>("-o"),
+                     const_cast<char*>(ostr_.c_str()),
+                     const_cast<char*>("-c"),
+                     const_cast<char*>(cstr_.c_str()),
+                     const_cast<char*>("-f"),
+                     const_cast<char*>(fstr_.c_str()),
+                     const_cast<char*>("-p"),
+                     const_cast<char*>(pstr_.c_str()),
+                     const_cast<char*>("-d")
+    };
+    int argc = 14;
+    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_, test_str);
+
+    // Create the test copy file
+    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.
+    // 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_, test_str);
+
+    // No copy file
+
+    // Run the cleanup
+    lfc_controller.launch(argc, argv);
+
+    // 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_, test_str);
+
+    // Run the cleanup
+    lfc_controller.launch(argc, argv);
+
+    // 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
+    // No previous file
+
+    // No copy file
+
+    // Run the cleanup
+    lfc_controller.launch(argc, argv);
+
+    // 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_), test_str);
+    EXPECT_TRUE(noExistIOFP());
+    removeTestFile();
+
+
+    // 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_, 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.
+    // 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

@@ -160,9 +160,60 @@ public:
             lease_file.close();
         }
     }
+
+    /// @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 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.
+    /// @param storage A reference to the container from which leases
+    /// 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.
+    template<typename LeaseObjectType, typename LeaseFileType,
+             typename StorageType>
+    static void write(LeaseFileType& lease_file, const StorageType& storage) {
+        // Reopen the file, as we don't know whether the file is open
+        // and we also don't know its current state.
+        lease_file.close();
+        lease_file.open();
+
+        // Iterate over the storage area writing out the leases
+        for (typename StorageType::const_iterator lease = storage.begin();
+             lease != storage.end();
+             ++lease) {
+            try {
+                lease_file.append(**lease);
+            } catch (const isc::Exception& ex) {
+            // Close the file
+            lease_file.close();
+            throw;
+            }
+        }
+
+        // Close the file
+        lease_file.close();
+    }
 };
 
-}
-}
+} // namesapce dhcp
+} // namespace isc
 
 #endif // LEASE_FILE_LOADER_H

src/lib/dhcpsrv/tests/lease_file_loader_unittest.cc

@@ -79,6 +79,37 @@ public:
         return (LeasePtrType());
     }
 
+    /// @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 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 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>
+    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());
+    }
+
+
     /// @brief Name of the test lease file.
     std::string filename_;
 
@@ -100,7 +131,10 @@ 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.
-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, 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.
@@ -143,12 +177,24 @@ TEST_F(LeaseFileLoaderTest, load4) {
     lease = getLease<Lease4Ptr>("192.0.3.15", storage);
     ASSERT_TRUE(lease);
     EXPECT_EQ(35, lease->cltt_);
+
+    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");
 }
 
 // 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.
-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, 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.
@@ -176,12 +222,22 @@ TEST_F(LeaseFileLoaderTest, load4LeaseRemove) {
     Lease4Ptr lease = getLease<Lease4Ptr>("192.0.3.15", storage);
     ASSERT_TRUE(lease);
     EXPECT_EQ(35, lease->cltt_);
+
+    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");
 }
 
 // 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.
-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, 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,"
@@ -226,12 +282,27 @@ TEST_F(LeaseFileLoaderTest, load6) {
     lease = getLease<Lease6Ptr>("2001:db8:2::10", storage);
     ASSERT_TRUE(lease);
     EXPECT_EQ(500, lease->cltt_);
+
+    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");
 }
 
 // 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.
-TEST_F(LeaseFileLoaderTest, load6LeaseRemove) {
+//
+// It also tests the write function by writing the storage to a file
+// and comparing that with the expected value.
+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.
@@ -261,6 +332,14 @@ TEST_F(LeaseFileLoaderTest, load6LeaseRemove) {
     Lease6Ptr lease = getLease<Lease6Ptr>("2001:db8:2::10", storage);
     ASSERT_TRUE(lease);
     EXPECT_EQ(500, lease->cltt_);
+
+    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");
 }
 
 // This test verifies that the exception is thrown when the specific
@@ -312,7 +391,10 @@ 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.
-TEST_F(LeaseFileLoaderTest, loadLeaseWithZeroLifetime) {
+//
+// It also tests the write function by writing the storage to a file
+// and comparing that with the expected value.
+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"
@@ -320,7 +402,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.
@@ -334,7 +416,13 @@ TEST_F(LeaseFileLoaderTest, loadLeaseWithZeroLifetime) {
 
     // The lease with a valid lifetime of 0 should not be loaded.
     EXPECT_FALSE(getLease<Lease4Ptr>("192.0.2.3", storage));
-}   
+
+    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");
+}
 
 
 } // end of anonymous namespace

src/lib/util/pid_file.cc

@@ -16,6 +16,7 @@
 #include <cstdio>
 #include <signal.h>
 #include <unistd.h>
+#include <cerrno>
 
 namespace isc {
 namespace util {
@@ -87,7 +88,8 @@ PIDFile::write(int pid) const {
 
 void
 PIDFile::deleteFile() const {
-    if (remove(filename_.c_str()) != 0) {
+    if ((remove(filename_.c_str()) != 0) &&
+        (errno != ENOENT)) {
         isc_throw(PIDFileError, "Unable to delete PID file '"
                   << filename_ << "'");
     }

src/lib/util/tests/pid_file_unittest.cc

@@ -64,7 +64,7 @@ protected:
 private:
     /// @brief Removes any remaining test files
     void removeTestFile() const {
-        remove(TESTNAME);
+        remove(absolutePath(TESTNAME).c_str());
     }
 
 };
@@ -192,4 +192,11 @@ TEST_F(PIDFileTest, pidWriteFail) {
     EXPECT_THROW(pid_file.write(10), PIDFileError);
 }
 
+/// @brief Test deleting a file that doesn't exist
+TEST_F(PIDFileTest, noDeleteFile) {
+    PIDFile pid_file(absolutePath(TESTNAME));
+
+    // Delete a file we haven't created
+    pid_file.deleteFile();
+}
 } // end of anonymous namespace