From: Marcin Siodelski Date: Fri, 14 Jul 2017 13:40:36 +0000 (+0200) Subject: [5329] Addressed review comments. X-Git-Tag: trac5124a_base~42^2~1 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=9da1db5384e542b928fb56c1a00614058ff26183;p=thirdparty%2Fkea.git [5329] Addressed review comments. - Extended commentary - Added exception safe ServerHooks::findIndex - Extended unit tests of ServerHooks --- diff --git a/src/lib/hooks/callout_manager.cc b/src/lib/hooks/callout_manager.cc index 7859810ee4..e0b3bc073d 100644 --- a/src/lib/hooks/callout_manager.cc +++ b/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)); diff --git a/src/lib/hooks/callout_manager.h b/src/lib/hooks/callout_manager.h index b8c24df08b..06e9dac39a 100644 --- a/src/lib/hooks/callout_manager.h +++ b/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 diff --git a/src/lib/hooks/library_handle.h b/src/lib/hooks/library_handle.h index e66c7bd39b..f04c95283e 100644 --- a/src/lib/hooks/library_handle.h +++ b/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. diff --git a/src/lib/hooks/server_hooks.cc b/src/lib/hooks/server_hooks.cc index e5e14f2d8a..e8f12ead24 100644 --- a/src/lib/hooks/server_hooks.cc +++ b/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. diff --git a/src/lib/hooks/server_hooks.h b/src/lib/hooks/server_hooks.h index 1ad7a36f61..43f7d7df14 100644 --- a/src/lib/hooks/server_hooks.h +++ b/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. diff --git a/src/lib/hooks/tests/server_hooks_unittest.cc b/src/lib/hooks/tests/server_hooks_unittest.cc index 8e4849c9c0..78d8e13788 100644 --- a/src/lib/hooks/tests/server_hooks_unittest.cc +++ b/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(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