[3088] Addressed review changes.

Changes are largely clean up and commentary.

src/bin/d2/d2_messages.mes

@@ -347,12 +347,12 @@ aborted.  This is most likely a configuration issue.
 This is a debug message issued after DHCP_DDNS has submitted DNS mapping
 additions which were received and accepted by an appropriate DNS server.
 
-% DHCP_DDNS_ADD_FAILED DHCP_DDNS failed attempting to make DNS mapping additions for this request: %1
+% DHCP_DDNS_ADD_FAILED DHCP_DDNS failed attempting to make DNS mapping additions for this request: %1, event: %2
 This is an error message issued after DHCP_DDNS attempts to submit DNS mapping
 entry additions have failed.  The precise reason for the failure should be
 documented in preceding log entries.
 
-% DHCP_DDNS_FORWARD_REMOVE_ADDRS_BUILD_FAILURE DNS udpate message to remove a forward DNS Address entry could not be constructed for this request: %1  reason: %2
+% DHCP_DDNS_FORWARD_REMOVE_ADDRS_BUILD_FAILURE DNS udpate message to remove a forward DNS Address entry could not be constructed for this request: %1,  reason: %2
 This is an error message issued when an error occurs attempting to construct
 the server bound packet requesting a forward address (A or AAAA) removal.  This
 is due to invalid data contained in the NameChangeRequest. The request will be
@@ -378,7 +378,7 @@ This is an error message issued when DNSClient returns an unrecognized status
 while DHCP_DDNS was removing a forward address mapping.  The request will be
 aborted.  This is most likely a programmatic issue and should be reported.
 
-% DHCP_DDNS_FORWARD_REMOVE_RRS_BUILD_FAILURE DNS udpate message to remove forward DNS RR entries could not be constructed for this request: %1  reason: %2
+% DHCP_DDNS_FORWARD_REMOVE_RRS_BUILD_FAILURE DNS udpate message to remove forward DNS RR entries could not be constructed for this request: %1,  reason: %2
 This is an error message issued when an error occurs attempting to construct
 the server bound packet requesting forward RR (DHCID RR) removal.  This is due
 to invalid data contained in the NameChangeRequest. The request will be aborted.This is most likely a configuration issue.
@@ -403,7 +403,7 @@ This is an error message issued when DNSClient returns an unrecognized status
 while DHCP_DDNS was removing forward RRs.  The request will be aborted. This is
 most likely a programmatic issue and should be reported.
 
-% DHCP_DDNS_REVERSE_REMOVE_BUILD_FAILURE DNS update message to remove a reverse DNS entry could not be constructed from this request: %1  reason: %2
+% DHCP_DDNS_REVERSE_REMOVE_BUILD_FAILURE DNS update message to remove a reverse DNS entry could not be constructed from this request: %1,  reason: %2
 This is an error message issued when an error occurs attempting to construct
 the server bound packet requesting a reverse PTR removal.  This is
 due to invalid data contained in the NameChangeRequest. The request will be
@@ -433,8 +433,8 @@ aborted.  This is most likely a programmatic issue and should be reported.
 This is a debug message issued after DHCP_DDNS has submitted DNS mapping
 removals which were received and accepted by an appropriate DNS server.
 
-% DHCP_DDNS_REMOVE_FAILED DHCP_DDNS failed attempting to make DNS mapping removals for this request: %1
+% DHCP_DDNS_REMOVE_FAILED DHCP_DDNS failed attempting to make DNS mapping removals for this request: %1, event: %2
 This is an error message issued after DHCP_DDNS attempts to submit DNS mapping
-entry removals have failed.  There precise reason for the failure should be
+entry removals have failed.  The precise reason for the failure should be
 documented in preceding log entries.
 

src/bin/d2/nc_add.cc

@@ -61,10 +61,18 @@ NameAddTransaction::defineEvents() {
 
 void
 NameAddTransaction::verifyEvents() {
-    // Call superclass impl first.
+    // Call superclass implementation first to verify its events. These are
+    // events common to all transactions, and they must be defined.
+    // SELECT_SERVER_EVT
+    // SERVER_SELECTED_EVT
+    // SERVER_IO_ERROR_EVT
+    // NO_MORE_SERVERS_EVT
+    // IO_COMPLETED_EVT
+    // UPDATE_OK_EVT
+    // UPDATE_FAILED_EVT
     NameChangeTransaction::verifyEvents();
 
-    // Verify NameAddTransaction events.
+    // Verify NameAddTransaction events by attempting to fetch them.
     getEvent(FQDN_IN_USE_EVT);
     getEvent(FQDN_NOT_IN_USE_EVT);
 }
@@ -102,10 +110,16 @@ NameAddTransaction::defineStates() {
 }
 void
 NameAddTransaction::verifyStates() {
-    // Call superclass impl first.
+    // Call superclass implementation first to verify its states. These are
+    // states common to all transactions, and they must be defined.
+    // READY_ST
+    // SELECTING_FWD_SERVER_ST
+    // SELECTING_REV_SERVER_ST
+    // PROCESS_TRANS_OK_ST
+    // PROCESS_TRANS_FAILED_ST
     NameChangeTransaction::verifyStates();
 
-    // Verify NameAddTransaction states.
+    // Verify NameAddTransaction states by attempting to fetch them.
     getState(ADDING_FWD_ADDRS_ST);
     getState(REPLACING_FWD_ADDRS_ST);
     getState(REPLACING_REV_PTRS_ST);
@@ -542,7 +556,8 @@ NameAddTransaction::processAddFailedHandler() {
     switch(getNextEvent()) {
     case UPDATE_FAILED_EVT:
     case NO_MORE_SERVERS_EVT:
-        LOG_ERROR(dctl_logger, DHCP_DDNS_ADD_FAILED).arg(getNcr()->toText());
+        LOG_ERROR(dctl_logger, DHCP_DDNS_ADD_FAILED).arg(getNcr()->toText())
+        .arg(getContextStr());
         setNcrStatus(dhcp_ddns::ST_FAILED);
         endModel();
         break;

src/bin/d2/nc_add.h

@@ -110,7 +110,8 @@ protected:
     /// @brief Validates the contents of the set of events.
     ///
     /// Invokes NameChangeTransaction's implementation and then verifies the
-    /// Add transaction's events.
+    /// Add transaction's.  This tests that the needed events are in the event
+    /// dictionary.
     ///
     /// @throw StateModelError if an event value is undefined.
     virtual void verifyEvents();
@@ -126,7 +127,8 @@ protected:
     /// @brief Validates the contents of the set of states.
     ///
     /// Invokes NameChangeTransaction's implementation and then verifies the
-    /// Add transaction's states.
+    /// Add transaction's states. This tests that the needed states are in the
+    /// state dictionary.
     ///
     /// @throw StateModelError if an event value is undefined.
     virtual void verifyStates();
@@ -441,7 +443,7 @@ protected:
     void buildReplaceRevPtrsRequest();
 };
 
-/// @brief Defines a pointer to a NameChangeTransaction.
+/// @brief Defines a pointer to a NameAddTransaction.
 typedef boost::shared_ptr<NameAddTransaction> NameAddTransactionPtr;
 
 } // namespace isc::d2

src/bin/d2/nc_remove.cc

@@ -58,10 +58,18 @@ NameRemoveTransaction::defineEvents() {
 
 void
 NameRemoveTransaction::verifyEvents() {
-    // Call superclass impl first.
+    // Call superclass implementation first to verify its events. These are
+    // events common to all transactions, and they must be defined.
+    // SELECT_SERVER_EVT
+    // SERVER_SELECTED_EVT
+    // SERVER_IO_ERROR_EVT
+    // NO_MORE_SERVERS_EVT
+    // IO_COMPLETED_EVT
+    // UPDATE_OK_EVT
+    // UPDATE_FAILED_EVT
     NameChangeTransaction::verifyEvents();
 
-    // Verify NameRemoveTransaction events.
+    // Verify NameRemoveTransaction events by attempting to fetch them.
     // Currently NameRemoveTransaction does not define any events.
     // getEvent(TBD_EVENT);
 }
@@ -106,10 +114,16 @@ NameRemoveTransaction::defineStates() {
 
 void
 NameRemoveTransaction::verifyStates() {
-    // Call superclass impl first.
+    // Call superclass implementation first to verify its states. These are
+    // states common to all transactions, and they must be defined.
+    // READY_ST
+    // SELECTING_FWD_SERVER_ST
+    // SELECTING_REV_SERVER_ST
+    // PROCESS_TRANS_OK_ST
+    // PROCESS_TRANS_FAILED_ST
     NameChangeTransaction::verifyStates();
 
-    // Verify NameRemoveTransaction states.
+    // Verify NameRemoveTransaction states by attempting to fetch them.
     getState(REMOVING_FWD_ADDRS_ST);
     getState(REMOVING_FWD_RRS_ST);
     getState(REMOVING_REV_PTRS_ST);
@@ -551,7 +565,8 @@ NameRemoveTransaction::processRemoveFailedHandler() {
     case UPDATE_FAILED_EVT:
     case NO_MORE_SERVERS_EVT:
     case SERVER_IO_ERROR_EVT:
-        LOG_ERROR(dctl_logger, DHCP_DDNS_REMOVE_FAILED).arg(getNcr()->toText());
+        LOG_ERROR(dctl_logger, DHCP_DDNS_REMOVE_FAILED).arg(getNcr()->toText())
+        .arg(getEventLabel(getNextEvent()));
         setNcrStatus(dhcp_ddns::ST_FAILED);
         endModel();
         break;

src/bin/d2/nc_remove.h

@@ -95,7 +95,7 @@ public:
     virtual ~NameRemoveTransaction();
 
 protected:
-    /// @brief Removes events defined by NameRemoveTransaction to the event set.
+    /// @brief Adds events defined by NameRemoveTransaction to the event set.
     ///
     /// Invokes NameChangeTransaction's implementation and then defines the
     /// events unique to NCR Remove transaction processing.
@@ -106,12 +106,13 @@ protected:
     /// @brief Validates the contents of the set of events.
     ///
     /// Invokes NameChangeTransaction's implementation and then verifies the
-    /// Remove transaction's events.
+    /// Remove transaction's events. This tests that the needed events are in
+    /// the event dictionary.
     ///
     /// @throw StateModelError if an event value is undefined.
     virtual void verifyEvents();
 
-    /// @brief Removes states defined by NameRemoveTransaction to the state set.
+    /// @brief Adds states defined by NameRemoveTransaction to the state set.
     ///
     /// Invokes NameChangeTransaction's implementation and then defines the
     /// states unique to NCR Remove transaction processing.
@@ -122,7 +123,8 @@ protected:
     /// @brief Validates the contents of the set of states.
     ///
     /// Invokes NameChangeTransaction's implementation and then verifies the
-    /// Remove transaction's states.
+    /// Remove transaction's states.  This tests that the needed states are in
+    /// the state dictionary.
     ///
     /// @throw StateModelError if an event value is undefined.
     virtual void verifyStates();
@@ -134,7 +136,7 @@ protected:
     ///
     /// The READY_ST is the state the model transitions into when the inherited
     /// method, startTransaction() is invoked.  This handler, therefore, is the
-    /// entry point into the state model execution.h  Its primary task is to
+    /// entry point into the state model execution.  Its primary task is to
     /// determine whether to start with a forward DNS change or a reverse DNS
     /// change.
     ///
@@ -424,7 +426,7 @@ protected:
     void buildRemoveRevPtrsRequest();
 };
 
-/// @brief Defines a pointer to a NameChangeTransaction.
+/// @brief Defines a pointer to a NameRemoveTransaction.
 typedef boost::shared_ptr<NameRemoveTransaction> NameRemoveTransactionPtr;
 
 

src/bin/d2/nc_trans.cc

@@ -263,12 +263,13 @@ NameChangeTransaction::addLeaseAddressRdata(dns::RRsetPtr& rrset) {
 
     try {
         // Manufacture an RData from the lease address then add it to the RR.
-        dns::rdata::ConstRdataPtr rdata;
         if (ncr_->isV4()) {
-            rdata.reset(new dns::rdata::in::A(ncr_->getIpAddress()));
+            dns::rdata::ConstRdataPtr rdata(new dns::rdata::in::
+                                            A(ncr_->getIpAddress()));
             rrset->addRdata(rdata);
         } else {
-            rdata.reset(new dns::rdata::in::AAAA(ncr_->getIpAddress()));
+            dns::rdata::ConstRdataPtr rdata(new dns::rdata::in::
+                                            AAAA(ncr_->getIpAddress()));
             rrset->addRdata(rdata);
         }
     } catch (const std::exception& ex) {
@@ -286,10 +287,10 @@ NameChangeTransaction::addDhcidRdata(dns::RRsetPtr& rrset) {
     }
 
     try {
-        dns::rdata::ConstRdataPtr rdata;
         const std::vector<uint8_t>& ncr_dhcid = ncr_->getDhcid().getBytes();
         util::InputBuffer buffer(ncr_dhcid.data(), ncr_dhcid.size());
-        rdata.reset(new dns::rdata::in::DHCID(buffer, ncr_dhcid.size()));
+        dns::rdata::ConstRdataPtr rdata (new dns::rdata::in::
+                                         DHCID(buffer, ncr_dhcid.size()));
         rrset->addRdata(rdata);
     } catch (const std::exception& ex) {
         isc_throw(NameChangeTransactionError, "Cannot add DCHID rdata: "
@@ -306,8 +307,8 @@ NameChangeTransaction::addPtrRdata(dns::RRsetPtr& rrset) {
     }
 
     try {
-        dns::rdata::ConstRdataPtr rdata;
-        rdata.reset(new dns::rdata::generic::PTR(getNcr()->getFqdn()));
+        dns::rdata::ConstRdataPtr rdata(new dns::rdata::generic::
+                                        PTR(getNcr()->getFqdn()));
         rrset->addRdata(rdata);
     } catch (const std::exception& ex) {
         isc_throw(NameChangeTransactionError, "Cannot add PTR rdata: "

src/bin/d2/nc_trans.h

@@ -277,7 +277,7 @@ protected:
     /// If the maximum number of attempts has been reached, it will transition
     /// to the given state with a next event of SERVER_IO_ERROR_EVT.
     ///
-    /// @param server_sel_state  State to transition to if maximum attempts
+    /// @param fail_to_state  State to transition to if maximum attempts
     /// have been tried.
     ///
     void retryTransition(const int fail_to_state);
@@ -435,13 +435,13 @@ public:
 
     /// @brief Fetches the forward DdnsDomain.
     ///
-    /// @return A pointer reference to the forward DdnsDomain.  If 
+    /// @return A pointer reference to the forward DdnsDomain.  If
     /// the request does not include a forward change, the pointer will empty.
     DdnsDomainPtr& getForwardDomain();
 
     /// @brief Fetches the reverse DdnsDomain.
     ///
-    /// @return A pointer reference to the reverse DdnsDomain.  If 
+    /// @return A pointer reference to the reverse DdnsDomain.  If
     /// the request does not include a reverse change, the pointer will empty.
     DdnsDomainPtr& getReverseDomain();
 

src/bin/d2/tests/nc_remove_unittests.cc

@@ -33,9 +33,9 @@ namespace {
 class NameRemoveStub : public NameRemoveTransaction {
 public:
     NameRemoveStub(IOServicePtr& io_service,
-                dhcp_ddns::NameChangeRequestPtr& ncr,
-                DdnsDomainPtr& forward_domain,
-                DdnsDomainPtr& reverse_domain)
+                   dhcp_ddns::NameChangeRequestPtr& ncr,
+                   DdnsDomainPtr& forward_domain,
+                   DdnsDomainPtr& reverse_domain)
         : NameRemoveTransaction(io_service, ncr, forward_domain,
                                 reverse_domain),
           simulate_send_exception_(false),
@@ -328,7 +328,6 @@ TEST_F(NameRemoveTransactionTest, buildRemoveFwdAddressRequest) {
     // and then verify the request contents.
     NameRemoveStubPtr name_remove;
     ASSERT_NO_THROW(name_remove = makeTransaction4(FORWARD_CHG));
-    (name_remove->buildRemoveFwdAddressRequest());
     ASSERT_NO_THROW(name_remove->buildRemoveFwdAddressRequest());
     checkRemoveFwdAddressRequest(*name_remove);