From: Tomek Mrugalski Date: Mon, 6 Mar 2017 16:10:18 +0000 (+0100) Subject: [5145b] Changes after review: X-Git-Tag: trac5152_base~11^2^2 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=4b93e51d588edf65ff321d30783b14d751d6e471;p=thirdparty%2Fkea.git [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 f680128d53..fe8b121952 100644 --- 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 5622df51ae..877610ee0c 100644 --- 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 4ad74428e0..72a0ad0c4c 100644 --- 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 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 49a49c0410..837727e40e 100644 --- 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 d6fc5ef646..4bec9a6aff 100644 --- 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); };