[2559] More changes as a result of the ticket review

src/bin/dhcp4/config_parser.cc

@@ -1375,17 +1375,20 @@ configureDhcp4Server(Dhcpv4Srv& , ConstElementPtr config_set) {
     // rollback informs whether error occured and original data
     // have to be restored to global storages.
     bool rollback = false;
-    string current_parser;  // For error messages
+    // config_pair holds the details of the current parser when iterating over
+    // parser.  It is declared outside the loops so in case of an error, the
+    // name of the failing parser can be retrieved in the "catch" clause.
+    ConfigPair config_pair;
     try {
 
         // Iterate over all independent parsers first (all but subnet4)
         // and try to parse the data.
-        BOOST_FOREACH(ConfigPair config_pair, config_set->mapValue()) {
+        BOOST_FOREACH(config_pair, config_set->mapValue()) {
             if (config_pair.first != "subnet4") {
-                current_parser = config_pair.first;
-                ParserPtr parser(createGlobalDhcp4ConfigParser(config_pair.first));
+                ParserPtr parser(createGlobalDhcp4ConfigParser(
+                                     config_pair.first));
                 LOG_DEBUG(dhcp4_logger, DBG_DHCP4_DETAIL, DHCP4_PARSER_CREATED1)
-                          .arg(current_parser);
+                          .arg(config_pair.first);
                 independent_parsers.push_back(parser);
                 parser->build(config_pair.second);
                 // The commit operation here may modify the global storage
@@ -1396,33 +1399,31 @@ configureDhcp4Server(Dhcpv4Srv& , ConstElementPtr config_set) {
         }
 
         // Process dependent configuration data.
-        BOOST_FOREACH(ConfigPair config_pair, config_set->mapValue()) {
+        BOOST_FOREACH(config_pair, config_set->mapValue()) {
             if (config_pair.first == "subnet4") {
-                current_parser = config_pair.first;
-                ParserPtr parser(createGlobalDhcp4ConfigParser(config_pair.first));
+                ParserPtr parser(createGlobalDhcp4ConfigParser(
+                                    config_pair.first));
                 LOG_DEBUG(dhcp4_logger, DBG_DHCP4_DETAIL, DHCP4_PARSER_CREATED2)
-                          .arg(current_parser);
+                          .arg(config_pair.first);
                 dependent_parsers.push_back(parser);
                 parser->build(config_pair.second);
             }
         }
 
     } catch (const isc::Exception& ex) {
-        LOG_ERROR(dhcp4_logger, DHCP4_PARSER_CREATE_FAIL).arg(current_parser)
-                  .arg(ex.what());
-        answer =
-            isc::config::createAnswer(1, string("Configuration parsing failed: ") + ex.what());
-
+        LOG_ERROR(dhcp4_logger, DHCP4_PARSER_CREATE_FAIL)
+                  .arg(config_pair.first).arg(ex.what());
+        answer = isc::config::createAnswer(1,
+                     string("Configuration parsing failed: ") + ex.what());
         // An error occured, so make sure that we restore original data.
         rollback = true;
 
     } catch (...) {
         // for things like bad_cast in boost::lexical_cast
         LOG_ERROR(dhcp4_logger, DHCP4_PARSER_CREATE_EXCEPTION)
-                  .arg(current_parser);
-        answer =
-            isc::config::createAnswer(1, string("Configuration parsing failed"));
-
+                  .arg(config_pair.first);
+        answer = isc::config::createAnswer(1,
+                     string("Configuration parsing failed"));
         // An error occured, so make sure that we restore original data.
         rollback = true;
     }

src/bin/dhcp6/config_parser.cc

@@ -1411,16 +1411,19 @@ configureDhcp6Server(Dhcpv6Srv& , ConstElementPtr config_set) {
     // rollback informs whether error occured and original data
     // have to be restored to global storages.
     bool rollback = false;
-    string current_parser;  // For error messages
+    // config_pair holds the details of the current parser when iterating over
+    // parser.  It is declared outside the loops so in case of an error, the
+    // name of the failing parser can be retrieved in the "catch" clause.
+    ConfigPair config_pair;
     try {
         // Iterate over all independent parsers first (all but subnet6)
         // and try to parse the data.
-        BOOST_FOREACH(ConfigPair config_pair, config_set->mapValue()) {
+        BOOST_FOREACH(config_pair, config_set->mapValue()) {
             if (config_pair.first != "subnet6") {
-                current_parser = config_pair.first;
-                ParserPtr parser(createGlobalDhcpConfigParser(config_pair.first));
+                ParserPtr parser(createGlobalDhcpConfigParser(
+                                     config_pair.first));
                 LOG_DEBUG(dhcp6_logger, DBG_DHCP6_DETAIL, DHCP6_PARSER_CREATED1)
-                          .arg(current_parser);
+                          .arg(config_pair.first);
                 independent_parsers.push_back(parser);
                 parser->build(config_pair.second);
                 // The commit operation here may modify the global storage
@@ -1431,31 +1434,31 @@ configureDhcp6Server(Dhcpv6Srv& , ConstElementPtr config_set) {
         }
 
         // Process dependent configuration data.
-        BOOST_FOREACH(ConfigPair config_pair, config_set->mapValue()) {
+        BOOST_FOREACH(config_pair, config_set->mapValue()) {
             if (config_pair.first == "subnet6") {
-                current_parser = config_pair.first;
-                ParserPtr parser(createGlobalDhcpConfigParser(config_pair.first));
+                ParserPtr parser(createGlobalDhcpConfigParser(
+                                     config_pair.first));
                 LOG_DEBUG(dhcp6_logger, DBG_DHCP6_DETAIL, DHCP6_PARSER_CREATED2)
-                          .arg(current_parser);
+                          .arg(config_pair.first);
                 dependent_parsers.push_back(parser);
                 parser->build(config_pair.second);
             }
         }
 
     } catch (const isc::Exception& ex) {
-        LOG_ERROR(dhcp6_logger, DHCP6_PARSER_CREATE_FAIL).arg(current_parser)
-                  .arg(ex.what());
-        answer =
-            isc::config::createAnswer(1, string("Configuration parsing failed: ") + ex.what());
+        LOG_ERROR(dhcp6_logger, DHCP6_PARSER_CREATE_FAIL)
+                  .arg(config_pair.first).arg(ex.what());
+        answer = isc::config::createAnswer(1,
+                     string("Configuration parsing failed: ") + ex.what());
         // An error occured, so make sure that we restore original data.
         rollback = true;
 
     } catch (...) {
         // for things like bad_cast in boost::lexical_cast
         LOG_ERROR(dhcp6_logger, DHCP6_PARSER_CREATE_EXCEPTION)
-                  .arg(current_parser);
-        answer =
-            isc::config::createAnswer(1, string("Configuration parsing failed"));
+                  .arg(config_pair.first);
+        answer = isc::config::createAnswer(1,
+                     string("Configuration parsing failed"));
         // An error occured, so make sure that we restore original data.
         rollback = true;
     }

src/lib/dhcpsrv/dbaccess_parser.cc

@@ -51,7 +51,7 @@ DbAccessParser::build(isc::data::ConstElementPtr config_value) {
     map<string, string> values_copy = values_;
 
     // 2. Update the copy with the passed keywords.
-    BOOST_FOREACH(DbAccessParser::ConfigPair param, config_value->mapValue()) {
+    BOOST_FOREACH(ConfigPair param, config_value->mapValue()) {
         values_copy[param.first] = param.second->stringValue();
     }
 
@@ -71,15 +71,16 @@ DbAccessParser::build(isc::data::ConstElementPtr config_value) {
         isc_throw(BadValue, "unknown backend database type: " << dbtype);
     }
 
-    // 4. If all is OK, update the stored keyword/value pairs.
-    values_ = values_copy;
+    // 4. If all is OK, update the stored keyword/value pairs.  We do this by
+    // swapping contents - values_copy is destroyed immediately after the
+    // operation (when the method exits), so we are not interested in its new
+    // value.
+    values_.swap(values_copy);
 }
 
-// Commit the changes - reopen the database with the new parameters
-void
-DbAccessParser::commit() {
-    // Close current lease manager.
-    LeaseMgrFactory::destroy();
+// Create the database access string
+std::string
+DbAccessParser::getDbAccessString() const {
 
     // Construct the database access string from all keywords and values in the
     // parameter map where the value is not null.
@@ -88,7 +89,7 @@ DbAccessParser::commit() {
         if (!keyval.second.empty()) {
 
             // Separate keyword/value pair from predecessor (if there is one).
-            if (! dbaccess.empty()) {
+            if (!dbaccess.empty()) {
                 dbaccess += std::string(" ");
             }
 
@@ -97,8 +98,17 @@ DbAccessParser::commit() {
         }
     }
 
-    // ... and open the database using that access string.
-    LeaseMgrFactory::create(dbaccess);
+    return (dbaccess);
+}
+
+// Commit the changes - reopen the database with the new parameters
+void
+DbAccessParser::commit() {
+    // Close current lease manager database.
+    LeaseMgrFactory::destroy();
+
+    // ... and open the new database using the access string.
+    LeaseMgrFactory::create(getDbAccessString());
 }
 
 };  // namespace dhcp

src/lib/dhcpsrv/dbaccess_parser.h

@@ -44,16 +44,13 @@ 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
+    /// @brief Constructor
     ///
     /// @param param_name Name of the parameter under which the database
     ///        access details are held.
@@ -105,17 +102,26 @@ public:
         return (new DbAccessParser(param_name));
     }
 
+protected:
     /// @brief Get database access parameters
     ///
     /// Used in testing to check that the configuration information has been
-    /// parsed corrected.
+    /// parsed correctly.
     ///
-    /// @return Map of keyword/value pairs representing database access
-    ///         information.
+    /// @return Reference to the internal map of keyword/value pairs
+    ///         representing database access information.  This is valid only
+    ///         for so long as the the parser remains in existence.
     const StringPairMap& getDbAccessParameters() const {
         return (values_);
     }
 
+    /// @brief Construct dbtabase access string
+    ///
+    /// Constructs the database access string from the stored parameters.
+    ///
+    /// @return Database access string
+    std::string getDbAccessString() const;
+
 private:
     std::map<std::string, std::string> values_; ///< Stored parameter values
 };

src/lib/dhcpsrv/dhcp_config_parser.h

@@ -32,6 +32,9 @@ typedef boost::shared_ptr<DhcpConfigParser> ParserPtr;
 /// This container is used to store pointer to parsers for a given scope.
 typedef std::vector<ParserPtr> ParserCollection;
 
+/// @brief Combination of parameter name and configuration contents
+typedef std::pair<std::string, isc::data::ConstElementPtr> ConfigPair;
+
 /// @brief Base abstract class for all DHCP parsers
 ///
 /// Each instance of a class derived from this class parses one specific config

src/lib/dhcpsrv/dhcpsrv_messages.mes

@@ -1,4 +1,4 @@
-# Copyright (C) 2012  Internet Systems Consortium, Inc. ("ISC")
+# Copyright (C) 2012-2013  Internet Systems Consortium, Inc. ("ISC")
 #
 # Permission to use, copy, modify, and/or distribute this software for any
 # purpose with or without fee is hereby granted, provided that the above
@@ -150,7 +150,7 @@ lease from the memory file database for the specified address.
 A debug message issued when the server is attempting to update IPv6
 lease from the memory file database for the specified address.
 
-% DHCPSRV_MEMFILE_WARNING using early version of memfile - leases will be lost after a restart
+% DHCPSRV_MEMFILE_WARNING using early version of memfile lease database - leases will be lost after a restart
 This warning message is issued when the 'memfile' lease database is
 opened.  The current version of memfile does not store anything
 to disk, so lease information will be lost in the event of a restart.

src/lib/dhcpsrv/tests/dbaccess_parser_unittest.cc

@@ -19,14 +19,9 @@
 #include <config/ccsession.h>
 #include <gtest/gtest.h>
 
-#include <fstream>
-#include <iostream>
 #include <map>
-#include <sstream>
 #include <string>
 
-#include <arpa/inet.h>
-
 using namespace std;
 using namespace isc;
 using namespace isc::dhcp;
@@ -38,7 +33,22 @@ namespace {
 /// @brief Database Access Parser test fixture class
 class DbAccessParserTest : public ::testing::Test {
 public:
-    /// @ Build JSON String
+    /// @brief Constructor
+    ///
+    /// Just make sure that the lease database is closed before every test
+    /// (the first in particular).
+    DbAccessParserTest() {
+        LeaseMgrFactory::destroy();
+    }
+    /// @brief Destructor
+    ///
+    /// Just make sure that the lease database is closed after every test
+    /// (the last in particular).
+    ~DbAccessParserTest() {
+        LeaseMgrFactory::destroy();
+    }
+
+    /// @brief Build JSON String
     ///
     /// Given a array of "const char*" strings representing in order, keyword,
     /// value, keyword, value, ... and terminated by a NULL, return a string
@@ -147,6 +157,47 @@ public:
     }
 };
 
+
+/// @brief Version of parser with protected methods public
+///
+/// Some of the methods in DbAccessParser are not required to be public in
+/// BIND 10.  Instead of being declared "private", they are declared "protected"
+/// so that they can be accessed through a derived class in the unit tests.
+class TestDbAccessParser : public DbAccessParser {
+public:
+
+    /// @brief Constructor
+    ///
+    /// @brief Keyword/value collection of ddatabase access parameters
+    TestDbAccessParser(const std::string& param_name)
+        : DbAccessParser(param_name)
+    {}
+
+    /// @brief Destructor
+    virtual ~TestDbAccessParser()
+    {}
+
+    /// @brief Get database access parameters
+    ///
+    /// Used in testing to check that the configuration information has been
+    /// parsed corrected.
+    ///
+    /// @return Map of keyword/value pairs representing database access
+    ///         information.
+    const StringPairMap& getDbAccessParameters() const {
+        return (DbAccessParser::getDbAccessParameters());
+    }
+
+    /// @brief Construct database access string
+    ///
+    /// Constructs the database access string from the stored parameters.
+    ///
+    /// @return Database access string
+    std::string getDbAccessString() const {
+        return (DbAccessParser::getDbAccessString());
+    }
+};
+
 // Check that the parser works with a simple configuration.
 TEST_F(DbAccessParserTest, validTypeMemfile) {
     const char* config[] = {"type", "memfile",
@@ -156,7 +207,7 @@ TEST_F(DbAccessParserTest, validTypeMemfile) {
     ConstElementPtr json_elements = Element::fromJSON(json_config);
     EXPECT_TRUE(json_elements);
 
-    DbAccessParser parser("lease-database");
+    TestDbAccessParser parser("lease-database");
     EXPECT_NO_THROW(parser.build(json_elements));
     checkAccessString("Valid memfile", parser.getDbAccessParameters(), config);
 }
@@ -172,7 +223,7 @@ TEST_F(DbAccessParserTest, emptyKeyword) {
     ConstElementPtr json_elements = Element::fromJSON(json_config);
     EXPECT_TRUE(json_elements);
 
-    DbAccessParser parser("lease-database");
+    TestDbAccessParser parser("lease-database");
     EXPECT_NO_THROW(parser.build(json_elements));
     checkAccessString("Valid memfile", parser.getDbAccessParameters(), config);
 }
@@ -190,7 +241,7 @@ TEST_F(DbAccessParserTest, validTypeMysql) {
     ConstElementPtr json_elements = Element::fromJSON(json_config);
     EXPECT_TRUE(json_elements);
 
-    DbAccessParser parser("lease-database");
+    TestDbAccessParser parser("lease-database");
     EXPECT_NO_THROW(parser.build(json_elements));
     checkAccessString("Valid mysql", parser.getDbAccessParameters(), config);
 }
@@ -207,46 +258,19 @@ TEST_F(DbAccessParserTest, missingTypeKeyword) {
     ConstElementPtr json_elements = Element::fromJSON(json_config);
     EXPECT_TRUE(json_elements);
 
-    DbAccessParser parser("lease-database");
+    TestDbAccessParser parser("lease-database");
     EXPECT_THROW(parser.build(json_elements), TypeKeywordMissing);
 }
 
-// If the value of the "type" keyword is unknown, a BadValue exception should
-// be thrown.
-TEST_F(DbAccessParserTest, badTypeKeyword) {
-    const char* config[] = {"type",     "invalid",
-                            "host",     "erewhon",
-                            "user",     "kea",
-                            "password", "keapassword",
-                            "name",     "keatest",
-                            NULL};
-
-    string json_config = toJson(config);
-    ConstElementPtr json_elements = Element::fromJSON(json_config);
-    EXPECT_TRUE(json_elements);
-
-    DbAccessParser parser("lease-database");
-    EXPECT_THROW(parser.build(json_elements), BadValue);
-}
-
 // Check that the factory function works.
 TEST_F(DbAccessParserTest, factory) {
-    const char* config[] = {"type", "memfile",
-                            NULL};
-
-    string json_config = toJson(config);
-    ConstElementPtr json_elements = Element::fromJSON(json_config);
-    EXPECT_TRUE(json_elements);
 
     // Check that the parser is built through the factory.
     boost::scoped_ptr<DhcpConfigParser> parser(
         DbAccessParser::factory("lease-database"));
-    EXPECT_NO_THROW(parser->build(json_elements));
-
-    // Access the "raw" parser.
+    EXPECT_TRUE(parser);
     DbAccessParser* dbap = dynamic_cast<DbAccessParser*>(parser.get());
     EXPECT_NE(static_cast<DbAccessParser*>(NULL), dbap);
-    checkAccessString("Valid mysql", dbap->getDbAccessParameters(), config);
 }
 
 // Check reconfiguration.  Checks that incremental changes applied to the
@@ -293,7 +317,7 @@ TEST_F(DbAccessParserTest, incrementalChanges) {
                              "name",     "keatest",
                              NULL};
 
-    DbAccessParser parser("lease-database");
+    TestDbAccessParser parser("lease-database");
 
     // First configuration string should cause a representation of that string
     // to be held.
@@ -346,4 +370,61 @@ TEST_F(DbAccessParserTest, incrementalChanges) {
                       config4);
 }
 
+// Check that the database access string is constructed correctly.
+TEST_F(DbAccessParserTest, getDbAccessString) {
+    const char* config[] = {"type",     "mysql",
+                            "host",     "" ,
+                            "name",     "keatest",
+                            NULL};
+
+    string json_config = toJson(config);
+    ConstElementPtr json_elements = Element::fromJSON(json_config);
+    EXPECT_TRUE(json_elements);
+
+    TestDbAccessParser parser("lease-database");
+    EXPECT_NO_THROW(parser.build(json_elements));
+
+    // Get the database access string
+    std::string dbaccess = parser.getDbAccessString();
+
+    // String should be either "type=mysql name=keatest" or
+    // "name=keatest type=mysql". The "host" entry is null, so should not be
+    // output.
+    EXPECT_TRUE((dbaccess == "type=mysql name=keatest") ||
+                (dbaccess == "name=keatest type=mysql"));
+}
+
+// Check that the "commit" function actually opens the database.  We will
+// only do this for the "memfile" database, as that does not assume that the
+// test has been built with MySQL support.
+TEST_F(DbAccessParserTest, commit) {
+
+    // Verify that no lease database is open
+    EXPECT_THROW({
+            LeaseMgr& manager = LeaseMgrFactory::instance();
+            manager.getType();  // Never executed but satisfies compiler
+            }, isc::dhcp::NoLeaseManager);
+
+    // Set up the parser to open the memfile database.
+    const char* config[] = {"type", "memfile",
+                            NULL};
+    string json_config = toJson(config);
+    ConstElementPtr json_elements = Element::fromJSON(json_config);
+    EXPECT_TRUE(json_elements);
+
+    TestDbAccessParser parser("lease-database");
+    EXPECT_NO_THROW(parser.build(json_elements));
+
+    // Ensure that the access string is as expected.
+    EXPECT_EQ(std::string("type=memfile"), parser.getDbAccessString());
+
+    // Committal of the parser changes should open the database.
+    EXPECT_NO_THROW(parser.commit());
+
+    // Verify by checking the type of database open.
+    std::string dbtype;
+    EXPECT_NO_THROW(dbtype = LeaseMgrFactory::instance().getType());
+    EXPECT_EQ(std::string("memfile"), dbtype);
+}
+
 };  // Anonymous namespace