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

[2974] Modifications as a result of review

parent 0df2936c
......@@ -34,12 +34,7 @@ CalloutHandle::CalloutHandle(const boost::shared_ptr<CalloutManager>& manager)
// Call the "context_create" hook. We should be OK doing this - although
// the constructor has not finished running, all the member variables
// have been created.
int status = manager_->callCallouts(ServerHooks::CONTEXT_CREATE, *this);
if (status > 0) {
isc_throw(ContextCreateFail, "error code of " << status << " returned "
"from context_create callout during the creation of a "
"ContextHandle object");
}
manager_->callCallouts(ServerHooks::CONTEXT_CREATE, *this);
}
// Destructor
......@@ -48,16 +43,7 @@ CalloutHandle::~CalloutHandle() {
// Call the "context_destroy" hook. We should be OK doing this - although
// the destructor is being called, all the member variables are still in
// existence.
int status = manager_->callCallouts(ServerHooks::CONTEXT_DESTROY, *this);
if (status > 0) {
// An exception is thrown on failure. This may be severe, but if
// none is thrown a resource leak in a library (signalled by the
// context_destroy callout returning an error) may be difficult to
// trace.
isc_throw(ContextDestroyFail, "error code of " << status << " returned "
"from context_destroy callout during the destruction of a "
"ContextHandle object");
}
manager_->callCallouts(ServerHooks::CONTEXT_DESTROY, *this);
}
// Return the name of all argument items.
......
......@@ -50,28 +50,6 @@ public:
isc::Exception(file, line, what) {}
};
/// @brief Context creation failure
///
/// Thrown if, during the running of the constructor, the call to the
/// context_create hook returns an error.
class ContextCreateFail : public Exception {
public:
ContextCreateFail(const char* file, size_t line, const char* what) :
isc::Exception(file, line, what) {}
};
/// @brief Context destruction failure
///
/// Thrown if, during the running of the desstructor, the call to the
/// context_destroy hook returns an error.
class ContextDestroyFail : public Exception {
public:
ContextDestroyFail(const char* file, size_t line, const char* what) :
isc::Exception(file, line, what) {}
};
// Forward declaration of the library handle and related collection classes.
class CalloutManager;
......@@ -109,14 +87,6 @@ class LibraryHandle;
class CalloutHandle {
public:
/// Callout success return status - the next callout in the list for the
/// hook will be called.
static const int SUCCESS = 0;
/// Callout complete return status - the callout has succeeded, but
/// remaining callouts on this hook (including any from other libraries)
/// should not be run.
static const int COMPLETE = 1;
/// Typedef to allow abbreviation of iterator specification in methods.
/// The std::string is the argument name and the "boost::any" is the
......
......@@ -63,7 +63,10 @@ CalloutManager::registerCallout(const std::string& name, CalloutPtr callout) {
bool
CalloutManager::calloutsPresent(int hook_index) const {
// Validate the hook index.
checkHookIndex(hook_index);
if ((hook_index < 0) || (hook_index >= hook_vector_.size())) {
isc_throw(NoSuchHook, "hook index " << hook_index <<
" is not valid for the list of registered hooks");
}
// Valid, so are there any callouts associated with that hook?
return (!hook_vector_[hook_index].empty());
......@@ -71,40 +74,40 @@ CalloutManager::calloutsPresent(int hook_index) const {
// Call all the callouts for a given hook.
int
void
CalloutManager::callCallouts(int hook_index, CalloutHandle& callout_handle) {
// Validate the hook index.
checkHookIndex(hook_index);
// Clear the "skip" flag so we don't carry state from a previous call.
callout_handle.setSkip(false);
// 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
// the list of callouts, the underlying CalloutVector would change and
// potentially affect the iteration through that vector.
CalloutVector callouts(hook_vector_[hook_index]);
// Call all the callouts, stopping if a non SUCCESS status is returned.
int status = CalloutHandle::SUCCESS;
for (CalloutVector::const_iterator i = callouts.begin();
i != callouts.end() && (status == CalloutHandle::SUCCESS); ++i) {
// In case the callout tries to register or deregister a callout, set
// the current library index to the index associated with library
// that registered the callout being called.
current_library_ = i->first;
// Call the callout
status = (*i->second)(callout_handle);
}
// Reset the current library index to an invalid value to catch any
// programming errors.
current_library_ = -1;
// Only initialize and iterate if there are callouts present. This check
// also catches the case of an invalid index.
if (calloutsPresent(hook_index)) {
// Clear the "skip" flag so we don't carry state from a previous call.
callout_handle.setSkip(false);
// 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
// the list of callouts on that hook, the underlying CalloutVector would
// change and potentially affect the iteration through that vector.
CalloutVector callouts(hook_vector_[hook_index]);
// Call all the callouts.
for (CalloutVector::const_iterator i = callouts.begin();
i != callouts.end(); ++i) {
// In case the callout tries to register or deregister a callout,
// set the current library index to the index associated with the
// library that registered the callout being called.
current_library_ = i->first;
// Call the callout
// @todo Log the return status if non-zero
static_cast<void>((*i->second)(callout_handle));
}
return (status);
// Reset the current library index to an invalid value to catch any
// programming errors.
current_library_ = -1;
}
}
// Deregister a callout registered by the current library on a particular hook.
......
......@@ -182,9 +182,7 @@ public:
/// @param hook_index Index of the hook to call.
/// @param callout_handle Reference to the CalloutHandle object for the
/// current object being processed.
///
/// @return Status return.
int callCallouts(int hook_index, CalloutHandle& callout_handle);
void callCallouts(int hook_index, CalloutHandle& callout_handle);
/// @brief Get number of libraries
///
......@@ -240,20 +238,6 @@ public:
}
private:
/// @brief Check hook index
///
/// Ensures that the passed hook index is valid.
///
/// @param index Hook index to test
///
/// @throw NoSuchHook Hooks does not exist.
void checkHookIndex(int hook_index) const {
if ((hook_index < 0) || (hook_index >= hook_vector_.size())) {
isc_throw(NoSuchHook, "hook index " << hook_index <<
" is not valid for the list of registered hooks");
}
}
/// @brief Check library index
///
/// Ensures that the current library index is valid. This is called by
......
......@@ -225,14 +225,13 @@ TEST_F(CalloutHandleTest, ContextItemNames) {
CalloutHandle handle(getCalloutManager());
vector<string> expected_names;
int value = 42;
expected_names.push_back("faith");
handle.setArgument("faith", value++);
handle.setArgument("faith", 42);
expected_names.push_back("hope");
handle.setArgument("hope", value++);
handle.setArgument("hope", 43);
expected_names.push_back("charity");
handle.setArgument("charity", value++);
handle.setArgument("charity", 44);
// Get the names and check against the expected names. We'll sort
// both arrays to simplify the checking.
......
......@@ -60,6 +60,9 @@ public:
// Set up for three libraries.
manager_.reset(new CalloutManager(hooks_, 3));
// Initialize remaining variables.
common_string_ = "";
}
/// @brief Return callout manager
......@@ -73,6 +76,9 @@ public:
int gamma_index_;
int delta_index_;
/// String accessible by all callouts whatever the library
static std::string common_string_;
private:
/// Server hooks
boost::shared_ptr<ServerHooks> hooks_;
......@@ -80,9 +86,12 @@ private:
/// Callout manager. Declared static so that the callout functions can
/// access it.
boost::shared_ptr<CalloutManager> manager_;
};
/// Define the common string
std::string HandlesTest::common_string_;
// The next set of functions define the callouts used by the tests. They
// manipulate the data in such a way that callouts called - and the order in
// which they were called - can be determined. The functions also check that
......@@ -547,24 +556,6 @@ TEST_F(HandlesTest, ConstructionDestructionCallouts) {
EXPECT_EQ("110120", resultCalloutString(0));
EXPECT_EQ((110 + 120), resultCalloutInt(0));
// Test that the destructor throws an error if the context_destroy
// callout returns an error. (As the constructor and destructor will
// have implicitly run the CalloutManager's callCallouts method, we need
// to set the library index again.)
getCalloutManager()->setLibraryIndex(0);
getCalloutManager()->registerCallout("context_destroy", returnError);
callout_handle.reset(new CalloutHandle(getCalloutManager()));
EXPECT_THROW(callout_handle.reset(), ContextDestroyFail);
// We don't know what callout_handle is pointing to - it could be to a
// half-destroyed object - so use a new CalloutHandle to test construction
// failure.
getCalloutManager()->setLibraryIndex(0);
getCalloutManager()->registerCallout("context_create", returnError);
boost::scoped_ptr<CalloutHandle> callout_handle2;
EXPECT_THROW(callout_handle2.reset(new CalloutHandle(getCalloutManager())),
ContextCreateFail);
}
// Dynamic callout registration and deregistration.
......@@ -765,6 +756,167 @@ TEST_F(HandlesTest, DynamicDeregistrationSameHook) {
EXPECT_EQ("212782", resultCalloutString(1));
}
// Testing the operation of the "skip" flag. Callouts print the value
// they see in the flag and either leave it unchanged, set it or clear it.
int
calloutPrintSkip(CalloutHandle& handle) {
static const std::string YES("Y");
static const std::string NO("N");
HandlesTest::common_string_ = HandlesTest::common_string_ +
(handle.getSkip() ? YES : NO);
return (0);
}
int
calloutSetSkip(CalloutHandle& handle) {
static_cast<void>(calloutPrintSkip(handle));
handle.setSkip(true);
return (0);
}
int
calloutClearSkip(CalloutHandle& handle) {
static_cast<void>(calloutPrintSkip(handle));
handle.setSkip(false);
return (0);
}
// Do a series of tests, returning with the skip flag set "true".
TEST_F(HandlesTest, ReturnSkipSet) {
getCalloutManager()->setLibraryIndex(0);
getCalloutManager()->registerCallout("alpha", calloutPrintSkip);
getCalloutManager()->registerCallout("alpha", calloutSetSkip);
getCalloutManager()->registerCallout("alpha", calloutSetSkip);
getCalloutManager()->registerCallout("alpha", calloutClearSkip);
getCalloutManager()->setLibraryIndex(1);
getCalloutManager()->registerCallout("alpha", calloutPrintSkip);
getCalloutManager()->registerCallout("alpha", calloutSetSkip);
getCalloutManager()->registerCallout("alpha", calloutSetSkip);
getCalloutManager()->registerCallout("alpha", calloutClearSkip);
getCalloutManager()->registerCallout("alpha", calloutClearSkip);
getCalloutManager()->setLibraryIndex(2);
getCalloutManager()->registerCallout("alpha", calloutPrintSkip);
getCalloutManager()->registerCallout("alpha", calloutSetSkip);
getCalloutManager()->registerCallout("alpha", calloutClearSkip);
getCalloutManager()->registerCallout("alpha", calloutSetSkip);
CalloutHandle callout_handle(getCalloutManager());
getCalloutManager()->callCallouts(alpha_index_, callout_handle);
// Check result. For each of visual checking, the expected string is
// divided into sections corresponding to the blocks of callouts above.
EXPECT_EQ(std::string("NNYY" "NNYYN" "NNYN"), common_string_);
// ... and check that the skip flag on exit from callCallouts is set.
EXPECT_TRUE(callout_handle.getSkip());
}
// Repeat the test, returning with the skip flag clear.
TEST_F(HandlesTest, ReturnSkipClear) {
getCalloutManager()->setLibraryIndex(0);
getCalloutManager()->registerCallout("alpha", calloutSetSkip);
getCalloutManager()->registerCallout("alpha", calloutSetSkip);
getCalloutManager()->registerCallout("alpha", calloutClearSkip);
getCalloutManager()->setLibraryIndex(1);
getCalloutManager()->registerCallout("alpha", calloutPrintSkip);
getCalloutManager()->registerCallout("alpha", calloutSetSkip);
getCalloutManager()->registerCallout("alpha", calloutClearSkip);
getCalloutManager()->registerCallout("alpha", calloutSetSkip);
getCalloutManager()->registerCallout("alpha", calloutClearSkip);
getCalloutManager()->registerCallout("alpha", calloutClearSkip);
getCalloutManager()->setLibraryIndex(2);
getCalloutManager()->registerCallout("alpha", calloutClearSkip);
getCalloutManager()->registerCallout("alpha", calloutPrintSkip);
getCalloutManager()->registerCallout("alpha", calloutSetSkip);
getCalloutManager()->registerCallout("alpha", calloutClearSkip);
CalloutHandle callout_handle(getCalloutManager());
getCalloutManager()->callCallouts(alpha_index_, callout_handle);
// Check result. For each of visual checking, the expected string is
// divided into sections corresponding to the blocks of callouts above.
EXPECT_EQ(std::string("NYY" "NNYNYN" "NNNY"), common_string_);
// ... and check that the skip flag on exit from callCallouts is set.
EXPECT_FALSE(callout_handle.getSkip());
}
// The next set of callouts do a similar thing to the above "skip" tests,
// but alter the value of a string argument. This is for testing that the
// a callout is able to change an argument and return it to the caller.
const char* MODIFIED_ARG = "modified_arg";
int
calloutSetArgumentCommon(CalloutHandle& handle, const char* what) {
std::string modified_arg = "";
handle.getArgument(MODIFIED_ARG, modified_arg);
modified_arg = modified_arg + std::string(what);
handle.setArgument(MODIFIED_ARG, modified_arg);
return (0);
}
int
calloutSetArgumentYes(CalloutHandle& handle) {
return (calloutSetArgumentCommon(handle, "Y"));
}
int
calloutSetArgumentNo(CalloutHandle& handle) {
return (calloutSetArgumentCommon(handle, "N"));
}
// ... and a callout to just copy the argument to the "common_string_" variable
// but otherwise not alter it.
int
calloutPrintArgument(CalloutHandle& handle) {
handle.getArgument(MODIFIED_ARG, HandlesTest::common_string_);
return (0);
}
TEST_F(HandlesTest, CheckModifiedArgument) {
getCalloutManager()->setLibraryIndex(0);
getCalloutManager()->registerCallout("alpha", calloutSetArgumentYes);
getCalloutManager()->registerCallout("alpha", calloutSetArgumentNo);
getCalloutManager()->registerCallout("alpha", calloutSetArgumentNo);
getCalloutManager()->setLibraryIndex(1);
getCalloutManager()->registerCallout("alpha", calloutSetArgumentYes);
getCalloutManager()->registerCallout("alpha", calloutSetArgumentYes);
getCalloutManager()->registerCallout("alpha", calloutPrintArgument);
getCalloutManager()->registerCallout("alpha", calloutSetArgumentNo);
getCalloutManager()->registerCallout("alpha", calloutSetArgumentNo);
getCalloutManager()->setLibraryIndex(2);
getCalloutManager()->registerCallout("alpha", calloutSetArgumentYes);
getCalloutManager()->registerCallout("alpha", calloutSetArgumentNo);
getCalloutManager()->registerCallout("alpha", calloutSetArgumentYes);
// Create the argument with an initial empty string value. Then call the
// sequence of callouts above.
CalloutHandle callout_handle(getCalloutManager());
std::string modified_arg = "";
callout_handle.setArgument(MODIFIED_ARG, modified_arg);
getCalloutManager()->callCallouts(alpha_index_, callout_handle);
// Check the intermediate and results. For visual checking, the expected
// string is divided into sections corresponding to the blocks of callouts
// above.
EXPECT_EQ(std::string("YNN" "YY"), common_string_);
callout_handle.getArgument(MODIFIED_ARG, modified_arg);
EXPECT_EQ(std::string("YNN" "YYNN" "YNY"), modified_arg);
}
} // Anonymous namespace
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment