[#4176] Addressed further comments

modified:   src/hooks/dhcp/lease_cmds/lease_cmds.cc
    now addOrUpdate4 prohibits registered state
modified:   src/hooks/dhcp/lease_cmds/libloadtests/lease_cmds4_unittest.cc
    added UT

modified:   src/lib/dhcpsrv/lease_mgr.*
    renamed bumpStatPrefixPool() to bumpStatPrefix()

modified:   src/lib/dhcpsrv/testutils/generic_lease_mgr_unittest.cc
    added missing scenarios

diff --git a/src/hooks/dhcp/lease_cmds/lease_cmds.cc b/src/hooks/dhcp/lease_cmds/lease_cmds.cc
index 12bae7b089dbca50860c16747b501905a607c430..b89c037d772041a196c653845b8aa9ef72ce3407 100644 (file)
--- a/src/hooks/dhcp/lease_cmds/lease_cmds.cc
+++ b/src/hooks/dhcp/lease_cmds/lease_cmds.cc
@@ -499,6 +499,10 @@ public:
 
 bool
 LeaseCmdsImpl::addOrUpdate4(Lease4Ptr lease, bool force_create) {
+    if (lease->stateRegistered()) {
+        isc_throw(BadValue, "DHCPv4 leases do not support registered state");
+    }
+
     Lease4Ptr existing = LeaseMgrFactory::instance().getLease4(lease->addr_);
     if (force_create && !existing) {
         // lease does not exist

diff --git a/src/hooks/dhcp/lease_cmds/libloadtests/lease_cmds4_unittest.cc b/src/hooks/dhcp/lease_cmds/libloadtests/lease_cmds4_unittest.cc
index e632c508c3c453b01174a2eacdc7aa86e01882ae..3a69d565f11ff8a8c3d6146100ed37c9ca0191ef 100644 (file)
--- a/src/hooks/dhcp/lease_cmds/libloadtests/lease_cmds4_unittest.cc
+++ b/src/hooks/dhcp/lease_cmds/libloadtests/lease_cmds4_unittest.cc
@@ -376,6 +376,9 @@ public:
     /// remote ids.
     void testLease4UpdateExtendedInfo();
 
+    /// @brief Check that lease4-update prohibits registered state.
+    void testLease4UpdateRegisteredInvalid();
+
     /// @brief Check that lease4-del can handle a situation when the query is
     /// broken (some required parameters are missing).
     void testLease4DelMissingParams();
@@ -2839,6 +2842,42 @@ void Lease4CmdsTest::testLease4UpdateExtendedInfo() {
     EXPECT_EQ(*l, *leases[0]);
 }
 
+void Lease4CmdsTest::testLease4UpdateRegisteredInvalid() {
+    // Initialize lease manager (false = v4, true = add leases)
+    initLeaseMgr(false, true);
+
+    checkLease4Stats(0, 4, 0);
+    checkLease4Stats(44, 2, 0);
+    checkLease4Stats(88, 2, 0);
+
+    // Now send the command.
+    string txt =
+        "{\n"
+        "    \"command\": \"lease4-update\",\n"
+        "    \"arguments\": {"
+        "        \"ip-address\": \"192.0.2.1\",\n"
+        "        \"hw-address\": \"1a:1b:1c:1d:1e:1f\",\n"
+        "        \"hostname\": \"newhostname.example.org\",\n"
+        "        \"state\": 4\n"
+        "    }\n"
+        "}";
+
+    string exp_rsp = "DHCPv4 leases do not support registered state";
+    testCommand(txt, CONTROL_RESULT_ERROR, exp_rsp);
+
+    // Stats should not have changed.
+    checkLease4Stats(0, 4, 0);
+    checkLease4Stats(44, 2, 0);
+    checkLease4Stats(88, 2, 0);
+
+    // Check that the lease is still there.
+    Lease4Ptr l = lmptr_->getLease4(IOAddress("192.0.2.1"));
+    ASSERT_TRUE(l);
+
+    // State should not have changed.
+    EXPECT_EQ(l->state_, Lease::STATE_DEFAULT);
+}
+
 void Lease4CmdsTest::testLease4DelMissingParams() {
     // No parameters whatsoever. You want just a lease, any lease?
     string cmd =
@@ -4016,6 +4055,15 @@ TEST_F(Lease4CmdsTest, lease4AddExtendedInfo) {
     testLease4AddExtendedInfo();
 }
 
+TEST_F(Lease4CmdsTest, lease4UpdateRegisteredInvalid) {
+    testLease4UpdateRegisteredInvalid();
+}
+
+TEST_F(Lease4CmdsTest, lease4UpdateRegisteredInvalidMultiThreading) {
+    MultiThreadingTest mt(true);
+    testLease4UpdateRegisteredInvalid();
+}
+
 TEST_F(Lease4CmdsTest, lease4AddExtendedInfoMultiThreading) {
     MultiThreadingTest mt(true);
     testLease4AddExtendedInfo();

diff --git a/src/lib/dhcpsrv/lease_mgr.cc b/src/lib/dhcpsrv/lease_mgr.cc
index 26de00a7d0594e27d7de0cba9e291b2a819e55f9..5782f3783ac9bbf95321fddcc80d69db02ad5b7f 100644 (file)
--- a/src/lib/dhcpsrv/lease_mgr.cc
+++ b/src/lib/dhcpsrv/lease_mgr.cc
@@ -1425,7 +1425,7 @@ LeaseMgr::bumpStat(const std::string& stat, SubnetID& subnet_id, PoolPtr pool, i
 }
 
 void 
-LeaseMgr::bumpStatPrefixPool(const std::string& stat, SubnetID& subnet_id, PoolPtr pool, int value) {
+LeaseMgr::bumpStatPrefix(const std::string& stat, SubnetID& subnet_id, PoolPtr pool, int value) {
     StatsMgr::instance().addValue(stat, static_cast<int64_t>(value));
     StatsMgr::instance().addValue(StatsMgr::generateName("subnet", subnet_id, stat),
                                   static_cast<int64_t>(value));
@@ -1668,7 +1668,7 @@ LeaseMgr::updateStatsOnUpdate(const Lease6Ptr& existing,
             if (existing->type_ == Lease::TYPE_NA) {
                 bumpStat("assigned-nas", existing->subnet_id_, pool, 1);
             } else {
-                bumpStatPrefixPool("assigned-pds", existing->subnet_id_, pool, 1);
+                bumpStatPrefix("assigned-pds", existing->subnet_id_, pool, 1);
             }
             break;
 
@@ -1697,7 +1697,7 @@ LeaseMgr::updateStatsOnUpdate(const Lease6Ptr& existing,
             if (existing->type_ == Lease::TYPE_NA) {
                 bumpStat("assigned-nas", existing->subnet_id_, pool, -1);
             } else {
-                bumpStatPrefixPool("assigned-pds", existing->subnet_id_, pool, -1);
+                bumpStatPrefix("assigned-pds", existing->subnet_id_, pool, -1);
             }
             break;
 
@@ -1760,8 +1760,8 @@ LeaseMgr::updateStatsOnUpdate(const Lease6Ptr& existing,
             bumpStat("assigned-nas", existing->subnet_id_, existing_pool, -1);
             bumpStat("assigned-nas", lease->subnet_id_, new_pool, 1);
         } else {
-            bumpStatPrefixPool("assigned-pds", existing->subnet_id_, existing_pool, -1);
-            bumpStatPrefixPool("assigned-pds", lease->subnet_id_, new_pool, 1);
+            bumpStatPrefix("assigned-pds", existing->subnet_id_, existing_pool, -1);
+            bumpStatPrefix("assigned-pds", lease->subnet_id_, new_pool, 1);
         }
         break;
 
@@ -1776,7 +1776,7 @@ LeaseMgr::updateStatsOnUpdate(const Lease6Ptr& existing,
         if (lease->type_ == Lease::TYPE_NA) {
             bumpStat("assigned-nas", lease->subnet_id_, new_pool, 1);
         } else {
-            bumpStatPrefixPool("assigned-pds", lease->subnet_id_, new_pool, 1);
+            bumpStatPrefix("assigned-pds", lease->subnet_id_, new_pool, 1);
         }
         break;
 
@@ -1815,7 +1815,7 @@ LeaseMgr::updateStatsOnUpdate(const Lease6Ptr& existing,
         if (lease->type_ == Lease::TYPE_NA) {
             bumpStat("assigned-nas", existing->subnet_id_, existing_pool, -1);
         } else {
-            bumpStatPrefixPool("assigned-pds", existing->subnet_id_, existing_pool, -1);
+            bumpStatPrefix("assigned-pds", existing->subnet_id_, existing_pool, -1);
         }
         break;
 

diff --git a/src/lib/dhcpsrv/lease_mgr.h b/src/lib/dhcpsrv/lease_mgr.h
index d7a55e220e371b38f4950ae68dd22b8bc6eb47cd..b42df98598d3fb08add4d79b837cf0474a939c22 100644 (file)
--- a/src/lib/dhcpsrv/lease_mgr.h
+++ b/src/lib/dhcpsrv/lease_mgr.h
@@ -1151,8 +1151,8 @@ public:
     /// @param pool pointer to the pool (if one) within the subnet, if empty
     /// pool level is skipped.
     /// @param value signed value to add to the statistic
-    static void bumpStatPrefixPool(const std::string& stat, SubnetID&
-                                   subnet_id, PoolPtr pool, int value);
+    static void bumpStatPrefix(const std::string& stat, SubnetID&
+                               subnet_id, PoolPtr pool, int value);
 protected:
 
     /// Extended information / Bulk Lease Query shared interface.

diff --git a/src/lib/dhcpsrv/testutils/generic_lease_mgr_unittest.cc b/src/lib/dhcpsrv/testutils/generic_lease_mgr_unittest.cc
index c09b5243ddd753cb011549abfd3ee1040a84d15d..ce57c6407174086871eb47658d92128c9c190693 100644 (file)
--- a/src/lib/dhcpsrv/testutils/generic_lease_mgr_unittest.cc
+++ b/src/lib/dhcpsrv/testutils/generic_lease_mgr_unittest.cc
@@ -5110,16 +5110,19 @@ GenericLeaseMgrTest::testUpdateStatsOn4SameSubnet() {
     { __LINE__, Lease::STATE_DEFAULT, Lease::STATE_DEFAULT, 1, 1 },
     { __LINE__, Lease::STATE_DEFAULT, Lease::STATE_DECLINED, 1, 0 },
     { __LINE__, Lease::STATE_DEFAULT, Lease::STATE_EXPIRED_RECLAIMED, 2, 1 },
+    { __LINE__, Lease::STATE_DEFAULT, Lease::STATE_RELEASED, 2, 1 },
 
     // New state DECLINED
     { __LINE__, Lease::STATE_DECLINED, Lease::STATE_DEFAULT, 1, 2 },
     { __LINE__, Lease::STATE_DECLINED, Lease::STATE_DECLINED, 1, 1 },
     { __LINE__, Lease::STATE_DECLINED, Lease::STATE_EXPIRED_RECLAIMED, 2, 2 },
+    { __LINE__, Lease::STATE_DECLINED, Lease::STATE_RELEASED, 2, 2 },
 
     // New state EXPIRED_RECLAIMED
     { __LINE__, Lease::STATE_EXPIRED_RECLAIMED, Lease::STATE_DEFAULT, 0, 1 },
     { __LINE__, Lease::STATE_EXPIRED_RECLAIMED, Lease::STATE_DECLINED, 0, 0 },
     { __LINE__, Lease::STATE_EXPIRED_RECLAIMED, Lease::STATE_EXPIRED_RECLAIMED, 1, 1 },
+    { __LINE__, Lease::STATE_EXPIRED_RECLAIMED, Lease::STATE_RELEASED, 1, 1 },
 
     // New state RELEASED
     { __LINE__, Lease::STATE_RELEASED, Lease::STATE_DEFAULT, 0, 1 },