[master] Merge branch 'trac3062'

Corrects a bug in Hooks code that was improperly
creating a new callout handle on every call, rather
than maintaining it throughout the context of the
packet being processed.

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>
 
@@ -969,33 +970,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) {

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

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

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

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

src/lib/dhcpsrv/callout_handle_store.h

+// 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

src/lib/dhcpsrv/tests/Makefile.am

@@ -41,6 +41,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 +54,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
 

src/lib/dhcpsrv/tests/callout_handle_store_unittest.cc

+// 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

src/lib/dhcpsrv/tests/test_get_callout_handle.cc

+// 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

src/lib/dhcpsrv/tests/test_get_callout_handle.h

+// 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