From ca432aa5986c073e46df89a48c94b9a8adc0063f Mon Sep 17 00:00:00 2001 From: Shawn Routhier Date: Tue, 3 Feb 2015 22:51:29 -0800 Subject: [PATCH] [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 | 90 ++-- src/bin/lfc/lfc_controller.h | 16 +- src/bin/lfc/tests/lfc_controller_unittests.cc | 415 +++++++++++------- src/lib/dhcpsrv/lease_file_loader.h | 12 +- .../tests/lease_file_loader_unittest.cc | 107 +++-- src/lib/util/pid_file.cc | 1 - 6 files changed, 392 insertions(+), 249 deletions(-) diff --git a/src/bin/lfc/lfc_controller.cc b/src/bin/lfc/lfc_controller.cc index 88b73f19fd..f47ec41128 100644 --- a/src/bin/lfc/lfc_controller.cc +++ b/src/bin/lfc/lfc_controller.cc @@ -22,8 +22,6 @@ #include #include -#include - #include #include #include @@ -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(); - } else { - processLeases(); - } + if (verbose_ == true) + std::cerr << "LFC Processing files" << std::endl; + + try { + if (protocol_version_ == 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; + 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 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(lfPrev, storage, - MAX_LEASE_ERRORS); + if (lf_prev.exists()) { + LeaseFileLoader::load(lf_prev, storage, + MAX_LEASE_ERRORS); } - // Follow that with the copy of the current lease file - if (lfCopy.exists()) { - LeaseFileLoader::load(lfCopy, storage, - MAX_LEASE_ERRORS); + // Follow that with the copy of the current lease file + if (lf_copy.exists()) { + LeaseFileLoader::load(lf_copy, storage, + MAX_LEASE_ERRORS); } // Write the result out to the output file - LeaseFileLoader::write(lfOutput, storage); + 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) diff --git a/src/bin/lfc/lfc_controller.h b/src/bin/lfc/lfc_controller.h index 24eb5e0da3..4eb5545d5e 100644 --- a/src/bin/lfc/lfc_controller.h +++ b/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 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; //@} diff --git a/src/bin/lfc/tests/lfc_controller_unittests.cc b/src/bin/lfc/tests/lfc_controller_unittests.cc index e5da609816..8ce0cb68c9 100644 --- a/src/bin/lfc/tests/lfc_controller_unittests.cc +++ b/src/bin/lfc/tests/lfc_controller_unittests.cc @@ -13,6 +13,7 @@ // PERFORMANCE OF THIS SOFTWARE. #include +#include #include #include #include @@ -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(fs)), - std::istreambuf_iterator()); + std::istreambuf_iterator()); 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("progName"), const_cast("-4"), const_cast("-x"), @@ -386,64 +371,135 @@ TEST_F(LFCControllerTest, programLaunch4) { const_cast("-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: + // _ + 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("progName"), const_cast("-6"), const_cast("-x"), @@ -461,56 +517,119 @@ TEST_F(LFCControllerTest, programLaunch6) { const_cast("-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: + // _. + 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 diff --git a/src/lib/dhcpsrv/lease_file_loader.h b/src/lib/dhcpsrv/lease_file_loader.h index fa47fe4b69..feb151ca3d 100644 --- a/src/lib/dhcpsrv/lease_file_loader.h +++ b/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(); } }; diff --git a/src/lib/dhcpsrv/tests/lease_file_loader_unittest.cc b/src/lib/dhcpsrv/tests/lease_file_loader_unittest.cc index fae1946b77..cdf4dbe011 100644 --- a/src/lib/dhcpsrv/tests/lease_file_loader_unittest.cc +++ b/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 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 - (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 + (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 (*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 (*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 (*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 (*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 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 (*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 diff --git a/src/lib/util/pid_file.cc b/src/lib/util/pid_file.cc index b4f54f2415..e4ac42f7b3 100644 --- a/src/lib/util/pid_file.cc +++ b/src/lib/util/pid_file.cc @@ -97,4 +97,3 @@ PIDFile::deleteFile() const { } // namespace isc::util } // namespace isc - -- 2.47.3