[#1282] Addressed comments

diff --git a/src/bin/dhcp4/dhcp4to6_ipc.cc b/src/bin/dhcp4/dhcp4to6_ipc.cc
index 6cb41c4c0043ba5f7434d7d03d01f63f6ef17fba..9795b9f9fe7db3a6fcd2c2833d67f249b7e8b060 100644 (file)
--- a/src/bin/dhcp4/dhcp4to6_ipc.cc
+++ b/src/bin/dhcp4/dhcp4to6_ipc.cc
@@ -1,4 +1,4 @@
-// Copyright (C) 2015-2019 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2015-2020 Internet Systems Consortium, Inc. ("ISC")
 //
 // This Source Code Form is subject to the terms of the Mozilla Public
 // License, v. 2.0. If a copy of the MPL was not distributed with this
@@ -121,6 +121,12 @@ void Dhcp4to6Ipc::handler(int /* fd */) {
             // Delete previously set arguments
             callout_handle->deleteAllArguments();
 
+            // Use the RAII wrapper to make sure that the callout handle state is
+            // reset when this object goes out of scope. All hook points must do
+            // it to prevent possible circular dependency between the callout
+            // handle and its arguments.
+            ScopedCalloutHandleState callout_handle_state(callout_handle);
+
             // Enable copying options from the packet within hook library.
             ScopedEnableOptionsCopy<Pkt4> response4_options_copy(rsp);
 

diff --git a/src/bin/dhcp4/tests/hooks_unittest.cc b/src/bin/dhcp4/tests/hooks_unittest.cc
index 0fc7742aff2b6b4fd37b4d9592dd8c9b0486d069..a343973fe2f31aceff0a0b8d79cdd70b5b19fd14 100644 (file)
--- a/src/bin/dhcp4/tests/hooks_unittest.cc
+++ b/src/bin/dhcp4/tests/hooks_unittest.cc
@@ -122,7 +122,6 @@ public:
 
     /// @brief destructor (deletes Dhcpv4Srv)
     virtual ~HooksDhcpv4SrvTest() {
-
         // clear static buffers
         resetCalloutBuffers();
 

diff --git a/src/bin/dhcp6/json_config_parser.cc b/src/bin/dhcp6/json_config_parser.cc
index f8f8285fec1099bf5160eeda039c0beae0a810d0..9a21a941b2a5a39c7437eea978598bede54a8a46 100644 (file)
--- a/src/bin/dhcp6/json_config_parser.cc
+++ b/src/bin/dhcp6/json_config_parser.cc
@@ -841,10 +841,10 @@ configureDhcp6Server(Dhcpv6Srv& server, isc::data::ConstElementPtr config_set,
             // This occurs last as if it succeeds, there is no easy way to
             // revert it.  As a result, the failure to commit a subsequent
             // change causes problems when trying to roll back.
-            const HooksConfig& libraries =
-                CfgMgr::instance().getStagingCfg()->getHooksConfig();
             HooksManager::prepareUnloadLibraries();
             static_cast<void>(HooksManager::unloadLibraries());
+            const HooksConfig& libraries =
+                CfgMgr::instance().getStagingCfg()->getHooksConfig();
             libraries.loadLibraries();
         }
         catch (const isc::Exception& ex) {

diff --git a/src/lib/hooks/hooks_manager.h b/src/lib/hooks/hooks_manager.h
index 85b6dc755a95efed78123f25929e010c5bfb2ac0..4946a01bbde5fbba334e200a5932b4675e3bba5b 100644 (file)
--- a/src/lib/hooks/hooks_manager.h
+++ b/src/lib/hooks/hooks_manager.h
@@ -86,7 +86,7 @@ public:
     ///        library manager collection) the method will fail to close
     ///        libraries and returns false. It is a fatal error as there
     ///        is no possible recovery. It is a logic error in the hook
-    ///        code too so the solution is to fix the it and to restart
+    ///        code too so the solution is to fix it and to restart
     ///        the server with a correct hook library binary.
     ///
     /// @return true if all libraries unloaded successfully, false if they
@@ -95,7 +95,7 @@ public:
 
     /// @brief Prepare the unloading of libraries
     ///
-    /// Calls the unload functions when they exists and removes callouts.
+    /// Calls the unload functions when they exist and removes callouts.
     ///
     /// @note: after the call to this method there should be no visible
     ///        dangling pointers (i.e. callout handles owning the library

diff --git a/src/lib/hooks/library_manager_collection.cc b/src/lib/hooks/library_manager_collection.cc
index 5856b83cd0eaaefb12cfc2acbb7f1a7722bbf9f0..6c32b76ce5ecaa6b1f28f307b61de8fdff7196b0 100644 (file)
--- a/src/lib/hooks/library_manager_collection.cc
+++ b/src/lib/hooks/library_manager_collection.cc
@@ -52,7 +52,7 @@ LibraryManagerCollection::LibraryManagerCollection(const HookLibsCollection& lib
 bool
 LibraryManagerCollection::loadLibraries() {
 
-    // There must be not libraries still in memory.
+    // There must be no libraries still in memory.
     if (!lib_managers_.empty()) {
         isc_throw(LibrariesStillOpened, "some libraries are still opened");
     }

diff --git a/src/lib/hooks/tests/library_manager_unittest.cc b/src/lib/hooks/tests/library_manager_unittest.cc
index 63eda0a0c15c54041b63f296e537e44ae2b7ccd8..8f12f350d585181464cbfdd2188c8940cfd4712c 100644 (file)
--- a/src/lib/hooks/tests/library_manager_unittest.cc
+++ b/src/lib/hooks/tests/library_manager_unittest.cc
@@ -604,8 +604,8 @@ TEST_F(LibraryManagerTest, LoadLibraryWrongVersion) {
 // Check that the full loadLibrary call works.
 
 TEST_F(LibraryManagerTest, LoadLibrary) {
- PublicLibraryManager lib_manager(std::string(FULL_CALLOUT_LIBRARY), 0,
-                                  callout_manager_);
+    PublicLibraryManager lib_manager(std::string(FULL_CALLOUT_LIBRARY), 0,
+                                     callout_manager_);
     EXPECT_TRUE(lib_manager.loadLibrary());
 
     // Now execute the callouts in the order expected.  The library performs