[3059] Additional review related changes.

D2UpdateMgr members were rescoped to be more appropriate
and unit tests adjusted accordingly. Other minor changes.

src/bin/d2/d2_config.cc

@@ -91,9 +91,6 @@ DdnsDomainListMgr::setDomains(DdnsDomainMapPtr domains) {
 
 bool
 DdnsDomainListMgr::matchDomain(const std::string& fqdn, DdnsDomainPtr& domain) {
-    // Clear the return parameter.
-    domain.reset();
-
     // First check the case of one domain to rule them all.
     if ((size() == 1) && (wildcard_domain_)) {
         domain = wildcard_domain_;
@@ -106,6 +103,7 @@ DdnsDomainListMgr::matchDomain(const std::string& fqdn, DdnsDomainPtr& domain) {
     size_t req_len = fqdn.size();
     size_t match_len = 0;
     DdnsDomainMapPair map_pair;
+    DdnsDomainPtr best_match;
     BOOST_FOREACH (map_pair, *domains_) {
         std::string domain_name = map_pair.first;
         size_t dom_len = domain_name.size();
@@ -134,13 +132,13 @@ DdnsDomainListMgr::matchDomain(const std::string& fqdn, DdnsDomainPtr& domain) {
                 // any we have matched so far.
                 if (dom_len > match_len) {
                     match_len = dom_len;
-                    domain = map_pair.second;
+                    best_match = map_pair.second;
                 }
             }
         }
     }
 
-    if (!domain) {
+    if (!best_match) {
         // There's no match. If they specified a wild card domain use it
         // otherwise there's no domain for this entry.
         if (wildcard_domain_) {
@@ -152,6 +150,7 @@ DdnsDomainListMgr::matchDomain(const std::string& fqdn, DdnsDomainPtr& domain) {
         return (false);
     }
 
+    domain = best_match;
     return (true);
 }
 

src/bin/d2/d2_config.h

@@ -414,8 +414,8 @@ public:
     /// it will be returned immediately for any FQDN.
     ///
     /// @param fqdn is the name for which to look.
-    /// @param domain receives the matching domain. Note that it will be reset
-    /// upon entry and only set if a match is subsequently found.
+    /// @param domain receives the matching domain. If no match is found its
+    /// contents will be unchanged.
     ///
     /// @return returns true if a match is found, false otherwise.
     /// @todo This is a very basic match method, which expects valid FQDNs

src/bin/d2/d2_update_mgr.h

@@ -152,7 +152,7 @@ public:
                 const size_t max_transactions = MAX_TRANSACTIONS_DEFAULT);
 
     /// @brief Destructor
-    ~D2UpdateMgr();
+    virtual ~D2UpdateMgr();
 
     /// @brief Check current transactions; start transactions for new requests.
     ///
@@ -170,6 +170,7 @@ public:
     /// add the transaction to the list of transactions.
     void sweep();
 
+protected:
     /// @brief Performs post-completion cleanup on completed transactions.
     ///
     /// Iterates through the list of transactions and removes any that have
@@ -208,6 +209,7 @@ public:
     /// exists. Note this would be programmatic error.
     void makeTransaction(isc::dhcp_ddns::NameChangeRequestPtr& ncr);
 
+public:
     /// @brief Returns the maximum number of concurrent transactions.
     size_t getMaxTransactions() const {
         return (max_transactions_);
@@ -286,6 +288,7 @@ private:
 /// @brief Defines a pointer to a D2UpdateMgr instance.
 typedef boost::shared_ptr<D2UpdateMgr> D2UpdateMgrPtr;
 
+
 } // namespace isc::d2
 } // namespace isc
 #endif

src/bin/d2/tests/d2_update_mgr_unittests.cc

@@ -31,23 +31,56 @@ using namespace isc::d2;
 
 namespace {
 
+/// @brief Wrapper class for D2UpdateMgr to provide acces non-public methods.
+///
+/// This class faciliates testing by making non-public methods accessible so
+/// they can be invoked directly in test routines.
+class D2UpdateMgrWrapper : public D2UpdateMgr {
+public:
+    /// @brief Constructor
+    ///
+    /// Parameters match those needed by D2UpdateMgr.
+    D2UpdateMgrWrapper(D2QueueMgrPtr& queue_mgr, D2CfgMgrPtr& cfg_mgr,
+                       isc::asiolink::IOService& io_service,
+                       const size_t max_transactions = MAX_TRANSACTIONS_DEFAULT)
+        : D2UpdateMgr(queue_mgr, cfg_mgr, io_service, max_transactions) {
+    }
+
+    /// @brief Destructor
+    virtual ~D2UpdateMgrWrapper() {
+    }
+
+    // Expose the protected methods to be tested.
+    using D2UpdateMgr::checkFinishedTransactions;
+    using D2UpdateMgr::pickNextJob;
+    using D2UpdateMgr::makeTransaction;
+};
+
+/// @brief Defines a pointer to a D2UpdateMgr instance.
+typedef boost::shared_ptr<D2UpdateMgrWrapper> D2UpdateMgrWrapperPtr;
+
 /// @brief Test fixture for testing D2UpdateMgr.
-/// D2UpdateMgr depends on both D2QueueMgr and D2CfgMgr.  This fixture
-/// provides an instance of each, plus a canned, valid DHCP_DDNS configuration
-/// sufficient to test D2UpdateMgr's basic functions.
+///
+/// Note this class uses D2UpdateMgrWrapper class to exercise non-public
+/// aspects of D2UpdateMgr. D2UpdateMgr depends on both D2QueueMgr and
+/// D2CfgMgr.  This fixture provides an instance of each, plus a canned,
+/// valid DHCP_DDNS configuration sufficient to test D2UpdateMgr's basic
+/// functions.
 class D2UpdateMgrTest : public ConfigParseTest {
 public:
     isc::asiolink::IOService io_service_;
     D2QueueMgrPtr queue_mgr_;
     D2CfgMgrPtr cfg_mgr_;
-    D2UpdateMgrPtr update_mgr_;
+    //D2UpdateMgrPtr update_mgr_;
+    D2UpdateMgrWrapperPtr update_mgr_;
     std::vector<NameChangeRequestPtr> canned_ncrs_;
     size_t canned_count_;
 
     D2UpdateMgrTest() {
         queue_mgr_.reset(new D2QueueMgr(io_service_));
         cfg_mgr_.reset(new D2CfgMgr());
-        update_mgr_.reset(new D2UpdateMgr(queue_mgr_, cfg_mgr_, io_service_));
+        update_mgr_.reset(new D2UpdateMgrWrapper(queue_mgr_, cfg_mgr_,
+                                                 io_service_));
         makeCannedNcrs();
         makeCannedConfig();
     }