Commit 6e6cff79 authored by Stephen Morris's avatar Stephen Morris
Browse files

[2980] Updates to ServerHooks

a) This is now a non-copyable singleton class, and pointers
to a ServerHooks object have been removed from other classes.
b) Added a getHookName() method to the CalloutHandle class to allow
callouts to determine the hook to which they are attached. (This
change was made principally to aid writing test cases.
parent 7ef14b47
......@@ -117,5 +117,26 @@ CalloutHandle::getContextNames() const {
return (names);
}
// Return name of current hook (the hook to which the current callout is
// attached) or the empty string if not called within the context of a
// callout.
string
CalloutHandle::getHookName() const {
// Get the current hook index.
int index = manager_->getHookIndex();
// ... and look up the hook.
string hook = "";
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.
}
return (hook);
}
} // namespace util
} // namespace isc
......@@ -105,7 +105,6 @@ public:
/// need to be set when the CalloutHandle is constructed.
typedef std::map<int, ElementCollection> ContextCollection;
/// @brief Constructor
///
/// Creates the object and calls the callouts on the "context_create"
......@@ -292,6 +291,14 @@ public:
getContextForLibrary().clear();
}
/// @brief Get hook name
///
/// Get the name of the hook to which the current callout is attached.
/// This can be the null string if the CalloutHandle is being accessed
/// outside of the CalloutManager's "callCallouts" method.
///
/// @return Name of the current hook or the empty string if none.
std::string getHookName() const;
private:
/// @brief Check index
......
......@@ -36,7 +36,7 @@ CalloutManager::registerCallout(const std::string& name, CalloutPtr callout) {
// Get the index associated with this hook (validating the name in the
// process).
int hook_index = hooks_->getIndex(name);
int hook_index = ServerHooks::getServerHooks().getIndex(name);
// Iterate through the callout vector for the hook from start to end,
// looking for the first entry where the library index is greater than
......@@ -84,6 +84,10 @@ CalloutManager::callCallouts(int hook_index, CalloutHandle& callout_handle) {
// Clear the "skip" flag so we don't carry state from a previous call.
callout_handle.setSkip(false);
// Set the current hook index. This is used should a callout wish to
// determine to what hook it is attached.
current_hook_ = hook_index;
// Duplicate the callout vector for this hook and work through that.
// This step is needed because we allow dynamic registration and
// deregistration of callouts. If a callout attached to a hook modified
......@@ -104,8 +108,9 @@ CalloutManager::callCallouts(int hook_index, CalloutHandle& callout_handle) {
static_cast<void>((*i->second)(callout_handle));
}
// Reset the current library index to an invalid value to catch any
// programming errors.
// Reset the current hook and library indexs to an invalid value to
// catch any programming errors.
current_hook_ = -1;
current_library_ = -1;
}
}
......@@ -119,7 +124,7 @@ CalloutManager::deregisterCallout(const std::string& name, CalloutPtr callout) {
// Get the index associated with this hook (validating the name in the
// process).
int hook_index = hooks_->getIndex(name);
int hook_index = ServerHooks::getServerHooks().getIndex(name);
/// Construct a CalloutEntry matching the current library and the callout
/// we want to remove.
......@@ -156,7 +161,7 @@ CalloutManager::deregisterAllCallouts(const std::string& name) {
// Get the index associated with this hook (validating the name in the
// process).
int hook_index = hooks_->getIndex(name);
int hook_index = ServerHooks::getServerHooks().getIndex(name);
/// Construct a CalloutEntry matching the current library (the callout
/// pointer is NULL as we are not checking that).
......
......@@ -92,26 +92,21 @@ public:
/// Initializes member variables, in particular sizing the hook vector
/// (the vector of callout vectors) to the appropriate size.
///
/// @param hooks Collection of known hook names.
/// @param num_libraries Number of loaded libraries.
///
/// @throw isc::BadValue if the number of libraries is less than or equal
/// to 0, or if the pointer to the server hooks object is empty.
CalloutManager(const boost::shared_ptr<ServerHooks>& hooks,
int num_libraries)
: current_library_(-1), hooks_(hooks), hook_vector_(),
CalloutManager(int num_libraries)
: current_hook_(-1), current_library_(-1), hook_vector_(),
library_handle_(this), num_libraries_(num_libraries)
{
if (!hooks) {
isc_throw(isc::BadValue, "must pass a pointer to a valid server "
"hooks object to the CalloutManager");
} else if (num_libraries <= 0) {
if (num_libraries <= 0) {
isc_throw(isc::BadValue, "number of libraries passed to the "
"CalloutManager must be >= 0");
}
// Parameters OK, do operations that depend on them.
hook_vector_.resize(hooks_->getCount());
hook_vector_.resize(ServerHooks::getServerHooks().getCount());
}
/// @brief Register a callout on a hook for the current library
......@@ -184,6 +179,14 @@ public:
/// current object being processed.
void callCallouts(int hook_index, CalloutHandle& callout_handle);
/// @brief Get current hook index
///
/// Made available during callCallouts, this is the index of the hook
/// on which callouts are being called.
int getHookIndex() const {
return (current_hook_);
}
/// @brief Get number of libraries
///
/// Returns the number of libraries that this CalloutManager is expected
......@@ -273,14 +276,16 @@ private:
}
};
/// Current hook. When a call is made to callCallouts, this holds the
/// index of the current hook. It is set to an invalid value (-1)
/// otherwise.
int current_hook_;
/// Current library index. When a call is made to any of the callout
/// registration methods, this variable indicates the index of the user
/// library that should be associated with the call.
int current_library_;
/// List of server hooks.
boost::shared_ptr<ServerHooks> hooks_;
/// Vector of callout vectors. There is one entry in this outer vector for
/// each hook. Each element is itself a vector, with one entry for each
/// callout registered for that hook.
......@@ -292,7 +297,6 @@ private:
/// Number of libraries.
int num_libraries_;
};
} // namespace util
......
......@@ -28,16 +28,7 @@ namespace util {
// assigned to them are as expected.
ServerHooks::ServerHooks() {
int create = registerHook("context_create");
int destroy = registerHook("context_destroy");
if ((create != CONTEXT_CREATE) || (destroy != CONTEXT_DESTROY)) {
isc_throw(Unexpected, "pre-defined hook indexes are not as expected. "
"context_create: expected = " << CONTEXT_CREATE <<
", actual = " << create <<
". context_destroy: expected = " << CONTEXT_DESTROY <<
", actual = " << destroy);
}
reset();
}
// Register a hook. The index assigned to the hook is the current number
......@@ -59,10 +50,49 @@ ServerHooks::registerHook(const string& name) {
" is already registered");
}
// New element inserted, return numeric index.
// Element was inserted, so add to the inverse hooks collection.
inverse_hooks_[index] = name;
// ... and return numeric index.
return (index);
}
// Reset ServerHooks object to initial state.
void
ServerHooks::reset() {
// Clear out the name->index and index->name maps.
hooks_.clear();
inverse_hooks_.clear();
// Register the pre-defined hooks.
int create = registerHook("context_create");
int destroy = registerHook("context_destroy");
// Check registration went as expected.
if ((create != CONTEXT_CREATE) || (destroy != CONTEXT_DESTROY)) {
isc_throw(Unexpected, "pre-defined hook indexes are not as expected. "
"context_create: expected = " << CONTEXT_CREATE <<
", actual = " << create <<
". context_destroy: expected = " << CONTEXT_DESTROY <<
", actual = " << destroy);
}
}
// Find the name associated with a hook index.
std::string
ServerHooks::getName(int index) const {
// Get iterator to matching element.
InverseHookCollection::const_iterator i = inverse_hooks_.find(index);
if (i == inverse_hooks_.end()) {
isc_throw(NoSuchHook, "hook index " << index << " is not recognised");
}
return (i->second);
}
// Find the index associated with a hook name.
int
......@@ -119,6 +149,13 @@ HookRegistrationFunction::execute(ServerHooks& hooks) {
}
}
// Return global ServerHooks object
ServerHooks&
ServerHooks::getServerHooks() {
static ServerHooks hooks;
return (hooks);
}
} // namespace util
......
......@@ -17,6 +17,8 @@
#include <exceptions/exceptions.h>
#include <boost/noncopyable.hpp>
#include <map>
#include <string>
#include <vector>
......@@ -57,24 +59,27 @@ public:
/// (Although it would be feasible to use a name as an index, using an integer
/// will speed up the time taken to locate the callouts, which may make a
/// difference in a frequently-executed piece of code.)
///
/// ServerHooks is a singleton object and is only accessible by the static
/// method getserverHooks().
class ServerHooks {
class ServerHooks : public boost::noncopyable {
public:
/// Index numbers for pre-defined hooks.
static const int CONTEXT_CREATE = 0;
static const int CONTEXT_DESTROY = 1;
/// @brief Constructor
/// @brief Reset to Initial State
///
/// This pre-registers two hooks, context_create and context_destroy, which
/// are called by the server before processing a packet and after processing
/// for the packet has completed. They allow the server code to allocate
/// and destroy per-packet context.
/// Resets the collection of hooks to the initial state, with just the
/// context_create and context_destroy hooks set. This used during
/// construction. It is also used during testing to reset the global
/// ServerHooks object.
///
/// @throws isc::Unexpected if the registration of the pre-defined hooks
/// fails in some way.
ServerHooks();
void reset();
/// @brief Register a hook
///
......@@ -90,6 +95,18 @@ public:
/// registered.
int registerHook(const std::string& name);
/// @brief Get hook name
///
/// Returns the name of a hook given the index. This is most likely to be
/// used in log messages.
///
/// @param index Index of the hoold
///
/// @return Name of the hook.
///
/// @throw NoSuchHook if the hook index is invalid.
std::string getName(int index) const;
/// @brief Get hook index
///
/// Returns the index of a hook.
......@@ -117,10 +134,37 @@ public:
/// @return Vector of strings holding hook names.
std::vector<std::string> getHookNames() const;
/// @brief Return ServerHookms object
///
/// Returns the global ServerHooks object.
///
/// @return Reference to the global ServerHooks object.
static ServerHooks& getServerHooks();
private:
/// @brief Constructor
///
/// This pre-registers two hooks, context_create and context_destroy, which
/// are called by the server before processing a packet and after processing
/// for the packet has completed. They allow the server code to allocate
/// and destroy per-packet context.
///
/// Constructor is declared private to enforce the singleton nature of
/// the object. A reference to the singleton is obtainable through the
/// ggetServerHooks() static method.
///
/// @throws isc::Unexpected if the registration of the pre-defined hooks
/// fails in some way.
ServerHooks();
/// Useful typedefs.
typedef std::map<std::string, int> HookCollection;
typedef std::map<int, std::string> InverseHookCollection;
HookCollection hooks_; ///< Hook name/index collection
/// Two maps, one for name->index, the other for index->name. (This is
/// simpler than using a multi-indexed container.)
HookCollection hooks_; ///< Hook name/index collection
InverseHookCollection inverse_hooks_; ///< Hook index/name collection
};
......
......@@ -40,8 +40,7 @@ public:
/// Sets up a callout manager to be referenced by the CalloutHandle in
/// these tests. (The "4" for the number of libraries in the
/// CalloutManager is arbitrary - it is not used in these tests.)
CalloutHandleTest()
: hooks_(new ServerHooks()), manager_(new CalloutManager(hooks_, 4))
CalloutHandleTest() : manager_(new CalloutManager(4))
{}
/// Obtain hook manager
......@@ -50,9 +49,6 @@ public:
}
private:
/// List of server hooks
boost::shared_ptr<ServerHooks> hooks_;
/// Callout manager accessed by this CalloutHandle.
boost::shared_ptr<CalloutManager> manager_;
};
......@@ -326,4 +322,8 @@ TEST_F(CalloutHandleTest, SkipFlag) {
EXPECT_FALSE(handle.getSkip());
}
// Further tests of the "skip" flag and tests of getting the name of the
// hook to which the current callout is attached is in the "handles_unittest"
// module.
} // Anonymous namespace
......@@ -44,17 +44,20 @@ public:
/// @brief Constructor
///
/// Sets up a collection of three LibraryHandle objects to use in the test.
CalloutManagerTest() : hooks_(new ServerHooks()) {
CalloutManagerTest() {
// Set up the server hooks
alpha_index_ = hooks_->registerHook("alpha");
beta_index_ = hooks_->registerHook("beta");
gamma_index_ = hooks_->registerHook("gamma");
delta_index_ = hooks_->registerHook("delta");
// Set up the server hooks. There is sone singleton for all tests,
// so reset it and explicitly set up the hooks for the test.
ServerHooks& hooks = ServerHooks::getServerHooks();
hooks.reset();
alpha_index_ = hooks.registerHook("alpha");
beta_index_ = hooks.registerHook("beta");
gamma_index_ = hooks.registerHook("gamma");
delta_index_ = hooks.registerHook("delta");
// Set up the callout manager with these hooks. Assume a maximum of
// four libraries.
callout_manager_.reset(new CalloutManager(hooks_, 10));
callout_manager_.reset(new CalloutManager(10));
// Set up the callout handle.
callout_handle_.reset(new CalloutHandle(callout_manager_));
......@@ -181,12 +184,8 @@ TEST_F(CalloutManagerTest, BadConstructorParameters) {
boost::scoped_ptr<CalloutManager> cm;
// Invalid number of libraries
EXPECT_THROW(cm.reset(new CalloutManager(getServerHooks(), 0)), BadValue);
EXPECT_THROW(cm.reset(new CalloutManager(getServerHooks(), -1)), BadValue);
// Invalid server hooks pointer.
boost::shared_ptr<ServerHooks> sh;
EXPECT_THROW(cm.reset(new CalloutManager(sh, 4)), BadValue);
EXPECT_THROW(cm.reset(new CalloutManager(0)), BadValue);
EXPECT_THROW(cm.reset(new CalloutManager(-1)), BadValue);
}
// Check the number of libraries is reported successfully.
......@@ -196,10 +195,10 @@ TEST_F(CalloutManagerTest, GetNumLibraries) {
// Check two valid values of number of libraries to ensure that the
// GetNumLibraries() returns the value set.
EXPECT_NO_THROW(cm.reset(new CalloutManager(getServerHooks(), 4)));
EXPECT_NO_THROW(cm.reset(new CalloutManager(4)));
EXPECT_EQ(4, cm->getNumLibraries());
EXPECT_NO_THROW(cm.reset(new CalloutManager(getServerHooks(), 42)));
EXPECT_NO_THROW(cm.reset(new CalloutManager(42)));
EXPECT_EQ(42, cm->getNumLibraries());
}
......@@ -760,6 +759,8 @@ TEST_F(CalloutManagerTest, LibraryHandleRegistration) {
EXPECT_EQ(1, callout_value_);
}
// The setting of the hook index is checked in the handles_unittest
// set of tests, as access restrictions mean it is not easily tested
// on its own.
} // Anonymous namespace
......@@ -50,16 +50,17 @@ public:
/// @brief Constructor
///
/// Sets up the various elements used in each test.
HandlesTest() : hooks_(new ServerHooks()), manager_()
{
HandlesTest() {
// Set up four hooks, although through gamma
alpha_index_ = hooks_->registerHook("alpha");
beta_index_ = hooks_->registerHook("beta");
gamma_index_ = hooks_->registerHook("gamma");
delta_index_ = hooks_->registerHook("delta");
ServerHooks& hooks = ServerHooks::getServerHooks();
hooks.reset();
alpha_index_ = hooks.registerHook("alpha");
beta_index_ = hooks.registerHook("beta");
gamma_index_ = hooks.registerHook("gamma");
delta_index_ = hooks.registerHook("delta");
// Set up for three libraries.
manager_.reset(new CalloutManager(hooks_, 3));
manager_.reset(new CalloutManager(3));
// Initialize remaining variables.
common_string_ = "";
......@@ -80,9 +81,6 @@ public:
static std::string common_string_;
private:
/// Server hooks
boost::shared_ptr<ServerHooks> hooks_;
/// Callout manager. Declared static so that the callout functions can
/// access it.
boost::shared_ptr<CalloutManager> manager_;
......@@ -917,6 +915,47 @@ TEST_F(HandlesTest, CheckModifiedArgument) {
EXPECT_EQ(std::string("YNN" "YYNN" "YNY"), modified_arg);
}
// Test that the CalloutHandle provides the name of the hook to which the
// callout is attached.
int
callout_hook_name(CalloutHandle& callout_handle) {
HandlesTest::common_string_ = callout_handle.getHookName();
return (0);
}
int
callout_hook_dummy(CalloutHandle&) {
return (0);
}
TEST_F(HandlesTest, HookName) {
getCalloutManager()->setLibraryIndex(0);
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.
CalloutHandle callout_handle(getCalloutManager());
EXPECT_EQ(std::string(""), HandlesTest::common_string_);
getCalloutManager()->callCallouts(alpha_index_, callout_handle);
EXPECT_EQ(std::string("alpha"), HandlesTest::common_string_);
getCalloutManager()->callCallouts(beta_index_, callout_handle);
EXPECT_EQ(std::string("beta"), HandlesTest::common_string_);
// Make sure that the callout accesses the name even if it is not the
// 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);
EXPECT_EQ(std::string("beta"), HandlesTest::common_string_);
getCalloutManager()->callCallouts(gamma_index_, callout_handle);
EXPECT_EQ(std::string("gamma"), HandlesTest::common_string_);
}
} // Anonymous namespace
......@@ -30,7 +30,8 @@ namespace {
// constructor registers two hooks, this is also a test of the constructor.
TEST(ServerHooksTest, RegisterHooks) {
ServerHooks hooks;
ServerHooks& hooks = ServerHooks::getServerHooks();
hooks.reset();
// There should be two hooks already registered, with indexes 0 and 1.
EXPECT_EQ(2, hooks.getCount());
......@@ -63,7 +64,8 @@ TEST(ServerHooksTest, RegisterHooks) {
// Check that duplicate names cannot be registered.
TEST(ServerHooksTest, DuplicateHooks) {
ServerHooks hooks;
ServerHooks& hooks = ServerHooks::getServerHooks();
hooks.reset();
// Ensure we can't duplicate one of the existing names.
EXPECT_THROW(hooks.registerHook("context_create"), DuplicateHook);
......@@ -77,8 +79,9 @@ TEST(ServerHooksTest, DuplicateHooks) {
// Checks that we can get the name of the hooks.
TEST(ServerHooksTest, GetHookNames) {
ServerHooks& hooks = ServerHooks::getServerHooks();
hooks.reset();
vector<string> expected_names;
ServerHooks hooks;
// Add names into the hooks object and to the set of expected names.
expected_names.push_back("alpha");
......@@ -104,10 +107,54 @@ TEST(ServerHooksTest, GetHookNames) {
EXPECT_TRUE(expected_names == actual_names);
}
// Test the inverse hooks functionality (i.e. given an index, get the name).
TEST(ServerHooksTest, GetHookIndexes) {
ServerHooks& hooks = ServerHooks::getServerHooks();
hooks.reset();
int alpha = hooks.registerHook("alpha");
int beta = hooks.registerHook("beta");
int gamma = hooks.registerHook("gamma");
EXPECT_EQ(std::string("context_create"),
hooks.getName(ServerHooks::CONTEXT_CREATE));
EXPECT_EQ(std::string("context_destroy"),
hooks.getName(ServerHooks::CONTEXT_DESTROY));
EXPECT_EQ(std::string("alpha"), hooks.getName(alpha));
EXPECT_EQ(std::string("beta"), hooks.getName(beta));
EXPECT_EQ(std::string("gamma"), hooks.getName(gamma));
// Check for an invalid index
EXPECT_THROW(hooks.getName(-1), NoSuchHook);
EXPECT_THROW(hooks.getName(42), NoSuchHook);
}
// Test the reset functionality.
TEST(ServerHooksTest, Reset) {
ServerHooks& hooks = ServerHooks::getServerHooks();
hooks.reset();
int alpha = hooks.registerHook("alpha");
int beta = hooks.registerHook("beta");
int gamma = hooks.registerHook("gamma");
// Check the counts before and after a reset.
EXPECT_EQ(5, hooks.getCount());
hooks.reset();
EXPECT_EQ(2, hooks.getCount());
// ... and check that the hooks are as expected.
EXPECT_EQ(0, hooks.getIndex("context_create"));
EXPECT_EQ(1, hooks.getIndex("context_destroy"));
}
// Check that getting an unknown name throws an exception.
TEST(ServerHooksTest, UnknownHookName) {
ServerHooks hooks;
ServerHooks& hooks = ServerHooks::getServerHooks();
hooks.reset();
EXPECT_THROW(static_cast<void>(hooks.getIndex("unknown")), NoSuchHook);
}
......@@ -115,7 +162,8 @@ TEST(ServerHooksTest, UnknownHookName) {
// Check that the count of hooks is correct.
TEST(ServerHooksTest, HookCount) {