[5145b] Changes after review:

 - added missing doxygen comment
 - moved comparison code from SrvConfig to HooksConfig::equal

diff --git a/src/lib/dhcpsrv/srv_config.cc b/src/lib/dhcpsrv/srv_config.cc
index f680128d53bbc812e34e7968000dd06f11930627..fe8b1219526f1962e2945e65e3ca3256d54681d2 100644 (file)
--- a/src/lib/dhcpsrv/srv_config.cc
+++ b/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

diff --git a/src/lib/dhcpsrv/tests/srv_config_unittest.cc b/src/lib/dhcpsrv/tests/srv_config_unittest.cc
index 5622df51ae3a6d28e15a264c2a758bd490f3b5e5..877610ee0c53f614f86eb0f3cbff61c1eaf3792b 100644 (file)
--- a/src/lib/dhcpsrv/tests/srv_config_unittest.cc
+++ b/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

diff --git a/src/lib/hooks/hooks_config.cc b/src/lib/hooks/hooks_config.cc
index 4ad74428e02db68bb7f9a128b905fee087e6e016..72a0ad0c4c8bf29b17ee40936a53064365f1454f 100644 (file)
--- a/src/lib/hooks/hooks_config.cc
+++ b/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

diff --git a/src/lib/hooks/hooks_config.h b/src/lib/hooks/hooks_config.h
index 49a49c0410565f5301b315738b470a1d662b0796..837727e40e139081851e9f582a8a39d6a4b5dd1e 100644 (file)
--- a/src/lib/hooks/hooks_config.h
+++ b/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

diff --git a/src/lib/hooks/hooks_parser.h b/src/lib/hooks/hooks_parser.h
index d6fc5ef6466f7942da9c63d9305d9fc3d58c79b9..4bec9a6aff6d1a1da6ff5c633ce6a5c07f6de877 100644 (file)
--- a/src/lib/hooks/hooks_parser.h
+++ b/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);
 };