[#597,!323] Use round() when calculating T1/T2 to ensure consistent results

src/bin/dhcp4/dhcp4_srv.cc
    Dhcpv4Srv::setTeeTimes() - added use of round()

src/bin/dhcp6/dhcp6_srv.cc
    Dhcpv6Srv::setTeeTimes() - added use of round()

src/bin/dhcp6/tests/dhcp6_test_utils.cc
    Dhcpv6SrvTest::checkIA_NA() - replaced used of
    EXPECTED_EQ with logic to add a failure so callers
    can ASSERT_NO_FATAL_FAILURE and pinpoint the invocation.

src/bin/dhcp6/tests/hooks_unittest.cc
    Wrapped calls to checkIA_NA() with ASSERT_NO_FATAL_FAILURE
    Updated expected T1 values to adjust for use of round().

diff --git a/src/bin/dhcp4/dhcp4_srv.cc b/src/bin/dhcp4/dhcp4_srv.cc
index 9f3727122589af39366752b407118694c41c2214..a04976c8713d8a67c42153ca4c2dc85d3adad021 100644 (file)
--- a/src/bin/dhcp4/dhcp4_srv.cc
+++ b/src/bin/dhcp4/dhcp4_srv.cc
@@ -2269,7 +2269,7 @@ Dhcpv4Srv::setTeeTimes(const Lease4Ptr& lease, const Subnet4Ptr& subnet, Pkt4Ptr
         t2_time = subnet->getT2();
     } else if (subnet->getCalculateTeeTimes()) {
         // Calculating tee times is enabled, so calculated it.
-        t2_time = static_cast<uint32_t>(subnet->getT2Percent() * (lease->valid_lft_));
+        t2_time = static_cast<uint32_t>(round(subnet->getT2Percent() * (lease->valid_lft_)));
     }
 
     // Send the T2 candidate value only if it's sane: to be sane it must be less than
@@ -2288,7 +2288,7 @@ Dhcpv4Srv::setTeeTimes(const Lease4Ptr& lease, const Subnet4Ptr& subnet, Pkt4Ptr
         t1_time = subnet->getT1();
     } else if (subnet->getCalculateTeeTimes()) {
         // Calculating tee times is enabled, so calculate it.
-        t1_time = static_cast<uint32_t>(subnet->getT1Percent() * (lease->valid_lft_));
+        t1_time = static_cast<uint32_t>(round(subnet->getT1Percent() * (lease->valid_lft_)));
     }
 
     // Send T1 if it's sane: If we sent T2, T1 must be less than that.  If not it must be

diff --git a/src/bin/dhcp6/dhcp6_srv.cc b/src/bin/dhcp6/dhcp6_srv.cc
index 607ca7640629921435dd3f514411744f0cae14b6..37ad8b9ffa62dd2cd2b4849108499277d4a488df 100644 (file)
--- a/src/bin/dhcp6/dhcp6_srv.cc
+++ b/src/bin/dhcp6/dhcp6_srv.cc
@@ -3908,7 +3908,7 @@ Dhcpv6Srv::setTeeTimes(uint32_t preferred_lft, const Subnet6Ptr& subnet, Option6
         t2_time = subnet->getT2();
     } else if (subnet->getCalculateTeeTimes()) {
         // Calculating tee times is enabled, so calculate it.
-        t2_time = static_cast<uint32_t>(subnet->getT2Percent() * preferred_lft);
+        t2_time = static_cast<uint32_t>(round(subnet->getT2Percent() * preferred_lft));
     }
 
     // We allow T2 to be any value.
@@ -3922,7 +3922,7 @@ Dhcpv6Srv::setTeeTimes(uint32_t preferred_lft, const Subnet6Ptr& subnet, Option6
         t1_time = subnet->getT1();
     } else if (subnet->getCalculateTeeTimes()) {
         // Calculating tee times is enabled, so calculate it.
-        t1_time = static_cast<uint32_t>(subnet->getT1Percent() * preferred_lft);
+        t1_time = static_cast<uint32_t>(round(subnet->getT1Percent() * preferred_lft));
     }
 
     // T1 is sane if it is less than or equal to T2.

diff --git a/src/bin/dhcp6/tests/dhcp6_test_utils.cc b/src/bin/dhcp6/tests/dhcp6_test_utils.cc
index f25e164586057124be688ccb18ab50ba28307075..3891f2aef5b2b63d8d026c5ed5c1fbfb40ae363e 100644 (file)
--- a/src/bin/dhcp6/tests/dhcp6_test_utils.cc
+++ b/src/bin/dhcp6/tests/dhcp6_test_utils.cc
@@ -1,4 +1,4 @@
-// Copyright (C) 2013-2018 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2013-2019 Internet Systems Consortium, Inc. ("ISC")
 //
 // This Source Code Form is subject to the terms of the Mozilla Public
 // License, v. 2.0. If a copy of the MPL was not distributed with this
@@ -96,9 +96,23 @@ Dhcpv6SrvTest::checkIA_NA(const Pkt6Ptr& rsp, uint32_t expected_iaid,
         return (boost::shared_ptr<Option6IAAddr>());
     }
 
-    EXPECT_EQ(expected_iaid, ia->getIAID());
-    EXPECT_EQ(expected_t1, ia->getT1());
-    EXPECT_EQ(expected_t2, ia->getT2());
+    if (expected_iaid != ia->getIAID()) {
+        ADD_FAILURE() << "ia->iaid: " << ia->getIAID()
+                      << " is not expected value: " << expected_iaid;
+        return (boost::shared_ptr<Option6IAAddr>());
+    }
+
+    if (expected_t1 != ia->getT1()) {
+        ADD_FAILURE() << "ia->t1: " << ia->getT1()
+                      << " is not expected value: " << expected_t1;
+        return (boost::shared_ptr<Option6IAAddr>());
+    }
+
+    if (expected_t2 != ia->getT2()) {
+        ADD_FAILURE() << "ia->t2: " << ia->getT2()
+                      << " is not expected value: " << expected_t2;
+        return (boost::shared_ptr<Option6IAAddr>());
+    }
 
     tmp = ia->getOption(D6O_IAADDR);
     boost::shared_ptr<Option6IAAddr> addr = boost::dynamic_pointer_cast<Option6IAAddr>(tmp);

diff --git a/src/bin/dhcp6/tests/hooks_unittest.cc b/src/bin/dhcp6/tests/hooks_unittest.cc
index 3a92dfbfd056a7151585748df4d0b83ec2b6bea7..e28c00baf975899477d5435e5cc484e5e478392f 100644 (file)
--- a/src/bin/dhcp6/tests/hooks_unittest.cc
+++ b/src/bin/dhcp6/tests/hooks_unittest.cc
@@ -2741,8 +2741,9 @@ TEST_F(HooksDhcpv6SrvTest, basicLease6Renew) {
     ASSERT_TRUE(tmp);
 
     // Check that IA_NA was returned and that there's an address included
-    boost::shared_ptr<Option6IAAddr> addr_opt = checkIA_NA(reply, 234, subnet_->getT1(),
-                                                           subnet_->getT2());
+    boost::shared_ptr<Option6IAAddr> addr_opt;
+    ASSERT_NO_FATAL_FAILURE(addr_opt = checkIA_NA(reply, 234, subnet_->getT1(),
+                                                  subnet_->getT2()));
 
     ASSERT_TRUE(addr_opt);
     // Check that the lease is really in the database
@@ -2834,7 +2835,8 @@ TEST_F(HooksDhcpv6SrvTest, leaseUpdateLease6Renew) {
     ASSERT_TRUE(tmp);
 
     // Check that IA_NA was returned and that there's an address included
-    boost::shared_ptr<Option6IAAddr> addr_opt = checkIA_NA(reply, 1000, 601, 802);
+    boost::shared_ptr<Option6IAAddr> addr_opt;
+    ASSERT_NO_FATAL_FAILURE(addr_opt = checkIA_NA(reply, 1000, 602, 802));
 
     ASSERT_TRUE(addr_opt);
     // Check that the lease is really in the database
@@ -3863,8 +3865,9 @@ TEST_F(HooksDhcpv6SrvTest, basicLease6Rebind) {
     ASSERT_TRUE(tmp);
 
     // Check that IA_NA was returned and that there's an address included
-    boost::shared_ptr<Option6IAAddr> addr_opt = checkIA_NA(reply, 234, subnet_->getT1(),
-                                                           subnet_->getT2());
+    boost::shared_ptr<Option6IAAddr> addr_opt;
+    ASSERT_NO_FATAL_FAILURE(addr_opt = checkIA_NA(reply, 234, subnet_->getT1(),
+                                                  subnet_->getT2()));
 
     ASSERT_TRUE(addr_opt);
     // Check that the lease is really in the database
@@ -3951,7 +3954,8 @@ TEST_F(HooksDhcpv6SrvTest, leaseUpdateLease6Rebind) {
 
     // Check that IA_NA was returned and that there's an address included
     // Note we also verify that T1 and T2 were calculated correctly.
-    boost::shared_ptr<Option6IAAddr> addr_opt = checkIA_NA(reply, 1000, 601, 802);
+    boost::shared_ptr<Option6IAAddr> addr_opt;
+    ASSERT_NO_FATAL_FAILURE(addr_opt = checkIA_NA(reply, 1000, 602, 802));
 
     ASSERT_TRUE(addr_opt);
     // Check that the lease is really in the database
@@ -4757,7 +4761,8 @@ TEST_F(HooksDhcpv6SrvTest, host6Identifier) {
     ASSERT_TRUE(tmp);
 
     // Check that IA_NA was returned and that there's an address included
-    boost::shared_ptr<Option6IAAddr> addr_opt = checkIA_NA(adv, 234, 1000, 2000);
+    boost::shared_ptr<Option6IAAddr> addr_opt;
+    ASSERT_NO_FATAL_FAILURE(addr_opt = checkIA_NA(adv, 234, 1000, 2000));
 
     ASSERT_TRUE(addr_opt);
     ASSERT_EQ("2001:db8::f00", addr_opt->getAddress().toText());
@@ -4837,7 +4842,8 @@ TEST_F(HooksDhcpv6SrvTest, host6Identifier_hwaddr) {
     ASSERT_TRUE(tmp);
 
     // Check that IA_NA was returned and that there's an address included
-    boost::shared_ptr<Option6IAAddr> addr_opt = checkIA_NA(adv, 234, 1000, 2000);
+    boost::shared_ptr<Option6IAAddr> addr_opt;
+    ASSERT_NO_FATAL_FAILURE(addr_opt = checkIA_NA(adv, 234, 1000, 2000));
 
     ASSERT_TRUE(addr_opt);
     ASSERT_EQ("2001:db8::f00", addr_opt->getAddress().toText());