From 7b894faf705e4f165e05a992d47eec68fca35b61 Mon Sep 17 00:00:00 2001 From: Shawn Routhier Date: Wed, 4 Feb 2015 21:39:53 -0800 Subject: [PATCH] [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 | 10 + src/bin/lfc/lfc_controller.cc | 72 +++-- src/bin/lfc/lfc_controller.h | 64 ++-- src/bin/lfc/tests/lfc_controller_unittests.cc | 296 ++++++++++-------- src/lib/dhcpsrv/lease_file_loader.h | 18 +- .../tests/lease_file_loader_unittest.cc | 12 +- 6 files changed, 274 insertions(+), 198 deletions(-) diff --git a/ChangeLog b/ChangeLog index 94cc62540a..b20a28d413 100644 --- a/ChangeLog +++ b/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 diff --git a/src/bin/lfc/lfc_controller.cc b/src/bin/lfc/lfc_controller.cc index f47ec41128..4787c9661a 100644 --- a/src/bin/lfc/lfc_controller.cc +++ b/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(); } else { processLeases(); } } 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 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(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(lf_copy, storage, MAX_LEASE_ERRORS); } // Write the result out to the output file + LeaseFileType lf_output(getOutputFile()); LeaseFileLoader::write(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 diff --git a/src/bin/lfc/lfc_controller.h b/src/bin/lfc/lfc_controller.h index 4eb5545d5e..560d285992 100644 --- a/src/bin/lfc/lfc_controller.h +++ b/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 - 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 + void processLeases() const; }; }; // namespace isc::lfc diff --git a/src/bin/lfc/tests/lfc_controller_unittests.cc b/src/bin/lfc/tests/lfc_controller_unittests.cc index 8ce0cb68c9..28edafd8fa 100644 --- a/src/bin/lfc/tests/lfc_controller_unittests.cc +++ b/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: // _ - 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: // _. - 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 diff --git a/src/lib/dhcpsrv/lease_file_loader.h b/src/lib/dhcpsrv/lease_file_loader.h index feb151ca3d..71a50ba762 100644 --- a/src/lib/dhcpsrv/lease_file_loader.h +++ b/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 diff --git a/src/lib/dhcpsrv/tests/lease_file_loader_unittest.cc b/src/lib/dhcpsrv/tests/lease_file_loader_unittest.cc index cdf4dbe011..1979953c47 100644 --- a/src/lib/dhcpsrv/tests/lease_file_loader_unittest.cc +++ b/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 (*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"); - } -- 2.47.3