From: Stephen Morris Date: Mon, 10 Jun 2013 18:27:33 +0000 (+0100) Subject: [2974] Final check of documentation for the hooks data structures X-Git-Tag: bind10-1.2.0beta1-release~402^2~2 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=0df2936c8f985071e97fb17d9685266fc0d443a5;p=thirdparty%2Fkea.git [2974] Final check of documentation for the hooks data structures --- diff --git a/src/lib/util/hooks/callout_handle.h b/src/lib/util/hooks/callout_handle.h index e65e0d2057..ad3ef673b6 100644 --- a/src/lib/util/hooks/callout_handle.h +++ b/src/lib/util/hooks/callout_handle.h @@ -109,6 +109,14 @@ class LibraryHandle; class CalloutHandle { public: + /// Callout success return status - the next callout in the list for the + /// hook will be called. + static const int SUCCESS = 0; + + /// Callout complete return status - the callout has succeeded, but + /// remaining callouts on this hook (including any from other libraries) + /// should not be run. + static const int COMPLETE = 1; /// Typedef to allow abbreviation of iterator specification in methods. /// The std::string is the argument name and the "boost::any" is the @@ -118,8 +126,8 @@ public: /// Typedef to allow abbreviations in specifications when accessing /// context. The ElementCollection is the name/value collection for /// a particular context. The "int" corresponds to the index of an - /// associated library handle - there is a 1:1 correspondence between - /// library handles and a name.value collection. + /// associated library - there is a 1:1 correspondence between libraries + /// and a name.value collection. /// /// The collection of contexts is stored in a map, as not every library /// will require creation of a context associated with each packet. In @@ -147,7 +155,7 @@ public: /// already exist. /// /// @param name Name of the argument. - /// @param value Value to set + /// @param value Value to set. That can be of any data type. template void setArgument(const std::string& name, T value) { arguments_[name] = value; @@ -181,7 +189,7 @@ public: /// Returns a vector holding the names of arguments in the argument /// vector. /// - /// @return Vector of strings reflecting argument names + /// @return Vector of strings reflecting argument names. std::vector getArgumentNames() const; /// @brief Delete argument @@ -232,15 +240,14 @@ public: /// Returns a reference to the current library handle. This function is /// only available when called by a callout (which in turn is called /// through the "callCallouts" method), as it is only then that the current - /// library index is valid. A callout would use this method to get to - /// the context associated with the library in which it resides. + /// library index is valid. A callout uses the library handle to + /// dynamically register or deregister callouts. /// - /// @return Reference to the current library handle. + /// @return Reference to the library handle. /// - /// @throw InvalidIndex thrown if this method is called outside of the - /// "callCallouts() method. (Exception is so-named because the - /// index used to access the library handle in the collection - /// is not valid at that point.) + /// @throw InvalidIndex thrown if this method is called when the current + /// library index is invalid (typically if it is called outside of + /// the active callout). LibraryHandle& getLibraryHandle() const; /// @brief Set context @@ -276,7 +283,7 @@ public: if (element_ptr == lib_context.end()) { isc_throw(NoSuchCalloutContext, "unable to find callout context " "item " << name << " in the context associated with " - "current library handle"); + "current library"); } value = boost::any_cast(element_ptr->second); diff --git a/src/lib/util/hooks/callout_manager.cc b/src/lib/util/hooks/callout_manager.cc index 7ab6131e7d..32afc9fec1 100644 --- a/src/lib/util/hooks/callout_manager.cc +++ b/src/lib/util/hooks/callout_manager.cc @@ -15,6 +15,8 @@ #include #include +#include + #include #include #include @@ -25,7 +27,7 @@ using namespace isc::util; namespace isc { namespace util { -// Register a callout for a particular library. +// Register a callout for the current library. void CalloutManager::registerCallout(const std::string& name, CalloutPtr callout) { @@ -42,17 +44,17 @@ CalloutManager::registerCallout(const std::string& name, CalloutPtr callout) { for (CalloutVector::iterator i = hook_vector_[hook_index].begin(); i != hook_vector_[hook_index].end(); ++i) { if (i->first > current_library_) { - // Found an element whose library number is greater than ours, - // so insert the new element ahead of this one. + // Found an element whose library index number is greater than the + // current index, so insert the new element ahead of this one. hook_vector_[hook_index].insert(i, make_pair(current_library_, callout)); return; } } - // Reach the end of the vector, so no element in the (possibly empty) - // set of callouts with a library index greater that the one related to - // this callout, insert at the end. + // Reached the end of the vector, so there is no element in the (possibly + // empty) set of callouts with a library index greater than the current + // library index. Inset the callout at the end of the list. hook_vector_[hook_index].push_back(make_pair(current_library_, callout)); } @@ -74,8 +76,7 @@ CalloutManager::callCallouts(int hook_index, CalloutHandle& callout_handle) { // Validate the hook index. checkHookIndex(hook_index); - // Clear the "skip" flag so we don't carry state from a previous - // call. + // Clear the "skip" flag so we don't carry state from a previous call. callout_handle.setSkip(false); // Duplicate the callout vector for this hook and work through that. @@ -85,14 +86,13 @@ CalloutManager::callCallouts(int hook_index, CalloutHandle& callout_handle) { // potentially affect the iteration through that vector. CalloutVector callouts(hook_vector_[hook_index]); - // Call all the callouts, stopping if a non-zero status is returned. - int status = 0; - + // Call all the callouts, stopping if a non SUCCESS status is returned. + int status = CalloutHandle::SUCCESS; for (CalloutVector::const_iterator i = callouts.begin(); - i != callouts.end() && (status == 0); ++i) { + i != callouts.end() && (status == CalloutHandle::SUCCESS); ++i) { // In case the callout tries to register or deregister a callout, set - // the current library index to the index associated with the callout - // being called. + // the current library index to the index associated with library + // that registered the callout being called. current_library_ = i->first; // Call the callout @@ -107,7 +107,7 @@ CalloutManager::callCallouts(int hook_index, CalloutHandle& callout_handle) { return (status); } -// Deregister a callout registered by a library on a particular hook. +// Deregister a callout registered by the current library on a particular hook. bool CalloutManager::deregisterCallout(const std::string& name, CalloutPtr callout) { diff --git a/src/lib/util/hooks/callout_manager.h b/src/lib/util/hooks/callout_manager.h index f42901ba7e..542afd040c 100644 --- a/src/lib/util/hooks/callout_manager.h +++ b/src/lib/util/hooks/callout_manager.h @@ -43,27 +43,33 @@ public: /// This class manages the registration, deregistration and execution of the /// library callouts. /// -/// In operation, the class need to know two items of data: +/// In operation, the class needs to know two items of data: /// -/// - The list of server hooks. This is used in two ways. Firstly, when a +/// - The list of server hooks, which is used in two ways. Firstly, when a /// callout registers or deregisters a hook, it does so by name: the /// @ref isc::util::ServerHooks object supplies the names of registered /// hooks. Secondly, when the callouts associated with a hook are called by -/// the server, it supplies the index of the relevant hook: this is validated -/// hook vector (which holds the callouts associated with each hook). +/// the server, the server supplies the index of the relevant hook: this is +/// validated by reference to the list of hook. /// /// - The number of loaded libraries. Each callout registered by a user /// library is associated with that library, the callout manager storing both /// a pointer to the callout and the index of the library in the list of /// loaded libraries. Callouts are allowed to dynamically register and -/// deregister callouts (including themselves), but only callouts in the -/// same library. When calling a callout, the callout manager maintains -/// the idea of a "current library index": if the callout calls one of the -/// callout registration functions in the callout manager (it can do this -/// indirectly via the @ref LibraryHandle object), the registration functions -/// use the "current library index" in their processing. +/// deregister callouts in the same library (including themselves): they +/// cannot affect callouts registered by another library. When calling a +/// callout, the callout manager maintains the idea of a "current library +/// index": if the callout calls one of the callout registration functions +/// (indirectly via the @ref LibraryHandle object), the registration +/// functions use the "current library index" in their processing. /// -/// These two items of data are supplied when the class is constructed. +/// These two items of data are supplied when an object of this class is +/// constructed. +/// +/// Note that the callout function do not access the library manager: instead, +/// they use a LibraryHandle object. This contains an internal pointer to +/// the CalloutManager, but provides a restricted interface. In that way, +/// callouts are unable to affect callouts supplied by other libraries. class CalloutManager { private: @@ -71,10 +77,12 @@ private: // Private typedefs /// Element in the vector of callouts. The elements in the pair are the - /// library index and the pointer to the callout. + /// index of the library from which this callout was registered, and a# + /// pointer to the callout itself. typedef std::pair CalloutEntry; - /// Entry in the list of callouts for an individual hook. + /// An element in the hook vector. Each element is a vector of callouts + /// associated with a given hook. typedef std::vector CalloutVector; public: @@ -82,7 +90,7 @@ public: /// @brief Constructor /// /// Initializes member variables, in particular sizing the hook vector - /// (the vector of callouts) to the appropriate size. + /// (the vector of callout vectors) to the appropriate size. /// /// @param hooks Collection of known hook names. /// @param num_libraries Number of loaded libraries. @@ -106,7 +114,7 @@ public: hook_vector_.resize(hooks_->getCount()); } - /// @brief Register a callout on a hook + /// @brief Register a callout on a hook for the current library /// /// Registers a callout function for the current library with a given hook /// (the index of the "current library" being given by the current_library_ @@ -121,7 +129,7 @@ public: /// is of the wrong size. void registerCallout(const std::string& name, CalloutPtr callout); - /// @brief De-Register a callout on a hook + /// @brief De-Register a callout on a hook for the current library /// /// Searches through the functions registered by the the current library /// (the index of the "current library" being given by the current_library_ @@ -138,7 +146,7 @@ public: /// is of the wrong size. bool deregisterCallout(const std::string& name, CalloutPtr callout); - /// @brief Removes all callouts on a hook + /// @brief Removes all callouts on a hook for the current library /// /// Removes all callouts associated with a given hook that were registered /// by the current library (the index of the "current library" being given @@ -194,7 +202,10 @@ public: /// with the currently executing callout when callCallouts is executing. /// When callCallouts() is not executing (as is the case when the load() /// function in a user-library is called during the library load process), - /// the index is the value set by setLibraryIndex(). + /// the index can be set by setLibraryIndex(). + /// + /// @note The value set by this method is lost after a call to + /// callCallouts. /// /// @return Current library index. int getLibraryIndex() const { @@ -204,8 +215,8 @@ public: /// @brief Set current library index /// /// Sets the current library index. This must be in the range 0 to - /// (numlib - 1), where "numlib" is the number of libraries loaded and is - /// passed to this object at construction time. + /// (numlib - 1), where "numlib" is the number of libraries loaded in the + /// system, this figure being passed to this object at construction time. /// /// @param library_index New library index. /// @@ -220,9 +231,10 @@ public: /// The library handle is available to the user callout via the callout /// handle object. It provides a cut-down view of the CalloutManager, /// allowing the callout to register and deregister callouts in the - /// library of which it is part. + /// library of which it is part, whilst denying access to anything that + /// may affect other libraries. /// - /// @return reference to callout handle for this manager + /// @return Reference to callout handle for this manager LibraryHandle& getLibraryHandle() { return (library_handle_); } @@ -249,7 +261,7 @@ private: /// /// @param library_index Value to check for validity as a library index. /// - /// @throw NoSuchLibrary Library index is not + /// @throw NoSuchLibrary Library index is not valid. void checkLibraryIndex(int library_index) const { if ((library_index < 0) || (library_index >= num_libraries_)) { isc_throw(NoSuchLibrary, "library index " << library_index << @@ -271,17 +283,18 @@ private: class CalloutLibraryEqual : public std::binary_function { public: - bool operator()(const CalloutEntry& ent1, const CalloutEntry& ent2) const { + bool operator()(const CalloutEntry& ent1, + const CalloutEntry& ent2) const { return (ent1.first == ent2.first); } }; /// Current library index. When a call is made to any of the callout /// registration methods, this variable indicates the index of the user - /// library that is calling the methods. + /// library that should be associated with the call. int current_library_; - /// List of server hooks. This is used + /// List of server hooks. boost::shared_ptr hooks_; /// Vector of callout vectors. There is one entry in this outer vector for @@ -293,7 +306,7 @@ private: /// registration methods on this CalloutManager object. LibraryHandle library_handle_; - /// Number of libraries. libindex_ can vary between 0 and numlib_ - 1. + /// Number of libraries. int num_libraries_; }; diff --git a/src/lib/util/hooks/library_handle.h b/src/lib/util/hooks/library_handle.h index 82d6f57cf1..43a78a176b 100644 --- a/src/lib/util/hooks/library_handle.h +++ b/src/lib/util/hooks/library_handle.h @@ -35,27 +35,37 @@ extern "C" { /// either by the library's load() function, or by one of the callouts /// themselves. /// -/// It is really little more than a shell around the CalloutManager/ By -/// presenting this object to the user-library callouts, callouts can manage the -/// callout list for their own library, but cannot affect the callouts +/// It is really little more than a shell around the CalloutManager. By +/// presenting this object to the user-library callouts, callouts can manage +/// the callout list for their own library, but cannot affect the callouts /// registered by other libraries. +/// +/// (This restriction is achieved by the CalloutManager maintaining the concept +/// of the "current library". When a callout is registered - either by the +/// library's load() function, or by a callout in the library - the registration +/// information includes the library active at the time. When that callout is +/// called, the CalloutManager uses that information to set the "current +/// library": the registration functions only operator on data whose +/// associated library is equal to the "current library".) class LibraryHandle { public: /// @brief Constructor /// - /// @param collection Back pointer to the containing CalloutManager. + /// @param manager Back pointer to the containing CalloutManager. /// This pointer is used to access appropriate methods in that - /// object. - LibraryHandle(CalloutManager* collection) : callout_manager_(collection) + /// object. Note that the "raw" pointer is safe - the only + /// instance of the LibraryHandle in the system is as a member of + /// the CalloutManager to which it points. + LibraryHandle(CalloutManager* manager) : callout_manager_(manager) {} /// @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 for this library that are associated with - /// that hook. + /// to the end of the callouts for the current library that are associated + /// with that hook. /// /// @param name Name of the hook to which the callout is added. /// @param callout Pointer to the callout function to be registered. @@ -67,9 +77,9 @@ public: /// @brief De-Register a callout on a hook /// - /// Searches through the functions registered by this library with the named - /// hook and removes all entries matching the callout. It does not affect - /// callouts registered by other libraries. + /// Searches through the functions registered by the current library with + /// the named hook and removes all entries matching the callout. It does + /// not affect callouts registered by other libraries. /// /// @param name Name of the hook from which the callout is removed. /// @param callout Pointer to the callout function to be removed. @@ -84,8 +94,8 @@ public: /// @brief Removes all callouts on a hook /// /// Removes all callouts associated with a given hook that were registered. - /// by this library. It does not affect callouts that were registered by - /// other libraries. + /// by the current library. It does not affect callouts that were + /// registered by other libraries. /// /// @param name Name of the hook from which the callouts are removed. /// diff --git a/src/lib/util/hooks/server_hooks.cc b/src/lib/util/hooks/server_hooks.cc index 9bb903b9a1..478d94cb1c 100644 --- a/src/lib/util/hooks/server_hooks.cc +++ b/src/lib/util/hooks/server_hooks.cc @@ -42,7 +42,7 @@ ServerHooks::ServerHooks() { // Register a hook. The index assigned to the hook is the current number // of entries in the collection, so ensuring that hook indexes are unique -// (and non-negative). +// and non-negative. int ServerHooks::registerHook(const string& name) { @@ -63,7 +63,7 @@ ServerHooks::registerHook(const string& name) { return (index); } -// Find the index associated with a hook name or return -1 if not found +// Find the index associated with a hook name. int ServerHooks::getIndex(const string& name) const { @@ -94,13 +94,6 @@ ServerHooks::getHookNames() const { // Hook registration function methods -// Constructor - add a registration function to the function vector - -HookRegistrationFunction::HookRegistrationFunction( - HookRegistrationFunction::RegistrationFunctionPtr reg_func) { - getFunctionVector().push_back(reg_func); -} - // Access the hook registration function vector itself std::vector& @@ -109,6 +102,13 @@ HookRegistrationFunction::getFunctionVector() { return (reg_functions); } +// Constructor - add a registration function to the function vector + +HookRegistrationFunction::HookRegistrationFunction( + HookRegistrationFunction::RegistrationFunctionPtr reg_func) { + getFunctionVector().push_back(reg_func); +} + // Execute all registered registration functions void diff --git a/src/lib/util/hooks/server_hooks.h b/src/lib/util/hooks/server_hooks.h index ca978f560d..ce41fa0d54 100644 --- a/src/lib/util/hooks/server_hooks.h +++ b/src/lib/util/hooks/server_hooks.h @@ -128,12 +128,12 @@ private: /// /// All hooks must be registered before libraries are loaded and callouts /// assigned to them. One way of doing this is to have a global list of hooks: -/// the addition of any hook anywhere would require updating the list. The -/// other way, chosen here, is to have each component in BIND 10 register the -/// hooks they are using. +/// the addition of any hook anywhere would require updating the list. This +/// is possible and, if desired, the author of a server can do it. /// -/// The chosen method requires that each component create a hook registration -/// function of the form: +/// An alternative is the option provided here, where each component of BIND 10 +/// registers the hooks they are using. To do this, the component should +/// create a hook registration function of the form: /// /// @code /// static int hook1_num = -1; // Initialize number for hook 1 @@ -145,21 +145,21 @@ private: /// } /// @endcode /// -/// The server then calls each of these hook registration functions during its -/// initialization before loading the libraries. -/// -/// It is to avoid the need to add an explicit call to each of the hook -/// registration functions to the server initialization code that this class -/// has been created. Declaring an object of this class in the same file as -/// the registration function and passing the registration function as an -/// argument, e.g. +/// ... which registers the hooks and stores the associated hook index. To +/// avoid the need to add an explicit call to each of the hook registration +/// functions to the server initialization code, the component should declare +/// an object of this class in the same file as the registration function, +/// but outside of any function. The declaration should include the name +/// of the registration function, i.e. /// /// @code /// HookRegistrationFunction f(myModuleRegisterHooks); /// @code /// -/// is sufficient to add the registration function to a list of such functions. -/// The server will execute all functions in the list wh3en it starts. +/// The constructor of this object will run prior to main() getting called and +/// will add the registration function to a list of such functions. The server +/// then calls the static class method "execute()" to run all the declared +/// registration functions. class HookRegistrationFunction { public: @@ -177,14 +177,14 @@ public: /// /// One of the problems with functions run prior to starting main() is the /// "static initialization fiasco". This occurs because the order in which - /// objects outside functions is not defined. So if this constructor were - /// to depend on a vector declared externally, we would not be able to - /// guarantee that the vector had been initialised proerly before we used - /// it. + /// objects outside functions are constructed is not defined. So if this + /// constructor were to depend on a vector declared externally, we would + /// not be able to guarantee that the vector had been initialised properly + /// before we used it. /// /// To get round this situation, the vector is declared statically within - /// a function. The first time the function is called, the object is - /// initialized. + /// a static function. The first time the function is called, the vector + /// is initialized before it is used. /// /// This function returns a reference to the vector used to hold the /// pointers. @@ -200,9 +200,6 @@ public: /// @param hooks ServerHooks object to which hook information will be added. static void execute(ServerHooks& hooks); }; -/// to the constructor -/// any function -/// } // namespace util } // namespace isc diff --git a/src/lib/util/tests/callout_handle_unittest.cc b/src/lib/util/tests/callout_handle_unittest.cc index 805db1c67e..635e570db1 100644 --- a/src/lib/util/tests/callout_handle_unittest.cc +++ b/src/lib/util/tests/callout_handle_unittest.cc @@ -26,6 +26,12 @@ using namespace std; namespace { +/// @file +/// @brief Holds the CalloutHandle argument tests +/// +/// Additional testing of the CalloutHandle - together with the interaction +/// of the LibraryHandle - is done in the handles_unittests set of tests. + class CalloutHandleTest : public ::testing::Test { public: diff --git a/src/lib/util/tests/callout_manager_unittest.cc b/src/lib/util/tests/callout_manager_unittest.cc index ebd4773d9f..8f35be3aff 100644 --- a/src/lib/util/tests/callout_manager_unittest.cc +++ b/src/lib/util/tests/callout_manager_unittest.cc @@ -98,10 +98,8 @@ private: // Definition of the static variable. int CalloutManagerTest::callout_value_ = 0; -// *** Callout Tests *** +// Callout definitions // -// The next set of tests check that callouts can be called. - // 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 @@ -170,6 +168,10 @@ int manager_four_error(CalloutHandle& handle) { }; // extern "C" +// *** Callout Tests *** +// +// The next set of tests check that callouts can be called. + // Constructor - check that we trap bad parameters. TEST_F(CalloutManagerTest, BadConstructorParameters) { @@ -270,6 +272,8 @@ TEST_F(CalloutManagerTest, RegisterCallout) { EXPECT_EQ(0, status); EXPECT_EQ(1345, callout_value_); + // ... and check the additional callouts were not registered on the "beta" + // hook. callout_value_ = 0; status = getCalloutManager()->callCallouts(beta_index_, getCalloutHandle()); EXPECT_EQ(0, status); @@ -310,7 +314,6 @@ TEST_F(CalloutManagerTest, CalloutsPresent) { // exact callouts attached to a hook are not relevant - only the fact // that some callouts are). Chose the libraries for which the callouts // are registered randomly. - getCalloutManager()->setLibraryIndex(0); getCalloutManager()->registerCallout("alpha", manager_one); @@ -716,7 +719,7 @@ TEST_F(CalloutManagerTest, DeregisterAllCallouts) { // More extensive tests (i.e. checking that when a callout is called it can // only register and deregister callouts within its library) require that // the CalloutHandle object pass the appropriate LibraryHandle to the -// callout. These tests are done in the CalloutHandle tests. +// callout. These tests are done in the handles_unittest tests. TEST_F(CalloutManagerTest, LibraryHandleRegistration) { // Ensure that no callouts are attached to any of the hooks. diff --git a/src/lib/util/tests/server_hooks_unittest.cc b/src/lib/util/tests/server_hooks_unittest.cc index 287c4fe76a..f029ad2852 100644 --- a/src/lib/util/tests/server_hooks_unittest.cc +++ b/src/lib/util/tests/server_hooks_unittest.cc @@ -179,14 +179,14 @@ TEST(HookRegistrationFunction, Registration) { // Should have two hook registration functions registered. EXPECT_EQ(2, HookRegistrationFunction::getFunctionVector().size()); - // Execute the functions and check that four new hooks were defined. - + // Execute the functions and check that four new hooks were defined (two + // from each function). ServerHooks hooks; EXPECT_EQ(2, hooks.getCount()); HookRegistrationFunction::execute(hooks); EXPECT_EQ(6, hooks.getCount()); - // Check the hook names + // Check the hook names are as expected. vector names = hooks.getHookNames(); ASSERT_EQ(6, names.size()); sort(names.begin(), names.end()); @@ -222,13 +222,13 @@ TEST(HookRegistrationFunction, Registration) { HookRegistrationFunction f3(registerEpsilon); EXPECT_EQ(1, HookRegistrationFunction::getFunctionVector().size()); - // Execute it and check that the hook was registered. + // Execute it... ServerHooks hooks2; EXPECT_EQ(0, epsilon); EXPECT_EQ(2, hooks2.getCount()); HookRegistrationFunction::execute(hooks2); - // Should be three hooks, with the new one assigned an index of 2. + // There should be three hooks, with the new one assigned an index of 2. names = hooks2.getHookNames(); ASSERT_EQ(3, names.size()); sort(names.begin(), names.end());