BIND 10 #3153: PD: Prefix renewal in DHCPv6 server (renew, rebind)

[BIND 10 Development]

#3153: PD: Prefix renewal in DHCPv6 server (renew, rebind)
-------------------------------------+-------------------------------------
            Reporter:  tomek         |                        Owner:  tomek
                Type:  enhancement   |                       Status:
            Priority:  medium        |  reviewing
           Component:  dhcp6         |                    Milestone:
            Keywords:                |  Sprint-DHCP-20131016
           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 stephen):

 * owner:  stephen => tomek


Comment:

 Reviewed commit f42fbaf960614930dc0c23591e430411a9417624

 I've made some minor corrections to a couple of files - please pull before
 continuing.


 '''src/bin/dhcp6/dhcp6_messages.mes'''[[BR]]
 Minor changes to wording made.

 '''src/bin/dhcp6/dhcp6_srv.cc'''[[BR]]
 General: Instances of "HooksManager::getHooksManager().calloutsPresent()"
 can be replaced with the shorter "HooksManager::calloutsPresent()".

 Dhcp6Srv::renewIA_PD: All paths through the code involve creating ia_rsp
 with the same arguments. Instead of three separate creation statements,
 this could be created once at the head of the method. (Incidentally, the
 comment associated with these statements is wrong - an empty IA_PD option
 is being created.)

 Dhcp6Srv::renewIA_PD: In the "if" branch dealing with the absence of a
 lease, the comment is wrong - a "!NoBinding" status is inserted in the
 status return option. (Also, the first comment in this block should start
 with a capital letter.)

 Dhcp6Srv::renewIA_PD: The comment in the code tells us that a copy of the
 lease is kept; but what modifications are being made to the active lease
 data in the next few statements?  A comment here would be helpful.

 Dhcp6Srv::renewIA_PD: Indentation of second line of the prefix creation
 statement should be indented by the standard indent size (4 characters).

 Dhcp6Srv::renewIA_PD: To avoid a redundant "if", I suggest that in the
 "if" block testing for the callout, "skip" is set as:
 {{{
 skip = callout_handle->getSkip();
 }}}
 ... and the LOG_DEBUG call put into the appropriate section of the
 following "if" block that checks the state of the "skip" flag.

 Dhcp6Srv::renewLeases: Add blank line above "case D6O_IA_PD" to match the
 one above the "default" statement.

 Dhcp6Srv::releaseIA_PD: Comment: when defining release_prefix, I would be
 inclined to break the line after the "=" sign.

 Dhcp6Srv::releaseIA_PD: Every exit path sets "general_status" except for
 the success exit path.  I note that the header says that the status may be
 updated in case of errors: but this requires that it be set correctly on
 entry to the function, which may be a source of error. (The same applies
 to releaseIA_NA()).

 Dhcp6Srv::releaseIA_PD: s/You did not include address/You did not include
 an address/

 Dhcp6Srv::releaseIA_PD: "client" shold start with a capital letter in the
 "if (!lease)" block.  Also comment about the status code is wrong at this
 point - it is a "!NoBinding" status.

 Dhcp6Srv::releaseIA_PD: Should have consistency as to whether a blank line
 immediately follows an "if" statement - or the comment after the "if"
 statment (three consecutive "if" blocks in this functions have different
 spacing).

 Dhcp6Srv::releaseIA_PD: Same comment as before regarding the setting of
 "skip".

 Dhcp6Srv::releaseIA_PD: In the final "if" statement in the method, both
 branches return ia_rsp.  That common statement can be taken out and put
 after the "if" block.

 '''src/bin/dhcp6/tests/dhcp6_test_utils.cc'''[[BR]]
 testRenewReject and testReleaseReject: The comments "Check that IA_?? was
 returned and there's an address included" is wrong, as the following check
 is that the status code is STATUS_NoBinding (this occurs several times).
 The comment should read "there's no address included".

 '''src/bin/dhcp6/tests/dhcp6_test_utils.h'''
 I made some minor changes to spacing and comments.

 Suggest placing the Dhcp6SrvTest destructor immediately after the
 constructor (and adding a header to both it and the constructor).  At the
 moment it's hidden between two method definitions in the middle of the
 file.

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