[3035] Addressed "minor" issues rised in the review comments.

The minor issues addressed are:
- Missing comments
- Lack of todo
- Log messages

src/bin/dhcp4/dhcp4.dox

@@ -96,7 +96,7 @@ client to indicate that this name will be used to perform DNS update.
 
 The b10-dhcp-ddns process is responsible for the actual communication with the
 DNS, i.e. to send DNS update messages. The b10-dhcp4 module is responsible for
-generating so called @ref isc::dhcp_ddns::NameChangeRequest and sending it to
+generating @ref isc::dhcp_ddns::NameChangeRequest and sending it to
 the b10-dhcp-ddns module. The @ref isc::dhcp_ddns::NameChangeRequest object
 represents changes to the DNS bindings, related to acquisition, renewal or
 release of the DHCP lease. The b10-dhcp4 module implements the simple FIFO queue
@@ -139,7 +139,7 @@ change comparing to the NameChangeRequest is not generated.
 
 The DHCPv4 Client FQDN %Option comprises flags which communicate to the server
 what updates (if any) client expects the server to perform. Server may be
-configured to obey client's preference to do FQDN processing in a different way.
+configured to obey client's preference or to do FQDN processing in a different way.
 If the server overrides client's preference it will communicate it by sending
 the DHCPv4 Client FQDN %Option in its responses to a client, with the appropriate
 flags set.

src/bin/dhcp4/dhcp4_messages.mes

@@ -146,8 +146,8 @@ from the acquired address. The message argument indicates the reason for the
 failure.
 
 % DHCP4_NCR_CREATION_FAILED failed to generate name change requests for DNS: %1
-This message indicates that server was unable to generate so called
-NameChangeRequests which should be sent to the b10-dhcp_ddns module to create
+This message indicates that server was unable to generate NameChangeRequests
+which should be sent to the b10-dhcp_ddns module to create
 new DNS records for the lease being acquired or to update existing records
 for the renewed lease. The reason for the failure is printed in the logged
 message.
@@ -229,15 +229,11 @@ failure is given in the message.
 % DHCP4_QUERY_DATA received packet type %1, data is <%2>
 A debug message listing the data received from the client.
 
-% DHCP4_QUEUE_ADDITION_NCR name change request for adding new DNS entry queued: %1
-A debug message which is logged when the NameChangeRequest to add new DNS
-entries for the particular lease has been queued. The parameter of this log
-carries the details of the NameChangeRequest.
-
-% DHCP4_QUEUE_REMOVAL_NCR name change request for removing a DNS entry queued: %1
-A debug message which is logged when the NameChangeRequest to remove existing
-DNS entry for the particular lease has been queued. The parameter of this log
-carries the details of the NameChangeRequest.
+% DHCP4_QUEUE_NCR name change request to %1 DNS entry queued: %2
+A debug message which is logged when the NameChangeRequest to add or remove
+a DNS entries for a particular lease has been queued. The first parameter of
+this log message indicates whether the DNS entry is to be added or removed.
+The second parameter carries the details of the NameChangeRequest.
 
 % DHCP4_RELEASE address %1 belonging to client-id %2, hwaddr %3 was released properly.
 This debug message indicates that an address was released properly. It

src/bin/dhcp4/dhcp4_srv.cc

@@ -78,10 +78,15 @@ namespace dhcp {
 
 namespace {
 
-// The following constants describe server's behavior with respect to the
+// @todo The following constants describe server's behavior with respect to the
 // DHCPv4 Client FQDN Option sent by a client. They will be removed
 // when DDNS parameters for DHCPv4 are implemented with the ticket #3033.
 
+// @todo Additional configuration parameter which we may consider is the one
+// that controls whether the DHCP server sends the removal NameChangeRequest
+// if it discovers that the entry for the particular client exists or that
+// it always updates the DNS.
+
 // Should server always include the FQDN option in its response, regardless
 // if it has been requested in Parameter Request List Option (Disabled).
 const bool FQDN_ALWAYS_INCLUDE = false;
@@ -940,31 +945,25 @@ queueNameChangeRequest(const isc::dhcp_ddns::NameChangeType chg_type,
         (!lease->fqdn_rev_ && !lease->fqdn_fwd_)) {
         return;
     }
+
+    // Create the DHCID for the NameChangeRequest.
     D2Dhcid dhcid;
     try {
         dhcid  = computeDhcid(lease);
-
     } catch (const DhcidComputeError& ex) {
         LOG_ERROR(dhcp4_logger, DHCP4_DHCID_COMPUTE_ERROR)
             .arg(lease->toText())
             .arg(ex.what());
         return;
-
     }
+    // Create NameChangeRequest
     NameChangeRequest ncr(chg_type, lease->fqdn_fwd_, lease->fqdn_rev_,
                           lease->hostname_, lease->addr_.toText(),
                           dhcid, 0, lease->valid_lft_);
-    if (chg_type == isc::dhcp_ddns::CHG_ADD) {
-        LOG_DEBUG(dhcp4_logger, DBG_DHCP4_DETAIL_DATA,
-                  DHCP4_QUEUE_ADDITION_NCR)
-            .arg(ncr.toText());
-
-    } else {
-        LOG_DEBUG(dhcp4_logger, DBG_DHCP4_DETAIL_DATA,
-                  DHCP4_QUEUE_REMOVAL_NCR)
-            .arg(ncr.toText());
-
-    }
+    // And queue it.
+    LOG_DEBUG(dhcp4_logger, DBG_DHCP4_DETAIL_DATA, DHCP4_QUEUE_NCR)
+        .arg(chg_type == CHG_ADD ? "add" : "remove")
+        .arg(ncr.toText());
     name_change_reqs_.push(ncr);
 }
 

src/bin/dhcp4/dhcp4_srv.h

@@ -300,6 +300,12 @@ protected:
 private:
     /// @brief Process Client FQDN %Option sent by a client.
     ///
+    /// This function is called by the @c Dhcpv4Srv::processClientName when
+    /// the client has sent the FQDN option in its message to the server.
+    /// It comprises the actual logic to parse the FQDN option and prepare
+    /// the FQDN option to be sent back to the client in the server's
+    /// response.
+    ///
     /// @param fqdn An DHCPv4 Client FQDN %Option sent by a client.
     /// @param [out] answer A response message to be sent to a client.
     void processClientFqdnOption(const Option4ClientFqdnPtr& fqdn,
@@ -433,8 +439,24 @@ protected:
 
     /// @brief Computes DHCID from a lease.
     ///
+    /// This method creates an object which represents DHCID. The DHCID value
+    /// is computed as described in RFC4701. The section 3.3. of RFC4701
+    /// specifies the DHCID RR Identifier Type codes:
+    /// - 0x0000 The 1 octet htype followed by glen octets of chaddr
+    /// - 0x0001 The data octets from the DHCPv4 client's Client Identifier
+    /// option.
+    /// - 0x0002 The client's DUID.
+    ///
+    /// Currently this function supports first two of these identifiers.
+    /// The 0x0001 is preferred over the 0x0000 - if the client identifier
+    /// option is present, the former is used. If the client identifier
+    /// is absent, the latter is used.
+    ///
+    /// @todo Implement support for 0x0002 DHCID identifier type.
+    ///
     /// @param lease A pointer to the structure describing a lease.
     /// @return An object encapsulating DHCID to be used for DNS updates.
+    /// @throw DhcidComputeError If the computation of the DHCID failed.
     static isc::dhcp_ddns::D2Dhcid computeDhcid(const Lease4Ptr& lease);
 
     /// @brief Selects a subnet for a given client's packet.

src/bin/dhcp4/tests/fqdn_unittest.cc

@@ -353,7 +353,8 @@ TEST_F(NameDhcpv4SrvTest, serverUpdateForwardFqdn) {
 }
 
 // Test that server processes the Hostname option sent by a client and
-// responds with the Hostname option to confirm that the 
+// responds with the Hostname option to confirm that the server has
+// taken responsibility for the update.
 TEST_F(NameDhcpv4SrvTest, serverUpdateHostname) {
     Pkt4Ptr query = generatePktWithHostname(DHCPREQUEST,
                                             "myhost.example.com.");