From: Thomas Markwalder Date: Tue, 1 Jul 2014 18:32:07 +0000 (-0400) Subject: [3436] Addressed review comments X-Git-Tag: trac3473_base~5^2~1 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=05fd498fd7ecf28a64017adc8f23b2bf86d9043b;p=thirdparty%2Fkea.git [3436] Addressed review comments Surrounded position info in error messages with parens. Replaced use of vector with map for tracking position values. General cleanup. --- diff --git a/src/bin/d2/d2_cfg_mgr.cc b/src/bin/d2/d2_cfg_mgr.cc index 0739a0f4e4..ad21aadac6 100644 --- a/src/bin/d2/d2_cfg_mgr.cc +++ b/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 diff --git a/src/bin/d2/d2_config.cc b/src/bin/d2/d2_config.cc index aeb9d69bf8..4ec3dbddee 100644 --- a/src/bin/d2/d2_config.cc +++ b/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 pos; + std::map 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 pos; + std::map 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 pos; + std::map 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. diff --git a/src/bin/d2/d2_messages.mes b/src/bin/d2/d2_messages.mes index d66373e74d..8bca4ea94d 100644 --- a/src/bin/d2/d2_messages.mes +++ b/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. diff --git a/src/bin/d2/d_cfg_mgr.cc b/src/bin/d2/d_cfg_mgr.cc index 7abc0e44c1..f014b355dd 100644 --- a/src/bin/d2/d_cfg_mgr.cc +++ b/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; diff --git a/src/bin/d2/tests/d2_cfg_mgr_unittests.cc b/src/bin/d2/tests/d2_cfg_mgr_unittests.cc index c2dc27912b..0a54dff2a1 100644 --- a/src/bin/d2/tests/d2_cfg_mgr_unittests.cc +++ b/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); } diff --git a/src/bin/d2/tests/d_cfg_mgr_unittests.cc b/src/bin/d2/tests/d_cfg_mgr_unittests.cc index f57f2f7640..d9aa843541 100644 --- a/src/bin/d2/tests/d_cfg_mgr_unittests.cc +++ b/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);