[5145b] Merge branch 'trac5145b' of ssh://git.kea.isc.org/git/kea into trac5145b

src/lib/dhcpsrv/srv_config.cc

@@ -173,35 +173,7 @@ SrvConfig::equals(const SrvConfig& other) const {
         return (false);
     }
     // Pass through all configured hooks libraries.
-    using namespace isc::hooks;
-    for (isc::hooks::HookLibsCollection::const_iterator this_it =
-             hooks_config_.get().begin();
-         this_it != hooks_config_.get().end(); ++this_it) {
-        bool match = false;
-        for (isc::hooks::HookLibsCollection::const_iterator other_it =
-                 other.hooks_config_.get().begin();
-             other_it != other.hooks_config_.get().end(); ++other_it) {
-            if (this_it->first != other_it->first) {
-                continue;
-            }
-            if (isNull(this_it->second) && isNull(other_it->second)) {
-                match = true;
-                break;
-            }
-            if (isNull(this_it->second) || isNull(other_it->second)) {
-                continue;
-            }
-            if (this_it->second->equals(*other_it->second)) {
-                match = true;
-                break;
-            }
-        }
-        // No match found for the particular hooks library so return false.
-        if (!match) {
-            return (false);
-        }
-    }
-    return (true);
+    return (hooks_config_.equal(other.hooks_config_));
 }
 
 void

src/lib/dhcpsrv/tests/srv_config_unittest.cc

@@ -423,6 +423,8 @@ TEST_F(SrvConfigTest, hooksLibraries) {
     ASSERT_NO_THROW(conf.copy(copied));
     ASSERT_TRUE(conf == copied);
     EXPECT_EQ(2, copied.getHooksConfig().get().size());
+
+    EXPECT_TRUE(copied.getHooksConfig().equal(conf.getHooksConfig()));
 }
 
 } // end of anonymous namespace

src/lib/hooks/hooks_config.cc

@@ -18,14 +18,17 @@ namespace hooks {
 
 void
 HooksConfig::verifyLibraries(const Element::Position& position) const {
+    // The code used to follow this logic:
+    //
     // Check if the list of libraries has changed.  If not, nothing is done
     // - the command "DhcpN libreload" is required to reload the same
     // libraries (this prevents needless reloads when anything else in the
     // configuration is changed).
-
+    //
     // We no longer rely on this. Parameters can change. And even if the
     // parameters stay the same, they could point to files that could
-    // change.
+    // change. We can skip loading routines only if there were and there still
+    // are no libraries specified.
     vector<string> current_libraries = HooksManager::getLibraryNames();
     if (current_libraries.empty() && libraries_.empty()) {
         return;
@@ -60,6 +63,45 @@ HooksConfig::loadLibraries() const {
     }
 }
 
+bool
+HooksConfig::equal(const HooksConfig& other) const {
+
+    /// @todo: This comparision assumes that the library order is not relevant,
+    /// so [ lib1, lib2 ] is equal to [ lib2, lib1 ]. However, this is not strictly
+    /// true, because callouts execution is called in other they're loaded. Therefore
+    /// changing the libraries order may change the server behavior.
+    ///
+    /// We don't have any libraries that are interacting (or would change their behavior
+    /// depending on the order in which their callouts are executed), so the code is
+    /// ok for now.
+    for (isc::hooks::HookLibsCollection::const_iterator this_it = libraries_.begin();
+         this_it != libraries_.end(); ++this_it) {
+        bool match = false;
+        for (isc::hooks::HookLibsCollection::const_iterator other_it =
+                 other.libraries_.begin(); other_it != other.libraries_.end(); ++other_it) {
+            if (this_it->first != other_it->first) {
+                continue;
+            }
+            if (isNull(this_it->second) && isNull(other_it->second)) {
+                match = true;
+                break;
+            }
+            if (isNull(this_it->second) || isNull(other_it->second)) {
+                continue;
+            }
+            if (this_it->second->equals(*other_it->second)) {
+                match = true;
+                break;
+            }
+        }
+        // No match found for the particular hooks library so return false.
+        if (!match) {
+            return (false);
+        }
+    }
+    return (true);
+}
+
 ElementPtr
 HooksConfig::toElement() const {
     // hooks-libraries is a list of maps

src/lib/hooks/hooks_config.h

@@ -60,6 +60,11 @@ public:
         libraries_.clear();
     }
 
+    /// @brief Compares two Hooks Config classes for equality
+    ///
+    /// @param other other hooksconfig to compare with
+    bool equal(const HooksConfig& other) const;
+
     /// @brief Verifies that libraries stored in libraries_ are valid.
     ///
     /// This method is a smart wrapper around @ref
@@ -81,9 +86,9 @@ public:
     /// @throw InvalidHooksLibraries if the call to HooksManager fails.
     void loadLibraries() const;
 
-    /// @brief Unparse a configuration objet
+    /// @brief Unparse a configuration object
     ///
-    /// Returns an element which must parse into the same objet, i.e.
+    /// Returns an element which must parse into the same object, i.e.
     /// @code
     /// for all valid config C parse(parse(C)->toElement()) == parse(C)
     /// @endcode

src/lib/hooks/hooks_parser.h

@@ -54,7 +54,8 @@ public:
     ///
     /// This method stores parsed libraries in libraries.
     ///
-    /// @param value pointer to the content of parsed values
+    /// @param libraries parsed libraries information will be stored here
+    /// @param value pointer to the content to be parsed
     void parse(HooksConfig& libraries, isc::data::ConstElementPtr value);
 };