BIND 10 #3222: DHCP6 should send generated NameChangeRequests to b10-dhcp-ddns

[BIND 10 Development]

#3222: DHCP6 should send generated NameChangeRequests to b10-dhcp-ddns
-------------------------------------+-------------------------------------
            Reporter:  tmark         |                        Owner:
                Type:  enhancement   |  marcin
            Priority:  medium        |                       Status:
           Component:  dhcp-ddns     |  reviewing
            Keywords:                |                    Milestone:  DHCP-
           Sensitive:  0             |  Kea0.9-alpha
         Sub-Project:  DHCP          |                   Resolution:
Estimated Difficulty:  32            |                 CVSS Scoring:
         Total Hours:  13            |              Defect Severity:  N/A
                                     |  Feature Depending on Ticket:
                                     |          Add Hours to Ticket:  3
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by tmark):

 * hours:  10 => 3
 * owner:  tmark => marcin
 * totalhours:  10 => 13


Comment:

 Replying to [comment:6 marcin]:
 > Reviewed commit 50f472025f22aa3afab6a699de50ff8ccfa8022c
 >
 > I fixed a few nits in the code and fixed it. Please pull these changes
 from the branch.
 >
 Ok, got them.  Thanks for fixing them.

 > '''src/bin/d2/nc_trans.h'''
 > returnString could be const.
 >
 > Also, there is no unit test for this function. The function is simple so
 I am not fully convinced that it is needed, but perhaps you just forgot to
 add it? ;)
 >

 I believe you mean "responseString", and actually I didn't add a unit test
 method for it but probably should have. I have since added one. It is also
 now const.

 You will also notice that I added a new, similiar method
 "transactionOutcomeString".  It has a unit test, and it is declared const.
 While conducting other
 testing I found the logging not quite clear so I improved it a bit.  This
 was
 not part of the originally reviewed material but this ticket is a good
 place
 to add it.

 Along with this I altered a few log messages too.


 > '''src/bin/dhcp6/dhcp6_srv.cc'''
 > In line 1077 you create an empty pointer and then reset it to the actual
 pointer to NCR. Why? You could just pass the raw pointer to the
 constructor to avoid double initialization.
 >
 > I wonder if it wouldn't be more logical if the following debug message:
 > {{{
 >         LOG_DEBUG(dhcp6_logger, DBG_DHCP6_DETAIL,
 >
 DHCP6_DDNS_CREATE_ADD_NAME_CHANGE_REQUEST).arg(ncr->toText());
 >
 > }}}
 >
 > was moved above this:
 > {{{
 >         // Post the NCR to the D2ClientMgr.
 >         CfgMgr::instance().getD2ClientMgr().sendRequest(ncr);
 > }}}
 >
 > If the sendRequest experiences an error, it will log the error message
 and return. Next, the debug message will be displayed informing that we
 have been sending the NCR. If you swap the two as I propose, first you'll
 get the debug message saying that we are sending NCR and if it fails, the
 error message, which makes more sense for me.
 >

 Agreed, done.

 > The same applies to createRemovalNameChangeRequest
 >

 Agreed, done.


 Also amended hours spent to include Marcin's initial 2 hours review time.

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