Skip to content

GitLab

Commit b61e894d authored by Stephen Morris's avatar Stephen Morris
Browse files

[2980] Various changes as a result of review

parent dc342c8d
Hide whitespace changes
Inline Side-by-side
src/lib/hooks/Makefile.am
View file @ b61e894d
......@@ -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
......
src/lib/hooks/hooks.h
View file @ b61e894d
......@@ -18,7 +18,22 @@
#include <hooks/callout_handle.h>
#include <hooks/library_handle.h>
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
src/lib/hooks/hooks_messages.mes
View file @ b61e894d
......@@ -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.
src/lib/hooks/library_manager.cc
View file @ b61e894d
......@@ -24,20 +24,83 @@
#include <dlfcn.h>
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<string> 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<void>(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<void>(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<void>(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
......
src/lib/hooks/library_manager.h
View file @ b61e894d
......@@ -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.
......
src/lib/hooks/tests/Makefile.am
View file @ b61e894d
......@@ -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
......
src/lib/hooks/tests/basic_callout_library.cc
View file @ b61e894d
......@@ -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 <hooks/hooks.h>
#include <fstream>
......@@ -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);
......
src/lib/hooks/tests/common_test_class.h
View file @ b61e894d
......@@ -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
src/lib/hooks/tests/full_callout_library.cc
View file @ b61e894d
......@@ -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 <hooks/hooks.h>
#include <hooks/tests/marker_file.h>
......@@ -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);
}
......