BIND 10 trac2559, updated. c61ae0fbbba155063ada04a08ec44f3dc27ab6bc [2559] Remove database access string from the parser class
BIND 10 source code commits
bind10-changes at lists.isc.org
Fri Jan 11 18:41:50 UTC 2013
The branch, trac2559 has been updated
via c61ae0fbbba155063ada04a08ec44f3dc27ab6bc (commit)
via 808decc6c1639304e030a84d2bb2a04bd1b27db8 (commit)
via f4e272c729e2a77f720b06a22819013f6695f06b (commit)
from ff5861d1c7d7a689d775b251086d12032fcabd54 (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 c61ae0fbbba155063ada04a08ec44f3dc27ab6bc
Author: Stephen Morris <stephen at isc.org>
Date: Fri Jan 11 18:40:56 2013 +0000
[2559] Remove database access string from the parser class
This is now constructed "ont the fly" when the configuration change
is committed.
commit 808decc6c1639304e030a84d2bb2a04bd1b27db8
Author: Stephen Morris <stephen at isc.org>
Date: Fri Jan 11 08:38:05 2013 +0000
[2559] Add database access parser to DHCPv4 server
commit f4e272c729e2a77f720b06a22819013f6695f06b
Author: Stephen Morris <stephen at isc.org>
Date: Fri Jan 11 08:37:15 2013 +0000
[2559] Add debug message when database is closed
-----------------------------------------------------------------------
Summary of changes:
src/bin/dhcp4/config_parser.cc | 2 +
src/bin/dhcp4/dhcp4.spec | 38 ++++++
src/lib/dhcpsrv/dbaccess_parser.cc | 42 ++++---
src/lib/dhcpsrv/dbaccess_parser.h | 34 ++++--
src/lib/dhcpsrv/dhcpsrv_messages.mes | 14 +++
src/lib/dhcpsrv/lease_mgr_factory.cc | 6 +
src/lib/dhcpsrv/tests/dbaccess_parser_unittest.cc | 128 +++++++--------------
7 files changed, 150 insertions(+), 114 deletions(-)
-----------------------------------------------------------------------
diff --git a/src/bin/dhcp4/config_parser.cc b/src/bin/dhcp4/config_parser.cc
index 795b61e..5d353ba 100644
--- a/src/bin/dhcp4/config_parser.cc
+++ b/src/bin/dhcp4/config_parser.cc
@@ -18,6 +18,7 @@
#include <dhcp/libdhcp++.h>
#include <dhcp/option_definition.h>
#include <dhcpsrv/cfgmgr.h>
+#include <dhcpsrv/dbaccess_parser.h>
#include <dhcpsrv/dhcp_config_parser.h>
#include <util/encode/hex.h>
#include <util/strutil.h>
@@ -1320,6 +1321,7 @@ DhcpConfigParser* createGlobalDhcp4ConfigParser(const std::string& config_id) {
factories["subnet4"] = Subnets4ListConfigParser::factory;
factories["option-data"] = OptionDataListParser::factory;
factories["version"] = StringParser::factory;
+ factories["lease-database"] = DbAccessParser::factory;
FactoryMap::iterator f = factories.find(config_id);
if (f == factories.end()) {
diff --git a/src/bin/dhcp4/dhcp4.spec b/src/bin/dhcp4/dhcp4.spec
index 13acf83..4c5f261 100644
--- a/src/bin/dhcp4/dhcp4.spec
+++ b/src/bin/dhcp4/dhcp4.spec
@@ -70,6 +70,44 @@
}
},
+ { "item_name": "lease-database",
+ "item_type": "map",
+ "item_optional": false,
+ "item_default": {"type": "memfile"},
+ "map_item_spec": [
+ {
+ "item_name": "type",
+ "item_type": "string",
+ "item_optional": false,
+ "item_default": ""
+ },
+ {
+ "item_name": "name",
+ "item_type": "string",
+ "item_optional": true,
+ "item_default": ""
+ },
+ {
+ "item_name": "user",
+ "item_type": "string",
+ "item_optional": true,
+ "item_default": ""
+ },
+ {
+ "item_name": "host",
+ "item_type": "string",
+ "item_optional": true,
+ "item_default": ""
+ },
+ {
+ "item_name": "password",
+ "item_type": "string",
+ "item_optional": true,
+ "item_default": ""
+ }
+ ]
+ },
+
{ "item_name": "subnet4",
"item_type": "list",
"item_optional": false,
diff --git a/src/lib/dhcpsrv/dbaccess_parser.cc b/src/lib/dhcpsrv/dbaccess_parser.cc
index 93b7870..e822e03 100644
--- a/src/lib/dhcpsrv/dbaccess_parser.cc
+++ b/src/lib/dhcpsrv/dbaccess_parser.cc
@@ -13,6 +13,8 @@
// PERFORMANCE OF THIS SOFTWARE.
#include <dhcpsrv/dbaccess_parser.h>
+#include <dhcpsrv/dhcpsrv_log.h>
+#include <dhcpsrv/lease_mgr_factory.h>
#include <boost/foreach.hpp>
@@ -26,10 +28,14 @@ using namespace isc::data;
namespace isc {
namespace dhcp {
-typedef map<string, ConstElementPtr> ConfigPairMap;
-typedef pair<string, ConstElementPtr> ConfigPair;
-typedef map<string, string> StringPairMap;
-typedef pair<string, string> StringPair;
+
+// Factory function to build the parser
+DbAccessParser::DbAccessParser(const std::string& param_name) : values_()
+{
+ if (param_name != "lease-database") {
+ LOG_WARN(dhcpsrv_logger, DHCPSRV_UNEXPECTED_NAME).arg(param_name);
+ }
+}
// Parse the configuration and check that the various keywords are consistent.
void
@@ -46,7 +52,7 @@ DbAccessParser::build(isc::data::ConstElementPtr config_value) {
map<string, string> values_copy = values_;
// 2. Update the copy with the passed keywords.
- BOOST_FOREACH(ConfigPair param, config_value->mapValue()) {
+ BOOST_FOREACH(DbAccessParser::ConfigPair param, config_value->mapValue()) {
values_copy[param.first] = param.second->stringValue();
}
@@ -71,28 +77,32 @@ DbAccessParser::build(isc::data::ConstElementPtr config_value) {
// 4. If all is OK, update the stored keyword/value pairs.
values_ = values_copy;
+}
+
+// Commit the changes - reopen the database with the new parameters
+void
+DbAccessParser::commit() {
+ // Close current lease manager.
+ LeaseMgrFactory::destroy();
- // 5. Construct the updated database access string: omit keywords where
- // the value string is empty.
- dbaccess_ = "";
+ // Construct the database access string from all keywords and values in the
+ // parameter map where the value is not null.
+ string dbaccess;
BOOST_FOREACH(StringPair keyval, values_) {
if (!keyval.second.empty()) {
// Separate keyword/value pair from predecessor (if there is one).
- if (! dbaccess_.empty()) {
- dbaccess_ += std::string(" ");
+ if (! dbaccess.empty()) {
+ dbaccess += std::string(" ");
}
// Add the keyword/value pair to the access string.
- dbaccess_ += (keyval.first + std::string("=") + keyval.second);
+ dbaccess += (keyval.first + std::string("=") + keyval.second);
}
}
-}
-// Commit the changes - reopen the database with the new parameters
-void
-DbAccessParser::commit() {
- std::cout << "DB_ACCESS_PARSER_COMMIT: " << dbaccess_ << "\n";
+ // ... and open the database using that access string.
+ LeaseMgrFactory::create(dbaccess);
}
}; // namespace dhcp
diff --git a/src/lib/dhcpsrv/dbaccess_parser.h b/src/lib/dhcpsrv/dbaccess_parser.h
index 235da04..01d757e 100644
--- a/src/lib/dhcpsrv/dbaccess_parser.h
+++ b/src/lib/dhcpsrv/dbaccess_parser.h
@@ -44,13 +44,20 @@ public:
/// depend on the datbase chosen.
class DbAccessParser: public DhcpConfigParser {
public:
+ /// @brief Combination of parameter name and configuration contents
+ typedef std::pair<std::string, isc::data::ConstElementPtr> ConfigPair;
+
+ /// @brief Keyword and associated value
+ typedef std::pair<std::string, std::string> StringPair;
+
+ /// @brief Keyword/value collection of database access parameters
+ typedef std::map<std::string, std::string> StringPairMap;
/// @brief Default constructor
///
- /// @param param_name Name of the configuration parameter being parsed.
- DbAccessParser(const std::string& param_name)
- : dbaccess_(), param_name_(param_name), values_()
- {}
+ /// @param param_name Name of the parameter under which the database
+ /// access details are held.
+ DbAccessParser(const std::string& param_name);
/// The destructor.
virtual ~DbAccessParser()
@@ -89,23 +96,28 @@ public:
/// @brief Factory method to create parser
///
/// Creates an instance of this parser.
+ ///
+ /// @param name Name of the parameter used to access the configuration.
+ ///
+ /// @return Pointer to a DbAccessParser. The caller is responsible for
+ /// destroying the parser after use.
static DhcpConfigParser* factory(const std::string& param_name) {
return (new DbAccessParser(param_name));
}
- /// @brief Get database access string
+ /// @brief Get database access parameters
///
/// Used in testing to check that the configuration information has been
/// parsed corrected.
- std::string getDbAccessString() const {
- return (dbaccess_);
+ ///
+ /// @return Map of keyword/value pairs representing database access
+ /// information.
+ const StringPairMap& getDbAccessParameters() const {
+ return (values_);
}
private:
- std::string dbaccess_; ///< Database access string
- std::string param_name_; ///< Parameter name
- std::map<std::string, std::string> values_;
- ///< Stored parameter values
+ std::map<std::string, std::string> values_; ///< Stored parameter values
};
}; // namespace dhcp
diff --git a/src/lib/dhcpsrv/dhcpsrv_messages.mes b/src/lib/dhcpsrv/dhcpsrv_messages.mes
index 27f12fc..cc8da23 100644
--- a/src/lib/dhcpsrv/dhcpsrv_messages.mes
+++ b/src/lib/dhcpsrv/dhcpsrv_messages.mes
@@ -60,6 +60,13 @@ This is a debug message reporting that the DHCP configuration manager has
returned the specified IPv6 subnet when given the address hint specified
as the address is within the subnet.
+% DHCPSRV_CLOSE_DB closing currently open %1 database
+This is a debug message, issues when the DHCP server closed the currently
+open lease database. It is issued at program shutdown, and whenever
+the database access parameters are changed: in the latter case, the
+server closes the currently open database, and opens a database using
+the new parameters.
+
% DHCPSRV_INVALID_ACCESS invalid database access string: %1
This is logged when an attempt has been made to parse a database access string
and the attempt ended in error. The access string in question - which
@@ -226,6 +233,13 @@ a database backend, but where no 'type' keyword has been included in
the access string. The access string (less any passwords) is included
in the message.
+% DHCPSRV_UNEXPECTED_NAME database access parameters passed through '%1', expected 'lease-database'
+The parameters for access the lease database were passed to the server through
+the named configuration parameter, but the code was expecting them to be
+passed via the parameter named "lease-database". If the database opens
+successfully, there is no impact on server operation. However, as this does
+indicate an error in the source code, please submit a bug report.
+
% DHCPSRV_UNKNOWN_DB unknown database type: %1
The database access string specified a database type (given in the
message) that is unknown to the software. This is a configuration error.
diff --git a/src/lib/dhcpsrv/lease_mgr_factory.cc b/src/lib/dhcpsrv/lease_mgr_factory.cc
index 9fd276d..ad31d12 100644
--- a/src/lib/dhcpsrv/lease_mgr_factory.cc
+++ b/src/lib/dhcpsrv/lease_mgr_factory.cc
@@ -141,6 +141,12 @@ LeaseMgrFactory::create(const std::string& dbaccess) {
void
LeaseMgrFactory::destroy() {
+ // Destroy current lease manager. This is a no-op if no lease manager
+ // is available.
+ if (getLeaseMgrPtr()) {
+ LOG_DEBUG(dhcpsrv_logger, DHCPSRV_DBG_TRACE, DHCPSRV_CLOSE_DB)
+ .arg(getLeaseMgrPtr()->getType());
+ }
getLeaseMgrPtr().reset();
}
diff --git a/src/lib/dhcpsrv/tests/dbaccess_parser_unittest.cc b/src/lib/dhcpsrv/tests/dbaccess_parser_unittest.cc
index 6c22fad..83bae85 100644
--- a/src/lib/dhcpsrv/tests/dbaccess_parser_unittest.cc
+++ b/src/lib/dhcpsrv/tests/dbaccess_parser_unittest.cc
@@ -101,15 +101,15 @@ public:
///
/// @param trace_string String that will be used to set the value of a
/// SCOPED_TRACE for this call.
- /// @param dbaccess Database access string to check
+ /// @param dbaccess set of database access parameters to check
/// @param keyval Array of "const char*" strings in the order keyword,
/// value, keyword, value ... A NULL entry terminates the list.
- void checkAccessString(const char* trace_string, std::string& dbaccess,
+ void checkAccessString(const char* trace_string,
+ const DbAccessParser::StringPairMap& parameters,
const char* keyval[]) {
SCOPED_TRACE(trace_string);
- // Construct a map of keyword value pairs. Check that no keyword
- // is repeated.
+ // Construct a map of keyword value pairs.
map<string, string> expected;
size_t expected_count = 0;
for (size_t i = 0; keyval[i] != NULL; i += 2) {
@@ -123,20 +123,18 @@ public:
++expected_count;
}
- // Check no duplicates in the supplied keywords
+ // Check no duplicates in the test set of reference keywords.
ASSERT_EQ(expected_count, expected.size()) <<
"Supplied reference keyword/value list contains duplicate keywords";
- // Split the database access string.
- const LeaseMgr::ParameterMap dbamap = LeaseMgrFactory::parse(dbaccess);
-
- // It should have the same number keyword value pairs as the
- EXPECT_EQ(expected_count, dbamap.size());
+ // The passed parameter map should have the same number of entries as
+ // the reference set of keywords.
+ EXPECT_EQ(expected_count, parameters.size());
// Check that the keywords and keyword values are the same: loop
// through the keywords in the database access string.
- for (LeaseMgr::ParameterMap::const_iterator actual = dbamap.begin();
- actual != dbamap.end(); ++actual) {
+ for (LeaseMgr::ParameterMap::const_iterator actual = parameters.begin();
+ actual != parameters.end(); ++actual) {
// Does the keyword exist in the set of expected keywords?
map<string, string>::iterator corresponding =
@@ -160,8 +158,23 @@ TEST_F(DbAccessParserTest, validTypeMemfile) {
DbAccessParser parser("lease-database");
EXPECT_NO_THROW(parser.build(json_elements));
- string dbaccess = parser.getDbAccessString();
- checkAccessString("Valid memfile", dbaccess, config);
+ checkAccessString("Valid memfile", parser.getDbAccessParameters(), config);
+}
+
+// Check that the parser works with a simple configuration that
+// includes empty elements.
+TEST_F(DbAccessParserTest, emptyKeyword) {
+ const char* config[] = {"type", "memfile",
+ "name", "",
+ NULL};
+
+ string json_config = toJson(config);
+ ConstElementPtr json_elements = Element::fromJSON(json_config);
+ EXPECT_TRUE(json_elements);
+
+ DbAccessParser parser("lease-database");
+ EXPECT_NO_THROW(parser.build(json_elements));
+ checkAccessString("Valid memfile", parser.getDbAccessParameters(), config);
}
// Check that the parser works with a valid MySQL configuration
@@ -179,8 +192,7 @@ TEST_F(DbAccessParserTest, validTypeMysql) {
DbAccessParser parser("lease-database");
EXPECT_NO_THROW(parser.build(json_elements));
- string dbaccess = parser.getDbAccessString();
- checkAccessString("Valid mysql", dbaccess, config);
+ checkAccessString("Valid mysql", parser.getDbAccessParameters(), config);
}
// A missing 'type' keyword should cause an exception to be thrown.
@@ -234,8 +246,7 @@ TEST_F(DbAccessParserTest, factory) {
// Access the "raw" parser.
DbAccessParser* dbap = dynamic_cast<DbAccessParser*>(parser.get());
EXPECT_NE(static_cast<DbAccessParser*>(NULL), dbap);
- string dbaccess = dbap->getDbAccessString();
- checkAccessString("Valid mysql", dbaccess, config);
+ checkAccessString("Valid mysql", dbap->getDbAccessParameters(), config);
}
// Check reconfiguration. Checks that incremental changes applied to the
@@ -273,12 +284,12 @@ TEST_F(DbAccessParserTest, incrementalChanges) {
// incremental4 is a compatible change and should cause a transition
// to config4.
const char* incremental4[] = {"user", "them",
- "password", "themagain",
+ "password", "",
NULL};
const char* config4[] = {"type", "mysql",
"host", "erewhon",
"user", "them",
- "password", "themagain",
+ "password", "",
"name", "keatest",
NULL};
@@ -291,8 +302,8 @@ TEST_F(DbAccessParserTest, incrementalChanges) {
EXPECT_TRUE(json_elements);
EXPECT_NO_THROW(parser.build(json_elements));
- string dbaccess = parser.getDbAccessString();
- checkAccessString("Initial configuration", dbaccess, config1);
+ checkAccessString("Initial configuration", parser.getDbAccessParameters(),
+ config1);
// Applying a wholesale change will cause the access string to change
// to a representation of the new configuration.
@@ -301,8 +312,8 @@ TEST_F(DbAccessParserTest, incrementalChanges) {
EXPECT_TRUE(json_elements);
EXPECT_NO_THROW(parser.build(json_elements));
- dbaccess = parser.getDbAccessString();
- checkAccessString("Subsequent configuration", dbaccess, config2);
+ checkAccessString("Subsequent configuration", parser.getDbAccessParameters(),
+ config2);
// Applying an incremental change will cause the representation to change
// incrementally.
@@ -311,8 +322,8 @@ TEST_F(DbAccessParserTest, incrementalChanges) {
EXPECT_TRUE(json_elements);
EXPECT_NO_THROW(parser.build(json_elements));
- dbaccess = parser.getDbAccessString();
- checkAccessString("Incremental configuration", dbaccess, config3);
+ checkAccessString("Incremental configuration", parser.getDbAccessParameters(),
+ config3);
// Applying the next incremental change should cause an exception to be
// thrown and there be no change to the access string.
@@ -321,8 +332,8 @@ TEST_F(DbAccessParserTest, incrementalChanges) {
EXPECT_TRUE(json_elements);
EXPECT_THROW(parser.build(json_elements), BadValue);
- dbaccess = parser.getDbAccessString();
- checkAccessString("Incompatible incremental change", dbaccess, config3);
+ checkAccessString("Incompatible incremental change", parser.getDbAccessParameters(),
+ config3);
// Applying an incremental change will cause the representation to change
// incrementally.
@@ -331,65 +342,8 @@ TEST_F(DbAccessParserTest, incrementalChanges) {
EXPECT_TRUE(json_elements);
EXPECT_NO_THROW(parser.build(json_elements));
- dbaccess = parser.getDbAccessString();
- checkAccessString("Compatible incremental change", dbaccess, config4);
-}
-
-// Check reconfiguration and that elements set to an empty string are omitted.
-TEST_F(DbAccessParserTest, emptyStringOmission) {
- const char* config1[] = {"type", "memfile",
- NULL};
-
- // Applying config2 will cause a wholesale change.
- const char* config2[] = {"type", "mysql",
- "host", "erewhon",
- "user", "kea",
- "password", "keapassword",
- "name", "keatest",
- NULL};
-
- // Applying incremental2 should cause a change to config3.
- const char* incremental2[] = {"user", "me",
- "password", "",
- "host", "",
- NULL};
-
- const char* config3[] = {"type", "mysql",
- "user", "me",
- "name", "keatest",
- NULL};
-
- DbAccessParser parser("lease-database");
-
- // First configuration string should cause a representation of that string
- // to be held.
- string json_config = toJson(config1);
- ConstElementPtr json_elements = Element::fromJSON(json_config);
- EXPECT_TRUE(json_elements);
-
- EXPECT_NO_THROW(parser.build(json_elements));
- string dbaccess = parser.getDbAccessString();
- checkAccessString("Initial configuration", dbaccess, config1);
-
- // Applying a wholesale change will cause the access string to change
- // to a representation of the new configuration.
- json_config = toJson(config2);
- json_elements = Element::fromJSON(json_config);
- EXPECT_TRUE(json_elements);
-
- EXPECT_NO_THROW(parser.build(json_elements));
- dbaccess = parser.getDbAccessString();
- checkAccessString("Subsequent configuration", dbaccess, config2);
-
- // Applying an incremental change will cause the representation to change
- // incrementally.
- json_config = toJson(incremental2);
- json_elements = Element::fromJSON(json_config);
- EXPECT_TRUE(json_elements);
-
- EXPECT_NO_THROW(parser.build(json_elements));
- dbaccess = parser.getDbAccessString();
- checkAccessString("Incremental configuration", dbaccess, config3);
+ checkAccessString("Compatible incremental change", parser.getDbAccessParameters(),
+ config4);
}
}; // Anonymous namespace
More information about the bind10-changes
mailing list