[3089] Addressed review comments.

Changed DNSClient ctor to require response param be
an empty pointer and clarified related commentary.
Added debug log statement to log transaction start.
Other minor cosmetics.

src/bin/d2/d2_messages.mes

@@ -1,4 +1,4 @@
-# Copyright (C) 2013  Internet Systems Consortium, Inc. ("ISC")
+# Copyright (C) 2013-2014  Internet Systems Consortium, Inc. ("ISC")
 #
 # Permission to use, copy, modify, and/or distribute this software for any
 # purpose with or without fee is hereby granted, provided that the above
@@ -438,10 +438,12 @@ This is an error message issued after DHCP_DDNS attempts to submit DNS mapping
 entry removals have failed.  The precise reason for the failure should be
 documented in preceding log entries.
 
-% DHCP_DDNS_UPDATE_REQUEST_SENT for transaction key: %1  to server : %2
+% DHCP_DDNS_STARTING_TRANSACTION Transaction Key: %1
+
+% DHCP_DDNS_UPDATE_REQUEST_SENT for transaction key: %1 to server: %2
 This is a debug message issued when DHCP_DDNS sends a DNS request to a DNS
 server.
 
-% DHCP_DDNS_UPDATE_RESPONSE_RECEIVED for transaction key: %1  to server : %2 status: %3
+% DHCP_DDNS_UPDATE_RESPONSE_RECEIVED for transaction key: %1  to server: %2 status: %3
 This is a debug message issued when DHCP_DDNS receives sends a DNS update
 response from a DNS server.

src/bin/d2/d2_update_mgr.cc

@@ -171,7 +171,7 @@ D2UpdateMgr::makeTransaction(dhcp_ddns::NameChangeRequestPtr& next_ncr) {
                                            forward_domain, reverse_domain));
     } else {
         trans.reset(new NameRemoveTransaction(io_service_, next_ncr,
-                                           forward_domain, reverse_domain));
+                                              forward_domain, reverse_domain));
     }
 
     // Add the new transaction to the list.

src/bin/d2/dns_client.cc

@@ -44,7 +44,14 @@ class DNSClientImpl : public asiodns::IOFetch::Callback {
 public:
     // A buffer holding response from a DNS.
     util::OutputBufferPtr in_buf_;
-    // A caller-supplied object holding a parsed response from DNS.
+    // A caller-supplied object which will hold the parsed response from DNS.
+    // The response object is (or descends from) isc::dns::Message and is
+    // populated using Message::fromWire().  This method may only be called
+    // once in the lifetime of a Message instance.  Therefore, response_ is a
+    // pointer reference thus allowing this class to replace the object
+    // pointed to with a new Message instance each time a message is
+    // received. This allows a single DNSClientImpl instance to be used in
+    // for multiple, sequential IOFetch calls.
     D2UpdateMessagePtr& response_;
     // A caller-supplied external callback which is invoked when DNS message
     // exchange is complete or interrupted.
@@ -81,6 +88,12 @@ DNSClientImpl::DNSClientImpl(D2UpdateMessagePtr& response_placeholder,
     : in_buf_(new OutputBuffer(DEFAULT_BUFFER_SIZE)),
       response_(response_placeholder), callback_(callback), proto_(proto) {
 
+    // Response should be an empty pointer. It gets populated by the
+    // operator() method.
+    if (response_) {
+        isc_throw(isc::BadValue, "Response buffer pointer should be null");
+    }
+
     // @todo Currently we only support UDP. The support for TCP is planned for
     // the future release.
     if (proto_ == DNSClient::TCP) {

src/bin/d2/dns_client.h

@@ -93,8 +93,8 @@ public:
 
     /// @brief Constructor.
     ///
-    /// @param response_placeholder Pointer to an object which will hold a
-    /// DNS server's response. Caller is responsible for allocating this object.
+    /// @param response_placeholder Messge object pointer which will be updated
+    /// with dynamically allocated object holding the DNS server's response.
     /// @param callback Pointer to an object implementing @c DNSClient::Callback
     /// class. This object will be called when DNS message exchange completes or
     /// if an error occurs. NULL value disables callback invocation.

src/bin/d2/nc_trans.cc

@@ -80,6 +80,10 @@ NameChangeTransaction::~NameChangeTransaction(){
 
 void
 NameChangeTransaction::startTransaction() {
+    LOG_DEBUG(dctl_logger, DBGLVL_TRACE_DETAIL,
+              DHCP_DDNS_STARTING_TRANSACTION)
+              .arg(getTransactionKey().toStr());
+
     setNcrStatus(dhcp_ddns::ST_PENDING);
     startModel(READY_ST);
 }
@@ -366,8 +370,9 @@ NameChangeTransaction::selectNextServer() {
     if ((current_server_list_) &&
         (next_server_pos_ < current_server_list_->size())) {
         current_server_  = (*current_server_list_)[next_server_pos_];
-        dns_update_response_.reset(new
-                                   D2UpdateMessage(D2UpdateMessage::INBOUND));
+        // Toss out any previous response.
+        dns_update_response_.reset();
+
         // @todo  Protocol is set on DNSClient constructor.  We need
         // to propagate a configuration value downward, probably starting
         // at global, then domain, then server

src/bin/d2/nc_trans.h

@@ -152,15 +152,10 @@ public:
     //@}
 
     /// @brief Defualt time to assign to a single DNS udpate.
-#if 0
-    /// @todo  This value will be configurable in the near future, but
-    /// until it is there is no way to replace its value.  For now
-    /// we will define it to be relatively short, so unit tests will
-    /// run within reasonable amount of time.
-    static const unsigned int DNS_UPDATE_DEFAULT_TIMEOUT = 5 * 1000;
-#else
+    /// @todo  This value will be made configurable in the very near future
+    /// under trac3268. For now we will define it to 100 milliseconds
+    /// so unit tests will run within a reasonable amount of time.
     static const unsigned int DNS_UPDATE_DEFAULT_TIMEOUT = 100;
-#endif
 
     /// @brief Maximum times to attempt a single update on a given server.
     static const unsigned int MAX_UPDATE_TRIES_PER_SERVER = 3;
@@ -296,7 +291,7 @@ protected:
     void setDnsUpdateRequest(D2UpdateMessagePtr& request);
 
     /// @brief Destroys the current update request packet and resets
-    /// udpate attempts count..
+    /// udpate attempts count.
     void clearDnsUpdateRequest();
 
     /// @brief Sets the update status to the given status value.

src/bin/d2/tests/d2_update_mgr_unittests.cc

@@ -32,7 +32,7 @@ using namespace isc::d2;
 
 namespace {
 
-/// @brief Wrapper class for D2UpdateMgr to provide access non-public methods.
+/// @brief Wrapper class for D2UpdateMgr providing access to non-public methods.
 ///
 /// This class facilitates testing by making non-public methods accessible so
 /// they can be invoked directly in test routines.

src/bin/d2/tests/dns_client_unittests.cc

@@ -89,7 +89,7 @@ public:
           test_timer_(service_),
           received_(0), expected_(0) {
         asiodns::logger.setSeverity(isc::log::INFO);
-        response_.reset(new D2UpdateMessage(D2UpdateMessage::INBOUND));
+        response_.reset();
         dns_client_.reset(new DNSClient(response_, this));
 
         // Set the test timeout to break any running tasks if they hang.

src/bin/d2/tests/nc_test_utils.h

@@ -101,7 +101,16 @@ public:
     }
 };
 
-class TimedIO {
+/// @brief Provides a means to process IOService IO for a finite amount of time.
+///
+/// This class instantiates an IOService provides a single method, runTimedIO
+/// which will run the IOService for no more than a finite amount of time,
+/// at least one event is executed or the IOService is stopped.
+/// It provides an overridable handler for timer expiration event.  It is
+/// intended to be used as a base class for test fixtures that need to process
+/// IO by providing them a consistent way to do so while retaining a safety valve
+/// so tests do not hang.
+class TimedIO  {
 public:
     IOServicePtr io_service_;
     asiolink::IntervalTimer timer_;

src/bin/d2/tests/nc_trans_unittests.cc

@@ -578,7 +578,16 @@ TEST_F(NameChangeTransactionTest, serverSelectionTest) {
     // they are correct after each selection.
     DnsServerInfoPtr prev_server = name_change->getCurrentServer();
     DNSClientPtr prev_client = name_change->getDNSClient();
-    D2UpdateMessagePtr prev_response = name_change->getDnsUpdateResponse();
+
+    // Verify response pointer is empty.
+    EXPECT_FALSE(name_change->getDnsUpdateResponse());
+
+    // Create dummy response so we can verify it is cleared at each
+    // new server select.
+    D2UpdateMessagePtr dummyResp;
+    dummyResp.reset(new D2UpdateMessage(D2UpdateMessage::INBOUND));
+    ASSERT_NO_THROW(name_change->setDnsUpdateResponse(dummyResp));
+    ASSERT_TRUE(name_change->getDnsUpdateResponse());
 
     // Iteratively select through the list of servers.
     int passes = 0;
@@ -591,17 +600,22 @@ TEST_F(NameChangeTransactionTest, serverSelectionTest) {
         // Verify that the new values are not empty.
         EXPECT_TRUE(server);
         EXPECT_TRUE(client);
-        EXPECT_TRUE(response);
+
+        // Verify response pointer is now empty.
+        EXPECT_FALSE(name_change->getDnsUpdateResponse());
 
         // Verify that the new values are indeed new.
         EXPECT_NE(server, prev_server);
         EXPECT_NE(client, prev_client);
-        EXPECT_NE(response, prev_response);
 
         // Remember the selected values for the next pass.
         prev_server = server;
         prev_client = client;
-        prev_response = response;
+
+        // Create new dummy response.
+        dummyResp.reset(new D2UpdateMessage(D2UpdateMessage::INBOUND));
+        ASSERT_NO_THROW(name_change->setDnsUpdateResponse(dummyResp));
+        ASSERT_TRUE(name_change->getDnsUpdateResponse());
 
         ++passes;
     }
@@ -626,12 +640,11 @@ TEST_F(NameChangeTransactionTest, serverSelectionTest) {
     ASSERT_NO_THROW(name_change->initServerSelection(domain));
 
     // The server selection process determines the current server,
-    // instantiates a new DNSClient, and a DNS response message buffer.
+    // instantiates a new DNSClient, and resets the DNS response message buffer.
     // We need to save the values before each selection, so we can verify
     // they are correct after each selection.
     prev_server = name_change->getCurrentServer();
     prev_client = name_change->getDNSClient();
-    prev_response = name_change->getDnsUpdateResponse();
 
     // Iteratively select through the list of servers.
     passes = 0;
@@ -644,17 +657,22 @@ TEST_F(NameChangeTransactionTest, serverSelectionTest) {
         // Verify that the new values are not empty.
         EXPECT_TRUE(server);
         EXPECT_TRUE(client);
-        EXPECT_TRUE(response);
+
+        // Verify response pointer is now empty.
+        EXPECT_FALSE(name_change->getDnsUpdateResponse());
 
         // Verify that the new values are indeed new.
         EXPECT_NE(server, prev_server);
         EXPECT_NE(client, prev_client);
-        EXPECT_NE(response, prev_response);
 
         // Remember the selected values for the next pass.
         prev_server = server;
         prev_client = client;
-        prev_response = response;
+
+        // Create new dummy response.
+        dummyResp.reset(new D2UpdateMessage(D2UpdateMessage::INBOUND));
+        ASSERT_NO_THROW(name_change->setDnsUpdateResponse(dummyResp));
+        ASSERT_TRUE(name_change->getDnsUpdateResponse());
 
         ++passes;
     }