[3360] Cleanup the CSV file parser for DHCPv6 leases.

src/lib/dhcpsrv/csv_lease_file6.cc

@@ -13,7 +13,6 @@
 // PERFORMANCE OF THIS SOFTWARE.
 
 #include <dhcpsrv/csv_lease_file6.h>
-#include <algorithm>
 
 using namespace isc::asiolink;
 using namespace isc::util;
@@ -27,22 +26,6 @@ CSVLeaseFile6::CSVLeaseFile6(const std::string& filename)
 }
 
 void
-CSVLeaseFile6::initColumns() {
-    addColumn("address");
-    addColumn("duid");
-    addColumn("valid_lifetime");
-    addColumn("expire");
-    addColumn("subnet_id");
-    addColumn("pref_lifetime");
-    addColumn("lease_type");
-    addColumn("iaid");
-    addColumn("prefix_len");
-    addColumn("fqdn_fwd");
-    addColumn("fqdn_rev");
-    addColumn("hostname");
-}
-
-void
 CSVLeaseFile6::append(const Lease6& lease) const {
     CSVRow row(getColumnCount());
     row.writeAt(getColumnIndex("address"), lease.addr_.toText());
@@ -63,26 +46,33 @@ 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.
     try {
         lease.reset(new Lease6(readType(row), readAddress(row), readDUID(row),
                                readIAID(row), readPreferred(row),
-                               readValid(row), 0, 0, readSubnetID(row),
+                               readValid(row), 0, 0, // t1, t2 = 0
+                               readSubnetID(row),
                                readPrefixLen(row)));
         lease->cltt_ = readCltt(row);
         lease->fqdn_fwd_ = readFqdnFwd(row);
         lease->fqdn_rev_ = readFqdnRev(row);
-        lease->hostname_ = readhostname(row);
+        lease->hostname_ = readHostname(row);
 
     } catch (std::exception& ex) {
+        // The lease might have been created, so let's set it back to NULL to
+        // signal that lease hasn't been parsed.
         lease.reset();
         setReadMsg(ex.what());
         return (false);
@@ -90,6 +80,22 @@ CSVLeaseFile6::next(Lease6Ptr& lease) {
     return (true);
 }
 
+void
+CSVLeaseFile6::initColumns() {
+    addColumn("address");
+    addColumn("duid");
+    addColumn("valid_lifetime");
+    addColumn("expire");
+    addColumn("subnet_id");
+    addColumn("pref_lifetime");
+    addColumn("lease_type");
+    addColumn("iaid");
+    addColumn("prefix_len");
+    addColumn("fqdn_fwd");
+    addColumn("fqdn_rev");
+    addColumn("hostname");
+}
+
 Lease::Type
 CSVLeaseFile6::readType(const CSVRow& row) {
     return (static_cast<Lease::Type>
@@ -131,7 +137,7 @@ CSVLeaseFile6::readValid(const CSVRow& row) {
 uint32_t
 CSVLeaseFile6::readCltt(const CSVRow& row) {
     uint32_t cltt = row.readAndConvertAt<uint32_t>(getColumnIndex("expire"))
-        - readValid();
+        - readValid(row);
     return (cltt);
 }
 
@@ -166,7 +172,5 @@ CSVLeaseFile6::readHostname(const CSVRow& row) {
     return (hostname);
 }
 
-
-
 } // end of namespace isc::dhcp
 } // end of namespace isc

src/lib/dhcpsrv/csv_lease_file6.h

@@ -20,7 +20,6 @@
 #include <dhcpsrv/lease.h>
 #include <dhcpsrv/subnet.h>
 #include <util/csv_file.h>
-#include <boost/shared_ptr.hpp>
 #include <stdint.h>
 #include <string>
 
@@ -28,6 +27,16 @@ namespace isc {
 namespace dhcp {
 
 /// @brief Provides methods to access CSV file with DHCPv6 leases.
+///
+/// This class contains methods customized to read DHCPv6 leases from the CSV
+/// file. It expects that the CSV file being parsed, contains the set of columns
+/// with well known names (initialized in the class constructor).
+///
+/// @todo This class doesn't validate the lease values read from the file.
+/// The @c Lease6 is a structure that should be itself responsible for this
+/// validation (see http://bind10.isc.org/ticket/2405). However, when #2405
+/// is implemented, the @c next function may need to be updated to use the
+/// validation capablity of @c Lease6.
 class CSVLeaseFile6 : public isc::util::CSVFile {
 public:
 
@@ -40,6 +49,11 @@ public:
 
     /// @brief Appends the lease record to the CSV file.
     ///
+    /// This function doesn't throw exceptions itself. In theory, exceptions
+    /// are possible when the index of the indexes of the values being written
+    /// to the file are invalid. However, this would have been a programming
+    /// error.
+    ///
     /// @param lease Structure representing a DHCPv6 lease.
     void append(const Lease6& lease) const;
 
@@ -49,12 +63,18 @@ public:
     /// message using @c CSVFile::setReadMsg and returns false. The error
     /// string may be read using @c CSVFile::getReadMsg.
     ///
+    /// This function is exception safe.
+    ///
     /// @param [out] lease Pointer to the lease read from CSV file or
     /// NULL pointer if lease hasn't been read.
     ///
     /// @return Boolean value indicating that the new lease has been
     /// read from the CSV file (if true), or that the error has occurred
     /// (false).
+    ///
+    /// @todo Make sure that the values read from the file are correct.
+    /// The appropriate @c Lease6 validation mechanism should be used once
+    /// ticket http://bind10.isc.org/ticket/2405 is implemented.
     bool next(Lease6Ptr& lease);
 
 private:

src/lib/dhcpsrv/tests/csv_lease_file6_unittest.cc

@@ -31,12 +31,17 @@ using namespace isc::util;
 
 namespace {
 
+// DUID values used by unit tests.
 const uint8_t DUID0[] = { 0, 1, 2, 3, 4, 5, 6, 0xa, 0xb, 0xc, 0xd, 0xe, 0xf };
 const uint8_t DUID1[] = { 1, 1, 1, 1, 0xa, 1, 2, 3, 4, 5 };
 
+/// @brief Test fixture class for @c CSVLeaseFile6 validation.
 class CSVLeaseFile6Test : public ::testing::Test {
 public:
 
+    /// @brief Constructor.
+    ///
+    /// Initializes IO for lease file used by unit tests.
     CSVLeaseFile6Test();
 
     /// @brief Prepends the absolute path to the file specified
@@ -46,11 +51,19 @@ public:
     /// @return Absolute path to the test file.
     static std::string absolutePath(const std::string& filename);
 
+    /// @brief Create DUID object from the binary.
+    ///
+    /// @param duid Binary value representing a DUID.
+    /// @param size Size of the DUID.
+    /// @return Pointer to the @c DUID object.
     DuidPtr makeDUID(const uint8_t* duid, const unsigned int size) const {
         return (DuidPtr(new DUID(duid, size)));
     }
 
+    /// @brief Name of the test lease file.
     std::string filename_;
+
+    /// @brief Object providing access to lease file IO.
     LeaseFileIO io_;
 
 };
@@ -66,16 +79,19 @@ CSVLeaseFile6Test::absolutePath(const std::string& filename) {
     return (s.str());
 }
 
+// This test checks the capability to read and parse leases from the file.
 TEST_F(CSVLeaseFile6Test, parse) {
+    // Open the lease file.
     boost::scoped_ptr<CSVLeaseFile6>
         lf(new CSVLeaseFile6(absolutePath("leases6_0.csv")));
     ASSERT_NO_THROW(lf->open());
 
     Lease6Ptr lease;
-    bool lease_read = false;
-    ASSERT_NO_THROW(lease_read = lf->next(lease));
+    // Reading first read should be successful.
+    EXPECT_TRUE(lf->next(lease));
     ASSERT_TRUE(lease);
-    EXPECT_TRUE(lease_read);
+
+    // Verify that the lease attributes are correct.
     EXPECT_EQ("2001:db8:1::1", lease->addr_.toText());
     ASSERT_TRUE(lease->duid_);
     EXPECT_EQ("00:01:02:03:04:05:06:0a:0b:0c:0d:0e:0f", lease->duid_->toText());
@@ -90,11 +106,14 @@ TEST_F(CSVLeaseFile6Test, parse) {
     EXPECT_TRUE(lease->fqdn_rev_);
     EXPECT_EQ("host.example.com", lease->hostname_);
 
-    EXPECT_FALSE(lease_read = lf->next(lease));
+    // Second lease is malformed - DUID is empty.
+    EXPECT_FALSE(lf->next(lease));
 
-    ASSERT_NO_THROW(lease_read = lf->next(lease));
+    // Even, parsing previous lease failed, reading the next lease should be
+    // successful.
+    EXPECT_TRUE(lf->next(lease));
     ASSERT_TRUE(lease);
-    EXPECT_TRUE(lease_read);
+    // Verify that the third lease is correct.
     EXPECT_EQ("2001:db8:2::10", lease->addr_.toText());
     ASSERT_TRUE(lease->duid_);
     EXPECT_EQ("01:01:01:01:0a:01:02:03:04:05", lease->duid_->toText());
@@ -109,11 +128,10 @@ TEST_F(CSVLeaseFile6Test, parse) {
     EXPECT_FALSE(lease->fqdn_rev_);
     EXPECT_TRUE(lease->hostname_.empty());
 
-    EXPECT_FALSE(lease_read = lf->next(lease));
-
-    ASSERT_NO_THROW(lease_read = lf->next(lease));
+    // Reading fourth lease should be successful.
+    EXPECT_TRUE(lf->next(lease));
     ASSERT_TRUE(lease);
-    EXPECT_TRUE(lease_read);
+    // Verify that lease is correct.
     EXPECT_EQ("3000:1::", lease->addr_.toText());
     ASSERT_TRUE(lease->duid_);
     EXPECT_EQ("00:01:02:03:04:05:06:0a:0b:0c:0d:0e:0f", lease->duid_->toText());
@@ -128,11 +146,18 @@ TEST_F(CSVLeaseFile6Test, parse) {
     EXPECT_FALSE(lease->fqdn_rev_);
     EXPECT_TRUE(lease->hostname_.empty());
 
-    ASSERT_NO_THROW(lease_read = lf->next(lease));
-    EXPECT_TRUE(lease_read);
+    // There are no more leases. Reading should cause no error, but the returned
+    // lease pointer should be NULL.
+    EXPECT_TRUE(lf->next(lease));
     EXPECT_FALSE(lease);
+
+    // We should be able to do it again.
+    EXPECT_TRUE(lf->next(lease));
+    EXPECT_FALSE(lease);
+
 }
 
+// This test checks creation of the lease file and writing leases.
 TEST_F(CSVLeaseFile6Test, recreate) {
     boost::scoped_ptr<CSVLeaseFile6> lf(new CSVLeaseFile6(filename_));
     ASSERT_NO_THROW(lf->recreate());
@@ -170,4 +195,10 @@ TEST_F(CSVLeaseFile6Test, recreate) {
               io_.readFile());
 }
 
+/// @todo Currently we don't check invalid lease attributes, such as invalid
+/// lease type, invalid preferred lifetime vs valid lifetime etc. The Lease6
+/// should be extended with the function that validates lease attributes. Once
+/// this is implemented we should provide more tests for malformed leases
+/// in the CSV file. See http://bind10.isc.org/ticket/2405.
+
 } // end of anonymous namespace

src/lib/dhcpsrv/tests/lease_file_io.cc

@@ -26,7 +26,7 @@ LeaseFileIO::LeaseFileIO(const std::string& filename)
 }
 
 LeaseFileIO::~LeaseFileIO() {
-    //    removeFile();
+    removeFile();
 }
 
 bool

src/lib/dhcpsrv/tests/testdata/leases6_0.csv

@@ -2,5 +2,4 @@ address,duid,valid_lifetime,expire,subnet_id,pref_lifetime,lease_type,iaid,prefi
 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
 2001:db8:1::1,,200,200,8,100,0,7,0,1,1,host.example.com
 2001:db8:2::10,01:01:01:01:0a:01:02:03:04:05,300,300,6,150,0,8,0,0,0,
-2001:db8:1::1,00:01:02:03:04:05:06:0a:0b:0c:0d:0e:0f,200,200,8,100,0,7,10,1,1,host.example.com
 3000:1::,00:01:02:03:04:05:06:0a:0b:0c:0d:0e:0f,0,200,8,0,2,16,64,0,0,