BIND 10 #3241: Implement NameAddTransaction's DNS update build methods

[BIND 10 Development]

#3241: Implement NameAddTransaction's DNS update build methods
-------------------------------------+-------------------------------------
            Reporter:  tmark         |                        Owner:  tmark
                Type:  task          |                       Status:
            Priority:  medium        |  reviewing
           Component:  dhcp-ddns     |                    Milestone:  DHCP-
            Keywords:  Task# 4.2.1   |  Kea1.0-alpha
           Sensitive:  0             |                   Resolution:
         Sub-Project:  DHCP          |                 CVSS Scoring:
Estimated Difficulty:  0             |              Defect Severity:  N/A
         Total Hours:  0             |  Feature Depending on Ticket:
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by marcin):

 * owner:  marcin => tmark


Comment:

 Reviewed commit 5414d2bdabfc5d440848014901c7744979a92486

 This work required a bit of reading RFCs and understanding them. Well
 done. A few rather minor comments....

 '''src/bin/d2/d2_messages.mes'''
 General comment to all messages: It may be reasonable to add a comma
 before word !''reason!'' to separate the request name from the failure
 reason. So for example:
 {{{
 +% DHCP_DDNS_FORWARD_ADD_BUILD_FAILURE A DNS udpate message to add a
 forward DNS entry could not be constructed for this request: %1, reason:
 %2
 }}}

 DHCP_DDNS_ADD_FAILED DHCP_DDNS: Typo !''There precise...!''. Shouldn't it
 be !''The precise..!''?


 '''src/bin/d2/d2_update_message.cc'''

 This line:
 {{{
         message_.setRcode(Rcode(Rcode::NOERROR_CODE));
 }}}
 could be replaced with:
 {{{
         message_.setRcode(Rcode::NOERROR());
 }}}

 '''src/bin/d2/nc_add.cc'''
 addingFwdAddrsHandler: What is it going to happen if the
 buildAddFwdAddressRequest throws an exception? In such case the error is
 logged, the state model is transitioned to the failed state. But, in line
 190 you call sendUpdate. Should sendUpdate be called if building ADD
 request fails? At least the appropriate commentary should be added to
 describe intended behavior of this code.

 The same comment applies to replacingFwdAddrsHandler() and
 replacingRevPtrsHandler().

 '''src/bin/d2/tests/nc_add_unittests.cc'''
 fakeResponse: Typo !''exercise!'' instead of !''exercised!''.

 addingFwrdAddrsHandler_sendUpdateException: Consider ASSERT here:
 {{{
     EXPECT_NO_THROW(name_add->addingFwdAddrsHandler());
 }}}

 '''src/bin/d2/tests/nc_trans_unittests.cc'''

 prepNewRequest: typo in todo: !DdsnDomain.

 '''src/lib/dhcp_ddns/ncr_msg.cc'''

 setIpAddress: If you keep the IP address in the IOAddress object called
 ip_io_address_, I don't see a reason why you should duplicate the IP
 address in the string object: ip_address_. You can always return string
 version of the IP address by calling .toText(). I don't think there is any
 serious overhead if you do it.

 Besides, the fact that you keep the same value in two distinct object is
 already making the code vulnerable to inconsistency. Consider the case
 when I call setIpAddress with an invalid address. The ip_address_ (string)
 object will happily accept the new value. The attempt to assign the bogus
 value to the ip_io_address_ object will result in exception and the new
 value will not be assigned. Your object ends up having two different IP
 address values being held in string and IOAddress objects.

 BTW, your test could have checked that.

 IMHO, the setters should be consistent and you could validate fqdn value
 in setFqdn, as you do it in setIpAddress. The same applies to setDhcid.

 '''src/lib/dhcp_ddns/ncr_msg.h'''

 isV4 and isV6 should be const.

 '''src/lib/dhcp_ddns/tests/ncr_unittests.cc'''

 Did you intend not to put any unit tests for isV4 and isV6?

 ipAddresses:
 {{{
     // Verify that an valid address fails.
     ASSERT_THROW(ncr.setIpAddress("x001:1::f3"),NcrMessageError);
 }}}

 It should be !''an invalid!''. Also, there should be a space before the
 exception name.

-- 
Ticket URL: <http://bind10.isc.org/ticket/3241#comment:7>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development