[3382] Addressed review comments

Added tests for Y2038 timestamps.
Added missing commentary.
Minor cleanup.

src/lib/dhcpsrv/database_backends.dox

@@ -22,16 +22,22 @@
   the abstract isc::dhcp::LeaseMgr class.  This provides methods to
   create, retrieve, modify and delete leases in the database.
 
-  There are currently two available Lease Managers, MySQL and Memfile:
+  There are currently three available Lease Managers, Memfile, MySQL and
+  PostgreSQL:
+
+  - Memfile is an in-memory lease database which can be configured to persist
+  its content to disk in a flat-file.  Support for the Memfile database
+  backend is built into Kea DHCP.
 
   - The MySQL lease manager uses the freely available MySQL as its backend
-  database.  This is not included in BIND 10 DHCP by default:
+  database.  This is not included in Kea DHCP by default:
   the \--with-dhcp-mysql switch must be supplied to "configure" for support
   to be compiled into the software.
-  - Memfile is an in-memory lease database, with (currently) nothing being
-  written to persistent storage.  The long-term plans for the backend do
-  include the ability to store this on disk, but it is currently a
-  low-priority item.
+
+  - The PostgreSQL lease manager uses the freely available PostgreSQL as its
+  backend database.  This is not included in Kea DHCP by default:
+  the \--with-dhcp-pgsql switch must be supplied to "configure" for
+  support to be compiled into the software.
 
   @section dhcpdb-instantiation Instantiation of Lease Managers
 
@@ -74,6 +80,16 @@
   - <b>user</b> - database user ID under which the database is accessed.  If not
     specified, no user ID is used - the database is assumed to be open.
 
+  @subsection dhcpdb-keywords-pgsql PostgreSQL connection string keywords
+
+  - <b>host</b> - host on which the selected database is running.  If not
+  supplied, "localhost" is assumed.
+  - <b>name</b> - name of the PostgreSQL database to access.  There is no
+  default - this must always be supplied.
+  - <b>password</b> - password for the selected user ID (see below).  If not
+  specified, no password is used.
+  - <b>user</b> - database user ID under which the database is accessed.  If not
+    specified, no user ID is used - the database is assumed to be open.
 
   @section dhcp-backend-unittest Running Unit Tests
 
@@ -81,7 +97,7 @@
   certain database-specific pre-requisites for successfully running the unit
   tests.  These are listed in the following sections.
 
-  @subsection dhcp-mysql-unittest MySQL
+  @subsection dhcp-mysql-unittest MySQL Unit Tests
 
   A database called <i>keatest</i> must be created. A database user, also called
   <i>keatest</i> (and with a password <i>keatest</i>) must also be created and
@@ -122,7 +138,7 @@
   that BIND 10 has been build with the \--with-dhcp-mysql switch (see the installation
   section in the <a href="http://bind10.isc.org/docs/bind10-guide.html">BIND 10 Guide</a>).
 
- @subsection dhcp-pgsql-unittest PostgreSQL unit-tests
+ @subsection dhcp-pgsql-unittest PostgreSQL Unit Tests
 
   Conceptually, the steps required to run PostgreSQL unit-tests are the same as
   in MySQL. First, a database called <i>keatest</i> must be created. A database

src/lib/dhcpsrv/lease_mgr.cc

@@ -32,6 +32,8 @@ using namespace std;
 namespace isc {
 namespace dhcp {
 
+const time_t LeaseMgr::MAX_DB_TIME = 2147483647;
+
 std::string LeaseMgr::getParameter(const std::string& name) const {
     ParameterMap::const_iterator param = parameters_.find(name);
     if (param == parameters_.end()) {

src/lib/dhcpsrv/lease_mgr.h

@@ -122,6 +122,10 @@ public:
 /// see the documentation of those classes for details.
 class LeaseMgr {
 public:
+    /// @brief Defines maxiumum value for time that can be reliably stored.
+    // If I'm still alive I'll be too old to care. You fix it.
+    static const time_t MAX_DB_TIME;
+
     /// Database configuration parameter map
     typedef std::map<std::string, std::string> ParameterMap;
 

src/lib/dhcpsrv/pgsql_lease_mgr.cc

@@ -268,6 +268,7 @@ std::string PsqlBindArray::toText() {
 class PgSqlLeaseExchange {
 public:
 
+
     PgSqlLeaseExchange()
         : addr_str_(""), valid_lifetime_(0), valid_lft_str_(""),
          expire_(0), expire_str_(""), subnet_id_(0), subnet_id_str_(""),
@@ -289,6 +290,14 @@ public:
     /// @return std::string containing the stringified time
     std::string
     convertToDatabaseTime(const time_t& time_val) {
+        // PostgreSQL does funny things with time if you get past Y2038.  It
+        // will accept the values (unlike MySQL which throws) but it
+        // stops correctly adjusting to local time when reading them back
+        // out. So lets disallow it here.
+        if (time_val > LeaseMgr::MAX_DB_TIME) {
+            isc_throw(BadValue, "Time value is too large: " << time_val);
+        }
+
         struct tm tinfo;
         char buffer[20];
         localtime_r(&time_val, &tinfo);
@@ -326,10 +335,11 @@ public:
     ///
     /// @return a const char* pointer to the column's raw data
     /// @throw  DbOperationError if the value cannot be fetched.
-    const char* getColumnValue(PGresult*& r, const int row, const size_t col) {
+    const char* getRawColumnValue(PGresult*& r, const int row,
+                                  const size_t col) {
         const char* value = PQgetvalue(r, row, col);
         if (!value) {
-            isc_throw(DbOperationError, "getColumnValue no data for :"
+            isc_throw(DbOperationError, "getRawColumnValue no data for :"
                       << getColumnLabel(col) << " row:" << row);
         }
 
@@ -347,7 +357,7 @@ public:
     /// invalid.
     void getColumnValue(PGresult*& r, const int row, const size_t col,
                         bool &value) {
-        const char* data = getColumnValue(r, row, col);
+        const char* data = getRawColumnValue(r, row, col);
         if (!strlen(data) || *data == 'f') {
             value = false;
         } else if (*data == 't') {
@@ -370,7 +380,7 @@ public:
     /// invalid.
     void getColumnValue(PGresult*& r, const int row, const size_t col,
                         uint32_t &value) {
-        const char* data = getColumnValue(r, row, col);
+        const char* data = getRawColumnValue(r, row, col);
         try {
             value = boost::lexical_cast<uint32_t>(data);
         } catch (const std::exception& ex) {
@@ -391,7 +401,7 @@ public:
     /// invalid.
     void getColumnValue(PGresult*& r, const int row, const size_t col,
                         uint8_t &value) {
-        const char* data = getColumnValue(r, row, col);
+        const char* data = getRawColumnValue(r, row, col);
         try {
             // lexically casting as uint8_t doesn't convert from char
             // so we use uint16_t and implicitly convert.
@@ -458,7 +468,7 @@ public:
         // Returns converted bytes in a dynamically allocated buffer, and
         // sets bytes_converted.
         unsigned char* bytes = PQunescapeBytea((const unsigned char*)
-                                               (getColumnValue(r, row, col)),
+                                               (getRawColumnValue(r, row, col)),
                                                &bytes_converted);
 
         // Unlikely it couldn't allocate it but you never know.
@@ -649,8 +659,8 @@ public:
 
             getColumnValue(r, row, VALID_LIFETIME_COL, valid_lifetime_);
 
-            expire_ = convertFromDatabaseTime(getColumnValue(r, row,
-                                                             EXPIRE_COL));
+            expire_ = convertFromDatabaseTime(getRawColumnValue(r, row,
+                                                                EXPIRE_COL));
 
             getColumnValue(r, row , SUBNET_ID_COL, subnet_id_);
 
@@ -659,7 +669,7 @@ public:
             getColumnValue(r, row, FQDN_FWD_COL, fqdn_fwd_);
             getColumnValue(r, row, FQDN_REV_COL, fqdn_rev_);
 
-            hostname_ = getColumnValue(r, row, HOSTNAME_COL);
+            hostname_ = getRawColumnValue(r, row, HOSTNAME_COL);
 
             return (Lease4Ptr(new Lease4(addr4_, hwaddr_buffer_, hwaddr_length_,
                                          client_id_buffer_, client_id_length_,
@@ -761,7 +771,11 @@ public:
             addr_str_ = lease_->addr_.toText();
             bind_array.add(addr_str_);
 
-            bind_array.add(lease_->duid_->getDuid());
+            if (lease_->duid_) {
+                bind_array.add(lease_->duid_->getDuid());
+            } else {
+                isc_throw (BadValue, "IPv6 Lease cannot have a null DUID");
+            }
 
             valid_lft_str_ = boost::lexical_cast<std::string>
                              (lease->valid_lft_);
@@ -819,8 +833,8 @@ public:
 
             getColumnValue(r, row, VALID_LIFETIME_COL, valid_lifetime_);
 
-            expire_ = convertFromDatabaseTime(getColumnValue(r, row,
-                                                             EXPIRE_COL));
+            expire_ = convertFromDatabaseTime(getRawColumnValue(r, row,
+                                                                EXPIRE_COL));
 
             cltt_ = expire_ - valid_lifetime_;
 
@@ -837,7 +851,7 @@ public:
             getColumnValue(r, row, FQDN_FWD_COL, fqdn_fwd_);
             getColumnValue(r, row, FQDN_REV_COL, fqdn_rev_);
 
-            hostname_ = getColumnValue(r, row, HOSTNAME_COL);
+            hostname_ = getRawColumnValue(r, row, HOSTNAME_COL);
 
             Lease6Ptr result(new Lease6(lease_type_, addr, duid_ptr, iaid_,
                                         pref_lifetime_, valid_lifetime_, 0, 0,
@@ -863,7 +877,7 @@ public:
     /// invalid.
     isc::asiolink::IOAddress getIPv6Value(PGresult*& r, const int row,
                                           const size_t col) {
-        const char* data = getColumnValue(r, row, col);
+        const char* data = getRawColumnValue(r, row, col);
         try {
             return (isc::asiolink::IOAddress(data));
         } catch (const std::exception& ex) {

src/lib/dhcpsrv/pgsql_lease_mgr.h

@@ -28,7 +28,7 @@ namespace isc {
 namespace dhcp {
 
 /// @brief Structure used to bind C++ input values to dynamic SQL parameters
-/// The structure contains three vectors which storing the input values,
+/// The structure contains three vectors which store the input values,
 /// data lengths, and formats.  These vectors are passed directly into the
 /// PostgreSQL execute call.
 ///
@@ -69,19 +69,37 @@ struct PsqlBindArray {
         return (values_.empty());
     }
 
-    /// @brief Adds a char value to the array.
+    /// @brief Adds a char array to bind array based
+    ///
+    /// Adds a TEXT_FMT value to the end of the bind array, using the given
+    /// char* as the data source. Note that value is expected to be NULL
+    /// terminated.
+    ///
+    /// @param value char array containing the null-terminated text to add.
     void add(const char* value);
 
-    /// @brief Adds a string value to the array.
+    /// @brief Adds an string value to the bind array
+    ///
+    /// Adds a TEXT formatted value to the end of the bind array using the
+    /// given string as the data source.
+    ///
+    /// @param value std::string containing the value to add.
     void add(const std::string& value);
 
-    /// @brief Adds a vector to the array.
+    /// @brief Adds a binary value to the bind array.
+    ///
+    /// Adds a BINARY_FMT value to the end of the bind array using the
+    /// given vector as the data source.
+    ///
+    /// @param value vector of binary bytes.
     void add(const std::vector<uint8_t>& data);
 
-    /// @brief Adds a uint32_t value to the array.
-    void add(const uint32_t& value);
-
-    /// @brief Adds a bool value to the array.
+    /// @brief Adds a boolean value to the bind array.
+    ///
+    /// Converts the given boolean value to its corresponding to PostgreSQL
+    /// string value and adds it as a TEXT_FMT value to the bind array.
+    ///
+    /// @param value std::string containing the value to add.
     void add(const bool& value);
 
     /// @brief Dumps the contents of the array to a string.
@@ -370,16 +388,14 @@ public:
 
     /// @brief Commit Transactions
     ///
-    /// Commits all pending database operations.  On databases that don't
-    /// support transactions, this is a no-op.
+    /// Commits all pending database operations.
     ///
     /// @throw DbOperationError Iif the commit failed.
     virtual void commit();
 
     /// @brief Rollback Transactions
     ///
-    /// Rolls back all pending database operations.  On databases that don't
-    /// support transactions, this is a no-op.
+    /// Rolls back all pending database operations.
     ///
     /// @throw DbOperationError If the rollback failed.
     virtual void rollback();

src/lib/dhcpsrv/tests/generic_lease_mgr_unittest.cc

@@ -682,6 +682,29 @@ GenericLeaseMgrTest::testAddGetDelete6(bool check_t1_t2) {
     EXPECT_FALSE(x);
 }
 
+void
+GenericLeaseMgrTest::testMaxDate4() {
+    // Get the leases to be used for the test.
+    vector<Lease4Ptr> leases = createLeases4();
+    Lease4Ptr lease = leases[1];
+
+    // Set valid_lft_ to 1 day, cllt_ to max time. This should make expire
+    // time too large to store.
+    lease->valid_lft_ = 24*60*60;
+    lease->cltt_ = LeaseMgr::MAX_DB_TIME;
+
+    // Insert should throw.
+    ASSERT_THROW(lmptr_->addLease(leases[1]), DbOperationError);
+
+    // Set valid_lft_ to 0, which should make expire time small enough to
+    // store and try again.
+    lease->valid_lft_ = 0;
+    EXPECT_TRUE(lmptr_->addLease(leases[1]));
+    Lease4Ptr l_returned = lmptr_->getLease4(ioaddress4_[1]);
+    ASSERT_TRUE(l_returned);
+    detailCompareLease(leases[1], l_returned);
+}
+
 void
 GenericLeaseMgrTest::testBasicLease4() {
     // Get the leases to be used for the test.
@@ -710,7 +733,6 @@ GenericLeaseMgrTest::testBasicLease4() {
     detailCompareLease(leases[3], l_returned);
 
     // Check that we can't add a second lease with the same address
-std::cout << "OK - Duplicate check here!!!!!!" << std::endl;
     EXPECT_FALSE(lmptr_->addLease(leases[1]));
 
     // Delete a lease, check that it's gone, and that we can't delete it
@@ -829,6 +851,29 @@ GenericLeaseMgrTest::testBasicLease6() {
     detailCompareLease(leases[2], l_returned);
 }
 
+void
+GenericLeaseMgrTest::testMaxDate6() {
+    // Get the leases to be used for the test.
+    vector<Lease6Ptr> leases = createLeases6();
+    Lease6Ptr lease = leases[1];
+
+    // Set valid_lft_ to 1 day, cllt_ to max time. This should make expire
+    // time too large to store.
+    lease->valid_lft_ = 24*60*60;
+    lease->cltt_ = LeaseMgr::MAX_DB_TIME;
+
+    // Insert should throw.
+    ASSERT_THROW(lmptr_->addLease(leases[1]), DbOperationError);
+
+    // Set valid_lft_ to 0, which should make expire time small enough to
+    // store and try again.
+    lease->valid_lft_ = 0;
+    EXPECT_TRUE(lmptr_->addLease(leases[1]));
+    Lease6Ptr l_returned = lmptr_->getLease6(leasetype6_[1], ioaddress6_[1]);
+    ASSERT_TRUE(l_returned);
+    detailCompareLease(leases[1], l_returned);
+}
+
 void
 GenericLeaseMgrTest::testLease4InvalidHostname() {
     // Get the leases to be used for the test.

src/lib/dhcpsrv/tests/generic_lease_mgr_unittest.h

@@ -105,6 +105,9 @@ public:
     /// @brief checks that addLease, getLease4(addr) and deleteLease() works
     void testBasicLease4();
 
+    /// @brief checks that invalid dates are safely handled.
+    void testMaxDate4();
+
     /// @brief Test lease retrieval using client id.
     void testGetLease4ClientId();
 
@@ -180,6 +183,10 @@ public:
     /// IPv6 address) works.
     void testBasicLease6();
 
+    /// @brief checks that invalid dates are safely handled.
+    void testMaxDate6();
+
+
     /// @brief Test that IPv6 lease can be added, retrieved and deleted.
     ///
     /// This method checks basic IPv6 lease operations. There's check_t1_t2

src/lib/dhcpsrv/tests/mysql_lease_mgr_unittest.cc

@@ -333,6 +333,11 @@ TEST_F(MySqlLeaseMgrTest, basicLease4) {
     testBasicLease4();
 }
 
+/// @brief Check that Lease4 code safely handles invalid dates.
+TEST_F(MySqlLeaseMgrTest, maxDate4) {
+    testMaxDate4();
+}
+
 /// @brief Lease4 update tests
 ///
 /// Checks that we are able to update a lease in the database.
@@ -437,6 +442,11 @@ TEST_F(MySqlLeaseMgrTest, basicLease6) {
     testBasicLease6();
 }
 
+/// @brief Check that Lease6 code safely handles invalid dates.
+TEST_F(MySqlLeaseMgrTest, maxDate6) {
+    testMaxDate6();
+}
+
 /// @brief Verify that too long hostname for Lease6 is not accepted.
 ///
 /// Checks that the it is not possible to create a lease when the hostname

src/lib/dhcpsrv/tests/pgsql_lease_mgr_unittest.cc

@@ -299,6 +299,11 @@ TEST_F(PgSqlLeaseMgrTest, basicLease4) {
     testBasicLease4();
 }
 
+/// @brief Check that Lease4 code safely handles invalid dates.
+TEST_F(PgSqlLeaseMgrTest, maxDate4) {
+    testMaxDate4();
+}
+
 /// @brief Lease4 update tests
 ///
 /// Checks that we are able to update a lease in the database.
@@ -403,6 +408,11 @@ TEST_F(PgSqlLeaseMgrTest, basicLease6) {
     testBasicLease6();
 }
 
+/// @brief Check that Lease6 code safely handles invalid dates.
+TEST_F(PgSqlLeaseMgrTest, maxDate6) {
+    testMaxDate6();
+}
+
 /// @brief Verify that too long hostname for Lease6 is not accepted.
 ///
 /// Checks that the it is not possible to create a lease when the hostname