[3669] Address review comments.

src/lib/dhcpsrv/dhcpsrv_messages.mes

@@ -294,26 +294,28 @@ 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!
 An error message logged when the Memfile lease database backend
-failed to re-open or re-create the lease file after moving the
-lease file to an input file for lease file cleanup. The server
-will continue to operate but leases will not be persisted to
-disk.
+failed to re-open or re-create the lease file after renaming the
+lease file for lease file cleanup. The server will continue to
+operate but leases will not be persisted to disk.
 
 % DHCPSRV_MEMFILE_LFC_SETUP setting up the Lease File Cleanup interval to %1 sec
 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
+% 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 be abandoned.
+will not take place.
 
 % DHCPSRV_MEMFILE_LFC_SPAWN_FAIL lease file cleanup failed to run because kea-lfc process couldn't be spawned
-An error message logged when spawning kea-lfc application, which performs
-a lease file cleanup, failed. The server will try at the time at which
-the next lease file cleanup is scheduled.
+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
+next time a lease file cleanup is scheduled. Although this message should
+not appear and the reason why it did investigated, the occasional failure
+to start the lease file cleanup will not impact operations. Should the
+failure persist however, the size of the lease file will increase without bound.
 
 % DHCPSRV_MEMFILE_LFC_START starting Lease File Cleanup
 An informational message issued when the Memfile lease database backend
@@ -321,9 +323,7 @@ starts the periodic Lease File Cleanup.
 
 % DHCPSRV_MEMFILE_LFC_EXECUTE executing Lease File Cleanup using: %1
 An informational message issued when the Memfile lease database backend
-starts the new process to perform Lease File Cleanup. Since the cleanup
-is performed in background, it is not guaranteed that the cleanup will
-be successful.
+starts a new process to perform Lease File Cleanup.
 
 % DHCPSRV_MEMFILE_NO_STORAGE running in non-persistent mode, leases will be lost after restart
 A warning message issued when writes of leases to disk have been disabled

src/lib/dhcpsrv/memfile_lease_mgr.cc

@@ -20,6 +20,8 @@
 #include <util/pid_file.h>
 #include <util/process_spawn.h>
 #include <cstdio>
+#include <cstring>
+#include <errno.h>
 #include <iostream>
 #include <sstream>
 
@@ -559,6 +561,9 @@ Memfile_LeaseMgr::lfcSetup() {
         asiolink::IntervalTimer::Callback cb =
             boost::bind(&Memfile_LeaseMgr::lfcCallback, this);
         LOG_INFO(dhcpsrv_logger, DHCPSRV_MEMFILE_LFC_SETUP).arg(lfc_interval);
+        // Multiple the lfc_interval value by 1000 as this value specifies
+        // a timeout in seconds, whereas the setup() method expects the
+        // timeout in milliseconds.
         lfc_timer_.setup(cb, lfc_interval * 1000);
 
         // Start preparing the command line for kea-lfc.
@@ -614,6 +619,8 @@ Memfile_LeaseMgr::lfcCallback() {
 template<typename LeaseFileType>
 void Memfile_LeaseMgr::leaseFileCleanup(boost::shared_ptr<LeaseFileType>& lease_file) {
     bool do_lfc = true;
+    // This string will hold a reason for the failure to rote the lease files.
+    std::string error_string = "(no details)";
     // Check if the copy of the lease file exists already. If it does, it
     // is an indication that another LFC instance may be in progress, in
     // which case we don't want to rotate the current lease file to avoid
@@ -626,6 +633,7 @@ void Memfile_LeaseMgr::leaseFileCleanup(boost::shared_ptr<LeaseFileType>& lease_
         // 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);
 
         // Regardless if we successfully moved the current file or not,
         // we need to re-open the current file for the server to write
@@ -651,6 +659,7 @@ void Memfile_LeaseMgr::leaseFileCleanup(boost::shared_ptr<LeaseFileType>& lease_
             // try to write leases to disk.
             lease_file.reset();
             do_lfc = false;
+            error_string = ex.what();
         }
     }
     // Once we have rotated files as needed, start the new kea-lfc process
@@ -668,7 +677,8 @@ void Memfile_LeaseMgr::leaseFileCleanup(boost::shared_ptr<LeaseFileType>& lease_
     } else {
         LOG_ERROR(dhcpsrv_logger, DHCPSRV_MEMFILE_LFC_ROTATION_FAIL)
             .arg(lease_file->getFilename())
-            .arg(lease_file_copy.getFilename());
+            .arg(lease_file_copy.getFilename())
+            .arg(error_string);
     }
 }
 

src/lib/util/process_spawn.cc

@@ -16,6 +16,7 @@
 #include <util/process_spawn.h>
 #include <util/signal_set.h>
 #include <boost/bind.hpp>
+#include <map>
 #include <signal.h>
 #include <stdlib.h>
 #include <sys/wait.h>
@@ -55,21 +56,25 @@ public:
     /// or when the executable does not exist. If the process ends successfully
     /// the EXIT_SUCCESS is returned.
     ///
+    /// @return PID of the spawned process.
     /// @throw ProcessSpawnError if forking a current process failed.
-    void spawn();
+    pid_t spawn();
 
     /// @brief Checks if the process is still running.
     ///
+    /// @param pid ID of the child processes for which state should be checked.
     /// @return true if the child process is running, false otherwise.
-    bool isRunning() const;
+    bool isRunning(const pid_t pid) const;
 
     /// @brief Returns exit status of the process.
     ///
     /// If the process is still running, the previous status is returned
     /// or 0, if the process is being ran for the first time.
     ///
+    /// @param pid ID of the child process for which exit status should be
+    /// returned.
     /// @return Exit code of the process.
-    int getExitStatus() const;
+    int getExitStatus(const pid_t pid) const;
 
 private:
 
@@ -98,11 +103,7 @@ private:
     /// @brief A signal set installing a handler for SIGCHLD.
     SignalSetPtr signals_;
 
-    /// @brief PID of the child process.
-    pid_t pid_;
-
-    /// @brief Last returned status code by the child process.
-    int status_;
+    std::map<pid_t, int> process_status_;
 
     /// @brief Path to an executable.
     std::string executable_;
@@ -113,7 +114,7 @@ private:
 
 ProcessSpawnImpl::ProcessSpawnImpl(const std::string& executable,
                                    const ProcessArgs& args)
-    : signals_(new SignalSet(SIGCHLD)), pid_(0), status_(0),
+    : signals_(new SignalSet(SIGCHLD)), process_status_(),
       executable_(executable), args_(new char*[args.size() + 2]) {
     // Set the handler which is invoked immediatelly when the signal
     // is received.
@@ -158,13 +159,13 @@ ProcessSpawnImpl::getCommandLine() const {
     return (s.str());
 }
 
-void
+pid_t
 ProcessSpawnImpl::spawn() {
-    pid_ = fork();
-    if (pid_ < 0) {
+    pid_t pid = fork();
+    if (pid < 0) {
         isc_throw(ProcessSpawnError, "unable to fork current process");
 
-    } else if (pid_ == 0) {
+    } else if (pid == 0) {
         // We're in the child process. Run the executable.
         if (execvp(executable_.c_str(), args_) != 0) {
             // We may end up here if the execvp failed, e.g. as a result
@@ -174,16 +175,30 @@ ProcessSpawnImpl::spawn() {
         // Process finished, exit the child process.
         exit(EXIT_SUCCESS);
     }
+
+    process_status_[pid] = 0;
+    return (pid);
 }
 
 bool
-ProcessSpawnImpl::isRunning() const {
-    return ((pid_ != 0) && (kill(pid_, 0) == 0));
+ProcessSpawnImpl::isRunning(const pid_t pid) const {
+    if (process_status_.find(pid) == process_status_.end()) {
+        isc_throw(BadValue, "the process with the pid '" << pid
+                  << "' hasn't been spawned and it status cannnot be"
+                  " returned");
+    }
+    return ((pid != 0) && (kill(pid, 0) == 0));
 }
 
 int
-ProcessSpawnImpl::getExitStatus() const {
-    return (WEXITSTATUS(status_));
+ProcessSpawnImpl::getExitStatus(const pid_t pid) const {
+    std::map<pid_t, int>::const_iterator status = process_status_.find(pid);
+    if (status == process_status_.end()) {
+        isc_throw(BadValue, "the process with the pid '" << pid
+                  << "' hasn't been spawned and it status cannnot be"
+                  " returned");
+    }
+    return (WEXITSTATUS(status->second));
 }
 
 char*
@@ -202,11 +217,11 @@ bool
 ProcessSpawnImpl::waitForProcess(int signum) {
     // We're only interested in SIGCHLD.
     if (signum == SIGCHLD) {
-        status_ = 0;
-        while (wait4(pid_, &status_, WNOHANG, NULL) > 0) {
-            // continue
+        int status = 0;
+        pid_t pid = waitpid(-1, &status, 0);
+        if (pid > 0) {
+            process_status_[pid] = status;
         }
-        pid_ = 0;
         return (true);
     }
     return (false);
@@ -227,19 +242,19 @@ ProcessSpawn::getCommandLine() const {
     return (impl_->getCommandLine());
 }
 
-void
+pid_t
 ProcessSpawn::spawn() {
-    impl_->spawn();
+    return (impl_->spawn());
 }
 
 bool
-ProcessSpawn::isRunning() const {
-    return (impl_->isRunning());
+ProcessSpawn::isRunning(const pid_t pid) const {
+    return (impl_->isRunning(pid));
 }
 
 int
-ProcessSpawn::getExitStatus() const {
-    return (impl_->getExitStatus());
+ProcessSpawn::getExitStatus(const pid_t pid) const {
+    return (impl_->getExitStatus(pid));
 }
 
 }

src/lib/util/process_spawn.h

@@ -17,6 +17,7 @@
 
 #include <exceptions/exceptions.h>
 #include <string>
+#include <sys/types.h>
 #include <vector>
 
 namespace isc {
@@ -80,20 +81,25 @@ public:
     /// the EXIT_SUCCESS is returned.
     ///
     /// @throw ProcessSpawnError if forking a current process failed.
-    void spawn();
+    pid_t spawn();
 
     /// @brief Checks if the process is still running.
     ///
+    /// @param pid ID of the child processes for which state should be checked.
+    ///
     /// @return true if the child process is running, false otherwise.
-    bool isRunning() const;
+    bool isRunning(const pid_t pid) const;
 
     /// @brief Returns exit status of the process.
     ///
     /// If the process is still running, the previous status is returned
     /// or 0, if the process is being ran for the first time.
     ///
+    /// @param pid ID of the child process for which exit status should be
+    /// returned.
+    ///
     /// @return Exit code of the process.
-    int getExitStatus() const;
+    int getExitStatus(const pid_t pid) const;
 
 private:
 

src/lib/util/tests/process_spawn_app.sh

@@ -14,10 +14,30 @@
 # OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR
 # PERFORMANCE OF THIS SOFTWARE.
 
-exit_code=${1}
+exit_code=
 
-if [ $# -eq 0 ]; then
-    exit 32
+while [ ! -z "${1}" ]
+do
+    option=${1}
+    echo ${option}
+    case ${option} in
+        -p)
+            shift
+            exit_code=$$
+            ;;
+        -e)
+            shift
+            exit_code=${1}
+            ;;
+        *)
+            exit 123
+            ;;
+    esac
+    shift
+done
+
+if [ -z "${exit_code}" ]; then
+    exit 32;
 fi
 
 exit ${exit_code}

src/lib/util/tests/process_spawn_unittest.cc

@@ -16,6 +16,7 @@
 #include <gtest/gtest.h>
 #include <signal.h>
 #include <stdint.h>
+#include <sys/types.h>
 #include <unistd.h>
 
 namespace {
@@ -38,13 +39,15 @@ std::string getApp() {
 /// @brief Waits for the specified process to finish.
 ///
 /// @param process An object which started the process.
+/// @param pid ID of the spawned process.
 /// @param timeout Timeout in seconds.
 ///
 /// @return true if the process ended, false otherwise
-bool waitForProcess(const ProcessSpawn& process, const uint8_t timeout) {
+bool waitForProcess(const ProcessSpawn& process, const pid_t pid,
+                    const uint8_t timeout) {
     uint32_t iterations = 0;
     const uint32_t iterations_max = timeout * 1000;
-    while (process.isRunning() && (iterations < iterations_max)) {
+    while (process.isRunning(pid) && (iterations < iterations_max)) {
         usleep(1000);
         ++iterations;
     }
@@ -55,13 +58,32 @@ bool waitForProcess(const ProcessSpawn& process, const uint8_t timeout) {
 // arguments and that the exit code is gathered.
 TEST(ProcessSpawn, spawnWithArgs) {
     std::vector<std::string> args;
+    args.push_back("-e");
     args.push_back("64");
     ProcessSpawn process(getApp(), args);
-    ASSERT_NO_THROW(process.spawn());
+    pid_t pid = 0;
+    ASSERT_NO_THROW(pid = process.spawn());
 
-    ASSERT_TRUE(waitForProcess(process, 2));
+    ASSERT_TRUE(waitForProcess(process, pid, 2));
 
-    EXPECT_EQ(64, process.getExitStatus());
+    EXPECT_EQ(64, process.getExitStatus(pid));
+}
+
+// This test verifies that the single ProcessSpawn object can be used
+// to start two processes and that their status codes can be gathered.
+TEST(ProcessSpawn, spawnTwoProcesses) {
+    std::vector<std::string> args;
+    args.push_back("-p");
+    ProcessSpawn process(getApp(), args);
+    pid_t pid1 = 0;
+    ASSERT_NO_THROW(pid1 = process.spawn());
+    ASSERT_TRUE(waitForProcess(process, pid1, 2));
+
+    pid_t pid2 = 0;
+    ASSERT_NO_THROW(pid2 = process.spawn());
+    ASSERT_TRUE(waitForProcess(process, pid2, 2));
+
+    EXPECT_NE(process.getExitStatus(pid1), process.getExitStatus(pid2));
 }
 
 // This test verifies that the external application can be ran without
@@ -69,11 +91,12 @@ TEST(ProcessSpawn, spawnWithArgs) {
 TEST(ProcessSpawn, spawnNoArgs) {
     std::vector<std::string> args;
     ProcessSpawn process(getApp());
-    ASSERT_NO_THROW(process.spawn());
+    pid_t pid = 0;
+    ASSERT_NO_THROW(pid = process.spawn());
 
-    ASSERT_TRUE(waitForProcess(process, 2));
+    ASSERT_TRUE(waitForProcess(process, pid, 2));
 
-    EXPECT_EQ(32, process.getExitStatus());
+    EXPECT_EQ(32, process.getExitStatus(pid));
 }
 
 
@@ -81,11 +104,12 @@ TEST(ProcessSpawn, spawnNoArgs) {
 // application can't be executed.
 TEST(ProcessSpawn, invalidExecutable) {
     ProcessSpawn process("foo");
-    ASSERT_NO_THROW(process.spawn());
+    pid_t pid = 0;
+    ASSERT_NO_THROW(pid = process.spawn());
 
-    ASSERT_TRUE(waitForProcess(process, 2));
+    ASSERT_TRUE(waitForProcess(process, pid, 2));
 
-    EXPECT_EQ(EXIT_FAILURE, process.getExitStatus());
+    EXPECT_EQ(EXIT_FAILURE, process.getExitStatus(pid));
 }
 
 // This test verifies that the full command line for the process is