From: Marcin Siodelski Date: Tue, 18 Jul 2017 15:44:33 +0000 (+0200) Subject: [5330] Addressed some of the review comments. X-Git-Tag: trac5124a_base~38^2~2 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=3aca9f70b89841ba48282b012492f548eb79feb9;p=thirdparty%2Fkea.git [5330] Addressed some of the review comments. - Improved commentary, corrected commentary issues. - Renamed class members in command_mgr_unittests. --- diff --git a/src/lib/config/hooked_command_mgr.cc b/src/lib/config/hooked_command_mgr.cc index b5d6186115..9a8e091c6a 100644 --- a/src/lib/config/hooked_command_mgr.cc +++ b/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); diff --git a/src/lib/config/hooked_command_mgr.h b/src/lib/config/hooked_command_mgr.h index 7154140e1c..8b8dcb4e33 100644 --- a/src/lib/config/hooked_command_mgr.h +++ b/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. diff --git a/src/lib/config/tests/command_mgr_unittests.cc b/src/lib/config/tests/command_mgr_unittests.cc index db251c0b23..6af26ee480 100644 --- a/src/lib/config/tests/command_mgr_unittests.cc +++ b/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 callout_argument_names; + static std::vector 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 CommandMgrTest::callout_argument_names; +std::vector 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 diff --git a/src/lib/hooks/server_hooks.h b/src/lib/hooks/server_hooks.h index edd5ab672b..bf100cdb15 100644 --- a/src/lib/hooks/server_hooks.h +++ b/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.