[3436] Addressed review comments

Surrounded position info in error messages with parens.
Replaced use of vector with map for tracking position values.
General cleanup.

src/bin/d2/d2_cfg_mgr.cc

@@ -218,13 +218,13 @@ D2CfgMgr::buildParams(isc::data::ConstElementPtr params_config) {
         ip_address = asiolink::IOAddress(ip_address_str);
     } catch (const std::exception& ex) {
         isc_throw(D2CfgError, "IP address invalid : \""
-                  << ip_address_str << "\" : "
-                  << strings->getPosition("ip_address"));
+                  << ip_address_str << "\" ("
+                  << strings->getPosition("ip_address") << ")");
     }
 
     if ((ip_address.toText() == "0.0.0.0") || (ip_address.toText() == "::")) {
-        isc_throw(D2CfgError, "IP address cannot be \"" << ip_address << "\" : "
-                   << strings->getPosition("ip_address"));
+        isc_throw(D2CfgError, "IP address cannot be \"" << ip_address << "\" ("
+                   << strings->getPosition("ip_address") << ")");
     }
 
     // Fetch and validate port.
@@ -232,8 +232,8 @@ D2CfgMgr::buildParams(isc::data::ConstElementPtr params_config) {
     uint32_t port = ints->getOptionalParam("port", D2Params::DFT_PORT);
 
     if (port == 0) {
-        isc_throw(D2CfgError, "port cannot be 0 : "
-                  << ints->getPosition("port"));
+        isc_throw(D2CfgError, "port cannot be 0 ("
+                  << ints->getPosition("port") << ")");
     }
 
     // Fetch and validate dns_server_timeout.
@@ -242,8 +242,8 @@ D2CfgMgr::buildParams(isc::data::ConstElementPtr params_config) {
                                  D2Params::DFT_DNS_SERVER_TIMEOUT);
 
     if (dns_server_timeout < 1) {
-        isc_throw(D2CfgError, "DNS server timeout must be larger than 0 : "
-                  << ints->getPosition("dns_server_timeout"));
+        isc_throw(D2CfgError, "DNS server timeout must be larger than 0 ("
+                  << ints->getPosition("dns_server_timeout") << ")");
     }
 
     // Fetch and validate ncr_protocol.
@@ -256,15 +256,15 @@ D2CfgMgr::buildParams(isc::data::ConstElementPtr params_config) {
                                                             DFT_NCR_PROTOCOL));
     } catch (const std::exception& ex) {
         isc_throw(D2CfgError, "ncr_protocol : "
-                  << ex.what() << " : "
-                  << strings->getPosition("ncr_protocol"));
+                  << ex.what() << " ("
+                  << strings->getPosition("ncr_protocol") << ")");
     }
 
     if (ncr_protocol != dhcp_ddns::NCR_UDP) {
         isc_throw(D2CfgError, "ncr_protocol : "
                   << dhcp_ddns::ncrProtocolToString(ncr_protocol)
-                  << " is not yet supported : "
-                  << strings->getPosition("ncr_protocol"));
+                  << " is not yet supported ("
+                  << strings->getPosition("ncr_protocol") << ")");
     }
 
     // Fetch and validate ncr_format.
@@ -277,15 +277,15 @@ D2CfgMgr::buildParams(isc::data::ConstElementPtr params_config) {
                                                         DFT_NCR_FORMAT));
     } catch (const std::exception& ex) {
         isc_throw(D2CfgError, "ncr_format : "
-                  << ex.what() << " : "
-                  << strings->getPosition("ncr_format"));
+                  << ex.what() << " ("
+                  << strings->getPosition("ncr_format") << ")");
     }
 
     if (ncr_format != dhcp_ddns::FMT_JSON) {
         isc_throw(D2CfgError, "NCR Format:"
                   << dhcp_ddns::ncrFormatToString(ncr_format)
-                  << " is not yet supported : "
-                  << strings->getPosition("ncr_format"));
+                  << " is not yet supported ("
+                  << strings->getPosition("ncr_format") << ")");
     }
 
     // Attempt to create the new client config. This ought to fly as

src/bin/d2/d2_config.cc

@@ -358,22 +358,22 @@ TSIGKeyInfoParser::build(isc::data::ConstElementPtr key_config) {
     std::string name;
     std::string algorithm;
     std::string secret;
-    std::vector<isc::data::Element::Position> pos;
+    std::map<std::string, isc::data::Element::Position> pos;
 
     // Fetch the key's parsed scalar values from parser's local storage.
     // All are required, if any are missing we'll throw.
     try {
-        pos.push_back(local_scalars_.getParam("name", name));
-        pos.push_back(local_scalars_.getParam("algorithm", algorithm));
-        pos.push_back(local_scalars_.getParam("secret", secret));
+        pos["name"] = local_scalars_.getParam("name", name);
+        pos["algorithm"] = local_scalars_.getParam("algorithm", algorithm);
+        pos["secret"] = local_scalars_.getParam("secret", secret);
     } catch (const std::exception& ex) {
         isc_throw(D2CfgError, "TSIG Key incomplete : " << ex.what()
-                  << " : " << key_config->getPosition());
+                  << " (" << key_config->getPosition() << ")");
     }
 
     // Name cannot be blank.
     if (name.empty()) {
-        isc_throw(D2CfgError, "TSIG key must specify name : " << pos[0]);
+        isc_throw(D2CfgError, "TSIG key must specify name (" << pos["name"] << ")");
     }
 
     // Currently, the premise is that key storage is always empty prior to
@@ -381,23 +381,23 @@ TSIGKeyInfoParser::build(isc::data::ConstElementPtr key_config) {
     // are not allowed and should be flagged as a configuration error.
     if (keys_->find(name) != keys_->end()) {
         isc_throw(D2CfgError, "Duplicate TSIG key name specified : " << name
-                              << " : " << pos[0]);
+                              << " (" << pos["name"] << ")");
     }
 
     // Algorithm must be valid.
     try {
         TSIGKeyInfo::stringToAlgorithmName(algorithm);
     } catch (const std::exception& ex) {
-        isc_throw(D2CfgError, "TSIG key : " << ex.what() << " : " << pos[1]);
+        isc_throw(D2CfgError, "TSIG key : " << ex.what() << " (" << pos["algorithm"] << ")");
     }
 
     // Secret cannot be blank.
-    // Cryptolink lib doesn't offer anyway to validate these. As long as it
+    // Cryptolink lib doesn't offer any way to validate these. As long as it
     // isn't blank we'll accept it.  If the content is bad, the call to in
     // TSIGKeyInfo::remakeKey() made in the TSIGKeyInfo ctor will throw.
     // We'll deal with that below.
     if (secret.empty()) {
-        isc_throw(D2CfgError, "TSIG key must specify secret : " << pos[2]);
+        isc_throw(D2CfgError, "TSIG key must specify secret (" << pos["secret"] << ")");
     }
 
     // Everything should be valid, so create the key instance.
@@ -407,7 +407,7 @@ TSIGKeyInfoParser::build(isc::data::ConstElementPtr key_config) {
     try {
         key_info.reset(new TSIGKeyInfo(name, algorithm, secret));
     } catch (const std::exception& ex) {
-        isc_throw(D2CfgError, ex.what() << " : " << key_config->getPosition());
+        isc_throw(D2CfgError, ex.what() << " (" << key_config->getPosition() << ")");
 
     }
 
@@ -429,7 +429,7 @@ TSIGKeyInfoParser::createConfigParser(const std::string& config_id,
     } else {
         isc_throw(NotImplemented,
                   "parser error: TSIGKeyInfo parameter not supported: "
-                  << config_id << " : " << pos);
+                  << config_id << " (" << pos << ")");
     }
 
     // Return the new parser instance.
@@ -522,28 +522,28 @@ DnsServerInfoParser::build(isc::data::ConstElementPtr server_config) {
     std::string hostname;
     std::string ip_address;
     uint32_t port = DnsServerInfo::STANDARD_DNS_PORT;
-    std::vector<isc::data::Element::Position> pos;
+    std::map<std::string, isc::data::Element::Position> pos;
 
     // Fetch the server configuration's parsed scalar values from parser's
     // local storage.  They're all optional, so no try-catch here.
-    pos.push_back(local_scalars_.getParam("hostname", hostname,
-                                          DCfgContextBase::OPTIONAL));
-    pos.push_back(local_scalars_.getParam("ip_address", ip_address,
-                                          DCfgContextBase::OPTIONAL));
-    pos.push_back(local_scalars_.getParam("port", port,
-                                           DCfgContextBase::OPTIONAL));
+    pos["hostname"] = local_scalars_.getParam("hostname", hostname,
+                                              DCfgContextBase::OPTIONAL);
+    pos["ip_address"] = local_scalars_.getParam("ip_address", ip_address,
+                                                DCfgContextBase::OPTIONAL);
+    pos["port"] =  local_scalars_.getParam("port", port,
+                                           DCfgContextBase::OPTIONAL);
 
     // The configuration must specify one or the other.
     if (hostname.empty() == ip_address.empty()) {
         isc_throw(D2CfgError, "Dns Server must specify one or the other"
                   " of hostname or IP address"
-                  << " : " << server_config->getPosition());
+                  << " (" << server_config->getPosition() << ")");
     }
 
     // Port cannot be zero.
     if (port == 0) {
         isc_throw(D2CfgError, "Dns Server : port cannot be 0"
-                  << " : " << pos[2]);
+                  << " (" << pos["port"] << ")");
     }
 
     DnsServerInfoPtr serverInfo;
@@ -563,7 +563,7 @@ DnsServerInfoParser::build(isc::data::ConstElementPtr server_config) {
         /// processing.
         /// Until then we'll throw unsupported.
         isc_throw(D2CfgError, "Dns Server : hostname is not yet supported"
-                  << " : " << pos[0]);
+                  << " (" << pos["hostname"] << ")");
     } else {
         try {
             // Create an IOAddress from the IP address string given and then
@@ -572,7 +572,7 @@ DnsServerInfoParser::build(isc::data::ConstElementPtr server_config) {
             serverInfo.reset(new DnsServerInfo(hostname, io_addr, port));
         } catch (const isc::asiolink::IOError& ex) {
             isc_throw(D2CfgError, "Dns Server : invalid IP address : "
-                      << ip_address << " : " << pos[1]);
+                      << ip_address << " (" << pos["ip_address"] << ")");
         }
     }
 
@@ -597,7 +597,7 @@ DnsServerInfoParser::createConfigParser(const std::string& config_id,
     } else {
         isc_throw(NotImplemented,
                   "parser error: DnsServerInfo parameter not supported: "
-                  << config_id << " : " << pos);
+                  << config_id << " (" << pos << ")");
     }
 
     // Return the new parser instance.
@@ -644,7 +644,7 @@ build(isc::data::ConstElementPtr server_list){
     // Domains must have at least one server.
     if (parsers_.size() == 0) {
         isc_throw (D2CfgError, "Server List must contain at least one server"
-                   << " : " << server_list->getPosition());
+                   << " (" << server_list->getPosition() << ")");
     }
 }
 
@@ -693,23 +693,24 @@ DdnsDomainParser::build(isc::data::ConstElementPtr domain_config) {
     // Now construct the domain.
     std::string name;
     std::string key_name;
-    std::vector<isc::data::Element::Position> pos;
+    std::map<std::string, isc::data::Element::Position> pos;
+
 
     // Fetch the parsed scalar values from parser's local storage.
     // Any required that are missing will throw.
     try {
-        pos.push_back(local_scalars_.getParam("name", name));
-        pos.push_back(local_scalars_.getParam("key_name", key_name,
-                                              DCfgContextBase::OPTIONAL));
+        pos["name"] = local_scalars_.getParam("name", name);
+        pos["key_name"] = local_scalars_.getParam("key_name", key_name,
+                                                  DCfgContextBase::OPTIONAL);
     } catch (const std::exception& ex) {
         isc_throw(D2CfgError, "DdnsDomain incomplete : " << ex.what()
-                  << " : " << domain_config->getPosition());
+                  << " (" << domain_config->getPosition() << ")");
     }
 
     // Blank domain names are not allowed.
     if (name.empty()) {
-        isc_throw(D2CfgError, "DndsDomain : name cannot be blank : "
-                   << pos[0]);
+        isc_throw(D2CfgError, "DndsDomain : name cannot be blank ("
+                   << pos["name"] << ")");
     }
 
     // Currently, the premise is that domain storage is always empty
@@ -718,7 +719,7 @@ DdnsDomainParser::build(isc::data::ConstElementPtr domain_config) {
     // error.
     if (domains_->find(name) != domains_->end()) {
         isc_throw(D2CfgError, "Duplicate domain specified:" << name
-                  << " : " << pos[0]);
+                  << " (" << pos["name"] << ")");
     }
 
     // Key name is optional. If it is not blank, then find the key in the
@@ -735,7 +736,7 @@ DdnsDomainParser::build(isc::data::ConstElementPtr domain_config) {
         if (!tsig_key_info) {
             isc_throw(D2CfgError, "DdnsDomain : " << name
                       << " specifies an undefined key: " << key_name
-                      << " : " << pos[1]);
+                      << " (" << pos["key_name"] << ")");
         }
     }
 
@@ -764,7 +765,7 @@ DdnsDomainParser::createConfigParser(const std::string& config_id,
     } else {
        isc_throw(NotImplemented,
                 "parser error: DdnsDomain parameter not supported: "
-                << config_id << " : " << pos);
+                << config_id << " (" << pos << ")");
     }
 
     // Return the new domain parser instance.
@@ -865,7 +866,7 @@ DdnsDomainListMgrParser::createConfigParser(const std::string& config_id,
     } else {
        isc_throw(NotImplemented, "parser error: "
                  "DdnsDomainListMgr parameter not supported: " << config_id
-                 << " : " << pos);
+                 << " (" << pos << ")");
     }
 
     // Return the new domain parser instance.

src/bin/d2/d2_messages.mes

@@ -32,7 +32,7 @@ new configuration. It is output during server startup, and when an updated
 configuration is committed by the administrator.  Additional information
 may be provided.
 
-% DCTL_CONFIG_FILE_LOAD_FAIL %1 : %2
+% DCTL_CONFIG_FILE_LOAD_FAIL %1 reason: %2
 This fatal error message indicates that the application attempted to load its
 initial configuration from file and has failed. The service will exit.
 
@@ -72,7 +72,7 @@ application and will exit.
 A warning message is issued when an attempt is made to shut down the
 application when it is not running.
 
-% DCTL_PARSER_FAIL %1
+% DCTL_PARSER_FAIL : %1
 On receipt of a new configuration, the server failed to create a parser to
 decode the contents of the named configuration element, or the creation
 succeeded but the parsing actions and committal of changes failed.

src/bin/d2/d_cfg_mgr.cc

@@ -208,8 +208,8 @@ DCfgMgrBase::parseConfig(isc::data::ConstElementPtr config_set) {
                 else {
                     isc_throw(DCfgMgrBaseError,
                                "Element required by parsing order is missing: "
-                               << element_id << " : "
-                               << config_set->getPosition());
+                               << element_id << " ("
+                               << config_set->getPosition() << ")");
                 }
             }
 
@@ -221,13 +221,13 @@ DCfgMgrBase::parseConfig(isc::data::ConstElementPtr config_set) {
             // and parsing them either first or last but which would be correct?
             // Better to hold the engineer accountable.  So, if there are any
             // left in the objects_map then they were not in the parse order.
-            if (objects_map.size() > 0) {
+            if (!objects_map.empty()) {
                 std::ostringstream stream;
                 bool add_comma = false;
                 ConfigPair config_pair;
                 BOOST_FOREACH(config_pair, objects_map) {
                     stream << ( add_comma ? ", " : "") << config_pair.first
-                           << " at: " << config_pair.second->getPosition();
+                           << " (" << config_pair.second->getPosition() << ")";
                     add_comma = true;
                 }
 
@@ -251,7 +251,7 @@ DCfgMgrBase::parseConfig(isc::data::ConstElementPtr config_set) {
 
     } catch (const std::exception& ex) {
         LOG_ERROR(dctl_logger, DCTL_PARSER_FAIL).arg(ex.what());
-        answer = isc::config::createAnswer(1, + ex.what());
+        answer = isc::config::createAnswer(1, ex.what());
 
         // An error occurred, so make sure that we restore original context.
         context_ = original_context;

src/bin/d2/tests/d2_cfg_mgr_unittests.cc

@@ -28,10 +28,22 @@ using namespace isc::d2;
 
 namespace {
 
+/// @brief Function to create full path to the spec file
+///
+/// The full path is dependent upon the value of D2_SRC_DIR which
+/// whose value is generated from test_data_files_config.h.in
+///
+/// @param name file name to which the path should be prepended
 std::string specfile(const std::string& name) {
     return (std::string(D2_SRC_DIR) + "/" + name);
 }
 
+/// @brief Function to create full path to test data file
+///
+/// The full path is dependent upon the value of D2_TEST_DATA_DIR which
+/// whose value is generated from test_data_files_config.h.in
+///
+/// @param name file name to which the path should be prepended
 std::string testDataFile(const std::string& name) {
     return (std::string(D2_TEST_DATA_DIR) + "/" + name);
 }

src/bin/d2/tests/d_cfg_mgr_unittests.cc

@@ -126,23 +126,18 @@ TEST_F(DStubCfgMgrTest, basicParseTest) {
     answer_ = cfg_mgr_->parseConfig(config_set_);
     EXPECT_TRUE(checkAnswer(0));
 
-
-std::cout << __FILE__ << ":" <<  __LINE__ << std::endl;
     // Verify that an error building the element is caught and returns a
     // failed parse result.
     SimFailure::set(SimFailure::ftElementBuild);
     answer_ = cfg_mgr_->parseConfig(config_set_);
     EXPECT_TRUE(checkAnswer(1));
 
-std::cout << __FILE__ << ":" <<  __LINE__ << std::endl;
     // Verify that an error committing the element is caught and returns a
     // failed parse result.
     SimFailure::set(SimFailure::ftElementCommit);
     answer_ = cfg_mgr_->parseConfig(config_set_);
     EXPECT_TRUE(checkAnswer(1));
 
-std::cout << __FILE__ << ":" <<  __LINE__ << std::endl;
-
     // Verify that an unknown element error is caught and returns a failed
     // parse result.
     SimFailure::set(SimFailure::ftElementUnknown);