[trac3687] Update per first set of review comments

src/bin/lfc/lfc_controller.cc

@@ -55,9 +55,15 @@ LFCController::launch(int argc, char* argv[]) {
 
     // verify we are the only instance
     PIDFile pid_file(pid_file_);
-    if (pid_file.check() == true) {
-        // Already running instance, bail out
-        std::cerr << "LFC instance already running" <<  std::endl;
+
+    try {
+        if (pid_file.check() == true) {
+            // Already running instance, bail out
+            std::cerr << "LFC instance already running" <<  std::endl;
+            return;
+        }
+    } catch (const PIDFileError& pid_ex) {
+        std::cerr << pid_ex.what() << std::endl;
         return;
     }
 
@@ -73,7 +79,12 @@ LFCController::launch(int argc, char* argv[]) {
     std::cerr << "Add code to perform lease cleanup" << std::endl;
 
     // delete the pid file for this instance
-    pid_file.deleteFile();
+    try {
+        pid_file.deleteFile();
+    } catch (const PIDFileError& pid_ex) {
+        std::cerr << pid_ex.what() << std::endl;
+        return;
+    }
 
     std::cerr << "LFC complete" << std::endl;
 }

src/lib/util/pid_file.cc

@@ -28,14 +28,14 @@ PIDFile::~PIDFile() {
 }
 
 bool
-PIDFile::check() {
+PIDFile::check() const {
     std::ifstream fs(filename_.c_str());
     int pid;
     bool good;
 
     // If we weren't able to open the file treat
     // it as if the process wasn't running
-    if (fs.is_open() == false) {
+    if (!fs.is_open()) {
         return (false);
     }
 
@@ -44,9 +44,14 @@ PIDFile::check() {
     good = fs.good();
     fs.close();
 
-    // Treat being unable to read a pid the same as if we
-    // got a pid and the process is still running.
-    if ((good == false) || (kill(pid, 0) == 0)) {
+    // If we weren't able to read a pid send back an execption
+    if (!good) {
+        isc_throw(PIDCantReadPID, "Unable to read PID from file '"
+                  << filename_ << "'");
+    }
+
+    // If the process is still running return true
+    if (kill(pid, 0) == 0) {
         return (true);
     }
 
@@ -55,28 +60,32 @@ PIDFile::check() {
 }
 
 void
-PIDFile::write() {
+PIDFile::write() const {
     write(getpid());
 }
 
 void
-PIDFile::write(int pid) {
-    std::ofstream fs(filename_.c_str());
+PIDFile::write(int pid) const {
+  std::ofstream fs(filename_.c_str(), std::ofstream::trunc);
 
-    if (fs.is_open() == false) {
+    if (!fs.is_open()) {
         isc_throw(PIDFileError, "Unable to open PID file '"
                   << filename_ << "' for write");
     }
 
     // File is open, write the pid.
     fs << pid << std::endl;
+    if (!fs.good()) {
+        isc_throw(PIDFileError, "Unable to write to PID file '"
+                  << filename_ << "'");
+    }
 
     // That's it
     fs.close();
 }
 
 void
-PIDFile::deleteFile() {
+PIDFile::deleteFile() const {
     if (remove(filename_.c_str()) != 0) {
         isc_throw(PIDFileError, "Unable to delete PID file '"
                   << filename_ << "'");

src/lib/util/pid_file.h

@@ -30,13 +30,20 @@ public:
         isc::Exception(file, line, what) { };
 };
 
+/// @brief Exception thrown when an error occurs trying to read a PID
+/// from an opened file.
+class PIDCantReadPID : public Exception {
+public:
+    PIDCantReadPID(const char* file, size_t line, const char* what) :
+        isc::Exception(file, line, what) { };
+};
+
 /// @brief Class to help with processing PID files
 ///
 /// This is a utility class to manipulate PID file. It provides
 /// functions for writing and deleting a file containing a PID as
 /// well as for extracting a PID from a file and checking if the
 /// process is still running.
-
 class PIDFile {
 public:
     /// @brief Constructor
@@ -56,16 +63,27 @@ public:
     /// the proper format treat it as the process existing.
     ///
     /// @return true if the PID is in use, false otherwise
-    bool check();
+    ///
+    /// @throw throws PIDCantReadPID if it was able to open the file
+    /// but was unable to read the PID from it.
+    bool check() const;
 
     /// @brief Write the PID to the file.
-    void write(int);
+    ///
+    /// @param pid the pid to write to the file.
+    ///
+    /// @throw throws PIDFileError if it can't open or write to the PID file.
+    void write(int) const;
 
     /// @brief Get PID of the current process and write it to the file.
-    void write();
+    ///
+    /// @throw throws PIDFileError if it can't open or write to the PID file.
+    void write() const;
 
     /// @brief Delete the PID file.
-    void deleteFile();
+    ///
+    /// @throw throws PIDFileError if it can't delete the PID file
+    void deleteFile() const;
 
     /// @brief Returns the path to the PID file.
     std::string getFilename() const {

src/lib/util/tests/pid_file_unittest.cc

@@ -19,7 +19,62 @@
 namespace {
 using namespace isc::util;
 
-#define TESTNAME "./pid_file.test"
+#define TESTNAME "pid_file.test"
+#define TESTNAME2 "pid_file.test.2"
+
+class PIDFileTest : public ::testing::Test {
+public:
+    /// @brief Prepends the absolute path to the file specified
+    /// as an argument.
+    ///
+    /// @param filename Name of the file.
+    /// @return Absolute path to the test file.
+    static std::string absolutePath(const std::string& filename);
+
+    /// @brief Generate a random number for use as a PID
+    ///
+    /// @param start - the start of the range we want the PID in
+    /// @param range - the size of the range for our PID
+    ///
+    /// @return returns a random value between start and start + range
+    int randomizePID(const uint32_t start, const uint32_t range) {
+        int pid;
+
+        for (pid = (random() % range) + start;
+             kill(pid, 0) == 0;
+             ++pid)
+            ;
+
+        return (pid);
+    }
+
+protected:
+    /// @brief Removes any old test files before the test
+    virtual void SetUp() {
+        removeTestFile();
+    }
+
+    /// @brief Removes any remaining test files after the test
+    virtual void TearDown() {
+        removeTestFile();
+    }
+
+private:
+    /// @brief Removes any remaining test files
+    void removeTestFile() const {
+        remove(TESTNAME);
+    }
+
+};
+
+std::string
+PIDFileTest::absolutePath(const std::string& filename) {
+    std::ostringstream s;
+    s << TEST_DATA_BUILDDIR << "/" << filename;
+
+    std::cerr << s.str() << std::endl;
+    return (s.str());
+}
 
 /// @brief Test file writing and deletion. Start by removing
 /// any leftover file. Then write a known PID to the file and
@@ -27,19 +82,16 @@ using namespace isc::util;
 /// a second and verify a second PID to verify that an existing
 /// file is properly overwritten.
 
-TEST(PIDFileTest, writeAndDelete) {
-    PIDFile pid_file(TESTNAME);
+TEST_F(PIDFileTest, writeAndDelete) {
+    PIDFile pid_file(absolutePath(TESTNAME));
     std::ifstream fs;
     int pid(0);
 
-    // Remove any leftovers
-    remove(TESTNAME);
-
     // Write a known process id
     pid_file.write(10);
 
     // Read the file and compare the pid
-    fs.open(TESTNAME, std::ifstream::in);
+    fs.open(absolutePath(TESTNAME), std::ifstream::in);
     fs >> pid;
     EXPECT_TRUE(fs.good());
     EXPECT_EQ(pid, 10);
@@ -49,7 +101,7 @@ TEST(PIDFileTest, writeAndDelete) {
     pid_file.write(20);
 
     // And comapre the second pid
-    fs.open(TESTNAME, std::ifstream::in);
+    fs.open(absolutePath(TESTNAME), std::ifstream::in);
     fs >> pid;
     EXPECT_TRUE(fs.good());
     EXPECT_EQ(pid, 20);
@@ -59,7 +111,7 @@ TEST(PIDFileTest, writeAndDelete) {
     pid_file.deleteFile();
 
     // And verify that it's gone
-    fs.open(TESTNAME, std::ifstream::in);
+    fs.open(absolutePath(TESTNAME), std::ifstream::in);
     EXPECT_FALSE(fs.good());
     fs.close();
 }
@@ -67,17 +119,14 @@ TEST(PIDFileTest, writeAndDelete) {
 /// @brief Test checking a PID. Write the PID of the current
 /// process to the PID file then verify that check indicates
 /// the process is running.
-TEST(PIDFileTest, pidInUse) {
-    PIDFile pid_file(TESTNAME);
+TEST_F(PIDFileTest, pidInUse) {
+    PIDFile pid_file(absolutePath(TESTNAME));
 
     // Write the current PID
     pid_file.write();
 
     // Check if we think the process is running
     EXPECT_TRUE(pid_file.check());
-
-    // Delete the file
-    pid_file.deleteFile();
 }
 
 /// @brief Test checking a PID. Write a PID that isn't in use
@@ -87,64 +136,53 @@ TEST(PIDFileTest, pidInUse) {
 /// errors if the first call to check fails we try again with a
 /// different range of values and only if both attempts fail do
 /// we declare the test to have failed.
-TEST(PIDFileTest, pidNotInUse) {
-    PIDFile pid_file(TESTNAME);
+TEST_F(PIDFileTest, pidNotInUse) {
+    PIDFile pid_file(absolutePath(TESTNAME));
     int pid;
 
     // get a pid betwen 10000 and 20000
-    for (pid = (random() % 10000) + 10000;
-        kill(pid, 0) == 0;
-        ++pid)
-        ;
+    pid = randomizePID(10000, 10000);
 
     // write it
     pid_file.write(pid);
 
     // Check to see if we think the process is running
-    if (pid_file.check() == false) {
-        // Delete the file
-        pid_file.deleteFile();
+    if (!pid_file.check()) {
         return;
     }
 
     // get a pid betwen 40000 and 50000
-    for (pid = (random() % 10000) + 40000;
-        kill(pid, 0) == 0;
-        ++pid)
-        ;
+    pid = randomizePID(10000, 40000);
 
     // write it
     pid_file.write(pid);
 
     // Check to see if we think the process is running
     EXPECT_FALSE(pid_file.check());
-
-    // Delete the file
-    pid_file.deleteFile();
 }
 
 /// @brief Test checking a PID.  Write garbage to the PID file
 /// and verify that check throws an error. In this situation
 /// the caller should probably log an error and may decide to
 /// continue or not depending on the requirements.
-TEST(PIDFileTest, pidGarbage) {
-    PIDFile pid_file(TESTNAME);
+TEST_F(PIDFileTest, pidGarbage) {
+    PIDFile pid_file(absolutePath(TESTNAME));
     std::ofstream fs;
 
-    // Remove any leftovers
-    remove(TESTNAME);
-
     // Open the file and write garbage to it
-    fs.open(TESTNAME, std::ofstream::out);
+    fs.open(absolutePath(TESTNAME), std::ofstream::out);
     fs << "text" << std::endl;
     fs.close();
 
-    // Run the check, we expect to find a process "running"
-    EXPECT_TRUE(pid_file.check());
+    // Run the check, we expect to get an execption
+    EXPECT_THROW(pid_file.check(), PIDCantReadPID);
+}
 
-    // And clean up the file
-    pid_file.deleteFile();
+/// @brief Test failing to write a file.
+TEST_F(PIDFileTest, pidWriteFail) {
+    PIDFile pid_file(absolutePath(TESTNAME2));
 
+    EXPECT_THROW(pid_file.write(10), PIDFileError);
 }
 
 } // end of anonymous namespace