From b1fb6811ffe149c375a4503d2d64604acae72f83 Mon Sep 17 00:00:00 2001 From: Francis Dupont Date: Tue, 10 Dec 2019 19:37:22 +0100 Subject: [#897] Addressed comments --- src/lib/dhcpsrv/alloc_engine.cc | 51 ++++++++++++---------- src/lib/dhcpsrv/cql_lease_mgr.cc | 4 +- src/lib/dhcpsrv/database_backends.dox | 17 ++++++++ src/lib/dhcpsrv/lease.cc | 31 +++++++------ src/lib/dhcpsrv/lease.h | 9 ++++ src/lib/dhcpsrv/mysql_lease_mgr.cc | 10 +++-- src/lib/dhcpsrv/pgsql_lease_mgr.cc | 24 +++++----- src/lib/dhcpsrv/tests/cql_lease_mgr_unittest.cc | 16 +++---- .../dhcpsrv/tests/generic_lease_mgr_unittest.cc | 8 ++-- src/lib/dhcpsrv/tests/generic_lease_mgr_unittest.h | 12 ++--- src/lib/dhcpsrv/tests/mysql_lease_mgr_unittest.cc | 16 +++---- src/lib/dhcpsrv/tests/pgsql_lease_mgr_unittest.cc | 16 +++---- 12 files changed, 126 insertions(+), 88 deletions(-) (limited to 'src/lib') 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 >(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 >(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 >(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 >(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(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(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(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 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 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. -- cgit v1.2.3