From: Francis Dupont <fdupont@isc.org>
Date: Mon, 29 Jun 2020 12:35:39 +0000 (+0200)
Subject: [#1282] Addressed comments
X-Git-Tag: Kea-1.7.10~127
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=ba83c314eb84fbb0219da275b2b00f222f7d1135;p=thirdparty%2Fkea.git

[#1282] Addressed comments
---

diff --git a/src/bin/dhcp4/dhcp4to6_ipc.cc b/src/bin/dhcp4/dhcp4to6_ipc.cc
index 6cb41c4c00..9795b9f9fe 100644
--- 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 0fc7742aff..a343973fe2 100644
--- 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 f8f8285fec..9a21a941b2 100644
--- 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 85b6dc755a..4946a01bbd 100644
--- 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 5856b83cd0..6c32b76ce5 100644
--- 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 63eda0a0c1..8f12f350d5 100644
--- 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