BIND 10 trac2981, updated. 578910f51f1dc96876a67a02d353c396cc4d2ec8 [2981] Changes as a result of review.
BIND 10 source code commits
bind10-changes at lists.isc.org
Tue Aug 13 12:14:54 UTC 2013
The branch, trac2981 has been updated
via 578910f51f1dc96876a67a02d353c396cc4d2ec8 (commit)
from a6cd22451be6684f4bebbc34d5344371afdeaa4b (commit)
Those revisions listed above that are new to this repository have
not appeared on any other notification email; so we list those
revisions in full, below.
- Log -----------------------------------------------------------------
commit 578910f51f1dc96876a67a02d353c396cc4d2ec8
Author: Stephen Morris <stephen at isc.org>
Date: Tue Aug 13 13:14:38 2013 +0100
[2981] Changes as a result of review.
-----------------------------------------------------------------------
Summary of changes:
src/bin/dhcp4/config_parser.cc | 15 ++++++-----
src/bin/dhcp4/ctrl_dhcp4_srv.cc | 3 ++-
src/bin/dhcp4/dhcp4_messages.mes | 10 +++----
src/bin/dhcp4/tests/callout_library_common.h | 12 +++++----
src/bin/dhcp4/tests/config_parser_unittest.cc | 35 ++++++++++++------------
src/bin/dhcp6/config_parser.cc | 13 ++++-----
src/bin/dhcp6/ctrl_dhcp6_srv.cc | 3 ++-
src/bin/dhcp6/dhcp6_messages.mes | 10 +++----
src/bin/dhcp6/tests/callout_library_common.h | 12 +++++----
src/bin/dhcp6/tests/config_parser_unittest.cc | 36 ++++++++++++-------------
src/lib/dhcpsrv/dhcp_parsers.h | 8 +++---
11 files changed, 82 insertions(+), 75 deletions(-)
-----------------------------------------------------------------------
diff --git a/src/bin/dhcp4/config_parser.cc b/src/bin/dhcp4/config_parser.cc
index 7827c02..98393c1 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 ad0c5ec..43c08c9 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<string> 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 2eeda9f..6334999 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 e8d4b5a..cbabcda 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 e7eb3ab..7c01145 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<std::string> 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<Dhcpv4Srv> srv_;
- int rcode_;
- ConstElementPtr comment_;
+ boost::scoped_ptr<Dhcpv4Srv> 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<std::string> 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<std::string> 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<std::string> 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<std::string> 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<std::string> 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 41c7d31..63bda52 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 0d07d8b..7168b91 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<string> 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 f4dc8fc..a6bae3e 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 e8d4b5a..cbabcda 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 a7a60c6..0c46d26 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<std::string> 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<std::string> 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<std::string> 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<std::string> 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<std::string> 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<std::string> 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 49c917c..0184a5c 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<std::string>& libraries, bool& changed);
More information about the bind10-changes
mailing list