From: Thomas Markwalder Date: Wed, 17 Dec 2025 14:00:13 +0000 (-0500) Subject: [#4176] Addressed further comments X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=9b945ae17c07aa2ef178ae2fce4f2f29e152d108;p=thirdparty%2Fkea.git [#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 12bae7b089..b89c037d77 100644 --- 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 e632c508c3..3a69d565f1 100644 --- 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 26de00a7d0..5782f3783a 100644 --- 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(value)); StatsMgr::instance().addValue(StatsMgr::generateName("subnet", subnet_id, stat), static_cast(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 d7a55e220e..b42df98598 100644 --- 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 c09b5243dd..ce57c64071 100644 --- 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 },