[trac3664] Update per review comments

src/bin/lfc/.gitignore

@@ -0,0 +1,5 @@
+/kea-lfc
+/kea-lfc.8
+/lfc_messages.cc
+/lfc_messages.h
+/s-messages

src/bin/lfc/Makefile.am

@@ -47,8 +47,8 @@ BUILT_SOURCES = lfc_messages.h lfc_messages.cc
 noinst_LTLIBRARIES = liblfc.la
 
 liblfc_la_SOURCES  =
-liblfc_la_SOURCES += lfc.h
-liblfc_la_SOURCES += lfc.cc
+liblfc_la_SOURCES += lfc_controller.h
+liblfc_la_SOURCES += lfc_controller.cc
 
 nodist_liblfc_la_SOURCES = lfc_messages.h lfc_messages.cc
 EXTRA_DIST += lfc_messages.mes

src/bin/lfc/kea-lfc.xml

@@ -1,6 +1,6 @@
 <!DOCTYPE book PUBLIC "-//OASIS//DTD DocBook XML V4.2//EN"
                "http://www.oasis-open.org/docbook/xml/4.2/docbookx.dtd"
-	       [<!ENTITY mdash "&#8212;">]>
+               [<!ENTITY mdash "&#8212;">]>
 <!--
  - Copyright (C) 2015  Internet Systems Consortium, Inc. ("ISC")
  -
@@ -53,6 +53,7 @@
       <arg><option>-v</option></arg>
       <arg><option>-V</option></arg>
       <arg><option>-d</option></arg>
+      <arg><option>-h</option></arg>
     </cmdsynopsis>
   </refsynopsisdiv>
 
@@ -65,7 +66,6 @@
       a stand alone process.  While it can be started externally it
       should be started by the Kea DHCP servers as desired and required.
     </para>
-
   </refsect1>
 
   <refsect1>
@@ -98,6 +98,13 @@
       </varlistentry>
 
       <varlistentry>
+        <term><option>-h</option></term>
+        <listitem><para>
+          Help causes the usage string to be printed.
+        </para></listitem>
+      </varlistentry>
+
+      <varlistentry>
         <term><option>-4 | -6</option></term>
         <listitem><para>
           The protocol version of the lease files, must be one of 4 or 6.
@@ -107,42 +114,57 @@
       <varlistentry>
         <term><option>-c</option></term>
         <listitem><para>
-          Configuration file including the configuration for LFC process.
-          It may also contain configuration entries for other Kea services.
+          Configuration file including the configuration for
+          <command>kea-lfc</command> process.  It may also
+          contain configuration entries for other Kea services.
+          Currently <command>kea-lfc</command> gets all of its arguments from 
+          the comamnd line, in the future it will be extended to get some arguments
+          from the config file.
         </para></listitem>
       </varlistentry>
 
       <varlistentry>
         <term><option>-p</option></term>
         <listitem><para>
-          Previous lease file - When LFC starts this is the result of any previous
-	  run of LFC.
+          Previous lease file - When <command>kea-lfc</command> starts this
+          is the result of any previous run of <command>kea-lfc</command>.
+          When <command>kea-lfc</command> finishes it is the result of this run.
+          If <command>kea-lfc</command> is interrupted before compelting
+          this file may not exist.
         </para></listitem>
       </varlistentry>
 
       <varlistentry>
         <term><option>-i</option></term>
         <listitem><para>
-          Input or copy of lease file - Before the DHCP server invokes LFC it will move
-	  the current lease file to this file and then call LFC with the new file.
+          Input or copy of lease file - Before the DHCP server invokes
+          <command>kea-lfc</command> it will copy the current lease file
+          here and then call <command>kea-lfc</command> with this file.
         </para></listitem>
       </varlistentry>
 
       <varlistentry>
         <term><option>-o</option></term>
         <listitem><para>
-          Output lease file - The temporary file LFC should use to write the leases.
-          Upon completion this file will be moved to the finish file (see below).
+          Output lease file - The temporary file <command>kea-lfc</command>
+          should use to write the leases.  Upon completion of writing this
+          this file it will be moved to the finish file (see below).
         </para></listitem>
       </varlistentry>
 
       <varlistentry>
         <term><option>-f</option></term>
         <listitem><para>
-          Finish or completion file - Another temporary file LFC uses for bookkeeping.
-          When LFC completes writing the output file it moves it to this file name.
-          After LFC finishes deleting the other files (previous and input) it moves
-          this file to previous lease file.
+          Finish or completion file - Another temporary file
+          <command>kea-lfc</command> uses for bookkeeping.  When
+          <command>kea-lfc</command> completes writing the output
+          file it moves it to this file name.  After
+          <command>kea-lfc</command> finishes deleting the other
+          files (previous and input) it moves this file to previous
+          lease file.  By moving the files in this fashion the
+          <command>kea-lfc</command> and the DHCP server processes
+          can determine the correct file to use even if one of the
+          processes was interrupted before completing its task.
         </para></listitem>
       </varlistentry>
     </variablelist>

src/bin/lfc/lfc.cc

@@ -12,10 +12,12 @@
 // OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR
 // PERFORMANCE OF THIS SOFTWARE.
 
-#include <lfc/lfc.h>
+#include <lfc/lfc_controller.h>
 #include <exceptions/exceptions.h>
 #include <config.h>
 #include <iostream>
+#include <sstream>
+#include <unistd.h>
 
 using namespace std;
 
@@ -24,21 +26,21 @@ namespace lfc {
 
 /// @brief Defines the application name, it may be used to locate
 /// configuration data and appears in log statements.
-const char* lfcController::lfc_app_name_ = "DhcpLFC";
+const char* LFCController::lfc_app_name_ = "DhcpLFC";
 
 /// @brief Defines the executable name.
-const char* lfcController::lfc_bin_name_ = "kea-lfc";
+const char* LFCController::lfc_bin_name_ = "kea-lfc";
 
-lfcController::lfcController()
+LFCController::LFCController()
     : protocol_version_(0), verbose_(false), config_file_(""), previous_file_(""),
       copy_file_(""), output_file_(""), finish_file_(""), pid_file_("") {
 }
 
-lfcController::~lfcController() {
+LFCController::~LFCController() {
 }
 
 void
-lfcController::launch(int argc, char* argv[], const bool test_mode) {
+LFCController::launch(int argc, char* argv[]) {
   try {
     parseArgs(argc, argv);
   } catch (const InvalidUsage& ex) {
@@ -48,9 +50,11 @@ lfcController::launch(int argc, char* argv[], const bool test_mode) {
 }
 
 void
-lfcController::parseArgs(int argc, char* argv[]) {
+LFCController::parseArgs(int argc, char* argv[]) {
     int ch;
 
+    opterr = 0;
+    optind = 1;
     while ((ch = getopt(argc, argv, "46dvVp:i:o:c:f:")) != -1) {
         switch (ch) {
         case '4':
@@ -118,8 +122,13 @@ lfcController::parseArgs(int argc, char* argv[]) {
             config_file_ = optarg;
             break;
 
-        default:
+        case 'h':
             usage("");
+            return;
+
+        default:
+            // We should never actually get here
+            isc_throw(InvalidUsage, "Illegal result.");
         }
     }
 
@@ -154,7 +163,7 @@ lfcController::parseArgs(int argc, char* argv[]) {
 
     // If verbose is set echo the input information
     if (verbose_ == true) {
-      std::cerr << "Protocol version:    " << protocol_version_ << std::endl
+      std::cerr << "Protocol version:    DHCPv" << protocol_version_ << std::endl
                 << "Previous lease file: " << previous_file_ << std::endl
                 << "Copy lease file:     " << copy_file_ << std::endl
                 << "Output lease file:   " << output_file_ << std::endl
@@ -165,7 +174,7 @@ lfcController::parseArgs(int argc, char* argv[]) {
 }
 
 void
-lfcController::usage(const std::string& text) {
+LFCController::usage(const std::string& text) {
     if (text != "") {
         std::cerr << "Usage error: " << text << std::endl;
     }
@@ -181,19 +190,20 @@ lfcController::usage(const std::string& text) {
               << "   -v: print version number and exit" << std::endl
               << "   -V: print extended version inforamtion and exit" << std::endl
               << "   -d: optional, verbose output " << std::endl
+              << "   -h: print this message " << std::endl
               << std::endl;
 }
 
 std::string
-lfcController::getVersion(bool extended) {
-    std::stringstream tmp;
+LFCController::getVersion(const bool extended) const{
+    std::stringstream version_stream;
 
-    tmp << VERSION;
+    version_stream << VERSION;
     if (extended) {
-        tmp << std::endl << EXTENDED_VERSION;
+        version_stream << std::endl << EXTENDED_VERSION;
     }
 
-    return (tmp.str());
+    return (version_stream.str());
 }
 
 }; // namespace isc::lfc

src/bin/lfc/lfc.h

@@ -12,11 +12,11 @@
 // OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR
 // PERFORMANCE OF THIS SOFTWARE.
 
-#ifndef LFC_H
-#define LFC_H
+#ifndef LFC_CONTROLLER_H
+#define LFC_CONTROLLER_H
 
-#include <boost/shared_ptr.hpp>
 #include <exceptions/exceptions.h>
+#include <string>
 
 namespace isc {
 namespace lfc {
@@ -28,10 +28,16 @@ public:
         isc::Exception(file, line, what) { };
 };
 
-//class lfcBase;
-//typedef boost::shared_ptr<lfcBase> lfcBasePtr;
-
-class lfcController {
+/// @brief Process controller for LFC process
+/// This class provides the LFC process functions.  These are used to:
+/// manage the command line, check for already running instances,
+/// invoke the code to process the lease files and finally to rename
+/// the lease files as necessary.
+/// @todo The current code simply processes the command line we still need
+/// -# handle PID file manipulation
+/// -# invoke the code to read, process and write the lease files
+/// -# rename and delete the shell files as required
+class LFCController {
 public:
     /// @brief Defines the application name, it may be used to locate
     /// configuration data and appears in log statements.
@@ -42,10 +48,10 @@ public:
     static const char* lfc_bin_name_;
 
     /// @brief Constructor
-    lfcController();
+    LFCController();
 
     /// @brief Destructor
-    ~lfcController();
+    ~LFCController();
 
     /// @brief Acts as the primary entry point to start execution
     /// of the process.  Provides the control logic:
@@ -56,10 +62,20 @@ public:
     /// -# .... TBD
     /// -# remove pid file (TBD)
     /// -# exit to the caller
-    void launch(int argc, char* argv[], const bool test_mode);
+    ///
+    /// @param argc Number of strings in the @c argv array.
+    /// @param argv Array of arguments passed in via the program's main function.
+    ///
+    /// @throw InvalidUsage if the command line parameters are invalid.
+    void launch(int argc, char* argv[]);
 
     /// @brief Process the command line arguments.  It is the first
     /// step taken after the process has been launched.
+    ///
+    /// @param argc Number of strings in the @c argv array.
+    /// @param argv Array of arguments passed in via the program's main function.
+    ///
+    /// @throw InvalidUsage if the command line parameters are invalid.
     void parseArgs(int argc, char* argv[]);
 
     /// @brief Prints the program usage text to std error.
@@ -72,72 +88,77 @@ public:
     ///
     /// @param extended is a boolean indicating if the version string
     /// should be short (false) or extended (true)
-    std::string getVersion(bool extended);
+    std::string getVersion(const bool extended) const;
 
-    // The following are primarly for the test code and not expected
-    // to be used in normal situations
+    /// @name Accessor methods mainly used for testing purposes
+    //@{
 
     /// @brief Gets the protocol version of the leaes files
     ///
-    /// @return Returns the value of the protocol version
+    /// @return Returns the value of the DHCP protocol version.
+    /// This can be 4 or 6 while in use and 0 before parsing
+    /// any arguments.
     int getProtocolVersion() const {
       return (protocol_version_);
     }
 
     /// @brief Gets the config file name
     ///
-    /// @return Returns the name of the config file
+    /// @return Returns the path to the config file
     std::string getConfigFile() const {
         return (config_file_);
     }
 
     /// @brief Gets the prevous file name
     ///
-    /// @return Returns the name of the previous file
+    /// @return Returns the path to the previous file
     std::string getPreviousFile() const {
         return (previous_file_);
     }
 
     /// @brief Gets the copy file name
     ///
-    /// @return Returns the name of the copy file
+    /// @return Returns the path to the copy file
     std::string getCopyFile() const {
         return (copy_file_);
     }
 
     /// @brief Gets the output file name
     ///
-    /// @return Returns the name of the output file
+    /// @return Returns the path to the output file
     std::string getOutputFile() const {
         return (output_file_);
     }
 
     /// @brief Gets the finish file name
     ///
-    /// @return Returns the name of the finish file
+    /// @return Returns the path to the finish file
     std::string getFinishFile() const {
         return (finish_file_);
     }
 
     /// @brief Gets the pid file name
     ///
-    /// @return Returns the name of the pid file
+    /// @return Returns the path to the pid file
     std::string getPidFile() const {
         return (pid_file_);
     }
+    //@}
 
 private:
+    ///< Version of the DHCP protocol used, i.e. 4 or 6.
     int protocol_version_;
+    ///< When true output the result of parsing the comamnd line
     bool verbose_;
-    std::string config_file_;
-    std::string previous_file_;
-    std::string copy_file_;
-    std::string output_file_;
-    std::string finish_file_;
-    std::string pid_file_;
+    std::string config_file_;   ///< The path to the config file
+    std::string previous_file_; ///< The path to the previous LFC file (if any)
+    std::string copy_file_;     ///< The path to the copy of the lease file
+    std::string output_file_;   ///< The path to the output file
+    std::string finish_file_;   ///< The path to the finished output file
+    std::string pid_file_;      ///< The path to the pid file
 };
 
 }; // namespace isc::lfc
 }; // namespace isc
 
-#endif
+#endif // LFC_CONTROLLER_H

src/bin/lfc/main.cc

@@ -12,7 +12,7 @@
 // OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR
 // PERFORMANCE OF THIS SOFTWARE.
 
-#include <lfc/lfc.h>
+#include <lfc/lfc_controller.h>
 #include <exceptions/exceptions.h>
 #include <log/logger_support.h>
 #include <log/logger_manager.h>
@@ -24,19 +24,19 @@ using namespace std;
 
 /// This file contains the entry point (main() function for the
 /// standard LFC process, kea-lfc, component of the Kea software suite.
-/// It fetches the lfccontroller singleton instance and invokes its launch
-/// method.  
+/// It creates an instance of the LFCController class and invokes
+/// its launch method.
 /// The exit value of the program will be EXIT_SUCCESS if there were no
 /// errors, EXIT_FAILURE otherwise.
 int main(int argc, char* argv[]) {
     int ret = EXIT_SUCCESS;
-    lfcController lfcController;
+    LFCController lfc_controller;
 
     // Launch the controller passing in command line arguments.
     // Exit program with the controller's return code.
     try  {
         // 'false' value disables test mode.
-        lfcController.launch(argc, argv, false);
+        lfc_controller.launch(argc, argv);
     } catch (const isc::Exception& ex) {
         std::cerr << "Service failed:" << ex.what() << std::endl;
         ret = EXIT_FAILURE;

src/bin/lfc/tests/.gitignore

@@ -0,0 +1 @@
+/lfc_unittests

src/bin/lfc/tests/lfc_controller_unittests.cc

@@ -12,7 +12,7 @@
 // OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR
 // PERFORMANCE OF THIS SOFTWARE.
 
-#include <lfc/lfc.h>
+#include <lfc/lfc_controller.h>
 #include <gtest/gtest.h>
 
 using namespace isc::lfc;
@@ -20,21 +20,21 @@ using namespace std;
 
 namespace {
 
-TEST(lfcControllerTest, initialValues) {
-    lfcController lfcController;
-
-    // Verify that we start with everything empty
-    EXPECT_EQ(lfcController.getProtocolVersion(), 0);
-    EXPECT_EQ(lfcController.getConfigFile(), "");
-    EXPECT_EQ(lfcController.getPreviousFile(), "");
-    EXPECT_EQ(lfcController.getCopyFile(), "");
-    EXPECT_EQ(lfcController.getOutputFile(), "");
-    EXPECT_EQ(lfcController.getFinishFile(), "");
-    EXPECT_EQ(lfcController.getPidFile(), "");
+TEST(LFCControllerTest, initialValues) {
+    LFCController lfc_controller;
+
+    // Verify that we start with all the private variables empty
+    EXPECT_EQ(lfc_controller.getProtocolVersion(), 0);
+    EXPECT_TRUE(lfc_controller.getConfigFile().empty());
+    EXPECT_TRUE(lfc_controller.getPreviousFile().empty());
+    EXPECT_TRUE(lfc_controller.getCopyFile().empty());
+    EXPECT_TRUE(lfc_controller.getOutputFile().empty());
+    EXPECT_TRUE(lfc_controller.getFinishFile().empty());
+    EXPECT_TRUE(lfc_controller.getPidFile().empty());
 }
 
-TEST(lfcControllerTest, fullCommandLine) {
-    lfcController lfcController;
+TEST(LFCControllerTest, fullCommandLine) {
+    LFCController lfc_controller;
 
     // Verify that standard options can be parsed without error
     char* argv[] = { const_cast<char*>("progName"),
@@ -51,22 +51,23 @@ TEST(lfcControllerTest, fullCommandLine) {
                      const_cast<char*>("finish") };
     int argc = 12;
 
-    EXPECT_NO_THROW(lfcController.parseArgs(argc, argv));
+    ASSERT_NO_THROW(lfc_controller.parseArgs(argc, argv));
 
-    // The parsed data
-    EXPECT_EQ(lfcController.getProtocolVersion(), 4);
-    EXPECT_EQ(lfcController.getConfigFile(), "config");
-    EXPECT_EQ(lfcController.getPreviousFile(), "previous");
-    EXPECT_EQ(lfcController.getCopyFile(), "copy");
-    EXPECT_EQ(lfcController.getOutputFile(), "output");
-    EXPECT_EQ(lfcController.getFinishFile(), "finish");
+    // Check all the parsed data from above to the known values
+    EXPECT_EQ(lfc_controller.getProtocolVersion(), 4);
+    EXPECT_EQ(lfc_controller.getConfigFile(), "config");
+    EXPECT_EQ(lfc_controller.getPreviousFile(), "previous");
+    EXPECT_EQ(lfc_controller.getCopyFile(), "copy");
+    EXPECT_EQ(lfc_controller.getOutputFile(), "output");
+    EXPECT_EQ(lfc_controller.getFinishFile(), "finish");
 }
 
-TEST(lfcControllerTest, notEnoughData) {
-    lfcController lfcController;
+TEST(LFCControllerTest, notEnoughData) {
+    LFCController lfc_controller;
 
-    // The standard options we shall test what happens
-    // if we don't include all of them
+    // Test the results if we don't include all of the required arguments
+    // This argument list is correct but we shall only suppy part of it
+    // to the parse routine via the argc variable.
     char* argv[] = { const_cast<char*>("progName"),
                      const_cast<char*>("-4"),
                      const_cast<char*>("-p"),
@@ -79,44 +80,21 @@ TEST(lfcControllerTest, notEnoughData) {
                      const_cast<char*>("config"),
                      const_cast<char*>("-f"),
                      const_cast<char*>("finish") };
-    int argc = 1;
-
-    EXPECT_THROW(lfcController.parseArgs(argc, argv), InvalidUsage);
-
-    argc = 2;
-    EXPECT_THROW(lfcController.parseArgs(argc, argv), InvalidUsage);
-
-    argc = 3;
-    EXPECT_THROW(lfcController.parseArgs(argc, argv), InvalidUsage);
-
-    argc = 4;
-    EXPECT_THROW(lfcController.parseArgs(argc, argv), InvalidUsage);
-
-    argc = 5;
-    EXPECT_THROW(lfcController.parseArgs(argc, argv), InvalidUsage);
-
-    argc = 6;
-    EXPECT_THROW(lfcController.parseArgs(argc, argv), InvalidUsage);
 
-    argc = 7;
-    EXPECT_THROW(lfcController.parseArgs(argc, argv), InvalidUsage);
-
-    argc = 8;
-    EXPECT_THROW(lfcController.parseArgs(argc, argv), InvalidUsage);
-
-    argc = 9;
-    EXPECT_THROW(lfcController.parseArgs(argc, argv), InvalidUsage);
-
-    argc = 10;
-    EXPECT_THROW(lfcController.parseArgs(argc, argv), InvalidUsage);
+    int argc = 1;
 
-    argc = 11;
-    EXPECT_THROW(lfcController.parseArgs(argc, argv), InvalidUsage);
+    for (; argc < 12; ++argc) {
+        EXPECT_THROW(lfc_controller.parseArgs(argc, argv), InvalidUsage)
+            << "test failed for argc = " << argc;
+    }
 
+    // Verify we can still parse the full string properly
+    ASSERT_NO_THROW(lfc_controller.parseArgs(argc, argv));
 }
 
-TEST(lfcControllerTest, tooMuchData) {
-    lfcController lfcController;
+
+TEST(LFCControllerTest, tooMuchData) {
+    LFCController lfc_controller;
 
     // The standard options plus some others
 
@@ -138,13 +116,14 @@ TEST(lfcControllerTest, tooMuchData) {
     };
     int argc = 15;
 
-    EXPECT_THROW(lfcController.parseArgs(argc, argv), InvalidUsage);
+    // We expect an error as we have arguments that aren't valid
+    EXPECT_THROW(lfc_controller.parseArgs(argc, argv), InvalidUsage);
 }
 
-TEST(lfcControllerTest, someBadData) {
-    lfcController lfcController;
+TEST(LFCControllerTest, someBadData) {
+    LFCController lfc_controller;
 
-    // The standard options plus some others
+    // Some random arguments
 
     char* argv[] = { const_cast<char*>("progName"),
                      const_cast<char*>("some"),
@@ -153,7 +132,8 @@ TEST(lfcControllerTest, someBadData) {
     };
     int argc = 4;
 
-    EXPECT_THROW(lfcController.parseArgs(argc, argv), InvalidUsage);
+    // We expect an error as the arguments aren't valid
+    EXPECT_THROW(lfc_controller.parseArgs(argc, argv), InvalidUsage);
 }
 
 } // end of anonymous namespace