From: Marcin Siodelski <marcin@isc.org>
Date: Mon, 9 Feb 2015 07:47:32 +0000 (+0100)
Subject: [3669] Second round of review comments.
X-Git-Tag: trac3712_base~13^2~1
X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=18709d8032b3c427a0b2447f779032e726fbb714;p=thirdparty%2Fkea.git

[3669] Second round of review comments.
---

diff --git a/src/lib/dhcpsrv/dhcpsrv_messages.mes b/src/lib/dhcpsrv/dhcpsrv_messages.mes
index 875fcb283d..9f81fa30e4 100644
--- a/src/lib/dhcpsrv/dhcpsrv_messages.mes
+++ b/src/lib/dhcpsrv/dhcpsrv_messages.mes
@@ -292,7 +292,13 @@ replaced by those read from the file.
 % DHCPSRV_MEMFILE_LEASE_LOAD loading lease %1
 A debug message issued when DHCP lease is being loaded from the file to memory.
 
-% DHCPSRV_MEMFILE_LFC_LEASE_FILE_REOPEN_FAIL failed to reopen lease file %1 after preparing input file for lease file cleanup: new leases will not be persisted!
+% DHCPSRV_MEMFILE_LFC_LEASE_FILE_RENAME_FAIL failed to rename the current lease file %1 to %2, reason: %3
+An error message logged when the Memfile lease database backend fails to
+move the current lease file to a new file on which the cleanup should
+be performed. This effectively means that the lease file cleanup
+will not take place.
+
+% DHCPSRV_MEMFILE_LFC_LEASE_FILE_REOPEN_FAIL failed to reopen lease file %1 after preparing input file for lease file cleanup, reason: %2, new leases will not be persisted!
 An error message logged when the Memfile lease database backend
 failed to re-open or re-create the lease file after renaming the
 lease file for lease file cleanup. The server will continue to
@@ -303,12 +309,6 @@ An informational message logged when the Memfile lease database backend
 configures the LFC to be executed periodically. The argument holds the
 interval in seconds in which the LFC will be executed.
 
-% DHCPSRV_MEMFILE_LFC_ROTATION_FAIL failed to rotate the current lease file %1 to %2, reason: %3
-An error message logged when the Memfile lease database backend fails to
-move the current lease file to a new file on which the cleanup should
-be performed. This effectively means that the lease file cleanup
-will not take place.
-
 % DHCPSRV_MEMFILE_LFC_SPAWN_FAIL lease file cleanup failed to run because kea-lfc process couldn't be spawned
 This error message is logged when the the Kea server fails to run kea-lfc,
 the program that cleans up the lease file. The server will try again the
diff --git a/src/lib/dhcpsrv/memfile_lease_mgr.cc b/src/lib/dhcpsrv/memfile_lease_mgr.cc
index 5a59462629..0172298aec 100644
--- a/src/lib/dhcpsrv/memfile_lease_mgr.cc
+++ b/src/lib/dhcpsrv/memfile_lease_mgr.cc
@@ -820,7 +820,13 @@ void Memfile_LeaseMgr::lfcExecute(boost::shared_ptr<LeaseFileType>& lease_file)
         // because we don't want to run LFC if the rename failed.
         do_lfc = (rename(lease_file->getFilename().c_str(),
                          lease_file_copy.getFilename().c_str()) == 0);
-        error_string = strerror(errno);
+
+        if (!do_lfc) {
+            LOG_ERROR(dhcpsrv_logger, DHCPSRV_MEMFILE_LFC_LEASE_FILE_RENAME_FAIL)
+                .arg(lease_file->getFilename())
+                .arg(lease_file_copy.getFilename())
+                .arg(strerror(errno));
+        }
 
         // Regardless if we successfully moved the current file or not,
         // we need to re-open the current file for the server to write
@@ -841,24 +847,18 @@ void Memfile_LeaseMgr::lfcExecute(boost::shared_ptr<LeaseFileType>& lease_file)
             /// is most likely related to a human error, e.g. changing
             /// file permissions.
             LOG_ERROR(dhcpsrv_logger, DHCPSRV_MEMFILE_LFC_LEASE_FILE_REOPEN_FAIL)
-                .arg(lease_file->getFilename());
+                .arg(lease_file->getFilename())
+                .arg(ex.what());
             // Reset the pointer to the file so as the backend doesn't
             // try to write leases to disk.
             lease_file.reset();
             do_lfc = false;
-            error_string = ex.what();
         }
     }
     // Once the files have been rotated, or untouched if another LFC had
     // not finished, a new process is started.
     if (do_lfc) {
         lfc_setup_->execute();
-
-    } else {
-        LOG_ERROR(dhcpsrv_logger, DHCPSRV_MEMFILE_LFC_ROTATION_FAIL)
-            .arg(lease_file->getFilename())
-            .arg(lease_file_copy.getFilename())
-            .arg(error_string);
     }
 }
 
diff --git a/src/lib/util/process_spawn.cc b/src/lib/util/process_spawn.cc
index c02b924d13..c1cb83436a 100644
--- a/src/lib/util/process_spawn.cc
+++ b/src/lib/util/process_spawn.cc
@@ -246,6 +246,9 @@ ProcessSpawnImpl::waitForProcess(int signum) {
         int status = 0;
         pid_t pid = waitpid(-1, &status, 0);
         if (pid > 0) {
+            /// @todo Check that the terminatin process was started
+            /// by our instance of ProcessSpawn and only handle it
+            /// if it was.
             process_status_[pid] = status;
         }
         return (true);