From: Stephen Morris Date: Wed, 3 Jul 2013 19:13:39 +0000 (+0100) Subject: [2980] Various changes as a result of review X-Git-Tag: bind10-1.2.0beta1-release~345^2~6 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=b61e894db16aa458c9d3d405210c357e1fdde657;p=thirdparty%2Fkea.git [2980] Various changes as a result of review --- diff --git a/src/lib/hooks/Makefile.am b/src/lib/hooks/Makefile.am index 08863be2d0..8b38442ddd 100644 --- a/src/lib/hooks/Makefile.am +++ b/src/lib/hooks/Makefile.am @@ -29,6 +29,7 @@ lib_LTLIBRARIES = libb10-hooks.la libb10_hooks_la_SOURCES = libb10_hooks_la_SOURCES += callout_handle.cc callout_handle.h libb10_hooks_la_SOURCES += callout_manager.cc callout_manager.h +libb10_hooks_la_SOURCES += framework_functions.h libb10_hooks_la_SOURCES += hooks.h libb10_hooks_la_SOURCES += hooks_log.cc hooks_log.h libb10_hooks_la_SOURCES += hooks_manager.cc hooks_manager.h diff --git a/src/lib/hooks/hooks.h b/src/lib/hooks/hooks.h index dcb27cb034..2d472e62c6 100644 --- a/src/lib/hooks/hooks.h +++ b/src/lib/hooks/hooks.h @@ -18,7 +18,22 @@ #include #include +namespace { + // Version 1 of the hooks framework. static const int BIND10_HOOKS_VERSION = 1; +// Names of the framework functions. +const char* LOAD_FUNCTION_NAME = "load"; +const char* UNLOAD_FUNCTION_NAME = "unload"; +const char* VERSION_FUNCTION_NAME = "version"; + +// Typedefs for pointers to the framework functions. +typedef int (*version_function_ptr)(); ///< version() signature +typedef int (*load_function_ptr)(isc::hooks::LibraryHandle&); + ///< load() signature +typedef int (*unload_function_ptr)(); ///< unload() signature + +} // Anonymous namespace + #endif // HOOKS_H diff --git a/src/lib/hooks/hooks_messages.mes b/src/lib/hooks/hooks_messages.mes index 90402cb9da..690a692be7 100644 --- a/src/lib/hooks/hooks_messages.mes +++ b/src/lib/hooks/hooks_messages.mes @@ -83,6 +83,12 @@ was called. The function returned a non-zero status (also given in the message) which was interpreted as an error. The library has been unloaded and no callouts from it will be installed. +% HOOKS_LOAD_EXCEPTION 'load' function in hook library %1 threw an exception +A "load" function was found in the library named in the message and +was called. The function threw an exception (an error indication) +during execution, which is an error condition. The library has been +unloaded and no callouts from it will be installed. + % HOOKS_LOAD_LIBRARY loading hooks library %1 This is a debug message called when the specified library is being loaded. @@ -117,10 +123,10 @@ This is a debug message indicating that a hook of the specified name was registered by a BIND 10 server. The server doing the logging is indicated by the full name of the logger used to log this message. -% HOOKS_REGISTER_STD_CALLOUT hooks library %1 registered standard callout for hook %2 +% HOOKS_REGISTER_STD_CALLOUT hooks library %1 registered standard callout for hook %2 at address %3 This is a debug message, output when the library loading function has located a standard callout (a callout with the same name as a hook point) -and registered it. +and registered it. The address of the callout is indicated. % HOOKS_RESET_HOOK_LIST the list of hooks has been reset This is a message indicating that the list of hooks has been reset. @@ -138,6 +144,17 @@ It was called, but returned an error (non-zero) status, resulting in the issuing of this message. The unload process continued after this message and the library has been unloaded. +% HOOKS_UNLOAD_EXCEPTION 'unload' function in hook library %1 threw an exception +During the unloading of a library, an "unload" function was found. It was +called, but in the process generated an exception (an error indication). +The unload process continued after this message and the library has +been unloaded. + % HOOKS_UNLOAD_LIBRARY unloading library %1 This is a debug message called when the specified library is being unloaded. + +% HOOKS_VERSION_EXCEPTION 'version' function in hook library %1 threw an exception +This error message is issued if the version() function in the specified +hooks library was called and generated an exception. The library is +considered unusable and will not be loaded. diff --git a/src/lib/hooks/library_manager.cc b/src/lib/hooks/library_manager.cc index 59fafa910c..2b73bf468c 100644 --- a/src/lib/hooks/library_manager.cc +++ b/src/lib/hooks/library_manager.cc @@ -24,20 +24,83 @@ #include -namespace { - -// String constants - -const char* LOAD_FUNCTION_NAME = "load"; -const char* UNLOAD_FUNCTION_NAME = "unload"; -const char* VERSION_FUNCTION_NAME = "version"; -} - using namespace std; namespace isc { namespace hooks { +/// @brief Local class for conversion of void pointers to function pointers +/// +/// Converting between void* and function pointers in C++ is fraught with +/// difficulty and pitfalls (e.g. see +/// https://groups.google.com/forum/?hl=en&fromgroups#!topic/comp.lang.c++/37o0l8rtEE0 +/// +/// The method given in that article - convert using a union is used here. A +/// union is declared (and zeroed) and the appropriate member extracted when +/// needed. + +class PointerConverter { +public: + /// @brief Constructor + /// + /// Zeroes the union and stores the void* pointer (returned by dlsym) there. + /// + /// @param dlsym_ptr void* pointer returned by call to dlsym() + PointerConverter(void* dlsym_ptr) { + memset(&pointers_, 0, sizeof(pointers_)); + pointers_.dlsym_ptr = dlsym_ptr; + } + + /// @name Pointer accessor functions + /// + /// It is up to the caller to ensure that the correct member is called so + /// that the correct trype of pointer is returned. + /// + ///@{ + + /// @brief Return pointer to callout function + /// + /// @return Pointer to the callout function + CalloutPtr calloutPtr() const { + return (pointers_.callout_ptr); + } + + /// @brief Return pointer to load function + /// + /// @return Pointer to the load function + load_function_ptr loadPtr() const { + return (pointers_.load_ptr); + } + + /// @brief Return pointer to unload function + /// + /// @return Pointer to the unload function + unload_function_ptr unloadPtr() const { + return (pointers_.unload_ptr); + } + + /// @brief Return pointer to version function + /// + /// @return Pointer to the version function + version_function_ptr versionPtr() const { + return (pointers_.version_ptr); + } + + ///@} + +private: + + /// @brief Union linking void* and pointers to functions. + union { + void* dlsym_ptr; // void* returned by dlsym + CalloutPtr callout_ptr; // Pointer to callout + load_function_ptr load_ptr; // Pointer to load function + unload_function_ptr unload_ptr; // Pointer to unload function + version_function_ptr version_ptr; // Pointer to version function + } pointers_; +}; + + // Open the library bool @@ -78,28 +141,18 @@ LibraryManager::closeLibrary() { bool LibraryManager::checkVersion() const { - // Look up the "version" string in the library. This is returned as - // "void*": without any other information, we must assume that it is of - // the correct type of version_function_ptr. - // - // Note that converting between void* and function pointers in C++ is - // fraught with difficulty and pitfalls (e.g. see - // https://groups.google.com/forum/?hl=en&fromgroups#!topic/ - // comp.lang.c++/37o0l8rtEE0) - // The method given in that article - convert using a union is used here. - union { - version_function_ptr ver_ptr; - void* dlsym_ptr; - } pointers; - - // Zero the union, whatever the size of the pointers. - pointers.ver_ptr = NULL; - pointers.dlsym_ptr = NULL; - // Get the pointer to the "version" function. - pointers.dlsym_ptr = dlsym(dl_handle_, VERSION_FUNCTION_NAME); - if (pointers.ver_ptr != NULL) { - int version = (*pointers.ver_ptr)(); + PointerConverter pc(dlsym(dl_handle_, VERSION_FUNCTION_NAME)); + if (pc.versionPtr() != NULL) { + int version = BIND10_HOOKS_VERSION - 1; // This is an invalid value + try { + version = (*pc.versionPtr())(); + } catch (...) { + // Exception - + LOG_ERROR(hooks_logger, HOOKS_VERSION_EXCEPTION).arg(library_name_); + return (false); + } + if (version == BIND10_HOOKS_VERSION) { // All OK, version checks out LOG_DEBUG(hooks_logger, HOOKS_DBG_CALLS, HOOKS_LIBRARY_VERSION) @@ -123,29 +176,20 @@ void LibraryManager::registerStandardCallouts() { // Create a library handle for doing the registration. We also need to // set the current library index to indicate the current library. - manager_->setLibraryIndex(index_); - LibraryHandle library_handle(manager_.get()); + LibraryHandle library_handle(manager_.get(), index_); - // Iterate through the list of known hooksv + // Iterate through the list of known hooks vector hook_names = ServerHooks::getServerHooks().getHookNames(); for (int i = 0; i < hook_names.size(); ++i) { - // Convert void* to function pointers using the same tricks as - // described above. - union { - CalloutPtr callout_ptr; - void* dlsym_ptr; - } pointers; - pointers.callout_ptr = NULL; - pointers.dlsym_ptr = NULL; - // Look up the symbol - pointers.dlsym_ptr = dlsym(dl_handle_, hook_names[i].c_str()); - if (pointers.callout_ptr != NULL) { + void* dlsym_ptr = dlsym(dl_handle_, hook_names[i].c_str()); + PointerConverter pc(dlsym_ptr); + if (pc.calloutPtr() != NULL) { // Found a symbol, so register it. LOG_DEBUG(hooks_logger, HOOKS_DBG_CALLS, HOOKS_REGISTER_STD_CALLOUT) - .arg(library_name_).arg(hook_names[i]); - library_handle.registerCallout(hook_names[i], pointers.callout_ptr); + .arg(library_name_).arg(hook_names[i]).arg(dlsym_ptr); + library_handle.registerCallout(hook_names[i], pc.calloutPtr()); } } @@ -156,27 +200,23 @@ LibraryManager::registerStandardCallouts() { bool LibraryManager::runLoad() { - // Look up the "load" function in the library. The code here is similar - // to that in "checkVersion". - union { - load_function_ptr load_ptr; - void* dlsym_ptr; - } pointers; - - // Zero the union, whatever the size of the pointers. - pointers.load_ptr = NULL; - pointers.dlsym_ptr = NULL; - // Get the pointer to the "load" function. - pointers.dlsym_ptr = dlsym(dl_handle_, LOAD_FUNCTION_NAME); - if (pointers.load_ptr != NULL) { + PointerConverter pc(dlsym(dl_handle_, LOAD_FUNCTION_NAME)); + if (pc.loadPtr() != NULL) { // Call the load() function with the library handle. We need to set // the CalloutManager's index appropriately. We'll invalidate it // afterwards. - manager_->setLibraryIndex(index_); - int status = (*pointers.load_ptr)(manager_->getLibraryHandle()); - manager_->setLibraryIndex(index_); + + int status = -1; + try { + manager_->setLibraryIndex(index_); + status = (*pc.loadPtr())(manager_->getLibraryHandle()); + } catch (...) { + LOG_ERROR(hooks_logger, HOOKS_LOAD_EXCEPTION).arg(library_name_); + return (false); + } + if (status != 0) { LOG_ERROR(hooks_logger, HOOKS_LOAD_ERROR).arg(library_name_) .arg(status); @@ -185,6 +225,7 @@ LibraryManager::runLoad() { LOG_DEBUG(hooks_logger, HOOKS_DBG_TRACE, HOOKS_LOAD) .arg(library_name_); } + } else { LOG_DEBUG(hooks_logger, HOOKS_DBG_TRACE, HOOKS_NO_LOAD) .arg(library_name_); @@ -199,25 +240,23 @@ LibraryManager::runLoad() { bool LibraryManager::runUnload() { - // Look up the "load" function in the library. The code here is similar - // to that in "checkVersion". - union { - unload_function_ptr unload_ptr; - void* dlsym_ptr; - } pointers; - - // Zero the union, whatever the relative size of the pointers. - pointers.unload_ptr = NULL; - pointers.dlsym_ptr = NULL; - // Get the pointer to the "load" function. - pointers.dlsym_ptr = dlsym(dl_handle_, UNLOAD_FUNCTION_NAME); - if (pointers.unload_ptr != NULL) { + PointerConverter pc(dlsym(dl_handle_, UNLOAD_FUNCTION_NAME)); + if (pc.unloadPtr() != NULL) { // Call the load() function with the library handle. We need to set // the CalloutManager's index appropriately. We'll invalidate it // afterwards. - int status = (*pointers.unload_ptr)(); + int status = -1; + try { + status = (*pc.unloadPtr())(); + } catch (...) { + // Exception generated. Note a warning as the unload will occur + // anyway. + LOG_WARN(hooks_logger, HOOKS_UNLOAD_EXCEPTION).arg(library_name_); + return (false); + } + if (status != 0) { LOG_ERROR(hooks_logger, HOOKS_UNLOAD_ERROR).arg(library_name_) .arg(status); @@ -245,20 +284,22 @@ LibraryManager::loadLibrary() { // have issued an error message so there is no need to issue another one // here. + // Open the library (which is a check that it exists and is accessible). if (openLibrary()) { // Library opened OK, see if a version function is present and if so, - // check it. + // check what value it returns. if (checkVersion()) { // Version OK, so now register the standard callouts and call the - // librarie's load() function if present. + // library's load() function if present. registerStandardCallouts(); if (runLoad()) { // Success - the library has been successfully loaded. LOG_INFO(hooks_logger, HOOKS_LIBRARY_LOADED).arg(library_name_); return (true); + } else { // The load function failed, so back out. We can't just close @@ -268,13 +309,12 @@ LibraryManager::loadLibrary() { // that have been installed. static_cast(unloadLibrary()); } - } else { - - // Version check failed so close the library and free up resources. - // Ignore the status return here - we already have an error - // consition. - static_cast(closeLibrary()); } + + // Either version check or call to load() failed, so close the library + // and free up resources. Ignore the status return here - we already + // know there's an error and will have output a message. + static_cast(closeLibrary()); } return (false); @@ -306,7 +346,7 @@ LibraryManager::unloadLibrary() { } // ... and close the library. - result = result && closeLibrary(); + result = closeLibrary() && result; if (result) { // Issue the informational message only if the library was unloaded diff --git a/src/lib/hooks/library_manager.h b/src/lib/hooks/library_manager.h index e4899a539a..56c770be81 100644 --- a/src/lib/hooks/library_manager.h +++ b/src/lib/hooks/library_manager.h @@ -35,8 +35,8 @@ class LibraryManager; /// 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 present, clears the callouts -/// all hooks and closes the library. +/// On unload, it calls the "unload" method if present, clears the callouts on +/// all hooks, and closes the library. /// /// @note Caution needs to be exercised when using the unload method. During /// normal use, data will pass between the server and the library. In @@ -46,7 +46,7 @@ class LibraryManager; /// 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.) Should this -/// happens, any reference to the memory will cause a segmentation fault. +/// happen, 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. @@ -60,12 +60,6 @@ class LibraryManager; /// reloading the libraries. class LibraryManager { -private: - /// Useful typedefs for the framework functions - typedef int (*version_function_ptr)(); ///< version() signature - typedef int (*load_function_ptr)(LibraryHandle&); ///< load() signature - typedef int (*unload_function_ptr)(); ///< unload() signature - public: /// @brief Constructor /// @@ -146,7 +140,9 @@ protected: /// /// With the library open, accesses the "version()" function and, if /// present, checks the returned value against the hooks version symbol - /// for the currently running BIND 10. + /// for the currently running BIND 10. The "version()" function is + /// mandatory and must be present (and return the correct value) for the + /// library to load. /// /// If there is no version() function, or if there is a mismatch in /// version number, a message logged. diff --git a/src/lib/hooks/tests/Makefile.am b/src/lib/hooks/tests/Makefile.am index ba23c3044b..8fe94b0273 100644 --- a/src/lib/hooks/tests/Makefile.am +++ b/src/lib/hooks/tests/Makefile.am @@ -27,18 +27,23 @@ TESTS_ENVIRONMENT = \ TESTS = if HAVE_GTEST # Build shared libraries for testing. -lib_LTLIBRARIES = libnvl.la libivl.la libbcl.la liblcl.la liblecl.la \ +lib_LTLIBRARIES = libnvl.la libivl.la libfxl.la libbcl.la liblcl.la liblecl.la \ libucl.la libfcl.la - + # No version function libnvl_la_SOURCES = no_version_library.cc libnvl_la_CXXFLAGS = $(AM_CXXFLAGS) libnvl_la_CPPFLAGS = $(AM_CPPFLAGS) $(LOG4CPLUS_INCLUDES) - + # Incorrect version function libivl_la_SOURCES = incorrect_version_library.cc libivl_la_CXXFLAGS = $(AM_CXXFLAGS) -libilv_la_CPPFLAGS = $(AM_CPPFLAGS) $(LOG4CPLUS_INCLUDES) +libivl_la_CPPFLAGS = $(AM_CPPFLAGS) $(LOG4CPLUS_INCLUDES) + +# All framework functions throw an exception +libfxl_la_SOURCES = framework_exception_library.cc +libfxl_la_CXXFLAGS = $(AM_CXXFLAGS) +libfxl_la_CPPFLAGS = $(AM_CPPFLAGS) $(LOG4CPLUS_INCLUDES) # The basic callout library - contains standard callouts libbcl_la_SOURCES = basic_callout_library.cc diff --git a/src/lib/hooks/tests/basic_callout_library.cc b/src/lib/hooks/tests/basic_callout_library.cc index ff39a9c8db..12d409336c 100644 --- a/src/lib/hooks/tests/basic_callout_library.cc +++ b/src/lib/hooks/tests/basic_callout_library.cc @@ -24,7 +24,7 @@ /// - A context_create callout is supplied. /// /// - Three "standard" callouts are supplied corresponding to the hooks -/// "lm_one", "lm_two", "lm_three". All do some trivial calculations +/// "hook_point_one", "hook_point_two", "hook_point_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 /// executing all four callouts in order is: @@ -32,8 +32,8 @@ /// @f[ (10 + data_1) * data_2 - data_3 @f] /// /// ...where data_1, data_2 and data_3 are the values passed in arguments of -/// the same name to the three callouts (data_1 passed to lm_one, data_2 to -/// lm_two etc.) and the result is returned in the argument "result". +/// the same name to the three callouts (data_1 passed to hook_point_one, data_2 to +/// hook_point_two etc.) and the result is returned in the argument "result". #include #include @@ -58,7 +58,7 @@ context_create(CalloutHandle& handle) { // between callouts in the same library.) int -lm_one(CalloutHandle& handle) { +hook_point_one(CalloutHandle& handle) { int data; handle.getArgument("data_1", data); @@ -75,7 +75,7 @@ lm_one(CalloutHandle& handle) { // argument. int -lm_two(CalloutHandle& handle) { +hook_point_two(CalloutHandle& handle) { int data; handle.getArgument("data_2", data); @@ -91,7 +91,7 @@ lm_two(CalloutHandle& handle) { // Final callout subtracts the result in "data_3". int -lm_three(CalloutHandle& handle) { +hook_point_three(CalloutHandle& handle) { int data; handle.getArgument("data_3", data); diff --git a/src/lib/hooks/tests/common_test_class.h b/src/lib/hooks/tests/common_test_class.h index bcb8e56ba8..d945fb5add 100644 --- a/src/lib/hooks/tests/common_test_class.h +++ b/src/lib/hooks/tests/common_test_class.h @@ -44,16 +44,16 @@ public: isc::hooks::ServerHooks& hooks = isc::hooks::ServerHooks::getServerHooks(); hooks.reset(); - lm_one_index_ = hooks.registerHook("lm_one"); - lm_two_index_ = hooks.registerHook("lm_two"); - lm_three_index_ = hooks.registerHook("lm_three"); + hook_point_one_index_ = hooks.registerHook("hook_point_one"); + hook_point_two_index_ = hooks.registerHook("hook_point_two"); + hook_point_three_index_ = hooks.registerHook("hook_point_three"); } /// @brief Call callouts test /// /// All of the loaded libraries for which callouts are called register four /// callouts: a context_create callout and three callouts that are attached - /// to hooks lm_one, lm_two and lm_three. These four callouts, executed + /// to hooks hook_point_one, hook_point_two and hook_point_three. These four callouts, executed /// in sequence, perform a series of calculations. Data is passed between /// callouts in the argument list, in a variable named "result". /// @@ -63,15 +63,15 @@ public: /// the purpose being to avoid exceptions when running this test with no /// libraries loaded. /// - /// Callout lm_one is passed a value d1 and performs a simple arithmetic + /// Callout hook_point_one is passed a value d1 and performs a simple arithmetic /// operation on it and r0 yielding a result r1. Hence we can say that /// @f[ r1 = lm1(r0, d1) @f] /// - /// Callout lm_two is passed a value d2 and peforms another simple + /// Callout hook_point_two is passed a value d2 and peforms another simple /// arithmetic operation on it and d2, yielding r2, i.e. /// @f[ r2 = lm2(d1, d2) @f] /// - /// lm_three does a similar operation giving @f[ r3 = lm3(r2, d3) @f]. + /// hook_point_three does a similar operation giving @f[ r3 = lm3(r2, d3) @f]. /// /// The details of the operations lm1, lm2 and lm3 depend on the library. /// However the sequence of calls needed to do this set of calculations @@ -112,27 +112,27 @@ public: // Perform the first calculation. handle.setArgument("data_1", d1); - manager->callCallouts(lm_one_index_, handle); + manager->callCallouts(hook_point_one_index_, handle); handle.getArgument(RESULT, result); - EXPECT_EQ(r1, result) << "lm_one" << COMMON_TEXT; + EXPECT_EQ(r1, result) << "hook_point_one" << COMMON_TEXT; // ... the second ... handle.setArgument("data_2", d2); - manager->callCallouts(lm_two_index_, handle); + manager->callCallouts(hook_point_two_index_, handle); handle.getArgument(RESULT, result); - EXPECT_EQ(r2, result) << "lm_two" << COMMON_TEXT; + EXPECT_EQ(r2, result) << "hook_point_two" << COMMON_TEXT; // ... and the third. handle.setArgument("data_3", d3); - manager->callCallouts(lm_three_index_, handle); + manager->callCallouts(hook_point_three_index_, handle); handle.getArgument(RESULT, result); - EXPECT_EQ(r3, result) << "lm_three" << COMMON_TEXT; + EXPECT_EQ(r3, result) << "hook_point_three" << COMMON_TEXT; } /// Hook indexes. These are are made public for ease of reference. - int lm_one_index_; - int lm_two_index_; - int lm_three_index_; + int hook_point_one_index_; + int hook_point_two_index_; + int hook_point_three_index_; }; #endif // COMMON_HOOKS_TEST_CLASS_H diff --git a/src/lib/hooks/tests/full_callout_library.cc b/src/lib/hooks/tests/full_callout_library.cc index df7a76f0a7..c51f2d4a2e 100644 --- a/src/lib/hooks/tests/full_callout_library.cc +++ b/src/lib/hooks/tests/full_callout_library.cc @@ -34,8 +34,8 @@ /// @f[ ((7 * data_1) - data_2) * data_3 @f] /// /// ...where data_1, data_2 and data_3 are the values passed in arguments of -/// the same name to the three callouts (data_1 passed to lm_one, data_2 to -/// lm_two etc.) and the result is returned in the argument "result". +/// the same name to the three callouts (data_1 passed to hook_point_one, data_2 to +/// hook_point_two etc.) and the result is returned in the argument "result". #include #include @@ -61,7 +61,7 @@ context_create(CalloutHandle& handle) { // between callouts in the same library.) int -lm_one(CalloutHandle& handle) { +hook_point_one(CalloutHandle& handle) { int data; handle.getArgument("data_1", data); @@ -78,7 +78,7 @@ lm_one(CalloutHandle& handle) { // running total. static int -lm_nonstandard_two(CalloutHandle& handle) { +hook_nonstandard_two(CalloutHandle& handle) { int data; handle.getArgument("data_2", data); @@ -94,7 +94,7 @@ lm_nonstandard_two(CalloutHandle& handle) { // Final callout multplies the current running total by data_3. static int -lm_nonstandard_three(CalloutHandle& handle) { +hook_nonstandard_three(CalloutHandle& handle) { int data; handle.getArgument("data_3", data); @@ -117,8 +117,8 @@ version() { int load(LibraryHandle& handle) { // Register the non-standard functions - handle.registerCallout("lm_two", lm_nonstandard_two); - handle.registerCallout("lm_three", lm_nonstandard_three); + handle.registerCallout("hook_point_two", hook_nonstandard_two); + handle.registerCallout("hook_point_three", hook_nonstandard_three); return (0); } diff --git a/src/lib/hooks/tests/hooks_manager_unittest.cc b/src/lib/hooks/tests/hooks_manager_unittest.cc index c1a3600c29..91f57052fe 100644 --- a/src/lib/hooks/tests/hooks_manager_unittest.cc +++ b/src/lib/hooks/tests/hooks_manager_unittest.cc @@ -82,21 +82,21 @@ public: // Perform the first calculation. handle->setArgument("data_1", d1); - HooksManager::callCallouts(lm_one_index_, *handle); + HooksManager::callCallouts(hook_point_one_index_, *handle); handle->getArgument(RESULT, result); - EXPECT_EQ(r1, result) << "lm_one" << COMMON_TEXT; + EXPECT_EQ(r1, result) << "hook_point_one" << COMMON_TEXT; // ... the second ... handle->setArgument("data_2", d2); - HooksManager::callCallouts(lm_two_index_, *handle); + HooksManager::callCallouts(hook_point_two_index_, *handle); handle->getArgument(RESULT, result); - EXPECT_EQ(r2, result) << "lm_two" << COMMON_TEXT; + EXPECT_EQ(r2, result) << "hook_point_two" << COMMON_TEXT; // ... and the third. handle->setArgument("data_3", d3); - HooksManager::callCallouts(lm_three_index_, *handle); + HooksManager::callCallouts(hook_point_three_index_, *handle); handle->getArgument(RESULT, result); - EXPECT_EQ(r3, result) << "lm_three" << COMMON_TEXT; + EXPECT_EQ(r3, result) << "hook_point_three" << COMMON_TEXT; } }; @@ -353,7 +353,7 @@ testPostCallout(CalloutHandle& handle) { } -// The next test registers the pre and post- callouts above for hook lm_two, +// The next test registers the pre and post- callouts above for hook hook_point_two, // and checks they are called. TEST_F(HooksManagerTest, PrePostCalloutTest) { @@ -364,12 +364,12 @@ TEST_F(HooksManagerTest, PrePostCalloutTest) { EXPECT_TRUE(HooksManager::loadLibraries(library_names)); // Load the pre- and post- callouts. - HooksManager::preCalloutsLibraryHandle().registerCallout("lm_two", + HooksManager::preCalloutsLibraryHandle().registerCallout("hook_point_two", testPreCallout); - HooksManager::postCalloutsLibraryHandle().registerCallout("lm_two", + HooksManager::postCalloutsLibraryHandle().registerCallout("hook_point_two", testPostCallout); - // Execute the callouts. lm_two implements the calculation: + // Execute the callouts. hook_point_two implements the calculation: // // "result - data_2" // @@ -380,7 +380,7 @@ TEST_F(HooksManagerTest, PrePostCalloutTest) { handle->setArgument("result", static_cast(0)); handle->setArgument("data_2", static_cast(15)); - HooksManager::callCallouts(lm_two_index_, *handle); + HooksManager::callCallouts(hook_point_two_index_, *handle); int result = 0; handle->getArgument("result", result); @@ -394,7 +394,7 @@ TEST_F(HooksManagerTest, PrePostCalloutTest) { handle->setArgument("result", static_cast(0)); handle->setArgument("data_2", static_cast(15)); - HooksManager::callCallouts(lm_two_index_, *handle); + HooksManager::callCallouts(hook_point_two_index_, *handle); result = 0; handle->getArgument("result", result); @@ -406,9 +406,9 @@ TEST_F(HooksManagerTest, PrePostCalloutTest) { TEST_F(HooksManagerTest, NoLibrariesCalloutsPresent) { // No callouts should be present on any hooks. - EXPECT_FALSE(HooksManager::calloutsPresent(lm_one_index_)); - EXPECT_FALSE(HooksManager::calloutsPresent(lm_two_index_)); - EXPECT_FALSE(HooksManager::calloutsPresent(lm_three_index_)); + EXPECT_FALSE(HooksManager::calloutsPresent(hook_point_one_index_)); + EXPECT_FALSE(HooksManager::calloutsPresent(hook_point_two_index_)); + EXPECT_FALSE(HooksManager::calloutsPresent(hook_point_three_index_)); } TEST_F(HooksManagerTest, NoLibrariesCallCallouts) { diff --git a/src/lib/hooks/tests/library_manager_unittest.cc b/src/lib/hooks/tests/library_manager_unittest.cc index 0ddfbc44b1..11e2283364 100644 --- a/src/lib/hooks/tests/library_manager_unittest.cc +++ b/src/lib/hooks/tests/library_manager_unittest.cc @@ -89,7 +89,8 @@ public: /// @param r0...r3, d1..d3 Values and intermediate values expected. They /// are ordered so that the variables appear in the argument list in /// the order they are used. See HooksCommonTestClass::execute for - /// a full description. + /// a full description. (rN is used to indicate an expected result, + /// dN is data to be passed to the calculation.) void executeCallCallouts(int r0, int d1, int r1, int d2, int r2, int d3, int r3) { HooksCommonTestClass::executeCallCallouts(callout_manager_, r0, d1, @@ -186,6 +187,22 @@ TEST_F(LibraryManagerTest, WrongVersion) { EXPECT_TRUE(lib_manager.closeLibrary()); } +// Check that the code handles the case of a library where the version function +// throws an exception. + +TEST_F(LibraryManagerTest, VersionException) { + PublicLibraryManager lib_manager(std::string(FRAMEWORK_EXCEPTION_LIBRARY), + 0, callout_manager_); + // Open should succeed. + EXPECT_TRUE(lib_manager.openLibrary()); + + // Version check should fail. + EXPECT_FALSE(lib_manager.checkVersion()); + + // Tidy up. + EXPECT_TRUE(lib_manager.closeLibrary()); +} + // Tests that checkVersion() function succeeds in the case of a library with a // version function that returns the correct version number. @@ -244,12 +261,12 @@ TEST_F(LibraryManagerTest, CheckLoadCalled) { // Load the standard callouts EXPECT_NO_THROW(lib_manager.registerStandardCallouts()); - // Check that only context_create and lm_one have callouts registered. + // Check that only context_create and hook_point_one have callouts registered. EXPECT_TRUE(callout_manager_->calloutsPresent( ServerHooks::CONTEXT_CREATE)); - EXPECT_TRUE(callout_manager_->calloutsPresent(lm_one_index_)); - EXPECT_FALSE(callout_manager_->calloutsPresent(lm_two_index_)); - EXPECT_FALSE(callout_manager_->calloutsPresent(lm_three_index_)); + EXPECT_TRUE(callout_manager_->calloutsPresent(hook_point_one_index_)); + EXPECT_FALSE(callout_manager_->calloutsPresent(hook_point_two_index_)); + EXPECT_FALSE(callout_manager_->calloutsPresent(hook_point_three_index_)); EXPECT_FALSE(callout_manager_->calloutsPresent( ServerHooks::CONTEXT_DESTROY)); @@ -257,9 +274,9 @@ TEST_F(LibraryManagerTest, CheckLoadCalled) { EXPECT_TRUE(lib_manager.runLoad()); EXPECT_TRUE(callout_manager_->calloutsPresent( ServerHooks::CONTEXT_CREATE)); - EXPECT_TRUE(callout_manager_->calloutsPresent(lm_one_index_)); - EXPECT_TRUE(callout_manager_->calloutsPresent(lm_two_index_)); - EXPECT_TRUE(callout_manager_->calloutsPresent(lm_three_index_)); + EXPECT_TRUE(callout_manager_->calloutsPresent(hook_point_one_index_)); + EXPECT_TRUE(callout_manager_->calloutsPresent(hook_point_two_index_)); + EXPECT_TRUE(callout_manager_->calloutsPresent(hook_point_three_index_)); EXPECT_FALSE(callout_manager_->calloutsPresent( ServerHooks::CONTEXT_DESTROY)); @@ -273,6 +290,23 @@ TEST_F(LibraryManagerTest, CheckLoadCalled) { EXPECT_TRUE(lib_manager.closeLibrary()); } +// Check handling of a "load" function that throws an exception + +TEST_F(LibraryManagerTest, CheckLoadException) { + + // Load the only library, specifying the index of 0 as it's the only + // library. This should load all callouts. + PublicLibraryManager lib_manager(std::string(FRAMEWORK_EXCEPTION_LIBRARY), + 0, callout_manager_); + EXPECT_TRUE(lib_manager.openLibrary()); + + // Check that we catch a load error + EXPECT_FALSE(lib_manager.runLoad()); + + // Tidy up + EXPECT_TRUE(lib_manager.closeLibrary()); +} + // Check handling of a "load" function that returns an error. TEST_F(LibraryManagerTest, CheckLoadError) { @@ -324,6 +358,23 @@ TEST_F(LibraryManagerTest, CheckUnloadError) { EXPECT_TRUE(lib_manager.closeLibrary()); } +// Unload function throws an exception. + +TEST_F(LibraryManagerTest, CheckUnloadException) { + + // Load the only library, specifying the index of 0 as it's the only + // library. This should load all callouts. + PublicLibraryManager lib_manager(std::string(FRAMEWORK_EXCEPTION_LIBRARY), + 0, callout_manager_); + EXPECT_TRUE(lib_manager.openLibrary()); + + // Check that unload function returning an error returns false. + EXPECT_FALSE(lib_manager.runUnload()); + + // Tidy up + EXPECT_TRUE(lib_manager.closeLibrary()); +} + // Check that the case of the library's unload() function returning a // success is handled correcty. @@ -367,33 +418,33 @@ TEST_F(LibraryManagerTest, LibUnload) { EXPECT_TRUE(lib_manager.checkVersion()); // No callouts should be registered at the moment. - EXPECT_FALSE(callout_manager_->calloutsPresent(lm_one_index_)); - EXPECT_FALSE(callout_manager_->calloutsPresent(lm_two_index_)); - EXPECT_FALSE(callout_manager_->calloutsPresent(lm_three_index_)); + EXPECT_FALSE(callout_manager_->calloutsPresent(hook_point_one_index_)); + EXPECT_FALSE(callout_manager_->calloutsPresent(hook_point_two_index_)); + EXPECT_FALSE(callout_manager_->calloutsPresent(hook_point_three_index_)); // Load the single standard callout and check it is registered correctly. EXPECT_NO_THROW(lib_manager.registerStandardCallouts()); - EXPECT_TRUE(callout_manager_->calloutsPresent(lm_one_index_)); - EXPECT_FALSE(callout_manager_->calloutsPresent(lm_two_index_)); - EXPECT_FALSE(callout_manager_->calloutsPresent(lm_three_index_)); + EXPECT_TRUE(callout_manager_->calloutsPresent(hook_point_one_index_)); + EXPECT_FALSE(callout_manager_->calloutsPresent(hook_point_two_index_)); + EXPECT_FALSE(callout_manager_->calloutsPresent(hook_point_three_index_)); // Call the load function to load the other callouts. EXPECT_TRUE(lib_manager.runLoad()); - EXPECT_TRUE(callout_manager_->calloutsPresent(lm_one_index_)); - EXPECT_TRUE(callout_manager_->calloutsPresent(lm_two_index_)); - EXPECT_TRUE(callout_manager_->calloutsPresent(lm_three_index_)); + EXPECT_TRUE(callout_manager_->calloutsPresent(hook_point_one_index_)); + EXPECT_TRUE(callout_manager_->calloutsPresent(hook_point_two_index_)); + EXPECT_TRUE(callout_manager_->calloutsPresent(hook_point_three_index_)); // Unload the library and check that the callouts have been removed from // the CalloutManager. lib_manager.unloadLibrary(); - EXPECT_FALSE(callout_manager_->calloutsPresent(lm_one_index_)); - EXPECT_FALSE(callout_manager_->calloutsPresent(lm_two_index_)); - EXPECT_FALSE(callout_manager_->calloutsPresent(lm_three_index_)); + EXPECT_FALSE(callout_manager_->calloutsPresent(hook_point_one_index_)); + EXPECT_FALSE(callout_manager_->calloutsPresent(hook_point_two_index_)); + EXPECT_FALSE(callout_manager_->calloutsPresent(hook_point_three_index_)); } // Now come the loadLibrary() tests that make use of all the methods tested // above. These tests are really to make sure that the methods have been -// tied toget correctly. +// tied together correctly. // First test the basic error cases - no library, no version function, version // function returning an error. @@ -437,9 +488,9 @@ TEST_F(LibraryManagerTest, LoadLibrary) { EXPECT_TRUE(lib_manager.unloadLibrary()); // Check that the callouts have been removed from the callout manager. - EXPECT_FALSE(callout_manager_->calloutsPresent(lm_one_index_)); - EXPECT_FALSE(callout_manager_->calloutsPresent(lm_two_index_)); - EXPECT_FALSE(callout_manager_->calloutsPresent(lm_three_index_)); + EXPECT_FALSE(callout_manager_->calloutsPresent(hook_point_one_index_)); + EXPECT_FALSE(callout_manager_->calloutsPresent(hook_point_two_index_)); + EXPECT_FALSE(callout_manager_->calloutsPresent(hook_point_three_index_)); } // Now test for multiple libraries. We'll load the full callout library diff --git a/src/lib/hooks/tests/load_callout_library.cc b/src/lib/hooks/tests/load_callout_library.cc index b42859719f..8461b4ce2d 100644 --- a/src/lib/hooks/tests/load_callout_library.cc +++ b/src/lib/hooks/tests/load_callout_library.cc @@ -20,9 +20,9 @@ /// file are: /// /// - The "version" and "load" framework functions are supplied. One "standard" -/// callout is supplied ("lm_one") and two non-standard ones which are -/// registered during the call to "load" on the hooks "lm_two" and -/// "lm_three". +/// callout is supplied ("hook_point_one") and two non-standard ones which are +/// registered during the call to "load" on the hooks "hook_point_two" and +/// "hook_point_three". /// /// All callouts do trivial calculations, the result of all being called in /// sequence being @@ -30,8 +30,8 @@ /// @f[ ((5 * data_1) + data_2) * data_3 @f] /// /// ...where data_1, data_2 and data_3 are the values passed in arguments of -/// the same name to the three callouts (data_1 passed to lm_one, data_2 to -/// lm_two etc.) and the result is returned in the argument "result". +/// the same name to the three callouts (data_1 passed to hook_point_one, data_2 to +/// hook_point_two etc.) and the result is returned in the argument "result". #include @@ -54,7 +54,7 @@ context_create(CalloutHandle& handle) { // between callouts in the same library.) int -lm_one(CalloutHandle& handle) { +hook_point_one(CalloutHandle& handle) { int data; handle.getArgument("data_1", data); @@ -71,7 +71,7 @@ lm_one(CalloutHandle& handle) { // argument. static int -lm_nonstandard_two(CalloutHandle& handle) { +hook_nonstandard_two(CalloutHandle& handle) { int data; handle.getArgument("data_2", data); @@ -87,7 +87,7 @@ lm_nonstandard_two(CalloutHandle& handle) { // Final callout adds "data_3" to the result. static int -lm_nonstandard_three(CalloutHandle& handle) { +hook_nonstandard_three(CalloutHandle& handle) { int data; handle.getArgument("data_3", data); @@ -109,8 +109,8 @@ version() { int load(LibraryHandle& handle) { // Register the non-standard functions - handle.registerCallout("lm_two", lm_nonstandard_two); - handle.registerCallout("lm_three", lm_nonstandard_three); + handle.registerCallout("hook_point_two", hook_nonstandard_two); + handle.registerCallout("hook_point_three", hook_nonstandard_three); return (0); } diff --git a/src/lib/hooks/tests/test_libraries.h.in b/src/lib/hooks/tests/test_libraries.h.in index 68ea4e9d67..8d9b932818 100644 --- a/src/lib/hooks/tests/test_libraries.h.in +++ b/src/lib/hooks/tests/test_libraries.h.in @@ -20,7 +20,7 @@ namespace { -// Take carse of differences in DLL naming between operating systems. +// Take care of differences in DLL naming between operating systems. #ifdef OS_BSD #define DLL_SUFFIX ".dylib" @@ -45,6 +45,10 @@ static const char* BASIC_CALLOUT_LIBRARY = "@abs_builddir@/.libs/libbcl" static const char* FULL_CALLOUT_LIBRARY = "@abs_builddir@/.libs/libfcl" DLL_SUFFIX; +// Library where the all framework functions throw an exception +static const char* FRAMEWORK_EXCEPTION_LIBRARY = "@abs_builddir@/.libs/libfxl" + DLL_SUFFIX; + // Library where the version() function returns an incorrect result. static const char* INCORRECT_VERSION_LIBRARY = "@abs_builddir@/.libs/libivl" DLL_SUFFIX; @@ -69,7 +73,6 @@ static const char* NO_VERSION_LIBRARY = "@abs_builddir@/.libs/libnvl" // Library where there is an unload() function. static const char* UNLOAD_CALLOUT_LIBRARY = "@abs_builddir@/.libs/libucl" DLL_SUFFIX; - } // anonymous namespace