[5329] Addressed review comments.

- Extended commentary
- Added exception safe ServerHooks::findIndex
- Extended unit tests of ServerHooks

src/lib/hooks/callout_manager.cc

@@ -106,24 +106,22 @@ CalloutManager::calloutsPresent(int hook_index) const {
 
 bool
 CalloutManager::commandHandlersPresent(const std::string& command_name) const {
-    try {
-        // Check if the hook point for the specified command exists.
-        // We don't want this to throw because this is not an error condition.
-        // We may simply not support this command in any of the attached
-        // hooks libraries. That's fine.
-        int index = ServerHooks::getServerHooks().getIndex(
-                        ServerHooks::commandToHookName(command_name));
-        // The hook point may exist but there are no callouts/command handlers.
-        // This is possible if there was a hook library supporting this command
-        // attached, but it was later unloaded. The hook points are not deregistered
-        // in this case. Only callouts are deregistered.
+    // Check if the hook point for the specified command exists.
+    int index = ServerHooks::getServerHooks().findIndex(
+                    ServerHooks::commandToHookName(command_name));
+    if (index >= 0) {
+        // The hook point exits but it is possible that there are no
+        // callouts/command handlers. This is possible if there was a
+        // hook library supporting this command attached, but it was
+        // later unloaded. The hook points are not deregistered in
+        // this case. Only callouts are deregistered.
+        // Let's check if callouts are present for this hook point.
         return (calloutsPresent(index));
-
-    } catch (...) {
-        // Hook point not created, so we don't support this command in
-        // any of the hooks libraries.
-        return (false);
     }
+
+    // Hook point not created, so we don't support this command in
+    // any of the hooks libraries.
+    return (false);
 }
 
 
@@ -218,7 +216,7 @@ void
 CalloutManager::callCommandHandlers(const std::string& command_name,
                                     CalloutHandle& callout_handle) {
     // Get the index of the hook point for the specified command.
-    // This may throw an exception if the hook point doesn't exist.
+    // This will throw an exception if the hook point doesn't exist.
     // The caller should check if the hook point exists by calling
     // commandHandlersPresent.
     int index = ServerHooks::getServerHooks().getIndex(
@@ -312,15 +310,7 @@ CalloutManager::deregisterAllCallouts(const std::string& name) {
 void
 CalloutManager::registerCommandHook(const std::string& command_name) {
     ServerHooks& hooks = ServerHooks::getServerHooks();
-    int hook_index = -1;
-    try {
-        hook_index = hooks.getIndex(ServerHooks::commandToHookName(command_name));
-
-    } catch (...) {
-        // Ignore an error whereby the hook doesn't exist for this command.
-        // In this case we're going to register a new hook.
-    }
-
+    int hook_index = hooks.findIndex(ServerHooks::commandToHookName(command_name));
     if (hook_index < 0) {
         // Hook for this command doesn't exist. Let's create one.
         hooks.registerHook(ServerHooks::commandToHookName(command_name));

src/lib/hooks/callout_manager.h

@@ -105,6 +105,18 @@ public:
 /// between hook points dedicated to commands handling and other (fixed)
 /// hook points.
 ///
+/// Prefixing hook names for command handlers with a dollar sign precludes
+/// auto registration of command handlers, i.e. hooks framework is unable
+/// to match hook points with names of functions implementing command
+/// handlers, because the dollar sign is not legal in C++ function names.
+/// This is intended because we want hook libraries to explicitly register
+/// commands handlers for supported commands and not rely on Kea to register
+/// hook points for them. Should we find use cases for auto registration of
+/// command handlers, we may modify the
+/// @ref ServerHooks::commandToHookName to use an encoding of hook
+/// point names for command handlers that would only contain characters
+/// allowed in function names.
+///
 /// The @ref CalloutManager::registerCommandHook has been added to allow for
 /// dynamically creating hook points for which command handlers are registered.
 /// This method is called from the @ref LibraryHandle::registerCommandHandler

src/lib/hooks/library_handle.h

@@ -54,8 +54,8 @@ extern "C" {
 /// @endcode
 ///
 /// which will result in automatic creation of the hook point for the command
-/// and associating the callout 'foo_bar_handler' with this hook point as
-/// a handler for the command.
+/// (if one doesn't exist) and associating the callout 'foo_bar_handler' with
+/// this hook point as a handler for the command.
 
 class LibraryHandle {
 public:
@@ -98,9 +98,9 @@ public:
     /// @brief Register control command handler
     ///
     /// Registers control command handler by creating a hook point for this
-    /// command and associating the callout as a command handler. It is possible
-    /// to register multiple command handlers for the same control command because
-    /// command handlers are implemented as callouts.
+    /// command (if it doesn't exist) and associating the callout as a command
+    /// handler. It is possible to register multiple command handlers for the
+    /// same control command because command handlers are implemented as callouts.
     ///
     /// @param command_name Command name for which handler should be installed.
     /// @param callout Pointer to the command handler implemented as a callout.

src/lib/hooks/server_hooks.cc

@@ -138,6 +138,13 @@ ServerHooks::getIndex(const string& name) const {
     return (i->second);
 }
 
+int
+ServerHooks::findIndex(const std::string& name) const {
+    // Get iterator to matching element.
+    auto i = hooks_.find(name);
+    return ((i == hooks_.end()) ? -1 : i->second);
+}
+
 // Return vector of hook names.  The names are not sorted - it is up to the
 // caller to perform sorting if required.
 

src/lib/hooks/server_hooks.h

@@ -113,6 +113,17 @@ public:
     /// @throw NoSuchHook if the hook name is unknown to the caller.
     int getIndex(const std::string& name) const;
 
+    /// @brief Find hook index
+    ///
+    /// Provides exception safe method of retrieving an index of the
+    /// specified hook.
+    ///
+    /// @param name Name of the hook
+    ///
+    /// @return Index of the hook if the hook point exists, or -1 if the
+    /// hook point doesn't exist.
+    int findIndex(const std::string& name) const;
+
     /// @brief Return number of hooks
     ///
     /// Returns the total number of hooks registered.

src/lib/hooks/tests/server_hooks_unittest.cc

@@ -28,7 +28,9 @@ TEST(ServerHooksTest, RegisterHooks) {
     // There should be two hooks already registered, with indexes 0 and 1.
     EXPECT_EQ(2, hooks.getCount());
     EXPECT_EQ(0, hooks.getIndex("context_create"));
+    EXPECT_EQ(0, hooks.findIndex("context_create"));
     EXPECT_EQ(1, hooks.getIndex("context_destroy"));
+    EXPECT_EQ(1, hooks.findIndex("context_destroy"));
 
     // Check that the constants are as expected. (The intermediate variables
     // are used because of problems with g++ 4.6.1/Ubuntu 11.10 when resolving
@@ -177,6 +179,7 @@ TEST(ServerHooksTest, UnknownHookName) {
     hooks.reset();
 
     EXPECT_THROW(static_cast<void>(hooks.getIndex("unknown")), NoSuchHook);
+    EXPECT_EQ(-1, hooks.findIndex("unknown"));
 }
 
 // Check that the count of hooks is correct.
@@ -200,6 +203,7 @@ TEST(ServerHooksTest, HookCount) {
 TEST(ServerHooksTest, CommandToHookName) {
     EXPECT_EQ("$x_y_z", ServerHooks::commandToHookName("x-y-z"));
     EXPECT_EQ("$foo_bar_foo", ServerHooks::commandToHookName("foo-bar_foo"));
+    EXPECT_EQ("$", ServerHooks::commandToHookName(""));
 }
 
 } // Anonymous namespace