[#897] Addressed comments

12 files changed, 126 insertions, 88 deletions

diff --git a/src/lib/dhcpsrv/alloc_engine.cc b/src/lib/dhcpsrv/alloc_engine.cc
index 040908c4fc..f096aa6a50 100644
--- a/src/lib/dhcpsrv/alloc_engine.cc
+++ b/src/lib/dhcpsrv/alloc_engine.cc
@@ -3549,20 +3549,22 @@ AllocEngine::createLease4(const ClientContext4& ctx, const IOAddress& addr,
         isc_throw(BadValue, "Can't create a lease without a subnet");
     }
 
-    // Use the dhcp-lease-time content or the default for lease length.
-    uint32_t valid_lft = ctx.subnet_->getValid();
-    OptionPtr opt = ctx.query_->getOption(DHO_DHCP_LEASE_TIME);
-    OptionUint32Ptr opt_lft;
-    if (opt) {
-        opt_lft = boost::dynamic_pointer_cast<OptionInt<uint32_t> >(opt);
-    }
-    if (opt_lft) {
-        valid_lft = ctx.subnet_->getValid().get(opt_lft->getValue());
-    }
-
-    // BOOTP uses infinite valid lifetime.
+    uint32_t valid_lft;
     if (ctx.query_->inClass("BOOTP")) {
+        // BOOTP uses infinite valid lifetime.
         valid_lft = Lease::INFINITY_LFT;
+    } else {
+        // Use the dhcp-lease-time content or the default for lease length.
+        OptionPtr opt = ctx.query_->getOption(DHO_DHCP_LEASE_TIME);
+        OptionUint32Ptr opt_lft;
+        if (opt) {
+            opt_lft = boost::dynamic_pointer_cast<OptionInt<uint32_t> >(opt);
+        }
+        if (opt_lft) {
+            valid_lft = ctx.subnet_->getValid().get(opt_lft->getValue());
+        } else {
+            valid_lft = ctx.subnet_->getValid();
+        }
     }
 
     time_t now = time(NULL);
@@ -3986,20 +3988,21 @@ AllocEngine::updateLease4Information(const Lease4Ptr& lease,
     lease->hwaddr_ = ctx.hwaddr_;
     lease->client_id_ = ctx.subnet_->getMatchClientId() ? ctx.clientid_ : ClientIdPtr();
     lease->cltt_ = time(NULL);
-    // Use the dhcp-lease-time content or the default for lease length.
-    OptionPtr opt = ctx.query_->getOption(DHO_DHCP_LEASE_TIME);
-    OptionUint32Ptr opt_lft;
-    if (opt) {
-        opt_lft = boost::dynamic_pointer_cast<OptionInt<uint32_t> >(opt);
-    }
-    if (opt_lft) {
-        lease->valid_lft_ = ctx.subnet_->getValid().get(opt_lft->getValue());
-    } else {
-        lease->valid_lft_ = ctx.subnet_->getValid();
-    }
-    // BOOTP uses infinite valid lifetime.
     if (ctx.query_->inClass("BOOTP")) {
+        // BOOTP uses infinite valid lifetime.
         lease->valid_lft_ = Lease::INFINITY_LFT;
+    } else {
+        // Use the dhcp-lease-time content or the default for lease length.
+        OptionPtr opt = ctx.query_->getOption(DHO_DHCP_LEASE_TIME);
+        OptionUint32Ptr opt_lft;
+        if (opt) {
+            opt_lft = boost::dynamic_pointer_cast<OptionInt<uint32_t> >(opt);
+        }
+        if (opt_lft) {
+            lease->valid_lft_ = ctx.subnet_->getValid().get(opt_lft->getValue());
+        } else {
+            lease->valid_lft_ = ctx.subnet_->getValid();
+        }
     }
     lease->fqdn_fwd_ = ctx.fwd_dns_update_;
     lease->fqdn_rev_ = ctx.rev_dns_update_;
diff --git a/src/lib/dhcpsrv/cql_lease_mgr.cc b/src/lib/dhcpsrv/cql_lease_mgr.cc
index 8d7e298aaa..20bb1e42ed 100644
--- a/src/lib/dhcpsrv/cql_lease_mgr.cc
+++ b/src/lib/dhcpsrv/cql_lease_mgr.cc
@@ -304,8 +304,8 @@ StatementMap CqlLease4Exchange::tagged_statements_{
       "fqdn_fwd, fqdn_rev, hostname, state, user_context "
       "FROM lease4 "
       "WHERE state = ? "
-      "AND expire < ? "
       "AND valid_lifetime < 4294967295 "
+      "AND expire < ? "
       "LIMIT ? "
       "ALLOW FILTERING "}},
 
@@ -1019,8 +1019,8 @@ StatementMap CqlLease6Exchange::tagged_statements_ = {
       "hwaddr_source, state, user_context "
       "FROM lease6 "
       "WHERE state = ? "
-      "AND expire < ? "
       "AND valid_lifetime < 4294967295 "
+      "AND expire < ? "
       "LIMIT ? "
       "ALLOW FILTERING "}},
 
diff --git a/src/lib/dhcpsrv/database_backends.dox b/src/lib/dhcpsrv/database_backends.dox
index e7019accd6..9f0bcffe13 100644
--- a/src/lib/dhcpsrv/database_backends.dox
+++ b/src/lib/dhcpsrv/database_backends.dox
@@ -105,6 +105,23 @@
 
   For details, see @ref isc::db::CqlConnection::openDatabase().
 
+  @subsection infinite-valid-lifetime Infinite Valid Lifetime
+
+  The @c isc::dhcp::Lease class uses cltt (client last transmission time)
+  and valid lifetime, backend lease uses expire and valid lifetime.
+  These quantities are bound by the equation:
+  @code
+        expire = cltt + valid_lifetime
+  @endcode
+
+  But when expire is a 32 bit date and valid lifetime is the infinity
+  special value (0xffffffff) this overflows so for MySQL and PostgreSQL
+  backends this becomes:
+  @code
+        expire = cltt + valid_lifetime if valid_lifetime != 0xffffffff
+        expire = cltt if valid_lifetime == 0xffffffff
+  @endcode
+
   @section dhcpdb-host Host Backends
 
   Host backends (known also as host data sources) are similar to lease
diff --git a/src/lib/dhcpsrv/lease.cc b/src/lib/dhcpsrv/lease.cc
index 61b440e40c..3ac260af7b 100644
--- a/src/lib/dhcpsrv/lease.cc
+++ b/src/lib/dhcpsrv/lease.cc
@@ -25,6 +25,17 @@ const uint32_t Lease::STATE_DEFAULT = 0x0;
 const uint32_t Lease::STATE_DECLINED = 0x1;
 const uint32_t Lease::STATE_EXPIRED_RECLAIMED = 0x2;
 
+std::string
+Lease::lifetimeToText(uint32_t lifetime) {
+    ostringstream repr;
+    if (lifetime == INFINITY_LFT) {
+        repr << "infinity";
+    } else {
+        repr << lifetime;
+    }
+    return repr.str();
+}
+
 Lease::Lease(const isc::asiolink::IOAddress& addr,
              uint32_t valid_lft, SubnetID subnet_id, time_t cltt,
              const bool fqdn_fwd, const bool fqdn_rev,
@@ -528,13 +539,9 @@ Lease6::toText() const {
            << "Address:       " << addr_ << "\n"
            << "Prefix length: " << static_cast<int>(prefixlen_) << "\n"
            << "IAID:          " << iaid_ << "\n"
-           << "Pref life:     " << preferred_lft_ << "\n";
-    if (valid_lft_ != INFINITY_LFT) {
-        stream << "Valid life:    " << valid_lft_ << "\n";
-    } else {
-        stream << "Valid life:    " << "infinity" << "\n";
-    }
-    stream << "Cltt:          " << cltt_ << "\n"
+           << "Pref life:     " << lifetimeToText(preferred_lft_) << "\n"
+           << "Valid life:    " << lifetimeToText(valid_lft_) << "\n"
+           << "Cltt:          " << cltt_ << "\n"
            << "DUID:          " << (duid_?duid_->toText():"(none)") << "\n"
            << "Hardware addr: " << (hwaddr_?hwaddr_->toText(false):"(none)") << "\n"
            << "Subnet ID:     " << subnet_id_ << "\n"
@@ -551,13 +558,9 @@ std::string
 Lease4::toText() const {
     ostringstream stream;
 
-    stream << "Address:       " << addr_ << "\n";
-    if (valid_lft_ != INFINITY_LFT) {
-        stream << "Valid life:    " << valid_lft_ << "\n";
-    } else {
-        stream << "Valid life:    " << "infinity" << "\n";
-    }
-    stream << "Cltt:          " << cltt_ << "\n"
+    stream << "Address:       " << addr_ << "\n"
+           << "Valid life:    " << lifetimeToText(valid_lft_) << "\n"
+           << "Cltt:          " << cltt_ << "\n"
            << "Hardware addr: " << (hwaddr_ ? hwaddr_->toText(false) : "(none)") << "\n"
            << "Client id:     " << (client_id_ ? client_id_->toText() : "(none)") << "\n"
            << "Subnet ID:     " << subnet_id_ << "\n"
diff --git a/src/lib/dhcpsrv/lease.h b/src/lib/dhcpsrv/lease.h
index 4c82d0a5b9..7354e2fa69 100644
--- a/src/lib/dhcpsrv/lease.h
+++ b/src/lib/dhcpsrv/lease.h
@@ -37,6 +37,15 @@ struct Lease : public isc::data::UserContext, public isc::data::CfgToElement {
     /// @brief Infinity (means static, i.e. never expire)
     static const uint32_t INFINITY_LFT = 0xffffffff;
 
+    /// @brief Print lifetime
+    ///
+    /// This converts a lifetime to a string taking into account the
+    /// infinity special value.
+    ///
+    /// @param lifetime lifetime to print
+    /// @return a string representing the finite value or "infinity"
+    static std::string lifetimeToText(uint32_t lifetime);
+
     /// @brief Type of lease or pool
     typedef enum {
         TYPE_NA = 0, ///< the lease contains non-temporary IPv6 address
diff --git a/src/lib/dhcpsrv/mysql_lease_mgr.cc b/src/lib/dhcpsrv/mysql_lease_mgr.cc
index db64671b3d..ddb3c93cf6 100644
--- a/src/lib/dhcpsrv/mysql_lease_mgr.cc
+++ b/src/lib/dhcpsrv/mysql_lease_mgr.cc
@@ -174,8 +174,9 @@ tagged_statements = { {
                         "fqdn_fwd, fqdn_rev, hostname, "
                         "state, user_context "
                             "FROM lease4 "
-                            "WHERE state != ? AND expire < ?"
-                            " AND valid_lifetime != 4294967295 "
+                            "WHERE state != ?"
+                            " AND valid_lifetime != 4294967295"
+                            " AND expire < ? "
                             "ORDER BY expire ASC "
                             "LIMIT ?"},
     {MySqlLeaseMgr::GET_LEASE6,
@@ -260,8 +261,9 @@ tagged_statements = { {
                         "hwaddr, hwtype, hwaddr_source, "
                         "state, user_context "
                             "FROM lease6 "
-                            "WHERE state != ? AND expire < ?"
-                            " AND valid_lifetime != 4294967295 "
+                            "WHERE state != ?"
+                            " AND valid_lifetime != 4294967295"
+                            " AND expire < ? "
                             "ORDER BY expire ASC "
                             "LIMIT ?"},
     {MySqlLeaseMgr::INSERT_LEASE4,
diff --git a/src/lib/dhcpsrv/pgsql_lease_mgr.cc b/src/lib/dhcpsrv/pgsql_lease_mgr.cc
index 134a991c4d..77889db472 100644
--- a/src/lib/dhcpsrv/pgsql_lease_mgr.cc
+++ b/src/lib/dhcpsrv/pgsql_lease_mgr.cc
@@ -157,7 +157,7 @@ PgSqlTaggedStatement tagged_statements[] = {
         "fqdn_fwd, fqdn_rev, hostname, "
         "state, user_context "
       "FROM lease4 "
-      "WHERE state != $1 AND expire < $2 AND valid_lifetime != 4294967295 "
+      "WHERE state != $1 AND valid_lifetime != 4294967295 AND expire < $2 "
       "ORDER BY expire "
       "LIMIT $3"},
 
@@ -261,7 +261,7 @@ PgSqlTaggedStatement tagged_statements[] = {
         "hwaddr, hwtype, hwaddr_source, "
         "state, user_context "
       "FROM lease6 "
-      "WHERE state != $1 AND expire < $2 AND valid_lifetime != 4294967295 "
+      "WHERE state != $1 AND valid_lifetime != 4294967295 AND expire < $2 "
       "ORDER BY expire "
       "LIMIT $3"},
 
@@ -496,9 +496,10 @@ public:
             // Avoid overflow
             uint32_t valid_lft = lease_->valid_lft_;
             if (valid_lft == Lease::INFINITY_LFT) {
-                valid_lft = 0;
+                expire_str_ = convertToDatabaseTime(lease->cltt_, 0);
+            } else {
+                expire_str_ = convertToDatabaseTime(lease->cltt_, valid_lft);
             }
-            expire_str_ = convertToDatabaseTime(lease->cltt_, valid_lft);
             bind_array.add(expire_str_);
 
             subnet_id_str_ = boost::lexical_cast<std::string>(lease->subnet_id_);
@@ -556,9 +557,10 @@ public:
             // Recover from overflow
             uint32_t valid_lft = valid_lifetime_;
             if (valid_lft == Lease::INFINITY_LFT) {
-                valid_lft = 0;
+                cltt_ = expire_;
+            } else {
+                cltt_ = expire_ - valid_lft;
             }
-            cltt_ = expire_ - valid_lft;
 
             getColumnValue(r, row, FQDN_FWD_COL, fqdn_fwd_);
 
@@ -737,9 +739,10 @@ public:
             // Avoid overflow
             uint32_t valid_lft = lease_->valid_lft_;
             if (valid_lft == Lease::INFINITY_LFT) {
-                valid_lft = 0;
+                expire_str_ = convertToDatabaseTime(lease->cltt_, 0);
+            } else {
+                expire_str_ = convertToDatabaseTime(lease->cltt_, valid_lft);
             }
-            expire_str_ = convertToDatabaseTime(lease->cltt_, valid_lft);
             bind_array.add(expire_str_);
 
             subnet_id_str_ = boost::lexical_cast<std::string>(lease->subnet_id_);
@@ -848,9 +851,10 @@ public:
             // Recover from overflow
             uint32_t valid_lft = valid_lifetime_;
             if (valid_lft == Lease::INFINITY_LFT) {
-                valid_lft = 0;
+                cltt_ = expire_;
+            } else {
+                cltt_ = expire_ - valid_lft;
             }
-            cltt_ = expire_ - valid_lft;
 
             getColumnValue(r, row , SUBNET_ID_COL, subnet_id_);
 
diff --git a/src/lib/dhcpsrv/tests/cql_lease_mgr_unittest.cc b/src/lib/dhcpsrv/tests/cql_lease_mgr_unittest.cc
index 272064768c..1f8c69d3fa 100644
--- a/src/lib/dhcpsrv/tests/cql_lease_mgr_unittest.cc
+++ b/src/lib/dhcpsrv/tests/cql_lease_mgr_unittest.cc
@@ -625,10 +625,10 @@ TEST_F(CqlLeaseMgrTest, getExpiredLeases4) {
     testCqlGetExpiredLeases4();
 }
 
-/// @brief Checks that the static (i.e. with infinite valid lifetime) DHCPv4
-/// leases will never expire.
-TEST_F(CqlLeaseMgrTest, staticLeases4) {
-    testStaticLeases4();
+/// @brief Checks that DHCPv4 leases with infinite valid lifetime
+/// will never expire.
+TEST_F(CqlLeaseMgrTest, getNeverExpiredLeases4) {
+    testGetNeverExpiredLeases4();
 }
 
 /// @brief Check that expired reclaimed DHCPv4 leases are removed.
@@ -780,10 +780,10 @@ TEST_F(CqlLeaseMgrTest, getExpiredLeases6) {
     testCqlGetExpiredLeases6();
 }
 
-/// @brief Checks that the static (i.e. with infinite valid lifetime) DHCPv6
-/// leases will never expire.
-TEST_F(CqlLeaseMgrTest, staticLeases6) {
-    testStaticLeases6();
+/// @brief Checks that DHCPv6 leases with infinite valid lifetime
+/// will never expire.
+TEST_F(CqlLeaseMgrTest, getNeverExpiredLeases6) {
+    testGetNeverExpiredLeases6();
 }
 
 /// @brief Check that expired reclaimed DHCPv6 leases are removed.
diff --git a/src/lib/dhcpsrv/tests/generic_lease_mgr_unittest.cc b/src/lib/dhcpsrv/tests/generic_lease_mgr_unittest.cc
index c60b0b6ac2..31ca47e9e9 100644
--- a/src/lib/dhcpsrv/tests/generic_lease_mgr_unittest.cc
+++ b/src/lib/dhcpsrv/tests/generic_lease_mgr_unittest.cc
@@ -127,7 +127,7 @@ GenericLeaseMgrTest::initializeLease4(std::string address) {
         // The times used in the next tests are deliberately restricted - we
         // should be able to cope with valid lifetimes up to 0xffffffff.
         //  However, this will lead to overflows.
-        // @TODO: test overflow conditions as now the code was fixed.
+        // @TODO: test overflow conditions when code has been fixed.
         lease->valid_lft_ = 7000;
         lease->cltt_ = 234567;
         lease->subnet_id_ = 37;
@@ -254,7 +254,7 @@ GenericLeaseMgrTest::initializeLease6(std::string address) {
         // The times used in the next tests are deliberately restricted - we
         // should be able to cope with valid lifetimes up to 0xffffffff.
         //  However, this will lead to overflows.
-        // @TODO: test overflow conditions as now the code was fixed.
+        // @TODO: test overflow conditions when code has been fixed.
         lease->preferred_lft_ = 7200;
         lease->valid_lft_ = 7000;
         lease->cltt_ = 234567;
@@ -2253,7 +2253,7 @@ GenericLeaseMgrTest::testGetExpiredLeases6() {
 }
 
 void
-GenericLeaseMgrTest::testStaticLeases4() {
+GenericLeaseMgrTest::testGetNeverExpiredLeases4() {
     // Get the leases to be used for the test.
     vector<Lease4Ptr> leases = createLeases4();
     Lease4Ptr lease = leases[1];
@@ -2274,7 +2274,7 @@ GenericLeaseMgrTest::testStaticLeases4() {
 }
 
 void
-GenericLeaseMgrTest::testStaticLeases6() {
+GenericLeaseMgrTest::testGetNeverExpiredLeases6() {
     // Get the leases to be used for the test.
     vector<Lease6Ptr> leases = createLeases6();
     Lease6Ptr lease = leases[1];
diff --git a/src/lib/dhcpsrv/tests/generic_lease_mgr_unittest.h b/src/lib/dhcpsrv/tests/generic_lease_mgr_unittest.h
index 1aa8051541..8b94fc653a 100644
--- a/src/lib/dhcpsrv/tests/generic_lease_mgr_unittest.h
+++ b/src/lib/dhcpsrv/tests/generic_lease_mgr_unittest.h
@@ -353,13 +353,13 @@ public:
     /// - reclaimed leases are not returned.
     void testGetExpiredLeases6();
 
-    /// @brief Checks that the static (i.e. with infinite valid lifetime)
-    /// DHCPv4 leases will never expire.
-    void testStaticLeases4();
+    /// @brief Checks that DHCPv4 leases with infinite valid lifetime
+    /// will never expire.
+    void testGetNeverExpiredLeases4();
 
-    /// @brief Checks that the static (i.e. with infinite valid lifetime)
-    /// DHCPv4 leases will never expire.
-    void testStaticLeases6();
+    /// @brief Checks that DHCPv6 leases with infinite valid lifetime
+    /// will never expire.
+    void testGetNeverExpiredLeases6();
 
     /// @brief Checks that declined IPv4 leases that have expired can be retrieved.
     ///
diff --git a/src/lib/dhcpsrv/tests/mysql_lease_mgr_unittest.cc b/src/lib/dhcpsrv/tests/mysql_lease_mgr_unittest.cc
index 25d04033fc..dd4488d100 100644
--- a/src/lib/dhcpsrv/tests/mysql_lease_mgr_unittest.cc
+++ b/src/lib/dhcpsrv/tests/mysql_lease_mgr_unittest.cc
@@ -591,10 +591,10 @@ TEST_F(MySqlLeaseMgrTest, getExpiredLeases4MultiThreading) {
     testGetExpiredLeases4();
 }
 
-/// @brief Checks that the static (i.e. with infinite valid lifetime) DHCPv4
-/// leases will never expire.
-TEST_F(MySqlLeaseMgrTest, staticLeases4) {
-    testStaticLeases4();
+/// @brief Checks that DHCPv4 leases with infinite valid lifetime
+/// will never expire.
+TEST_F(MySqlLeaseMgrTest, getNeverExpiredLeases4) {
+    testGetNeverExpiredLeases4();
 }
 
 /// @brief Check that expired reclaimed DHCPv4 leases are removed.
@@ -856,10 +856,10 @@ TEST_F(MySqlLeaseMgrTest, getExpiredLeases6MultiThreading) {
     testGetExpiredLeases6();
 }
 
-/// @brief Checks that the static (i.e. with infinite valid lifetime) DHCPv6
-/// leases will never expire.
-TEST_F(MySqlLeaseMgrTest, staticLeases6) {
-    testStaticLeases6();
+/// @brief Checks that DHCPv6 leases with infinite valid lifetime
+/// will never expire.
+TEST_F(MySqlLeaseMgrTest, getNeverExpiredLeases6) {
+    testGetNeverExpiredLeases6();
 }
 
 /// @brief Check that expired reclaimed DHCPv6 leases are removed.
diff --git a/src/lib/dhcpsrv/tests/pgsql_lease_mgr_unittest.cc b/src/lib/dhcpsrv/tests/pgsql_lease_mgr_unittest.cc
index 8a33980711..8da66097a3 100644
--- a/src/lib/dhcpsrv/tests/pgsql_lease_mgr_unittest.cc
+++ b/src/lib/dhcpsrv/tests/pgsql_lease_mgr_unittest.cc
@@ -545,10 +545,10 @@ TEST_F(PgSqlLeaseMgrTest, getExpiredLeases4MultiThreading) {
     testGetExpiredLeases4();
 }
 
-/// @brief Checks that the static (i.e. with infinite valid lifetime) DHCPv4
-/// leases will never expire.
-TEST_F(PgSqlLeaseMgrTest, staticLeases4) {
-    testStaticLeases4();
+/// @brief Checks that DHCPv4 leases with infinite valid lifetime
+/// will never expire.
+TEST_F(PgSqlLeaseMgrTest, getNeverExpiredLeases4) {
+    testGetNeverExpiredLeases4();
 }
 
 /// @brief Check that expired reclaimed DHCPv4 leases are removed.
@@ -810,10 +810,10 @@ TEST_F(PgSqlLeaseMgrTest, getExpiredLeases6MultiThreading) {
     testGetExpiredLeases6();
 }
 
-/// @brief Checks that the static (i.e. with infinite valid lifetime) DHCPv6
-/// leases will never expire.
-TEST_F(PgSqlLeaseMgrTest, staticLeases6) {
-    testStaticLeases6();
+/// @brief Checks that DHCPv6 leases with infinite valid lifetime
+/// will never expire.
+TEST_F(PgSqlLeaseMgrTest, getNeverExpiredLeases6) {
+    testGetNeverExpiredLeases6();
 }
 
 /// @brief Check that expired reclaimed DHCPv6 leases are removed.