[3360] Addressed review comments.

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

doc/guide/bind10-guide.xml

@@ -3732,7 +3732,7 @@ Dhcp4/subnet4	[]	list	(default)
 <screen>
 &gt; <userinput>config set Dhcp4/lease-database/type "memfile"</userinput>
 &gt; <userinput>config set Dhcp4/lease-database/persist true</userinput>
-&gt; <userinput>config set Dhcp4/lease-database/leasefile "/tmp/kea-leases4.csv"</userinput>
+&gt; <userinput>config set Dhcp4/lease-database/name "/tmp/kea-leases4.csv"</userinput>
 &gt; <userinput>config commit</userinput>
 </screen>
         will change the default location of the lease file to /tmp/kea-leases4.csv.

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": ""
             }
         ]
       },

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": ""
             }
         ]
       },

src/lib/dhcp/duid.cc

@@ -54,11 +54,18 @@ DUID::decode(const std::string& text) {
     /// @todo optimize stream operations here.
     std::vector<std::string> 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) {

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<std::string> 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 {

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;

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
     );
 }
 

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"))),

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  =

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.

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

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.

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);
     }

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 {

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)

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);

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<Lease4Ptr> 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<Lease6Ptr> 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

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<std::string>  straddress4_;
 

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.

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<Memfile_LeaseMgr> 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<Memfile_LeaseMgr> 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

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

src/lib/dhcpsrv/tests/testdata/Makefile.am

@@ -1,5 +0,0 @@
-SUBDIRS = .
-
-# CSV files are created by unit tests which check the CSVLeaseFile6
-# and CSVLeaseFile4 classes.
-CLEANFILES = *.csv

src/lib/util/csv_file.cc

@@ -13,6 +13,9 @@
 // PERFORMANCE OF THIS SOFTWARE.
 
 #include <util/csv_file.h>
+#include <boost/algorithm/string/classification.hpp>
+#include <boost/algorithm/string/constants.hpp>
+#include <boost/algorithm/string/split.hpp>
 #include <fstream>
 #include <sstream>
 
@@ -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<std::string> 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());
-        }
+        // 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_ << "'");
+            }
 
-        // 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_ << "'");
-        }
+            // 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) {
-                addColumn(header.readAt(i));
+            // 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;
         }
     }
 }

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<std::string> 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;

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<CSVFile> 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_);