[2980] Miscellaneous changes

* Catch user-library-generated exceptions.
* Remove hook registration function (no longer needed with a singleton
  ServerHooks object.
* Miscellaneous documentation changes.

doc/Doxyfile

@@ -679,6 +679,7 @@ INPUT                  = ../src/lib/exceptions \
                          ../src/lib/cache \
                          ../src/lib/server_common/ \
                          ../src/bin/sockcreator/ \
+                         ../src/lib/hooks/ \
                          ../src/lib/util/ \
                          ../src/lib/util/io/ \
                          ../src/lib/util/threads/ \

src/lib/hooks/callout_handle.cc

@@ -52,14 +52,13 @@ CalloutHandle::~CalloutHandle() {
     context_collection_.clear();
 
     // Normal destruction of the remaining variables will include the
-    // of the reference count on the library manager collection (which holds
-    // the libraries that could have allocated memory in the argument and
-    // context members).  When that goes to zero, the libraries will be
-    // unloaded: however, at that point nothing in the hooks framework will
-    // access memory in the libraries' address space.  It is possible that some
-    // other data structure in the server (the program using the hooks library)
-    // still references address space, but that is outside the scope of this
-    // framework.
+    // decrementing of the reference count on the library manager collection
+    // (which holds the libraries that could have allocated memory in the
+    // argument and context members).  When that goes to zero, the libraries
+    // will be unloaded: however, at that point nothing in the hooks framework
+    // will be accessing memory in the libraries' address space.  It is possible    // that some other data structure in the server (the program using the hooks
+    // library) still references the address space, but that is outside the
+    // scope of this framework.
 }
 
 // Return the name of all argument items.
@@ -147,8 +146,9 @@ CalloutHandle::getHookName() const {
     try {
         hook = ServerHooks::getServerHooks().getName(index);
     } catch (const NoSuchHook&) {
-        // Hook index is invalid, so probably called outside of a callout.
-        // This is a no-op.
+        // Hook index is invalid, so this methods probably called from outside
+        // a callout being executed via a call to CalloutManager::callCallouts.
+        // In this case, the empty string is returned.
     }
 
     return (hook);

src/lib/hooks/callout_manager.cc

@@ -29,9 +29,9 @@ namespace isc {
 namespace hooks {
 
 // Check that the index of a library is valid.  It can range from 1 - n
-// (n is the number of libraries) or it can be 0 (pre-user library callouts)
-// of INT_MAX (post-user library callouts).  It can also be -1 to indicate
-// an invalid value.
+// (n is the number of libraries), 0 (pre-user library callouts), or INT_MAX
+// (post-user library callouts).  It can also be -1 to indicate an invalid
+// value.
 
 void
 CalloutManager::checkLibraryIndex(int library_index) const {
@@ -138,15 +138,24 @@ CalloutManager::callCallouts(int hook_index, CalloutHandle& callout_handle) {
             current_library_ = i->first;
 
             // Call the callout
-            // @todo Log the return status if non-zero
-            int status = (*i->second)(callout_handle);
-            if (status == 0) {
-                LOG_DEBUG(hooks_logger, HOOKS_DBG_EXTENDED_CALLS, HOOKS_CALLOUT)
-                    .arg(current_library_)
-                    .arg(ServerHooks::getServerHooks().getName(current_hook_))
-                    .arg(reinterpret_cast<void*>(i->second));
-            } else {
-                LOG_WARN(hooks_logger, HOOKS_CALLOUT_ERROR)
+            try {
+                int status = (*i->second)(callout_handle);
+                if (status == 0) {
+                    LOG_DEBUG(hooks_logger, HOOKS_DBG_EXTENDED_CALLS,
+                              HOOKS_CALLOUT).arg(current_library_)
+                        .arg(ServerHooks::getServerHooks()
+                            .getName(current_hook_))
+                        .arg(reinterpret_cast<void*>(i->second));
+                } else {
+                    LOG_ERROR(hooks_logger, HOOKS_CALLOUT_ERROR)
+                        .arg(current_library_)
+                        .arg(ServerHooks::getServerHooks()
+                            .getName(current_hook_))
+                        .arg(reinterpret_cast<void*>(i->second));
+                }
+            } catch (...) {
+                // Any exception, not just ones based on isc::Exception
+                LOG_ERROR(hooks_logger, HOOKS_CALLOUT_EXCEPTION)
                     .arg(current_library_)
                     .arg(ServerHooks::getServerHooks().getName(current_hook_))
                     .arg(reinterpret_cast<void*>(i->second));

src/lib/hooks/callout_manager.h

@@ -71,7 +71,7 @@ public:
 ///
 /// The library index is important because it determines in what order callouts
 /// on a particular hook are called.  For each hook, the CalloutManager
-/// maintains a vector of callouts, ordered by library index.  When a callout
+/// maintains a vector of callouts ordered by library index.  When a callout
 /// is added to the list, it is added at the end of the callouts associated
 /// with the current library.  To clarify this further, suppose that three
 /// libraries are loaded, A (assigned an index 1), B (assigned an index 2) and
@@ -87,22 +87,21 @@ public:
 /// loaded) and are assigned to libraries based on the order the libraries
 /// presented to the hooks framework for loading (something that occurs in the
 /// isc::util::HooksManager) class.  However, two other indexes are recognised,
-/// 0 and n+1.  These are used when the server itself registers callouts - the
-/// server is able to register callouts that get called before any user-library
-/// callouts, and callouts that get called after user-library callouts. In other
-/// words, assuming the callouts on a hook are A1, A2, B1, B2, B3, C2, C2 as
-/// before, and that the server registers Spre (to run before the
-/// user-registered callouts) and Spost (to run after them), the callouts are
-/// stored (and executed) in the order Spre, A1, A2, B1, B2, B3, C2, C2, Spost.
-/// In summary, the index values are:
+/// 0 and INT_MAX.  These are used when the server itself registers callouts -
+/// the server is able to register callouts that get called before any
+/// user-library callouts, and ones that get called after user-library callouts.
+/// In other words, assuming the callouts on a hook are A1, A2, B1, B2, B3, C2,
+/// C2 as before, and that the server registers S1 (to run before the
+/// user-registered callouts) and S2 (to run after them), the callouts are
+/// stored (and executed) in the order S1, A1, A2, B1, B2, B3, C2, C2, S2.  In
+/// summary, the recognised index values are:
 ///
 /// - < 0: invalid.
 /// - 0: used for server-registered callouts that are called before
 ///   user-registered callouts.
-/// - 1 - n: callouts from user-libraries.
+/// - 1 - n: callouts from user libraries.
 /// - INT_MAX: used for server-registered callouts called after
 ///   user-registered callouts.
-/// - > n + 1: invalid
 ///
 /// Note that the callout functions do not access the CalloutManager: instead,
 /// they use a LibraryHandle object.  This contains an internal pointer to

src/lib/hooks/hooks.h

@@ -18,7 +18,7 @@
 #include <hooks/callout_handle.h>
 #include <hooks/library_handle.h>
 
-// Version 1.0
+// Version 1 of the hooks framework.
 static const int BIND10_HOOKS_VERSION = 1;
 
 #endif  // HOOKS_H

src/lib/hooks/hooks_messages.mes

@@ -21,10 +21,10 @@ index (in the list of loaded libraries) has been called and returned a
 success state.  The address of the callout is given in the message
 
 % HOOKS_CALLOUT_ERROR error returned by callout on hook %1 registered by library with index %2 (callout address %3)
-If a callout returns an error status when called, this warning message
-is issues.  It identifies the hook to which the callout is attached,
-and the index of the library (in the list of loaded libraries) that
-registered it.  The address of the callout is also given.
+If a callout returns an error status when called, this error message
+is issued.  It identifies the hook to which the callout is attached, the
+index of the library (in the list of loaded libraries) that registered
+it and the address of the callout.  The error is otherwise ignored.
 
 % HOOKS_CALLOUT_REMOVED callout removed from hook %1 for library %2
 This is a debug message issued during library unloading.  It notes that
@@ -37,6 +37,12 @@ Although this is an error, this should not affect the running system
 other than as a loss of resources.  If this error persists, you should
 restart BIND 10.
 
+% HOOKS_CALLOUT_EXCEPTION exception thrown by callout on hook %1 registered by library with index %2 (callout address %3)
+If a callout throws an exception when called, this error message is
+issued.  It identifies the hook to which the callout is attached, the
+index of the library (in the list of loaded libraries) that registered
+it and the address of the callout.  The error is otherwise ignored.
+
 % HOOKS_DEREGISTER_ALL_CALLOUTS hook library at index %1 deregistered all callouts on hook %2
 A debug message issued when all callouts on the specified hook registered
 by the library with the given index were removed.

src/lib/hooks/library_manager.h

@@ -32,27 +32,26 @@ class LibraryManager;
 ///
 /// On loading, it opens the library using dlopen and checks the version (set
 /// with the "version" method.  If all is OK, it iterates through the list of
-/// known hooks and locates their symbols, registering each callout as it
-/// does so.  Finally it locates the "load" and "unload" functions (if present),
-/// calling the "load" callout if present.
+/// known hooks and locates their symbols, registering each callout as it does
+/// so.  Finally it locates the "load" function (if present) and calls it.
 ///
-/// On unload, it calls the "unload" method if one was located, clears the
-/// callouts from all hooks and closes the library.
+/// On unload, it calls the "unload" method if present, clears the callouts
+/// all hooks and closes the library.
 ///
 /// @note Caution needs to be exercised when using the unload method. During
-///       use, data will pass between the server and the library.  In this
-///       process, the library may allocate memory and pass it back to the
+///       normal use, data will pass between the server and the library.  In
+///       this process, the library may allocate memory and pass it back to the
 ///       server.  This could happen by the server setting arguments or context
 ///       in the CalloutHandle object, or by the library modifying the content
 ///       of pointed-to data. If the library is unloaded, this memory may lie
 ///       in the virtual address space deleted in that process. (The word "may"
-///       is used, as this could be operating-system specific.) If this happens,
-///       any reference to the memory will cause a segmentation fault.  This can
-///       occur in a quite obscure place, for example in the middle of a
-///       destructor of an STL class when it is deleting memory allocated
-///       when the data structure was extended.
+///       is used, as this could be operating-system specific.) Should this
+///       happens, any reference to the memory will cause a segmentation fault.
+///       This can occur in a quite obscure place, for example in the middle of
+///       a destructor of an STL class when it is deleting memory allocated
+///       when the data structure was extended by a function in the library.
 ///
-/// @par  The only safe way to run the "unload" function is to ensure that all
+/// @note The only safe way to run the "unload" function is to ensure that all
 ///       possible references to it are removed first.  This means that all
 ///       CalloutHandles must be destroyed, as must any data items that were
 ///       passed to the callouts.  In practice, it could mean that a server
@@ -85,22 +84,20 @@ public:
     /// @brief Destructor
     ///
     /// If the library is open, closes it.  This is principally a safety
-    /// feature to ensure closure in the case of an exception destroying
-    /// this object.
-    ///
-    /// However, see the caveat in the class header about when it is safe
-    /// to unload libraries.
+    /// feature to ensure closure in the case of an exception destroying this
+    /// object.  However, see the caveat in the class header about when it is
+    /// safe to unload libraries.
     ~LibraryManager() {
         static_cast<void>(unloadLibrary());
     }
 
     /// @brief Loads a library
     ///
-    /// Open the library and check the version.  If all is OK, load all
-    /// standard symbols then call "load" if present.
+    /// Open the library and check the version.  If all is OK, load all standard
+    /// symbols then call "load" if present.
     ///
-    /// @return true if the library loaded successfully, false otherwise.
-    ///         In the latter case, the library will be unloaded if possible.
+    /// @return true if the library loaded successfully, false otherwise. In the
+    ///         latter case, the library will be unloaded if possible.
     bool loadLibrary();
 
     /// @brief Unloads a library
@@ -108,8 +105,8 @@ public:
     /// Calls the libraries "unload" function if present, the closes the
     /// library.
     ///
-    /// However, see the caveat in the class header about when it is safe
-    /// to unload libraries.
+    /// However, see the caveat in the class header about when it is safe to
+    /// unload libraries.
     ///
     /// @return true if the library unloaded successfully, false if an error
     ///         occurred in the process (most likely the unload() function
@@ -140,9 +137,9 @@ protected:
     /// Closes the library associated with this LibraryManager.  A message is
     /// logged on an error.
     ///
-    /// @return true if the library closed successfully, false otherwise.
-    ///         "true" is also returned if the library were already closed
-    ///         when this method was called.
+    /// @return true if the library closed successfully, false otherwise. "true"
+    ///         is also returned if the library were already closed when this
+    ///         method was called.
     bool closeLibrary();
 
     /// @brief Check library version

src/lib/hooks/server_hooks.cc

@@ -65,7 +65,7 @@ ServerHooks::registerHook(const string& name) {
 
 void
 ServerHooks::reset() {
-    // Log a wanring - although this is done during testing, it should never be
+    // Log a warning - although this is done during testing, it should never be
     // seen in a production system.
     LOG_WARN(hooks_logger, HOOKS_RESET_HOOK_LIST);
 
@@ -130,33 +130,6 @@ ServerHooks::getHookNames() const {
     return (names);
 }
 
-// Hook registration function methods
-
-// Access the hook registration function vector itself
-
-std::vector<HookRegistrationFunction::RegistrationFunctionPtr>&
-HookRegistrationFunction::getFunctionVector() {
-    static std::vector<RegistrationFunctionPtr> reg_functions;
-    return (reg_functions);
-}
-
-// Constructor - add a registration function to the function vector
-
-HookRegistrationFunction::HookRegistrationFunction(
-    HookRegistrationFunction::RegistrationFunctionPtr reg_func) {
-    getFunctionVector().push_back(reg_func);
-}
-
-// Execute all registered registration functions
-
-void
-HookRegistrationFunction::execute(ServerHooks& hooks) {
-    std::vector<RegistrationFunctionPtr>& reg_functions = getFunctionVector();
-    for (int i = 0; i < reg_functions.size(); ++i) {
-        (*reg_functions[i])(hooks);
-    }
-}
-
 // Return global ServerHooks object
 
 ServerHooks&

src/lib/hooks/server_hooks.h

@@ -61,7 +61,7 @@ public:
 /// difference in a frequently-executed piece of code.)
 ///
 /// ServerHooks is a singleton object and is only accessible by the static
-/// method getserverHooks().
+/// method getServerHooks().
 
 class ServerHooks : public boost::noncopyable {
 public:
@@ -134,7 +134,7 @@ public:
     /// @return Vector of strings holding hook names.
     std::vector<std::string> getHookNames() const;
 
-    /// @brief Return ServerHookms object
+    /// @brief Return ServerHooks object
     ///
     /// Returns the global ServerHooks object.
     ///
@@ -167,84 +167,6 @@ private:
     InverseHookCollection inverse_hooks_;   ///< Hook index/name collection
 };
 
-
-/// @brief Hooks Registration
-///
-/// All hooks must be registered before libraries are loaded and callouts
-/// assigned to them.  One way of doing this is to have a global list of hooks:
-/// the addition of any hook anywhere would require updating the list. This
-/// is possible and, if desired, the author of a server can do it.
-///
-/// An alternative is the option provided here, where each component of BIND 10
-/// registers the hooks they are using.  To do this, the component should
-/// create a hook registration function of the form:
-///
-/// @code
-/// static int hook1_num = -1;  // Initialize number for hook 1
-/// static int hook2_num = -1;  // Initialize number for hook 2
-///
-/// void myModuleRegisterHooks(ServerHooks& hooks) {
-///     hook1_num = hooks.registerHook("hook1");
-///     hook2_num = hooks.registerHook("hook2");
-/// }
-/// @endcode
-///
-/// ... which registers the hooks and stores the associated hook index. To
-/// avoid the need to add an explicit call to each of the hook registration
-/// functions to the server initialization code, the component should declare
-/// an object of this class in the same file as the registration function,
-/// but outside of any function.  The declaration should include the name
-/// of the registration function, i.e.
-///
-/// @code
-/// HookRegistrationFunction f(myModuleRegisterHooks);
-/// @code
-///
-/// The constructor of this object will run prior to main() getting called and
-/// will add the registration function to a list of such functions.  The server
-/// then calls the static class method "execute()" to run all the declared
-/// registration functions.
-
-class HookRegistrationFunction {
-public:
-    /// @brief Pointer to a hook registration function
-    typedef void (*RegistrationFunctionPtr)(ServerHooks&);
-
-    /// @brief Constructor
-    ///
-    /// For variables declared outside functions or methods, the constructors
-    /// are run after the program is loaded and before main() is called. This
-    /// constructor adds the passed pointer to a vector of such pointers.
-    HookRegistrationFunction(RegistrationFunctionPtr reg_func);
-
-    /// @brief Access registration function vector
-    ///
-    /// One of the problems with functions run prior to starting main() is the
-    /// "static initialization fiasco".  This occurs because the order in which
-    /// objects outside functions are constructed is not defined.  So if this
-    /// constructor were to depend on a vector declared externally, we would
-    /// not be able to guarantee that the vector had been initialised properly
-    /// before we used it.
-    ///
-    /// To get round this situation, the vector is declared statically within
-    /// a static function.  The first time the function is called, the vector
-    /// is initialized before it is used.
-    ///
-    /// This function returns a reference to the vector used to hold the
-    /// pointers.
-    ///
-    /// @return Reference to the (static) list of registration functions
-    static std::vector<RegistrationFunctionPtr>& getFunctionVector();
-
-    /// @brief Execute registration functions
-    ///
-    /// Called by the server initialization code, this function executes all
-    /// registered hook registration functions.
-    ///
-    /// @param hooks ServerHooks object to which hook information will be added.
-    static void execute(ServerHooks& hooks);
-};
-
 } // namespace util
 } // namespace isc
 

src/lib/hooks/tests/basic_callout_library.cc

@@ -27,7 +27,7 @@
 ///   "lm_one", "lm_two", "lm_three".  All do some trivial calculations
 ///   on the arguments supplied to it and the context variables, returning
 ///   intermediate results through the "result" argument. The result of
-///   the calculation is:
+///   executing all four callouts in order is:
 ///
 ///   @f[ (10 + data_1) * data_2 - data_3 @f]
 ///

src/lib/hooks/tests/handles_unittest.cc

@@ -934,7 +934,7 @@ TEST_F(HandlesTest, HookName) {
     getCalloutManager()->registerCallout("alpha", callout_hook_name);
     getCalloutManager()->registerCallout("beta", callout_hook_name);
 
-    // Call alpha abd beta callouts and check the hook to which they belong.
+    // Call alpha and beta callouts and check the hook to which they belong.
     CalloutHandle callout_handle(getCalloutManager());
 
     EXPECT_EQ(std::string(""), HandlesTest::common_string_);
@@ -949,8 +949,8 @@ TEST_F(HandlesTest, HookName) {
     // only callout in the list.
     getCalloutManager()->setLibraryIndex(1);
     getCalloutManager()->registerCallout("gamma", callout_hook_dummy);
-    getCalloutManager()->registerCallout("gamma", callout_hook_dummy);
     getCalloutManager()->registerCallout("gamma", callout_hook_name);
+    getCalloutManager()->registerCallout("gamma", callout_hook_dummy);
 
     EXPECT_EQ(std::string("beta"), HandlesTest::common_string_);
     getCalloutManager()->callCallouts(gamma_index_, callout_handle);

src/lib/hooks/tests/server_hooks_unittest.cc

@@ -175,118 +175,4 @@ TEST(ServerHooksTest, HookCount) {
     EXPECT_EQ(6, hooks.getCount());
 }
 
-// HookRegistrationFunction tests
-
-// Declare some hook registration functions.
-
-int alpha = 0;
-int beta = 0;
-int gamma = 0;
-int delta = 0;
-
-void registerAlphaBeta(ServerHooks& hooks) {
-    alpha = hooks.registerHook("alpha");
-    beta = hooks.registerHook("beta");
-}
-
-void registerGammaDelta(ServerHooks& hooks) {
-    gamma = hooks.registerHook("gamma");
-    delta = hooks.registerHook("delta");
-}
-
-// Add them to the registration vector.  This addition should happen before
-// any tests are run, so we should start off with two functions in the
-// registration vector.
-
-HookRegistrationFunction f1(registerAlphaBeta);
-HookRegistrationFunction f2(registerGammaDelta);
-
-// This is not registered statically: it is used in the latter part of the
-// test.
-
-int epsilon = 0;
-void registerEpsilon(ServerHooks& hooks) {
-    epsilon = hooks.registerHook("epsilon");
-}
-
-// Test that the registration functions were defined and can be executed.
-
-TEST(HookRegistrationFunction, Registration) {
-
-    // The first part of the tests checks the static registration.  As there
-    // is only one list of registration functions, we have to do this first
-    // as the static registration is done outside our control, before the
-    // tests are loaded.
-
-    // Ensure that the hook numbers are initialized.
-    EXPECT_EQ(0, alpha);
-    EXPECT_EQ(0, beta);
-    EXPECT_EQ(0, gamma);
-    EXPECT_EQ(0, delta);
-
-    // Should have two hook registration functions registered.
-    EXPECT_EQ(2, HookRegistrationFunction::getFunctionVector().size());
-
-    // Execute the functions and check that four new hooks were defined (two
-    // from each function).
-    ServerHooks& hooks = ServerHooks::getServerHooks();
-    hooks.reset();
-
-    EXPECT_EQ(2, hooks.getCount());
-    HookRegistrationFunction::execute(hooks);
-    EXPECT_EQ(6, hooks.getCount());
-
-    // Check the hook names are as expected.
-    vector<string> names = hooks.getHookNames();
-    ASSERT_EQ(6, names.size());
-    sort(names.begin(), names.end());
-    EXPECT_EQ(string("alpha"), names[0]);
-    EXPECT_EQ(string("beta"), names[1]);
-    EXPECT_EQ(string("context_create"), names[2]);
-    EXPECT_EQ(string("context_destroy"), names[3]);
-    EXPECT_EQ(string("delta"), names[4]);
-    EXPECT_EQ(string("gamma"), names[5]);
-
-    // Check that numbers in the range 2-5 inclusive were assigned as the
-    // hook indexes (0 and 1 being reserved for context_create and
-    // context_destroy).
-    vector<int> indexes;
-    indexes.push_back(alpha);
-    indexes.push_back(beta);
-    indexes.push_back(gamma);
-    indexes.push_back(delta);
-    sort(indexes.begin(), indexes.end());
-    EXPECT_EQ(2, indexes[0]);
-    EXPECT_EQ(3, indexes[1]);
-    EXPECT_EQ(4, indexes[2]);
-    EXPECT_EQ(5, indexes[3]);
-
-    // One last check.  We'll test that the constructor of does indeed
-    // add a function to the function vector and that the static initialization
-    // was not somehow by chance.
-    HookRegistrationFunction::getFunctionVector().clear();
-    EXPECT_TRUE(HookRegistrationFunction::getFunctionVector().empty());
-    epsilon = 0;
-
-    // Register a single registration function.
-    HookRegistrationFunction f3(registerEpsilon);
-    EXPECT_EQ(1, HookRegistrationFunction::getFunctionVector().size());
-
-    // Execute it...
-    hooks.reset();
-    EXPECT_EQ(0, epsilon);
-    EXPECT_EQ(2, hooks.getCount());
-    HookRegistrationFunction::execute(hooks);
-
-    // There should be three hooks, with the new one assigned an index of 2.
-    names = hooks.getHookNames();
-    ASSERT_EQ(3, names.size());
-    sort(names.begin(), names.end());
-    EXPECT_EQ(string("context_create"), names[0]);
-    EXPECT_EQ(string("context_destroy"), names[1]);
-    EXPECT_EQ(string("epsilon"), names[2]);
-
-    EXPECT_EQ(2, epsilon);
-}
-
 } // Anonymous namespace