[2994] Changes after review:

 - pointers to DHCPv4 and DHCPv6 hooks removed
 - Many methods in HooksManager are now called directly in
   dhcp4_srv.cc, dhcp4_srv_unittest.cc and alloc_engine_unittest.cc
 - Comments wrapped up differently in dhcp4_srv_unittest.cc
 - Comments clarified in alloc_engine.{cc|h}

doc/devel/mainpage.dox

@@ -51,13 +51,11 @@
  *   - @subpage dhcpv4ConfigParser
  *   - @subpage dhcpv4ConfigInherit
  *   - @subpage dhcpv4Other
- *   - @subpage dhcpv4Hooks
  * - @subpage dhcp6
  *   - @subpage dhcpv6Session
  *   - @subpage dhcpv6ConfigParser
  *   - @subpage dhcpv6ConfigInherit
  *   - @subpage dhcpv6Other
- *   - @subpage dhcpv6Hooks
  * - @subpage libdhcp
  *   - @subpage libdhcpIntro
  *   - @subpage libdhcpRelay

src/bin/dhcp4/dhcp4_srv.cc

@@ -134,9 +134,6 @@ Dhcpv4Srv::Dhcpv4Srv(uint16_t port, const char* dbconfig, const bool use_bcast,
         hook_index_pkt4_send_      = Hooks.hook_index_pkt4_send_;
 
         /// @todo call loadLibraries() when handling configuration changes
-        vector<string> libraries; // no libraries at this time
-        HooksManager::loadLibraries(libraries);
-
 
     } catch (const std::exception &e) {
         LOG_ERROR(dhcp4_logger, DHCP4_SRV_CONSTRUCT_ERROR).arg(e.what());
@@ -157,11 +154,13 @@ Dhcpv4Srv::shutdown() {
     shutdown_ = true;
 }
 
-Pkt4Ptr Dhcpv4Srv::receivePacket(int timeout) {
+Pkt4Ptr
+Dhcpv4Srv::receivePacket(int timeout) {
     return (IfaceMgr::instance().receive4(timeout));
 }
 
-void Dhcpv4Srv::sendPacket(const Pkt4Ptr& packet) {
+void
+Dhcpv4Srv::sendPacket(const Pkt4Ptr& packet) {
     IfaceMgr::instance().send(packet);
 }
 
@@ -200,7 +199,7 @@ Dhcpv4Srv::run() {
                       .arg(query->toText());
 
             // Let's execute all callouts registered for packet_received
-            if (HooksManager::getHooksManager().calloutsPresent(hook_index_pkt4_receive_)) {
+            if (HooksManager::calloutsPresent(hook_index_pkt4_receive_)) {
                 CalloutHandlePtr callout_handle = getCalloutHandle(query);
 
                 // Delete previously set arguments
@@ -210,8 +209,8 @@ Dhcpv4Srv::run() {
                 callout_handle->setArgument("query4", query);
 
                 // Call callouts
-                HooksManager::getHooksManager().callCallouts(hook_index_pkt4_receive_,
-                                                *callout_handle);
+                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
@@ -289,7 +288,7 @@ Dhcpv4Srv::run() {
                 rsp->setIndex(query->getIndex());
 
                 // Execute all callouts registered for packet6_send
-                if (HooksManager::getHooksManager().calloutsPresent(hook_index_pkt4_send_)) {
+                if (HooksManager::calloutsPresent(hook_index_pkt4_send_)) {
                     CalloutHandlePtr callout_handle = getCalloutHandle(query);
 
                     // Delete all previous arguments
@@ -302,8 +301,8 @@ Dhcpv4Srv::run() {
                     callout_handle->setArgument("response4", rsp);
 
                     // Call all installed callouts
-                    HooksManager::getHooksManager().callCallouts(hook_index_pkt4_send_,
-                                                    *callout_handle);
+                    HooksManager::callCallouts(hook_index_pkt4_send_,
+                                               *callout_handle);
 
                     // Callouts decided to skip the next processing step. The next
                     // processing step would to send the packet, so skip at this
@@ -885,8 +884,9 @@ Dhcpv4Srv::selectSubnet(const Pkt4Ptr& question) {
     Subnet4Ptr subnet;
     // Is this relayed message?
     IOAddress relay = question->getGiaddr();
-    if (relay.toText() != "0.0.0.0") {
+    static const IOAddress notset("0.0.0.0");
 
+    if (relay != notset) {
         // Yes: Use relay address to select subnet
         subnet = CfgMgr::instance().getSubnet4(relay);
     } else {
@@ -898,7 +898,7 @@ Dhcpv4Srv::selectSubnet(const Pkt4Ptr& question) {
     /// @todo Implement getSubnet4(interface-name)
 
     // Let's execute all callouts registered for packet_received
-    if (HooksManager::getHooksManager().calloutsPresent(hook_index_subnet4_select_)) {
+    if (HooksManager::calloutsPresent(hook_index_subnet4_select_)) {
         CalloutHandlePtr callout_handle = getCalloutHandle(question);
 
         // We're reusing callout_handle from previous calls
@@ -910,8 +910,7 @@ Dhcpv4Srv::selectSubnet(const Pkt4Ptr& question) {
         callout_handle->setArgument("subnet4collection", CfgMgr::instance().getSubnets4());
 
         // Call user (and server-side) callouts
-        HooksManager::getHooksManager().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
@@ -953,8 +952,9 @@ Dhcpv4Srv::sanityCheck(const Pkt4Ptr& pkt, RequirementLevel serverid) {
     }
 
     // If there is HWAddress set and it is non-empty, then we're good
-    if (pkt->getHWAddr() && !pkt->getHWAddr()->hwaddr_.empty())
+    if (pkt->getHWAddr() && !pkt->getHWAddr()->hwaddr_.empty()) {
         return;
+    }
 
     // There has to be something to uniquely identify the client:
     // either non-zero MAC address or client-id option present (or both)
@@ -989,7 +989,7 @@ isc::hooks::CalloutHandlePtr Dhcpv4Srv::getCalloutHandle(const Pkt4Ptr& pkt) {
         // Remember the pointer to this packet
         old_pointer = pkt;
 
-        callout_handle = HooksManager::getHooksManager().createCalloutHandle();
+        callout_handle = HooksManager::createCalloutHandle();
     }
 
     return (callout_handle);

src/bin/dhcp4/tests/dhcp4_srv_unittest.cc

@@ -89,32 +89,28 @@ public:
     /// @brief fakes packet reception
     /// @param timeout ignored
     ///
-    /// The method receives all packets queued in receive
-    /// queue, one after another. Once the queue is empty,
-    /// it initiates the shutdown procedure.
+    /// The method receives all packets queued in receive queue, one after
+    /// another. Once the queue is empty, it initiates the shutdown procedure.
     ///
     /// See fake_received_ field for description
     virtual Pkt4Ptr receivePacket(int /*timeout*/) {
 
-        // If there is anything prepared as fake incoming
-        // traffic, use it
+        // If there is anything prepared as fake incoming traffic, use it
         if (!fake_received_.empty()) {
             Pkt4Ptr pkt = fake_received_.front();
             fake_received_.pop_front();
             return (pkt);
         }
 
-        // If not, just trigger shutdown and
-        // return immediately
+        // If not, just trigger shutdown and return immediately
         shutdown();
         return (Pkt4Ptr());
     }
 
     /// @brief fake packet sending
     ///
-    /// Pretend to send a packet, but instead just store
-    /// it in fake_send_ list where test can later inspect
-    /// server's response.
+    /// Pretend to send a packet, but instead just store it in fake_send_ list
+    /// where test can later inspect server's response.
     virtual void sendPacket(const Pkt4Ptr& pkt) {
         fake_sent_.push_back(pkt);
     }
@@ -157,11 +153,11 @@ static const char* SRVID_FILE = "server-id-test.txt";
 
 /// @brief Dummy Packet Filtering class.
 ///
-/// This class reports capability to respond directly to the
-/// client which doesn't have address configured yet.
+/// This class reports capability to respond directly to the client which
+/// doesn't have address configured yet.
 ///
-/// All packet and socket handling functions do nothing because
-/// they are not used in unit tests.
+/// All packet and socket handling functions do nothing because they are not
+/// used in unit tests.
 class PktFilterTest : public PktFilter {
 public:
 
@@ -1690,6 +1686,11 @@ public:
 
     /// @brief destructor (deletes Dhcpv4Srv)
     virtual ~HooksDhcpv4SrvTest() {
+
+        HooksManager::preCalloutsLibraryHandle().deregisterAllCallouts("pkt4_receive");
+        HooksManager::preCalloutsLibraryHandle().deregisterAllCallouts("pkt4_send");
+        HooksManager::preCalloutsLibraryHandle().deregisterAllCallouts("subnet4_select");
+
         delete srv_;
     }
 

src/lib/dhcpsrv/alloc_engine.cc

@@ -578,10 +578,10 @@ Lease4Ptr AllocEngine::reuseExpiredLease(Lease4Ptr& expired,
 
         // Pass necessary arguments
 
-        // Subnet from which we do the allocation (That's as far as we can go
-        // with using SubnetPtr to point to Subnet4 object. Users should not
-        // be confused with dynamic_pointer_casts. They should get a concrete
-        // pointer (Subnet4Ptr) pointing to a Subnet4 object.
+        // 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);
         callout_handle->setArgument("subnet4", subnet4);
 
@@ -638,9 +638,6 @@ Lease6Ptr AllocEngine::createLease6(const Subnet6Ptr& subnet,
         // Delete all previous arguments
         callout_handle->deleteAllArguments();
 
-        // Clear skip flag if it was set in previous callouts
-        callout_handle->setSkip(false);
-
         // Pass necessary arguments
 
         // Subnet from which we do the allocation
@@ -723,9 +720,6 @@ Lease4Ptr AllocEngine::createLease4(const SubnetPtr& subnet,
         // Delete all previous arguments
         callout_handle->deleteAllArguments();
 
-        // Clear skip flag if it was set in previous callouts
-        callout_handle->setSkip(false);
-
         // Pass necessary arguments
 
         // Subnet from which we do the allocation (That's as far as we can go

src/lib/dhcpsrv/alloc_engine.h

@@ -266,7 +266,8 @@ private:
     /// @param hwaddr client's hardware address
     /// @param addr an address that was selected and is confirmed to be available
     /// @param callout_handle a callout handle (used in hooks). A lease callouts
-    ///        will be executed if this parameter is passed.
+    ///        will be executed if this parameter is passed (and there are callouts
+    ///        registered)
     /// @param fake_allocation is this real i.e. REQUEST (false) or just picking
     ///        an address for DISCOVER that is not really allocated (true)
     /// @return allocated lease (or NULL in the unlikely case of the lease just
@@ -288,7 +289,8 @@ private:
     /// @param iaid IAID from the IA_NA container the client sent to us
     /// @param addr an address that was selected and is confirmed to be available
     /// @param callout_handle a callout handle (used in hooks). A lease callouts
-    ///        will be executed if this parameter is passed.
+    ///        will be executed if this parameter is passed (and there are callouts
+    ///        registered)
     /// @param fake_allocation is this real i.e. REQUEST (false) or just picking
     ///        an address for SOLICIT that is not really allocated (true)
     /// @return allocated lease (or NULL in the unlikely case of the lease just

src/lib/dhcpsrv/tests/alloc_engine_unittest.cc

@@ -1151,13 +1151,13 @@ TEST_F(HookAllocEngine6Test, lease6_select) {
 
     // Initialize Hooks Manager
     vector<string> libraries; // no libraries at this time
-    HooksManager::getHooksManager().loadLibraries(libraries);
+    HooksManager::loadLibraries(libraries);
 
     // Install pkt6_receive_callout
     EXPECT_NO_THROW(HooksManager::preCalloutsLibraryHandle().registerCallout(
                         "lease6_select", lease6_select_callout));
 
-    CalloutHandlePtr callout_handle = HooksManager::getHooksManager().createCalloutHandle();
+    CalloutHandlePtr callout_handle = HooksManager::createCalloutHandle();
 
     Lease6Ptr lease = engine->allocateAddress6(subnet_, duid_, iaid_, IOAddress("::"),
                                                false, callout_handle);
@@ -1181,7 +1181,7 @@ TEST_F(HookAllocEngine6Test, lease6_select) {
     ASSERT_TRUE(callback_subnet6_);
     EXPECT_EQ(subnet_->toText(), callback_subnet6_->toText());
 
-    EXPECT_EQ(callback_fake_allocation_, false);
+    EXPECT_FALSE(callback_fake_allocation_);
 
     // Check if all expected parameters are reported. It's a bit tricky, because
     // order may be different. If the test starts failing, because someone tweaked
@@ -1190,6 +1190,10 @@ TEST_F(HookAllocEngine6Test, lease6_select) {
     expected_argument_names.push_back("fake_allocation");
     expected_argument_names.push_back("lease6");
     expected_argument_names.push_back("subnet6");
+
+    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);
 }
 
@@ -1212,7 +1216,7 @@ TEST_F(HookAllocEngine6Test, change_lease6_select) {
 
     // Initialize Hooks Manager
     vector<string> libraries; // no libraries at this time
-    HooksManager::getHooksManager().loadLibraries(libraries);
+    HooksManager::loadLibraries(libraries);
 
     // Install a callout
     EXPECT_NO_THROW(HooksManager::preCalloutsLibraryHandle().registerCallout(
@@ -1220,7 +1224,7 @@ TEST_F(HookAllocEngine6Test, change_lease6_select) {
 
     // Normally, dhcpv6_srv would passed the handle when calling allocateAddress6,
     // but in tests we need to create it on our own.
-    CalloutHandlePtr callout_handle = HooksManager::getHooksManager().createCalloutHandle();
+    CalloutHandlePtr callout_handle = HooksManager::createCalloutHandle();
 
     // Call allocateAddress6. Callouts should be triggered here.
     Lease6Ptr lease = engine->allocateAddress6(subnet_, duid_, iaid_, IOAddress("::"),
@@ -1363,13 +1367,13 @@ TEST_F(HookAllocEngine4Test, lease4_select) {
 
     // Initialize Hooks Manager
     vector<string> libraries; // no libraries at this time
-    HooksManager::getHooksManager().loadLibraries(libraries);
+    HooksManager::loadLibraries(libraries);
 
     // Install pkt4_receive_callout
     EXPECT_NO_THROW(HooksManager::preCalloutsLibraryHandle().registerCallout(
                         "lease4_select", lease4_select_callout));
 
-    CalloutHandlePtr callout_handle = HooksManager::getHooksManager().createCalloutHandle();
+    CalloutHandlePtr callout_handle = HooksManager::createCalloutHandle();
 
     Lease4Ptr lease = engine->allocateAddress4(subnet_, clientid_, hwaddr_,
                                                IOAddress("0.0.0.0"),
@@ -1424,7 +1428,7 @@ TEST_F(HookAllocEngine4Test, change_lease4_select) {
 
     // Initialize Hooks Manager
     vector<string> libraries; // no libraries at this time
-    HooksManager::getHooksManager().loadLibraries(libraries);
+    HooksManager::loadLibraries(libraries);
 
     // Install a callout
     EXPECT_NO_THROW(HooksManager::preCalloutsLibraryHandle().registerCallout(
@@ -1432,7 +1436,7 @@ TEST_F(HookAllocEngine4Test, change_lease4_select) {
 
     // Normally, dhcpv4_srv would passed the handle when calling allocateAddress4,
     // but in tests we need to create it on our own.
-    CalloutHandlePtr callout_handle = HooksManager::getHooksManager().createCalloutHandle();
+    CalloutHandlePtr callout_handle = HooksManager::createCalloutHandle();
 
     // Call allocateAddress4. Callouts should be triggered here.
     Lease4Ptr lease = engine->allocateAddress4(subnet_, clientid_, hwaddr_, IOAddress("0.0.0.0"),