[5100] Addressed review comments.

Also, added a test for testing the hook library modifying a
received control command.

src/bin/dhcp4/dhcp4_hooks.dox

@@ -295,7 +295,7 @@ to the end of this list.
 @subsection dhcpv4HooksControlCommandReceive control_command_receive
 
  - @b Arguments:
-   - name: @b command, type: isc::data::ConstElementPtr, direction: <b>in</b>
+   - name: @b command, type: isc::data::ConstElementPtr, direction: <b>in/out</b>
    - name: @b response, type: isc::data::ConstElementPtr, direction: <b>in/out</b>
 
  - @b Description: this callout is executed when DHCPv4 server receives a
@@ -313,6 +313,10 @@ to the end of this list.
    by the callouts with the list of commands supported by the server. If
    the callout sets the next step action to SKIP in this case, the server
    will only return the list of commands supported by the hook library.
+   The callout can modify the command arguments to influence the command
+   processing by the Command Manager. For example, it may freely modify
+   the configuration received in 'set-config' before it is processed by
+   the server. The SKIP action is not set in this case.
 
  - <b>Next step status</b>: if any callout sets the next step action to SKIP,
    the server will assume that the command has been handled by the callouts

src/bin/dhcp6/dhcp6_hooks.dox

@@ -337,7 +337,7 @@ to the end of this list.
 @subsection dhcpv6HooksControlCommandReceive control_command_receive
 
  - @b Arguments:
-   - name: @b command, type: ConstElementPtr, direction: <b>in</b>
+   - name: @b command, type: ConstElementPtr, direction: <b>in/out</b>
    - name: @b response, type: ConstElementPtr, direction: <b>in/out</b>
 
  - @b Description: this callout is executed when DHCPv4 server receives a
@@ -355,6 +355,10 @@ to the end of this list.
    by the callouts with the list of commands supported by the server. If
    the callout sets the next step action to SKIP in this case, the server
    will only return the list of commands supported by the hook library.
+   The callout can modify the command arguments to influence the command
+   processing by the Command Manager. For example, it may freely modify
+   the configuration received in 'set-config' before it is processed by
+   the server. The SKIP action is not set in this case.
 
  - <b>Next step status</b>: if any callout sets the next step action to SKIP,
    the server will assume that the command has been handled by the callouts

src/lib/config/hooked_command_mgr.cc

@@ -4,9 +4,11 @@
 // License, v. 2.0. If a copy of the MPL was not distributed with this
 // file, You can obtain one at http://mozilla.org/MPL/2.0/.
 
+#include <cc/command_interpreter.h>
 #include <config/hooked_command_mgr.h>
 #include <config/config_log.h>
 #include <hooks/hooks_manager.h>
+#include <boost/pointer_cast.hpp>
 
 using namespace isc::data;
 using namespace isc::hooks;
@@ -48,6 +50,9 @@ HookedCommandMgr::handleCommand(const std::string& cmd_name,
                   "Manager: this is a programming error");
     }
 
+    std::string final_cmd_name = cmd_name;
+    ConstElementPtr final_params = boost::const_pointer_cast<Element>(params);
+
     ConstElementPtr hook_response;
     if (HooksManager::calloutsPresent(Hooks.hook_index_control_command_receive_)) {
 
@@ -57,13 +62,10 @@ HookedCommandMgr::handleCommand(const std::string& cmd_name,
         // Being in this function we don't have access to the original data
         // object holding the whole command (name and arguments). Let's
         // recreate it.
-        ElementPtr original_command = Element::createMap();
-        original_command->set("command", Element::create(cmd_name));
-        original_command->set("arguments", params);
+        ConstElementPtr original_command = createCommand(cmd_name, params);
 
         // And pass it to the hook library.
-        callout_handle_->setArgument("command", boost::dynamic_pointer_cast<
-                                     const Element>(original_command));
+        callout_handle_->setArgument("command", original_command);
         callout_handle_->setArgument("response", hook_response);
 
         HooksManager::callCallouts(Hooks.hook_index_control_command_receive_,
@@ -74,23 +76,31 @@ HookedCommandMgr::handleCommand(const std::string& cmd_name,
 
         // If the hook return 'skip' status, simply return the response.
         if (callout_handle_->getStatus() == CalloutHandle::NEXT_STEP_SKIP) {
-            return (hook_response);
-
-        } else {
             LOG_DEBUG(command_logger, DBG_COMMAND, COMMAND_HOOK_RECEIVE_SKIP)
                 .arg(cmd_name);
+
+            return (hook_response);
+
         }
+
+        // The hook library can modify the command or arguments. Thus, we
+        // retrieve the command returned by the callouts and use it as input
+        // to the local command handler.
+        ConstElementPtr hook_command;
+        callout_handle_->getArgument("command", hook_command);
+        final_cmd_name = parseCommand(final_params, hook_command);
     }
 
     // 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.
-    ConstElementPtr response = BaseCommandMgr::handleCommand(cmd_name, params);
+    ConstElementPtr response = BaseCommandMgr::handleCommand(final_cmd_name,
+                                                             final_params);
 
     // For the 'list-commands' case we will have to combine commands supported
     // by the hook libraries with the commands that this Command Manager supports.
-    if ((cmd_name == "list-commands") && hook_response && response) {
+    if ((final_cmd_name == "list-commands") && hook_response && response) {
         response = combineCommandsLists(hook_response, response);
     }
 

src/lib/config/tests/command_mgr_unittests.cc

@@ -93,7 +93,7 @@ public:
         return (0);
     }
 
-    /// @brief Text callback which stores callout name and passed arguments and
+    /// @brief Test callback which stores callout name and passed arguments and
     /// which handles the command.
     ///
     /// This callout returns the skip status to indicate the the command has
@@ -130,6 +130,38 @@ public:
         return (0);
     }
 
+    /// @brief Test callback which modifies parameters of the command and
+    /// does not return skip status.
+    ///
+    /// This callout is used to test the case when the callout modifies the
+    /// received command and does not set next state SKIP to propagate the
+    /// command with modified parameters to the local command handler.
+    ///
+    /// @param callout_handle Handle passed by the hooks framework.
+    /// @return Always 0.
+    static int
+    control_command_receive_modify_callout(CalloutHandle& callout_handle) {
+        callout_name = "control_command_receive";
+
+        ConstElementPtr command;
+        callout_handle.getArgument("command", command);
+
+        ConstElementPtr arg;
+        std::string command_name = parseCommand(arg, command);
+
+        ElementPtr new_arg = Element::createList();
+        new_arg->add(Element::create("hook-param"));
+        command = createCommand(command_name, new_arg);
+
+        callout_handle.setArgument("command", command);
+
+        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());
+        return (0);
+    }
+
     /// @brief Name of the command (used in my_handler)
     static std::string handler_name;
 
@@ -408,3 +440,53 @@ TEST_F(CommandMgrTest, delegateListCommands) {
     EXPECT_EQ("my-command", command_names_list[1]);
     EXPECT_EQ("my-command-bis", command_names_list[2]);
 }
+
+// This test verifies the scenario in which the hook library influences the
+// command processing by the Kea server. In this test, the callout modifies
+// the arguments of the command and passes the command on to the Command
+// Manager for processing.
+TEST_F(CommandMgrTest, modifyCommandArgsInHook) {
+    // Register callout so as we can check that it is called before
+    // processing the command by the manager.
+    HooksManager::preCalloutsLibraryHandle().registerCallout(
+        "control_command_receive", control_command_receive_modify_callout);
+
+    // Install local handler
+    EXPECT_NO_THROW(CommandMgr::instance().registerCommand("my-command",
+                                                           my_handler));
+
+    // Now tell CommandMgr to process a command 'my-command' with the
+    // specified parameter.
+    ElementPtr my_params = Element::fromJSON("[ \"just\", \"some\", \"data\" ]");
+    ConstElementPtr command = createCommand("my-command", my_params);
+    ConstElementPtr answer;
+    ASSERT_NO_THROW(answer = CommandMgr::instance().processCommand(command));
+
+    // There should be an answer.
+    ASSERT_TRUE(answer);
+
+    // Returned status should be unique for the my_handler.
+    ConstElementPtr answer_arg;
+    int status_code;
+    ASSERT_NO_THROW(answer_arg = parseAnswer(status_code, answer));
+    EXPECT_EQ(123, status_code);
+
+    // Local handler should have been called after the callout.
+    ASSERT_TRUE(handler_called);
+    EXPECT_EQ("my-command", handler_name);
+    ASSERT_TRUE(handler_params);
+    // Check that the local handler received the command with arguments
+    // set by the callout.
+    EXPECT_EQ("[ \"hook-param\" ]", handler_params->str());
+
+
+    // Check that the callout has been called with appropriate parameters.
+    EXPECT_EQ("control_command_receive", 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]);
+
+}