From: Marcin Siodelski Date: Mon, 24 Mar 2014 09:08:33 +0000 (+0100) Subject: [3360] Cleanup the CSV file parser for DHCPv6 leases. X-Git-Tag: bind10-1.2.0rc1-release~14^2~15 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=587f21a5bc1243c06b52bb4b70d23b7dc4300645;p=thirdparty%2Fkea.git [3360] Cleanup the CSV file parser for DHCPv6 leases. --- diff --git a/src/lib/dhcpsrv/csv_lease_file6.cc b/src/lib/dhcpsrv/csv_lease_file6.cc index 886ea0805f..0f832b1752 100644 --- a/src/lib/dhcpsrv/csv_lease_file6.cc +++ b/src/lib/dhcpsrv/csv_lease_file6.cc @@ -13,7 +13,6 @@ // PERFORMANCE OF THIS SOFTWARE. #include -#include using namespace isc::asiolink; using namespace isc::util; @@ -26,22 +25,6 @@ CSVLeaseFile6::CSVLeaseFile6(const std::string& filename) initColumns(); } -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()); @@ -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 @@ -131,7 +137,7 @@ CSVLeaseFile6::readValid(const CSVRow& row) { uint32_t CSVLeaseFile6::readCltt(const CSVRow& row) { uint32_t cltt = row.readAndConvertAt(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 diff --git a/src/lib/dhcpsrv/csv_lease_file6.h b/src/lib/dhcpsrv/csv_lease_file6.h index 7e58470780..76faa81f16 100644 --- a/src/lib/dhcpsrv/csv_lease_file6.h +++ b/src/lib/dhcpsrv/csv_lease_file6.h @@ -20,7 +20,6 @@ #include #include #include -#include #include #include @@ -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: diff --git a/src/lib/dhcpsrv/tests/csv_lease_file6_unittest.cc b/src/lib/dhcpsrv/tests/csv_lease_file6_unittest.cc index e15696999d..18f294b2de 100644 --- a/src/lib/dhcpsrv/tests/csv_lease_file6_unittest.cc +++ b/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 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 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 diff --git a/src/lib/dhcpsrv/tests/lease_file_io.cc b/src/lib/dhcpsrv/tests/lease_file_io.cc index 7f7b459bdb..8ba00d9500 100644 --- a/src/lib/dhcpsrv/tests/lease_file_io.cc +++ b/src/lib/dhcpsrv/tests/lease_file_io.cc @@ -26,7 +26,7 @@ LeaseFileIO::LeaseFileIO(const std::string& filename) } LeaseFileIO::~LeaseFileIO() { - // removeFile(); + removeFile(); } bool diff --git a/src/lib/dhcpsrv/tests/testdata/leases6_0.csv b/src/lib/dhcpsrv/tests/testdata/leases6_0.csv index 3e3eaab3b5..764ee8f13a 100644 --- a/src/lib/dhcpsrv/tests/testdata/leases6_0.csv +++ b/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,