diff --git a/configure.ac b/configure.ac index 0a006966fe..83707ee50e 100644 --- a/configure.ac +++ b/configure.ac @@ -1535,7 +1535,6 @@ AC_CONFIG_FILES([compatcheck/Makefile src/lib/dhcpsrv/Makefile src/lib/dhcpsrv/tests/Makefile src/lib/dhcpsrv/tests/test_libraries.h - src/lib/dhcpsrv/tests/testdata/Makefile src/lib/dhcp/tests/Makefile src/lib/dns/benchmarks/Makefile src/lib/dns/gen-rdatacode.py diff --git a/doc/guide/bind10-guide.xml b/doc/guide/bind10-guide.xml index 771e3bb02e..aef7fbc2d0 100644 --- a/doc/guide/bind10-guide.xml +++ b/doc/guide/bind10-guide.xml @@ -3732,7 +3732,7 @@ Dhcp4/subnet4 [] list (default) > config set Dhcp4/lease-database/type "memfile" > config set Dhcp4/lease-database/persist true -> config set Dhcp4/lease-database/leasefile "/tmp/kea-leases4.csv" +> config set Dhcp4/lease-database/name "/tmp/kea-leases4.csv" > config commit will change the default location of the lease file to /tmp/kea-leases4.csv. diff --git a/src/bin/dhcp4/dhcp4.spec b/src/bin/dhcp4/dhcp4.spec index 2ffec0cd2f..daddb5f159 100644 --- a/src/bin/dhcp4/dhcp4.spec +++ b/src/bin/dhcp4/dhcp4.spec @@ -197,12 +197,6 @@ "item_type": "boolean", "item_optional": true, "item_default": true - }, - { - "item_name": "leasefile", - "item_type": "string", - "item_optional": true, - "item_default": "" } ] }, diff --git a/src/bin/dhcp6/dhcp6.spec b/src/bin/dhcp6/dhcp6.spec index d914c5bc97..80f5351a31 100644 --- a/src/bin/dhcp6/dhcp6.spec +++ b/src/bin/dhcp6/dhcp6.spec @@ -191,12 +191,6 @@ "item_type": "boolean", "item_optional": true, "item_default": true - }, - { - "item_name": "leasefile", - "item_type": "string", - "item_optional": true, - "item_default": "" } ] }, diff --git a/src/lib/dhcp/duid.cc b/src/lib/dhcp/duid.cc index 6ad2129914..e4b7592b78 100644 --- a/src/lib/dhcp/duid.cc +++ b/src/lib/dhcp/duid.cc @@ -54,11 +54,18 @@ DUID::decode(const std::string& text) { /// @todo optimize stream operations here. std::vector split_text; boost::split(split_text, text, boost::is_any_of(":"), - boost::algorithm::token_compress_on); + boost::algorithm::token_compress_off); std::ostringstream s; for (size_t i = 0; i < split_text.size(); ++i) { - if (split_text[i].size() == 1) { + // If there are multiple tokens and the current one is empty, it + // means that two consecutive colons were specified. This is not + // allowed for client identifier. + if ((split_text.size() > 1) && split_text[i].empty()) { + isc_throw(isc::BadValue, "invalid identifier '" << text << "': " + << " tokens must be separated with a single colon"); + + } else if (split_text[i].size() == 1) { s << "0"; } else if (split_text[i].size() > 2) { diff --git a/src/lib/dhcp/hwaddr.cc b/src/lib/dhcp/hwaddr.cc index 0ca10eaa89..5f130804b5 100644 --- a/src/lib/dhcp/hwaddr.cc +++ b/src/lib/dhcp/hwaddr.cc @@ -65,15 +65,23 @@ std::string HWAddr::toText(bool include_htype) const { } HWAddr -HWAddr::fromText(const std::string& text) { +HWAddr::fromText(const std::string& text, const uint8_t htype) { /// @todo optimize stream operations here. std::vector split_text; boost::split(split_text, text, boost::is_any_of(":"), - boost::algorithm::token_compress_on); + boost::algorithm::token_compress_off); std::ostringstream s; for (size_t i = 0; i < split_text.size(); ++i) { - if (split_text[i].size() == 1) { + // If there are multiple tokens and the current one is empty, it + // means that two consecutive colons were specified. This is not + // allowed for hardware address. + if ((split_text.size() > 1) && split_text[i].empty()) { + isc_throw(isc::BadValue, "failed to create hardware address" + " from text '" << text << "': tokens of the hardware" + " address must be separated with a single colon"); + + } else if (split_text[i].size() == 1) { s << "0"; } else if (split_text[i].size() > 2) { @@ -89,7 +97,7 @@ HWAddr::fromText(const std::string& text) { isc_throw(isc::BadValue, "failed to create hwaddr from text '" << text << "': " << ex.what()); } - return (HWAddr(binary, HTYPE_ETHER)); + return (HWAddr(binary, htype)); } bool HWAddr::operator==(const HWAddr& other) const { diff --git a/src/lib/dhcp/hwaddr.h b/src/lib/dhcp/hwaddr.h index b296b2dde8..3b4d62687c 100644 --- a/src/lib/dhcp/hwaddr.h +++ b/src/lib/dhcp/hwaddr.h @@ -79,9 +79,11 @@ public: /// type. /// /// @param text HW address in the textual format. + /// @param htype Hardware type. /// /// @return Instance of the HW address created from text. - static HWAddr fromText(const std::string& text); + static HWAddr fromText(const std::string& text, + const uint8_t htype = HTYPE_ETHER); /// @brief Compares two hardware addresses for equality bool operator==(const HWAddr& other) const; diff --git a/src/lib/dhcp/tests/duid_unittest.cc b/src/lib/dhcp/tests/duid_unittest.cc index 71b2d0290d..46704404ef 100644 --- a/src/lib/dhcp/tests/duid_unittest.cc +++ b/src/lib/dhcp/tests/duid_unittest.cc @@ -145,11 +145,11 @@ TEST(DuidTest, fromText) { duid.reset(new DUID(DUID::fromText("00:a:bb:D:ee:EF:ab"))) ); EXPECT_EQ("00:0a:bb:0d:ee:ef:ab", duid->toText()); - // Repeated colon sign in the DUID should be ignored. - ASSERT_NO_THROW( - duid.reset(new DUID(DUID::fromText("00::bb:D:ee:EF:ab"))) + // Repeated colon sign is not allowed. + EXPECT_THROW( + duid.reset(new DUID(DUID::fromText("00::bb:D:ee:EF:ab"))), + isc::BadValue ); - EXPECT_EQ("00:bb:0d:ee:ef:ab", duid->toText()); // DUID with excessive number of digits for one of the bytes. EXPECT_THROW( duid.reset(new DUID(DUID::fromText("00:01:021:03:04:05:06"))), @@ -299,15 +299,16 @@ TEST(ClientIdTest, fromText) { cid = ClientId::fromText("00:a:bb:D:ee:EF:ab") ); EXPECT_EQ("00:0a:bb:0d:ee:ef:ab", cid->toText()); - // Repeated colon sign in the ClientId should be ignored. - ASSERT_NO_THROW( - cid = ClientId::fromText("00::bb:D:ee:EF:ab") + // Repeated colon sign in the ClientId is not allowed. + EXPECT_THROW( + ClientId::fromText("00::bb:D:ee:EF:ab"), + isc::BadValue + ); - EXPECT_EQ("00:bb:0d:ee:ef:ab", cid->toText()); // ClientId with excessive number of digits for one of the bytes. EXPECT_THROW( - cid = ClientId::fromText("00:01:021:03:04:05:06"), - isc::BadValue + ClientId::fromText("00:01:021:03:04:05:06"), + isc::BadValue ); } diff --git a/src/lib/dhcp/tests/hwaddr_unittest.cc b/src/lib/dhcp/tests/hwaddr_unittest.cc index 03084eaf5a..7a904d36e5 100644 --- a/src/lib/dhcp/tests/hwaddr_unittest.cc +++ b/src/lib/dhcp/tests/hwaddr_unittest.cc @@ -149,6 +149,12 @@ TEST(HWAddrTest, fromText) { ); EXPECT_TRUE(hwaddr->toText(false).empty()); + // HWAddr should not allow multiple consecutive colons. + EXPECT_THROW( + hwaddr.reset(new HWAddr(HWAddr::fromText("00::01:00:bc:0d:67"))), + isc::BadValue + ); + // There should be no more than two digits per byte of the HW addr. EXPECT_THROW( hwaddr.reset(new HWAddr(HWAddr::fromText("00:01:00A:bc:0d:67"))), diff --git a/src/lib/dhcpsrv/Makefile.am b/src/lib/dhcpsrv/Makefile.am index 69a9f25ea0..6625eb3027 100644 --- a/src/lib/dhcpsrv/Makefile.am +++ b/src/lib/dhcpsrv/Makefile.am @@ -33,6 +33,8 @@ AM_CXXFLAGS += $(WARNING_NO_MISSING_FIELD_INITIALIZERS_CFLAG) # Make sure the generated files are deleted in a "clean" operation CLEANFILES = *.gcno *.gcda dhcpsrv_messages.h dhcpsrv_messages.cc s-messages +# Remove CSV files created by the CSVLeaseFile6 and CSVLeaseFile4 unit tests. +CLEANFILES += *.csv lib_LTLIBRARIES = libb10-dhcpsrv.la libb10_dhcpsrv_la_SOURCES = diff --git a/src/lib/dhcpsrv/csv_lease_file4.cc b/src/lib/dhcpsrv/csv_lease_file4.cc index 3434ae67eb..49760c7ad3 100644 --- a/src/lib/dhcpsrv/csv_lease_file4.cc +++ b/src/lib/dhcpsrv/csv_lease_file4.cc @@ -46,20 +46,20 @@ CSVLeaseFile4::append(const Lease4& lease) const { bool CSVLeaseFile4::next(Lease4Ptr& lease) { - // We will return NULL pointer if the lease is not read. - lease.reset(); - // Get the row of CSV values. - CSVRow row; - CSVFile::next(row); - // The empty row signals EOF. - if (row == CSVFile::EMPTY_ROW()) { - return (true); - } - - // Try to create a lease from the values read. This may easily result in - // exception. We don't want this function to throw exceptions, so we catch - // them all and rather return the false value. + // Read the CSV row and try to create a lease from the values read. + // This may easily result in exception. We don't want this function + // to throw exceptions, so we catch them all and rather return the + // false value. try { + // Get the row of CSV values. + CSVRow row; + CSVFile::next(row); + // The empty row signals EOF. + if (row == CSVFile::EMPTY_ROW()) { + lease.reset(); + return (true); + } + // Get client id. It is possible that the client id is empty and the // returned pointer is NULL. This is ok, but if the client id is NULL, // we need to be careful to not use the NULL pointer. @@ -68,7 +68,7 @@ CSVLeaseFile4::next(Lease4Ptr& lease) { if (client_id) { client_id_vec = client_id->getClientId(); } - size_t client_id_len = client_id_vec.empty() ? 0 : client_id_vec.size(); + size_t client_id_len = client_id_vec.size(); // Get the HW address. It should never be empty and the readHWAddr checks // that. diff --git a/src/lib/dhcpsrv/csv_lease_file6.cc b/src/lib/dhcpsrv/csv_lease_file6.cc index 0f832b1752..4965df546e 100644 --- a/src/lib/dhcpsrv/csv_lease_file6.cc +++ b/src/lib/dhcpsrv/csv_lease_file6.cc @@ -46,20 +46,20 @@ CSVLeaseFile6::append(const Lease6& lease) const { bool CSVLeaseFile6::next(Lease6Ptr& lease) { - // We will return NULL pointer if the lease is not read. - lease.reset(); - // Get the row of CSV values. - CSVRow row; - CSVFile::next(row); - // The empty row signals EOF. - if (row == CSVFile::EMPTY_ROW()) { - return (true); - } - - // Try to create a lease from the values read. This may easily result in - // exception. We don't want this function to throw exceptions, so we catch - // them all and rather return the false value. + // Read the CSV row and try to create a lease from the values read. + // This may easily result in exception. We don't want this function + // to throw exceptions, so we catch them all and rather return the + // false value. try { + // Get the row of CSV values. + CSVRow row; + CSVFile::next(row); + // The empty row signals EOF. + if (row == CSVFile::EMPTY_ROW()) { + lease.reset(); + return (true); + } + lease.reset(new Lease6(readType(row), readAddress(row), readDUID(row), readIAID(row), readPreferred(row), readValid(row), 0, 0, // t1, t2 = 0 diff --git a/src/lib/dhcpsrv/dbaccess_parser.cc b/src/lib/dhcpsrv/dbaccess_parser.cc index afd2c07e51..ee3a49cd54 100644 --- a/src/lib/dhcpsrv/dbaccess_parser.cc +++ b/src/lib/dhcpsrv/dbaccess_parser.cc @@ -34,7 +34,6 @@ namespace dhcp { DbAccessParser::DbAccessParser(const std::string&, const ParserContext& ctx) : values_(), ctx_(ctx) { - ctx_ = ctx; } // Parse the configuration and check that the various keywords are consistent. diff --git a/src/lib/dhcpsrv/memfile_lease_mgr.cc b/src/lib/dhcpsrv/memfile_lease_mgr.cc index e2f3c6273d..29a6fe6ca4 100644 --- a/src/lib/dhcpsrv/memfile_lease_mgr.cc +++ b/src/lib/dhcpsrv/memfile_lease_mgr.cc @@ -447,7 +447,7 @@ Memfile_LeaseMgr::initLeaseFilePath(Universe u) { std::string lease_file; try { - lease_file = getParameter("leasefile"); + lease_file = getParameter("name"); } catch (const Exception& ex) { lease_file = getDefaultLeaseFilePath(u); } diff --git a/src/lib/dhcpsrv/memfile_lease_mgr.h b/src/lib/dhcpsrv/memfile_lease_mgr.h index f1826a3435..31e738b11e 100644 --- a/src/lib/dhcpsrv/memfile_lease_mgr.h +++ b/src/lib/dhcpsrv/memfile_lease_mgr.h @@ -57,19 +57,15 @@ namespace dhcp { /// /// Originally, the Memfile backend didn't write leases to disk. This was /// particularly useful for testing server performance in non-disk bound -/// conditions. In order to preserve this capability, the new parameters -/// "persist4=yes|no" and "persist6=yes|no" has been introduced in the -/// database access string. For example, database access string: -/// "type=memfile persist4=no persist6=yes" disables disk writes to disk -/// of DHCPv4 leases enables writes to disk of DHCPv6 leases. +/// conditions. In order to preserve this capability, the new parameter +/// "persist=true|false" has been introduced in the database access string. +/// For example, database access string: "type=memfile persist=true" +/// enables writes of leases to a disk. /// -/// The lease file locations can be specified for DHCPv4 and DHCPv6 leases -/// with the following two parameters in the database access string: -/// - leasefile4 -/// - leasefile6 -/// -/// They specify the absolute path to the files (including file names). -/// If they are not specified, the default location in the installation +/// The lease file locations can be specified with the "name=[path]" +/// parameter in the database access string. The [path] is the +/// absolute path to the file (including file name). If this parameter +/// is not specified, the default location in the installation /// directory is used: var/bind10/kea-leases4.csv and /// var/bind10/kea-leases6.csv. class Memfile_LeaseMgr : public LeaseMgr { diff --git a/src/lib/dhcpsrv/tests/Makefile.am b/src/lib/dhcpsrv/tests/Makefile.am index a8a5e1c641..8ce9ffc876 100644 --- a/src/lib/dhcpsrv/tests/Makefile.am +++ b/src/lib/dhcpsrv/tests/Makefile.am @@ -1,9 +1,9 @@ -SUBDIRS = . testdata +SUBDIRS = . AM_CPPFLAGS = -I$(top_builddir)/src/lib -I$(top_srcdir)/src/lib AM_CPPFLAGS += $(BOOST_INCLUDES) -AM_CPPFLAGS += -DTEST_DATA_BUILDDIR=\"$(abs_top_builddir)/src/lib/dhcpsrv/tests/testdata\" -AM_CPPFLAGS += -DDHCP_DATA_DIR=\"$(abs_top_builddir)/src/lib/dhcpsrv/tests/testdata\" +AM_CPPFLAGS += -DTEST_DATA_BUILDDIR=\"$(abs_top_builddir)/src/lib/dhcpsrv/tests\" +AM_CPPFLAGS += -DDHCP_DATA_DIR=\"$(abs_top_builddir)/src/lib/dhcpsrv/tests\" AM_CPPFLAGS += -DINSTALL_PROG=\"$(abs_top_srcdir)/install-sh\" AM_CXXFLAGS = $(B10_CXXFLAGS) diff --git a/src/lib/dhcpsrv/tests/dbaccess_parser_unittest.cc b/src/lib/dhcpsrv/tests/dbaccess_parser_unittest.cc index 55d04330fd..86d51d8f03 100644 --- a/src/lib/dhcpsrv/tests/dbaccess_parser_unittest.cc +++ b/src/lib/dhcpsrv/tests/dbaccess_parser_unittest.cc @@ -248,7 +248,7 @@ TEST_F(DbAccessParserTest, emptyKeyword) { TEST_F(DbAccessParserTest, persistV4Memfile) { const char* config[] = {"type", "memfile", "persist", "true", - "leasefile", "/opt/bind10/var/kea-leases4.csv", + "name", "/opt/bind10/var/kea-leases4.csv", NULL}; string json_config = toJson(config); @@ -267,7 +267,7 @@ TEST_F(DbAccessParserTest, persistV4Memfile) { TEST_F(DbAccessParserTest, persistV6Memfile) { const char* config[] = {"type", "memfile", "persist", "true", - "leasefile", "/opt/bind10/var/kea-leases6.csv", + "name", "/opt/bind10/var/kea-leases6.csv", NULL}; string json_config = toJson(config); diff --git a/src/lib/dhcpsrv/tests/generic_lease_mgr_unittest.cc b/src/lib/dhcpsrv/tests/generic_lease_mgr_unittest.cc index cb331fb8c8..8dc2a785a0 100644 --- a/src/lib/dhcpsrv/tests/generic_lease_mgr_unittest.cc +++ b/src/lib/dhcpsrv/tests/generic_lease_mgr_unittest.cc @@ -673,7 +673,13 @@ GenericLeaseMgrTest::testAddGetDelete6(bool check_t1_t2) { // after the lease is deleted, it should really be gone x = lmptr_->getLease6(Lease::TYPE_NA, IOAddress("2001:db8:1::456")); - EXPECT_EQ(Lease6Ptr(), x); + EXPECT_FALSE(x); + + // Reopen the lease storage to make sure that lease is gone from the + // persistent storage. + reopen(V6); + x = lmptr_->getLease6(Lease::TYPE_NA, IOAddress("2001:db8:1::456")); + EXPECT_FALSE(x); } void @@ -689,7 +695,7 @@ GenericLeaseMgrTest::testBasicLease4() { lmptr_->commit(); // Reopen the database to ensure that they actually got stored. - reopen(); + reopen(V4); Lease4Ptr l_returned = lmptr_->getLease4(ioaddress4_[1]); ASSERT_TRUE(l_returned); @@ -718,7 +724,7 @@ GenericLeaseMgrTest::testBasicLease4() { ASSERT_TRUE(l_returned); detailCompareLease(leases[2], l_returned); - reopen(); + reopen(V4); // The deleted lease should be still gone after we re-read leases from // persistent storage. @@ -740,7 +746,7 @@ GenericLeaseMgrTest::testBasicLease4() { ASSERT_NO_THROW(lmptr_->updateLease4(leases[2])); - reopen(); + reopen(V4); // The lease should be now updated in the storage. l_returned = lmptr_->getLease4(ioaddress4_[2]); @@ -1420,6 +1426,87 @@ GenericLeaseMgrTest::testUpdateLease6() { EXPECT_THROW(lmptr_->updateLease6(leases[2]), isc::dhcp::NoSuchLease); } +void +GenericLeaseMgrTest::testRecreateLease4() { + // Create a lease. + std::vector leases = createLeases4(); + // Copy the lease so as we can freely modify it. + Lease4Ptr lease(new Lease4(*leases[0])); + + // Add a lease. + EXPECT_TRUE(lmptr_->addLease(lease)); + lmptr_->commit(); + + // Check that the lease has been successfuly added. + Lease4Ptr l_returned = lmptr_->getLease4(ioaddress4_[0]); + ASSERT_TRUE(l_returned); + detailCompareLease(lease, l_returned); + + // Delete a lease, check that it's gone. + EXPECT_TRUE(lmptr_->deleteLease(ioaddress4_[0])); + EXPECT_FALSE(lmptr_->getLease4(ioaddress4_[0])); + + // Modify the copy of the lease. Increasing values or negating them ensures + // that they are really modified, because we will never get the same values. + ++lease->subnet_id_; + ++lease->valid_lft_; + lease->fqdn_fwd_ = !lease->fqdn_fwd_; + // Make sure that the lease has been really modified. + ASSERT_NE(*lease, *leases[1]); + // Add the updated lease. + EXPECT_TRUE(lmptr_->addLease(lease)); + lmptr_->commit(); + + // Reopen the lease database, so as the lease is re-read. + reopen(V4); + + // The lease in the database should be modified. + l_returned = lmptr_->getLease4(ioaddress4_[0]); + ASSERT_TRUE(l_returned); + detailCompareLease(lease, l_returned); +} + +void +GenericLeaseMgrTest::testRecreateLease6() { + // Create a lease. + std::vector leases = createLeases6(); + // Copy the lease so as we can freely modify it. + Lease6Ptr lease(new Lease6(*leases[0])); + + // Add a lease. + EXPECT_TRUE(lmptr_->addLease(lease)); + lmptr_->commit(); + + // Check that the lease has been successfuly added. + Lease6Ptr l_returned = lmptr_->getLease6(Lease::TYPE_NA, ioaddress6_[0]); + ASSERT_TRUE(l_returned); + detailCompareLease(lease, l_returned); + + // Delete a lease, check that it's gone. + EXPECT_TRUE(lmptr_->deleteLease(ioaddress6_[0])); + EXPECT_FALSE(lmptr_->getLease6(Lease::TYPE_NA, ioaddress6_[0])); + + // Modify the copy of the lease. Increasing values or negating them ensures + // that they are really modified, because we will never get the same values. + ++lease->subnet_id_; + ++lease->valid_lft_; + lease->fqdn_fwd_ = !lease->fqdn_fwd_; + // Make sure that the lease has been really modified. + ASSERT_NE(*lease, *leases[1]); + // Add the updated lease. + EXPECT_TRUE(lmptr_->addLease(lease)); + lmptr_->commit(); + + // Reopen the lease database, so as the lease is re-read. + reopen(V6); + + // The lease in the database should be modified. + l_returned = lmptr_->getLease6(Lease::TYPE_NA, ioaddress6_[0]); + ASSERT_TRUE(l_returned); + detailCompareLease(lease, l_returned); +} + + }; // namespace test }; // namespace dhcp }; // namespace isc diff --git a/src/lib/dhcpsrv/tests/generic_lease_mgr_unittest.h b/src/lib/dhcpsrv/tests/generic_lease_mgr_unittest.h index 0ee0613555..c153f08237 100644 --- a/src/lib/dhcpsrv/tests/generic_lease_mgr_unittest.h +++ b/src/lib/dhcpsrv/tests/generic_lease_mgr_unittest.h @@ -167,6 +167,13 @@ public: /// @todo: check if it does overlap with @ref testGetLease4NullClientId() void testLease4NullClientId(); + /// @brief Check that the DHCPv4 lease can be added, removed and recreated. + /// + /// This test creates a lease, removes it and then recreates it with some + /// of the attributes changed. Next it verifies that the lease in the + /// persistent storage has been updated as expected. + void testRecreateLease4(); + /// @brief Basic Lease6 Checks /// /// Checks that the addLease, getLease6 (by address) and deleteLease (with an @@ -231,6 +238,13 @@ public: /// Checks that the code is able to update an IPv6 lease in the database. void testUpdateLease6(); + /// @brief Check that the DHCPv6 lease can be added, removed and recreated. + /// + /// This test creates a lease, removes it and then recreates it with some + /// of the attributes changed. Next it verifies that the lease in the + /// persistent storage has been updated as expected. + void testRecreateLease6(); + /// @brief String forms of IPv4 addresses std::vector straddress4_; diff --git a/src/lib/dhcpsrv/tests/lease_file_io.h b/src/lib/dhcpsrv/tests/lease_file_io.h index b2e4da29f1..8e8e128058 100644 --- a/src/lib/dhcpsrv/tests/lease_file_io.h +++ b/src/lib/dhcpsrv/tests/lease_file_io.h @@ -21,10 +21,20 @@ namespace isc { namespace dhcp { namespace test { +/// @brief This class contains functions to perform IO operations on files. +/// +/// This class is solely used by unit tests. Some tests often need files +/// as an input. This class allows for easy creation of text files that can +/// be later used by unit tests. It also provides method to read the contents +/// of the existing file and remove existing file (cleanup after unit test). class LeaseFileIO { public: + /// @brief Constructor + /// + /// @param filename Abolsute path to the file. LeaseFileIO(const std::string& filename); + /// @brief Destructor. ~LeaseFileIO(); /// @brief Check if test file exists on disk. diff --git a/src/lib/dhcpsrv/tests/memfile_lease_mgr_unittest.cc b/src/lib/dhcpsrv/tests/memfile_lease_mgr_unittest.cc index 475646d2b0..30a888443d 100644 --- a/src/lib/dhcpsrv/tests/memfile_lease_mgr_unittest.cc +++ b/src/lib/dhcpsrv/tests/memfile_lease_mgr_unittest.cc @@ -96,7 +96,7 @@ public: static std::string getConfigString(Universe u) { std::ostringstream s; s << "type=memfile " << (u == V4 ? "universe=4 " : "universe=6 ") - << "leasefile=" + << "name=" << getLeaseFilePath(u == V4 ? "leasefile4_0.csv" : "leasefile6_0.csv"); return (s.str()); } @@ -134,13 +134,13 @@ TEST_F(MemfileLeaseMgrTest, constructor) { EXPECT_NO_THROW(lease_mgr.reset(new Memfile_LeaseMgr(pmap))); pmap["persist"] = "true"; - pmap["leasefile"] = getLeaseFilePath("leasefile4_1.csv"); + pmap["name"] = getLeaseFilePath("leasefile4_1.csv"); EXPECT_NO_THROW(lease_mgr.reset(new Memfile_LeaseMgr(pmap))); // Expecting that persist parameter is yes or no. Everything other than // that is wrong. pmap["persist"] = "bogus"; - pmap["leasefile"] = getLeaseFilePath("leasefile4_1.csv"); + pmap["name"] = getLeaseFilePath("leasefile4_1.csv"); EXPECT_THROW(lease_mgr.reset(new Memfile_LeaseMgr(pmap)), isc::BadValue); } @@ -160,10 +160,10 @@ TEST_F(MemfileLeaseMgrTest, getLeaseFilePath) { LeaseMgr::ParameterMap pmap; pmap["universe"] = "4"; - pmap["leasefile"] = getLeaseFilePath("leasefile4_1.csv"); + pmap["name"] = getLeaseFilePath("leasefile4_1.csv"); boost::scoped_ptr lease_mgr(new Memfile_LeaseMgr(pmap)); - EXPECT_EQ(pmap["leasefile"], + EXPECT_EQ(pmap["name"], lease_mgr->getLeaseFilePath(Memfile_LeaseMgr::V4)); pmap["persist"] = "false"; @@ -183,7 +183,7 @@ TEST_F(MemfileLeaseMgrTest, persistLeases) { LeaseMgr::ParameterMap pmap; pmap["universe"] = "4"; // Specify the names of the lease files. Leases will be written. - pmap["leasefile"] = getLeaseFilePath("leasefile4_1.csv"); + pmap["name"] = getLeaseFilePath("leasefile4_1.csv"); boost::scoped_ptr lease_mgr(new Memfile_LeaseMgr(pmap)); lease_mgr.reset(new Memfile_LeaseMgr(pmap)); @@ -191,7 +191,7 @@ TEST_F(MemfileLeaseMgrTest, persistLeases) { EXPECT_FALSE(lease_mgr->persistLeases(Memfile_LeaseMgr::V6)); pmap["universe"] = "6"; - pmap["leasefile"] = getLeaseFilePath("leasefile6_1.csv"); + pmap["name"] = getLeaseFilePath("leasefile6_1.csv"); lease_mgr.reset(new Memfile_LeaseMgr(pmap)); EXPECT_FALSE(lease_mgr->persistLeases(Memfile_LeaseMgr::V4)); EXPECT_TRUE(lease_mgr->persistLeases(Memfile_LeaseMgr::V6)); @@ -384,6 +384,25 @@ TEST_F(MemfileLeaseMgrTest, DISABLED_updateLease6) { testUpdateLease6(); } +/// @brief DHCPv4 Lease recreation tests +/// +/// Checks that the lease can be created, deleted and recreated with +/// different parameters. It also checks that the re-created lease is +/// correctly stored in the lease database. +TEST_F(MemfileLeaseMgrTest, testRecreateLease4) { + startBackend(V4); + testRecreateLease4(); +} + +/// @brief DHCPv6 Lease recreation tests +/// +/// Checks that the lease can be created, deleted and recreated with +/// different parameters. It also checks that the re-created lease is +/// correctly stored in the lease database. +TEST_F(MemfileLeaseMgrTest, testRecreateLease6) { + startBackend(V6); + testRecreateLease6(); +} // The following tests are not applicable for memfile. When adding // new tests to the list here, make sure to provide brief explanation diff --git a/src/lib/dhcpsrv/tests/mysql_lease_mgr_unittest.cc b/src/lib/dhcpsrv/tests/mysql_lease_mgr_unittest.cc index 447bab3c1f..8724cdf5c1 100644 --- a/src/lib/dhcpsrv/tests/mysql_lease_mgr_unittest.cc +++ b/src/lib/dhcpsrv/tests/mysql_lease_mgr_unittest.cc @@ -478,4 +478,22 @@ TEST_F(MySqlLeaseMgrTest, updateLease6) { testUpdateLease6(); } +/// @brief DHCPv4 Lease recreation tests +/// +/// Checks that the lease can be created, deleted and recreated with +/// different parameters. It also checks that the re-created lease is +/// correctly stored in the lease database. +TEST_F(MySqlLeaseMgrTest, testRecreateLease4) { + testRecreateLease4(); +} + +/// @brief DHCPv6 Lease recreation tests +/// +/// Checks that the lease can be created, deleted and recreated with +/// different parameters. It also checks that the re-created lease is +/// correctly stored in the lease database. +TEST_F(MySqlLeaseMgrTest, testRecreateLease6) { + testRecreateLease6(); +} + }; // Of anonymous namespace diff --git a/src/lib/dhcpsrv/tests/testdata/Makefile.am b/src/lib/dhcpsrv/tests/testdata/Makefile.am deleted file mode 100644 index e36578ef13..0000000000 --- a/src/lib/dhcpsrv/tests/testdata/Makefile.am +++ /dev/null @@ -1,5 +0,0 @@ -SUBDIRS = . - -# CSV files are created by unit tests which check the CSVLeaseFile6 -# and CSVLeaseFile4 classes. -CLEANFILES = *.csv diff --git a/src/lib/util/csv_file.cc b/src/lib/util/csv_file.cc index 637106ff54..968ee929d7 100644 --- a/src/lib/util/csv_file.cc +++ b/src/lib/util/csv_file.cc @@ -13,6 +13,9 @@ // PERFORMANCE OF THIS SOFTWARE. #include +#include +#include +#include #include #include @@ -20,53 +23,21 @@ namespace isc { namespace util { CSVRow::CSVRow(const size_t cols, const char separator) - : separator_(separator), values_(cols) { + : separator_(1, separator), values_(cols) { } CSVRow::CSVRow(const std::string& text, const char separator) - : separator_(separator) { + : separator_(1, separator) { // Parsing is exception safe, so this will not throw. parse(text.c_str()); } void -CSVRow::parse(const char* line) { - std::string s(line); - // The 'pos' value holds the current position in the parsed stream. - // Normally, it points to the position of one of the the separator - // characters following the parsed value. For the first value, it - // has to be set to -1. - int pos = -1; - // Position of the first character of the currently parsed value. - size_t start_pos; - // Flag which indicates whether parsing should end because last value - // has been just parsed. - bool leave = false; - // Temporary container which holds parsed values. On successful - // parsing completion, the contents of this container are moved - // to the container holding values for the row. - std::vector values; - - do { - // Set the position of the currently parsed value. - start_pos = pos + 1; - // Find the first separator, following the character at - // start_pos. - pos = s.find(separator_, start_pos); - // The last value is not followed by a separator, so if - // we reached the end of line, take reminder of the string - // and make it a value. - if (pos == std::string::npos) { - pos = s.length(); - // Finish parsing as we already parsed the last value. - leave = true; - } - // Store the parsed value. - values.push_back(s.substr(start_pos, pos - start_pos)); - } while (!leave); - - // Assign new values. - std::swap(values, values_); +CSVRow::parse(const std::string& line) { + // Tokenize the string using a specified separator. Disable compression, + // so as the two consecutive separators mark an empty value. + boost::split(values_, line, boost::is_any_of(separator_), + boost::algorithm::token_compress_off); } std::string @@ -94,21 +65,6 @@ CSVRow::writeAt(const size_t at, const char* value) { values_[at] = value; } -void -CSVRow::writeAt(const size_t at, const std::string& value) { - writeAt(at, value.c_str()); -} - -bool -CSVRow::operator==(const CSVRow& other) const { - return (render() == other.render()); -} - -bool -CSVRow::operator!=(const CSVRow& other) const { - return (render() != other.render()); -} - std::ostream& operator<<(std::ostream& os, const CSVRow& row) { os << row.render(); return (os); @@ -144,12 +100,23 @@ CSVFile::close() { void CSVFile::flush() const { - checkStreamStatus("flush"); + checkStreamStatusAndReset("flush"); fs_->flush(); } void CSVFile::addColumn(const std::string& col_name) { + // It is not allowed to add a new column when file is open. + if (fs_) { + isc_throw(CSVFileError, "attempt to add a column '" << col_name + << "' while the file '" << getFilename() + << "' is open"); + } + addColumnInternal(col_name); +} + +void +CSVFile::addColumnInternal(const std::string& col_name) { if (getColumnIndex(col_name) >= 0) { isc_throw(CSVFileError, "attempt to add duplicate column '" << col_name << "'"); @@ -159,10 +126,7 @@ CSVFile::addColumn(const std::string& col_name) { void CSVFile::append(const CSVRow& row) const { - checkStreamStatus("append"); - - // If a stream is in invalid state, reset the state. - fs_->clear(); + checkStreamStatusAndReset("append"); if (row.getValuesCount() != getColumnCount()) { isc_throw(CSVFileError, "number of values in the CSV row '" @@ -170,6 +134,14 @@ CSVFile::append(const CSVRow& row) const { " columns in the CSV file '" << getColumnCount() << "'"); } + /// @todo Apparently, seekp and seekg are interchangable. A call to seekp + /// results in moving the input pointer too. This is ok for now. It means + /// that when the append() is called, the read pointer is moved to the EOF. + /// For the current use cases we only read a file and then append a new + /// content. If we come up with the scenarios when read and write is + /// needed at the same time, we may revisit this: perhaps remember the + /// old pointer. Also, for safety, we call both functions so as we are + /// sure that both pointers are moved. fs_->seekp(0, std::ios_base::end); fs_->seekg(0, std::ios_base::end); fs_->clear(); @@ -184,15 +156,18 @@ CSVFile::append(const CSVRow& row) const { } void -CSVFile::checkStreamStatus(const std::string& operation) const { +CSVFile::checkStreamStatusAndReset(const std::string& operation) const { if (!fs_) { isc_throw(CSVFileError, "NULL stream pointer when performing '" << operation << "' on file '" << filename_ << "'"); } else if (!fs_->is_open()) { + fs_->clear(); isc_throw(CSVFileError, "closed stream when performing '" << operation << "' on file '" << filename_ << "'"); + } else { + fs_->clear(); } } @@ -231,7 +206,7 @@ CSVFile::getColumnIndex(const std::string& col_name) const { std::string CSVFile::getColumnName(const size_t col_index) const { - if (col_index > cols_.size()) { + if (col_index >= cols_.size()) { isc_throw(isc::OutOfRange, "column index " << col_index << " in the " " CSV file '" << filename_ << "' is out of range; the CSV" " file has only " << cols_.size() << " columns "); @@ -248,16 +223,13 @@ CSVFile::next(CSVRow& row, const bool skip_validation) { try { // Check that stream is "ready" for any IO operations. - checkStreamStatus("get next row"); + checkStreamStatusAndReset("get next row"); } catch (isc::Exception& ex) { setReadMsg(ex.what()); return (false); } - // If a stream is in invalid state, reset the state. - fs_->clear(); - // Get exactly one line of the file. std::string line; std::getline(*fs_, line); @@ -275,7 +247,7 @@ CSVFile::next(CSVRow& row, const bool skip_validation) { return (false); } // If we read anything, parse it. - row.parse(line.c_str()); + row.parse(line); // And check if it is correct. return (skip_validation ? true : validate(row)); @@ -290,44 +262,47 @@ CSVFile::open() { } else { // Try to open existing file, holding some data. fs_.reset(new std::fstream(filename_.c_str())); - // The file may fail to open. For example, because of insufficient - // persmissions. Although the file is not open we should call close - // to reset our internal pointer. - if (!fs_->is_open()) { - close(); - isc_throw(CSVFileError, "unable to open '" << filename_ << "'"); - } - // Make sure we are on the beginning of the file, so as we can parse - // the header. - fs_->seekg(0); - if (!fs_->good()) { - close(); - isc_throw(CSVFileError, "unable to set read pointer in the file '" - << filename_ << "'"); - } - // Read the header. - CSVRow header; - if (!next(header, true)) { - close(); - isc_throw(CSVFileError, "failed to read and parse header of the" - " CSV file '" << filename_ << "': " - << getReadMsg()); - } - - // Check the header against the columns specified for the CSV file. - if (!validateHeader(header)) { - close(); - isc_throw(CSVFileError, "invalid header '" << header - << "' in CSV file '" << filename_ << "'"); - } - - // Everything is good, so if we haven't added any columns yet, - // add them. - if (getColumnCount() == 0) { - for (size_t i = 0; i < header.getValuesCount(); ++i) { - addColumn(header.readAt(i)); + // Catch exceptions so as we can close the file if error occurs. + try { + // The file may fail to open. For example, because of insufficient + // persmissions. Although the file is not open we should call close + // to reset our internal pointer. + if (!fs_->is_open()) { + isc_throw(CSVFileError, "unable to open '" << filename_ << "'"); } + // Make sure we are on the beginning of the file, so as we can parse + // the header. + fs_->seekg(0); + if (!fs_->good()) { + isc_throw(CSVFileError, "unable to set read pointer in the file '" + << filename_ << "'"); + } + + // Read the header. + CSVRow header; + if (!next(header, true)) { + isc_throw(CSVFileError, "failed to read and parse header of the" + " CSV file '" << filename_ << "': " + << getReadMsg()); + } + + // Check the header against the columns specified for the CSV file. + if (!validateHeader(header)) { + isc_throw(CSVFileError, "invalid header '" << header + << "' in CSV file '" << filename_ << "'"); + } + + // Everything is good, so if we haven't added any columns yet, + // add them. + if (getColumnCount() == 0) { + for (size_t i = 0; i < header.getValuesCount(); ++i) { + addColumnInternal(header.readAt(i)); + } + } + } catch (const std::exception& ex) { + close(); + throw; } } } diff --git a/src/lib/util/csv_file.h b/src/lib/util/csv_file.h index 591455cd33..12938dbbe6 100644 --- a/src/lib/util/csv_file.h +++ b/src/lib/util/csv_file.h @@ -103,7 +103,7 @@ public: /// This function is exception-free. /// /// @param line String holding a row of comma separated values. - void parse(const char* line); + void parse(const std::string& line); /// @brief Retrieves a value from the internal container. /// @@ -174,7 +174,9 @@ public: /// @param value Value to be written given as string. /// /// @throw CSVFileError if index is out of range. - void writeAt(const size_t at, const std::string& value); + void writeAt(const size_t at, const std::string& value) { + writeAt(at, value.c_str()); + } /// @brief Replaces the value at specified index. /// @@ -204,7 +206,9 @@ public: /// includes the order of fields, separator etc. /// /// @param other Object to compare to. - bool operator==(const CSVRow& other) const; + bool operator==(const CSVRow& other) const { + return (render() == other.render()); + } /// @brief Unequality operator. /// @@ -212,7 +216,9 @@ public: /// This includes the order of fields, separator etc. /// /// @param other Object to compare to. - bool operator!=(const CSVRow& other) const; + bool operator!=(const CSVRow& other) const { + return (render() != other.render()); + } private: @@ -225,7 +231,12 @@ private: void checkIndex(const size_t at) const; /// @brief Separator character specifed in the constructor. - char separator_; + /// + /// @note Separator is held as a string object (one character long), + /// because the boost::is_any_of algorithm requires a string, not a + /// char value. If we held the separator as a char, we would need to + /// convert it to string on every call to @c CSVRow::parse. + std::string separator_; /// @brief Internal container holding values that belong to the row. std::vector values_; @@ -388,6 +399,19 @@ public: protected: + /// @brief Adds a column regardless if the file is open or not. + /// + /// This function adds as new column to the collection. It is meant to be + /// called internally by the methods of the base class and derived classes. + /// It must not be used in the public scope. The @c CSVFile::addColumn + /// must be used in the public scope instead, because it prevents addition + /// of the new column when the file is open. + /// + /// @param col_name Name of the column. + /// + /// @throw CSVFileError if a column with the specified name exists. + void addColumnInternal(const std::string& col_name); + /// @brief Validate the row read from a file. /// /// This function implements a basic validation for the row read from the @@ -425,10 +449,11 @@ private: /// Checks if the file stream is open so as IO operations can be performed /// on it. This is internally called by the public class members to prevent /// them from performing IO operations on invalid stream and using NULL - /// pointer to a stream. + /// pointer to a stream. The @c clear() method is called on the stream + /// after the status has been checked. /// /// @throw CSVFileError if stream is closed or pointer to it is NULL. - void checkStreamStatus(const std::string& operation) const; + void checkStreamStatusAndReset(const std::string& operation) const; /// @brief Returns size of the CSV file. std::ifstream::pos_type size() const; diff --git a/src/lib/util/tests/csv_file_unittest.cc b/src/lib/util/tests/csv_file_unittest.cc index defb3b7c0f..29c3f13cab 100644 --- a/src/lib/util/tests/csv_file_unittest.cc +++ b/src/lib/util/tests/csv_file_unittest.cc @@ -191,6 +191,32 @@ CSVFileTest::writeFile(const std::string& contents) const { } } +// This test checks that the function which is used to add columns of the +// CSV file works as expected. +TEST_F(CSVFileTest, addColumn) { + boost::scoped_ptr csv(new CSVFile(testfile_)); + // Add two columns. + ASSERT_NO_THROW(csv->addColumn("animal")); + ASSERT_NO_THROW(csv->addColumn("color")); + // Make sure we can't add duplicates. + EXPECT_THROW(csv->addColumn("animal"), CSVFileError); + EXPECT_THROW(csv->addColumn("color"), CSVFileError); + // But we should still be able to add unique columns. + EXPECT_NO_THROW(csv->addColumn("age")); + EXPECT_NO_THROW(csv->addColumn("comments")); + // Assert that the file is opened, because the rest of the test relies + // on this. + ASSERT_NO_THROW(csv->recreate()); + ASSERT_TRUE(exists()); + + // Make sure we can't add columns (even unique) when the file is open. + ASSERT_THROW(csv->addColumn("zoo"), CSVFileError); + // Close the file. + ASSERT_NO_THROW(csv->close()); + // And check that now it is possible to add the column. + EXPECT_NO_THROW(csv->addColumn("zoo")); +} + // This test checks that the appropriate file name is initialized. TEST_F(CSVFileTest, getFilename) { CSVFile csv(testfile_);