BIND 10 trac3360, updated. 587f21a5bc1243c06b52bb4b70d23b7dc4300645 [3360] Cleanup the CSV file parser for DHCPv6 leases.
BIND 10 source code commits
bind10-changes at lists.isc.org
Mon Mar 24 09:08:40 UTC 2014
The branch, trac3360 has been updated
via 587f21a5bc1243c06b52bb4b70d23b7dc4300645 (commit)
from fbae37a0d687905cc456031131ec020f8182c16d (commit)
Those revisions listed above that are new to this repository have
not appeared on any other notification email; so we list those
revisions in full, below.
- Log -----------------------------------------------------------------
commit 587f21a5bc1243c06b52bb4b70d23b7dc4300645
Author: Marcin Siodelski <marcin at isc.org>
Date: Mon Mar 24 10:08:33 2014 +0100
[3360] Cleanup the CSV file parser for DHCPv6 leases.
-----------------------------------------------------------------------
Summary of changes:
src/lib/dhcpsrv/csv_lease_file6.cc | 52 ++++++++++---------
src/lib/dhcpsrv/csv_lease_file6.h | 22 ++++++++-
src/lib/dhcpsrv/tests/csv_lease_file6_unittest.cc | 55 ++++++++++++++++-----
src/lib/dhcpsrv/tests/lease_file_io.cc | 2 +-
src/lib/dhcpsrv/tests/testdata/leases6_0.csv | 1 -
5 files changed, 93 insertions(+), 39 deletions(-)
-----------------------------------------------------------------------
diff --git a/src/lib/dhcpsrv/csv_lease_file6.cc b/src/lib/dhcpsrv/csv_lease_file6.cc
index 886ea08..0f832b1 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 <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
diff --git a/src/lib/dhcpsrv/csv_lease_file6.h b/src/lib/dhcpsrv/csv_lease_file6.h
index 7e58470..76faa81 100644
--- a/src/lib/dhcpsrv/csv_lease_file6.h
+++ b/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:
diff --git a/src/lib/dhcpsrv/tests/csv_lease_file6_unittest.cc b/src/lib/dhcpsrv/tests/csv_lease_file6_unittest.cc
index e156969..18f294b 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<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
diff --git a/src/lib/dhcpsrv/tests/lease_file_io.cc b/src/lib/dhcpsrv/tests/lease_file_io.cc
index 7f7b459..8ba00d9 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 3e3eaab..764ee8f 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,
More information about the bind10-changes
mailing list