[3059] Addressed review comments.

Reverse IP address methods in D2UpdateMgr were declared static. Other
cosmetic changes.

src/bin/d2/d2_cfg_mgr.cc

@@ -97,21 +97,21 @@ D2CfgMgr::reverseIpAddress(const std::string& address) {
         // Convert string address into an IOAddress and invoke the
         // appropriate reverse method.
         isc::asiolink::IOAddress ioaddr(address);
-        if (ioaddr.getFamily() == AF_INET) {
+        if (ioaddr.isV4()) {
             return (reverseV4Address(ioaddr));
         }
 
         return (reverseV6Address(ioaddr));
 
     } catch (const isc::Exception& ex) {
-        isc_throw(D2CfgError, "D2CfgMgr cannot reverse address :"
-                              << address << " : " << ex.what());
+        isc_throw(D2CfgError, "D2CfgMgr cannot reverse address: "
+                               << address << " : " << ex.what());
     }
 }
 
 std::string
 D2CfgMgr::reverseV4Address(const isc::asiolink::IOAddress& ioaddr) {
-    if (ioaddr.getFamily() != AF_INET) {
+    if (!ioaddr.isV4()) {
         isc_throw(D2CfgError, "D2CfgMgr address is not IPv4 address :"
                               << ioaddr.toText());
     }
@@ -121,8 +121,10 @@ D2CfgMgr::reverseV4Address(const isc::asiolink::IOAddress& ioaddr) {
 
     // Walk backwards through vector outputting each octet and a dot.
     std::ostringstream stream;
-    for (int i = 3; i >= 0; i--) {
-        stream << (unsigned int)(bytes[i]) << ".";
+    std::vector<uint8_t>::const_reverse_iterator rit;
+
+    for (rit = bytes.rbegin(); rit != bytes.rend(); ++rit) {
+        stream << static_cast<unsigned int>(*rit) << ".";
     }
 
     // Tack on the suffix and we're done.
@@ -132,8 +134,8 @@ D2CfgMgr::reverseV4Address(const isc::asiolink::IOAddress& ioaddr) {
 
 std::string
 D2CfgMgr::reverseV6Address(const isc::asiolink::IOAddress& ioaddr) {
-    if (ioaddr.getFamily() != AF_INET6) {
-        isc_throw(D2CfgError, "D2Cfg address is not IPv6 address :"
+    if (!ioaddr.isV6()) {
+        isc_throw(D2CfgError, "D2Cfg address is not IPv6 address: "
                               << ioaddr.toText());
     }
 
@@ -146,7 +148,7 @@ D2CfgMgr::reverseV6Address(const isc::asiolink::IOAddress& ioaddr) {
     std::ostringstream stream;
     std::string::const_reverse_iterator rit;
     for (rit = digits.rbegin(); rit != digits.rend(); ++rit) {
-        stream << (char)(*rit) << ".";
+        stream << static_cast<char>(*rit) << ".";
     }
 
     // Tack on the suffix and we're done.

src/bin/d2/d2_cfg_mgr.h

@@ -165,7 +165,7 @@ public:
     /// @return a std::string containing the reverse order address.
     ///
     /// @throw D2CfgError if given an invalid address.
-    std::string reverseIpAddress(const std::string& address);
+    static std::string reverseIpAddress(const std::string& address);
 
     /// @brief Generate a reverse order string for the given IP address
     ///
@@ -184,7 +184,7 @@ public:
     /// @return a std::string containing the reverse order address.
     ///
     /// @throw D2CfgError if not given an IPv4  address.
-    std::string reverseV4Address(const isc::asiolink::IOAddress& ioaddr);
+    static std::string reverseV4Address(const isc::asiolink::IOAddress& ioaddr);
 
     /// @brief Generate a reverse order string for the given IP address
     ///
@@ -204,7 +204,7 @@ public:
     /// @return a std::string containing the reverse order address.
     ///
     /// @throw D2CfgError if not given an IPv6 address.
-    std::string reverseV6Address(const isc::asiolink::IOAddress& ioaddr);
+    static std::string reverseV6Address(const isc::asiolink::IOAddress& ioaddr);
 
 protected:
     /// @brief Given an element_id returns an instance of the appropriate

src/bin/d2/d2_config.cc

@@ -118,7 +118,7 @@ DdnsDomainListMgr::matchDomain(const std::string& fqdn, DdnsDomainPtr& domain) {
 
         // If the lengths are identical and the names match we're done.
         if (req_len == dom_len) {
-            if (memcmp(req_name, domain_name.c_str(), req_len) == 0) {
+            if (req_name == domain_name) {
                 // exact match, done
                 domain = map_pair.second;
                 return (true);
@@ -130,7 +130,7 @@ DdnsDomainListMgr::matchDomain(const std::string& fqdn, DdnsDomainPtr& domain) {
             // prevents "onetwo.net" from matching "two.net".
             size_t offset = req_len - dom_len;
             if ((req_name[offset - 1] == '.')  &&
-               (memcmp(&req_name[offset], domain_name.c_str(), dom_len) == 0)) {
+               (fqdn.compare(offset, std::string::npos, domain_name) == 0)) {
                 // Fqdn contains domain name, keep it if its better than
                 // any we have matched so far.
                 if (dom_len > match_len) {

src/bin/d2/d2_config.h

@@ -321,8 +321,8 @@ typedef boost::shared_ptr<DnsServerInfoStorage> DnsServerInfoStoragePtr;
 /// 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. 
+/// may be worthwhile to change this to a dns::Name for purposes of better
+/// validation and matching capabilities.
 class DdnsDomain {
 public:
     /// @brief Constructor
@@ -385,7 +385,11 @@ typedef boost::shared_ptr<DdnsDomainMap> DdnsDomainMapPtr;
 /// services.  These services are used to match a FQDN to a domain.  Currently
 /// it supports a single matching service, which will return the matching
 /// domain or a wild card domain if one is specified.  The wild card domain is
-/// specified as a domain whose name is "*".
+/// specified as a domain whose name is "*".  The wild card domain will match
+/// any entry and is provided for flexibility in FQDNs  If for instance, all
+/// forward requests are handled by the same servers, the configuration could
+/// specify the wild card domain as the only forward domain. All forward DNS
+/// updates would be sent to that one list of servers, regardless of the FQDN.
 /// As matching capabilities evolve this class is expected to expand.
 class DdnsDomainListMgr {
 public:

src/bin/d2/d2_messages.mes

@@ -1,4 +1,3 @@
-/Users/tmark/ddns/build/trac3059/bind10
 # Copyright (C) 2013  Internet Systems Consortium, Inc. ("ISC")
 #
 # Permission to use, copy, modify, and/or distribute this software for any
@@ -113,7 +112,7 @@ service first starts.
 This is an informational message issued when the controller is exiting
 following a shut down (normal or otherwise) of the service.
 
-% DHCP_DDNS_AT_MAX_TRANSACTIONS application has: %1 queued requests but has reached maximum number of: %2 concurrent transactions
+% DHCP_DDNS_AT_MAX_TRANSACTIONS application has %1 queued requests but has reached maximum number of %2 concurrent transactions
 This is a debug message that indicates that the application has DHCP_DDNS
 requests in the queue but is working as many concurrent requests as allowed.
 
@@ -139,19 +138,19 @@ This is a debug messge issued when all of the queued requests represent clients
 for which there is a an update already in progress.  This may occur under
 normal operations but should be temporary situation.
 
-% DHCP_DDNS_NO_FWD_MATCH_ERROR the configured list of forward DDNS domains does not contain a match for FQDN: %1  The request has been discarded.
+% DHCP_DDNS_NO_FWD_MATCH_ERROR the configured list of forward DDNS domains does not contain a match for FQDN %1  The request has been discarded.
 This is an error message that indicates that DHCP_DDNS received a request to
 update a the forward DNS information for the given FQDN but for which there are
 no configured DDNS domains in the DHCP_DDNS configuration.  Either the DHCP_DDNS
 configuration needs to be updated or the source of the FQDN itself should be
 investigated.
 
-% DHCP_DDNS_NO_MATCH No DNS servers match FQDN: %1
+% DHCP_DDNS_NO_MATCH No DNS servers match FQDN %1
 This is warning message issued when there are no domains in the configuration
 which match the cited fully qualified domain name (FQDN).  The DNS Update
 request for the FQDN cannot be processed.
 
-% DHCP_DDNS_NO_REV_MATCH_ERROR the configured list of reverse DDNS domains does not contain a match for FQDN: %1  The request has been discarded.
+% DHCP_DDNS_NO_REV_MATCH_ERROR the configured list of reverse DDNS domains does not contain a match for FQDN %1  The request has been discarded.
 This is an error message that indicates that DHCP_DDNS received a request to
 update a the reverse DNS information for the given FQDN but for which there are
 no configured DDNS domains in the DHCP_DDNS configuration.  Either the DHCP_DDNS
@@ -162,7 +161,7 @@ investigated.
 This is a debug message issued when the Dhcp-Ddns application enters
 its init method.
 
-% DHCP_DDNS_QUEUE_MGR_QUEUE_FULL application request queue has reached maximum number of entries: %1
+% DHCP_DDNS_QUEUE_MGR_QUEUE_FULL application request queue has reached maximum number of entries %1
 This an error message indicating that DHCP-DDNS is receiving DNS update
 requests faster than they can be processed.  This may mean the maximum queue
 needs to be increased, the DHCP-DDNS clients are simply generating too many

src/bin/d2/d2_update_mgr.cc

@@ -100,7 +100,7 @@ void D2UpdateMgr::pickNextJob() {
     // the same DHCID as a transaction, they are presumed to be for the same
     // "end user".
     size_t queue_count = getQueueCount();
-    for (size_t index = 0; index < queue_count; index++) {
+    for (size_t index = 0; index < queue_count; ++index) {
         dhcp_ddns::NameChangeRequestPtr found_ncr = queue_mgr_->peekAt(index);
         if (!hasTransaction(found_ncr->getDhcid())) {
             queue_mgr_->dequeueAt(index);
@@ -213,12 +213,12 @@ D2UpdateMgr::setMaxTransactions(const size_t new_trans_max) {
 }
 
 size_t
-D2UpdateMgr::getQueueCount() {
+D2UpdateMgr::getQueueCount() const {
     return (queue_mgr_->getQueueSize());
 }
 
 size_t
-D2UpdateMgr::getTransactionCount() {
+D2UpdateMgr::getTransactionCount() const {
     return (transaction_list_.size());
 }
 

src/bin/d2/d2_update_mgr.h

@@ -23,6 +23,7 @@
 #include <d2/d2_queue_mgr.h>
 #include <d2/d2_cfg_mgr.h>
 
+#include <boost/noncopyable.hpp>
 #include <boost/shared_ptr.hpp>
 #include <map>
 
@@ -117,15 +118,15 @@ typedef std::map<TransactionKey, NameChangeTransactionPtr> TransactionList;
 /// The upper layer(s) are responsible for calling sweep in a timely and cyclic
 /// manner.
 ///
-class D2UpdateMgr {
+class D2UpdateMgr : public boost::noncopyable {
 public:
     /// @brief Maximum number of concurrent transactions
     /// NOTE that 32 is an arbitrary choice picked for the initial
     /// implementation.
     static const size_t MAX_TRANSACTIONS_DEFAULT = 32;
 
-#if 0
-    // @todo This is here as a reminder to add statistics.
+    // @todo This structure is not yet used. It is here in anticipation of
+    // enabled statistics capture.
     struct Stats {
         uint64_t start_time_;
         uint64_t stop_time_;
@@ -134,8 +135,7 @@ public:
         uint64_t max_update_time_;
         uint64_t server_rejects_;
         uint64_t server_timeouts_;
-    }
-#endif
+    };
 
     /// @brief Constructor
     ///
@@ -235,6 +235,8 @@ public:
 
     /// @brief Convenience method that checks transaction list for the given key
     ///
+    /// @param key the transaction key value for which to search.
+    ///
     /// @return Returns true if the key is found within the list, false
     /// otherwise.
     bool hasTransaction(const TransactionKey& key);
@@ -254,10 +256,10 @@ public:
     void clearTransactionList();
 
     /// @brief Convenience method that returns the number of requests queued.
-    size_t getQueueCount();
+    size_t getQueueCount() const;
 
     /// @brief Returns the current number of transactions.
-    size_t getTransactionCount();
+    size_t getTransactionCount() const;
 
 private:
     /// @brief Pointer to the queue manager.

src/bin/d2/tests/d2_cfg_mgr_unittests.cc

@@ -53,7 +53,7 @@ public:
 /// @brief Tests that the spec file is valid.
 /// Verifies that the BIND10 DHCP-DDNS configuration specification file
 //  is valid.
-TEST(D2SpecTest, basicSpecTest) {
+TEST(D2SpecTest, basicSpec) {
     ASSERT_NO_THROW(isc::config::
                     moduleSpecFromFile(specfile("dhcp-ddns.spec")));
 }
@@ -252,7 +252,7 @@ public:
 /// 3. Secret cannot be blank.
 /// @TODO TSIG keys are not fully functional. Only basic validation is
 /// currently supported. This test will need to expand as they evolve.
-TEST_F(TSIGKeyInfoTest, invalidEntryTests) {
+TEST_F(TSIGKeyInfoTest, invalidEntry) {
     // Config with a blank name entry.
     std::string config = "{"
                          " \"name\": \"\" , "
@@ -294,7 +294,7 @@ TEST_F(TSIGKeyInfoTest, invalidEntryTests) {
 
 /// @brief Verifies that TSIGKeyInfo parsing creates a proper TSIGKeyInfo
 /// when given a valid combination of entries.
-TEST_F(TSIGKeyInfoTest, validEntryTests) {
+TEST_F(TSIGKeyInfoTest, validEntry) {
     // Valid entries for TSIG key, all items are required.
     std::string config = "{"
                          " \"name\": \"d2_key_one\" , "
@@ -448,7 +448,7 @@ TEST_F(TSIGKeyInfoTest, validTSIGKeyList) {
 /// 1. Specifying both a hostname and an ip address is not allowed.
 /// 2. Specifying both blank a hostname and blank ip address is not allowed.
 /// 3. Specifying a negative port number is not allowed.
-TEST_F(DnsServerInfoTest, invalidEntryTests) {
+TEST_F(DnsServerInfoTest, invalidEntry) {
     // Create a config in which both host and ip address are supplied.
     // Verify that it builds without throwing but commit fails.
     std::string config = "{ \"hostname\": \"pegasus.tmark\", "
@@ -480,7 +480,7 @@ TEST_F(DnsServerInfoTest, invalidEntryTests) {
 /// 1. A DnsServerInfo entry is correctly made, when given only a hostname.
 /// 2. A DnsServerInfo entry is correctly made, when given ip address and port.
 /// 3. A DnsServerInfo entry is correctly made, when given only an ip address.
-TEST_F(DnsServerInfoTest, validEntryTests) {
+TEST_F(DnsServerInfoTest, validEntry) {
     // Valid entries for dynamic host
     std::string config = "{ \"hostname\": \"pegasus.tmark\" }";
     ASSERT_TRUE(fromJSON(config));
@@ -831,7 +831,7 @@ TEST_F(DdnsDomainTest, DdnsDomainListParsing) {
 }
 
 /// @brief Tests that a domain list configuration cannot contain duplicates.
-TEST_F(DdnsDomainTest, duplicateDomainTest) {
+TEST_F(DdnsDomainTest, duplicateDomain) {
     // Create a domain list configuration that contains two domains with
     // the same name.
     std::string config =
@@ -885,7 +885,7 @@ TEST(D2CfgMgr, construction) {
 /// This tests passes the configuration into an instance of D2CfgMgr just
 /// as it would be done by d2_process in response to a configuration update
 /// event.
-TEST_F(D2CfgMgrTest, fullConfigTest) {
+TEST_F(D2CfgMgrTest, fullConfig) {
     // Create a configuration with all of application level parameters, plus
     // both the forward and reverse ddns managers.  Both managers have two
     // domains with three servers per domain.
@@ -1018,7 +1018,7 @@ TEST_F(D2CfgMgrTest, fullConfigTest) {
 /// 2. Given a FQDN for sub-domain in the list, returns the proper match.
 /// 3. Given a FQDN that matches no domain name, returns the wild card domain
 /// as a match.
-TEST_F(D2CfgMgrTest, forwardMatchTest) {
+TEST_F(D2CfgMgrTest, forwardMatch) {
     // Create  configuration with one domain, one sub domain, and the wild
     // card.
     std::string config = "{ "
@@ -1244,7 +1244,7 @@ TEST_F(D2CfgMgrTest, matchReverse) {
     EXPECT_TRUE(cfg_mgr_->matchReverse("2001:db8:302:99::",match));
     EXPECT_EQ("2.0.3.0.8.B.D.0.1.0.0.2.ip6.arpa.", match->getName());
 
-    // Verify a IPv6 wild card match. 
+    // Verify a IPv6 wild card match.
     EXPECT_TRUE(cfg_mgr_->matchReverse("2001:db8:99:302::",match));
     EXPECT_EQ("*", match->getName());