BIND 10 trac2974, updated. e37828dd1c90541e6e6d7f8d373d95a25ce6bbea [2974] Minor tweaks and documentation changes - now ready for review
BIND 10 source code commits
bind10-changes at lists.isc.org
Wed May 29 21:51:33 UTC 2013
The branch, trac2974 has been updated
via e37828dd1c90541e6e6d7f8d373d95a25ce6bbea (commit)
from 923462dbabee6917d6693e62c80bd0f7c8fd980b (commit)
Those revisions listed above that are new to this repository have
not appeared on any other notification email; so we list those
revisions in full, below.
- Log -----------------------------------------------------------------
commit e37828dd1c90541e6e6d7f8d373d95a25ce6bbea
Author: Stephen Morris <stephen at isc.org>
Date: Wed May 29 22:50:56 2013 +0100
[2974] Minor tweaks and documentation changes - now ready for review
-----------------------------------------------------------------------
Summary of changes:
src/lib/util/hooks/callout_handle.cc | 7 +-
src/lib/util/hooks/callout_handle.h | 82 ++++++------
src/lib/util/hooks/library_handle.cc | 43 +++---
src/lib/util/hooks/library_handle.h | 141 +++++++++++---------
src/lib/util/hooks/server_hooks.cc | 14 +-
src/lib/util/hooks/server_hooks.h | 14 +-
src/lib/util/tests/callout_handle_unittest.cc | 29 ++--
src/lib/util/tests/handles_unittest.cc | 44 +++---
.../tests/library_handle_collection_unittest.cc | 63 ++++-----
src/lib/util/tests/library_handle_unittest.cc | 86 ++++++------
src/lib/util/tests/server_hooks_unittest.cc | 30 +++--
11 files changed, 286 insertions(+), 267 deletions(-)
-----------------------------------------------------------------------
diff --git a/src/lib/util/hooks/callout_handle.cc b/src/lib/util/hooks/callout_handle.cc
index 60b86d3..6d2f867 100644
--- a/src/lib/util/hooks/callout_handle.cc
+++ b/src/lib/util/hooks/callout_handle.cc
@@ -50,7 +50,6 @@ CalloutHandle::getLibraryHandle() const {
// after this method returns, because this object maintains a shared
// pointer to the LibraryHandleCollection, which in turn maintains
// a shared pointer to the LibraryHandle in question.
-
return (*handle);
}
@@ -72,7 +71,6 @@ CalloutHandle::getLibraryIndex() const {
CalloutHandle::ElementCollection&
CalloutHandle::getContextForLibrary() {
-
int libindex = getLibraryIndex();
// Access a reference to the element collection for the given index,
@@ -81,13 +79,12 @@ CalloutHandle::getContextForLibrary() {
}
// The "const" version of the above, used by the "getContext()" method. If
-// the context for the current library doesn't exist, throw a
-// "NoSuchCalloutContext" exception.
+// the context for the current library doesn't exist, throw an exception.
const CalloutHandle::ElementCollection&
CalloutHandle::getContextForLibrary() const {
-
int libindex = getLibraryIndex();
+
ContextCollection::const_iterator libcontext =
context_collection_.find(libindex);
if (libcontext == context_collection_.end()) {
diff --git a/src/lib/util/hooks/callout_handle.h b/src/lib/util/hooks/callout_handle.h
index 59df050..35d0e31 100644
--- a/src/lib/util/hooks/callout_handle.h
+++ b/src/lib/util/hooks/callout_handle.h
@@ -29,7 +29,8 @@ namespace util {
/// @brief No such argument
///
-/// Thrown if an attempt is made to use an invalid argument name.
+/// Thrown if an attempt is made access an argument that does not exist.
+
class NoSuchArgument : public Exception {
public:
NoSuchArgument(const char* file, size_t line, const char* what) :
@@ -39,38 +40,39 @@ public:
/// @brief No such callout context item
///
/// Thrown if an attempt is made to get an item of data from this callout's
-/// context.
+/// context and either the context or an item in the context with that name
+/// does not exist.
+
class NoSuchCalloutContext : public Exception {
public:
NoSuchCalloutContext(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 LibraryHandle;
class LibraryHandleCollection;
/// @brief Per-packet callout handle
///
/// An object of this class is associated with every packet (or request)
-/// processed by the server. It forms the principle means of passing
-/// data between the server and the user-library callouts.
+/// processed by the server. It forms the principle means of passing data
+/// between the server and the user-library callouts.
///
/// The class allows access to the following information:
///
/// - Arguments. When the callouts associated with a hook are called, they
/// are passed information by the server (and can return information to it)
/// through name/value pairs. Each of these pairs is an argument and the
-/// information is accessed through the {get,get}Argument() methods.
+/// information is accessed through the {get,set}Argument() methods.
///
/// - Per-packet context. Each packet has a context associated with it, this
-/// context being a per-library context. In other words, As a packet passes
+/// context being on a per-library basis. In other words, As a packet passes
/// through the callouts associated with a given library, the callouts can
/// associate and retrieve information with the packet. The per-library
/// nature of the context means that the callouts within a given library can
-/// pass packet-specific information between one another, they cannot pass
+/// pass packet-specific information between one another, but they cannot pass
/// information to callous within another library. Typically such context
/// is created in the "context_create" callout and destroyed in the
/// "context_destroy" callout. The information is accessed through the
@@ -79,7 +81,7 @@ class LibraryHandleCollection;
/// - Per-library context. Each library has a context associated with it that
/// is independent of the packet. All callouts in the library share the
/// information in the context, regardless of the packet being processed.
-/// The context is typically createed in the library's "load()" method and
+/// The context is typically created in the library's "load()" method and
/// destroyed in its "unload()" method. The information is available by
/// obtaining the handle to the library through this class's
/// getLibraryHandle() method, then calling {get,set}Context() on the
@@ -100,7 +102,7 @@ public:
/// library handles and a name.value collection.
///
/// The collection of contexts is stored in a map, as not every library
- /// will require creating a context associated with each packet. In
+ /// will require creation of a context associated with each packet. In
/// addition, the structure is more flexible in that the size does not
/// need to be set when the CalloutHandle is constructed.
typedef std::map<int, ElementCollection> ContextCollection;
@@ -116,9 +118,10 @@ public:
/// @brief Set argument
///
- /// Sets an argument. If the argument is already present, it is replaced.
+ /// Sets the value of an argument. The argument is created if it does not
+ /// already exist.
///
- /// @param name Name of the element in the context to set
+ /// @param name Name of the argument.
/// @param value Value to set
template <typename T>
void setArgument(const std::string& name, T value) {
@@ -127,17 +130,16 @@ public:
/// @brief Get argument
///
- /// Gets an argument. If an argument of the given name does not exist,
- /// a "NoSuchArgument" exception is thrown.
+ /// Gets the value of an argument.
///
- /// @param name Name of the element in the argument list to set.
+ /// @param name Name of the element in the argument list to get.
/// @param value [out] Value to set. The type of "value" is important:
/// it must match the type of the value set.
///
- /// @throw NoSuchArgument Thrown if no argument with the name "name" is
- /// present.
- /// @throw boost::bad_any_cast Thrown if the context element is present,
- /// but the type of the element is not that expected
+ /// @throw NoSuchArgument No argument with the given name is present.
+ /// @throw boost::bad_any_cast An argument with the given name is present,
+ /// but the data type of the value is not the same as the type of
+ /// the variable provided to receive the value.
template <typename T>
void getArgument(const std::string& name, T& value) const {
ElementCollection::const_iterator element_ptr = arguments_.find(name);
@@ -208,7 +210,7 @@ public:
/// library index is valid. A callout would use this method to get to
/// the context associated with the library in which it resides.
///
- /// @return Reference to the current library handle
+ /// @return Reference to the current library handle.
///
/// @throw InvalidIndex thrown if this method is called outside of the
/// "callCallouts() method. (Exception is so-named because the
@@ -221,8 +223,8 @@ public:
/// Sets an element in the context associated with the current library. If
/// an element of the name is already present, it is replaced.
///
- /// @param name Name of the element in the context to set
- /// @param value Value to set
+ /// @param name Name of the element in the context to set.
+ /// @param value Value to set.
template <typename T>
void setContext(const std::string& name, T value) {
getContextForLibrary()[name] = value;
@@ -238,8 +240,9 @@ public:
///
/// @throw NoSuchCalloutContext Thrown if no context element with the name
/// "name" is present.
- /// @throw boost::bad_any_cast Thrown if the context element is present,
- /// but the type of the element is not that expected
+ /// @throw boost::bad_any_cast Thrown if the context element is present
+ /// but the type of the data is not the same as the type of the
+ /// variable provided to receive its value.
template <typename T>
void getContext(const std::string& name, T& value) const {
const ElementCollection& lib_context = getContextForLibrary();
@@ -259,7 +262,8 @@ public:
/// Returns a vector holding the names of items in the context associated
/// with the current library.
///
- /// @return Vector of strings reflecting argument names
+ /// @return Vector of strings reflecting the names of items in the callout
+ /// context associated with the current library.
std::vector<std::string> getContextNames() const;
/// @brief Delete context element
@@ -271,7 +275,7 @@ public:
/// N.B. If the element is a raw pointer, the pointed-to data is NOT deleted
/// by this.
///
- /// @param name Name of the element in the argument list to set.
+ /// @param name Name of the context item to delete.
void deleteContext(const std::string& name) {
static_cast<void>(getContextForLibrary().erase(name));
}
@@ -294,29 +298,29 @@ private:
/// or is invalid for the current library collection.
///
/// @return Current library index, valid for this library collection.
+ ///
+ /// @throw InvalidIndex current library index is not valid for the library
+ /// handle collection.
int getLibraryIndex() const;
/// @brief Return reference to context for current library
///
- /// Called by all context-accessing functions, this checks if the current
- /// library index is valid (throwing an exception if not). If it is, it
- /// returns a reference to the appropriate context, creating one if it does
- /// not exist. (The last task allows the list of library handles to grow
- /// dynamically - although only if handles are appended to the end of the
- /// library handle collection.)
+ /// Called by all context-setting functions, this returns a reference to
+ /// the callout context for the current library, creating a context if it
+ /// does not exist.
///
/// @return Reference to the collection of name/value pairs associated
/// with the current library.
+ ///
+ /// @throw InvalidIndex current library index is not valid for the library
+ /// handle collection.
ElementCollection& getContextForLibrary();
/// @brief Return reference to context for current library (const version)
///
- /// Called by all context-accessing functions, this checks if the current
- /// library index is valid (throwing an exception if not). If it is, it
- /// returns a reference to the appropriate context, creating one if it does
- /// not exist. (The last task allows the list of library handles to grow
- /// dynamically - although only if handles are appended to the end of the
- /// library handle collection.)
+ /// Called by all context-accessing functions, this a reference to the
+ /// callout context for the current library. An exception is thrown if
+ /// it does not exist.
///
/// @return Reference to the collection of name/value pairs associated
/// with the current library.
diff --git a/src/lib/util/hooks/library_handle.cc b/src/lib/util/hooks/library_handle.cc
index 6947a6f..1fabfc8 100644
--- a/src/lib/util/hooks/library_handle.cc
+++ b/src/lib/util/hooks/library_handle.cc
@@ -24,7 +24,8 @@ using namespace isc::util;
namespace isc {
namespace util {
-// Check that an index is valid for the hook vector
+// Check that an index is valid for the hook vector.
+
void
LibraryHandle::checkHookIndex(int index) const {
if ((index < 0) || (index >= hook_vector_.size())) {
@@ -33,7 +34,8 @@ LibraryHandle::checkHookIndex(int index) const {
}
}
-// Get index for named hook
+// Get index for named hook.
+
int
LibraryHandle::getHookIndex(const std::string& name) const {
@@ -51,7 +53,8 @@ LibraryHandle::getHookIndex(const std::string& name) const {
return (index);
}
-// Register a callout at the back of the named hook
+// Register a callout for a hook, adding it to run after any previously
+// registered callouts on that hook.
void
LibraryHandle::registerCallout(const std::string& name, CalloutPtr callout) {
@@ -60,7 +63,7 @@ LibraryHandle::registerCallout(const std::string& name, CalloutPtr callout) {
// do so.
int index = getHookIndex(name);
- // Index valid, so add the callout to the end of the list.
+ // Index valid, so add the callout to the end of the list of callouts.
hook_vector_[index].push_back(callout);
}
@@ -84,8 +87,8 @@ LibraryHandle::callCallouts(int index, CalloutHandle& callout_handle) {
// Validate the hook index.
checkHookIndex(index);
- // Call all the callouts, stopping if a non-zero status is returned or if
- // the "skip" flag is set.
+ // Call all the callouts, stopping if the "skip" flag is set or if a
+ // non-zero status is returned.
int status = 0;
for (int i = 0;
(i < hook_vector_[index].size()) && !callout_handle.getSkip() &&
@@ -97,7 +100,7 @@ LibraryHandle::callCallouts(int index, CalloutHandle& callout_handle) {
return (status);
}
-// Deregister a callout
+// Deregister a callout on a given hook.
void
LibraryHandle::deregisterCallout(const std::string& name, CalloutPtr callout) {
@@ -108,10 +111,10 @@ LibraryHandle::deregisterCallout(const std::string& name, CalloutPtr callout) {
if (!hook_vector_[index].empty()) {
// The next bit is standard STL (see "Item 33" in "Effective STL" by
- // Scott Meyters.
+ // Scott Meyers).
//
// remove_if reorders the hook vector so that all items not matching
- // the predicate are at the start of the vector, and returns a pointer
+ // the predicate are at the start of the vector and returns a pointer
// to the next element. (In this case, the predicate is that the item
// is equal to the value of the passed callout.) The erase() call
// removes everything from that element to the end of the vector, i.e.
@@ -124,7 +127,7 @@ LibraryHandle::deregisterCallout(const std::string& name, CalloutPtr callout) {
}
}
-// Deregister all callouts
+// Deregister all callouts on a given hook.
void
LibraryHandle::deregisterAll(const std::string& name) {
@@ -137,7 +140,7 @@ LibraryHandle::deregisterAll(const std::string& name) {
hook_vector_[index].clear();
}
-// return the name of all context items.
+// Return the name of all items in the library's context.
vector<string>
LibraryHandle::getContextNames() const {
@@ -160,13 +163,14 @@ LibraryHandleCollection::getLibraryHandle() const {
if ((curidx_ < 0) || (curidx_ >= handles_.size())) {
isc_throw(InvalidIndex, "current library handle index of (" <<
curidx_ << ") is not valid for the library handle vector "
- "of size " << handles_.size());
+ "(size = " << handles_.size() << ")");
}
return (handles_[curidx_]);
}
-// Check if a callout is present on a hook in any of the libraries.
+// Check if a any of the libraries have at least one callout present on a given
+// hook.
bool
LibraryHandleCollection::calloutsPresent(int index) const {
@@ -188,21 +192,22 @@ int
LibraryHandleCollection::callCallouts(int index,
CalloutHandle& callout_handle) {
- // Don't validate the hook index, that is checked in the call to the
+ // Don't validate the hook index here as it is checked in the call to the
// callCallouts() method of the first library handle.
// Clear the skip flag before we start so that no state from a previous
// call of a hook accidentally leaks through.
callout_handle.setSkip(false);
- // Call all the callouts, stopping if a non-zero status is returned or if
- // the "skip" flag is set. Note that we iterate using the current index
- // as the counter (so that the callout handle object retrieves the
- // correct LibraryHandle from the collection).
+ // Call all the callouts, stopping if the "skip" flag is set or if a
+ // non-zero status is returned. Note that we iterate using the current
+ // index as the counter to allow callout handle object to retrieve the
+ // current LibraryHandle.
int status = 0;
for (curidx_ = 0;
(curidx_ < handles_.size()) && !callout_handle.getSkip() &&
- (status == 0); ++curidx_) {
+ (status == 0);
+ ++curidx_) {
status = handles_[curidx_]->callCallouts(index, callout_handle);
}
diff --git a/src/lib/util/hooks/library_handle.h b/src/lib/util/hooks/library_handle.h
index 22091ad..b7ddf61 100644
--- a/src/lib/util/hooks/library_handle.h
+++ b/src/lib/util/hooks/library_handle.h
@@ -15,14 +15,14 @@
#ifndef LIBRARY_HANDLE_H
#define LIBRARY_HANDLE_H
-#include <map>
-#include <string>
+#include <exceptions/exceptions.h>
+#include <util/hooks/server_hooks.h>
#include <boost/any.hpp>
#include <boost/shared_ptr.hpp>
-#include <exceptions/exceptions.h>
-#include <util/hooks/server_hooks.h>
+#include <map>
+#include <string>
namespace isc {
namespace util {
@@ -30,6 +30,7 @@ namespace util {
/// @brief No such hook
///
/// Thrown if an attempt is made to use an invalid hook name or hook index.
+
class NoSuchHook : public Exception {
public:
NoSuchHook(const char* file, size_t line, const char* what) :
@@ -41,9 +42,9 @@ public:
/// Thrown if an attempt is made to obtain context that has not been previously
/// set.
-class NoSuchContext : public Exception {
+class NoSuchLibraryContext : public Exception {
public:
- NoSuchContext(const char* file, size_t line, const char* what) :
+ NoSuchLibraryContext(const char* file, size_t line, const char* what) :
isc::Exception(file, line, what) {}
};
@@ -62,7 +63,7 @@ public:
// Forward declaration for CalloutHandle
class CalloutHandle;
-/// Typedef for a callout pointer
+/// Typedef for a callout pointer. (Callouts must have "C" linkage.)
extern "C" {
typedef int (*CalloutPtr)(CalloutHandle&);
};
@@ -74,12 +75,15 @@ extern "C" {
/// library to register callouts and by the HookManager to call them. The
/// class also contains storage for library-specific context.
///
-/// Although there is a persuasive argument for the class to load unload the
-/// user library, that is handled by the HookManager to prevent the user library
-/// from accessing those functions.
+/// The functions related to loading and unloading the asssociated library are
+/// handled in the related LibraryManager class - there is a 1:1 correspondence
+/// between LibraryManager and LibraryHandle objects. The separation prevents
+/// the user library callouts from tinkering around with the loading and
+/// unloading of libraries.
class LibraryHandle {
private:
+
/// Typedef to allow abbreviation of iterator specification in methods
typedef std::map<std::string, boost::any> ContextCollection;
@@ -87,9 +91,8 @@ public:
/// @brief Constructor
///
- /// This is passed the ServerHooks object and an index number: the former
- /// allows for the sizing of the internal hook vector, and the latter
- /// is used by the CalloutHandle object to access appropriate context
+ /// This is passed the ServerHooks object, which is used both to size the
+ /// internal hook vector and in the registration of callouts.
///
/// @param hooks Pointer to the hooks registered by the server.
LibraryHandle(boost::shared_ptr<ServerHooks>& hooks)
@@ -109,25 +112,24 @@ public:
}
/// @brief Get context
-
///
- /// Sets an element in the library context. If the name does not exist,
- /// a "NoSuchContext" exception is thrown.
+ /// Gets an element in the library context.
///
- /// @param name Name of the element in the context to set.
- /// @param value [out] Value to set. The type of "value" is important:
+ /// @param name Name of the element in the context to get.
+ /// @param value [out] retrieved value. The type of "value" is important:
/// it must match the type of the value set.
///
- /// @throw NoSuchContext Thrown if no context element with the name
- /// "name" is present.
- /// @throw boost::bad_any_cast Thrown if the context element is present,
- /// but the type of the element is not that expected
+ /// @throw NoSuchLibraryContext No context element with the given name
+ /// is present.
+ /// @throw boost::bad_any_cast The context element is present, but the
+ /// type of the element does not match the type of the variable
+ /// specified to receive it.
template <typename T>
void getContext(const std::string& name, T& value) const {
ContextCollection::const_iterator element_ptr = context_.find(name);
if (element_ptr == context_.end()) {
- isc_throw(NoSuchContext, "unable to find library context item " <<
- name << " in library handle");
+ isc_throw(NoSuchLibraryContext, "unable to find library context "
+ "item " << name << " in library handle");
}
value = boost::any_cast<T>(element_ptr->second);
@@ -142,11 +144,11 @@ public:
/// @brief Delete context element
///
- /// Deletes context item of the given name. If an item of that name
+ /// Deletes context item of the given name. If an item of that name
/// does not exist, the method is a no-op.
///
- /// N.B. If the element is a raw pointer, the pointed-to data is
- /// NOT deleted by this.
+ /// N.B. If the element is a raw pointer, the pointed-to data is NOT deleted
+ /// by this method.
///
/// @param name Name of the element in the argument list to set.
void deleteContext(const std::string& name) {
@@ -157,13 +159,13 @@ public:
///
/// Deletes all arguments associated with this context.
///
- /// N.B. If any elements are raw pointers, the pointed-to data is
- /// NOT deleted by this.
+ /// N.B. If any elements are raw pointers, the pointed-to data is NOT
+ /// deleted by this method.
void deleteAllContext() {
context_.clear();
}
- /// @brief Register a callout
+ /// @brief Register a callout on a hook
///
/// Registers a callout function with a given hook. The callout is added
/// to the end of the callouts associated with the hook.
@@ -171,12 +173,12 @@ public:
/// @param name Name of the hook to which the callout is added.
/// @param callout Pointer to the callout function to be registered.
///
- /// @throw NoSuchHook Thrown if the hook name is unrecognised.
- /// @throw Unexpected Hooks name is valid but internal data structure is
- /// of the wrong size.
+ /// @throw NoSuchHook The hook name is unrecognised.
+ /// @throw Unexpected The hook name is valid but an internal data structure
+ /// is of the wrong size.
void registerCallout(const std::string& name, CalloutPtr callout);
- /// @brief De-Register a callout
+ /// @brief De-Register a callout on a hook
///
/// Searches through the functions associated with the named hook and
/// removes all entries matching the callout. If there are no matching
@@ -185,12 +187,12 @@ public:
/// @param name Name of the hook from which the callout is removed.
/// @param callout Pointer to the callout function to be removed.
///
- /// @throw NoSuchHook Thrown if the hook name is unrecognised.
- /// @throw Unexpected Hooks name is valid but internal data structure is
- /// of the wrong size.
+ /// @throw NoSuchHook The hook name is unrecognised.
+ /// @throw Unexpected The hook name is valid but an internal data structure
+ /// is of the wrong size.
void deregisterCallout(const std::string& name, CalloutPtr callout);
- /// @brief Removes all callouts
+ /// @brief Removes all callouts on a hook
///
/// Removes all callouts associated with a given hook. This is a no-op
/// if there are no callouts associated with the hook.
@@ -200,7 +202,7 @@ public:
/// @throw NoSuchHook Thrown if the hook name is unrecognised.
void deregisterAll(const std::string& name);
- /// @brief Checks if callouts are present
+ /// @brief Checks if callouts are present on a hook
///
/// @param index Hook index for which callouts are checked.
///
@@ -218,10 +220,10 @@ public:
/// current object being processed.
///
/// @return Status return.
- ///
int callCallouts(int index, CalloutHandle& callout_handle);
private:
+
/// @brief Check hook index
///
/// Checks that the hook index is valid for the hook vector. If not,
@@ -229,7 +231,9 @@ private:
///
/// @param index Hooks index to check.
///
- /// @throw NoSuchHook Thrown if the index is not valid for the hook vector.
+ /// @throw NoSuchHook The index is not valid for the hook vector (i.e.
+ /// less than zero or equal to or greater than the size of the
+ /// vector).
void checkHookIndex(int index) const;
/// @brief Get hook index
@@ -238,15 +242,16 @@ private:
/// also checks for validity of the index: if the name is valid, the
/// index should be valid. However, as the class only keeps a pointer to
/// a shared ServerHooks object, it is possible that the object was modified
- /// after the hook_vector_ was sized. This function performs some checks
- /// on the name and throws an exception if the checks fail.
+ /// after the hook_vector_ was sized: in this case the name could be valid
+ /// but the index is invalid.
///
/// @param name Name of the hook to check
///
/// @return Index of the hook in the hook_vector_
///
- /// @throw NoSuchHook Thrown if the hook name is unrecognised.
- /// @throw Unexpected Index not valid for the hook vector.
+ /// @throw NoSuchHook The hook name is unrecognised.
+ /// @throw Unexpected Index of the hook name is not valid for the hook
+ /// vector.
int getHookIndex(const std::string& name) const;
// Member variables
@@ -255,11 +260,11 @@ private:
ContextCollection context_;
/// Pointer to the list of hooks registered by the server
- boost::shared_ptr<ServerHooks> hooks_; ///< Pointer to hooks
+ boost::shared_ptr<ServerHooks> hooks_;
/// Each element in the following vector corresponds to a single hook and
/// is an ordered list of callouts for that hook.
- std::vector<std::vector<CalloutPtr> > hook_vector_;
+ std::vector<std::vector<CalloutPtr> > hook_vector_;
};
@@ -267,33 +272,37 @@ private:
///
/// This simple class is a collection of handles for all libraries loaded.
/// It is pointed to by the CalloutHandle object and is used by that object
-/// to locate the correct LibraryHandle to pass to a callout function. To
-/// do this, the class contains an index indicating the "current" handle. This
-/// is used in the calling of callouts: prior to calling a callout associated
-/// with a LibraryHandle, the index is updated to point to that handle.
-/// If the callout requests access to the LibraryHandle, it is passed a
-/// reference to the correct one.
+/// to locate the correct LibraryHandle should one be requested by a callout
+/// function.
+///
+/// To do this, the class contains an index indicating the "current" handle.
+/// This is updated during the calling of callouts: prior to calling a callout
+/// associated with a particular LibraryHandle, the index is updated to point to
+/// that handle. If the callout requests access to the LibraryHandle, it is
+/// passed a reference to the correct one.
class LibraryHandleCollection {
private:
+
/// Private typedef to abbreviate statements in class methods.
typedef std::vector<boost::shared_ptr<LibraryHandle> > HandleVector;
+
public:
/// @brief Constructor
///
- /// Initializes member variables, in particular setting the current index
- /// to an invalid value.
+ /// Initializes member variables, in particular setting the "current library
+ /// handle index" to an invalid value.
LibraryHandleCollection() : curidx_(-1), handles_()
{}
/// @brief Add library handle
///
/// Adds a library handle to the collection. The collection is ordered,
- /// and this adds a library handle to the end of the current list.
+ /// and this adds a library handle to the end of it.
///
/// @param library_handle Pointer to the a library handle to be added.
- void addLibraryHandle(boost::shared_ptr<LibraryHandle>& handle) {
+ void addLibraryHandle(const boost::shared_ptr<LibraryHandle>& handle) {
handles_.push_back(handle);
}
@@ -301,8 +310,8 @@ public:
///
/// Returns the value of the "current library index". Although a callout
/// callout can retrieve this information, it is of limited use: the
- /// value is intended for use by the CalloutHandle object in order to
- /// access the per-library context.
+ /// value is intended for use by the CalloutHandle object to access the
+ /// per-library context.
///
/// @return Current library index value
int getLibraryIndex() const {
@@ -316,20 +325,20 @@ public:
/// library handles: calling it at any other time is meaningless and will
/// cause an exception to be thrown.
///
- /// @return Pointer to current library handle. This is the handle for
- /// which a callout is being called.
+ /// @return Pointer to current library handle. This is the handle for the
+ /// library on which the callout currently running is associated.
boost::shared_ptr<LibraryHandle> getLibraryHandle() const;
- /// @brief Checks if callouts are present
+ /// @brief Checks if callouts are present on a hook
///
/// Checks all loaded libraries and returns true if at least one callout
- /// has been registered for a hook in any of them.
+ /// has been registered by any of them for the given hook.
///
/// @param index Hook index for which callouts are checked.
///
/// @return true if callouts are present, false if not.
///
- /// @throw NoSuchHook Thrown if the index is not valid.
+ /// @throw NoSuchHook Given index does not correspond to a valid hook.
bool calloutsPresent(int index) const;
/// @brief Calls the callouts for a given hook
@@ -342,11 +351,11 @@ public:
/// current object being processed.
///
/// @return Status return.
- ///
int callCallouts(int index, CalloutHandle& callout_handle);
private:
- /// Index of the current library handle during iteration.
+ /// Index of the library handle on which the currently called callout is
+ /// registered.
int curidx_;
/// Vector of pointers to library handles.
diff --git a/src/lib/util/hooks/server_hooks.cc b/src/lib/util/hooks/server_hooks.cc
index 81ef571..1d7fcf5 100644
--- a/src/lib/util/hooks/server_hooks.cc
+++ b/src/lib/util/hooks/server_hooks.cc
@@ -40,7 +40,8 @@ ServerHooks::ServerHooks() {
}
// Register a hook. The index assigned to the hook is the current number
-// of entries in the collection.
+// of entries in the collection, so ensuring that hook indexes are unique
+// (and non-negative).
int
ServerHooks::registerHook(const string& name) {
@@ -66,16 +67,15 @@ ServerHooks::registerHook(const string& name) {
int
ServerHooks::getIndex(const string& name) const {
- // Return pair of <hook name, index>.
+ // Get iterator to matching element.
HookCollection::const_iterator i = hooks_.find(name);
- if (i == hooks_.end()) {
- return (-1);
- }
- return (i->second);
+ // ... and convert this into a return value.
+ return ((i == hooks_.end()) ? -1 : i->second);
}
-// Return list of hooks
+// Return vector of hook names. The names are not sorted - it is up to the
+// caller to perform sorting if required.
vector<string>
ServerHooks::getHookNames() const {
diff --git a/src/lib/util/hooks/server_hooks.h b/src/lib/util/hooks/server_hooks.h
index 7aa5acb..42bbb35 100644
--- a/src/lib/util/hooks/server_hooks.h
+++ b/src/lib/util/hooks/server_hooks.h
@@ -35,7 +35,7 @@ public:
};
-/// @brief Server Hook List
+/// @brief Server hook collection
///
/// This class is used by the server-side code to register hooks - points at
/// in the server processing at which libraries can register functions
@@ -45,17 +45,20 @@ public:
/// The ServerHooks class is little more than a wrapper around the std::map
/// class. It stores a hook, assigning to it a unique index number. This
/// number is then used by the server code to identify the hook being called.
+/// (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.)
class ServerHooks {
public:
- /// Pre-Defined Hooks
+ /// Index numbers for pre-defined hooks.
static const int CONTEXT_CREATE = 0;
static const int CONTEXT_DESTROY = 1;
/// @brief Constructor
///
- /// This pre-registers two hooks, context_create and context_destroy. These
+ /// 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.
@@ -70,8 +73,9 @@ public:
///
/// @param name Name of the hook
///
- /// @return Index of the hook, to be used in subsequent calls. This will
- /// be greater than or equal to zero.
+ /// @return Index of the hook, to be used in subsequent hook-related calls.
+ /// This will be greater than or equal to zero (so allowing a
+ /// negative value to indicate an invalid index).
///
/// @throws DuplicateHook A hook with the same name has already been
/// registered.
diff --git a/src/lib/util/tests/callout_handle_unittest.cc b/src/lib/util/tests/callout_handle_unittest.cc
index 0a8139b..705c9d7 100644
--- a/src/lib/util/tests/callout_handle_unittest.cc
+++ b/src/lib/util/tests/callout_handle_unittest.cc
@@ -27,6 +27,7 @@ namespace {
class CalloutHandleTest : public ::testing::Test {
public:
+
/// @brief Constructor
///
/// Sets up an appropriate number of server hooks to pass to the
@@ -68,22 +69,21 @@ TEST_F(CalloutHandleTest, ArgumentDistinctSimpleType) {
handle.setArgument("integer2", c);
EXPECT_EQ(142, c);
- int d = -1;
+ int d = 0;
handle.getArgument("integer2", d);
EXPECT_EQ(142, d);
// Add a short (random value).
- short e = 81;
+ short e = -81;
handle.setArgument("short", e);
- EXPECT_EQ(81, e);
+ EXPECT_EQ(-81, e);
- short f = -1;
+ short f = 0;
handle.getArgument("short", f);
- EXPECT_EQ(81, f);
+ EXPECT_EQ(-81, f);
}
-// Test that trying to get something with an incorrect name throws an
-// exception.
+// Test that trying to get an unknown argument throws an exception.
TEST_F(CalloutHandleTest, ArgumentUnknownName) {
CalloutHandle handle(getLibraryHandleCollection());
@@ -99,11 +99,12 @@ TEST_F(CalloutHandleTest, ArgumentUnknownName) {
EXPECT_EQ(42, b);
// Check that getting an unknown name throws an exception.
- int c = -1;
+ int c = 0;
EXPECT_THROW(handle.getArgument("unknown", c), NoSuchArgument);
}
-// Test that trying to get something with an incorrect type throws an exception.
+// Test that trying to get an argument with an incorrect type throws an
+// exception.
TEST_F(CalloutHandleTest, ArgumentIncorrectType) {
CalloutHandle handle(getLibraryHandleCollection());
@@ -150,7 +151,7 @@ TEST_F(CalloutHandleTest, ComplexTypes) {
EXPECT_EQ(22, beth.d);
handle.setArgument("beth", beth);
- // Ensure we can extract the data correctly
+ // Ensure we can extract the data correctly.
Alpha aleph2;
EXPECT_EQ(0, aleph2.a);
EXPECT_EQ(0, aleph2.b);
@@ -183,7 +184,7 @@ TEST_F(CalloutHandleTest, PointerTypes) {
Alpha* pa = ℵ
const Beta* pcb = ℶ
- // Check pointers can be set and retrieved OK
+ // Check pointers can be set and retrieved OK.
handle.setArgument("non_const_pointer", pa);
handle.setArgument("const_pointer", pcb);
@@ -229,7 +230,7 @@ TEST_F(CalloutHandleTest, ContextItemNames) {
EXPECT_TRUE(expected_names == actual_names);
}
-// Test that we can delete and argument.
+// Test that we can delete an argument.
TEST_F(CalloutHandleTest, DeleteArgument) {
CalloutHandle handle(getLibraryHandleCollection());
@@ -245,7 +246,7 @@ TEST_F(CalloutHandleTest, DeleteArgument) {
handle.setArgument("three", three);
handle.setArgument("four", four);
- // Delete "one"
+ // Delete "one".
handle.getArgument("one", value);
EXPECT_EQ(1, value);
handle.deleteArgument("one");
@@ -271,7 +272,7 @@ TEST_F(CalloutHandleTest, DeleteArgument) {
EXPECT_EQ(4, value);
}
-// Test that we can delete all arguments
+// Test that we can delete all arguments.
TEST_F(CalloutHandleTest, DeleteAllArguments) {
CalloutHandle handle(getLibraryHandleCollection());
diff --git a/src/lib/util/tests/handles_unittest.cc b/src/lib/util/tests/handles_unittest.cc
index 94ac627..4759824 100644
--- a/src/lib/util/tests/handles_unittest.cc
+++ b/src/lib/util/tests/handles_unittest.cc
@@ -48,7 +48,7 @@ namespace {
// 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
+// which they were called - can be determined. The functions also check that
// the "callout context" and "library context" data areas are separate.
//
// Three libraries are assumed, and each supplies four callouts. All callouts
@@ -57,9 +57,9 @@ namespace {
// the type of data manipulated).
//
// For the string item, each callout shifts data to the left and inserts its own
-// data. Data is a string of the form "nmwc", where "n" is the number of the
-// library, "m" is the callout number and "w" is an indication of what is being
-// altered (library context ["x"] or callout context ["c"]) and "y" is the
+// data. The aata is a string of the form "nmwc", where "n" is the number of
+// the library, "m" is the callout number and "w" is an indication of what is
+// being altered (library context ["x"] or callout context ["c"]) and "y" is the
// indication of what callout was passed as an argument ("x" or "b"). ("x" is
// used instead of "l" to indicate that library context is being altered since
// in the results, these single characters will be mixed with digits and "l"
@@ -78,16 +78,17 @@ namespace {
// use of a name the same as that of one of the context elements serves as a
// check that the argument name space and argument context space are separate.
//
-// For integer data, the value starts at zero and an increment added on each
+// For integer data, the value starts at zero and an increment is added on each
// call. This increment is equal to:
//
// 1000 * library number + 100 * callout_number + 10 * lib/callout + indicator
//
-// where "lib/callout" is 1 for updating a library context and 2 for updating
-// a callout context, and "indicator" is 1 for callout a and 2 for callout b.
-// This gives a direct correspondence between the characters appended to the
-// string context item and the amount by which the integer context item is
-// incremented. For example, the string "21cb" corresponds to a value of 2122.
+// where "lib/callout" is 1 if a library context is updated and 2 if a
+// a callout context is changed. "indicator" is 1 for callout a and 2 for
+// callout b. This scheme gives a direct correspondence between the characters
+// appended to the string context item and the amount by which the integers
+// context item is incremented. For example, the string "21cb" corresponds to
+// a value of 2122.
//
// Although this gives less information than the string value, the reasons for
// using it are:
@@ -157,14 +158,14 @@ execute(CalloutHandle& callout_handle, int library_num, int callout_num) {
string string_value = "";
try {
callout_handle.getLibraryHandle().getContext("string", string_value);
- } catch (const NoSuchContext&) {
+ } catch (const NoSuchLibraryContext&) {
string_value = "";
}
int int_value = 0;
try {
callout_handle.getLibraryHandle().getContext("int", int_value);
- } catch (const NoSuchContext&) {
+ } catch (const NoSuchLibraryContext&) {
int_value = 0;
}
@@ -355,10 +356,10 @@ TEST(HandlesTest, ContextAccessCheck) {
// To explain the expected library context results:
//
// The first callCallouts() call above calls the callouts for hook "one"
- // with callout handle "a". This calls the callouts attached to hook "one"
- // from library 1, then those attached to hook "one" from library 2, then
+ // with callout handle "a". This calls the callout attached to hook "one"
+ // from library 1, then that attached to hook "one" from library 2, then
// from library 3. The callout in library 1 appends "11xa" to the first
- // library's context. The callout in library 2 appends "21xa" to that
+ // library's context. The callout in library 2 appends "21xa" to its
// library's context. Finally, the third library's context gets "31xa"
// appended to it.
//
@@ -366,11 +367,11 @@ TEST(HandlesTest, ContextAccessCheck) {
// to hook "one", which result in "11xb", "21xb", "31xb" being appended to
// the context of libraries 1, 2, and 3 respectively.
//
- // The expected integer values can be found by summing up the values
- // corresponding to the elements of the strings.
- //
// The process is then repeated for hooks "two" and "three", leading to
// the expected context values listed below.
+ //
+ // The expected integer values can be found by summing up the values
+ // corresponding to the elements of the strings.
EXPECT_EQ("11xa11xb12xa12xb13xa13xb", resultLibraryString(0));
EXPECT_EQ("21xa21xb22xa22xb23xa23xb", resultLibraryString(1));
@@ -397,6 +398,9 @@ TEST(HandlesTest, ContextAccessCheck) {
// "12ca", "22ca" and "32ca" for hook "two" and "31ca", "32ca" and "33ca"
// for hook "three".
//
+ // The expected integer values can be found by summing up the values
+ // corresponding to the elements of the strings.
+
// At this point, we have only called the "print" function for callout
// handle "a", so the following results are checking the context values
// maintained in that callout handle.
@@ -554,11 +558,11 @@ TEST(HandlesTest, ContextDeletionCheck) {
collection->callCallouts(four_index, callout_handle_a);
// The logic by which the expected results are arrived at is described
- // in the ContextAccessCheck test. The results here are difference
+ // in the ContextAccessCheck test. The results here are different
// because context items have been modified along the way.
//
// As only the ContextHandle context is modified, the LibraryHandle
- // context is unaltered.
+ // context is unaltered from the values obtained in the previous test.
EXPECT_EQ("11xa11xb12xa12xb13xa13xb", resultLibraryString(0));
EXPECT_EQ("21xa21xb22xa22xb23xa23xb", resultLibraryString(1));
diff --git a/src/lib/util/tests/library_handle_collection_unittest.cc b/src/lib/util/tests/library_handle_collection_unittest.cc
index 08d31c6..953c300 100644
--- a/src/lib/util/tests/library_handle_collection_unittest.cc
+++ b/src/lib/util/tests/library_handle_collection_unittest.cc
@@ -83,7 +83,7 @@ public:
///
/// @return Reference to shared pointer pointing to the relevant handle.
///
- /// @throws InvalidIndex if the requeste dindex is not valid.
+ /// @throws InvalidIndex if the requested index is not valid.
boost::shared_ptr<LibraryHandle>& getLibraryHandle(int i) {
if ((i < 0) || (i >= handles_.size())) {
isc_throw(InvalidIndex, "handle index of " << i << " not valid for "
@@ -115,24 +115,14 @@ int LibraryHandleCollectionTest::callout_value_ = 0;
//
// The next set of tests check that callouts can be called.
-// Supply callouts structured in such a way that we can determine the order
-// that they are called and whether they are called at all. The method used
-// is simple - after a sequence of callouts, the digits in the value, reading
-// left to right, determines the order of the callouts and whether they were
-// called at all. So:
+// The callouts defined here are structured in such a way that it is possible
+// to determine the order in which they are called and whether they are called
+// at all. The method used is simple - after a sequence of callouts, the digits
+// in the value, reading left to right, determines the order of the callouts
+// called. For example, callout one followed by two followed by three followed
+// by two followed by one results in a value of 12321.
//
-// * one followed by two, the resulting value is 12
-// * two followed by one, the resuling value is 21
-// * one and two is not called, the resulting value is 1
-// * two and one is not called, the resulting value is 2
-// * neither called, the resulting value is 0
-//
-// ... and extending beyond two callouts:
-//
-// * one followed by two followed by three followed by two followed by one
-// results in a value of 12321.
-//
-// Functions return a zero indicating success.
+// Functions return a zero to indicate success.
extern "C" {
int collection_one(CalloutHandle&) {
@@ -205,7 +195,8 @@ int collection_four_skip(CalloutHandle& handle) {
}; // extern "C"
-// Check that we know which hooks have callouts attached to them.
+// Check the "calloutsPresent()" method.
+//
// Note: as we needed to use the addHandleMethod() to set up the handles to
// which the callouts are attached, this can also be construed as a test
// of the addLibraryHandle method as well.
@@ -250,7 +241,7 @@ TEST_F(LibraryHandleCollectionTest, CalloutsPresent) {
EXPECT_FALSE(empty_collection.calloutsPresent(-1));
}
-// Test that the callouts are called in order.
+// Test that the callouts are called in the correct order.
TEST_F(LibraryHandleCollectionTest, CallCalloutsSuccess) {
const int one_index = getServerHooks()->getIndex("one");
@@ -290,8 +281,7 @@ TEST_F(LibraryHandleCollectionTest, CallCalloutsSuccess) {
EXPECT_EQ(0, status);
EXPECT_EQ(1124, callout_value_);
- // Ensure that calling the callouts on a hook with no callouts works,
- // even though it does not return any value.
+ // Ensure that calling the callouts on a hook with no callouts works.
callout_value_ = 0;
status = getLibraryHandleCollection()->callCallouts(three_index,
callout_handle);
@@ -299,8 +289,11 @@ TEST_F(LibraryHandleCollectionTest, CallCalloutsSuccess) {
EXPECT_EQ(0, callout_value_);
}
-// Test that the callouts are called in order, but not after a callout
-// returing an error code.
+// Test that the callouts are called in order, but that callouts occurring
+// after a callout that returns an error are not called.
+//
+// (Note: in this test, the callouts that return an error set the value of
+// callout_value_ before they return the error code.)
TEST_F(LibraryHandleCollectionTest, CallCalloutsError) {
const int one_index = getServerHooks()->getIndex("one");
@@ -318,7 +311,7 @@ TEST_F(LibraryHandleCollectionTest, CallCalloutsError) {
CalloutHandle callout_handle(getLibraryHandleCollection());
int status;
- // Each library contributing one callout on hook "one". First callout
+ // Each library contributing one callout on hook "one". The first callout
// returns an error.
callout_value_ = 0;
getLibraryHandle(0)->registerCallout("one", collection_one_error);
@@ -330,8 +323,8 @@ TEST_F(LibraryHandleCollectionTest, CallCalloutsError) {
EXPECT_EQ(1, status);
EXPECT_EQ(1, callout_value_);
- // Each library contributing multiple callouts on hook "two". Last callout
- // on first library returns an error after updating the value.
+ // Each library contributing multiple callouts on hook "two". The last
+ // callout on the first library returns an error.
callout_value_ = 0;
getLibraryHandle(0)->registerCallout("two", collection_one);
getLibraryHandle(0)->registerCallout("two", collection_one_error);
@@ -346,7 +339,7 @@ TEST_F(LibraryHandleCollectionTest, CallCalloutsError) {
EXPECT_EQ(1, status);
EXPECT_EQ(11, callout_value_);
- // Random callout returns an error.
+ // A callout in a random position in the callout list returns an error.
callout_value_ = 0;
getLibraryHandle(0)->registerCallout("three", collection_one);
getLibraryHandle(0)->registerCallout("three", collection_one);
@@ -359,7 +352,7 @@ TEST_F(LibraryHandleCollectionTest, CallCalloutsError) {
EXPECT_EQ(1, status);
EXPECT_EQ(11224, callout_value_);
- // Last callout returns an error.
+ // The last callout on a hook returns an error.
callout_value_ = 0;
getLibraryHandle(0)->registerCallout("four", collection_one);
getLibraryHandle(0)->registerCallout("four", collection_one);
@@ -394,8 +387,8 @@ TEST_F(LibraryHandleCollectionTest, CallCalloutsSkip) {
CalloutHandle callout_handle(getLibraryHandleCollection());
int status;
- // Each library contributing one callout on hook "one". First callout
- // returns an error.
+ // Each library contributing one callout on hook "one". The first callout
+ // sets the "skip" flag.
callout_value_ = 0;
getLibraryHandle(0)->registerCallout("one", collection_one_skip);
getLibraryHandle(1)->registerCallout("one", collection_two);
@@ -406,8 +399,8 @@ TEST_F(LibraryHandleCollectionTest, CallCalloutsSkip) {
EXPECT_EQ(0, status);
EXPECT_EQ(1, callout_value_);
- // Each library contributing multiple callouts on hook "two". Last callout
- // on first library returns an error after updating the value.
+ // Each library contributing multiple callouts on hook "two". The last
+ // callout on the first library sets the "skip" flag.
callout_value_ = 0;
getLibraryHandle(0)->registerCallout("two", collection_one);
getLibraryHandle(0)->registerCallout("two", collection_one_skip);
@@ -422,7 +415,7 @@ TEST_F(LibraryHandleCollectionTest, CallCalloutsSkip) {
EXPECT_EQ(0, status);
EXPECT_EQ(11, callout_value_);
- // Random callout returns an error.
+ // A callout in a random position in the callout list sets the "skip" flag.
callout_value_ = 0;
getLibraryHandle(0)->registerCallout("three", collection_one);
getLibraryHandle(0)->registerCallout("three", collection_one);
@@ -435,7 +428,7 @@ TEST_F(LibraryHandleCollectionTest, CallCalloutsSkip) {
EXPECT_EQ(0, status);
EXPECT_EQ(11224, callout_value_);
- // Last callout returns an error.
+ // The last callout on a hook sets the "skip" flag.
callout_value_ = 0;
getLibraryHandle(0)->registerCallout("four", collection_one);
getLibraryHandle(0)->registerCallout("four", collection_one);
diff --git a/src/lib/util/tests/library_handle_unittest.cc b/src/lib/util/tests/library_handle_unittest.cc
index ede31b1..912c244 100644
--- a/src/lib/util/tests/library_handle_unittest.cc
+++ b/src/lib/util/tests/library_handle_unittest.cc
@@ -40,23 +40,24 @@ public:
hooks_->registerHook("beta");
hooks_->registerHook("gamma");
- // Also initialize the callout variables.
+ // Also initialize the variable used to pass information back from the
+ // callouts to the tests.
callout_value = 0;
}
- /// Obtain constructed server hooks
+ /// Obtain constructed server hooks.
boost::shared_ptr<ServerHooks>& getServerHooks() {
return (hooks_);
}
- // Obtain constructed hook manager
+ /// Obtain constructed hook manager.
boost::shared_ptr<LibraryHandleCollection>& getLibraryHandleCollection() {
return (collection_);
}
/// Variable for callouts test. This is public and static to allow non-
- /// member functions to access it. It is initialized every time a
- /// new test starts.
+ /// member functions to access it. It is initialized every time a new
+ /// test starts.
static int callout_value;
private:
@@ -72,8 +73,8 @@ int LibraryHandleTest::callout_value = 0;
// The first set of tests check that the LibraryHandle can store and retrieve
// context.
-// Test that we can store multiple values of the same type and that they
-// are distinct.
+// Test that we can store multiple values of the same type and that they are
+// distinct.
TEST_F(LibraryHandleTest, ContextDistinctSimpleType) {
LibraryHandle handle(getServerHooks());
@@ -92,18 +93,18 @@ TEST_F(LibraryHandleTest, ContextDistinctSimpleType) {
handle.setContext("integer2", c);
EXPECT_EQ(142, c);
- int d = -1;
+ int d = 0;
handle.getContext("integer2", d);
EXPECT_EQ(142, d);
// Add a short (random value).
- short e = 81;
+ short e = -81;
handle.setContext("short", e);
- EXPECT_EQ(81, e);
+ EXPECT_EQ(-81, e);
- short f = -1;
+ short f = 0;
handle.getContext("short", f);
- EXPECT_EQ(81, f);
+ EXPECT_EQ(-81, f);
}
// Test that trying to get something with an incorrect name throws an
@@ -124,7 +125,7 @@ TEST_F(LibraryHandleTest, ContextUnknownName) {
// Check that getting an unknown name throws an exception.
int c = -1;
- EXPECT_THROW(handle.getContext("unknown", c), NoSuchContext);
+ EXPECT_THROW(handle.getContext("unknown", c), NoSuchLibraryContext);
}
// Test that trying to get something with an incorrect type throws an exception.
@@ -137,7 +138,7 @@ TEST_F(LibraryHandleTest, ContextIncorrectType) {
handle.setContext("integer1", a);
EXPECT_EQ(42, a);
- // Check we can retrieve it
+ // Check we can't retrieve it using a variable of the wrong type.
long b = 0;
EXPECT_THROW(handle.getContext("integer1", b), boost::bad_any_cast);
}
@@ -194,7 +195,7 @@ TEST_F(LibraryHandleTest, ComplexTypes) {
EXPECT_THROW(handle.getContext("aleph", beth), boost::bad_any_cast);
}
-// Check that the context can store pointers. And also check that it respects
+// Check that the context can store pointers. Also check that it respects
// that a "pointer to X" is not the same as a "pointer to const X".
TEST_F(LibraryHandleTest, PointerTypes) {
@@ -261,18 +262,18 @@ TEST_F(LibraryHandleTest, DeleteContext) {
int value = 42;
handle.setContext("faith", value++);
handle.setContext("hope", value++);
- value = 0;
// Delete "faith" and verify that getting it throws an exception
handle.deleteContext("faith");
- EXPECT_THROW(handle.getContext("faith", value), NoSuchContext);
+ EXPECT_THROW(handle.getContext("faith", value), NoSuchLibraryContext);
// Check that the other item is untouched.
+ value = 0;
EXPECT_NO_THROW(handle.getContext("hope", value));
EXPECT_EQ(43, value);
}
-// Delete all all items of context.
+// Check we can delete all all items of context.
TEST_F(LibraryHandleTest, DeleteAllContext) {
LibraryHandle handle(getServerHooks());
@@ -281,13 +282,14 @@ TEST_F(LibraryHandleTest, DeleteAllContext) {
handle.setContext("faith", value++);
handle.setContext("hope", value++);
handle.setContext("charity", value++);
- value = 0;
// Delete all items of context and verify that they are gone.
handle.deleteAllContext();
- EXPECT_THROW(handle.getContext("faith", value), NoSuchContext);
- EXPECT_THROW(handle.getContext("hope", value), NoSuchContext);
- EXPECT_THROW(handle.getContext("charity", value), NoSuchContext);
+
+ value = 0;
+ EXPECT_THROW(handle.getContext("faith", value), NoSuchLibraryContext);
+ EXPECT_THROW(handle.getContext("hope", value), NoSuchLibraryContext);
+ EXPECT_THROW(handle.getContext("charity", value), NoSuchLibraryContext);
}
@@ -296,24 +298,14 @@ TEST_F(LibraryHandleTest, DeleteAllContext) {
//
// The next set of tests check that callouts can be registered.
-// Supply callouts structured in such a way that we can determine the order
-// that they are called and whether they are called at all. The method used
-// is simple - after a sequence of callouts, the digits in the value, reading
-// left to right, determines the order of the callouts and whether they were
-// called at all. So:
+// The callouts defined here are structured in such a way that it is possible
+// to determine the order in which they are called and whether they are called
+// at all. The method used is simple - after a sequence of callouts, the digits
+// in the value, reading left to right, determines the order of the callouts
+// called. For example, callout one followed by two followed by three followed
+// by two followed by one results in a value of 12321.
//
-// * one followed by two, the resulting value is 12
-// * two followed by one, the resuling value is 21
-// * one and two is not called, the resulting value is 1
-// * two and one is not called, the resulting value is 2
-// * neither called, the resulting value is 0
-//
-// ... and extending beyond two callouts:
-//
-// * one followed by two followed by three followed by two followed by one
-// results in a value of 12321.
-//
-// Functions return a zero indicating success.
+// Functions return a zero to indicate success.
extern "C" {
int one(CalloutHandle&) {
@@ -337,7 +329,7 @@ int three(CalloutHandle&) {
// The next function is a duplicate of "one", but returns an error status.
int one_error(CalloutHandle& handle) {
- (void) one(handle);
+ static_cast<void>(one(handle));
return (1);
}
@@ -366,9 +358,9 @@ TEST_F(LibraryHandleTest, RegisterSingleCallout) {
EXPECT_TRUE(handle.calloutsPresent(getServerHooks()->getIndex("beta")));
}
-// Check that we can call a single callout on a particular hook. Refer
-// to the above definition of the callouts "one" and "two" to understand
-// the expected return values.
+// Check that we can call a single callout on a particular hook. Refer to the
+// above definition of the callouts "one" and "two" to understand the expected
+// return values.
TEST_F(LibraryHandleTest, CallSingleCallout) {
LibraryHandle handle(getServerHooks());
@@ -396,11 +388,11 @@ TEST_F(LibraryHandleTest, CallSingleCallout) {
TEST_F(LibraryHandleTest, TwoCallouts) {
LibraryHandle handle(getServerHooks());
- // Register two callouts for hook alpha...
+ // Register two callouts for hook alpha.
handle.registerCallout("alpha", one);
handle.registerCallout("alpha", two);
- // ... and call them.
+ // Call them.
EXPECT_EQ(0, LibraryHandleTest::callout_value);
int index = getServerHooks()->getIndex("alpha");
@@ -438,7 +430,7 @@ TEST_F(LibraryHandleTest, TwoCalloutsWithError) {
TEST_F(LibraryHandleTest, TwoCalloutsWithSkip) {
LibraryHandle handle(getServerHooks());
- // Register callout for hook alpha...
+ // Register callout for hook alpha.
handle.registerCallout("alpha", one_skip);
handle.registerCallout("alpha", two);
@@ -458,7 +450,7 @@ TEST_F(LibraryHandleTest, TwoCalloutsWithSkip) {
TEST_F(LibraryHandleTest, MultipleRegistration) {
LibraryHandle handle(getServerHooks());
- // Register callouts for hook alpha...
+ // Register callouts for hook alpha.
handle.registerCallout("alpha", one);
handle.registerCallout("alpha", two);
handle.registerCallout("alpha", one);
diff --git a/src/lib/util/tests/server_hooks_unittest.cc b/src/lib/util/tests/server_hooks_unittest.cc
index dd9ea1b..478c28f 100644
--- a/src/lib/util/tests/server_hooks_unittest.cc
+++ b/src/lib/util/tests/server_hooks_unittest.cc
@@ -24,19 +24,28 @@ using namespace isc;
using namespace isc::util;
using namespace std;
-// Checks the registration of hooks and the interrogation methods. As the
-// constructor registers two hooks, this is also a test of the construction.
-
namespace {
+// Checks the registration of hooks and the interrogation methods. As the
+// constructor registers two hooks, this is also a test of the constructor.
+
TEST(ServerHooksTest, RegisterHooks) {
ServerHooks hooks;
- // Should be two hooks already registered, with indexes 0 and 1.
+ // There should be two hooks already registered, with indexes 0 and 1.
EXPECT_EQ(2, hooks.getCount());
EXPECT_EQ(0, hooks.getIndex("context_create"));
EXPECT_EQ(1, hooks.getIndex("context_destroy"));
+ // Check that the constants are as expected. (The intermediate variables
+ // are used because of problems with g++ 4.6.1/Ubuntu 11.10 when resolving
+ // the value of the ServerHooks constants when they appeared within the
+ // gtest macro.)
+ const int create_value = ServerHooks::CONTEXT_CREATE;
+ const int destroy_value = ServerHooks::CONTEXT_DESTROY;
+ EXPECT_EQ(0, create_value);
+ EXPECT_EQ(1, destroy_value);
+
// Register another couple of hooks. The test on returned index is based
// on knowledge that the hook indexes are assigned in ascending order.
int alpha = hooks.registerHook("alpha");
@@ -51,26 +60,27 @@ TEST(ServerHooksTest, RegisterHooks) {
EXPECT_EQ(4, hooks.getCount());
}
-// Checks that duplcate names cannot be registered.
+// Check that duplcate names cannot be registered.
TEST(ServerHooksTest, DuplicateHooks) {
ServerHooks hooks;
- // Ensure we can duplicate one of the existing names.
+ // Ensure we can't duplicate one of the existing names.
EXPECT_THROW(hooks.registerHook("context_create"), DuplicateHook);
+ // Check we can't duplicate a newly registered hook.
int gamma = hooks.registerHook("gamma");
EXPECT_EQ(2, gamma);
EXPECT_THROW(hooks.registerHook("gamma"), DuplicateHook);
}
-// Checks that we can get the name of the hooks
+// Checks that we can get the name of the hooks.
TEST(ServerHooksTest, GetHookNames) {
vector<string> expected_names;
ServerHooks hooks;
- // Insert the names into the hooks object
+ // Add names into the hooks object and to the set of expected names.
expected_names.push_back("alpha");
expected_names.push_back("beta");
expected_names.push_back("gamma");
@@ -86,7 +96,7 @@ TEST(ServerHooksTest, GetHookNames) {
// Get the actual hook names
vector<string> actual_names = hooks.getHookNames();
- // For comparison, sort the name sinto alphabetical order and do a straight
+ // For comparison, sort the names into alphabetical order and do a straight
// vector comparison.
sort(expected_names.begin(), expected_names.end());
sort(actual_names.begin(), actual_names.end());
@@ -101,7 +111,7 @@ TEST(ServerHooksTest, HookCount) {
// Insert the names into the hooks object
hooks.registerHook("alpha");
- hooks.registerHook("betha");
+ hooks.registerHook("beta");
hooks.registerHook("gamma");
hooks.registerHook("delta");
More information about the bind10-changes
mailing list