Browse Source

[3083] Merge branch 'master' into trac3083

Conflicts:
	src/lib/dhcpsrv/alloc_engine.cc
	src/lib/dhcpsrv/alloc_engine.h
	src/lib/dhcpsrv/tests/alloc_engine_unittest.cc
Marcin Siodelski 11 years ago
parent
commit
21b9e729ef
39 changed files with 2232 additions and 382 deletions
  1. 24 2
      ChangeLog
  2. 6 4
      doc/devel/mainpage.dox
  3. 0 2
      src/bin/cmdctl/b10-cmdctl.xml
  4. 0 2
      src/bin/cmdctl/cmdctl.py.in
  5. 0 5
      src/bin/cmdctl/cmdctl.spec.pre.in
  6. 16 4
      src/bin/cmdctl/tests/cmdctl_test.py
  7. 1 1
      src/bin/dbutil/b10-dbutil.xml
  8. 104 26
      src/bin/dhcp4/dhcp4_hooks.dox
  9. 22 0
      src/bin/dhcp4/dhcp4_messages.mes
  10. 294 174
      src/bin/dhcp4/dhcp4_srv.cc
  11. 0 7
      src/bin/dhcp4/dhcp4_srv.h
  12. 678 25
      src/bin/dhcp4/tests/dhcp4_srv_unittest.cc
  13. 1 1
      src/bin/dhcp6/dhcp6_messages.mes
  14. 4 30
      src/bin/dhcp6/dhcp6_srv.cc
  15. 4 6
      src/bin/dhcp6/dhcp6_srv.h
  16. 2 6
      src/bin/dhcp6/tests/hooks_unittest.cc
  17. 27 27
      src/lib/dhcp/pkt4.cc
  18. 38 28
      src/lib/dhcp/pkt4.h
  19. 1 0
      src/lib/dhcpsrv/Makefile.am
  20. 55 3
      src/lib/dhcpsrv/alloc_engine.cc
  21. 3 0
      src/lib/dhcpsrv/alloc_engine.h
  22. 91 0
      src/lib/dhcpsrv/callout_handle_store.h
  23. 36 15
      src/lib/dhcpsrv/dhcp_parsers.cc
  24. 16 3
      src/lib/dhcpsrv/dhcp_parsers.h
  25. 6 0
      src/lib/dhcpsrv/dhcpsrv_messages.mes
  26. 5 0
      src/lib/dhcpsrv/tests/Makefile.am
  27. 8 1
      src/lib/dhcpsrv/tests/alloc_engine_unittest.cc
  28. 122 0
      src/lib/dhcpsrv/tests/callout_handle_store_unittest.cc
  29. 307 1
      src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc
  30. 31 0
      src/lib/dhcpsrv/tests/test_get_callout_handle.cc
  31. 46 0
      src/lib/dhcpsrv/tests/test_get_callout_handle.h
  32. 3 2
      src/lib/hooks/callout_manager.cc
  33. 4 4
      src/lib/hooks/hooks_component_developer.dox
  34. 274 0
      src/lib/hooks/hooks_maintenance.dox
  35. 1 1
      src/lib/hooks/hooks_messages.mes
  36. 1 1
      src/lib/hooks/hook_user.dox
  37. BIN
      src/lib/hooks/images/HooksUml.dia
  38. BIN
      src/lib/hooks/images/HooksUml.png
  39. 1 1
      tests/tools/perfdhcp/perf_pkt4.cc

+ 24 - 2
ChangeLog

@@ -1,12 +1,34 @@
+667.	[func]		tomek
+	Additional hooks (buffer4_receive, lease4_renew,
+	lease4_release, buffer4_send) added to the DHCPv4 server.
+	(Trac #2983, git fd47f18f898695b98623a63a0a1c68d2e4b37568)
+
+666.	[func]		vorner
+	The CmdCtl's command "print_settings" was removed. It served no real
+	purpose and was just experimental leftover from early development.
+	(Trac #3028, git 0d22246092ad4822d48f5a52af5f644f5ae2f5e2)
+
+665.	[doc]		stephen
+	Added the "Hook's Maintenance Guide" to the BIND 10 developer
+	documentation.
+	(Trac #3063, git 5d1ee7b7470fc644b798ac47db1811c829f5ac24)
+
+664.	[bug]		tmark
+	Corrects a bug in Hooks processing that was improperly
+	creating a new callout handle on every call, rather
+	than maintaining it throughout the context of the
+	packet being processed.
+	(Trac #3062, git 28684bcfe5e54ad0421d75d4445a04b75358ce77)
+
 663.	[func]		marcin
-	bind10-dhcp6: Server processes the DHCPv6 Client FQDN Option
+	b10-dhcp6: Server processes the DHCPv6 Client FQDN Option
 	sent by a client and generates the response. The DHCPv6 Client
 	FQDN Option is represented by the new class in the libdhcp++.
 	As a result of FQDN Option processing, the server generates
 	NameChangeRequests which represent changes to DNS mappings for
 	a particular lease (addition or removal of DNS mappings).
 	Currently all generated NameChangeRequests are dropped. Sending
-	them to bind10-d2 will be implemented with the future tickets.
+	them to b10-dhcp-ddns will be implemented with the future tickets.
 	(Trac #3036, git 209f3964b9f12afbf36f3fa6b62964e03049ec6e)
 
 662.	[func]		marcin

+ 6 - 4
doc/devel/mainpage.dox

@@ -21,7 +21,7 @@
  *
  * If you wish to write "hook" code - code that is loaded by BIND 10 at
  * run-time and modifies its behavior you should read the section
- * @ref hookDevelopersGuide.
+ * @ref hooksdgDevelopersGuide.
  *
  * BIND 10 maintanenace information is divided into a number of sections
  * depending on focus.  DNS-specific issues are covered in the
@@ -30,16 +30,18 @@
  * specific to any protocol, are discussed in @ref miscellaneousTopics.
  *
  * If you are a user or system administrator, rather than software engineer,
- * you should read <a href="http://bind10.isc.org/docs/bind10-guide.html">BIND10
+ * you should read the
+ * <a href="http://bind10.isc.org/docs/bind10-guide.html">BIND10
  * Guide (Administrator Reference for BIND10)</a> instead.
  *
- * Regardless of your field of expertise, you are encouraged to visit
+ * Regardless of your field of expertise, you are encouraged to visit the
  * <a href="http://bind10.isc.org/">BIND10 webpage (http://bind10.isc.org)</a>
  * @section hooksFramework Hooks Framework
  * - @subpage hooksdgDevelopersGuide
  * - @subpage dhcpv4Hooks
  * - @subpage dhcpv6Hooks
  * - @subpage hooksComponentDeveloperGuide
+ * - @subpage hooksmgMaintenanceGuide
  *
  * @section dnsMaintenanceGuide DNS Maintenance Guide
  * - Authoritative DNS (todo)
@@ -70,7 +72,7 @@
  * - @subpage perfdhcpInternals
  * - @subpage libdhcp_ddns
  *
- * @section miscellaneousTopics Miscellaneous topics
+ * @section miscellaneousTopics Miscellaneous Topics
  * - @subpage LoggingApi
  *   - @subpage LoggingApiOverview
  *   - @subpage LoggingApiLoggerNames

+ 0 - 2
src/bin/cmdctl/b10-cmdctl.xml

@@ -169,8 +169,6 @@
       The configuration command is:
     </para>
 
-<!-- NOTE: print_settings is not documented since I think will be removed -->
-
     <para>
       <command>shutdown</command> exits <command>b10-cmdctl</command>.
       This has an optional <varname>pid</varname> argument to

+ 0 - 2
src/bin/cmdctl/cmdctl.py.in

@@ -370,8 +370,6 @@ class CommandControl():
             self._httpserver.shutdown()
             self._serving = False
 
-        elif command == 'print_settings':
-            answer = ccsession.create_answer(0, self._cmdctl_config_data)
         else:
             answer = ccsession.create_answer(1, 'unknown command: ' + command)
 

+ 0 - 5
src/bin/cmdctl/cmdctl.spec.pre.in

@@ -24,11 +24,6 @@
     ],
     "commands": [
       {
-        "command_name": "print_settings",
-        "command_description": "Print some_string and some_int to stdout",
-        "command_args": []
-      },
-      {
         "command_name": "shutdown",
         "command_description": "shutdown cmdctl",
         "command_args": [

+ 16 - 4
src/bin/cmdctl/tests/cmdctl_test.py

@@ -462,10 +462,22 @@ class TestCommandControl(unittest.TestCase):
         answer = self.cmdctl.command_handler('unknown-command', None)
         self._check_answer(answer, 1, 'unknown command: unknown-command')
 
-        answer = self.cmdctl.command_handler('print_settings', None)
+        # Send a real command. Mock stuff so the shutdown command doesn't
+        # cause an exception.
+        class ModuleCC:
+            def send_stopping():
+                pass
+        self.cmdctl._module_cc = ModuleCC
+        called = []
+        class Server:
+            def shutdown():
+                called.append('shutdown')
+        self.cmdctl._httpserver = Server
+        answer = self.cmdctl.command_handler('shutdown', None)
         rcode, msg = ccsession.parse_answer(answer)
         self.assertEqual(rcode, 0)
-        self.assertTrue(msg != None)
+        self.assertIsNone(msg)
+        self.assertEqual(['shutdown'], called)
 
     def test_command_handler_spec_update(self):
         # Should not be present
@@ -543,10 +555,10 @@ class TestCommandControl(unittest.TestCase):
         self.assertEqual(1, rcode)
 
         # Send a command to cmdctl itself.  Should be the same effect.
-        rcode, value = self.cmdctl.send_command('Cmdctl', 'print_settings',
+        rcode, value = self.cmdctl.send_command('Cmdctl', 'shutdown',
                                                 None)
         self.assertEqual(2, len(self.cmdctl.sent_messages))
-        self.assertEqual(({'command': ['print_settings']}, 'Cmdctl'),
+        self.assertEqual(({'command': ['shutdown']}, 'Cmdctl'),
                          self.cmdctl.sent_messages[-1])
         self.assertEqual(1, rcode)
 

+ 1 - 1
src/bin/dbutil/b10-dbutil.xml

@@ -68,7 +68,7 @@
     </para>
 
     <para>
-      <command>b10-dbutil</command> operates in one of two modesr: check mode
+      <command>b10-dbutil</command> operates in one of two modes: check mode
       or upgrade mode.
     </para>
 

+ 104 - 26
src/bin/dhcp4/dhcp4_hooks.dox

@@ -49,22 +49,45 @@ The following list is ordered by appearance of specific hook points during
 packet processing. Hook points that are not specific to packet processing
 (e.g. lease expiration) will be added to the end of this list.
 
+ @subsection dhcpv4HooksBuffer4Receive buffer4_receive
+
+ - @b Arguments:
+   - name: @b query4, type: isc::dhcp::Pkt4Ptr, direction: <b>in/out</b>
+
+ - @b Description: this callout is executed when an incoming DHCPv4
+   buffer is received, before its content is parsed. The sole argument
+   - query4 - contains a pointer to an isc::dhcp::Pkt4 object that
+   contains raw information regarding incoming packet, including its
+   source and destination addresses, interface over which it was
+   received, and a raw buffer stored in data_ field. None of the
+   packet fields (op_, hlen_, chaddr_, etc.) are set yet. Callouts
+   installed on this hook point can modify the incoming buffer. The
+   server will parse the buffer afterwards.
+
+ - <b>Skip flag action</b>: If any callout sets the skip flag, the server will
+   skip the buffer parsing. In such case there is an expectation that
+   the callout will parse the buffer and create option objects (see
+   isc::dhcp::Pkt4::addOption()). Otherwise the server will find out
+   that some mandatory options are missing (e.g. DHCP Message Type) and
+   will drop the packet. If you want to have the capability to drop
+   a message, it is better to use skip flag in pkt4_receive callout.
+
  @subsection dhcpv4HooksPkt4Receive pkt4_receive
 
  - @b Arguments:
    - name: @b query4, type: isc::dhcp::Pkt4Ptr, direction: <b>in/out</b>
 
  - @b Description: this callout is executed when an incoming DHCPv4
-   packet is received and its content is parsed. The sole argument -
-   query4 - contains a pointer to an isc::dhcp::Pkt4 object that contains
-   all information regarding incoming packet, including its source and
-   destination addresses, interface over which it was received, a list
-   of all options present within and relay information.  All fields of
-   the Pkt4 object can be modified at this time, except data_. (data_
-   contains the incoming packet as raw buffer. By the time this hook is
-   reached, that information has already parsed and is available though
-   other fields in the Pkt4 object.  For this reason, it doesn't make
-   sense to modify it.)
+   packet is received and its content has been parsed. The sole
+   argument - query4 - contains a pointer to an isc::dhcp::Pkt4 object
+   that contains all information regarding incoming packet, including
+   its source and destination addresses, interface over which it was
+   received, a list of all options present within and relay
+   information.  All fields of the Pkt4 object can be modified at this
+   time, except data_. (By the time this hook is reached, the contents
+   of the data_ field has been already parsed and stored in other
+   fields. Therefore, the modification in the data_ field has no
+   effect.)
 
  - <b>Skip flag action</b>: If any callout sets the skip flag, the server will
    drop the packet and start processing the next one.  The reason for the drop
@@ -97,42 +120,97 @@ packet processing. Hook points that are not specific to packet processing
    - name: @b lease4, type: isc::dhcp::Lease4Ptr, direction: <b>in/out</b>
 
  - @b Description: this callout is executed after the server engine
-   has selected a lease for client's request but before the lease
-   has been inserted into the database. Any modifications made to the
-   isc::dhcp::Lease4 object will be stored in the lease's record in the
-   database. The callout should make sure that any modifications are
-   sanity checked as the server will use that data as is with no further
-   checking.\n\n The server processes lease requests for DISCOVER and
-   REQUEST in a very similar way. The only major difference is that
-   for DISCOVER the lease is just selected, but not inserted into
-   the database.  It is possible to distinguish between DISCOVER and
-   REQUEST by checking value of the fake_allocation flag: a value of true
-   means that the lease won't be inserted into the database (DISCOVER),
-   a value of false means that it will (REQUEST).
+   has selected a lease for client's request but before the lease has
+   been inserted into the database. Any modifications made to the
+   isc::dhcp::Lease4 object will be stored in the lease's record in
+   the database. The callout should sanity check all modifications as
+   the server will use that data as is with no further checking.\n\n
+   The server processes lease requests for DISCOVER and REQUEST in a
+   very similar way. The only major difference is that for DISCOVER
+   the lease is just selected, but not inserted into the database.  It
+   is possible to distinguish between DISCOVER and REQUEST by checking
+   value of the fake_allocation flag: a value of true indicates that the
+   lease won't be inserted into the database (DISCOVER), a value of
+   false indicates that it will (REQUEST).
 
  - <b>Skip flag action</b>: If any callout installed on 'lease4_select'
    sets the skip flag, the server will not assign any lease. Packet
    processing will continue, but client will not get an address.
 
+@subsection dhcpv4HooksLeaseRenew lease4_renew
+
+ - @b Arguments:
+   - name: @b subnet4, type: isc::dhcp::Subnet4Ptr, direction: <b>in</b>
+   - name: @b clientid, type: isc::dhcp::ClientId, direction: <b>in</b>
+   - name: @b hwaddr, type: isc::dhcp::HWAddr, direction: <b>in</b>
+   - name: @b lease4, type: isc::dhcp::Lease4Ptr, direction: <b>in/out</b>
+
+ - @b Description: this callout is executed when the server engine
+   is about to renew a lease, as a result of receiving REQUEST/Renewing
+   packet. The lease4 argument points to Lease4 object that contains
+   the updated values. Callout can modify those values. Care should be taken
+   as the server will attempt to update the lease in the database without
+   any additional checks.
+
+ - <b>Skip flag action</b>: If any callout installed on 'lease4_renew'
+   sets the skip flag, the server will not update the lease and will
+   use old values instead.
+
+@subsection dhcpv4HooksLeaseRelease lease4_release
+
+ - @b Arguments:
+   - name: @b query4, type: isc::dhcp::Pkt4Ptr, direction: <b>in</b>
+   - name: @b lease4, type: isc::dhcp::Lease4Ptr, direction: <b>in</b>
+
+ - @b Description: this callout is executed when the server engine
+   is about to release a lease, as a result of receiving RELEASE packet.
+   The lease4 argument points to Lease4 object that contains the lease to
+   be released. It doesn't make sense to modify it at this time.
+
+ - <b>Skip flag action</b>: If any callout installed on 'lease4_release'
+   sets the skip flag, the server will not delete the lease. It will be
+   kept in the database and will go through the regular expiration/reuse
+   process.
+
 @subsection dhcpv4HooksPkt4Send pkt4_send
 
  - @b Arguments:
    - name: @b response4, type: isc::dhcp::Pkt4Ptr, direction: <b>in/out</b>
 
  - @b Description: this callout is executed when server's response
-   is about to be send back to the client. The sole argument - response4 -
+   is about to be sent back to the client. The sole argument - response4 -
    contains a pointer to an isc::dhcp::Pkt4 object that contains the
-   packet, with set source and destination addresses, interface over which
-   it will be send, list of all options and relay information.  All fields
-   of the Pkt4 object can be modified at this time, except bufferOut_.
+   packet, with source and destination addresses set, interface over which
+   it will be sent, and a list of all options and relay information.  All fields
+   of the Pkt4 object can be modified at this time, except buffer_out_.
    (This is scratch space used for constructing the packet after all
    pkt4_send callouts are complete, so any changes to that field will
    be overwritten.)
 
  - <b>Skip flag action</b>: if any callout sets the skip flag, the server
+   will not construct raw buffer. The expectation is that if the callout
+   set skip flag, it is responsible for constructing raw form on its own.
+   Otherwise the output packet will be sent with zero length.
+
+@subsection dhcpv4HooksBuffer4Send buffer4_send
+
+ - @b Arguments:
+   - name: @b response4, type: isc::dhcp::Pkt4Ptr, direction: <b>in/out</b>
+
+ - @b Description: this callout is executed when server's response
+   is about to be sent back to the client. The sole argument - response4 -
+   contains a pointer to an isc::dhcp::Pkt4 object that contains the
+   packet, with source and destination addresses set, interface over which
+   it will be sent, and a list of all options and relay information. The raw
+   on-wire form is already prepared in buffer_out_ (see isc::dhcp::Pkt4::getBuffer())
+   It doesn't make any sense to modify packet fields or options content
+   at this time, because they were already used to construct on-wire buffer.
+
+ - <b>Skip flag action</b>: if any callout sets the skip flag, the server
    will drop this response packet. However, the original request packet
    from a client was processed, so server's state was most likely changed
    (e.g. lease was allocated). Setting this flag merely stops the change
    being communicated to the client.
 
+
 */

+ 22 - 0
src/bin/dhcp4/dhcp4_messages.mes

@@ -70,6 +70,24 @@ This message is printed when DHCPv4 server disables an interface from being
 used to receive DHCPv4 traffic. Sockets on this interface will not be opened
 by the Interface Manager until interface is enabled.
 
+% DHCP4_HOOK_BUFFER_RCVD_SKIP received DHCPv4 buffer was dropped because a callout set the skip flag.
+This debug message is printed when a callout installed on buffer4_receive
+hook point set the skip flag. For this particular hook point, the
+setting of the flag by a callout instructs the server to drop the packet.
+
+% DHCP4_HOOK_BUFFER_SEND_SKIP prepared DHCPv4 response was dropped because a callout set the skip flag.
+This debug message is printed when a callout installed on buffer4_send
+hook point set the skip flag. For this particular hook point, the
+setting of the flag by a callout instructs the server to drop the packet.
+Server completed all the processing (e.g. may have assigned, updated
+or released leases), but the response will not be send to the client.
+
+% DHCP4_HOOK_LEASE4_RELEASE_SKIP DHCPv4 lease was not released because a callout set the skip flag.
+This debug message is printed when a callout installed on lease4_release
+hook point set the skip flag. For this particular hook point, the
+setting of the flag by a callout instructs the server to not release
+a lease.
+
 % DHCP4_HOOK_PACKET_RCVD_SKIP received DHCPv4 packet was dropped, because a callout set the skip flag.
 This debug message is printed when a callout installed on the pkt4_receive
 hook point sets the skip flag. For this particular hook point, the
@@ -138,6 +156,10 @@ This is a general catch-all message indicating that the processing of a
 received packet failed.  The reason is given in the message.  The server
 will not send a response but will instead ignore the packet.
 
+% DHCP4_PACKET_DROP_NO_TYPE packet received on interface %1 dropped, because of missing msg-type option
+This is a debug message informing that incoming DHCPv4 packet did not
+have mandatory DHCP message type option and thus was dropped.
+
 % DHCP4_PACKET_RECEIVED %1 (type %2) packet received on interface %3
 A debug message noting that the server has received the specified type of
 packet on the specified interface.  Note that a packet marked as UNKNOWN

+ 294 - 174
src/bin/dhcp4/dhcp4_srv.cc

@@ -14,24 +14,25 @@
 
 #include <asiolink/io_address.h>
 #include <dhcp/dhcp4.h>
+#include <dhcp/duid.h>
+#include <dhcp/hwaddr.h>
 #include <dhcp/iface_mgr.h>
 #include <dhcp/option4_addrlst.h>
 #include <dhcp/option_int.h>
 #include <dhcp/option_int_array.h>
 #include <dhcp/pkt4.h>
-#include <dhcp/duid.h>
-#include <dhcp/hwaddr.h>
 #include <dhcp4/dhcp4_log.h>
 #include <dhcp4/dhcp4_srv.h>
-#include <dhcpsrv/utils.h>
+#include <dhcpsrv/addr_utilities.h>
+#include <dhcpsrv/callout_handle_store.h>
 #include <dhcpsrv/cfgmgr.h>
 #include <dhcpsrv/lease_mgr.h>
 #include <dhcpsrv/lease_mgr_factory.h>
 #include <dhcpsrv/subnet.h>
 #include <dhcpsrv/utils.h>
-#include <dhcpsrv/addr_utilities.h>
-#include <hooks/hooks_manager.h>
+#include <dhcpsrv/utils.h>
 #include <hooks/callout_handle.h>
+#include <hooks/hooks_manager.h>
 
 #include <boost/algorithm/string/erase.hpp>
 
@@ -46,16 +47,22 @@ using namespace isc::log;
 using namespace std;
 
 /// Structure that holds registered hook indexes
-struct Dhcp6Hooks {
+struct Dhcp4Hooks {
+    int hook_index_buffer4_receive_;///< index for "buffer4_receive" hook point
     int hook_index_pkt4_receive_;   ///< index for "pkt4_receive" hook point
     int hook_index_subnet4_select_; ///< index for "subnet4_select" hook point
+    int hook_index_lease4_release_; ///< index for "lease4_release" hook point
     int hook_index_pkt4_send_;      ///< index for "pkt4_send" hook point
+    int hook_index_buffer4_send_;   ///< index for "buffer4_send" hook point
 
-    /// Constructor that registers hook points for DHCPv6 engine
-    Dhcp6Hooks() {
+    /// Constructor that registers hook points for DHCPv4 engine
+    Dhcp4Hooks() {
+        hook_index_buffer4_receive_= HooksManager::registerHook("buffer4_receive");
         hook_index_pkt4_receive_   = HooksManager::registerHook("pkt4_receive");
         hook_index_subnet4_select_ = HooksManager::registerHook("subnet4_select");
         hook_index_pkt4_send_      = HooksManager::registerHook("pkt4_send");
+        hook_index_lease4_release_ = HooksManager::registerHook("lease4_release");
+        hook_index_buffer4_send_   = HooksManager::registerHook("buffer4_send");
     }
 };
 
@@ -63,7 +70,7 @@ struct Dhcp6Hooks {
 // 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.
-Dhcp6Hooks Hooks;
+Dhcp4Hooks Hooks;
 
 namespace isc {
 namespace dhcp {
@@ -82,8 +89,8 @@ static const char* SERVER_ID_FILE = "b10-dhcp4-serverid";
 
 Dhcpv4Srv::Dhcpv4Srv(uint16_t port, const char* dbconfig, const bool use_bcast,
                      const bool direct_response_desired)
-: serverid_(), shutdown_(true), alloc_engine_(), port_(port), 
-    use_bcast_(use_bcast), hook_index_pkt4_receive_(-1), 
+: serverid_(), shutdown_(true), alloc_engine_(), port_(port),
+    use_bcast_(use_bcast), hook_index_pkt4_receive_(-1),
     hook_index_subnet4_select_(-1), hook_index_pkt4_send_(-1) {
 
     LOG_DEBUG(dhcp4_logger, DBG_DHCP4_START, DHCP4_OPEN_SOCKET).arg(port);
@@ -183,151 +190,245 @@ Dhcpv4Srv::run() {
             LOG_ERROR(dhcp4_logger, DHCP4_PACKET_RECEIVE_FAIL).arg(e.what());
         }
 
-        if (query) {
+        // Timeout may be reached or signal received, which breaks select()
+        // with no reception ocurred
+        if (!query) {
+            continue;
+        }
+
+        bool skip_unpack = false;
+
+        // The packet has just been received so contains the uninterpreted wire
+        // data; execute callouts registered for buffer4_receive.
+        if (HooksManager::getHooksManager()
+            .calloutsPresent(Hooks.hook_index_buffer4_receive_)) {
+            CalloutHandlePtr callout_handle = getCalloutHandle(query);
+
+            // Delete previously set arguments
+            callout_handle->deleteAllArguments();
+
+            // Pass incoming packet as argument
+            callout_handle->setArgument("query4", query);
+
+            // Call callouts
+            HooksManager::callCallouts(Hooks.hook_index_buffer4_receive_,
+                                       *callout_handle);
+
+            // Callouts decided to skip the next processing step. The next
+            // processing step would to parse the packet, so skip at this
+            // stage means that callouts did the parsing already, so server
+            // should skip parsing.
+            if (callout_handle->getSkip()) {
+                LOG_DEBUG(dhcp4_logger, DBG_DHCP4_HOOKS, DHCP4_HOOK_BUFFER_RCVD_SKIP);
+                skip_unpack = true;
+            }
+
+            callout_handle->getArgument("query4", query);
+        }
+
+        // Unpack the packet information unless the buffer4_receive callouts
+        // indicated they did it
+        if (!skip_unpack) {
             try {
                 query->unpack();
-
             } catch (const std::exception& e) {
                 // Failed to parse the packet.
                 LOG_DEBUG(dhcp4_logger, DBG_DHCP4_DETAIL,
                           DHCP4_PACKET_PARSE_FAIL).arg(e.what());
                 continue;
             }
-            LOG_DEBUG(dhcp4_logger, DBG_DHCP4_DETAIL, DHCP4_PACKET_RECEIVED)
-                      .arg(serverReceivedPacketName(query->getType()))
-                      .arg(query->getType())
-                      .arg(query->getIface());
-            LOG_DEBUG(dhcp4_logger, DBG_DHCP4_DETAIL_DATA, DHCP4_QUERY_DATA)
-                      .arg(static_cast<int>(query->getType()))
-                      .arg(query->toText());
-
-            // Let's execute all callouts registered for packet_received
-            if (HooksManager::calloutsPresent(hook_index_pkt4_receive_)) {
-                CalloutHandlePtr callout_handle = getCalloutHandle(query);
-
-                // Delete previously set arguments
-                callout_handle->deleteAllArguments();
+        }
 
-                // Pass incoming packet as argument
-                callout_handle->setArgument("query4", query);
+        // When receiving a packet without message type option, getType() will
+        // throw. Let's set type to -1 as default error indicator.
+        int type = -1;
+        try {
+            type = query->getType();
+        } catch (...) {
+            LOG_DEBUG(dhcp4_logger, DBG_DHCP4_DETAIL, DHCP4_PACKET_DROP_NO_TYPE)
+                .arg(query->getIface());
+            continue;
+        }
 
-                // Call callouts
-                HooksManager::callCallouts(hook_index_pkt4_receive_,
-                                           *callout_handle);
+        LOG_DEBUG(dhcp4_logger, DBG_DHCP4_DETAIL, DHCP4_PACKET_RECEIVED)
+            .arg(serverReceivedPacketName(type))
+            .arg(type)
+            .arg(query->getIface());
+        LOG_DEBUG(dhcp4_logger, DBG_DHCP4_DETAIL_DATA, DHCP4_QUERY_DATA)
+            .arg(type)
+            .arg(query->toText());
+
+        // Let's execute all callouts registered for pkt4_receive
+        if (HooksManager::calloutsPresent(hook_index_pkt4_receive_)) {
+            CalloutHandlePtr callout_handle = getCalloutHandle(query);
+
+            // Delete previously set arguments
+            callout_handle->deleteAllArguments();
+
+            // Pass incoming packet as argument
+            callout_handle->setArgument("query4", query);
+
+            // Call callouts
+            HooksManager::callCallouts(hook_index_pkt4_receive_,
+                                       *callout_handle);
+
+            // Callouts decided to skip the next processing step. The next
+            // processing step would to process the packet, so skip at this
+            // stage means drop.
+            if (callout_handle->getSkip()) {
+                LOG_DEBUG(dhcp4_logger, DBG_DHCP4_HOOKS, DHCP4_HOOK_PACKET_RCVD_SKIP);
+                continue;
+            }
 
-                // Callouts decided to skip the next processing step. The next
-                // processing step would to process the packet, so skip at this
-                // stage means drop.
-                if (callout_handle->getSkip()) {
-                    LOG_DEBUG(dhcp4_logger, DBG_DHCP4_HOOKS, DHCP4_HOOK_PACKET_RCVD_SKIP);
-                    continue;
-                }
+            callout_handle->getArgument("query4", query);
+        }
 
-                callout_handle->getArgument("query4", query);
+        try {
+            switch (query->getType()) {
+            case DHCPDISCOVER:
+                rsp = processDiscover(query);
+                break;
+
+            case DHCPREQUEST:
+                // Note that REQUEST is used for many things in DHCPv4: for
+                // requesting new leases, renewing existing ones and even
+                // for rebinding.
+                rsp = processRequest(query);
+                break;
+
+            case DHCPRELEASE:
+                processRelease(query);
+                break;
+
+            case DHCPDECLINE:
+                processDecline(query);
+                break;
+
+            case DHCPINFORM:
+                processInform(query);
+                break;
+
+            default:
+                // Only action is to output a message if debug is enabled,
+                // and that is covered by the debug statement before the
+                // "switch" statement.
+                ;
             }
-
-            try {
-                switch (query->getType()) {
-                case DHCPDISCOVER:
-                    rsp = processDiscover(query);
-                    break;
-
-                case DHCPREQUEST:
-                    rsp = processRequest(query);
-                    break;
-
-                case DHCPRELEASE:
-                    processRelease(query);
-                    break;
-
-                case DHCPDECLINE:
-                    processDecline(query);
-                    break;
-
-                case DHCPINFORM:
-                    processInform(query);
-                    break;
-
-                default:
-                    // Only action is to output a message if debug is enabled,
-                    // and that is covered by the debug statement before the
-                    // "switch" statement.
-                    ;
-                }
-            } catch (const isc::Exception& e) {
-
-                // Catch-all exception (at least for ones based on the isc
-                // Exception class, which covers more or less all that
-                // are explicitly raised in the BIND 10 code).  Just log
-                // the problem and ignore the packet. (The problem is logged
-                // as a debug message because debug is disabled by default -
-                // it prevents a DDOS attack based on the sending of problem
-                // packets.)
-                if (dhcp4_logger.isDebugEnabled(DBG_DHCP4_BASIC)) {
-                    std::string source = "unknown";
-                    HWAddrPtr hwptr = query->getHWAddr();
-                    if (hwptr) {
-                        source = hwptr->toText();
-                    }
-                    LOG_DEBUG(dhcp4_logger, DBG_DHCP4_BASIC,
-                              DHCP4_PACKET_PROCESS_FAIL)
-                              .arg(source).arg(e.what());
+        } catch (const isc::Exception& e) {
+
+            // Catch-all exception (at least for ones based on the isc
+            // Exception class, which covers more or less all that
+            // are explicitly raised in the BIND 10 code).  Just log
+            // the problem and ignore the packet. (The problem is logged
+            // as a debug message because debug is disabled by default -
+            // it prevents a DDOS attack based on the sending of problem
+            // packets.)
+            if (dhcp4_logger.isDebugEnabled(DBG_DHCP4_BASIC)) {
+                std::string source = "unknown";
+                HWAddrPtr hwptr = query->getHWAddr();
+                if (hwptr) {
+                    source = hwptr->toText();
                 }
+                LOG_DEBUG(dhcp4_logger, DBG_DHCP4_BASIC,
+                          DHCP4_PACKET_PROCESS_FAIL)
+                    .arg(source).arg(e.what());
             }
+        }
 
-            if (rsp) {
+        if (!rsp) {
+            continue;
+        }
 
-                adjustRemoteAddr(query, rsp);
+        adjustRemoteAddr(query, rsp);
 
-                if (!rsp->getHops()) {
-                    rsp->setRemotePort(DHCP4_CLIENT_PORT);
-                } else {
-                    rsp->setRemotePort(DHCP4_SERVER_PORT);
-                }
+        if (!rsp->getHops()) {
+            rsp->setRemotePort(DHCP4_CLIENT_PORT);
+        } else {
+            rsp->setRemotePort(DHCP4_SERVER_PORT);
+        }
 
-                rsp->setLocalAddr(query->getLocalAddr());
-                rsp->setLocalPort(DHCP4_SERVER_PORT);
-                rsp->setIface(query->getIface());
-                rsp->setIndex(query->getIndex());
+        rsp->setLocalAddr(query->getLocalAddr());
+        rsp->setLocalPort(DHCP4_SERVER_PORT);
+        rsp->setIface(query->getIface());
+        rsp->setIndex(query->getIndex());
 
-                // Execute all callouts registered for packet6_send
-                if (HooksManager::calloutsPresent(hook_index_pkt4_send_)) {
-                    CalloutHandlePtr callout_handle = getCalloutHandle(query);
+        // Specifies if server should do the packing
+        bool skip_pack = false;
 
-                    // Delete all previous arguments
-                    callout_handle->deleteAllArguments();
+        // Execute all callouts registered for pkt4_send
+        if (HooksManager::calloutsPresent(hook_index_pkt4_send_)) {
+            CalloutHandlePtr callout_handle = getCalloutHandle(query);
 
-                    // Clear skip flag if it was set in previous callouts
-                    callout_handle->setSkip(false);
+            // Delete all previous arguments
+            callout_handle->deleteAllArguments();
 
-                    // Set our response
-                    callout_handle->setArgument("response4", rsp);
+            // Clear skip flag if it was set in previous callouts
+            callout_handle->setSkip(false);
 
-                    // Call all installed callouts
-                    HooksManager::callCallouts(hook_index_pkt4_send_,
-                                               *callout_handle);
+            // Set our response
+            callout_handle->setArgument("response4", rsp);
 
-                    // Callouts decided to skip the next processing step. The next
-                    // processing step would to send the packet, so skip at this
-                    // stage means "drop response".
-                    if (callout_handle->getSkip()) {
-                        LOG_DEBUG(dhcp4_logger, DBG_DHCP4_HOOKS, DHCP4_HOOK_PACKET_SEND_SKIP);
-                        continue;
-                    }
-                }
+            // Call all installed callouts
+            HooksManager::callCallouts(hook_index_pkt4_send_,
+                                       *callout_handle);
 
-                LOG_DEBUG(dhcp4_logger, DBG_DHCP4_DETAIL_DATA,
-                          DHCP4_RESPONSE_DATA)
-                          .arg(rsp->getType()).arg(rsp->toText());
+            // Callouts decided to skip the next processing step. The next
+            // processing step would to send the packet, so skip at this
+            // stage means "drop response".
+            if (callout_handle->getSkip()) {
+                LOG_DEBUG(dhcp4_logger, DBG_DHCP4_HOOKS, DHCP4_HOOK_PACKET_SEND_SKIP);
+                skip_pack = true;
+            }
+        }
+
+        if (!skip_pack) {
+            try {
+                rsp->pack();
+            } catch (const std::exception& e) {
+                LOG_ERROR(dhcp4_logger, DHCP4_PACKET_SEND_FAIL)
+                    .arg(e.what());
+            }
+        }
+
+        try {
+            // Now all fields and options are constructed into output wire buffer.
+            // Option objects modification does not make sense anymore. Hooks
+            // can only manipulate wire buffer at this stage.
+            // Let's execute all callouts registered for buffer4_send
+            if (HooksManager::getHooksManager()
+                .calloutsPresent(Hooks.hook_index_buffer4_send_)) {
+                CalloutHandlePtr callout_handle = getCalloutHandle(query);
+
+                // Delete previously set arguments
+                callout_handle->deleteAllArguments();
 
-                try {
-                    rsp->pack();
-                    sendPacket(rsp);
-                } catch (const std::exception& e) {
-                    LOG_ERROR(dhcp4_logger, DHCP4_PACKET_SEND_FAIL)
-                              .arg(e.what());
+                // Pass incoming packet as argument
+                callout_handle->setArgument("response4", rsp);
+
+                // Call callouts
+                HooksManager::callCallouts(Hooks.hook_index_buffer4_send_,
+                                           *callout_handle);
+
+                // Callouts decided to skip the next processing step. The next
+                // processing step would to parse the packet, so skip at this
+                // stage means drop.
+                if (callout_handle->getSkip()) {
+                    LOG_DEBUG(dhcp4_logger, DBG_DHCP4_HOOKS,
+                              DHCP4_HOOK_BUFFER_SEND_SKIP);
+                    continue;
                 }
+
+                callout_handle->getArgument("response4", rsp);
             }
+
+            LOG_DEBUG(dhcp4_logger, DBG_DHCP4_DETAIL_DATA,
+                      DHCP4_RESPONSE_DATA)
+                .arg(static_cast<int>(rsp->getType())).arg(rsp->toText());
+
+            sendPacket(rsp);
+        } catch (const std::exception& e) {
+            LOG_ERROR(dhcp4_logger, DHCP4_PACKET_SEND_FAIL)
+                .arg(e.what());
         }
     }
 
@@ -755,6 +856,10 @@ Dhcpv4Srv::processDiscover(Pkt4Ptr& discover) {
 
 Pkt4Ptr
 Dhcpv4Srv::processRequest(Pkt4Ptr& request) {
+
+    /// @todo Uncomment this (see ticket #3116)
+    // sanityCheck(request, MANDATORY);
+
     Pkt4Ptr ack = Pkt4Ptr
         (new Pkt4(DHCPACK, request->getTransid()));
 
@@ -762,6 +867,9 @@ Dhcpv4Srv::processRequest(Pkt4Ptr& request) {
     appendDefaultOptions(ack, DHCPACK);
     appendRequestedOptions(request, ack);
 
+    // Note that we treat REQUEST message uniformly, regardless if this is a
+    // first request (requesting for new address), renewing existing address
+    // or even rebinding.
     assignLease(request, ack);
 
     // There are a few basic options that we always want to
@@ -775,6 +883,9 @@ Dhcpv4Srv::processRequest(Pkt4Ptr& request) {
 void
 Dhcpv4Srv::processRelease(Pkt4Ptr& release) {
 
+    /// @todo Uncomment this (see ticket #3116)
+    // sanityCheck(release, MANDATORY);
+
     // Try to find client-id
     ClientIdPtr client_id;
     OptionPtr opt = release->getOption(DHO_DHCP_CLIENT_IDENTIFIER);
@@ -816,21 +927,53 @@ Dhcpv4Srv::processRelease(Pkt4Ptr& release) {
             return;
         }
 
-        // Ok, hw and client-id match - let's release the lease.
-        if (LeaseMgrFactory::instance().deleteLease(lease->addr_)) {
+        bool skip = false;
 
-            // Release successful - we're done here
-            LOG_DEBUG(dhcp4_logger, DBG_DHCP4_DETAIL, DHCP4_RELEASE)
-                .arg(lease->addr_.toText())
-                .arg(client_id ? client_id->toText() : "(no client-id)")
-                .arg(release->getHWAddr()->toText());
-        } else {
+        // Execute all callouts registered for lease4_release
+        if (HooksManager::getHooksManager()
+            .calloutsPresent(Hooks.hook_index_lease4_release_)) {
+            CalloutHandlePtr callout_handle = getCalloutHandle(release);
+
+            // Delete all previous arguments
+            callout_handle->deleteAllArguments();
+
+            // Pass the original packet
+            callout_handle->setArgument("query4", release);
+
+            // Pass the lease to be updated
+            callout_handle->setArgument("lease4", lease);
+
+            // Call all installed callouts
+            HooksManager::callCallouts(Hooks.hook_index_lease4_release_,
+                                       *callout_handle);
+
+            // Callouts decided to skip the next processing step. The next
+            // processing step would to send the packet, so skip at this
+            // stage means "drop response".
+            if (callout_handle->getSkip()) {
+                skip = true;
+                LOG_DEBUG(dhcp4_logger, DBG_DHCP4_HOOKS,
+                          DHCP4_HOOK_LEASE4_RELEASE_SKIP);
+            }
+        }
 
-            // Release failed -
-            LOG_ERROR(dhcp4_logger, DHCP4_RELEASE_FAIL)
-                .arg(lease->addr_.toText())
+        // Ok, hw and client-id match - let's release the lease.
+        if (!skip) {
+            bool success = LeaseMgrFactory::instance().deleteLease(lease->addr_);
+
+            if (success) {
+                // Release successful - we're done here
+                LOG_DEBUG(dhcp4_logger, DBG_DHCP4_DETAIL, DHCP4_RELEASE)
+                    .arg(lease->addr_.toText())
+                    .arg(client_id ? client_id->toText() : "(no client-id)")
+                    .arg(release->getHWAddr()->toText());
+            } else {
+                // Release failed -
+                LOG_ERROR(dhcp4_logger, DHCP4_RELEASE_FAIL)
+                    .arg(lease->addr_.toText())
                 .arg(client_id ? client_id->toText() : "(no client-id)")
-                .arg(release->getHWAddr()->toText());
+                    .arg(release->getHWAddr()->toText());
+            }
         }
     } catch (const isc::Exception& ex) {
         // Rethrow the exception with a bit more data.
@@ -843,12 +986,13 @@ Dhcpv4Srv::processRelease(Pkt4Ptr& release) {
 
 void
 Dhcpv4Srv::processDecline(Pkt4Ptr& /* decline */) {
-    /// TODO: Implement this.
+    /// @todo Implement this (also see ticket #3116)
 }
 
 Pkt4Ptr
 Dhcpv4Srv::processInform(Pkt4Ptr& inform) {
-    /// TODO: Currently implemented echo mode. Implement this for real
+
+    /// @todo Implement this for real. (also see ticket #3116)
     return (inform);
 }
 
@@ -902,7 +1046,7 @@ Dhcpv4Srv::selectSubnet(const Pkt4Ptr& question) {
 
     /// @todo Implement getSubnet4(interface-name)
 
-    // Let's execute all callouts registered for packet_received
+    // Let's execute all callouts registered for subnet4_select
     if (HooksManager::calloutsPresent(hook_index_subnet4_select_)) {
         CalloutHandlePtr callout_handle = getCalloutHandle(question);
 
@@ -912,16 +1056,19 @@ Dhcpv4Srv::selectSubnet(const Pkt4Ptr& question) {
         // Set new arguments
         callout_handle->setArgument("query4", question);
         callout_handle->setArgument("subnet4", subnet);
-        callout_handle->setArgument("subnet4collection", CfgMgr::instance().getSubnets4());
+        callout_handle->setArgument("subnet4collection",
+                                    CfgMgr::instance().getSubnets4());
 
         // Call user (and server-side) callouts
-        HooksManager::callCallouts(hook_index_subnet4_select_, *callout_handle);
+        HooksManager::callCallouts(hook_index_subnet4_select_,
+                                   *callout_handle);
 
         // Callouts decided to skip this step. This means that no subnet will be
-        // selected. Packet processing will continue, but it will be severly limited
-        // (i.e. only global options will be assigned)
+        // selected. Packet processing will continue, but it will be severly
+        // limited (i.e. only global options will be assigned)
         if (callout_handle->getSkip()) {
-            LOG_DEBUG(dhcp4_logger, DBG_DHCP4_HOOKS, DHCP4_HOOK_SUBNET4_SELECT_SKIP);
+            LOG_DEBUG(dhcp4_logger, DBG_DHCP4_HOOKS,
+                      DHCP4_HOOK_SUBNET4_SELECT_SKIP);
             return (Subnet4Ptr());
         }
 
@@ -973,33 +1120,6 @@ Dhcpv4Srv::sanityCheck(const Pkt4Ptr& pkt, RequirementLevel serverid) {
     }
 }
 
-isc::hooks::CalloutHandlePtr Dhcpv4Srv::getCalloutHandle(const Pkt4Ptr& pkt) {
-    // This method returns a CalloutHandle for a given packet. It is guaranteed
-    // to return the same callout_handle (so user library contexts are
-    // preserved). This method works well if the server processes one packet
-    // at a time. Once the server architecture is extended to cover parallel
-    // packets processing (e.g. delayed-ack, some form of buffering etc.), this
-    // method has to be extended (e.g. store callouts in a map and use pkt as
-    // a key). Additional code would be required to release the callout handle
-    // once the server finished processing.
-
-    CalloutHandlePtr callout_handle;
-    static Pkt4Ptr old_pointer;
-
-    if (!callout_handle ||
-        old_pointer != pkt) {
-        // This is the first packet or a different packet than previously
-        // passed to getCalloutHandle()
-
-        // Remember the pointer to this packet
-        old_pointer = pkt;
-
-        callout_handle = HooksManager::createCalloutHandle();
-    }
-
-    return (callout_handle);
-}
-
 void
 Dhcpv4Srv::openActiveSockets(const uint16_t port,
                              const bool use_bcast) {

+ 0 - 7
src/bin/dhcp4/dhcp4_srv.h

@@ -366,13 +366,6 @@ private:
     uint16_t port_;  ///< UDP port number on which server listens.
     bool use_bcast_; ///< Should broadcast be enabled on sockets (if true).
 
-    /// @brief returns callout handle for specified packet
-    ///
-    /// @param pkt packet for which the handle should be returned
-    ///
-    /// @return a callout handle to be used in hooks related to said packet
-    isc::hooks::CalloutHandlePtr getCalloutHandle(const Pkt4Ptr& pkt);
-
     /// Indexes for registered hook points
     int hook_index_pkt4_receive_;
     int hook_index_subnet4_select_;

+ 678 - 25
src/bin/dhcp4/tests/dhcp4_srv_unittest.cc

@@ -1631,21 +1631,33 @@ TEST_F(Dhcpv4SrvTest, Hooks) {
     NakedDhcpv4Srv srv(0);
 
     // check if appropriate hooks are registered
-    int hook_index_pkt4_received = -1;
-    int hook_index_select_subnet = -1;
-    int hook_index_pkt4_send     = -1;
+    int hook_index_buffer4_receive = -1;
+    int hook_index_pkt4_receive    = -1;
+    int hook_index_select_subnet   = -1;
+    int hook_index_lease4_release  = -1;
+    int hook_index_pkt4_send       = -1;
+    int hook_index_buffer4_send    = -1;
 
     // check if appropriate indexes are set
-    EXPECT_NO_THROW(hook_index_pkt4_received = ServerHooks::getServerHooks()
+    EXPECT_NO_THROW(hook_index_buffer4_receive = ServerHooks::getServerHooks()
+                    .getIndex("buffer4_receive"));
+    EXPECT_NO_THROW(hook_index_pkt4_receive = ServerHooks::getServerHooks()
                     .getIndex("pkt4_receive"));
     EXPECT_NO_THROW(hook_index_select_subnet = ServerHooks::getServerHooks()
                     .getIndex("subnet4_select"));
-    EXPECT_NO_THROW(hook_index_pkt4_send     = ServerHooks::getServerHooks()
+    EXPECT_NO_THROW(hook_index_lease4_release = ServerHooks::getServerHooks()
+                    .getIndex("lease4_release"));
+    EXPECT_NO_THROW(hook_index_pkt4_send = ServerHooks::getServerHooks()
                     .getIndex("pkt4_send"));
+    EXPECT_NO_THROW(hook_index_buffer4_send = ServerHooks::getServerHooks()
+                    .getIndex("buffer4_send"));
 
-    EXPECT_TRUE(hook_index_pkt4_received > 0);
+    EXPECT_TRUE(hook_index_buffer4_receive > 0);
+    EXPECT_TRUE(hook_index_pkt4_receive > 0);
     EXPECT_TRUE(hook_index_select_subnet > 0);
+    EXPECT_TRUE(hook_index_lease4_release > 0);
     EXPECT_TRUE(hook_index_pkt4_send > 0);
+    EXPECT_TRUE(hook_index_buffer4_send > 0);
 }
 
     // a dummy MAC address
@@ -1689,9 +1701,13 @@ public:
     /// @brief destructor (deletes Dhcpv4Srv)
     virtual ~HooksDhcpv4SrvTest() {
 
+        HooksManager::preCalloutsLibraryHandle().deregisterAllCallouts("buffer4_receive");
+        HooksManager::preCalloutsLibraryHandle().deregisterAllCallouts("buffer4_send");
         HooksManager::preCalloutsLibraryHandle().deregisterAllCallouts("pkt4_receive");
         HooksManager::preCalloutsLibraryHandle().deregisterAllCallouts("pkt4_send");
         HooksManager::preCalloutsLibraryHandle().deregisterAllCallouts("subnet4_select");
+        HooksManager::preCalloutsLibraryHandle().deregisterAllCallouts("lease4_renew");
+        HooksManager::preCalloutsLibraryHandle().deregisterAllCallouts("lease4_release");
 
         delete srv_;
     }
@@ -1766,6 +1782,67 @@ public:
         return (Pkt4Ptr(new Pkt4(&buf[0], buf.size())));
     }
 
+    /// Test callback that stores received callout name and pkt4 value
+    /// @param callout_handle handle passed by the hooks framework
+    /// @return always 0
+    static int
+    buffer4_receive_callout(CalloutHandle& callout_handle) {
+        callback_name_ = string("buffer4_receive");
+
+        callout_handle.getArgument("query4", callback_pkt4_);
+
+        callback_argument_names_ = callout_handle.getArgumentNames();
+        return (0);
+    }
+
+    /// Test callback that changes hwaddr value
+    /// @param callout_handle handle passed by the hooks framework
+    /// @return always 0
+    static int
+    buffer4_receive_change_hwaddr(CalloutHandle& callout_handle) {
+
+        Pkt4Ptr pkt;
+        callout_handle.getArgument("query4", pkt);
+
+        // If there is at least one option with data
+        if (pkt->data_.size() >= Pkt4::DHCPV4_PKT_HDR_LEN) {
+            // Offset of the first byte of the CHWADDR field. Let's the first
+            // byte to some new value that we could later check
+            pkt->data_[28] = 0xff;
+        }
+
+        // Carry on as usual
+        return buffer4_receive_callout(callout_handle);
+    }
+
+    /// Test callback that deletes MAC address
+    /// @param callout_handle handle passed by the hooks framework
+    /// @return always 0
+    static int
+    buffer4_receive_delete_hwaddr(CalloutHandle& callout_handle) {
+
+        Pkt4Ptr pkt;
+        callout_handle.getArgument("query4", pkt);
+
+        pkt->data_[2] = 0; // offset 2 is hlen, let's set it to zero
+        memset(&pkt->data_[28], 0, Pkt4::MAX_CHADDR_LEN); // Clear CHADDR content
+
+        // carry on as usual
+        return buffer4_receive_callout(callout_handle);
+    }
+
+    /// Test callback that sets skip flag
+    /// @param callout_handle handle passed by the hooks framework
+    /// @return always 0
+    static int
+    buffer4_receive_skip(CalloutHandle& callout_handle) {
+
+        callout_handle.setSkip(true);
+
+        // Carry on as usual
+        return buffer4_receive_callout(callout_handle);
+    }
+
     /// test callback that stores received callout name and pkt4 value
     /// @param callout_handle handle passed by the hooks framework
     /// @return always 0
@@ -1894,6 +1971,46 @@ public:
         return pkt4_send_callout(callout_handle);
     }
 
+    /// Test callback that stores received callout name and pkt4 value
+    /// @param callout_handle handle passed by the hooks framework
+    /// @return always 0
+    static int
+    buffer4_send_callout(CalloutHandle& callout_handle) {
+        callback_name_ = string("buffer4_send");
+
+        callout_handle.getArgument("response4", callback_pkt4_);
+
+        callback_argument_names_ = callout_handle.getArgumentNames();
+        return (0);
+    }
+
+    /// Test callback changes the output buffer to a hardcoded value
+    /// @param callout_handle handle passed by the hooks framework
+    /// @return always 0
+    static int
+    buffer4_send_change_callout(CalloutHandle& callout_handle) {
+
+        Pkt4Ptr pkt;
+        callout_handle.getArgument("response4", pkt);
+
+        // modify buffer to set a diffferent payload
+        pkt->getBuffer().clear();
+        pkt->getBuffer().writeData(dummyFile, sizeof(dummyFile));
+
+        return (0);
+    }
+
+    /// Test callback that stores received callout name and pkt4 value
+    /// @param callout_handle handle passed by the hooks framework
+    /// @return always 0
+    static int
+    skip_callout(CalloutHandle& callout_handle) {
+
+        callout_handle.setSkip(true);
+
+        return (0);
+    }
+
     /// Test callback that stores received callout name and subnet4 values
     /// @param callout_handle handle passed by the hooks framework
     /// @return always 0
@@ -1932,10 +2049,44 @@ public:
         return (0);
     }
 
+    /// Test callback that stores received callout name passed parameters
+    /// @param callout_handle handle passed by the hooks framework
+    /// @return always 0
+    static int
+    lease4_release_callout(CalloutHandle& callout_handle) {
+        callback_name_ = string("lease4_release");
+
+        callout_handle.getArgument("query4", callback_pkt4_);
+        callout_handle.getArgument("lease4", callback_lease4_);
+
+        callback_argument_names_ = callout_handle.getArgumentNames();
+        return (0);
+    }
+
+    /// Test callback that stores received callout name and subnet4 values
+    /// @param callout_handle handle passed by the hooks framework
+    /// @return always 0
+    static int
+    lease4_renew_callout(CalloutHandle& callout_handle) {
+        callback_name_ = string("lease4_renew");
+
+        callout_handle.getArgument("subnet4", callback_subnet4_);
+        callout_handle.getArgument("lease4", callback_lease4_);
+        callout_handle.getArgument("hwaddr", callback_hwaddr_);
+        callout_handle.getArgument("clientid", callback_clientid_);
+
+        callback_argument_names_ = callout_handle.getArgumentNames();
+        return (0);
+    }
+
+
     /// resets buffers used to store data received by callouts
     void resetCalloutBuffers() {
         callback_name_ = string("");
         callback_pkt4_.reset();
+        callback_lease4_.reset();
+        callback_hwaddr_.reset();
+        callback_clientid_.reset();
         callback_subnet4_.reset();
         callback_subnet4collection_ = NULL;
         callback_argument_names_.clear();
@@ -1952,6 +2103,15 @@ public:
     /// Pkt4 structure returned in the callout
     static Pkt4Ptr callback_pkt4_;
 
+    /// Lease4 structure returned in the callout
+    static Lease4Ptr callback_lease4_;
+
+    /// Hardware address returned in the callout
+    static HWAddrPtr callback_hwaddr_;
+
+    /// Client-id returned in the callout
+    static ClientIdPtr callback_clientid_;
+
     /// Pointer to a subnet received by callout
     static Subnet4Ptr callback_subnet4_;
 
@@ -1967,23 +2127,124 @@ public:
 string HooksDhcpv4SrvTest::callback_name_;
 Pkt4Ptr HooksDhcpv4SrvTest::callback_pkt4_;
 Subnet4Ptr HooksDhcpv4SrvTest::callback_subnet4_;
+HWAddrPtr HooksDhcpv4SrvTest::callback_hwaddr_;
+ClientIdPtr HooksDhcpv4SrvTest::callback_clientid_;
+Lease4Ptr HooksDhcpv4SrvTest::callback_lease4_;
 const Subnet4Collection* HooksDhcpv4SrvTest::callback_subnet4collection_;
 vector<string> HooksDhcpv4SrvTest::callback_argument_names_;
 
+// Checks if callouts installed on pkt4_receive are indeed called and the
+// all necessary parameters are passed.
+//
+// Note that the test name does not follow test naming convention,
+// but the proper hook name is "buffer4_receive".
+TEST_F(HooksDhcpv4SrvTest, Buffer4ReceiveSimple) {
+
+    // Install pkt6_receive_callout
+    EXPECT_NO_THROW(HooksManager::preCalloutsLibraryHandle().registerCallout(
+                        "buffer4_receive", buffer4_receive_callout));
+
+    // Let's create a simple DISCOVER
+    Pkt4Ptr dis = generateSimpleDiscover();
+
+    // Simulate that we have received that traffic
+    srv_->fakeReceive(dis);
+
+    // Server will now process to run its normal loop, but instead of calling
+    // IfaceMgr::receive4(), it will read all packets from the list set by
+    // fakeReceive()
+    // In particular, it should call registered buffer4_receive callback.
+    srv_->run();
+
+    // Check that the callback called is indeed the one we installed
+    EXPECT_EQ("buffer4_receive", callback_name_);
+
+    // Check that pkt4 argument passing was successful and returned proper value
+    EXPECT_TRUE(callback_pkt4_.get() == dis.get());
 
-// Checks if callouts installed on pkt4_received are indeed called and the
+    // Check that all expected parameters are there
+    vector<string> expected_argument_names;
+    expected_argument_names.push_back(string("query4"));
+
+    EXPECT_TRUE(expected_argument_names == callback_argument_names_);
+}
+
+// Checks if callouts installed on buffer4_receive is able to change
+// the values and the parameters are indeed used by the server.
+TEST_F(HooksDhcpv4SrvTest, buffer4RreceiveValueChange) {
+
+    // Install callback that modifies MAC addr of incoming packet
+    EXPECT_NO_THROW(HooksManager::preCalloutsLibraryHandle().registerCallout(
+                        "buffer4_receive", buffer4_receive_change_hwaddr));
+
+    // Let's create a simple DISCOVER
+    Pkt4Ptr discover = generateSimpleDiscover();
+
+    // Simulate that we have received that traffic
+    srv_->fakeReceive(discover);
+
+    // Server will now process to run its normal loop, but instead of calling
+    // IfaceMgr::receive6(), it will read all packets from the list set by
+    // fakeReceive()
+    // In particular, it should call registered buffer4_receive callback.
+    srv_->run();
+
+    // Check that the server did send a reposonce
+    ASSERT_EQ(1, srv_->fake_sent_.size());
+
+    // Make sure that we received a response
+    Pkt4Ptr offer = srv_->fake_sent_.front();
+    ASSERT_TRUE(offer);
+
+    // Get client-id...
+    HWAddrPtr hwaddr = offer->getHWAddr();
+
+    ASSERT_TRUE(hwaddr); // basic sanity check. HWAddr is always present
+
+    // ... and check if it is the modified value
+    ASSERT_FALSE(hwaddr->hwaddr_.empty()); // there must be a MAC address
+    EXPECT_EQ(0xff, hwaddr->hwaddr_[0]); // check that its first byte was modified
+}
+
+// Checks if callouts installed on buffer4_receive is able to set skip flag that
+// will cause the server to not parse the packet. Even though the packet is valid,
+// the server should eventually drop it, because there won't be mandatory options
+// (or rather option objects) in it.
+TEST_F(HooksDhcpv4SrvTest, buffer4ReceiveSkip) {
+
+    // Install pkt6_receive_callout
+    EXPECT_NO_THROW(HooksManager::preCalloutsLibraryHandle().registerCallout(
+                        "buffer4_receive", buffer4_receive_skip));
+
+    // Let's create a simple DISCOVER
+    Pkt4Ptr discover = generateSimpleDiscover();
+
+    // Simulate that we have received that traffic
+    srv_->fakeReceive(discover);
+
+    // Server will now process to run its normal loop, but instead of calling
+    // IfaceMgr::receive6(), it will read all packets from the list set by
+    // fakeReceive()
+    // In particular, it should call registered pkt6_receive callback.
+    srv_->run();
+
+    // Check that the server dropped the packet and did not produce any response
+    ASSERT_EQ(0, srv_->fake_sent_.size());
+}
+
+// Checks if callouts installed on pkt4_receive are indeed called and the
 // all necessary parameters are passed.
 //
 // Note that the test name does not follow test naming convention,
 // but the proper hook name is "pkt4_receive".
-TEST_F(HooksDhcpv4SrvTest, simple_pkt4_receive) {
+TEST_F(HooksDhcpv4SrvTest, pkt4ReceiveSimple) {
 
     // Install pkt4_receive_callout
     EXPECT_NO_THROW(HooksManager::preCalloutsLibraryHandle().registerCallout(
                         "pkt4_receive", pkt4_receive_callout));
 
     // Let's create a simple DISCOVER
-    Pkt4Ptr sol = Pkt4Ptr(generateSimpleDiscover());
+    Pkt4Ptr sol = generateSimpleDiscover();
 
     // Simulate that we have received that traffic
     srv_->fakeReceive(sol);
@@ -2016,7 +2277,7 @@ TEST_F(HooksDhcpv4SrvTest, valueChange_pkt4_receive) {
                         "pkt4_receive", pkt4_receive_change_clientid));
 
     // Let's create a simple DISCOVER
-    Pkt4Ptr sol = Pkt4Ptr(generateSimpleDiscover());
+    Pkt4Ptr sol = generateSimpleDiscover();
 
     // Simulate that we have received that traffic
     srv_->fakeReceive(sol);
@@ -2045,14 +2306,14 @@ TEST_F(HooksDhcpv4SrvTest, valueChange_pkt4_receive) {
 // Checks if callouts installed on pkt4_received is able to delete
 // existing options and that change impacts server processing (mandatory
 // client-id option is deleted, so the packet is expected to be dropped)
-TEST_F(HooksDhcpv4SrvTest, deleteClientId_pkt4_receive) {
+TEST_F(HooksDhcpv4SrvTest, pkt4ReceiveDeleteClientId) {
 
     // Install pkt4_receive_callout
     EXPECT_NO_THROW(HooksManager::preCalloutsLibraryHandle().registerCallout(
                         "pkt4_receive", pkt4_receive_delete_clientid));
 
     // Let's create a simple DISCOVER
-    Pkt4Ptr sol = Pkt4Ptr(generateSimpleDiscover());
+    Pkt4Ptr sol = generateSimpleDiscover();
 
     // Simulate that we have received that traffic
     srv_->fakeReceive(sol);
@@ -2069,14 +2330,14 @@ TEST_F(HooksDhcpv4SrvTest, deleteClientId_pkt4_receive) {
 
 // Checks if callouts installed on pkt4_received is able to set skip flag that
 // will cause the server to not process the packet (drop), even though it is valid.
-TEST_F(HooksDhcpv4SrvTest, skip_pkt4_receive) {
+TEST_F(HooksDhcpv4SrvTest, pkt4ReceiveSkip) {
 
     // Install pkt4_receive_callout
     EXPECT_NO_THROW(HooksManager::preCalloutsLibraryHandle().registerCallout(
                         "pkt4_receive", pkt4_receive_skip));
 
     // Let's create a simple DISCOVER
-    Pkt4Ptr sol = Pkt4Ptr(generateSimpleDiscover());
+    Pkt4Ptr sol = generateSimpleDiscover();
 
     // Simulate that we have received that traffic
     srv_->fakeReceive(sol);
@@ -2094,14 +2355,14 @@ TEST_F(HooksDhcpv4SrvTest, skip_pkt4_receive) {
 
 // Checks if callouts installed on pkt4_send are indeed called and the
 // all necessary parameters are passed.
-TEST_F(HooksDhcpv4SrvTest, simple_pkt4_send) {
+TEST_F(HooksDhcpv4SrvTest, pkt4SendSimple) {
 
     // Install pkt4_receive_callout
     EXPECT_NO_THROW(HooksManager::preCalloutsLibraryHandle().registerCallout(
                         "pkt4_send", pkt4_send_callout));
 
     // Let's create a simple DISCOVER
-    Pkt4Ptr sol = Pkt4Ptr(generateSimpleDiscover());
+    Pkt4Ptr sol = generateSimpleDiscover();
 
     // Simulate that we have received that traffic
     srv_->fakeReceive(sol);
@@ -2130,14 +2391,14 @@ TEST_F(HooksDhcpv4SrvTest, simple_pkt4_send) {
 
 // Checks if callouts installed on pkt4_send is able to change
 // the values and the packet sent contains those changes
-TEST_F(HooksDhcpv4SrvTest, valueChange_pkt4_send) {
+TEST_F(HooksDhcpv4SrvTest, pkt4SendValueChange) {
 
     // Install pkt4_receive_callout
     EXPECT_NO_THROW(HooksManager::preCalloutsLibraryHandle().registerCallout(
                         "pkt4_send", pkt4_send_change_serverid));
 
     // Let's create a simple DISCOVER
-    Pkt4Ptr sol = Pkt4Ptr(generateSimpleDiscover());
+    Pkt4Ptr sol = generateSimpleDiscover();
 
     // Simulate that we have received that traffic
     srv_->fakeReceive(sol);
@@ -2167,14 +2428,14 @@ TEST_F(HooksDhcpv4SrvTest, valueChange_pkt4_send) {
 // existing options and that server applies those changes. In particular,
 // we are trying to send a packet without server-id. The packet should
 // be sent
-TEST_F(HooksDhcpv4SrvTest, deleteServerId_pkt4_send) {
+TEST_F(HooksDhcpv4SrvTest, pkt4SendDeleteServerId) {
 
     // Install pkt4_receive_callout
     EXPECT_NO_THROW(HooksManager::preCalloutsLibraryHandle().registerCallout(
                         "pkt4_send", pkt4_send_delete_serverid));
 
     // Let's create a simple DISCOVER
-    Pkt4Ptr sol = Pkt4Ptr(generateSimpleDiscover());
+    Pkt4Ptr sol = generateSimpleDiscover();
 
     // Simulate that we have received that traffic
     srv_->fakeReceive(sol);
@@ -2205,7 +2466,7 @@ TEST_F(HooksDhcpv4SrvTest, skip_pkt4_send) {
                         "pkt4_send", pkt4_send_skip));
 
     // Let's create a simple REQUEST
-    Pkt4Ptr sol = Pkt4Ptr(generateSimpleDiscover());
+    Pkt4Ptr sol = generateSimpleDiscover();
 
     // Simulate that we have received that traffic
     srv_->fakeReceive(sol);
@@ -2213,16 +2474,111 @@ TEST_F(HooksDhcpv4SrvTest, skip_pkt4_send) {
     // Server will now process to run its normal loop, but instead of calling
     // IfaceMgr::receive4(), it will read all packets from the list set by
     // fakeReceive()
+    // In particular, it should call registered pkt4_send callback.
+    srv_->run();
+
+    // Check that the server sent the message
+    ASSERT_EQ(1, srv_->fake_sent_.size());
+
+    // Get the first packet and check that it has zero length (i.e. the server
+    // did not do packing on its own)
+    Pkt4Ptr sent = srv_->fake_sent_.front();
+    EXPECT_EQ(0, sent->getBuffer().getLength());
+}
+
+// Checks if callouts installed on buffer4_send are indeed called and the
+// all necessary parameters are passed.
+TEST_F(HooksDhcpv4SrvTest, buffer4SendSimple) {
+
+    // Install pkt4_receive_callout
+    EXPECT_NO_THROW(HooksManager::preCalloutsLibraryHandle().registerCallout(
+                        "buffer4_send", buffer4_send_callout));
+
+    // Let's create a simple DISCOVER
+    Pkt4Ptr discover = generateSimpleDiscover();
+
+    // Simulate that we have received that traffic
+    srv_->fakeReceive(discover);
+
+    // Server will now process to run its normal loop, but instead of calling
+    // IfaceMgr::receive4(), it will read all packets from the list set by
+    // fakeReceive()
     // In particular, it should call registered pkt4_receive callback.
     srv_->run();
 
-    // check that the server dropped the packet and did not produce any response
+    // Check that the callback called is indeed the one we installed
+    EXPECT_EQ("buffer4_send", callback_name_);
+
+    // Check that there is one packet sent
+    ASSERT_EQ(1, srv_->fake_sent_.size());
+    Pkt4Ptr adv = srv_->fake_sent_.front();
+
+    // Check that pkt4 argument passing was successful and returned proper value
+    EXPECT_TRUE(callback_pkt4_.get() == adv.get());
+
+    // Check that all expected parameters are there
+    vector<string> expected_argument_names;
+    expected_argument_names.push_back(string("response4"));
+    EXPECT_TRUE(expected_argument_names == callback_argument_names_);
+}
+
+// Checks if callouts installed on buffer4_send are indeed called and that
+// the output buffer can be changed.
+TEST_F(HooksDhcpv4SrvTest, buffer4Send) {
+
+    // Install pkt4_receive_callout
+    EXPECT_NO_THROW(HooksManager::preCalloutsLibraryHandle().registerCallout(
+                        "buffer4_send", buffer4_send_change_callout));
+
+    // Let's create a simple DISCOVER
+    Pkt4Ptr discover = generateSimpleDiscover();
+
+    // Simulate that we have received that traffic
+    srv_->fakeReceive(discover);
+
+    // Server will now process to run its normal loop, but instead of calling
+    // IfaceMgr::receive4(), it will read all packets from the list set by
+    // fakeReceive()
+    // In particular, it should call registered pkt4_receive callback.
+    srv_->run();
+
+    // Check that there is one packet sent
+    ASSERT_EQ(1, srv_->fake_sent_.size());
+    Pkt4Ptr adv = srv_->fake_sent_.front();
+
+    // The callout is supposed to fill the output buffer with dummyFile content
+    ASSERT_EQ(sizeof(dummyFile), adv->getBuffer().getLength());
+    EXPECT_EQ(0, memcmp(adv->getBuffer().getData(), dummyFile, sizeof(dummyFile)));
+}
+
+// Checks if callouts installed on buffer4_send can set skip flag and that flag
+// causes the packet to not be sent
+TEST_F(HooksDhcpv4SrvTest, buffer4SendSkip) {
+
+    // Install pkt4_receive_callout
+    EXPECT_NO_THROW(HooksManager::preCalloutsLibraryHandle().registerCallout(
+                        "buffer4_send", skip_callout));
+
+    // Let's create a simple DISCOVER
+    Pkt4Ptr discover = generateSimpleDiscover();
+
+    // Simulate that we have received that traffic
+    srv_->fakeReceive(discover);
+
+    // Server will now process to run its normal loop, but instead of calling
+    // IfaceMgr::receive4(), it will read all packets from the list set by
+    // fakeReceive()
+    // In particular, it should call registered pkt4_receive callback.
+    srv_->run();
+
+    // Check that there is no packet sent.
     ASSERT_EQ(0, srv_->fake_sent_.size());
 }
 
+
 // This test checks if subnet4_select callout is triggered and reports
 // valid parameters
-TEST_F(HooksDhcpv4SrvTest, subnet4_select) {
+TEST_F(HooksDhcpv4SrvTest, subnet4SelectSimple) {
 
     // Install pkt4_receive_callout
     EXPECT_NO_THROW(HooksManager::preCalloutsLibraryHandle().registerCallout(
@@ -2288,9 +2644,9 @@ TEST_F(HooksDhcpv4SrvTest, subnet4_select) {
 
 // This test checks if callout installed on subnet4_select hook point can pick
 // a different subnet.
-TEST_F(HooksDhcpv4SrvTest, subnet_select_change) {
+TEST_F(HooksDhcpv4SrvTest, subnet4SelectChange) {
 
-    // Install pkt4_receive_callout
+    // Install a callout
     EXPECT_NO_THROW(HooksManager::preCalloutsLibraryHandle().registerCallout(
                         "subnet4_select", subnet4_select_different_subnet_callout));
 
@@ -2345,6 +2701,303 @@ TEST_F(HooksDhcpv4SrvTest, subnet_select_change) {
     EXPECT_TRUE((*subnets)[1]->inPool(addr));
 }
 
+// This test verifies that incoming (positive) REQUEST/Renewing can be handled
+// properly and that callout installed on lease4_renew is triggered with
+// expected parameters.
+TEST_F(HooksDhcpv4SrvTest, lease4RenewSimple) {
 
+    const IOAddress addr("192.0.2.106");
+    const uint32_t temp_t1 = 50;
+    const uint32_t temp_t2 = 75;
+    const uint32_t temp_valid = 100;
+    const time_t temp_timestamp = time(NULL) - 10;
+
+    // Install a callout
+    EXPECT_NO_THROW(HooksManager::preCalloutsLibraryHandle().registerCallout(
+                        "lease4_renew", lease4_renew_callout));
+
+    // Generate client-id also sets client_id_ member
+    OptionPtr clientid = generateClientId();
+
+    // Check that the address we are about to use is indeed in pool
+    ASSERT_TRUE(subnet_->inPool(addr));
+
+    // let's create a lease and put it in the LeaseMgr
+    uint8_t hwaddr2[] = { 0, 0xfe, 0xfe, 0xfe, 0xfe, 0xfe};
+    Lease4Ptr used(new Lease4(IOAddress("192.0.2.106"), hwaddr2, sizeof(hwaddr2),
+                              &client_id_->getDuid()[0], client_id_->getDuid().size(),
+                              temp_valid, temp_t1, temp_t2, temp_timestamp,
+                              subnet_->getID()));
+    ASSERT_TRUE(LeaseMgrFactory::instance().addLease(used));
+
+    // Check that the lease is really in the database
+    Lease4Ptr l = LeaseMgrFactory::instance().getLease4(addr);
+    ASSERT_TRUE(l);
+
+    // Let's create a RENEW
+    Pkt4Ptr req = Pkt4Ptr(new Pkt4(DHCPREQUEST, 1234));
+    req->setRemoteAddr(IOAddress(addr));
+    req->setYiaddr(addr);
+    req->setCiaddr(addr); // client's address
+
+    req->addOption(clientid);
+    req->addOption(srv_->getServerID());
+
+    // Pass it to the server and hope for a REPLY
+    Pkt4Ptr ack = srv_->processRequest(req);
+
+    // Check if we get response at all
+    checkResponse(ack, DHCPACK, 1234);
+
+    // Check that the lease is really in the database
+    l = checkLease(ack, clientid, req->getHWAddr(), addr);
+    ASSERT_TRUE(l);
+
+    // Check that T1, T2, preferred, valid and cltt were really updated
+    EXPECT_EQ(l->t1_, subnet_->getT1());
+    EXPECT_EQ(l->t2_, subnet_->getT2());
+    EXPECT_EQ(l->valid_lft_, subnet_->getValid());
+
+    // Check that the callback called is indeed the one we installed
+    EXPECT_EQ("lease4_renew", callback_name_);
+
+    // Check that hwaddr parameter is passed properly
+    ASSERT_TRUE(callback_hwaddr_);
+    EXPECT_TRUE(*callback_hwaddr_ == *req->getHWAddr());
+
+    // Check that the subnet is passed properly
+    ASSERT_TRUE(callback_subnet4_);
+    EXPECT_EQ(callback_subnet4_->toText(), subnet_->toText());
+
+    ASSERT_TRUE(callback_clientid_);
+    ASSERT_TRUE(client_id_);
+    EXPECT_TRUE(*client_id_ == *callback_clientid_);
+
+    // Check if all expected parameters were really received
+    vector<string> expected_argument_names;
+    expected_argument_names.push_back("subnet4");
+    expected_argument_names.push_back("clientid");
+    expected_argument_names.push_back("hwaddr");
+    expected_argument_names.push_back("lease4");
+    sort(callback_argument_names_.begin(), callback_argument_names_.end());
+    sort(expected_argument_names.begin(), expected_argument_names.end());
+    EXPECT_TRUE(callback_argument_names_ == expected_argument_names);
+
+    EXPECT_TRUE(LeaseMgrFactory::instance().deleteLease(addr));
+}
+
+// This test verifies that a callout installed on lease4_renew can trigger
+// the server to not renew a lease.
+TEST_F(HooksDhcpv4SrvTest, lease4RenewSkip) {
+
+    const IOAddress addr("192.0.2.106");
+    const uint32_t temp_t1 = 50;
+    const uint32_t temp_t2 = 75;
+    const uint32_t temp_valid = 100;
+    const time_t temp_timestamp = time(NULL) - 10;
+
+    // Install a callout
+    EXPECT_NO_THROW(HooksManager::preCalloutsLibraryHandle().registerCallout(
+                        "lease4_renew", skip_callout));
+
+    // Generate client-id also sets client_id_ member
+    OptionPtr clientid = generateClientId();
+
+    // Check that the address we are about to use is indeed in pool
+    ASSERT_TRUE(subnet_->inPool(addr));
+
+    // let's create a lease and put it in the LeaseMgr
+    uint8_t hwaddr2[] = { 0, 0xfe, 0xfe, 0xfe, 0xfe, 0xfe};
+    Lease4Ptr used(new Lease4(IOAddress("192.0.2.106"), hwaddr2, sizeof(hwaddr2),
+                              &client_id_->getDuid()[0], client_id_->getDuid().size(),
+                              temp_valid, temp_t1, temp_t2, temp_timestamp,
+                              subnet_->getID()));
+    ASSERT_TRUE(LeaseMgrFactory::instance().addLease(used));
+
+    // Check that the lease is really in the database
+    Lease4Ptr l = LeaseMgrFactory::instance().getLease4(addr);
+    ASSERT_TRUE(l);
+
+    // Check that T1, T2, preferred, valid and cltt really set.
+    // Constructed lease looks as if it was assigned 10 seconds ago
+    // EXPECT_EQ(l->t1_, temp_t1);
+    // EXPECT_EQ(l->t2_, temp_t2);
+    EXPECT_EQ(l->valid_lft_, temp_valid);
+    EXPECT_EQ(l->cltt_, temp_timestamp);
+
+    // Let's create a RENEW
+    Pkt4Ptr req = Pkt4Ptr(new Pkt4(DHCPREQUEST, 1234));
+    req->setRemoteAddr(IOAddress(addr));
+    req->setYiaddr(addr);
+    req->setCiaddr(addr); // client's address
+
+    req->addOption(clientid);
+    req->addOption(srv_->getServerID());
+
+    // Pass it to the server and hope for a REPLY
+    Pkt4Ptr ack = srv_->processRequest(req);
+    ASSERT_TRUE(ack);
+
+    // Check that the lease is really in the database
+    l = checkLease(ack, clientid, req->getHWAddr(), addr);
+    ASSERT_TRUE(l);
+
+    // Check that T1, T2, valid and cltt were NOT updated
+    EXPECT_EQ(temp_t1, l->t1_);
+    EXPECT_EQ(temp_t2, l->t2_);
+    EXPECT_EQ(temp_valid, l->valid_lft_);
+    EXPECT_EQ(temp_timestamp, l->cltt_);
+
+    EXPECT_TRUE(LeaseMgrFactory::instance().deleteLease(addr));
+}
+
+// This test verifies that valid RELEASE triggers lease4_release callouts
+TEST_F(HooksDhcpv4SrvTest, lease4ReleaseSimple) {
+
+    const IOAddress addr("192.0.2.106");
+    const uint32_t temp_t1 = 50;
+    const uint32_t temp_t2 = 75;
+    const uint32_t temp_valid = 100;
+    const time_t temp_timestamp = time(NULL) - 10;
+
+    // Install a callout
+    EXPECT_NO_THROW(HooksManager::preCalloutsLibraryHandle().registerCallout(
+                        "lease4_release", lease4_release_callout));
+
+    // Generate client-id also duid_
+    OptionPtr clientid = generateClientId();
+
+    // Check that the address we are about to use is indeed in pool
+    ASSERT_TRUE(subnet_->inPool(addr));
+
+    // Let's create a lease and put it in the LeaseMgr
+    uint8_t mac_addr[] = { 0, 0xfe, 0xfe, 0xfe, 0xfe, 0xfe};
+    HWAddrPtr hw(new HWAddr(mac_addr, sizeof(mac_addr), HTYPE_ETHER));
+    Lease4Ptr used(new Lease4(addr, mac_addr, sizeof(mac_addr),
+                              &client_id_->getDuid()[0], client_id_->getDuid().size(),
+                              temp_valid, temp_t1, temp_t2, temp_timestamp,
+                              subnet_->getID()));
+    ASSERT_TRUE(LeaseMgrFactory::instance().addLease(used));
+
+    // Check that the lease is really in the database
+    Lease4Ptr l = LeaseMgrFactory::instance().getLease4(addr);
+    ASSERT_TRUE(l);
+
+    // Let's create a RELEASE
+    // Generate client-id also duid_
+    Pkt4Ptr rel = Pkt4Ptr(new Pkt4(DHCPRELEASE, 1234));
+    rel->setRemoteAddr(addr);
+    rel->setYiaddr(addr);
+    rel->addOption(clientid);
+    rel->addOption(srv_->getServerID());
+    rel->setHWAddr(hw);
+
+    // Pass it to the server and hope for a REPLY
+    // Note: this is no response to RELEASE in DHCPv4
+    EXPECT_NO_THROW(srv_->processRelease(rel));
+
+    // The lease should be gone from LeaseMgr
+    l = LeaseMgrFactory::instance().getLease4(addr);
+    EXPECT_FALSE(l);
+
+    // Try to get the lease by hardware address
+    // @todo: Uncomment this once trac2592 is implemented
+    // Lease4Collection leases = LeaseMgrFactory::instance().getLease4(hw->hwaddr_);
+    // EXPECT_EQ(leases.size(), 0);
+
+    // Try to get it by hw/subnet_id combination
+    l = LeaseMgrFactory::instance().getLease4(hw->hwaddr_, subnet_->getID());
+    EXPECT_FALSE(l);
+
+    // Try by client-id
+    // @todo: Uncomment this once trac2592 is implemented
+    //Lease4Collection leases = LeaseMgrFactory::instance().getLease4(*client_id_);
+    //EXPECT_EQ(leases.size(), 0);
+
+    // Try by client-id/subnet-id
+    l = LeaseMgrFactory::instance().getLease4(*client_id_, subnet_->getID());
+    EXPECT_FALSE(l);
+
+    // Ok, the lease is *really* not there.
+
+    // Check that the callback called is indeed the one we installed
+    EXPECT_EQ("lease4_release", callback_name_);
+
+    // Check that pkt4 argument passing was successful and returned proper value
+    EXPECT_TRUE(callback_pkt4_.get() == rel.get());
+
+    // Check if all expected parameters were really received
+    vector<string> expected_argument_names;
+    expected_argument_names.push_back("query4");
+    expected_argument_names.push_back("lease4");
+    sort(callback_argument_names_.begin(), callback_argument_names_.end());
+    sort(expected_argument_names.begin(), expected_argument_names.end());
+    EXPECT_TRUE(callback_argument_names_ == expected_argument_names);
+}
+
+// This test verifies that skip flag returned by a callout installed on the
+// lease4_release hook point will keep the lease
+TEST_F(HooksDhcpv4SrvTest, lease4ReleaseSkip) {
+
+    const IOAddress addr("192.0.2.106");
+    const uint32_t temp_t1 = 50;
+    const uint32_t temp_t2 = 75;
+    const uint32_t temp_valid = 100;
+    const time_t temp_timestamp = time(NULL) - 10;
+
+    // Install a callout
+    EXPECT_NO_THROW(HooksManager::preCalloutsLibraryHandle().registerCallout(
+                        "lease4_release", skip_callout));
+
+    // Generate client-id also duid_
+    OptionPtr clientid = generateClientId();
+
+    // Check that the address we are about to use is indeed in pool
+    ASSERT_TRUE(subnet_->inPool(addr));
+
+    // Let's create a lease and put it in the LeaseMgr
+    uint8_t mac_addr[] = { 0, 0xfe, 0xfe, 0xfe, 0xfe, 0xfe};
+    HWAddrPtr hw(new HWAddr(mac_addr, sizeof(mac_addr), HTYPE_ETHER));
+    Lease4Ptr used(new Lease4(addr, mac_addr, sizeof(mac_addr),
+                              &client_id_->getDuid()[0], client_id_->getDuid().size(),
+                              temp_valid, temp_t1, temp_t2, temp_timestamp,
+                              subnet_->getID()));
+    ASSERT_TRUE(LeaseMgrFactory::instance().addLease(used));
+
+    // Check that the lease is really in the database
+    Lease4Ptr l = LeaseMgrFactory::instance().getLease4(addr);
+    ASSERT_TRUE(l);
+
+    // Let's create a RELEASE
+    // Generate client-id also duid_
+    Pkt4Ptr rel = Pkt4Ptr(new Pkt4(DHCPRELEASE, 1234));
+    rel->setRemoteAddr(addr);
+    rel->setYiaddr(addr);
+    rel->addOption(clientid);
+    rel->addOption(srv_->getServerID());
+    rel->setHWAddr(hw);
+
+    // Pass it to the server and hope for a REPLY
+    // Note: this is no response to RELEASE in DHCPv4
+    EXPECT_NO_THROW(srv_->processRelease(rel));
+
+    // The lease should be still there
+    l = LeaseMgrFactory::instance().getLease4(addr);
+    EXPECT_TRUE(l);
+
+    // Try by client-id/subnet-id
+    l = LeaseMgrFactory::instance().getLease4(*client_id_, subnet_->getID());
+    EXPECT_TRUE(l);
+
+    // Try to get the lease by hardware address
+    // @todo: Uncomment this once trac2592 is implemented
+    // Lease4Collection leases = LeaseMgrFactory::instance().getLease4(hw->hwaddr_);
+    // EXPECT_EQ(leases.size(), 1);
+
+    // Try by client-id
+    // @todo: Uncomment this once trac2592 is implemented
+    //Lease4Collection leases = LeaseMgrFactory::instance().getLease4(*client_id_);
+    //EXPECT_EQ(leases.size(), 1);
+}
 
 } // end of anonymous namespace

+ 1 - 1
src/bin/dhcp6/dhcp6_messages.mes

@@ -118,7 +118,7 @@ hook point set the skip flag. For this particular hook point, the
 setting of the flag by a callout instructs the server to drop the packet.
 
 % DHCP6_HOOK_BUFFER_SEND_SKIP prepared DHCPv6 response was dropped because a callout set the skip flag.
-This debug message is printed when a callout installed on buffer6_receive
+This debug message is printed when a callout installed on buffer6_send
 hook point set the skip flag. For this particular hook point, the
 setting of the flag by a callout instructs the server to drop the packet.
 Server completed all the processing (e.g. may have assigned, updated

+ 4 - 30
src/bin/dhcp6/dhcp6_srv.cc

@@ -30,17 +30,18 @@
 #include <dhcp/pkt6.h>
 #include <dhcp6/dhcp6_log.h>
 #include <dhcp6/dhcp6_srv.h>
+#include <dhcpsrv/callout_handle_store.h>
 #include <dhcpsrv/cfgmgr.h>
 #include <dhcpsrv/lease_mgr.h>
 #include <dhcpsrv/lease_mgr_factory.h>
 #include <dhcpsrv/subnet.h>
 #include <dhcpsrv/utils.h>
 #include <exceptions/exceptions.h>
+#include <hooks/callout_handle.h>
+#include <hooks/hooks_manager.h>
+#include <util/encode/hex.h>
 #include <util/io_utilities.h>
 #include <util/range_utilities.h>
-#include <util/encode/hex.h>
-#include <hooks/hooks_manager.h>
-#include <hooks/callout_handle.h>
 
 #include <boost/foreach.hpp>
 #include <boost/tokenizer.hpp>
@@ -1810,33 +1811,6 @@ Dhcpv6Srv::processInfRequest(const Pkt6Ptr& infRequest) {
     return reply;
 }
 
-isc::hooks::CalloutHandlePtr Dhcpv6Srv::getCalloutHandle(const Pkt6Ptr& pkt) {
-    // This method returns a CalloutHandle for a given packet. It is guaranteed
-    // to return the same callout_handle (so user library contexts are
-    // preserved). This method works well if the server processes one packet
-    // at a time. Once the server architecture is extended to cover parallel
-    // packets processing (e.g. delayed-ack, some form of buffering etc.), this
-    // method has to be extended (e.g. store callouts in a map and use pkt as
-    // a key). Additional code would be required to release the callout handle
-    // once the server finished processing.
-
-    CalloutHandlePtr callout_handle;
-    static Pkt6Ptr old_pointer;
-
-    if (!callout_handle ||
-        old_pointer != pkt) {
-        // This is the first packet or a different packet than previously
-        // passed to getCalloutHandle()
-
-        // Remember the pointer to this packet
-        old_pointer = pkt;
-
-        callout_handle = HooksManager::getHooksManager().createCalloutHandle();
-    }
-
-    return (callout_handle);
-}
-
 void
 Dhcpv6Srv::openActiveSockets(const uint16_t port) {
     IfaceMgr::instance().closeSockets();

+ 4 - 6
src/bin/dhcp6/dhcp6_srv.h

@@ -473,12 +473,10 @@ private:
     /// initiate server shutdown procedure.
     volatile bool shutdown_;
 
-    /// @brief returns callout handle for specified packet
-    ///
-    /// @param pkt packet for which the handle should be returned
-    ///
-    /// @return a callout handle to be used in hooks related to said packet
-    isc::hooks::CalloutHandlePtr getCalloutHandle(const Pkt6Ptr& pkt);
+    /// Indexes for registered hook points
+    int hook_index_pkt6_receive_;
+    int hook_index_subnet6_select_;
+    int hook_index_pkt6_send_;
 
     /// UDP port number on which server listens.
     uint16_t port_;

+ 2 - 6
src/bin/dhcp6/tests/hooks_unittest.cc

@@ -285,10 +285,6 @@ public:
     /// @return always 0
     static int
     buffer6_receive_skip(CalloutHandle& callout_handle) {
-
-        Pkt6Ptr pkt;
-        callout_handle.getArgument("query6", pkt);
-
         callout_handle.setSkip(true);
 
         // Carry on as usual
@@ -561,7 +557,7 @@ TEST_F(HooksDhcpv6SrvTest, simple_buffer6_receive) {
     // Server will now process to run its normal loop, but instead of calling
     // IfaceMgr::receive6(), it will read all packets from the list set by
     // fakeReceive()
-    // In particular, it should call registered pkt6_receive callback.
+    // In particular, it should call registered buffer6_receive callback.
     srv_->run();
 
     // Check that the callback called is indeed the one we installed
@@ -577,7 +573,7 @@ TEST_F(HooksDhcpv6SrvTest, simple_buffer6_receive) {
     EXPECT_TRUE(expected_argument_names == callback_argument_names_);
 }
 
-// Checks if callouts installed on pkt6_received is able to change
+// Checks if callouts installed on buffer6_receive is able to change
 // the values and the parameters are indeed used by the server.
 TEST_F(HooksDhcpv6SrvTest, valueChange_buffer6_receive) {
 

+ 27 - 27
src/lib/dhcp/pkt4.cc

@@ -33,7 +33,8 @@ namespace dhcp {
 const IOAddress DEFAULT_ADDRESS("0.0.0.0");
 
 Pkt4::Pkt4(uint8_t msg_type, uint32_t transid)
-     :local_addr_(DEFAULT_ADDRESS),
+     :buffer_out_(DHCPV4_PKT_HDR_LEN),
+      local_addr_(DEFAULT_ADDRESS),
       remote_addr_(DEFAULT_ADDRESS),
       iface_(""),
       ifindex_(0),
@@ -48,8 +49,7 @@ Pkt4::Pkt4(uint8_t msg_type, uint32_t transid)
       ciaddr_(DEFAULT_ADDRESS),
       yiaddr_(DEFAULT_ADDRESS),
       siaddr_(DEFAULT_ADDRESS),
-      giaddr_(DEFAULT_ADDRESS),
-      bufferOut_(DHCPV4_PKT_HDR_LEN)
+      giaddr_(DEFAULT_ADDRESS)
 {
     memset(sname_, 0, MAX_SNAME_LEN);
     memset(file_, 0, MAX_FILE_LEN);
@@ -58,7 +58,8 @@ Pkt4::Pkt4(uint8_t msg_type, uint32_t transid)
 }
 
 Pkt4::Pkt4(const uint8_t* data, size_t len)
-     :local_addr_(DEFAULT_ADDRESS),
+     :buffer_out_(0), // not used, this is RX packet
+      local_addr_(DEFAULT_ADDRESS),
       remote_addr_(DEFAULT_ADDRESS),
       iface_(""),
       ifindex_(0),
@@ -72,8 +73,7 @@ Pkt4::Pkt4(const uint8_t* data, size_t len)
       ciaddr_(DEFAULT_ADDRESS),
       yiaddr_(DEFAULT_ADDRESS),
       siaddr_(DEFAULT_ADDRESS),
-      giaddr_(DEFAULT_ADDRESS),
-      bufferOut_(0) // not used, this is RX packet
+      giaddr_(DEFAULT_ADDRESS)
 {
     if (len < DHCPV4_PKT_HDR_LEN) {
         isc_throw(OutOfRange, "Truncated DHCPv4 packet (len=" << len
@@ -111,25 +111,25 @@ Pkt4::pack() {
     try {
         size_t hw_len = hwaddr_->hwaddr_.size();
 
-        bufferOut_.writeUint8(op_);
-        bufferOut_.writeUint8(hwaddr_->htype_);
-        bufferOut_.writeUint8(hw_len < MAX_CHADDR_LEN ? 
+        buffer_out_.writeUint8(op_);
+        buffer_out_.writeUint8(hwaddr_->htype_);
+        buffer_out_.writeUint8(hw_len < MAX_CHADDR_LEN ?
                               hw_len : MAX_CHADDR_LEN);
-        bufferOut_.writeUint8(hops_);
-        bufferOut_.writeUint32(transid_);
-        bufferOut_.writeUint16(secs_);
-        bufferOut_.writeUint16(flags_);
-        bufferOut_.writeUint32(ciaddr_);
-        bufferOut_.writeUint32(yiaddr_);
-        bufferOut_.writeUint32(siaddr_);
-        bufferOut_.writeUint32(giaddr_);
+        buffer_out_.writeUint8(hops_);
+        buffer_out_.writeUint32(transid_);
+        buffer_out_.writeUint16(secs_);
+        buffer_out_.writeUint16(flags_);
+        buffer_out_.writeUint32(ciaddr_);
+        buffer_out_.writeUint32(yiaddr_);
+        buffer_out_.writeUint32(siaddr_);
+        buffer_out_.writeUint32(giaddr_);
 
 
         if (hw_len <= MAX_CHADDR_LEN) {
             // write up to 16 bytes of the hardware address (CHADDR field is 16
             // bytes long in DHCPv4 message).
-            bufferOut_.writeData(&hwaddr_->hwaddr_[0], 
-                                 (hw_len < MAX_CHADDR_LEN ? 
+            buffer_out_.writeData(&hwaddr_->hwaddr_[0],
+                                 (hw_len < MAX_CHADDR_LEN ?
                                   hw_len : MAX_CHADDR_LEN) );
             hw_len = MAX_CHADDR_LEN - hw_len;
         } else {
@@ -138,20 +138,20 @@ Pkt4::pack() {
 
         // write (len) bytes of padding
         vector<uint8_t> zeros(hw_len, 0);
-        bufferOut_.writeData(&zeros[0], hw_len);
-        // bufferOut_.writeData(chaddr_, MAX_CHADDR_LEN);
+        buffer_out_.writeData(&zeros[0], hw_len);
+        // buffer_out_.writeData(chaddr_, MAX_CHADDR_LEN);
 
-        bufferOut_.writeData(sname_, MAX_SNAME_LEN);
-        bufferOut_.writeData(file_, MAX_FILE_LEN);
+        buffer_out_.writeData(sname_, MAX_SNAME_LEN);
+        buffer_out_.writeData(file_, MAX_FILE_LEN);
 
         // write DHCP magic cookie
-        bufferOut_.writeUint32(DHCP_OPTIONS_COOKIE);
+        buffer_out_.writeUint32(DHCP_OPTIONS_COOKIE);
 
-        LibDHCP::packOptions(bufferOut_, options_);
+        LibDHCP::packOptions(buffer_out_, options_);
 
         // add END option that indicates end of options
         // (End option is very simple, just a 255 octet)
-         bufferOut_.writeUint8(DHO_END);
+         buffer_out_.writeUint8(DHO_END);
      } catch(const Exception& e) {
         // An exception is thrown and message will be written to Logger
          isc_throw(InvalidOperation, e.what());
@@ -259,7 +259,7 @@ void Pkt4::setType(uint8_t dhcp_type) {
 }
 
 void Pkt4::repack() {
-    bufferOut_.writeData(&data_[0], data_.size());
+    buffer_out_.writeData(&data_[0], data_.size());
 }
 
 std::string

+ 38 - 28
src/lib/dhcp/pkt4.h

@@ -70,7 +70,7 @@ public:
     ///
     /// Prepares on-wire format of message and all its options.
     /// Options must be stored in options_ field.
-    /// Output buffer will be stored in bufferOut_.
+    /// Output buffer will be stored in buffer_out_.
     ///
     /// @throw InvalidOperation if packing fails
     void
@@ -103,7 +103,7 @@ public:
     ///
     /// This is mostly a diagnostic function. It is being used for sending
     /// received packet. Received packet is stored in bufferIn_, but
-    /// transmitted data is stored in bufferOut_. If we want to send packet
+    /// transmitted data is stored in buffer_out_. If we want to send packet
     /// that we just received, a copy between those two buffers is necessary.
     void repack();
 
@@ -307,11 +307,12 @@ public:
     /// is only valid till Pkt4 object is valid.
     ///
     /// RX packet or TX packet before pack() will return buffer with
-    /// zero length
+    /// zero length. This buffer is returned as non-const, so hooks
+    /// framework (and user's callouts) can modify them if needed
     ///
     /// @return reference to output buffer
-    const isc::util::OutputBuffer&
-    getBuffer() const { return (bufferOut_); };
+    isc::util::OutputBuffer&
+    getBuffer() { return (buffer_out_); };
 
     /// @brief Add an option.
     ///
@@ -489,6 +490,38 @@ public:
     /// @throw isc::Unexpected if timestamp update failed
     void updateTimestamp();
 
+    /// Output buffer (used during message transmission)
+    ///
+    /// @warning This public member is accessed by derived
+    /// classes directly. One of such derived classes is
+    /// @ref perfdhcp::PerfPkt4. The impact on derived clasess'
+    /// behavior must be taken into consideration before making
+    /// changes to this member such as access scope restriction or
+    /// data format change etc. This field is also public, because
+    /// it may be modified by callouts (which are written in C++ now,
+    /// but we expect to also have them in Python, so any accesibility
+    /// methods would overly complicate things here and degrade
+    /// performance).
+    isc::util::OutputBuffer buffer_out_;
+
+    /// @brief That's the data of input buffer used in RX packet.
+    ///
+    /// @note Note that InputBuffer does not store the data itself, but just
+    /// expects that data will be valid for the whole life of InputBuffer.
+    /// Therefore we need to keep the data around.
+    ///
+    /// @warning This public member is accessed by derived
+    /// classes directly. One of such derived classes is
+    /// @ref perfdhcp::PerfPkt4. The impact on derived clasess'
+    /// behavior must be taken into consideration before making
+    /// changes to this member such as access scope restriction or
+    /// data format change etc. This field is also public, because
+    /// it may be modified by callouts (which are written in C++ now,
+    /// but we expect to also have them in Python, so any accesibility
+    /// methods would overly complicate things here and degrade
+    /// performance).
+    std::vector<uint8_t> data_;
+
 private:
 
     /// @brief Generic method that validates and sets HW address.
@@ -591,29 +624,6 @@ protected:
 
     // end of real DHCPv4 fields
 
-    /// output buffer (used during message transmission)
-    ///
-    /// @warning This protected member is accessed by derived
-    /// classes directly. One of such derived classes is
-    /// @ref perfdhcp::PerfPkt4. The impact on derived clasess'
-    /// behavior must be taken into consideration before making
-    /// changes to this member such as access scope restriction or
-    /// data format change etc.
-    isc::util::OutputBuffer bufferOut_;
-
-    /// that's the data of input buffer used in RX packet. Note that
-    /// InputBuffer does not store the data itself, but just expects that
-    /// data will be valid for the whole life of InputBuffer. Therefore we
-    /// need to keep the data around.
-    ///
-    /// @warning This protected member is accessed by derived
-    /// classes directly. One of such derived classes is
-    /// @ref perfdhcp::PerfPkt4. The impact on derived clasess'
-    /// behavior must be taken into consideration before making
-    /// changes to this member such as access scope restriction or
-    /// data format change etc.
-    std::vector<uint8_t> data_;
-
     /// collection of options present in this message
     ///
     /// @warning This protected member is accessed by derived

+ 1 - 0
src/lib/dhcpsrv/Makefile.am

@@ -35,6 +35,7 @@ lib_LTLIBRARIES = libb10-dhcpsrv.la
 libb10_dhcpsrv_la_SOURCES  =
 libb10_dhcpsrv_la_SOURCES += addr_utilities.cc addr_utilities.h
 libb10_dhcpsrv_la_SOURCES += alloc_engine.cc alloc_engine.h
+libb10_dhcpsrv_la_SOURCES += callout_handle_store.h
 libb10_dhcpsrv_la_SOURCES += dbaccess_parser.cc dbaccess_parser.h
 libb10_dhcpsrv_la_SOURCES += dhcpsrv_log.cc dhcpsrv_log.h
 libb10_dhcpsrv_la_SOURCES += cfgmgr.cc cfgmgr.h

+ 55 - 3
src/lib/dhcpsrv/alloc_engine.cc

@@ -31,11 +31,13 @@ namespace {
 /// Structure that holds registered hook indexes
 struct AllocEngineHooks {
     int hook_index_lease4_select_; ///< index for "lease4_receive" hook point
+    int hook_index_lease4_renew_;  ///< index for "lease4_renew" hook point
     int hook_index_lease6_select_; ///< index for "lease6_receive" hook point
 
     /// Constructor that registers hook points for AllocationEngine
     AllocEngineHooks() {
         hook_index_lease4_select_ = HooksManager::registerHook("lease4_select");
+        hook_index_lease4_renew_  = HooksManager::registerHook("lease4_renew");
         hook_index_lease6_select_ = HooksManager::registerHook("lease6_select");
     }
 };
@@ -364,7 +366,7 @@ AllocEngine::allocateAddress4(const SubnetPtr& subnet,
             // its reboot.
             existing = renewLease4(subnet, clientid, hwaddr,
                                    fwd_dns_update, rev_dns_update, hostname,
-                                   existing, fake_allocation);
+                                   existing, callout_handle, fake_allocation);
             if (existing) {
                 return (existing);
             }
@@ -382,7 +384,8 @@ AllocEngine::allocateAddress4(const SubnetPtr& subnet,
                 // its reboot.
                 existing = renewLease4(subnet, clientid, hwaddr,
                                        fwd_dns_update, rev_dns_update,
-                                       hostname, existing, fake_allocation);
+                                       hostname, existing, callout_handle,
+                                       fake_allocation);
                 // @todo: produce a warning. We haven't found him using MAC address, but
                 // we found him using client-id
                 if (existing) {
@@ -495,8 +498,19 @@ Lease4Ptr AllocEngine::renewLease4(const SubnetPtr& subnet,
                                    const bool rev_dns_update,
                                    const std::string& hostname,
                                    const Lease4Ptr& lease,
+                                   const isc::hooks::CalloutHandlePtr& callout_handle,
                                    bool fake_allocation /* = false */) {
 
+    if (!lease) {
+        isc_throw(InvalidOperation, "Lease4 must be specified");
+    }
+
+    // Let's keep the old data. This is essential if we are using memfile
+    // (the lease returned points directly to the lease4 object in the database)
+    // We'll need it if we want to skip update (i.e. roll back renewal)
+    /// @todo: remove this once #3083 is implemented
+    Lease4 old_values = *lease;
+
     lease->subnet_id_ = subnet->getID();
     lease->hwaddr_ = hwaddr->hwaddr_;
     lease->client_id_ = clientid;
@@ -508,10 +522,48 @@ Lease4Ptr AllocEngine::renewLease4(const SubnetPtr& subnet,
     lease->fqdn_rev_ = rev_dns_update;
     lease->hostname_ = hostname;
 
-    if (!fake_allocation) {
+    bool skip = false;
+    // Execute all callouts registered for packet6_send
+    if (HooksManager::getHooksManager().calloutsPresent(Hooks.hook_index_lease4_renew_)) {
+
+        // Delete all previous arguments
+        callout_handle->deleteAllArguments();
+
+        // Subnet from which we do the allocation. Convert the general subnet
+        // pointer to a pointer to a Subnet4.  Note that because we are using
+        // boost smart pointers here, we need to do the cast using the boost
+        // version of dynamic_pointer_cast.
+        Subnet4Ptr subnet4 = boost::dynamic_pointer_cast<Subnet4>(subnet);
+
+        // Pass the parameters
+        callout_handle->setArgument("subnet4", subnet4);
+        callout_handle->setArgument("clientid", clientid);
+        callout_handle->setArgument("hwaddr", hwaddr);
+
+        // Pass the lease to be updated
+        callout_handle->setArgument("lease4", lease);
+
+        // Call all installed callouts
+        HooksManager::callCallouts(Hooks.hook_index_lease4_renew_, *callout_handle);
+
+        // Callouts decided to skip the next processing step. The next
+        // processing step would to actually renew the lease, so skip at this
+        // stage means "keep the old lease as it is".
+        if (callout_handle->getSkip()) {
+            skip = true;
+            LOG_DEBUG(dhcpsrv_logger, DHCPSRV_DBG_HOOKS, DHCPSRV_HOOK_LEASE4_RENEW_SKIP);
+        }
+    }
+
+    if (!fake_allocation && !skip) {
         // for REQUEST we do update the lease
         LeaseMgrFactory::instance().updateLease4(lease);
     }
+    if (skip) {
+        // Rollback changes (really useful only for memfile)
+        /// @todo: remove this once #3083 is implemented
+        *lease = old_values;
+    }
 
     return (lease);
 }

+ 3 - 0
src/lib/dhcpsrv/alloc_engine.h

@@ -258,6 +258,8 @@ protected:
     /// performed for the client (true) or not (false).
     /// @param hostname A string carrying hostname to be used for DNS updates.
     /// @param lease A lease to be renewed
+    /// @param callout_handle a callout handle (used in hooks). A lease callouts
+    ///        will be executed if this parameter is passed.
     /// @param fake_allocation Is this real i.e. REQUEST (false) or just picking
     ///        an address for DISCOVER that is not really allocated (true)
     Lease4Ptr
@@ -268,6 +270,7 @@ protected:
                 const bool rev_dns_update,
                 const std::string& hostname,
                 const Lease4Ptr& lease,
+                const isc::hooks::CalloutHandlePtr& callout_handle,
                 bool fake_allocation /* = false */);
 
     /// @brief Allocates an IPv6 lease

+ 91 - 0
src/lib/dhcpsrv/callout_handle_store.h

@@ -0,0 +1,91 @@
+// Copyright (C) 2013  Internet Systems Consortium, Inc. ("ISC")
+//
+// Permission to use, copy, modify, and/or distribute this software for any
+// purpose with or without fee is hereby granted, provided that the above
+// copyright notice and this permission notice appear in all copies.
+//
+// THE SOFTWARE IS PROVIDED "AS IS" AND ISC DISCLAIMS ALL WARRANTIES WITH
+// REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY
+// AND FITNESS.  IN NO EVENT SHALL ISC BE LIABLE FOR ANY SPECIAL, DIRECT,
+// INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM
+// LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE
+// OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR
+// PERFORMANCE OF THIS SOFTWARE.
+
+#ifndef CALLOUT_HANDLE_STORE_H
+#define CALLOUT_HANDLE_STORE_H
+
+#include <hooks/hooks_manager.h>
+#include <hooks/callout_handle.h>
+
+namespace isc {
+namespace dhcp {
+
+/// @brief CalloutHandle Store
+///
+/// When using the Hooks Framework, there is a need to associate an
+/// isc::hooks::CalloutHandle object with each request passing through the
+/// server.  For the DHCP servers, the association is provided by this function.
+///
+/// The DHCP servers process a single request at a time. At points where the
+/// CalloutHandle is required, the pointer to the current request (packet) is
+/// passed to this function.  If the request is a new one, a pointer to
+/// the request is stored, a new CalloutHandle is allocated (and stored) and
+/// a pointer to the latter object returned to the caller.  If the request
+/// matches the one stored, the pointer to the stored CalloutHandle is
+/// returned.
+///
+/// A special case is a null pointer being passed.  This has the effect of
+/// clearing the stored pointers to the packet being processed and
+/// CalloutHandle.  As the stored pointers are shared pointers, clearing them
+/// removes one reference that keeps the pointed-to objects in existence.
+///
+/// @note If the behaviour of the server changes so that multiple packets can
+///       be active at the same time, this simplistic approach will no longer
+///       be adequate and a more complicated structure (such as a map) will
+///       be needed.
+///
+/// @param pktptr Pointer to the packet being processed.  This is typically a
+///        Pkt4Ptr or Pkt6Ptr object.  An empty pointer is passed to clear
+///        the stored pointers.
+///
+/// @return Shared pointer to a CalloutHandle.  This is the previously-stored
+///         CalloutHandle if pktptr points to a packet that has been seen
+///         before or a new CalloutHandle if it points to a new one.  An empty
+///         pointer is returned if pktptr is itself an empty pointer.
+
+template <typename T>
+isc::hooks::CalloutHandlePtr getCalloutHandle(const T& pktptr) {
+
+    // Stored data is declared static, so is initialized when first accessed
+    static T stored_pointer;                // Pointer to last packet seen
+    static isc::hooks::CalloutHandlePtr stored_handle;
+                                            // Pointer to stored handle
+
+    if (pktptr) {
+
+        // Pointer given, have we seen it before? (If we have, we don't need to
+        // do anything as we will automatically return the stored handle.)
+        if (pktptr != stored_pointer) {
+
+            // Not seen before, so store the pointer passed to us and get a new
+            // CalloutHandle.  (The latter operation frees and probably deletes
+            // (depending on other pointers) the stored one.)
+            stored_pointer = pktptr;
+            stored_handle = isc::hooks::HooksManager::createCalloutHandle();
+        }
+        
+    } else {
+
+        // Empty pointer passed, clear stored data
+        stored_pointer.reset();
+        stored_handle.reset();
+    }
+
+    return (stored_handle);
+}
+
+} // namespace shcp
+} // namespace isc
+
+#endif // CALLOUT_HANDLE_STORE_H

+ 36 - 15
src/lib/dhcpsrv/dhcp_parsers.cc

@@ -47,37 +47,58 @@ ParserContext::ParserContext(Option::Universe universe):
     string_values_(new StringStorage()),
     options_(new OptionStorage()),
     option_defs_(new OptionDefStorage()),
+    hooks_libraries_(),
     universe_(universe)
 {
 }
 
 ParserContext::ParserContext(const ParserContext& rhs):
-    boolean_values_(new BooleanStorage(*(rhs.boolean_values_))),
-    uint32_values_(new Uint32Storage(*(rhs.uint32_values_))),
-    string_values_(new StringStorage(*(rhs.string_values_))),
-    options_(new OptionStorage(*(rhs.options_))),
-    option_defs_(new OptionDefStorage(*(rhs.option_defs_))),
+    boolean_values_(),
+    uint32_values_(),
+    string_values_(),
+    options_(),
+    option_defs_(),
+    hooks_libraries_(),
     universe_(rhs.universe_)
 {
+    copyContext(rhs);
 }
 
 ParserContext&
+// The cppcheck version 1.56 doesn't recognize that copyContext
+// copies all context fields.
+// cppcheck-suppress operatorEqVarError
 ParserContext::operator=(const ParserContext& rhs) {
     if (this != &rhs) {
-        boolean_values_ =
-            BooleanStoragePtr(new BooleanStorage(*(rhs.boolean_values_)));
-        uint32_values_ =
-            Uint32StoragePtr(new Uint32Storage(*(rhs.uint32_values_)));
-        string_values_ =
-            StringStoragePtr(new StringStorage(*(rhs.string_values_)));
-        options_ = OptionStoragePtr(new OptionStorage(*(rhs.options_)));
-        option_defs_ =
-            OptionDefStoragePtr(new OptionDefStorage(*(rhs.option_defs_)));
-        universe_ = rhs.universe_;
+        copyContext(rhs);
     }
+
     return (*this);
 }
 
+void
+ParserContext::copyContext(const ParserContext& ctx) {
+    copyContextPointer(ctx.boolean_values_, boolean_values_);
+    copyContextPointer(ctx.uint32_values_, uint32_values_);
+    copyContextPointer(ctx.string_values_, string_values_);
+    copyContextPointer(ctx.options_, options_);
+    copyContextPointer(ctx.option_defs_, option_defs_);
+    copyContextPointer(ctx.hooks_libraries_, hooks_libraries_);
+    // Copy universe.
+    universe_ = ctx.universe_;
+}
+
+template<typename T>
+void
+ParserContext::copyContextPointer(const boost::shared_ptr<T>& source_ptr,
+                                  boost::shared_ptr<T>& dest_ptr) {
+    if (source_ptr) {
+        dest_ptr.reset(new T(*source_ptr));
+    } else {
+        dest_ptr.reset();
+    }
+}
+
 
 // **************************** DebugParser *************************
 

+ 16 - 3
src/lib/dhcpsrv/dhcp_parsers.h

@@ -45,7 +45,8 @@ typedef OptionSpaceContainer<Subnet::OptionContainer,
 /// @brief Shared pointer to option storage.
 typedef boost::shared_ptr<OptionStorage> OptionStoragePtr;
 
-
+/// @brief Shared pointer to collection of hooks libraries.
+typedef boost::shared_ptr<std::vector<std::string> > HooksLibsStoragePtr;
 
 /// @brief A template class that stores named elements of a given data type.
 ///
@@ -104,7 +105,6 @@ class ValueStorage {
             values_.clear();
         }
 
-
     private:
         /// @brief An std::map of the data values, keyed by parameter names.
         std::map<std::string, ValueType> values_;
@@ -166,13 +166,26 @@ public:
     /// the list of current names can be obtained from the HooksManager) or it
     /// is non-null (this is the new list of names, reload the libraries when
     /// possible).
-    boost::shared_ptr<std::vector<std::string> > hooks_libraries_;
+    HooksLibsStoragePtr hooks_libraries_;
 
     /// @brief The parsing universe of this context.
     Option::Universe universe_;
 
     /// @brief Assignment operator
     ParserContext& operator=(const ParserContext& rhs);
+
+    /// @brief Copy the context fields.
+    ///
+    /// This class method initializes the context data by copying the data
+    /// stored in the context instance provided as an argument. Note that
+    /// this function will also handle copying the NULL pointers.
+    ///
+    /// @param ctx context to be copied.
+    void copyContext(const ParserContext& ctx);
+
+    template<typename T>
+    void copyContextPointer(const boost::shared_ptr<T>& source_ptr,
+                            boost::shared_ptr<T>& dest_ptr);
 };
 
 /// @brief Pointer to various parser context.

+ 6 - 0
src/lib/dhcpsrv/dhcpsrv_messages.mes

@@ -141,6 +141,12 @@ hook point sets the skip flag. It means that the server was told that
 no lease4 should be assigned. The server will not put that lease in its
 database and the client will get a NAK packet.
 
+% DHCPSRV_HOOK_LEASE4_RENEW_SKIP DHCPv4 lease was not renewed because a callout set the skip flag.
+This debug message is printed when a callout installed on lease4_renew
+hook point set the skip flag. For this particular hook point, the setting
+of the flag by a callout instructs the server to not renew a lease. The
+server will use existing lease as it is, without extending its lifetime.
+
 % DHCPSRV_HOOK_LEASE6_SELECT_SKIP Lease6 (non-temporary) creation was skipped, because of callout skip flag.
 This debug message is printed when a callout installed on lease6_select
 hook point sets the skip flag. It means that the server was told that

+ 5 - 0
src/lib/dhcpsrv/tests/Makefile.am

@@ -15,6 +15,7 @@ AM_CXXFLAGS += $(WARNING_NO_MISSING_FIELD_INITIALIZERS_CFLAG)
 
 if USE_STATIC_LINK
 AM_LDFLAGS = -static
+TEST_LIBS_LDFLAGS = -Bshareable
 endif
 
 CLEANFILES = *.gcno *.gcda
@@ -30,10 +31,12 @@ lib_LTLIBRARIES = libco1.la libco2.la
 libco1_la_SOURCES  = callout_library.cc
 libco1_la_CXXFLAGS = $(AM_CXXFLAGS)
 libco1_la_CPPFLAGS = $(AM_CPPFLAGS) $(LOG4CPLUS_INCLUDES)
+libco1_la_LDFLAGS = $(TEST_LIBS_LDFLAGS)
 
 libco2_la_SOURCES  = callout_library.cc
 libco2_la_CXXFLAGS = $(AM_CXXFLAGS)
 libco2_la_CPPFLAGS = $(AM_CPPFLAGS) $(LOG4CPLUS_INCLUDES)
+libco2_la_LDFLAGS = $(TEST_LIBS_LDFLAGS)
 
 
 TESTS += libdhcpsrv_unittests
@@ -41,6 +44,7 @@ TESTS += libdhcpsrv_unittests
 libdhcpsrv_unittests_SOURCES  = run_unittests.cc
 libdhcpsrv_unittests_SOURCES += addr_utilities_unittest.cc
 libdhcpsrv_unittests_SOURCES += alloc_engine_unittest.cc
+libdhcpsrv_unittests_SOURCES += callout_handle_store_unittest.cc
 libdhcpsrv_unittests_SOURCES += cfgmgr_unittest.cc
 libdhcpsrv_unittests_SOURCES += dbaccess_parser_unittest.cc
 libdhcpsrv_unittests_SOURCES += lease_mgr_factory_unittest.cc
@@ -53,6 +57,7 @@ endif
 libdhcpsrv_unittests_SOURCES += pool_unittest.cc
 libdhcpsrv_unittests_SOURCES += schema_copy.h
 libdhcpsrv_unittests_SOURCES += subnet_unittest.cc
+libdhcpsrv_unittests_SOURCES += test_get_callout_handle.cc test_get_callout_handle.h
 libdhcpsrv_unittests_SOURCES += triplet_unittest.cc
 libdhcpsrv_unittests_SOURCES += test_utils.cc test_utils.h
 

+ 8 - 1
src/lib/dhcpsrv/tests/alloc_engine_unittest.cc

@@ -1096,6 +1096,8 @@ TEST_F(AllocEngine4Test, requestReuseExpiredLease4) {
 // called
 TEST_F(AllocEngine4Test, renewLease4) {
     boost::scoped_ptr<AllocEngine> engine;
+    CalloutHandlePtr callout_handle = HooksManager::createCalloutHandle();
+
     ASSERT_NO_THROW(engine.reset(new AllocEngine(AllocEngine::ALLOC_ITERATIVE, 100)));
     ASSERT_TRUE(engine);
 
@@ -1117,7 +1119,8 @@ TEST_F(AllocEngine4Test, renewLease4) {
     // renew it.
     ASSERT_FALSE(lease->expired());
     lease = engine->renewLease4(subnet_, clientid_, hwaddr_, true,
-                                true, "host.example.com.", lease, false);
+                                true, "host.example.com.", lease, 
+                                callout_handle, false);
     // Check that he got that single lease
     ASSERT_TRUE(lease);
     EXPECT_EQ(addr.toText(), lease->addr_.toText());
@@ -1358,6 +1361,10 @@ TEST_F(HookAllocEngine6Test, change_lease6_select) {
 ///
 /// It features a couple of callout functions and buffers to store
 /// the data that is accessible via callouts.
+///
+/// Note: lease4_renew callout is tested from DHCPv4 server.
+/// See HooksDhcpv4SrvTest.basic_lease4_renew in
+/// src/bin/dhcp4/tests/dhcp4_srv_unittest.cc
 class HookAllocEngine4Test : public AllocEngine4Test {
 public:
     HookAllocEngine4Test() {

+ 122 - 0
src/lib/dhcpsrv/tests/callout_handle_store_unittest.cc

@@ -0,0 +1,122 @@
+// Copyright (C) 2012-2013 Internet Systems Consortium, Inc. ("ISC")
+//
+// Permission to use, copy, modify, and/or distribute this software for any
+// purpose with or without fee is hereby granted, provided that the above
+// copyright notice and this permission notice appear in all copies.
+//
+// THE SOFTWARE IS PROVIDED "AS IS" AND ISC DISCLAIMS ALL WARRANTIES WITH
+// REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY
+// AND FITNESS.  IN NO EVENT SHALL ISC BE LIABLE FOR ANY SPECIAL, DIRECT,
+// INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM
+// LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE
+// OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR
+// PERFORMANCE OF THIS SOFTWARE.
+
+#include <config.h>
+
+#include <dhcp/dhcp4.h>
+#include <dhcp/dhcp6.h>
+#include <dhcp/pkt4.h>
+#include <dhcpsrv/callout_handle_store.h>
+#include "test_get_callout_handle.h"
+
+#include <gtest/gtest.h>
+
+using namespace isc;
+using namespace isc::dhcp;
+using namespace isc::hooks;
+
+namespace {
+
+TEST(CalloutHandleStoreTest, StoreRetrieve) {
+
+    // Create two DHCP4 packets during tests.  The constructor arguments are
+    // arbitrary.
+    Pkt4Ptr pktptr_1(new Pkt4(DHCPDISCOVER, 1234));
+    Pkt4Ptr pktptr_2(new Pkt4(DHCPDISCOVER, 5678));
+
+    // Check that the pointers point to objects that are different, and that
+    // the pointers are the only pointers pointing to the packets.
+    ASSERT_TRUE(pktptr_1);
+    ASSERT_TRUE(pktptr_2);
+
+    ASSERT_TRUE(pktptr_1 != pktptr_2);
+    EXPECT_EQ(1, pktptr_1.use_count());
+    EXPECT_EQ(1, pktptr_2.use_count());
+
+    // Get the CalloutHandle for the first packet.
+    CalloutHandlePtr chptr_1 = getCalloutHandle(pktptr_1);
+    ASSERT_TRUE(chptr_1);
+
+    // Reference counts on both the callout handle and the packet should have
+    // been incremented because of the stored data.  The reference count on the
+    // other Pkt4 object should not have changed.
+    EXPECT_EQ(2, chptr_1.use_count());
+    EXPECT_EQ(2, pktptr_1.use_count());
+    EXPECT_EQ(1, pktptr_2.use_count());
+
+    // Try getting another pointer for the same packet.  Use a different
+    // pointer object to check that the function returns a handle based on the
+    // pointed-to data and not the pointer.  (Clear the temporary pointer after
+    // use to avoid complicating reference counts.)
+    Pkt4Ptr pktptr_temp = pktptr_1;
+    CalloutHandlePtr chptr_2 = getCalloutHandle(pktptr_temp);
+    pktptr_temp.reset();
+
+    ASSERT_TRUE(chptr_2);
+    EXPECT_TRUE(chptr_1 == chptr_2);
+
+    // Reference count is now 3 on the callout handle - two for pointers here,
+    // one for the static pointer in the function.  The count is 2 for the]
+    // object pointed to by pktptr_1 - one for that pointer and one for the
+    // pointer in the function.
+    EXPECT_EQ(3, chptr_1.use_count());
+    EXPECT_EQ(3, chptr_2.use_count());
+    EXPECT_EQ(2, pktptr_1.use_count());
+    EXPECT_EQ(1, pktptr_2.use_count());
+
+    // Now ask for a CalloutHandle for a different object.  This should return
+    // a different CalloutHandle.
+    chptr_2 = getCalloutHandle(pktptr_2);
+    EXPECT_FALSE(chptr_1 == chptr_2);
+
+    // Check reference counts.  The getCalloutHandle function should be storing
+    // pointers to the objects poiunted to by chptr_2 and pktptr_2.
+    EXPECT_EQ(1, chptr_1.use_count());
+    EXPECT_EQ(1, pktptr_1.use_count());
+    EXPECT_EQ(2, chptr_2.use_count());
+    EXPECT_EQ(2, pktptr_2.use_count());
+
+    // Now try clearing the stored pointers.
+    Pkt4Ptr pktptr_empty;
+    ASSERT_FALSE(pktptr_empty);
+
+    CalloutHandlePtr chptr_empty = getCalloutHandle(pktptr_empty);
+    EXPECT_FALSE(chptr_empty);
+
+    // Reference counts should be back to 1 for the CalloutHandles and the
+    // Packet pointers.
+    EXPECT_EQ(1, chptr_1.use_count());
+    EXPECT_EQ(1, pktptr_1.use_count());
+    EXPECT_EQ(1, chptr_2.use_count());
+    EXPECT_EQ(1, pktptr_2.use_count());
+}
+
+// The followings is a trival test to check that if the template function
+// is referred to in a separate compilation unit, only one copy of the static
+// objects stored in it are returned.  (For a change, we'll use a Pkt6 as the
+// packet object.)
+
+TEST(CalloutHandleStoreTest, SeparateCompilationUnit) {
+
+    // Access the template function here.
+    Pkt6Ptr pktptr_1(new Pkt6(DHCPV6_ADVERTISE, 4321));
+    CalloutHandlePtr chptr_1 = getCalloutHandle(pktptr_1);
+    ASSERT_TRUE(chptr_1);
+
+    // Access it from within another compilation unit.
+    CalloutHandlePtr chptr_2 = isc::dhcp::test::testGetCalloutHandle(pktptr_1);
+    EXPECT_TRUE(chptr_1 == chptr_2);
+}
+
+} // Anonymous namespace

+ 307 - 1
src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc

@@ -397,7 +397,7 @@ public:
     ///
     /// @return retuns 0 if the configuration parsed successfully,
     /// non-zero otherwise failure.
-    int parseConfiguration(const std::string& config) {    
+    int parseConfiguration(const std::string& config) {
         int rcode_ = 1;
         // Turn config into elements.
         // Test json just to make sure its valid.
@@ -692,3 +692,309 @@ TEST_F(ParseConfigTest, invalidHooksLibrariesTest) {
     EXPECT_FALSE(error_text_.find(NOT_PRESENT_LIBRARY) == string::npos) <<
         "Error text returned from parse failure is " << error_text_;
 }
+
+/// @brief DHCP Configuration Parser Context test fixture.
+class ParserContextTest : public ::testing::Test {
+public:
+    /// @brief Constructor
+    ParserContextTest() { }
+
+    /// @brief Check that the storages of the specific type hold the
+    /// same value.
+    ///
+    /// This function assumes that the ref_values storage holds exactly
+    /// one parameter called 'foo'.
+    ///
+    /// @param ref_values A storage holding reference value. In the typical
+    /// case it is a storage held in the original context, which is assigned
+    /// to another context.
+    /// @param values A storage holding value to be checked.
+    /// @tparam ContainerType A type of the storage.
+    /// @tparam ValueType A type of the value in the container.
+    template<typename ContainerType, typename ValueType>
+    void checkValueEq(const boost::shared_ptr<ContainerType>& ref_values,
+                      const boost::shared_ptr<ContainerType>& values) {
+        ValueType param;
+        ASSERT_NO_THROW(param = values->getParam("foo"));
+        EXPECT_EQ(ref_values->getParam("foo"), param);
+    }
+
+    /// @brief Check that the storages of the specific type hold different
+    /// value.
+    ///
+    /// This function assumes that the ref_values storage holds exactly
+    /// one parameter called 'foo'.
+    ///
+    /// @param ref_values A storage holding reference value. In the typical
+    /// case it is a storage held in the original context, which is assigned
+    /// to another context.
+    /// @param values A storage holding value to be checked.
+    /// @tparam ContainerType A type of the storage.
+    /// @tparam ValueType A type of the value in the container.
+    template<typename ContainerType, typename ValueType>
+    void checkValueNeq(const boost::shared_ptr<ContainerType>& ref_values,
+                       const boost::shared_ptr<ContainerType>& values) {
+        ValueType param;
+        ASSERT_NO_THROW(param = values->getParam("foo"));
+        EXPECT_NE(ref_values->getParam("foo"), param);
+    }
+
+    /// @brief Check that option definition storage in the context holds
+    /// one option definition of the specified type.
+    ///
+    /// @param ctx A pointer to a context.
+    /// @param opt_type Expected option type.
+    void checkOptionDefinitionType(const ParserContext& ctx,
+                                   const uint16_t opt_type) {
+        OptionDefContainerPtr opt_defs =
+            ctx.option_defs_->getItems("option-space");
+        ASSERT_TRUE(opt_defs);
+        OptionDefContainerTypeIndex& idx = opt_defs->get<1>();
+        OptionDefContainerTypeRange range = idx.equal_range(opt_type);
+        EXPECT_EQ(1, std::distance(range.first, range.second));
+    }
+
+    /// @brief Check that option storage in the context holds one option
+    /// of the specified type.
+    ///
+    /// @param ctx A pointer to a context.
+    /// @param opt_type Expected option type.
+    void checkOptionType(const ParserContext& ctx, const uint16_t opt_type) {
+        Subnet::OptionContainerPtr options =
+            ctx.options_->getItems("option-space");
+        ASSERT_TRUE(options);
+        Subnet::OptionContainerTypeIndex& idx = options->get<1>();
+        Subnet::OptionContainerTypeRange range = idx.equal_range(opt_type);
+        ASSERT_EQ(1, std::distance(range.first, range.second));
+    }
+
+    /// @brief Test copy constructor or assignment operator when values
+    /// being copied are NULL.
+    ///
+    /// @param copy Indicates that copy constructor should be tested
+    /// (if true), or assignment operator (if false).
+    void testCopyAssignmentNull(const bool copy) {
+        ParserContext ctx(Option::V6);
+        // Release all pointers in the context.
+        ctx.boolean_values_.reset();
+        ctx.uint32_values_.reset();
+        ctx.string_values_.reset();
+        ctx.options_.reset();
+        ctx.option_defs_.reset();
+        ctx.hooks_libraries_.reset();
+
+        // Even if the fields of the context are NULL, it should get
+        // copied.
+        ParserContextPtr ctx_new(new ParserContext(Option::V6));
+        if (copy) {
+            ASSERT_NO_THROW(ctx_new.reset(new ParserContext(ctx)));
+        } else {
+            *ctx_new = ctx;
+        }
+
+        // The resulting context has its fields equal to NULL.
+        EXPECT_FALSE(ctx_new->boolean_values_);
+        EXPECT_FALSE(ctx_new->uint32_values_);
+        EXPECT_FALSE(ctx_new->string_values_);
+        EXPECT_FALSE(ctx_new->options_);
+        EXPECT_FALSE(ctx_new->option_defs_);
+        EXPECT_FALSE(ctx_new->hooks_libraries_);
+
+    }
+
+    /// @brief Test copy constructor or assignment operator.
+    ///
+    /// @param copy Indicates that copy constructor should be tested (if true),
+    /// or assignment operator (if false).
+    void testCopyAssignment(const bool copy) {
+        // Create new context. It will be later copied/assigned to another
+        // context.
+        ParserContext ctx(Option::V6);
+
+        // Set boolean parameter 'foo'.
+        ASSERT_TRUE(ctx.boolean_values_);
+        ctx.boolean_values_->setParam("foo", true);
+
+        // Set uint32 parameter 'foo'.
+        ASSERT_TRUE(ctx.uint32_values_);
+        ctx.uint32_values_->setParam("foo", 123);
+
+        // Ser string parameter 'foo'.
+        ASSERT_TRUE(ctx.string_values_);
+        ctx.string_values_->setParam("foo", "some string");
+
+        // Add new option, with option code 10, to the context.
+        ASSERT_TRUE(ctx.options_);
+        OptionPtr opt1(new Option(Option::V6, 10));
+        Subnet::OptionDescriptor desc1(opt1, false);
+        std::string option_space = "option-space";
+        ASSERT_TRUE(desc1.option);
+        ctx.options_->addItem(desc1, option_space);
+
+        // Add new option definition, with option code 123.
+        OptionDefinitionPtr def1(new OptionDefinition("option-foo", 123,
+                                                      "string"));
+        ctx.option_defs_->addItem(def1, option_space);
+
+        // Allocate container for hooks libraries and add one library name.
+        ctx.hooks_libraries_.reset(new std::vector<std::string>());
+        ctx.hooks_libraries_->push_back("library1");
+
+        // We will use ctx_new to assign another context to it or copy
+        // construct.
+        ParserContextPtr ctx_new(new ParserContext(Option::V4));;
+        if (copy) {
+            ctx_new.reset(new ParserContext(ctx));
+        } else {
+            *ctx_new = ctx;
+        }
+
+        // New context has the same boolean value.
+        ASSERT_TRUE(ctx_new->boolean_values_);
+        {
+            SCOPED_TRACE("Check that boolean values are equal in both"
+                         " contexts");
+            checkValueEq<BooleanStorage, bool>(ctx.boolean_values_,
+                                               ctx_new->boolean_values_);
+        }
+
+        // New context has the same uint32 value.
+        ASSERT_TRUE(ctx_new->uint32_values_);
+        {
+            SCOPED_TRACE("Check that uint32_t values are equal in both"
+                         " contexts");
+            checkValueEq<Uint32Storage, uint32_t>(ctx.uint32_values_,
+                                                  ctx_new->uint32_values_);
+        }
+
+        // New context has the same string value.
+        ASSERT_TRUE(ctx_new->string_values_);
+        {
+            SCOPED_TRACE("Check that string values are equal in both contexts");
+            checkValueEq<StringStorage, std::string>(ctx.string_values_,
+                                                     ctx_new->string_values_);
+        }
+
+        // New context has the same option.
+        ASSERT_TRUE(ctx_new->options_);
+        {
+            SCOPED_TRACE("Check that the option values are equal in both"
+                         " contexts");
+            checkOptionType(*ctx_new, 10);
+        }
+
+        // New context has the same option definition.
+        ASSERT_TRUE(ctx_new->option_defs_);
+        {
+            SCOPED_TRACE("check that the option definition is equal in both"
+                         " contexts");
+            checkOptionDefinitionType(*ctx_new, 123);
+        }
+
+        // New context has the same hooks library.
+        ASSERT_TRUE(ctx_new->hooks_libraries_);
+        {
+            ASSERT_EQ(1, ctx_new->hooks_libraries_->size());
+            EXPECT_EQ("library1", (*ctx_new->hooks_libraries_)[0]);
+        }
+
+        // New context has the same universe.
+        EXPECT_EQ(ctx.universe_, ctx_new->universe_);
+
+        // Change the value of the boolean parameter. This should not affect the
+        // corresponding value in the new context.
+        {
+            SCOPED_TRACE("Check that boolean value isn't changed when original"
+                         " value is changed");
+            ctx.boolean_values_->setParam("foo", false);
+            checkValueNeq<BooleanStorage, bool>(ctx.boolean_values_,
+                                                ctx_new->boolean_values_);
+        }
+
+        // Change the value of the uint32_t parameter. This should not affect
+        // the corresponding value in the new context.
+        {
+            SCOPED_TRACE("Check that uint32_t value isn't changed when original"
+                         " value is changed");
+            ctx.uint32_values_->setParam("foo", 987);
+            checkValueNeq<Uint32Storage, uint32_t>(ctx.uint32_values_,
+                                                   ctx_new->uint32_values_);
+        }
+
+        // Change the value of the string parameter. This should not affect the
+        // corresponding value in the new context.
+        {
+            SCOPED_TRACE("Check that string value isn't changed when original"
+                         " value is changed");
+            ctx.string_values_->setParam("foo", "different string");
+            checkValueNeq<StringStorage, std::string>(ctx.string_values_,
+                                                      ctx_new->string_values_);
+        }
+
+        // Change the option. This should not affect the option instance in the
+        // new context.
+        {
+            SCOPED_TRACE("Check that option remains the same in the new context"
+                         " when the option in the original context is changed");
+            ctx.options_->clearItems();
+            Subnet::OptionDescriptor desc(OptionPtr(new Option(Option::V6,
+                                                               100)),
+                                          false);
+
+            ASSERT_TRUE(desc.option);
+            ctx.options_->addItem(desc, "option-space");
+            checkOptionType(*ctx_new, 10);
+        }
+
+        // Change the option definition. This should not affect the option
+        // definition in the new context.
+        {
+            SCOPED_TRACE("Check that option definition remains the same in"
+                         " the new context when the option definition in the"
+                         " original context is changed");
+            ctx.option_defs_->clearItems();
+            OptionDefinitionPtr def(new OptionDefinition("option-foo", 234,
+                                                         "string"));
+
+            ctx.option_defs_->addItem(def, option_space);
+            checkOptionDefinitionType(*ctx_new, 123);
+        }
+
+        // Change the list of libraries. this should not affect the list in the
+        // new context.
+        ctx.hooks_libraries_->clear();
+        ctx.hooks_libraries_->push_back("library2");
+        ASSERT_EQ(1, ctx_new->hooks_libraries_->size());
+        EXPECT_EQ("library1", (*ctx_new->hooks_libraries_)[0]);
+
+        // Change the universe. This should not affect the universe value in the
+        // new context.
+        ctx.universe_ = Option::V4;
+        EXPECT_EQ(Option::V6, ctx_new->universe_);
+
+    }
+
+};
+
+// Check that the assignment operator of the ParserContext class copies all
+// fields correctly.
+TEST_F(ParserContextTest, assignment) {
+    testCopyAssignment(false);
+}
+
+// Check that the assignment operator of the ParserContext class copies all
+// fields correctly when these fields are NULL.
+TEST_F(ParserContextTest, assignmentNull) {
+    testCopyAssignmentNull(false);
+}
+
+// Check that the context is copy constructed correctly.
+TEST_F(ParserContextTest, copyConstruct) {
+    testCopyAssignment(true);
+}
+
+// Check that the context is copy constructed correctly, when context fields
+// are NULL.
+TEST_F(ParserContextTest, copyConstructNull) {
+    testCopyAssignmentNull(true);
+}

+ 31 - 0
src/lib/dhcpsrv/tests/test_get_callout_handle.cc

@@ -0,0 +1,31 @@
+// Copyright (C) 2013  Internet Systems Consortium, Inc. ("ISC")
+//
+// Permission to use, copy, modify, and/or distribute this software for any
+// purpose with or without fee is hereby granted, provided that the above
+// copyright notice and this permission notice appear in all copies.
+//
+// THE SOFTWARE IS PROVIDED "AS IS" AND ISC DISCLAIMS ALL WARRANTIES WITH
+// REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY
+// AND FITNESS.  IN NO EVENT SHALL ISC BE LIABLE FOR ANY SPECIAL, DIRECT,
+// INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM
+// LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE
+// OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR
+// PERFORMANCE OF THIS SOFTWARE.
+
+#include <dhcpsrv/callout_handle_store.h>
+#include "test_get_callout_handle.h"
+
+// Just instantiate the getCalloutHandle function and call it.
+
+namespace isc {
+namespace dhcp {
+namespace test {
+
+isc::hooks::CalloutHandlePtr
+testGetCalloutHandle(const Pkt6Ptr& pktptr) {
+    return (isc::dhcp::getCalloutHandle(pktptr));
+}
+
+} // namespace test
+} // namespace dhcp
+} // namespace isc

+ 46 - 0
src/lib/dhcpsrv/tests/test_get_callout_handle.h

@@ -0,0 +1,46 @@
+// Copyright (C) 2013  Internet Systems Consortium, Inc. ("ISC")
+//
+// Permission to use, copy, modify, and/or distribute this software for any
+// purpose with or without fee is hereby granted, provided that the above
+// copyright notice and this permission notice appear in all copies.
+//
+// THE SOFTWARE IS PROVIDED "AS IS" AND ISC DISCLAIMS ALL WARRANTIES WITH
+// REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY
+// AND FITNESS.  IN NO EVENT SHALL ISC BE LIABLE FOR ANY SPECIAL, DIRECT,
+// INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM
+// LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE
+// OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR
+// PERFORMANCE OF THIS SOFTWARE.
+
+#ifndef TEST_GET_CALLOUT_HANDLE_H
+#define TEST_GET_CALLOUT_HANDLE_H
+
+#include <dhcp/pkt6.h>
+#include <hooks/callout_handle.h>
+
+namespace isc {
+namespace dhcp {
+namespace test {
+
+/// @file
+/// @brief Get Callout Handle
+///
+/// This function is a shall around getCalloutHandle.  It's purpose is to
+/// ensure that the getCalloutHandle() template function is referred to by
+/// two separate compilation units, and so test that data stored in one unit
+/// can be accessed by another. (This should be the case, but some compilers
+/// mabe be odd when it comes to template instantiation.)
+///
+/// @param pktptr Pointer to a Pkt6 object.
+///
+/// @return CalloutHandlePtr pointing to CalloutHandle associated with the
+///         Pkt6 object.
+isc::hooks::CalloutHandlePtr
+testGetCalloutHandle(const Pkt6Ptr& pktptr);
+
+} // namespace test
+} // namespace dhcp
+} // namespace isc
+
+
+#endif // TEST_GET_CALLOUT_HANDLE_H

+ 3 - 2
src/lib/hooks/callout_manager.cc

@@ -157,12 +157,13 @@ CalloutManager::callCallouts(int hook_index, CalloutHandle& callout_handle) {
                             .getName(current_hook_))
                         .arg(PointerConverter(i->second).dlsymPtr());
                 }
-            } catch (...) {
+            } catch (const std::exception& e) {
                 // Any exception, not just ones based on isc::Exception
                 LOG_ERROR(hooks_logger, HOOKS_CALLOUT_EXCEPTION)
                     .arg(current_library_)
                     .arg(ServerHooks::getServerHooks().getName(current_hook_))
-                    .arg(PointerConverter(i->second).dlsymPtr());
+                    .arg(PointerConverter(i->second).dlsymPtr())
+                    .arg(e.what());
             }
 
         }

+ 4 - 4
src/lib/hooks/hooks_component_developer.dox

@@ -31,7 +31,7 @@ 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.
+during its execution, see the document @ref hooksdgDevelopersGuide.
 
 @subsection hooksComponentTerminology Terminology
 
@@ -53,7 +53,7 @@ to execute a user-written function.
 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 @ref hooksDevelopersGuide for more details.)
+other. (See the document @ref hooksdgDevelopersGuide for more details.)
 
 @subsection hooksComponentLanguages Languages
 
@@ -463,7 +463,7 @@ It is possible for a component to register its own functions (i.e. within
 its own address space) as hook callouts.  These functions are called
 in eactly the same way as user callouts, being passed their arguments
 though a CalloutHandle object.  (Guidelines for writing callouts can be
-found in @ref hooksDevelopersGuide.)
+found in @ref hooksdgDevelopersGuide.)
 
 A component can associate with a hook callouts that run either before
 user-registered callouts or after them.  Registration is done via a
@@ -473,7 +473,7 @@ through the methods isc::hooks::HooksManager::preCalloutLibraryHandle()
 callouts) or isc::hooks::HooksManager::postCalloutLibraryHandle() (for
 a handle to register callouts to run after the user callouts).  Use of
 the LibraryHandle to register and deregister callouts is described in
-@ref hooksLibraryHandle.
+@ref hooksdgLibraryHandle.
 
 Finally, it should be noted that callouts registered in this way only
 remain registered until the next call to isc::hooks::loadLibraries().

+ 274 - 0
src/lib/hooks/hooks_maintenance.dox

@@ -0,0 +1,274 @@
+// Copyright (C) 2013  Internet Systems Consortium, Inc. ("ISC")
+//
+// Permission to use, copy, modify, and/or distribute this software for any
+// purpose with or without fee is hereby granted, provided that the above
+// copyright notice and this permission notice appear in all copies.
+//
+// THE SOFTWARE IS PROVIDED "AS IS" AND ISC DISCLAIMS ALL WARRANTIES WITH
+// REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY
+// AND FITNESS.  IN NO EVENT SHALL ISC BE LIABLE FOR ANY SPECIAL, DIRECT,
+// INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM
+// LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE
+// OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR
+// PERFORMANCE OF THIS SOFTWARE.
+
+// Note: the prefix "hooksmg" to all labels is an abbreviation for "Hooks
+// Maintenance Guide" and is used to prevent a clash with symbols in any
+// other Doxygen file.
+
+/**
+ @page hooksmgMaintenanceGuide Hooks Maintenance Guide
+
+ @section hooksmgIntroduction Introduction
+
+ This document is aimed at BIND 10 maintainers responsible for the hooks
+ system.  It provides an overview of the classes that make up the hooks
+ framework and notes important aspects of processing.  More detailed
+ information can be found in the source code.
+
+ It is assumed that the reader is familiar with the contents of the @ref
+ hooksdgDevelopersGuide and the @ref hooksComponentDeveloperGuide.
+
+ @section hooksmgObjects Hooks Framework Objects
+
+ The relationships between the various objects in the hooks framework
+ is shown below:
+
+ @image html HooksUml.png "High-Level Class Diagram of the Hooks Framework"
+
+ (To avoid clutter, the @ref hooksmgServerHooks object, used to pass
+ information about registered hooks to the components, is not shown on
+ the diagram.)
+
+ The hooks framework objects can be split into user-side objects and
+ server-side objects.  The former are those objects used or referenced
+ by user-written hooks libraries.  The latter are those objects used in
+ the hooks framework.
+
+ @subsection hooksmgUserObjects User-Side Objects
+
+ The user-side code is able to access two objects in the framework,
+ the @ref hooksmgCalloutHandle and the @ref hooksmgLibraryHandle.
+ The @ref hooksmgCalloutHandle is used to pass data between the BIND 10
+ component and the loaded library; the @ref hooksmgLibraryHandle is used
+ for registering callouts.
+
+ @subsubsection hooksmgCalloutHandle Callout Handle
+
+ The @ref isc::hooks::CalloutHandle has two functions: passing arguments
+ between the BIND 10 component and the user-written library, and storing
+ per-request context between library calls.  In both cases the data is
+ stored in a std::map structure, keyed by argument (or context item) name.
+ The actual data is stored in a boost::any object, which allows any
+ data type to be stored, although a penalty for this flexibility is
+ the restriction (mentioned in the @ref hooksdgDevelopersGuide) that
+ the type of data retrieved must be identical (and not just compatible)
+ with that stored.
+
+ The storage of context data is slightly complex because there is
+ separate context for each user library.  For this reason, the @ref
+ hooksmgCalloutHandle has multiple maps, one for each library loaded.
+ The maps are stored in another map, the appropriate map being identified
+ by the "current library index" (this index is explained further below).
+ The reason for the second map (rather than a structure such as a vector)
+ is to avoid creating individual context maps unless needed; given the
+ key to the map (in this case the current library index) accessing an
+ element in a map using the operator[] method returns the element in
+ question if it exists, or creates a new one (and stores it in the map)
+ if its doesn't.
+
+ @subsubsection hooksmgLibraryHandle Library Handle
+
+ Little more than a restricted interface to the @ref
+ hooksmgCalloutManager, the @ref isc::hooks::LibraryHandle allows a
+ callout to register and deregister callouts.  However, there are some
+ quirks to callout registration which, although the processing involved
+ is in the @ref hooksmgCalloutManager, are best described here.
+
+ Firstly, a callout can be deregistered by a function within a user
+ library only if it was registered by a function within that library. That
+ is to say, if library A registers the callout A_func() on hook "alpha"
+ and library B registers B_func(), functions within library A are only
+ able to remove A_func() (and functions in library B remove B_func()).
+ The restriction - here to prevent one library interfering with the
+ callouts of another - is enforced by means of the current library index.
+ As described below, each entry in the vector of callouts associated with
+ a hook is a pair object, comprising a pointer to the callout and
+ the index of the library with which it is associated. A callout
+ can only modify entries in that vector where the current library index
+ matches the index element of the pair.
+
+ A second quirk is that when dynamically modifying the list of callouts,
+ the change only takes effect when the current call out from the server
+ completes. To clarify this, suppose that functions A_func(), B_func()
+ and C_func() are registered on a hook, and the server executes a callout
+ on the hook. Suppose also during this call, A_func() removes the callout
+ C_func() and that B_func() adds D_func(). As changes only take effect
+ when the current call out completes, the user callouts executed will be
+ A_func(), B_func() then C_func(). When the server calls the hook callouts
+ again, the functions executed will be A_func(), B_func() and D_func().
+
+ This restriction is down to implementation.  When a set of callouts on a hook
+ is being called, the @ref hooksmgCalloutManager iterates through a
+ vector (the "callout vector") of (index, callout pointer) pairs.  Since
+ registration or deregistration of a callout on that hook would change the
+ vector (and so potentially invalidate the iterators used to access the it),
+ a copy of the vector is taken before the iteration starts.  The @ref
+ hooksmgCalloutManager iterates over this copy while any changes made
+ by the callout registration functions affect the relevant callout vector.
+ Such approach was chosen because of performance considerations.
+
+ @subsection hooksmgServerObjects Server-Side Objects
+
+ Those objects are not accessible by user libraries. Please do not
+ attempt to use them if you are developing user callouts.
+
+ @subsubsection hooksmgServerHooks Server Hooks
+
+ The singleton @ref isc::hooks::ServerHooks object is used to register
+ hooks. It is little more than a wrapper around a map of (hook index,
+ hook name), generating a unique number (the hook index) for each
+ hook registered.  It also handles the registration of the pre-defined
+ context_create and context_destroy hooks.
+
+ In operation, the @ref hooksmgHooksManager provides a thin wrapper
+ around it, so that the BIND 10 component developer does not have to
+ worry about another object.
+
+ @subsubsection hooksmgLibraryManager Library Manager
+
+ An @ref isc::hooks::LibraryManager is created by the @ref
+ hooksmgHooksManager object for each shared library loaded. It
+ controls the loading and unloading of the library and in essence
+ represents the library in the hooks framework. It also handles the
+ registration of the standard callouts (functions in the library with
+ the same name as the hook name).
+
+ Of particular importance is the "library's index", a number associated
+ with the library.  This is passed to the LibraryManager at creation
+ time and is used to tag the callout pointers.  It is discussed
+ further below.
+
+ As the LibraryManager provides all the methods needed to manage the
+ shared library, it is the natural home for the static validateLibrary()
+ method. The function called the parsing of the BIND 10 configuration, when
+ the "hooks-libraries" element is processed. It checks that shared library
+ exists, that it can be opened, that it contains the "version()" function
+ and that that function returns a valid value. It then closes the shared
+ library and returns an appropriate indication as to the library status.
+
+ @subsubsection hooksmgLibraryManagerCollection Library Manager Collection
+
+ The hooks framework can handle multiple libraries and as
+ a result will create a @ref hooksmgLibraryManager for each
+ of them. The collection of LibraryManagers is managed by the
+ @ref isc::hooks::LibraryManagerCollection object which, in most
+ cases has a method corresponding to a @ref hooksmgLibraryManager
+ method, e.g. it has a loadLibraries() that corresponds to the @ref
+ hooksmgLibraryManager's loadLibrary() call. As would be expected, methods
+ on the LibraryManagerCollection iterate through all specified libraries,
+ calling the corresponding LibraryManager method for each library.
+
+ One point of note is that LibraryManagerCollection operates on an "all
+ or none" principle. When loadLibraries() is called, on exit either all
+ libraries have been successfully opened or none of them have. There
+ is no use-case in BIND 10 where, after a user has specified the shared
+ libraries they want to load, the system will operate with only some of
+ them loaded.
+
+ The LibraryManagerCollection is the place where each library's index is set.
+ Each library is assigned a number ranging from 1 through to the number
+ of libraries being loaded.  As mentioned in the previous section, this
+ index is used to tag callout pointers, something that is discussed
+ in the next section.
+
+ (Whilst on the subject of library index numbers, two additional
+ numbers - 0 and INT_MAX - are also valid as "current library index".
+ For flexibility, the BIND 10 component is able to register its own
+ functions as hook callouts.  It does this by obtaining a suitable @ref
+ hooksmgLibraryHandle from the @ref hooksmgHooksManager.  A choice
+ of two is available: one @ref hooksmgLibraryHandle (with an index
+ of 0) can be used to register a callout on a hook to execute before
+ any user-supplied callouts.  The second (with an index of INT_MAX)
+ is used to register a callout to run after user-specified callouts.
+ Apart from the index number, the hooks framework does not treat these
+ callouts any differently from user-supplied ones.)
+
+ @subsubsection hooksmgCalloutManager Callout Manager
+
+ The @ref isc::hooks::CalloutManager is the core of the framework insofar
+ as the registration and calling of callouts is concerned.
+
+ It maintains a "hook vector" - a vector with one element for
+ each registered hook. Each element in this vector is itself a
+ vector (the callout vector), each element of which is a pair of
+ (library index, callback pointer). When a callout is registered, the
+ CalloutManager's current library index is used to supply the "library
+ index" part of the pair. The library index is set explicitly by the
+ @ref hooksmgLibraryManager prior to calling the user library's load()
+ function (and prior to registering the standard callbacks).
+
+ The situation is slightly more complex when a callout is executing. In
+ order to execute a callout, the CalloutManager's callCallouts()
+ method must be called. This iterates through the callout vector for
+ a hook and for each element in the vector, uses the "library index"
+ part of the pair to set the "current library index" before calling the
+ callout function recorded in the second part of the pair. In most cases,
+ the setting of the library index has no effect on the callout. However,
+ if the callout wishes to dynamically register or deregister a callout,
+ the @ref hooksmgLibraryHandle (see above) calls a method on the
+ @ref hooksmgCalloutManager which in turn uses that information.
+
+ @subsubsection hooksmgHooksManager Hooks Manager
+
+ The @ref isc::hooks::HooksManager is the main object insofar as the
+ server is concerned. It controls the creation of the library-related
+ objects and provides the framework in which they interact. It also
+ provides a shell around objects such as @ref hooksmgServerHooks so that all
+ interaction with the hooks framework by the server is through the
+ HooksManager object.  Apart from this, it supplies no functionality to
+ the hooks framework.
+
+ @section hooksmgOtherIssues Other Issues
+
+ @subsection hooksmgMemoryAllocation Memory Allocation
+
+ 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 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 lie in the virtual address allocated to the library. This has
+ implications in the hooks framework because:
+
+ - Argument information stored in a @ref hooksmgCalloutHandle 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 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 the @ref hooksmgCalloutHandle
+ by keeping in that object a shared pointer to the object controlling
+ library unloading (the @ref hooksmgLibraryManagerCollection). Although
+ the libraries can be unloaded at any time, it is only when every
+ @ref hooksmgCalloutHandle 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 BIND 10 server incorporating the
+ hooks. It is up to the server developer to ensure that all such objects
+ have been destroyed before libraries are reloaded. In extreme cases
+ this may mean the server suspending all processing of incoming requests
+ until all currently executing requests have completed and data object
+ destroyed, reloading the libraries, then resuming processing.
+*/

+ 1 - 1
src/lib/hooks/hooks_messages.mes

@@ -44,7 +44,7 @@ is issued.  It identifies the hook to which the callout is attached, the
 index of the library (in the list of loaded libraries) that registered
 it and the address of the callout.  The error is otherwise ignored.
 
-% HOOKS_CALLOUT_EXCEPTION exception thrown by callout on hook %1 registered by library with index %2 (callout address %3)
+% HOOKS_CALLOUT_EXCEPTION exception thrown by callout on hook %1 registered by library with index %2 (callout address %3): %4
 If a callout throws an exception when called, this error message is
 issued.  It identifies the hook to which the callout is attached, the
 index of the library (in the list of loaded libraries) that registered

+ 1 - 1
src/lib/hooks/hook_user.dox

@@ -17,7 +17,7 @@
 // other Doxygen file.
 
 /**
- @page hooksdgDevelopersGuide Hook Developer's Guide
+ @page hooksdgDevelopersGuide Hooks Developer's Guide
 
  @section hooksdgIntroduction Introduction
 

BIN
src/lib/hooks/images/HooksUml.dia


BIN
src/lib/hooks/images/HooksUml.png


+ 1 - 1
tests/tools/perfdhcp/perf_pkt4.cc

@@ -40,7 +40,7 @@ PerfPkt4::rawPack() {
                                options_,
                                getTransidOffset(),
                                getTransid(),
-                               bufferOut_));
+                               buffer_out_));
 }
 
 bool