Commit 41d5b2ea authored by Thomas Markwalder's avatar Thomas Markwalder
Browse files

[2957] Additional review-related changes. Replaced TODO with todo

and corrected doxygen errors.
parent 971e7700
......@@ -106,8 +106,6 @@ typedef boost::shared_ptr<DdnsDomainListMgr> DdnsDomainListMgrPtr;
class D2CfgMgr : public DCfgMgrBase {
public:
/// @brief Constructor
///
/// @param context is a pointer to the configuration context the manager
D2CfgMgr();
/// @brief Destructor
......@@ -131,7 +129,7 @@ public:
///
/// @return returns true if a match is found, false otherwise.
/// @throw throws D2CfgError if given an invalid fqdn.
bool matchForward(const std::string& fdqn, DdnsDomainPtr &domain);
bool matchForward(const std::string& fqdn, DdnsDomainPtr &domain);
/// @brief Matches a given FQDN to a reverse domain.
///
......@@ -144,7 +142,7 @@ public:
///
/// @return returns true if a match is found, false otherwise.
/// @throw throws D2CfgError if given an invalid fqdn.
bool matchReverse(const std::string& fdqn, DdnsDomainPtr &domain);
bool matchReverse(const std::string& fqdn, DdnsDomainPtr &domain);
protected:
/// @brief Given an element_id returns an instance of the appropriate
......
......@@ -103,7 +103,7 @@ DdnsDomainListMgr::matchDomain(const std::string& fqdn, DdnsDomainPtr& domain) {
// Start with the longest version of the fqdn and search the list.
// Continue looking for shorter versions of fqdn so long as no match is
// found.
// @TODO This can surely be optimized, time permitting.
// @todo This can surely be optimized, time permitting.
std::string match_name = fqdn;
std::size_t start_pos = 0;
while (start_pos != std::string::npos) {
......@@ -195,7 +195,7 @@ TSIGKeyInfoParser::commit() {
local_scalars_.getParam("algorithm", algorithm);
local_scalars_.getParam("secret", secret);
// @TODO Validation here is very superficial. This will expand as TSIG
// @todo Validation here is very superficial. This will expand as TSIG
// Key use is more fully implemented.
// Name cannot be blank.
......
......@@ -42,7 +42,7 @@ namespace d2 {
/// The key list consists of one or more TSIG keys, each entry described by
/// a name, the algorithm method name, and its secret key component.
///
/// @TODO NOTE that TSIG configuration parsing is functional, the use of
/// @todo NOTE that TSIG configuration parsing is functional, the use of
/// TSIG Keys during the actual DNS update transactions is not. This will be
/// implemented in a future release.
///
......@@ -149,7 +149,7 @@ public:
/// a TSIG Key. It is intended primarily as a reference for working with
/// actual keys and may eventually be replaced by isc::dns::TSIGKey. TSIG Key
/// functionality at this stage is strictly limited to configuration parsing.
/// @TODO full functionality for using TSIG during DNS updates will be added
/// @todo full functionality for using TSIG during DNS updates will be added
/// in a future release.
class TSIGKeyInfo {
public:
......@@ -158,7 +158,7 @@ public:
///
/// @param name the unique label used to identify this key
/// @param algorithm the name of the encryption alogirthm this key uses.
/// (@TODO This will be a fixed list of choices)
/// (@todo This will be a fixed list of choices)
/// @param secret the secret component of this key
TSIGKeyInfo(const std::string& name, const std::string& algorithm,
const std::string& secret);
......@@ -243,7 +243,7 @@ public:
/// enabled for use. It defaults to true.
DnsServerInfo(const std::string& hostname,
isc::asiolink::IOAddress ip_address,
uint32_t port=STANDARD_DNS_PORT,
uint32_t port = STANDARD_DNS_PORT,
bool enabled=true);
/// @brief Destructor
......@@ -320,6 +320,9 @@ typedef boost::shared_ptr<DnsServerInfoStorage> DnsServerInfoStoragePtr;
/// This class specifies a DNS domain and the list of DNS servers that support
/// it. It's primary use is to map a domain to the DNS server(s) responsible
/// for it.
/// @todo Currently the name entry for a domain is just an std::string. It
/// may be worthwhile to change this to a dns::Name for purposes of better
/// validation and matching capabilities.
class DdnsDomain {
public:
/// @brief Constructor
......@@ -411,6 +414,10 @@ public:
/// upon entry and only set if a match is subsequently found.
///
/// @return returns true if a match is found, false otherwise.
/// @todo This is a very basic match method, which expects valid FQDNs
/// both as input and for the DdnsDomain::getName(). Currently both are
/// simple strings and there is no normalization (i.e. added trailing dots
/// if missing).
virtual bool matchDomain(const std::string& fqdn, DdnsDomainPtr& domain);
/// @brief Fetches the manager's name.
......@@ -513,7 +520,7 @@ public:
/// @param entry_name is an arbitrary label assigned to this configuration
/// definition. Since servers are specified in a list this value is likely
/// be something akin to "key:0", set during parsing.
/// @param servers is a pointer to the storage area to which the parser
/// @param keys is a pointer to the storage area to which the parser
/// should commit the newly created TSIGKeyInfo instance.
TSIGKeyInfoParser(const std::string& entry_name, TSIGKeyInfoMapPtr keys);
......@@ -686,7 +693,7 @@ public:
/// @param servers is a pointer to the storage area to which the parser
/// should commit the newly created DnsServerInfo instance.
DnsServerInfoListParser(const std::string& list_name,
DnsServerInfoStoragePtr servers_);
DnsServerInfoStoragePtr servers);
/// @brief Destructor
virtual ~DnsServerInfoListParser();
......@@ -747,7 +754,7 @@ public:
/// The results of the parsing are retained internally for use during
/// commit.
///
/// @param server_config is the "ddns_domain" configuration to parse
/// @param domain_config is the "ddns_domain" configuration to parse
virtual void build(isc::data::ConstElementPtr domain_config);
/// @brief Creates a parser for the given "ddns_domain" member element id.
......@@ -810,7 +817,7 @@ public:
/// @param keys is a pointer to a map of the defined TSIG keys.
/// should commit the newly created DdnsDomain instance.
DdnsDomainListParser(const std::string& list_name,
DdnsDomainMapPtr domains_, TSIGKeyInfoMapPtr keys);
DdnsDomainMapPtr domains, TSIGKeyInfoMapPtr keys);
/// @brief Destructor
virtual ~DdnsDomainListParser();
......@@ -922,7 +929,7 @@ private:
/// @brief Local storage area for scalar parameter values. Use to hold
/// data until time to commit.
/// @TODO Currently, the manager has no scalars but this is likely to
/// @todo Currently, the manager has no scalars but this is likely to
/// change as matching capabilities expand.
DScalarContext local_scalars_;
};
......
......@@ -24,7 +24,7 @@ namespace d2 {
/// This class is the DHCP-DDNS specific derivation of DControllerBase. It
/// creates and manages an instance of the DHCP-DDNS application process,
/// D2Process.
/// @TODO Currently, this class provides only the minimum required specialized
/// @todo Currently, this class provides only the minimum required specialized
/// behavior to run the DHCP-DDNS service. It may very well expand as the
/// service implementation evolves. Some thought was given to making
/// DControllerBase a templated class but the labor savings versus the
......
......@@ -74,8 +74,7 @@ element ids not specified the configuration manager's parse order list. This
is a programmatic error.
% DCTL_ORDER_NO_ELEMENT element: %1 is in the parsing order but is missing from the configuration
An error message indicating that the listed element is in specified in the
parsing order but is not in the configuration. The configuration is incorrect.
An error message output during a configuration update. The program is expecting an item but has not found it in the new configuration. This may mean that the BIND 10 configuration database is corrupt.
% DCTL_PARSER_FAIL configuration parsing failed for configuration element: %1
On receipt of message containing details to a change of its configuration,
......
......@@ -61,7 +61,7 @@ D2Process::shutdown() {
isc::data::ConstElementPtr
D2Process::configure(isc::data::ConstElementPtr config_set) {
// @TODO This is the initial implementation passes the configuration onto
// @todo This is the initial implementation passes the configuration onto
// the D2CfgMgr. There may be additional steps taken added to handle
// configuration changes but for now, assume that D2CfgMgr is handling it
// all.
......@@ -73,7 +73,7 @@ D2Process::configure(isc::data::ConstElementPtr config_set) {
isc::data::ConstElementPtr
D2Process::command(const std::string& command, isc::data::ConstElementPtr args){
// @TODO This is the initial implementation. If and when D2 is extended
// @todo This is the initial implementation. If and when D2 is extended
// to support its own commands, this implementation must change. Otherwise
// it should reject all commands as it does now.
LOG_DEBUG(dctl_logger, DBGLVL_TRACE_BASIC,
......
......@@ -41,7 +41,7 @@ public:
D2Process(const char* name, IOServicePtr io_service);
/// @brief Will be used after instantiation to perform initialization
/// unique to D2. @TODO This will likely include interactions with
/// unique to D2. @todo This will likely include interactions with
/// QueueMgr and UpdateMgr, to prepare for request receipt and processing.
/// Current implementation successfully does nothing.
/// @throw throws a DProcessBaseError if the initialization fails.
......@@ -58,7 +58,7 @@ public:
/// @brief Implements the process's shutdown processing. When invoked, it
/// should ensure that the process gracefully exits the run method.
/// Current implementation simply sets the shutdown flag monitored by the
/// run method. @TODO this may need to expand as the implementation evolves.
/// run method. @todo this may need to expand as the implementation evolves.
/// @throw throws a DProcessBaseError if an error is encountered.
virtual void shutdown();
......
......@@ -305,7 +305,7 @@ isc::data::ConstElementPtr
DControllerBase::updateConfig(isc::data::ConstElementPtr new_config) {
isc::data::ConstElementPtr full_config;
if (stand_alone_) {
// @TODO Until there is a configuration manager to provide retrieval
// @todo Until there is a configuration manager to provide retrieval
// we'll just assume the incoming config is the full configuration set.
// It may also make more sense to isolate the controller from the
// configuration manager entirely. We could do something like
......
......@@ -171,7 +171,7 @@ public:
/// various configuration values. Installing the dummy handler
/// that guarantees to return success causes initial configuration
/// to be stored for the session being created and that it can
/// be later accessed with \ref isc::ConfigData::getFullConfig.
/// be later accessed with \ref isc::config::ConfigData::getFullConfig.
///
/// @param new_config new configuration.
///
......@@ -210,7 +210,7 @@ public:
/// implementation will merge the configuration update into the existing
/// configuration and then invoke the application process' configure method.
///
/// @TODO This implementation is will evolve as the D2 configuration
/// @todo This implementation is will evolve as the D2 configuration
/// management task is implemented (trac #2957).
///
/// @param new_config is the new configuration
......@@ -261,7 +261,7 @@ protected:
///
/// @param option is the option "character" from the command line, without
/// any prefixing hyphen(s)
/// @optarg optarg is the argument value (if one) associated with the option
/// @param optarg is the argument value (if one) associated with the option
///
/// @return must return true if the option was valid, false is it is
/// invalid. (Note the default implementation always returns false.)
......@@ -395,7 +395,7 @@ protected:
/// @brief Setter for setting the name of the controller's BIND10 spec file.
///
/// @param value is the file name string.
/// @param spec_file_name the file name string.
void setSpecFileName(const std::string& spec_file_name) {
spec_file_name_ = spec_file_name;
}
......
......@@ -60,6 +60,8 @@ public:
/// in log statements, but otherwise arbitrary.
/// @param io_service is the io_service used by the caller for
/// asynchronous event handling.
/// @param cfg_mgr the configuration manager instance that handles
/// configuration parsing.
///
/// @throw DProcessBaseError is io_service is NULL.
DProcessBase(const char* app_name, IOServicePtr io_service,
......
......@@ -85,19 +85,19 @@ bool checkServer(DnsServerInfoPtr server, const char* hostname,
// Check hostname.
if (server->getHostname() != hostname) {
EXPECT_EQ(server->getHostname(),hostname);
EXPECT_EQ(hostname, server->getHostname());
result = false;
}
// Check IP address.
if (server->getIpAddress().toText() != ip_address) {
EXPECT_EQ(server->getIpAddress().toText(), ip_address);
EXPECT_EQ(ip_address, server->getIpAddress().toText());
result = false;
}
// Check port.
if (server->getPort() != port) {
EXPECT_EQ (server->getPort(), port);
EXPECT_EQ (port, server->getPort());
result = false;
}
......@@ -122,27 +122,26 @@ bool checkKey(TSIGKeyInfoPtr key, const char* name,
{
// Return value, assume its a match.
bool result = true;
if (!key)
{
if (!key) {
EXPECT_TRUE(key);
return false;
}
// Check name.
if (key->getName() != name) {
EXPECT_EQ(key->getName(),name);
EXPECT_EQ(name, key->getName());
result = false;
}
// Check algorithm.
if (key->getAlgorithm() != algorithm) {
EXPECT_EQ(key->getAlgorithm(), algorithm);
EXPECT_EQ(algorithm, key->getAlgorithm());
result = false;
}
// Check secret.
if (key->getSecret() != secret) {
EXPECT_EQ (key->getSecret(), secret);
EXPECT_EQ (secret, key->getSecret());
result = false;
}
......@@ -260,7 +259,6 @@ TEST_F(TSIGKeyInfoTest, invalidEntryTests) {
" \"algorithm\": \"md5\" , "
" \"secret\": \"0123456789\" "
"}";
ASSERT_TRUE(fromJSON(config));
// Verify that build succeeds but commit fails on blank name.
......@@ -311,7 +309,7 @@ TEST_F(TSIGKeyInfoTest, validEntryTests) {
// Verify the correct number of keys are present
int count = keys_->size();
EXPECT_EQ(count, 1);
EXPECT_EQ(1, count);
// Find the key and retrieve it.
TSIGKeyInfoMap::iterator gotit = keys_->find("d2_key_one");
......@@ -493,7 +491,7 @@ TEST_F(DnsServerInfoTest, validEntryTests) {
// Verify the correct number of servers are present
int count = servers_->size();
EXPECT_EQ(count, 1);
EXPECT_EQ(1, count);
// Verify the server exists and has the correct values.
DnsServerInfoPtr server = (*servers_)[0];
......@@ -515,7 +513,7 @@ TEST_F(DnsServerInfoTest, validEntryTests) {
// Verify the correct number of servers are present
count = servers_->size();
EXPECT_EQ(count, 1);
EXPECT_EQ(1, count);
// Verify the server exists and has the correct values.
server = (*servers_)[0];
......@@ -534,7 +532,7 @@ TEST_F(DnsServerInfoTest, validEntryTests) {
// Verify the correct number of servers are present
count = servers_->size();
EXPECT_EQ(count, 1);
EXPECT_EQ(1, count);
// Verify the server exists and has the correct values.
server = (*servers_)[0];
......@@ -583,7 +581,7 @@ TEST_F(ConfigParseTest, validServerList) {
// Verify that the server storage contains the correct number of servers.
int count = servers->size();
EXPECT_EQ(count, 3);
EXPECT_EQ(3, count);
// Verify the first server exists and has the correct values.
DnsServerInfoPtr server = (*servers)[0];
......@@ -693,7 +691,7 @@ TEST_F(DdnsDomainTest, ddnsDomainParsing) {
// Verify that the domain storage contains the correct number of domains.
int count = domains_->size();
EXPECT_EQ(count, 1);
EXPECT_EQ(1, count);
// Verify that the expected domain exists and can be retrieved from
// the storage.
......@@ -702,15 +700,15 @@ TEST_F(DdnsDomainTest, ddnsDomainParsing) {
DdnsDomainPtr& domain = gotit->second;
// Verify the name and key_name values.
EXPECT_EQ(domain->getName(), "tmark.org");
EXPECT_EQ(domain->getKeyName(), "d2_key.tmark.org");
EXPECT_EQ("tmark.org", domain->getName());
EXPECT_EQ("d2_key.tmark.org", domain->getKeyName());
// Verify that the server list exists and contains the correct number of
// servers.
const DnsServerInfoStoragePtr& servers = domain->getServers();
EXPECT_TRUE(servers);
count = servers->size();
EXPECT_EQ(count, 3);
EXPECT_EQ(3, count);
// Fetch each server and verify its contents.
DnsServerInfoPtr server = (*servers)[0];
......@@ -775,7 +773,7 @@ TEST_F(DdnsDomainTest, DdnsDomainListParsing) {
// Verify that the domain storage contains the correct number of domains.
int count = domains_->size();
EXPECT_EQ(count, 2);
EXPECT_EQ(2, count);
// Verify that the first domain exists and can be retrieved.
DdnsDomainMap::iterator gotit = domains_->find("tmark.org");
......@@ -783,14 +781,14 @@ TEST_F(DdnsDomainTest, DdnsDomainListParsing) {
DdnsDomainPtr& domain = gotit->second;
// Verify the name and key_name values of the first domain.
EXPECT_EQ(domain->getName(), "tmark.org");
EXPECT_EQ(domain->getKeyName(), "d2_key.tmark.org");
EXPECT_EQ("tmark.org", domain->getName());
EXPECT_EQ("d2_key.tmark.org", domain->getKeyName());
// Verify the each of the first domain's servers
DnsServerInfoStoragePtr servers = domain->getServers();
EXPECT_TRUE(servers);
count = servers->size();
EXPECT_EQ(count, 3);
EXPECT_EQ(3, count);
DnsServerInfoPtr server = (*servers)[0];
EXPECT_TRUE(server);
......@@ -810,14 +808,14 @@ TEST_F(DdnsDomainTest, DdnsDomainListParsing) {
domain = gotit->second;
// Verify the name and key_name values of the second domain.
EXPECT_EQ(domain->getName(), "billcat.net");
EXPECT_EQ(domain->getKeyName(), "d2_key.billcat.net");
EXPECT_EQ("billcat.net", domain->getName());
EXPECT_EQ("d2_key.billcat.net", domain->getKeyName());
// Verify the each of second domain's servers
servers = domain->getServers();
EXPECT_TRUE(servers);
count = servers->size();
EXPECT_EQ(count, 3);
EXPECT_EQ(3, count);
server = (*servers)[0];
EXPECT_TRUE(server);
......@@ -956,15 +954,15 @@ TEST_F(D2CfgMgrTest, fullConfigTest) {
// Verify that the application level scalars have the proper values.
std::string interface;
EXPECT_NO_THROW (context->getParam("interface", interface));
EXPECT_EQ(interface, "eth1");
EXPECT_EQ("eth1", interface);
std::string ip_address;
EXPECT_NO_THROW (context->getParam("ip_address", ip_address));
EXPECT_EQ(ip_address, "192.168.1.33");
EXPECT_EQ("192.168.1.33", ip_address);
uint32_t port = 0;
EXPECT_NO_THROW (context->getParam("port", port));
EXPECT_EQ(port, 88);
EXPECT_EQ(88, port);
// Verify that the forward manager can be retrieved.
DdnsDomainListMgrPtr mgr = context->getForwardMgr();
......@@ -974,7 +972,7 @@ TEST_F(D2CfgMgrTest, fullConfigTest) {
DdnsDomainMapPtr domains = mgr->getDomains();
ASSERT_TRUE(domains);
int count = domains->size();
EXPECT_EQ(count, 2);
EXPECT_EQ(2, count);
// Verify that the server count in each of the forward manager domains.
// NOTE that since prior tests have validated server parsing, we are are
......@@ -986,7 +984,7 @@ TEST_F(D2CfgMgrTest, fullConfigTest) {
DnsServerInfoStoragePtr servers = domain->getServers();
count = servers->size();
EXPECT_TRUE(servers);
EXPECT_EQ(count, 3);
EXPECT_EQ(3, count);
}
// Verify that the reverse manager can be retrieved.
......@@ -996,7 +994,7 @@ TEST_F(D2CfgMgrTest, fullConfigTest) {
// Verify that the reverse manager has the correct number of domains.
domains = mgr->getDomains();
count = domains->size();
EXPECT_EQ(count, 2);
EXPECT_EQ(2, count);
// Verify that the server count in each of the reverse manager domains.
// NOTE that since prior tests have validated server parsing, we are are
......@@ -1007,7 +1005,7 @@ TEST_F(D2CfgMgrTest, fullConfigTest) {
DnsServerInfoStoragePtr servers = domain->getServers();
count = servers->size();
EXPECT_TRUE(servers);
EXPECT_EQ(count, 3);
EXPECT_EQ(3, count);
}
}
......@@ -1237,45 +1235,4 @@ TEST_F(D2CfgMgrTest, matchReverse) {
ASSERT_THROW(cfg_mgr_->matchReverse("", match), D2CfgError);
}
TEST_F(D2CfgMgrTest, tsigTest) {
std::string config = "{ "
"\"interface\" : \"eth1\" , "
"\"ip_address\" : \"192.168.1.33\" , "
"\"port\" : 88 , "
"\"tsig_keys\": [] ,"
"\"forward_ddns\" : {"
"\"ddns_domains\": [ "
"{ \"name\": \"tmark.org\" , "
" \"dns_servers\" : [ "
" { \"ip_address\": \"127.0.0.1\" } "
" ] } "
", "
"{ \"name\": \"one.tmark.org\" , "
" \"dns_servers\" : [ "
" { \"ip_address\": \"127.0.0.2\" } "
" ] } "
"] },"
"\"reverse_ddns\" : {"
"\"ddns_domains\": [ "
"{ \"name\": \"100.168.192.in-addr.arpa\" , "
" \"dns_servers\" : [ "
" { \"ip_address\": \"127.0.0.1\" } "
" ] }, "
"{ \"name\": \"168.192.in-addr.arpa\" , "
" \"dns_servers\" : [ "
" { \"ip_address\": \"127.0.0.1\" } "
" ] }, "
"{ \"name\": \"*\" , "
" \"dns_servers\" : [ "
" { \"ip_address\": \"127.0.0.1\" } "
" ] } "
"] } }";
ASSERT_TRUE(fromJSON(config));
// Verify that we can parse the configuration.
answer_ = cfg_mgr_->parseConfig(config_set_);
ASSERT_TRUE(checkAnswer(0));
}
} // end of anonymous namespace
......@@ -602,40 +602,37 @@ public:
///
/// @param json_text contains the configuration text in JSON format to
/// convert.
/// @return returns true if the conversion is successful, false otherwise.
bool fromJSON(std::string& json_text) {
/// @return returns AssertionSuccess if there were no parsing errors,
/// AssertionFailure otherwise.
::testing::AssertionResult fromJSON(std::string& json_text) {
try {
config_set_ = isc::data::Element::fromJSON(json_text);
} catch (...) {
#if 0
// Handy for diagnostics
std::cout << "fromJSON failed to parse text" << json_text
<< std::endl;
#endif
return (false);
} catch (const isc::Exception &ex) {
return ::testing::AssertionFailure()
<< "JSON text failed to parse:" << ex.what();
}
return (true);
return ::testing::AssertionSuccess();
}
/// @brief Compares the status in the parse result stored in member
/// variable answer_ to a given value.
///
/// @param should_be is an integer against which to compare the status.
///
/// @return returns true if the status value is equal to the given value.
bool checkAnswer(int should_be) {
/// @return returns AssertionSuccess if there were no parsing errors,
/// AssertionFailure otherwise.
::testing::AssertionResult checkAnswer(int should_be) {
int rcode = 0;
isc::data::ConstElementPtr comment;
comment = isc::config::parseAnswer(rcode, answer_);
#if 0
// Handy for diagnostics
if (rcode != 0) {
std::cout << "checkAnswer rcode:" << rcode << " comment: "
<< *comment << std::endl;
if (rcode == should_be) {
return testing::AssertionSuccess();
}
#endif
return (rcode == should_be);
return ::testing::AssertionFailure() << "checkAnswer rcode:"
<< rcode << " comment: " << *comment << std::endl;
}
/// @brief Configuration set being tested.
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment