From: Stephen Morris Date: Tue, 13 Aug 2013 12:14:38 +0000 (+0100) Subject: [2981] Changes as a result of review. X-Git-Tag: bind10-1.2.0beta1-release~272^2~3 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=578910f51f1dc96876a67a02d353c396cc4d2ec8;p=thirdparty%2Fkea.git [2981] Changes as a result of review. --- diff --git a/src/bin/dhcp4/config_parser.cc b/src/bin/dhcp4/config_parser.cc index 7827c0280c..98393c1560 100644 --- a/src/bin/dhcp4/config_parser.cc +++ b/src/bin/dhcp4/config_parser.cc @@ -403,7 +403,7 @@ configureDhcp4Server(Dhcpv4Srv&, isc::data::ConstElementPtr config_set) { // Some of the parsers alter the state of the system in a way that can't // easily be undone. (Or alter it in a way such that undoing the change has - // the same risk of failurre as doing the change.) + // the same risk of failure as doing the change.) ParserPtr hooks_parser_; // The subnet parsers implement data inheritance by directly @@ -506,6 +506,13 @@ configureDhcp4Server(Dhcpv4Srv&, isc::data::ConstElementPtr config_set) { if (iface_parser) { iface_parser->commit(); } + + // This occurs last as if it succeeds, there is no easy way + // revert it. As a result, the failure to commit a subsequent + // change causes problems when trying to roll back. + if (hooks_parser_) { + hooks_parser_->commit(); + } } catch (const isc::Exception& ex) { LOG_ERROR(dhcp4_logger, DHCP4_PARSER_COMMIT_FAIL).arg(ex.what()); @@ -527,12 +534,6 @@ configureDhcp4Server(Dhcpv4Srv&, isc::data::ConstElementPtr config_set) { return (answer); } - // Now commit changes that have been validated but not yet committed, - // and which can't be rolled back. - if (hooks_parser_) { - hooks_parser_->commit(); - } - LOG_INFO(dhcp4_logger, DHCP4_CONFIG_COMPLETE).arg(config_details); // Everything was fine. Configuration is successful. diff --git a/src/bin/dhcp4/ctrl_dhcp4_srv.cc b/src/bin/dhcp4/ctrl_dhcp4_srv.cc index ad0c5ec290..43c08c9d74 100644 --- a/src/bin/dhcp4/ctrl_dhcp4_srv.cc +++ b/src/bin/dhcp4/ctrl_dhcp4_srv.cc @@ -145,13 +145,14 @@ ControlledDhcpv4Srv::dhcp4CommandHandler(const string& command, ConstElementPtr ConstElementPtr answer = isc::config::createAnswer(0, "Shutting down."); return (answer); + } else if (command == "libreload") { // TODO delete any stored CalloutHandles referring to the old libraries // Get list of currently loaded libraries and reload them. vector loaded = HooksManager::getLibraryNames(); bool status = HooksManager::loadLibraries(loaded); if (!status) { - LOG_ERROR(dhcp4_logger, DHCP4_RELOAD_FAIL); + LOG_ERROR(dhcp4_logger, DHCP4_HOOKS_LIBS_RELOAD_FAIL); ConstElementPtr answer = isc::config::createAnswer(1, "Failed to reload hooks libraries."); return (answer); diff --git a/src/bin/dhcp4/dhcp4_messages.mes b/src/bin/dhcp4/dhcp4_messages.mes index 2eeda9fe5b..63349999d4 100644 --- a/src/bin/dhcp4/dhcp4_messages.mes +++ b/src/bin/dhcp4/dhcp4_messages.mes @@ -89,6 +89,11 @@ point, the setting of the flag instructs the server not to choose a subnet, an action that severely limits further processing; the server will be only able to offer global options - no addresses will be assigned. +% DHCP4_HOOKS_LIBS_RELOAD_FAIL reload of hooks libraries failed +A "libreload" command was issued to reload the hooks libraries but for +some reason the reload failed. Other error messages issued from the +hooks framework will indicate the nature of the problem. + % DHCP4_LEASE_ADVERT lease %1 advertised (client client-id %2, hwaddr %3) This debug message indicates that the server successfully advertised a lease. It is up to the client to choose one server out of othe advertised @@ -226,11 +231,6 @@ a different hardware address. One possible reason for using different hardware address is that a cloned virtual machine was not updated and both clones use the same client-id. -% DHCP4_RELOAD_FAIL reload of hooks libraries failed -A "libreload" command was issued to reload the hooks libraries but for -some reason the reload failed. Other error messages issued from the -hooks framework will indicate the nature of the problem. - % DHCP4_RESPONSE_DATA responding with packet type %1, data is <%2> A debug message listing the data returned to the client. diff --git a/src/bin/dhcp4/tests/callout_library_common.h b/src/bin/dhcp4/tests/callout_library_common.h index e8d4b5ac60..cbabcda094 100644 --- a/src/bin/dhcp4/tests/callout_library_common.h +++ b/src/bin/dhcp4/tests/callout_library_common.h @@ -21,9 +21,9 @@ /// To check that they libraries are loaded and unloaded correctly, the load /// and unload functions in this library maintain two marker files - the load /// marker file and the unload marker file. The functions append a single -/// to the single line in the file, creating the file if need be. In -/// this way, the test code can determine whether the load/unload functions -/// have been run and, if so, in what order. +/// line to the file, creating the file if need be. In this way, the test code +/// can determine whether the load/unload functions have been run and, if so, +/// in what order. /// /// This file is the common library file for the tests. It will not compile /// by itself - it is included into each callout library which specifies the @@ -69,11 +69,13 @@ version() { return (BIND10_HOOKS_VERSION); } -int load(LibraryHandle&) { +int +load(LibraryHandle&) { return (appendDigit(LOAD_MARKER_FILE)); } -int unload() { +int +unload() { return (appendDigit(UNLOAD_MARKER_FILE)); } diff --git a/src/bin/dhcp4/tests/config_parser_unittest.cc b/src/bin/dhcp4/tests/config_parser_unittest.cc index e7eb3ab357..7c011455b7 100644 --- a/src/bin/dhcp4/tests/config_parser_unittest.cc +++ b/src/bin/dhcp4/tests/config_parser_unittest.cc @@ -60,6 +60,15 @@ public: CfgMgr::instance().deleteActiveIfaces(); } + // Check that no hooks libraries are loaded. This is a pre-condition for + // a number of tests, so is checked in one place. As this uses an + // ASSERT call - and it is not clear from the documentation that Gtest + // predicates can be used in a constructor - the check is placed in SetUp. + void SetUp() { + std::vector libraries = HooksManager::getLibraryNames(); + ASSERT_TRUE(libraries.empty()); + } + // Checks if global parameter of name have expected_value void checkGlobalUint32(string name, uint32_t expected_value) { const Uint32StoragePtr uint32_defaults = @@ -317,10 +326,10 @@ public: "reset configuration database")); } - boost::scoped_ptr srv_; - int rcode_; - ConstElementPtr comment_; + boost::scoped_ptr srv_; // DHCP4 server under test + int rcode_; // Return code from element parsing + ConstElementPtr comment_; // Reason for parse fail }; // Goal of this test is a verification if a very simple config update @@ -1785,7 +1794,9 @@ TEST_F(Dhcp4ParserTest, stdOptionDataEncapsulate) { EXPECT_FALSE(desc.option->getOption(3)); } -// Tests of the hooks libraries configuration. +// Tests of the hooks libraries configuration. All tests have the pre- +// condition (checked in the test fixture's SetUp() method) that no hooks +// libraries are loaded at the start of the tests. // Helper function to return a configuration containing an arbitrary number // of hooks libraries. @@ -1862,10 +1873,6 @@ buildHooksLibrariesConfig(const char* library1 = NULL, // The goal of this test is to verify the configuration of hooks libraries if // none are specified. TEST_F(Dhcp4ParserTest, NoHooksLibraries) { - // Ensure that no libraries are loaded at the start of the test. - std::vector libraries = HooksManager::getLibraryNames(); - ASSERT_TRUE(libraries.empty()); - // Parse a configuration containing no names. string config = buildHooksLibrariesConfig(); if (!executeConfiguration(config, @@ -1874,17 +1881,13 @@ TEST_F(Dhcp4ParserTest, NoHooksLibraries) { } else { // No libraries should be loaded at the end of the test. - libraries = HooksManager::getLibraryNames(); + std::vector libraries = HooksManager::getLibraryNames(); EXPECT_TRUE(libraries.empty()); } } // Verify parsing fails with one library that will fail validation. TEST_F(Dhcp4ParserTest, InvalidLibrary) { - // Ensure that no libraries are loaded at the start of the test. - std::vector libraries = HooksManager::getLibraryNames(); - ASSERT_TRUE(libraries.empty()); - // Parse a configuration containing a failing library. string config = buildHooksLibrariesConfig(NOT_PRESENT_LIBRARY); @@ -1902,10 +1905,6 @@ TEST_F(Dhcp4ParserTest, InvalidLibrary) { // Verify the configuration of hooks libraries with two being specified. TEST_F(Dhcp4ParserTest, LibrariesSpecified) { - // Ensure that no libraries are loaded at the start of the test. - std::vector libraries = HooksManager::getLibraryNames(); - ASSERT_TRUE(libraries.empty()); - // Marker files should not be present. EXPECT_FALSE(checkMarkerFileExists(LOAD_MARKER_FILE)); EXPECT_FALSE(checkMarkerFileExists(UNLOAD_MARKER_FILE)); @@ -1918,7 +1917,7 @@ TEST_F(Dhcp4ParserTest, LibrariesSpecified) { // Expect two libraries to be loaded in the correct order (load marker file // is present, no unload marker file). - libraries = HooksManager::getLibraryNames(); + std::vector libraries = HooksManager::getLibraryNames(); ASSERT_EQ(2, libraries.size()); EXPECT_TRUE(checkMarkerFile(LOAD_MARKER_FILE, "12")); EXPECT_FALSE(checkMarkerFileExists(UNLOAD_MARKER_FILE)); diff --git a/src/bin/dhcp6/config_parser.cc b/src/bin/dhcp6/config_parser.cc index 41c7d3180a..63bda525e6 100644 --- a/src/bin/dhcp6/config_parser.cc +++ b/src/bin/dhcp6/config_parser.cc @@ -572,6 +572,13 @@ configureDhcp6Server(Dhcpv6Srv&, isc::data::ConstElementPtr config_set) { if (iface_parser) { iface_parser->commit(); } + + // 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. + if (hooks_parser) { + hooks_parser->commit(); + } } catch (const isc::Exception& ex) { LOG_ERROR(dhcp6_logger, DHCP6_PARSER_COMMIT_FAIL).arg(ex.what()); @@ -595,12 +602,6 @@ configureDhcp6Server(Dhcpv6Srv&, isc::data::ConstElementPtr config_set) { return (answer); } - // Now commit any changes that have been validated but not yet committed, - // and which can't be rolled back. - if (hooks_parser) { - hooks_parser->commit(); - } - LOG_INFO(dhcp6_logger, DHCP6_CONFIG_COMPLETE).arg(config_details); // Everything was fine. Configuration is successful. diff --git a/src/bin/dhcp6/ctrl_dhcp6_srv.cc b/src/bin/dhcp6/ctrl_dhcp6_srv.cc index 0d07d8b253..7168b91f8b 100644 --- a/src/bin/dhcp6/ctrl_dhcp6_srv.cc +++ b/src/bin/dhcp6/ctrl_dhcp6_srv.cc @@ -145,13 +145,14 @@ ControlledDhcpv6Srv::dhcp6CommandHandler(const string& command, ConstElementPtr ConstElementPtr answer = isc::config::createAnswer(0, "Shutting down."); return (answer); + } else if (command == "libreload") { // TODO delete any stored CalloutHandles referring to the old libraries // Get list of currently loaded libraries and reload them. vector loaded = HooksManager::getLibraryNames(); bool status = HooksManager::loadLibraries(loaded); if (!status) { - LOG_ERROR(dhcp6_logger, DHCP6_RELOAD_FAIL); + LOG_ERROR(dhcp6_logger, DHCP6_HOOKS_LIBS_RELOAD_FAIL); ConstElementPtr answer = isc::config::createAnswer(1, "Failed to reload hooks libraries."); return (answer); diff --git a/src/bin/dhcp6/dhcp6_messages.mes b/src/bin/dhcp6/dhcp6_messages.mes index f4dc8fc281..a6bae3e5cb 100644 --- a/src/bin/dhcp6/dhcp6_messages.mes +++ b/src/bin/dhcp6/dhcp6_messages.mes @@ -96,6 +96,11 @@ subnet, an action that severely limits further processing; the server will be only able to offer global options - no addresses or prefixes will be assigned. +% DHCP6_HOOKS_LIBS_RELOAD_FAIL reload of hooks libraries failed +A "libreload" command was issued to reload the hooks libraries but for +some reason the reload failed. Other error messages issued from the +hooks framework will indicate the nature of the problem. + % DHCP6_LEASE_ADVERT lease %1 advertised (client duid=%2, iaid=%3) This debug message indicates that the server successfully advertised a lease. It is up to the client to choose one server out of the @@ -246,11 +251,6 @@ mandatory client-id option. This is most likely caused by a buggy client (or a relay that malformed forwarded message). This request will not be processed and a response with error status code will be sent back. -% DHCP6_RELOAD_FAIL reload of hooks libraries failed -A "libreload" command was issued to reload the hooks libraries but for -some reason the reload failed. Other error messages issued from the -hooks framework will indicate the nature of the problem. - % DHCP6_RENEW_UNKNOWN_SUBNET RENEW message received from client on unknown subnet (duid=%1, iaid=%2) A warning message indicating that a client is attempting to renew his lease, but the server does not have any information about the subnet this client belongs diff --git a/src/bin/dhcp6/tests/callout_library_common.h b/src/bin/dhcp6/tests/callout_library_common.h index e8d4b5ac60..cbabcda094 100644 --- a/src/bin/dhcp6/tests/callout_library_common.h +++ b/src/bin/dhcp6/tests/callout_library_common.h @@ -21,9 +21,9 @@ /// To check that they libraries are loaded and unloaded correctly, the load /// and unload functions in this library maintain two marker files - the load /// marker file and the unload marker file. The functions append a single -/// to the single line in the file, creating the file if need be. In -/// this way, the test code can determine whether the load/unload functions -/// have been run and, if so, in what order. +/// line to the file, creating the file if need be. In this way, the test code +/// can determine whether the load/unload functions have been run and, if so, +/// in what order. /// /// This file is the common library file for the tests. It will not compile /// by itself - it is included into each callout library which specifies the @@ -69,11 +69,13 @@ version() { return (BIND10_HOOKS_VERSION); } -int load(LibraryHandle&) { +int +load(LibraryHandle&) { return (appendDigit(LOAD_MARKER_FILE)); } -int unload() { +int +unload() { return (appendDigit(UNLOAD_MARKER_FILE)); } diff --git a/src/bin/dhcp6/tests/config_parser_unittest.cc b/src/bin/dhcp6/tests/config_parser_unittest.cc index a7a60c6ca4..0c46d263b3 100644 --- a/src/bin/dhcp6/tests/config_parser_unittest.cc +++ b/src/bin/dhcp6/tests/config_parser_unittest.cc @@ -81,6 +81,15 @@ public: resetConfiguration(); } + // Check that no hooks libraries are loaded. This is a pre-condition for + // a number of tests, so is checked in one place. As this uses an + // ASSERT call - and it is not clear from the documentation that Gtest + // predicates can be used in a constructor - the check is placed in SetUp. + void SetUp() { + std::vector libraries = HooksManager::getLibraryNames(); + ASSERT_TRUE(libraries.empty()); + } + ~Dhcp6ParserTest() { // Reset configuration database after each test. resetConfiguration(); @@ -1895,7 +1904,10 @@ TEST_F(Dhcp6ParserTest, stdOptionDataEncapsulate) { EXPECT_FALSE(desc.option->getOption(112)); } -// Tests of the hooks libraries configuration. +// Tests of the hooks libraries configuration. All tests have the pre- +// condition (checked in the test fixture's SetUp() method) that no hooks +// libraries are loaded at the start of the tests. + // Helper function to return a configuration containing an arbitrary number // of hooks libraries. @@ -1977,10 +1989,6 @@ buildHooksLibrariesConfig(const char* library1 = NULL, // The goal of this test is to verify the configuration of hooks libraries if // none are specified. TEST_F(Dhcp6ParserTest, NoHooksLibraries) { - // Ensure that no libraries are loaded at the start of the test. - std::vector libraries = HooksManager::getLibraryNames(); - ASSERT_TRUE(libraries.empty()); - // Parse a configuration containing no names. string config = buildHooksLibrariesConfig(); if (!executeConfiguration(config, @@ -1989,17 +1997,13 @@ TEST_F(Dhcp6ParserTest, NoHooksLibraries) { } else { // No libraries should be loaded at the end of the test. - libraries = HooksManager::getLibraryNames(); + std::vector libraries = HooksManager::getLibraryNames(); EXPECT_TRUE(libraries.empty()); } } // Verify parsing fails with one library that will fail validation. TEST_F(Dhcp6ParserTest, InvalidLibrary) { - // Ensure that no libraries are loaded at the start of the test. - std::vector libraries = HooksManager::getLibraryNames(); - ASSERT_TRUE(libraries.empty()); - // Parse a configuration containing a failing library. string config = buildHooksLibrariesConfig(NOT_PRESENT_LIBRARY); @@ -2017,10 +2021,6 @@ TEST_F(Dhcp6ParserTest, InvalidLibrary) { // Verify the configuration of hooks libraries with two being specified. TEST_F(Dhcp6ParserTest, LibrariesSpecified) { - // Ensure that no libraries are loaded at the start of the test. - std::vector libraries = HooksManager::getLibraryNames(); - ASSERT_TRUE(libraries.empty()); - // Marker files should not be present. EXPECT_FALSE(checkMarkerFileExists(LOAD_MARKER_FILE)); EXPECT_FALSE(checkMarkerFileExists(UNLOAD_MARKER_FILE)); @@ -2033,10 +2033,10 @@ TEST_F(Dhcp6ParserTest, LibrariesSpecified) { // Expect two libraries to be loaded in the correct order (load marker file // is present, no unload marker file). - libraries = HooksManager::getLibraryNames(); - ASSERT_EQ(2, libraries.size()); - EXPECT_TRUE(checkMarkerFile(LOAD_MARKER_FILE, "12")); - EXPECT_FALSE(checkMarkerFileExists(UNLOAD_MARKER_FILE)); + std::vector libraries = HooksManager::getLibraryNames(); + ASSERT_EQ(2, libraries.size()); + EXPECT_TRUE(checkMarkerFile(LOAD_MARKER_FILE, "12")); + EXPECT_FALSE(checkMarkerFileExists(UNLOAD_MARKER_FILE)); // Unload the libraries. The load file should not have changed, but // the unload one should indicate the unload() functions have been run. diff --git a/src/lib/dhcpsrv/dhcp_parsers.h b/src/lib/dhcpsrv/dhcp_parsers.h index 49c917c78e..0184a5c0d6 100644 --- a/src/lib/dhcpsrv/dhcp_parsers.h +++ b/src/lib/dhcpsrv/dhcp_parsers.h @@ -338,7 +338,7 @@ private: std::string param_name_; }; -/// @brief parser for hooks library list +/// @brief Parser for hooks library list /// /// This parser handles the list of hooks libraries. This is an optional list, /// which may be empty. @@ -364,7 +364,7 @@ public: /// @brief Constructor /// /// As this is a dedicated parser, it must be used to parse - /// "hooks_libraries" parameter only. All other types will throw exception. + /// "hooks-libraries" parameter only. All other types will throw exception. /// /// @param param_name name of the configuration parameter being parsed. /// @@ -395,9 +395,9 @@ public: /// an indication as to whether the list is different from the list of /// libraries already loaded. /// - /// @param libraries (out) List of libraries that were specified in the + /// @param libraries [out] List of libraries that were specified in the /// new configuration. - /// @param changed (out) true if the list is different from that currently + /// @param changed [out] true if the list is different from that currently /// loaded. void getLibraries(std::vector& libraries, bool& changed);