From: Francis Dupont <fdupont@isc.org>
Date: Thu, 25 Sep 2025 20:22:46 +0000 (+0200)
Subject: [#4090] Addressed comments
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=1f11acf82bb7a28402960ce9ec2d54ae910636a2;p=thirdparty%2Fkea.git

[#4090] Addressed comments
---

diff --git a/src/bin/lfc/tests/lfc_controller_unittests.cc b/src/bin/lfc/tests/lfc_controller_unittests.cc
index 871fa21ced..7520da2e21 100644
--- a/src/bin/lfc/tests/lfc_controller_unittests.cc
+++ b/src/bin/lfc/tests/lfc_controller_unittests.cc
@@ -721,7 +721,7 @@ TEST_F(LFCControllerTest, alreadyRunning) {
     };
     int argc = 14;
     PIDFile pid_file(pstr_);
-    string lockname = pstr_ + ".lock";
+    string lockname = pid_file.getLockname();
     int parent_pid = getpid();
     int child_pid;
     LFCController lfc_controller;
diff --git a/src/lib/dhcpsrv/memfile_lease_mgr.cc b/src/lib/dhcpsrv/memfile_lease_mgr.cc
index e6f85151d7..32659b231e 100644
--- a/src/lib/dhcpsrv/memfile_lease_mgr.cc
+++ b/src/lib/dhcpsrv/memfile_lease_mgr.cc
@@ -265,7 +265,17 @@ LFCSetup::execute(const std::string& lease_file) {
 
         LOG_INFO(dhcpsrv_logger, DHCPSRV_MEMFILE_LFC_EXECUTE)
             .arg(process_->getCommandLine());
-        pid_ = process_->spawn();
+        try {
+            pid_ = process_->spawn();
+        } catch (const ProcessSpawnError&) {
+            LOG_ERROR(dhcpsrv_logger, DHCPSRV_MEMFILE_LFC_SPAWN_FAIL);
+            try {
+                pid_file.deleteFile();
+            } catch (...) {
+                // Ignore errors.
+            }
+            return;
+        }
 
         // Write the pid of the child in the pid file.
         pid_file.write(pid_);
@@ -273,17 +283,6 @@ LFCSetup::execute(const std::string& lease_file) {
     } catch (const PIDFileError& ex) {
         LOG_ERROR(dhcpsrv_logger, DHCPSRV_MEMFILE_LFC_FAIL_PID_CREATE)
             .arg(ex.what());
-    } catch (const ProcessSpawnError&) {
-        LOG_ERROR(dhcpsrv_logger, DHCPSRV_MEMFILE_LFC_SPAWN_FAIL);
-        // Reacquire the lock to remove the pid file.
-        PIDLock pid_lock(pid_file.getLockname());
-        if (pid_lock.isLocked() && !pid_file.check()) {
-            try {
-                pid_file.deleteFile();
-            } catch (...) {
-                // Ignore errors.
-            }
-        }
     }
 }
 
diff --git a/src/lib/dhcpsrv/tests/memfile_lease_mgr_unittest.cc b/src/lib/dhcpsrv/tests/memfile_lease_mgr_unittest.cc
index c0d3abd8cf..10d03fa641 100644
--- a/src/lib/dhcpsrv/tests/memfile_lease_mgr_unittest.cc
+++ b/src/lib/dhcpsrv/tests/memfile_lease_mgr_unittest.cc
@@ -4665,12 +4665,12 @@ TEST_F(MemfileLeaseMgrLogTest, lfcStartHandlerPersistFalse) {
     expected += "was configured to 'false'\" }";
     EXPECT_EQ(expected, response->str());
 
-    // No reschedule.
+    // Verify that LFC reschedule was not logged.
     string msg = "DHCPSRV_MEMFILE_LFC_RESCHEDULED ";
     msg += "rescheduled Lease File Cleanup";
     EXPECT_EQ(0, countFile(msg));
 
-    // No start.
+    // Verify that LFC start was not logged.
     msg = "DHCPSRV_MEMFILE_LFC_START ";
     msg += "starting Lease File Cleanup";
     EXPECT_EQ(0, countFile(msg));
@@ -4706,18 +4706,18 @@ TEST_F(MemfileLeaseMgrLogTest, lfcStartHandlerLfcInterval0) {
                                          Memfile_LeaseMgr::FILE_PID)));
     EXPECT_FALSE(file::exists(pid_file_->getFilename()));
 
-    // No reschedule.
+    // Verify that LFC reschedule was not logged.
     string msg = "DHCPSRV_MEMFILE_LFC_RESCHEDULED ";
     msg += "rescheduled Lease File Cleanup";
     EXPECT_EQ(0, countFile(msg));
 
-    // Start.
+    // Verify that LFC start was logged.
     msg = "DHCPSRV_MEMFILE_LFC_START ";
     msg += "starting Lease File Cleanup";
     EXPECT_EQ(1, countFile(msg));
 }
 
-/// @brief Verifies that lfcStartHandler reschedules and starts lfc.
+/// @brief Verifies that lfcStartHandler reschedules and starts LFC.
 TEST_F(MemfileLeaseMgrLogTest, lfcStartHandler) {
     DatabaseConnection::ParameterMap pmap;
     pmap["universe"] = "4";
@@ -4747,19 +4747,19 @@ TEST_F(MemfileLeaseMgrLogTest, lfcStartHandler) {
                                          Memfile_LeaseMgr::FILE_PID)));
     EXPECT_FALSE(file::exists(pid_file_->getFilename()));
 
-    // Reschedule.
+    // Verify that LFC reschedule was logged.
     string msg = "DHCPSRV_MEMFILE_LFC_RESCHEDULED ";
     msg += "rescheduled Lease File Cleanup";
     EXPECT_EQ(1, countFile(msg));
 
-    // Start.
+    // Verify that LFC start was logged.
     msg = "DHCPSRV_MEMFILE_LFC_START ";
     msg += "starting Lease File Cleanup";
     EXPECT_EQ(1, countFile(msg));
 }
 
-/// @brief Verifies that lfcStartHandler reschedules and but not start lfc
-/// if the pid file lock is already taken.
+/// @brief Verifies that lfcStartHandler reschedules and but does not start
+/// LFC if the pid file lock is already taken.
 TEST_F(MemfileLeaseMgrLogTest, lfcStartHandlerLocked) {
     DatabaseConnection::ParameterMap pmap;
     pmap["universe"] = "6";
@@ -4784,24 +4784,24 @@ TEST_F(MemfileLeaseMgrLogTest, lfcStartHandlerLocked) {
     string expected = "{ \"result\": 3, \"text\": \"kea-lfc already running\" }";
     EXPECT_EQ(expected, response->str());
 
-    // Reschedule.
+    // Verify that LFC reschedule was logged.
     string msg = "DHCPSRV_MEMFILE_LFC_RESCHEDULED ";
     msg += "rescheduled Lease File Cleanup";
     EXPECT_EQ(1, countFile(msg));
 
-    // Start.
+    // Verify that LFC start was logged.
     msg = "DHCPSRV_MEMFILE_LFC_START ";
     msg += "starting Lease File Cleanup";
     EXPECT_EQ(1, countFile(msg));
 
-    // Locked.
+    // Locked: verify that LFC running was logged.
     msg = "DHCPSRV_MEMFILE_LFC_RUNNING ";
     msg += "Lease File Cleanup instance already running";
     EXPECT_EQ(1, countFile(msg));
 }
 
-/// @brief Verifies that lfcStartHandler reschedules and but not start lfc
-/// if the pid file check says lfc is already running.
+/// @brief Verifies that lfcStartHandler reschedules and but does not start
+/// LFC if the pid file check says lfc is already running.
 TEST_F(MemfileLeaseMgrLogTest, lfcStartHandlerAlreadyRunning) {
     DatabaseConnection::ParameterMap pmap;
     pmap["universe"] = "4";
@@ -4829,24 +4829,24 @@ TEST_F(MemfileLeaseMgrLogTest, lfcStartHandlerAlreadyRunning) {
     string expected = "{ \"result\": 3, \"text\": \"kea-lfc already running\" }";
     EXPECT_EQ(expected, response->str());
 
-    // Reschedule.
+    // Verify that LFC reschedule was logged.
     string msg = "DHCPSRV_MEMFILE_LFC_RESCHEDULED ";
     msg += "rescheduled Lease File Cleanup";
     EXPECT_EQ(1, countFile(msg));
 
-    // Start.
+    // Verify that LFC start was logged.
     msg = "DHCPSRV_MEMFILE_LFC_START ";
     msg += "starting Lease File Cleanup";
     EXPECT_EQ(1, countFile(msg));
 
-    // Already running.
+    // Verify that LFC running was logged.
     msg = "DHCPSRV_MEMFILE_LFC_RUNNING ";
     msg += "Lease File Cleanup instance already running";
     EXPECT_EQ(1, countFile(msg));
 }
 
-/// @brief Verifies that lfcStartHandler reschedules and but not start lfc
-/// if the pid file is not writable.
+/// @brief Verifies that lfcStartHandler reschedules and but does not start
+/// LFC if the pid file is not writable.
 TEST_F(MemfileLeaseMgrLogTest, lfcStartHandlerPidNotWritable) {
     DatabaseConnection::ParameterMap pmap;
     pmap["universe"] = "6";
@@ -4875,17 +4875,17 @@ TEST_F(MemfileLeaseMgrLogTest, lfcStartHandlerPidNotWritable) {
     string expected = "{ \"result\": 1, \"text\": \"failed to start kea-lfc\" }";
     EXPECT_EQ(expected, response->str());
 
-    // Reschedule.
+    // Verify that LFC reschedule was logged.
     string msg = "DHCPSRV_MEMFILE_LFC_RESCHEDULED ";
     msg += "rescheduled Lease File Cleanup";
     EXPECT_EQ(1, countFile(msg));
 
-    // Start.
+    // Verify that LFC start was logged.
     msg = "DHCPSRV_MEMFILE_LFC_START ";
     msg += "starting Lease File Cleanup";
     EXPECT_EQ(1, countFile(msg));
 
-    // PID file is not writable.
+    // PID file is not writable: verify that PID create failure was logged.
     msg = "DHCPSRV_MEMFILE_LFC_FAIL_PID_CREATE ";
     msg += "Lease File Cleanup pid file create: ";
     msg += "Unable to open PID file '";