[2974] Minor tweaks and documentation changes - now ready for review

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()) {

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.

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);
     }
 

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.

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 {

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.

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());

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));

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);

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);

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");