From: Stephen Morris Date: Fri, 11 Jan 2013 18:40:56 +0000 (+0000) Subject: [2559] Remove database access string from the parser class X-Git-Tag: bind10-1.0.0-rc-release~76^2~21^2~10 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=c61ae0fbbba155063ada04a08ec44f3dc27ab6bc;p=thirdparty%2Fkea.git [2559] Remove database access string from the parser class This is now constructed "ont the fly" when the configuration change is committed. --- diff --git a/src/lib/dhcpsrv/dbaccess_parser.cc b/src/lib/dhcpsrv/dbaccess_parser.cc index 93b7870375..e822e03144 100644 --- a/src/lib/dhcpsrv/dbaccess_parser.cc +++ b/src/lib/dhcpsrv/dbaccess_parser.cc @@ -13,6 +13,8 @@ // PERFORMANCE OF THIS SOFTWARE. #include +#include +#include #include @@ -26,10 +28,14 @@ using namespace isc::data; namespace isc { namespace dhcp { -typedef map ConfigPairMap; -typedef pair ConfigPair; -typedef map StringPairMap; -typedef pair 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 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 235da04b5b..01d757ebb8 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 ConfigPair; + + /// @brief Keyword and associated value + typedef std::pair StringPair; + + /// @brief Keyword/value collection of database access parameters + typedef std::map 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 values_; - ///< Stored parameter values + std::map values_; ///< Stored parameter values }; }; // namespace dhcp diff --git a/src/lib/dhcpsrv/dhcpsrv_messages.mes b/src/lib/dhcpsrv/dhcpsrv_messages.mes index ee057dfec8..cc8da23732 100644 --- a/src/lib/dhcpsrv/dhcpsrv_messages.mes +++ b/src/lib/dhcpsrv/dhcpsrv_messages.mes @@ -233,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/tests/dbaccess_parser_unittest.cc b/src/lib/dhcpsrv/tests/dbaccess_parser_unittest.cc index 6c22fad9d1..83bae85f39 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 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::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(parser.get()); EXPECT_NE(static_cast(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