[5330] Addressed some of the review comments.

- Improved commentary, corrected commentary issues.
- Renamed class members in command_mgr_unittests.

src/lib/config/hooked_command_mgr.cc

@@ -84,10 +84,8 @@ HookedCommandMgr::handleCommand(const std::string& cmd_name,
 
     }
 
-    // If we're here it means that the callouts weren't called or the 'skip'
-    // status wasn't returned. The latter is the case when the 'list-commands'
-    // is being processed. Anyhow, we need to handle the command using local
-    // Command Mananger.
+    // If we're here it means that the callouts weren't called. We need
+    // to handle the command using local Command Mananger.
     ConstElementPtr response = BaseCommandMgr::handleCommand(cmd_name,
                                                              params,
                                                              original_cmd);

src/lib/config/hooked_command_mgr.h

@@ -33,7 +33,7 @@ namespace config {
 /// command on its own.
 ///
 /// The @ref isc::hooks::CalloutHandle::CalloutNextStep flag setting by the
-/// command handlers have influence on the operation of the
+/// command handlers does NOT have any influence on the operation of the
 /// @ref HookedCommandMgr, i.e. it will always skip processing command on
 /// its own if the command handlers are present for the given command, even
 /// if the handlers return an error code.

src/lib/config/tests/command_mgr_unittests.cc

@@ -33,9 +33,9 @@ public:
 
         CommandMgr::instance().setIOService(io_service_);
 
-        handler_name = "";
-        handler_params = ElementPtr();
-        handler_called = false;
+        handler_name_ = "";
+        handler_params_ = ElementPtr();
+        handler_called_ = false;
 
         CommandMgr::instance().deregisterAll();
         CommandMgr::instance().closeCommandSocket();
@@ -68,8 +68,8 @@ public:
     ///
     /// It also removes any registered callouts.
     static void resetCalloutIndicators() {
-        callout_name = "";
-        callout_argument_names.clear();
+        callout_name_ = "";
+        callout_argument_names_.clear();
 
         // Iterate over existing hook points and for each of them remove
         // callouts registered.
@@ -83,9 +83,9 @@ public:
     static ConstElementPtr my_handler(const std::string& name,
                                       const ConstElementPtr& params) {
 
-        handler_name = name;
-        handler_params = params;
-        handler_called = true;
+        handler_name_ = name;
+        handler_params_ = params;
+        handler_called_ = true;
 
         return (createAnswer(123, "test error message"));
     }
@@ -106,7 +106,7 @@ public:
     /// @return Always 0.
     static int
     control_command_receive_handle_callout(CalloutHandle& callout_handle) {
-        callout_name = "control_command_receive_handle";
+        callout_name_ = "control_command_receive_handle";
 
         ConstElementPtr command;
         callout_handle.getArgument("command", command);
@@ -117,10 +117,10 @@ public:
         callout_handle.setArgument("response",
                                    createAnswer(234, "text generated by hook handler"));
 
-        callout_argument_names = callout_handle.getArgumentNames();
+        callout_argument_names_ = callout_handle.getArgumentNames();
         // Sort arguments alphabetically, so as we can access them on
         // expected positions and verify.
-        std::sort(callout_argument_names.begin(), callout_argument_names.end());
+        std::sort(callout_argument_names_.begin(), callout_argument_names_.end());
         return (0);
     }
 
@@ -128,35 +128,35 @@ public:
     IOServicePtr io_service_;
 
     /// @brief Name of the command (used in my_handler)
-    static std::string handler_name;
+    static std::string handler_name_;
 
     /// @brief Parameters passed to the handler (used in my_handler)
-    static ConstElementPtr handler_params;
+    static ConstElementPtr handler_params_;
 
     /// @brief Indicates whether my_handler was called
-    static bool handler_called;
+    static bool handler_called_;
 
     /// @brief Holds invoked callout name.
-    static std::string callout_name;
+    static std::string callout_name_;
 
     /// @brief Holds a list of arguments passed to the callout.
-    static std::vector<std::string> callout_argument_names;
+    static std::vector<std::string> callout_argument_names_;
 };
 
 /// Name passed to the handler (used in my_handler)
-std::string CommandMgrTest::handler_name("");
+std::string CommandMgrTest::handler_name_("");
 
 /// Parameters passed to the handler (used in my_handler)
-ConstElementPtr CommandMgrTest::handler_params;
+ConstElementPtr CommandMgrTest::handler_params_;
 
 /// Indicates whether my_handler was called
-bool CommandMgrTest::handler_called(false);
+bool CommandMgrTest::handler_called_(false);
 
 /// Holds invoked callout name.
-std::string CommandMgrTest::callout_name("");
+std::string CommandMgrTest::callout_name_("");
 
 /// Holds a list of arguments passed to the callout.
-std::vector<std::string> CommandMgrTest::callout_argument_names;
+std::vector<std::string> CommandMgrTest::callout_argument_names_;
 
 // Test checks whether the internal command 'list-commands'
 // is working properly.
@@ -302,14 +302,14 @@ TEST_F(CommandMgrTest, processCommand) {
     EXPECT_EQ(123, status_code);
 
     // Check that the parameters passed are correct.
-    EXPECT_EQ(true, handler_called);
-    EXPECT_EQ("my-command", handler_name);
-    ASSERT_TRUE(handler_params);
-    EXPECT_EQ("[ \"just\", \"some\", \"data\" ]", handler_params->str());
+    EXPECT_EQ(true, handler_called_);
+    EXPECT_EQ("my-command", handler_name_);
+    ASSERT_TRUE(handler_params_);
+    EXPECT_EQ("[ \"just\", \"some\", \"data\" ]", handler_params_->str());
 
     // Command handlers not installed so expecting that callouts weren't
     // called.
-    EXPECT_TRUE(callout_name.empty());
+    EXPECT_TRUE(callout_name_.empty());
 }
 
 // Verify that processing a command can be delegated to a hook library.
@@ -335,7 +335,7 @@ TEST_F(CommandMgrTest, delegateProcessCommand) {
 
     // Local handler shouldn't be called because the command is handled by the
     // hook library.
-    ASSERT_FALSE(handler_called);
+    ASSERT_FALSE(handler_called_);
 
     // Returned status should be unique for the hook library.
     ConstElementPtr answer_arg;
@@ -343,13 +343,13 @@ TEST_F(CommandMgrTest, delegateProcessCommand) {
     ASSERT_NO_THROW(answer_arg = parseAnswer(status_code, answer));
     EXPECT_EQ(234, status_code);
 
-    EXPECT_EQ("control_command_receive_handle", callout_name);
+    EXPECT_EQ("control_command_receive_handle", callout_name_);
 
     // Check that the appropriate arguments have been set. Include the
     // 'response' which should have been set by the callout.
-    ASSERT_EQ(2, callout_argument_names.size());
-    EXPECT_EQ("command", callout_argument_names[0]);
-    EXPECT_EQ("response", callout_argument_names[1]);
+    ASSERT_EQ(2, callout_argument_names_.size());
+    EXPECT_EQ("command", callout_argument_names_[0]);
+    EXPECT_EQ("response", callout_argument_names_[1]);
 }
 
 // Verify that 'list-command' command returns combined list of supported

src/lib/hooks/server_hooks.h

@@ -168,7 +168,8 @@ public:
     /// @param command_name Command name for which the hook point name is
     ///        to be generated.
     ///
-    /// @return Hook point name.
+    /// @return Hook point name, or an empty string if the command name
+    /// can't be converted to a hook name (e.g. when it lacks dollar sign).
     static std::string commandToHookName(const std::string& command_name);
 
     /// @brief Returns command name for a specified hook name.