[2980] Changes as a result for the first part of the review

src/lib/hooks/callout_handle.cc

@@ -52,12 +52,12 @@ CalloutHandle::~CalloutHandle() {
     context_collection_.clear();
 
     // Normal destruction of the remaining variables will include the
-    // destruction of lm_collection_, wn action that will decrement the
-    // reference count on the library manager collection (which holds the
-    // libraries that could have allocated memory in the argument and context
-    // members).  When that goes to zero, the libraries will be unloaded:
-    // at that point nothing in the hooks framework will be pointing to memory
-    // in the libraries' address space.
+    // destruction of lm_collection_, an action that decrements the reference
+    // count on the library manager collection (which holds the libraries that
+    // could have allocated memory in the argument and context members.)  When
+    // that goes to zero, the libraries will be unloaded: at that point nothing
+    // in the hooks framework will be pointing to memory in the libraries'
+    // address space.
     //
     // It is possible that some other data structure in the server (the program
     // using the hooks library) still references the address space and attempts

src/lib/hooks/callout_handle.h

@@ -80,11 +80,11 @@ class LibraryManagerCollection;
 ///   "context_destroy" callout.  The information is accessed through the
 ///   {get,set}Context() methods.
 ///
-/// - Per-library handle.  Allows the callout to dynamically register and
-///   deregister callouts. (In the latter case, only functions registered by
-///   functions in the same library as the callout doing the deregistration
-///   can be removed: callouts registered by other libraries cannot be
-///   modified.)
+/// - Per-library handle (LibraryHandle). The library handle allows the
+///   callout to dynamically register and deregister callouts. In the latter
+///   case, only functions registered by functions in the same library as the
+///   callout doing the deregistration can be removed: callouts registered by
+///   other libraries cannot be modified.
 
 class CalloutHandle {
 public:

src/lib/hooks/hooks_component_developer.dox

@@ -17,60 +17,65 @@
 
 @section hooksComponentIntroduction Introduction
 
-The hooks framework is a BIND 10 library that simplifies the way that
+The hooks framework is a BIND 10 system that simplifies the way that
 users can write code to modify the behavior of BIND 10.  Instead of
 altering the BIND 10 source code, they write functions that are compiled
 and linked into a shared library.  The library is specified in the BIND 10
 configuration database and run time, BIND 10 dynamically loads the library
-into its address space.  At various points in the processing, the server
+into its address space.  At various points in the processing, the component
 "calls out" to functions in the library, passing to them the data is it
 currently working on.  They can examine and modify the data as required.
 
-The document @ref hooksDevelopersGuide describes how to write a library
-that interfaces into a BIND 10 component.  This guide describes how to
-write or modify a BIND 10 component so that it can load a shared library
-and call out to functions in it.
+This guide is aimed at BIND 10 developers who want to write or modify a
+BIND 10 component to use hooks.  It shows how the component should be written
+to load a shared library at run-time and how to call functions in it.
+
+For information about writing a hooks library containing functions called by BIND 10
+during its execution, see the document @ref hooksDevelopersGuide.
 
 @subsection hooksComponentTerminology Terminology
 
 In the remainder of this guide, the following terminology is used:
 
+- Component - a BIND 10 process, e.g. the authoritative DNS server or the
+DHCPv4 server.
+
 - Hook/Hook Point - used interchageably, this is a point in the code at
 which a call to user-written functions is made. Each hook has a name and
 each hook can have any number (including 0) of user-written functions
 attached to it.
 
-- Callout - a user-written function called by the server at a hook
-point. This is so-named because the server "calls out" to the library
+- Callout - a user-written function called by the component at a hook
+point. This is so-named because the component "calls out" to the library
 to execute a user-written function.
 
-- Component - a BIND 10 process, e.g. the authoritative server or the
-DHCPv4 server.
-
 - User code/user library - non-BIND 10 code that is compiled into a
 shared library and loaded by BIND 10 into its address space.  Multiple
 user libraries can be loaded at the same time, each containing callouts for
 the same hooks.  The hooks framework calls these libraries one after the
-other. (See the document @hooksDevelopersGuide for more details.)
+other. (See the document @ref hooksDevelopersGuide for more details.)
 
 @subsection hooksComponentLanguages Languages
 
 The core of BIND 10 is written in C++ with some parts in Python.  While it is
 the intention to provide the hooks framework for all languages, the initial
-versions are for C++.  All examples in this guide are in that language.
+version is for C++.  All examples in this guide are in that language.
 
 @section hooksComponentBasicIdeas Basic Ideas
 
 From the point of view of the component author, the basic ideas of the hooks
 framework are quite simple:
 
-- The hook points need to be defined.
+- The location of hook points in the code need to be determined.
 
-- At each hook point, the component needs to:
-  - copy data into the object used to pass information to the callout.
-  - call the callout.
-  - copy data back from the object used to exchange information.
-  - take action based on information returned.
+- Name the hook points and register them.
+
+- At each hook point, the component needs to complete the following steps to
+  execute callouts registered by the user-library:
+  -# copy data into the object used to pass information to the callout.
+  -# call the callout.
+  -# copy data back from the object used to exchange information.
+  -# take action based on information returned.
 
 Of course, to set up the system the libraries need to be loaded in the first
 place.  The component also needs to:
@@ -82,59 +87,64 @@ component.
 
 The following sections will describe these tasks in more detail.
 
-@section hooksComponentDefinition Defining the Hook Points
-
-Before any other action takes place, the hook points in the code need to be
-defined.  Each hook point has a name that must be unique amongst all hook
-points for the server, and the first step is to register those names.  The
-naming is done using the static method isc::hooks::HooksManager::registerHook():
+@section hooksComponentDefinition Determing the Hook Points
+
+Before any other action takes place, the location of the hook points
+in the code need to be determined.  This of course depends on the
+component but as a general guideline, hook locations should be chosen
+where a callout is able to obtain useful information from BIND 10 and/or
+affect processing.  Typically this means at the start or end of a major
+step in the processing of a request, at a point where either useful
+information can be passed to a callout and/or the callout can affect
+the processing of the component. The latter is achieved in either or both
+of the following eays:
+
+- Setting the "skip" flag.  This is a boolean flag that the callout can set
+  and is a quick way of passing information back to the component.  It is used
+  to indicate that the component should skip the processing step associated with
+  the hook.  The exact action is up to the component, but is likely to be one
+  of skipping the processing step (probably because the callout has
+  done its own processing for the action) or dropping the current packet
+  and starting on a new request.
+
+- Modifying data passed to it.  The component should be prepared to continue
+  processing with the data returned by the callout.  It is up to the component
+  author whether the data is validated before being used, but doing so will
+  have performance implications.
+
+@section hooksComponentRegistration Naming and Registering the Hooks Points
+
+Once the location of the hook point has been determined, it should be
+given a name.  This name should be unique amongst all hook points and is
+subject to certain restrictions (see below).
+
+Before the callouts at any hook point are called and any user libraries
+loaded - so typically during component initialization - the component must
+register the names of all the hooks.  The registration is done using
+the static method isc::hooks::HooksManager::registerHook():
 
 @code
 
 #include <hooks/hooks_manager.h>
     :
-    int example_index = HooksManager::registerHook("manager");
+    int example_index = HooksManager::registerHook("lease_allocate");
 @endcode
 
-The name of the hooks is passed as the sole argument to the
-HooksManager::registerHook() method.  The value returned is the index of that
-hook point and should be retained - it is needed to call the hook.
-
-All hooks used by the component must be registered before the component
-starts operations.
-
-@subsection hooksComponentHookNames Hook Names
+The name of the hook is passed as the sole argument to the registerHook()
+method.  The value returned is the index of that hook point and should
+be retained - it is needed to call the callouts attached to that hook.
 
-Hook names are strings and in principle, any string can be used as the
-name of a hook, even one containing spaces and non-printable characters.
-However, the following guidelines should be observed:
-
-- The names <b>context_create</b> and <b>context_destroy</b> are reserved to
-the hooks system and are automatically registered: an attempt to register
-one of these will lead to a isc::hooks::DuplicateHook exception being thrown.
-
-- The hook name should be a valid function name in C.  If a user gives a
-callout the same name as one of the hooks, the hooks framework will
-automatically load that callout and attach it to the hook: the user does not
-have to explicitly register it. <b>TBD: do we still want this given
-the possibility of confusion with functions in system libraries?</b>
-
-- The hook name should not conflict with the name of a function in any of
-the system libraries (e.g. naming a hook "sqrt" could lead to the
-square-root function in the system's maths library being attached to the hook
-as a callout).
-
-- Although hook names can be in any case (including mixed case), the BIND 10
-convention is that they are lower-case.
+Note that a hook only needs to be registered once.  There is no mechanism for
+unregistering a hook and there is no need to do so.
 
 @subsection hooksComponentAutomaticRegistration Automatic Registration of Hooks
 
-In some components, it may be convenient to set up a separate
-initialization function that registers all hooks.  For others, it may
-be more convenient for each module within the component to perform its
-own initialization.  Since the HooksManager object is a singleton and
-is created when first requested, a useful trick is to automatically
-register the hooks when the module is loaded.
+In some components, it may be convenient to set up a single initialization
+function that registers all hooks.  For others, it may be more convenient
+for each module within the component to perform its own initialization.
+Since the isc::hooks::HooksManager object is a singleton and is created when first
+accessed, a useful trick is to automatically register the hooks when
+the module is loaded.
 
 This technique involves declaring an object outside of any execution
 unit in the module.  When the module is loaded, the object's constructor
@@ -151,8 +161,8 @@ namespace {
 // Declare structure to perform initialization and store the hook indexes.
 //
 struct MyHooks {
-    int pkt_rcvd;   // Packet received
-    int pkt_sent;   // Packet sent
+    int pkt_rcvd;   // Index of "packet received" hook
+    int pkt_sent;   // Index of "packet sent" hook
 
     // Constructor
     MyHooks() {
@@ -161,23 +171,47 @@ struct MyHooks {
     }
 };
 
-// Instantiate a "Hooks" object.  The constructor is run when the module is
-// loaded and so the hook indexes will be defined before any method in this
+// Declare a "MyHooks" object.  As this is outside any function or method, it
+// will be instantiated (and the constructor run) when the module is loaded.
+// As a result, the hook indexes will be defined before any method in this
 // module is called.
-Hooks hooks;
+MyHooks my_hooks;
 
 } // Anonymous namespace
 
 void Someclass::someFunction() {
     :
     // Check if any callouts are defined on the pkt_rcvd hook.
-    if (HooksManager::calloutPresent(hooks.pkt_rcvd)) {
+    if (HooksManager::calloutPresent(my_hooks.pkt_rcvd)) {
           :
     }
     :
 }
 @endcode
 
+@subsection hooksComponentHookNames Hook Names
+
+Hook names are strings and in principle, any string can be used as the
+name of a hook, even one containing spaces and non-printable characters.
+However, the following guidelines should be observed:
+
+- The names <b>context_create</b> and <b>context_destroy</b> are reserved to
+the hooks system and are automatically registered: an attempt to register
+one of these will lead to a isc::hooks::DuplicateHook exception being thrown.
+
+- The hook name should be a valid "C" function name.  If a user gives a
+callout the same name as one of the hooks, the hooks framework will
+automatically load that callout and attach it to the hook: the user does not
+have to explicitly register it.
+
+- The hook name should not conflict with the name of a function in any of
+the system libraries (e.g. naming a hook "sqrt" could lead to the
+square-root function in the system's maths library being attached to the hook
+as a callout).
+
+- Although hook names can be in any case (including mixed case), the BIND 10
+convention is that they are lower-case.
+
 @section hooksComponentCallingCallouts Calling Callouts on a Hook
 
 @subsection hooksComponentArgument The Callout Handle
@@ -187,12 +221,16 @@ how to pass data to it.
 
 Each user callout has the signature:
 @code
-int callout_name(CalloutHandle& handle);
+int callout_name(isc::hooks::CalloutHandle& handle);
 @endcode
 
 The isc::hooks::CalloutHandle object is the object used to pass data to
 and from the callout.  This holds the data as a set of name/value pairs,
-each pair being considered an argument to the callout.
+each pair being considered an argument to the callout.  If there are
+multiple callouts attached to a hook, the CalloutHandle is passed to
+each in turn. Should a callout modify an argument, the updated data is
+passed subsequent callouts (each of which could also modify it) before
+being returned to the component.
 
 Two methods are provided to get and set the arguments passed to
 the callout called (naturally enough) getArgument and SetArgument.
@@ -202,23 +240,25 @@ Their usage is illustrated by the following code snippets.
     int count = 10;
     boost::shared_ptr<Pkt4> pktptr = ... // Set to appropriate value
 
-    // Assume that "handle" has been created
-    handle.setArgument("data_count", count);
-    handle.setArgument("inpacket", pktptr);
+    // Assume that "handle_ptr" has been created and is a pointer to a
+    // CalloutHandle.
+    handle_ptr->setArgument("data_count", count);
+    handle_ptr->setArgument("inpacket", pktptr);
 
-    // Call the hook code...
-    ...
+    // Call the hook code.  lease_assigned_index is the value returned from
+    // HooksManager::registerHook() when the hook was registered.
+    HooksManager::callCallouts(lease_assigned_index, *handle_ptr);
 
     // Retrieve the modified values
-    handle.getArgument("data_count", count);
-    handle.getArgument("inpacket", pktptr);
+    handle_ptr->getArgument("data_count", count);
+    handle_ptr->getArgument("inpacket", pktptr);
 @endcode
 
 As can be seen "getArgument" is used to retrieve data from the
-isc::hooks::CalloutHandle, and setArgument used to put data into it.
-If a callout wishes to alter data and pass it back to the server,
-it should retrieve the data with getArgument, modify it, and call
-setArgument to send it back.
+CalloutHandle, and "setArgument" used to put data into it.  If a callout
+wishes to alter data and pass it back to the component, it should retrieve
+the data with getArgument, modify it, and call setArgument to send
+it back.
 
 There are a couple points to be aware of:
 
@@ -234,7 +274,7 @@ data pointed to by pointers, e.g. if an argument is defined as a "char*",
 an exception will be thrown if an attempt is made to retrieve it into
 a variable of type "const char*".  (However, if an argument is set as a
 "const int", it can be retrieved into an "int".)  The documentation of
-each hook point should detail the exact data type of each argument.
+a hook point should detail the exact data type of each argument.
 
 - If a pointer to an object is passed to a callout (either a "raw"
 pointer, or a boost smart pointer (as in the example above), and the
@@ -242,6 +282,47 @@ underlying object is altered through that pointer, the change will be
 reflected in the component even if the callout makes no call to setArgument.
 This can be avoided by passing a pointer to a "const" object.
 
+@subsection hooksComponentSkipFlag The Skip Flag
+
+Although information is passed back to the component from callouts through
+CalloutHandle arguments, a common action for callouts is to inform the component
+that its flow of control should be altered.  For example:
+
+- In the DHCP servers, there is a hook at the point at which a lease is
+  about to be assigned.  Callouts attached to this hooks may handle the
+  lease assignment in special cases, in which case they set the skip flag
+  to indicate that the server should not perform lease assignment in this
+  case.
+- A server may define a hook just after a packet is received.  A callout
+  attached to the hook might inspect the source address and compare it
+  against a blacklist.  If the address is on the list, the callout could set
+  the skip flag to indicate to the server that the packet should be dropped.
+
+For ease of processing, the CalloutHandle contains
+two methods, isc::hooks::CalloutHandle::getSkip() and
+isc::hooks::CalloutHandle::setSkip().  It is only meaningful for the
+component to use the "get" method.  The skip flag is cleared by the hooks
+framework when the component requests that callouts be executed, so any
+value set by the component is lost.  Callouts can both inspect the flag (it
+might have been set by callouts earlier in the callout list for the hook)
+and set it.  Note that the setting of the flag by a callout does not
+prevent callouts later in the list from being called: the skip flag is
+just a boolean flag - the only significance comes from its interpretation
+by the component.
+
+An example of use could be:
+@code
+// Set up arguments for DHCP lease assignment.
+handle->setArgument("query", query);
+handle->setArgument("response", response);
+HooksManager::callCallouts(lease_hook_index, *handle_ptr);
+if (! handle_ptr->getSkip()) {
+    // Skip flag not set, do the address allocation
+    :
+}
+@endcode
+
+
 @subsection hooksComponentGettingHandle Getting the Callout Handle
 
 The CalloutHandle object is linked to the loaded libraries
@@ -249,15 +330,17 @@ for lifetime reasons (described below).  Components
 should retrieve a isc::hooks::CalloutHandle using
 isc::hooks::HooksManager::createCalloutHandle():
 @code
-    CalloutHandlePtr handle = HooksManager::createCalloutHandle();
+    CalloutHandlePtr handle_ptr = HooksManager::createCalloutHandle();
 @endcode
-(isc::hooks::CalloutHandlePtr is a typedef for a boost shared pointer to a
+(isc::hooks::CalloutHandlePtr is a typedef for a Boost shared pointer to a
 CalloutHandle.)  The CalloutHandle so retrieved may be used for as
 long as the libraries are loaded.
+
+The handle is deleted by resetting the pointer:
 @code
-    handle.reset();
+    handle_ptr.reset();
 @endcode
-... or by letting the handle object go out of scope.  The actual deletion
+... or by letting the handle pointer go out of scope.  The actual deletion
 occurs when the CallHandle's reference count goes to zero. (The
 current version of the hooks framework does not maintain any other
 pointer to the returned CalloutHandle, so it gets destroyed when the
@@ -271,52 +354,25 @@ isc::hooks::HooksManager::callCallouts() method for the hook index in
 question.  For example, with the hook index pkt_sent defined as above,
 the hook can be executed by:
 @code
-    HooksManager::callCallouts(pkt_rcvd, *handle);
+    HooksManager::callCallouts(pkt_sent, *handle_ptr);
 @endcode
-... where "*handle" is a reference (note: not a pointer) to the
+... where "*handle_ptr" is a reference (note: not a pointer) to the
 isc::hooks::CalloutHandle object holding the arguments.  No status code
-is returned.  If a component needs to get data returned, it should define
-an argument through which the callout can do so.
-
-Actually, the statement "no status code is returned" is not strictly true. At
-many hooks, the following logic applies:
-@code
-call hook_code
-if (hook_code has not performed an action) {
-    perform the action
-}
-@endcode
-For example, in a DHCP server an address should be allocated for a client.
-The DHCP server does that by default, but the hook code may want to override
-it in some situations.
-
-As this logic is so common, the CalloutHandle includes a "skip" flag.  This
-is a boolean flag that can be set by the callout to pass a basic yes/no
-response to the component.  Its use is illustrated by the following code
-snippet:
-@code
-// Set up arguments for lease assignment
-handle->setArgument("query", query);
-handle->setArgument("response", response);
-HooksManager::callCallouts(lease_hook_index, *handle);
-if (! handle->getSkip()) {
-    // Skip flag not set, do the address allocation
-    :
-}
-@endcode
-<b>SHOULD WE GET RID OF THE SKIP FLAG AND WHERE APPROPRIATE, SIGNAL SUCH
-PROCESSING THROUGH AN ARGUMENT?</b>
+is returned.  If a component needs to get data returned (other than that
+provided by the "skip" flag), it should define an argument through which
+the callout can do so.
 
 @subsubsection hooksComponentConditionalCallout Conditionally Calling Hook Callouts
 
-Most hooks in a server will not have callouts attached to them. To avoid the
-overhead of setting up arguments in the CalloutHandle, a component can
-check for callouts before doing that processing.  The
-isc::hooks::HooksManager::calloutsPresent() method performs this check.
-Taking the index of a hook as its sole argument, it returns true if there
-are any callouts attached to the hook and false otherwise.
+Most hooks in a component will not have callouts attached to them. To
+avoid the overhead of setting up arguments in the CalloutHandle, a
+component can check for callouts before doing that processing using
+isc::hooks::HooksManager::calloutsPresent().  Taking the index of a
+hook as its sole argument, the function returns true if there are any
+callouts attached to the hook and false otherwise.
 
-With this check, the above example can be modified to:
+With this check, the code in the component for calling a hook would look
+something like:
 @code
 if (HooksManager::calloutsPresent(lease_hook_index)) {
     // Set up arguments for lease assignment
@@ -332,7 +388,7 @@ if (HooksManager::calloutsPresent(lease_hook_index)) {
 
 @section hooksComponentLoadLibraries Loading the User Libraries
 
-Once hooks are defined, all the hooks code describe above will
+Once hooks are defined, all the hooks code described above will
 work, even if no libraries are loaded (and even if the library
 loading method is not called).  The CalloutHandle returned by
 isc::hooks::HooksManager::createCalloutHandle() will be valid,
@@ -365,8 +421,8 @@ loadLibraries() with an empty vector as an argument.
 @subsection hooksComponentUnloadIssues Unload and Reload Issues
 
 Unloading a shared library works by unmapping the part of the process's
-virtual address space in which the library lies.  This may lead to problems
-consequences if there are still references to that address space elsewhere
+virtual address space in which the library lies.  This may lead to
+problems if there are still references to that address space elsewhere
 in the process.
 
 In many operating systems, heap storage allowed by a shared library will
@@ -376,21 +432,21 @@ in the hooks framework because:
 - Argument information stored in a CalloutHandle by a callout in a library
 may lie in the library's address space.
 - Data modified in objects passed as arguments may lie in the address
-space.  For example, it is common for a DHCP callout to add "options" to
-a packet: the memory allocated for those options will like in library address
-space.
+space.  For example, it is common for a DHCP callout to add "options"
+to a packet: the memory allocated for those options will most likely
+lie in library address space.
 
 The problem really arises because of the extensive use by BIND 10 of boost
 smart pointers.  When the pointer is destroyed, the pointed-to memory is
 deallocated.  If the pointer points to address space that is unmapped because
 a library has been unloaded, the deletion causes a segmentation fault.
 
-The hooks framework addresses the issue for CalloutHandles by keeping
-in that object a shared pointer to the object controlling library
-unloading.  Although a library can be unloaded at any time, it is only when
-all CalloutHandles that could possibly reference address space in the
-library have been deleted that the library will be unloaded and the address
-space unmapped.
+The hooks framework addresses the issue for CalloutHandles by keeping in
+that object a shared pointer to the object controlling library unloading.
+Although a library can be unloaded at any time, it is only when all
+CalloutHandles that could possibly reference address space in the library
+have been deleted that the library will actually be unloaded and the
+address space unmapped.
 
 The hooks framework cannot solve the second issue as the objects in
 question are under control of the component.  It is up to the component
@@ -421,7 +477,7 @@ the LibraryHandle to register and deregister callouts is described in
 
 Finally, it should be noted that callouts registered in this way only
 remain registered until the next call to isc::hooks::loadLibraries().
-It is up to the server to re-register the callouts after this
+It is up to the component to re-register the callouts after this
 method has been called.
 
 */

src/lib/hooks/server_hooks.h

@@ -151,7 +151,7 @@ private:
     ///
     /// Constructor is declared private to enforce the singleton nature of
     /// the object.  A reference to the singleton is obtainable through the
-    /// ggetServerHooks() static method.
+    /// getServerHooks() static method.
     ///
     /// @throws isc::Unexpected if the registration of the pre-defined hooks
     ///         fails in some way.