BIND 10 #2326: Allocation engine v6: release

[BIND 10 Development]

#2326: Allocation engine v6: release
-------------------------------------+-------------------------------------
            Reporter:  tomek         |                        Owner:  tomek
                Type:  task          |                       Status:
            Priority:  medium        |  reviewing
           Component:  dhcp6         |                    Milestone:
            Keywords:                |  Sprint-DHCP-20130103
           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 6ce0831748423ab48611785f29ee999beeaf8322

 '''src/bin/dhcp6/dhcp6_messages.mes'''[[BR]]
 I've made some changes here and pushed the result.  However:

 DHCP6_DB_ERROR_LEASE6_WITHOUT_DUID: if the workaround is to remove the
 lease entry from the database, why does the software not do that
 regardless?

 Also:
 * As this error is most likely not a problem with the database (but with
 the code using the database), I suggest taking out the "DB" from the
 symbolic name.
 * I also suggest removing "ERROR" from the name as well (no other messages
 indicate the severity in the message name).
 * I suggest removing "6" from "LEASE6". (It must be a Lease6 since the
 origin of the message is DHCP6: also, DHCP6_LEASE_ALLOC_FAIL uses "LEASE"
 and not "LEASE6".)

 In summary, the name "DHCP6_LEASE_WITHOUT_DUID" is suggested.


 '''src/bin/dhcp6/dhcp6_srv.cc'''[[BR]]
 Line 715: "This should not happen. We have checked this before."  This
 seems to indicate a software failure and I would suggest that a message be
 logged at this point (with at least WARNING severity).

 Line 735: the "default" clause does not need a "break" - it could just
 drop through. (Leave the "default" clause there though with a comment to
 note that the remaining cases have been considered.)

 Line 780: suggest that this be a DEBUG message - a malicious client could
 trigger a flood of warning messages by sending release messages for random
 addresses. (It is assumed that by default, WARNING messages are enabled
 and DEBUG messages disabled.)

 Line 828: correction - s/necessary if/necessary to check if/

 Line 833: looking at the code, a failure here is not a database error (as
 indicated by the message for DHCP6_RELEASE_FAIL).  A database error would
 trigger an exception: deleteLease() returning false indicates that there
 was no lease with the given address in the database.  As getLease6 has
 established that the address is present, the implication is that something
 else has deleted the lease in the meantime.



 '''src/bin/dhcp6/dhcp6_srv.h'''[[BR]]
 Header for releaseIA_NA does not quite parse.  How about:
 {{{
 As RFC 3315 requires that a single status code be sent for the whole
 message,
 this method may update the passed general_status: it is set to SUCCESS
 when
 message processing begins, but may be updated to some error code if the
 release process fails.
 }}}

 '''src/bin/dhcp6/tests/dhcp6_srv_unittest.cc'''[[BR]]
 !Dhcpv6SrvTest(s): instead of using a scope_ptr, why not declare
 !NamedDhcpv6Srv as an automatic variable?

 Line 1245: "check that the packets originating from local addresses can
 be" what?

 !ReleaseReject: the last statement before case 1 adds a server ID to the
 renew packet with the comment that it is mandatory in a RENEW.  Is there a
 test for the case where it is absent?

 '''!ChangeLog'''[[BR]]
 Proposed entry is OK.

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