[5152] Addressed review comments

diff --git a/src/bin/agent/ca_process.h b/src/bin/agent/ca_process.h
index a6a119bfe4d9eab7da696c26d50943e035adeebb..69de600021ca0d5abf9f3c316e8a8bf855361573 100644 (file)
--- a/src/bin/agent/ca_process.h
+++ b/src/bin/agent/ca_process.h
@@ -84,7 +84,8 @@ public:
     /// of an integer status value (0 means successful, non-zero means failure),
     /// and a string explanation of the outcome.
     virtual isc::data::ConstElementPtr
-    configure(isc::data::ConstElementPtr config_set, bool check_only);
+    configure(isc::data::ConstElementPtr config_set,
+              bool check_only = false);
 
     /// @brief Processes the given command.
     ///

diff --git a/src/bin/d2/d2_messages.mes b/src/bin/d2/d2_messages.mes
index 736e8df2fd5e1b684a0c3b4742c101ed3bdbcf88..9e6649692d92a357c502c27e30a6a237d7b94c8e 100644 (file)
--- a/src/bin/d2/d2_messages.mes
+++ b/src/bin/d2/d2_messages.mes
@@ -37,7 +37,7 @@ to shutdown and has met the required criteria to exit.
 This is a debug message issued when the DHCP-DDNS application command method
 has been invoked.
 
-% DHCP_DDNS_CONFIGURE configuration update received: %1
+% DHCP_DDNS_CONFIGURE configuration %1 received: %2
 This is a debug message issued when the DHCP-DDNS application configure method
 has been invoked.
 

diff --git a/src/bin/d2/d2_process.cc b/src/bin/d2/d2_process.cc
index a786cd3d9fac81603fa42a6e22c67b503770591f..4070ff4856c3e34165aca9661995be950f0a9982 100644 (file)
--- a/src/bin/d2/d2_process.cc
+++ b/src/bin/d2/d2_process.cc
@@ -192,8 +192,9 @@ D2Process::shutdown(isc::data::ConstElementPtr args) {
 
 isc::data::ConstElementPtr
 D2Process::configure(isc::data::ConstElementPtr config_set, bool check_only) {
-    LOG_DEBUG(d2_logger, DBGLVL_TRACE_BASIC,
-              DHCP_DDNS_CONFIGURE).arg(config_set->str());
+    LOG_DEBUG(d2_logger, DBGLVL_TRACE_BASIC, DHCP_DDNS_CONFIGURE)
+        .arg(check_only ? "check" : "update")
+        .arg(config_set->str());
 
     isc::data::ConstElementPtr answer;
     answer = getCfgMgr()->parseConfig(config_set, check_only);;

diff --git a/src/bin/d2/d2_process.h b/src/bin/d2/d2_process.h
index b053083bb4df62f9f4ef961d2700aba1656fa6ae..14a5127389a30a8f865237c1959c0c313bc8c70a 100644 (file)
--- a/src/bin/d2/d2_process.h
+++ b/src/bin/d2/d2_process.h
@@ -158,7 +158,8 @@ public:
     /// of an integer status value (0 means successful, non-zero means failure),
     /// and a string explanation of the outcome.
     virtual isc::data::ConstElementPtr
-    configure(isc::data::ConstElementPtr config_set, bool check_only);
+    configure(isc::data::ConstElementPtr config_set,
+              bool check_only = false);
 
     /// @brief Processes the given command.
     ///

diff --git a/src/lib/process/d_controller.cc b/src/lib/process/d_controller.cc
index b56b4a8bfec695b1a4429e1bffd37453bf9e2c10..28cbf8d4270d98f1882eb6b7f70307ce83dc8590 100644 (file)
--- a/src/lib/process/d_controller.cc
+++ b/src/lib/process/d_controller.cc
@@ -37,7 +37,7 @@ DControllerBasePtr DControllerBase::controller_;
 // Note that the constructor instantiates the controller's primary IOService.
 DControllerBase::DControllerBase(const char* app_name, const char* bin_name)
     : app_name_(app_name), bin_name_(bin_name),
-      verbose_(false), spec_file_name_(""),
+      verbose_(false), check_only_(false), spec_file_name_(""),
       io_service_(new isc::asiolink::IOService()),
       io_signal_queue_() {
 }
@@ -74,60 +74,10 @@ DControllerBase::launch(int argc, char* argv[], const bool test_mode) {
 
     setProcName(bin_name_);
 
-    if (!test_mode && check_only_) {
-        try {
-            // We need to initialize logging, in case any error
-            // messages are to be printed.
-            // This is just a test, so we don't care about lockfile.
-            setenv("KEA_LOCKFILE_DIR", "none", 0);
-            isc::dhcp::CfgMgr::instance().setDefaultLoggerName(bin_name_);
-            isc::dhcp::CfgMgr::instance().setVerbose(verbose_);
-            Daemon::loggerInit(bin_name_.c_str(), verbose_);
-
-            // Check the syntax first.
-            std::string config_file = getConfigFile();
-            if (config_file.empty()) {
-                // Basic sanity check: file name must not be empty.
-                isc_throw(InvalidUsage,
-                          "JSON configuration file not specified");
-            }
-            isc::data::ConstElementPtr whole_config = parseFile(config_file);
-            if (!whole_config) {
-                // No fallback to fromJSONFile
-                isc_throw(InvalidUsage, "No configuration found");
-            }
-            if (verbose_) {
-                std::cerr << "Syntax check OK" << std::endl;
-            }
-
-            // Check the logic next.
-            isc::data::ConstElementPtr module_config;
-            module_config = whole_config->get(getAppName());
-            if (!module_config) {
-                isc_throw(InvalidUsage, "Config file " << config_file <<
-                          " does not include '" << getAppName() << "' entry");
-            }
-
-            // Get an application process object.
-            initProcess();
-
-            isc::data::ConstElementPtr answer;
-            answer = checkConfig(module_config);
-            int rcode = 0;
-            answer = isc::config::parseAnswer(rcode, answer);
-            if (rcode != 0) {
-                isc_throw(InvalidUsage, "Error encountered: "
-                          << answer->stringValue());
-            }
-        } catch (const VersionMessage&) {
-            throw;
-        } catch (const InvalidUsage&) {
-            throw;
-        } catch (const std::exception& ex) {
-            isc_throw(InvalidUsage, "Syntax check failed with: " << ex.what());
-        }
+    if (isCheckOnly()) {
+        checkConfigOnly();
         return;
-    }   
+    }
 
     // It is important that we set a default logger name because this name
     // will be used when the user doesn't provide the logging configuration
@@ -202,6 +152,61 @@ DControllerBase::launch(int argc, char* argv[], const bool test_mode) {
         .arg(app_name_).arg(getpid()).arg(VERSION);
 }
 
+void
+DControllerBase::checkConfigOnly() {
+    try {
+        // We need to initialize logging, in case any error
+        // messages are to be printed.
+        // This is just a test, so we don't care about lockfile.
+        setenv("KEA_LOCKFILE_DIR", "none", 0);
+        isc::dhcp::CfgMgr::instance().setDefaultLoggerName(bin_name_);
+        isc::dhcp::CfgMgr::instance().setVerbose(verbose_);
+        Daemon::loggerInit(bin_name_.c_str(), verbose_);
+
+        // Check the syntax first.
+        std::string config_file = getConfigFile();
+        if (config_file.empty()) {
+            // Basic sanity check: file name must not be empty.
+            isc_throw(InvalidUsage, "JSON configuration file not specified");
+        }
+        isc::data::ConstElementPtr whole_config = parseFile(config_file);
+        if (!whole_config) {
+            // No fallback to fromJSONFile
+            isc_throw(InvalidUsage, "No configuration found");
+        }
+        if (verbose_) {
+            std::cerr << "Syntax check OK" << std::endl;
+        }
+
+        // Check the logic next.
+        isc::data::ConstElementPtr module_config;
+        module_config = whole_config->get(getAppName());
+        if (!module_config) {
+            isc_throw(InvalidUsage, "Config file " << config_file <<
+                      " does not include '" << getAppName() << "' entry");
+        }
+
+        // Get an application process object.
+        initProcess();
+
+        isc::data::ConstElementPtr answer;
+        answer = checkConfig(module_config);
+        int rcode = 0;
+        answer = isc::config::parseAnswer(rcode, answer);
+        if (rcode != 0) {
+            isc_throw(InvalidUsage, "Error encountered: "
+                      << answer->stringValue());
+        }
+    } catch (const VersionMessage&) {
+        throw;
+    } catch (const InvalidUsage&) {
+        throw;
+    } catch (const std::exception& ex) {
+        isc_throw(InvalidUsage, "Syntax check failed with: " << ex.what());
+    }
+    return;
+}
+
 void
 DControllerBase::parseArgs(int argc, char* argv[])
 {

diff --git a/src/lib/process/d_controller.h b/src/lib/process/d_controller.h
index 6b4a3c2507d4b0313b485c91704e2a47838463b7..edd92c70e02c38b6f25578e7a823890dcf6c771b 100644 (file)
--- a/src/lib/process/d_controller.h
+++ b/src/lib/process/d_controller.h
@@ -326,6 +326,13 @@ protected:
         return ("");
     }
 
+    /// @brief Check the configuration
+    ///
+    /// Called by @c launch() when @c check_only_ mode is enabled
+    /// @throw VersionMessage when successful but a message should be displayed
+    /// @throw InvalidUsage when an error was detected
+    void checkConfigOnly();
+
     /// @brief Application-level signal processing method.
     ///
     /// This method is the last step in processing a OS signal occurrence.  It
@@ -367,6 +374,8 @@ protected:
 
     /// @brief Method for enabling or disabling check only mode.
     ///
+    /// @todo this method and @c setVerbose are currently not used.
+    ///
     /// @param value is the new value to assign the flag.
     void setCheckOnly(bool value) {
         check_only_ = value;
@@ -567,7 +576,8 @@ private:
     /// @brief Indicates if the verbose logging mode is enabled.
     bool verbose_;
 
-    /// @brief Indicates if the check only mode is enabled.
+    /// @brief Indicates if the check only mode for the configuration
+    /// is enabled (usually specified by the command line -t argument).
     bool check_only_;
 
     /// @brief The absolute file name of the JSON spec file.

diff --git a/src/lib/process/d_process.h b/src/lib/process/d_process.h
index 7de31b2ce3adb6f56c45484388df6982a5df4e25..ea920e4275ace722a3a4fddd1f699148ca2d4730 100644 (file)
--- a/src/lib/process/d_process.h
+++ b/src/lib/process/d_process.h
@@ -102,7 +102,7 @@ public:
     ///  
     /// @throw DProcessBaseError if an operational error is encountered.
     virtual isc::data::ConstElementPtr 
-        shutdown(isc::data::ConstElementPtr args) = 0;
+    shutdown(isc::data::ConstElementPtr args) = 0;
 
     /// @brief Processes the given configuration.
     ///
@@ -118,7 +118,8 @@ public:
     /// of an integer status value (0 means successful, non-zero means failure),
     /// and a string explanation of the outcome.
     virtual isc::data::ConstElementPtr
-    configure(isc::data::ConstElementPtr config_set, bool check_only) = 0;
+    configure(isc::data::ConstElementPtr config_set,
+              bool check_only = false) = 0;
 
     /// @brief Processes the given command.
     ///