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:
                Type:  enhancement   |  stephen
            Priority:  medium        |                       Status:
           Component:  dhcp6         |  reviewing
            Keywords:                |                    Milestone:
           Sensitive:  0             |  Sprint-DHCP-20131016
         Sub-Project:  DHCP          |                   Resolution:
Estimated Difficulty:  0             |                 CVSS Scoring:
         Total Hours:  0             |              Defect Severity:  N/A
                                     |  Feature Depending on Ticket:
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by tomek):

 * owner:  tomek => stephen


Comment:

 Replying to [comment:8 stephen]:
 > Reviewed commit f42fbaf960614930dc0c23591e430411a9417624
 >
 > I've made some minor corrections to a couple of files - please pull
 before continuing.
 Thank you.

 > '''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()".
 Done. Also fixed the same thing in dhcp4. It was very small change, so I
 hope it's ok.

 > 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.)
 Fixed. Also done the same in releaseIA_PD.

 > 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.)
 Fixed.

 > 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.
 The modifications are done in the next couple lines (t1,t2, pref, valid
 and cltt). They are now commented as modifications, so the comment should
 be clear.

 > Dhcp6Srv::renewIA_PD: Indentation of second line of the prefix creation
 statement should be indented by the standard indent size (4 characters).
 Done. Let me just repeat my previous opinion that keeping 80 columns limit
 does not mix well with such long C++ names. The code is overly wrapped,
 which makes it difficult to read.

 On a related note, one of IESG members recently asked to keep doc write-
 ups to have reasonable length. As a result for my request to define
 "reasonable length" and whether 80 would be ok, very interesting 50+ posts
 discussion unfolded, with people talking about punch cards readers, quirks
 of COBOL and old FORTRAN processing rules. One poster even shared an
 amusing story. A long time ago he was using columns 73-80 for sequence
 numbers, exactly as the yet unpublished book his brother was writing
 recommended. The brother commended on this clever use and unceremonially
 dropped the whole stack on the floor. That brother was Steve Crooker,
 author of RFC1.

 > 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.
 Done.

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

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

 > 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()).
 It is. See line 1769. I added a comment there. The important thing here is
 to understand that we can send status code success only if every IA
 succeeds. If any of them fails, the overall status code should not be
 success.

 > Dhcp6Srv::releaseIA_PD: s/You did not include address/You did not
 include an address/
 No. That should actually be "You did not include a prefix" :) Fixed.

 > 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.
 Updated.

 > 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).
 I think I caught them all. Please let me know if I missed any.

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

 > 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.
 Refactored.

 > '''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".
 Updated.

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

 > 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.
 Moved and commented.

 Thank you for your review. Ticket is back with you.

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