BIND 10 #2325: Allocation engine v6: renewal

[BIND 10 Development]

#2325: Allocation engine v6: renewal
-------------------------------------+-------------------------------------
            Reporter:  tomek         |                        Owner:  tomek
                Type:  task          |                       Status:
            Priority:  medium        |  reviewing
           Component:  dhcp6         |                    Milestone:
            Keywords:                |  Sprint-DHCP-20121213
           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 d6fb00a3198fe327cdbf4ebfe14f47eb23086325

 '''src/bin/dhcp6/dhcp6_messages.mes'''[[BR]]
 The text of DHCP6_REQUIRE_OPTIONS_CHECK_FAIL might be better phrased as:
 {{{
 This message indicates that received DHCPv6 packet is invalid.  This may
 be due
 to a number of reasons, e.g. the mandatory client-id option is missing,
 the server-id forbidden in that particular type of message is present,
 there is more than one instance of client-id or server-id present,
 etc. The exact reason for rejecting the packet is included in the message.
 }}}

 '''src/bin/dhcp6/dhcp6_srv.h'''[[BR]]
 renewIA_NA(): typo s/IA_NA response is generates with/an IA_NA response is
 generated with an/

 "Assigns leases" header.  Is the restriction that only IA_NA leases are
 supported permanent or temporary?  If the latter, please add a "@todo".

 '''src/bin/dhcp6/dhcp6_srv.cc'''[[BR]]
 assignLeases()/renewLeases() (comment for "if (!subnet)"): if the problem
 is a misconfiguration of something under the control of the server
 administrator, I would suggest that it be logged as an error instead of as
 a debug message.

 Throwing an RFC violation - suggest that a debug message is logged before
 the "throw".

 renewIA_NA: if a client is trying to renew a lease we know nothing about,
 that might be worth a debug message being logged.



 '''src/bin/dhcp6/tests/dhcp6_srv_unittest.cc'''[[BR]]
 !RenewBasic test: can "addr" be made "const"?

 !RenewBasic test: comment looks unfinished: "Check that T1, T2, preferred,
 valid really".

 !RenewBasic test: In the part that starts "Let's create a RENEW", it's not
 clear why the ASSERT_TRUE is present at that point.  As subnet_ is a test
 class member, and addr is not altered in the test, presumably addr was
 created as an address in the subnet.  The logical place to put the check
 is therefore immediately after the creation of addr.

 !RenewBasic test: Please start one-line comments with a capital letter.

 !RenewBasic test: "check that lease is really in the database".  That
 should be an ASSERT_TRUE check: if the lease is not in the database, the
 following EXPECT_EQ checks will attempt to dereference a null pointer.

 !RenewReject test: the comment "check that the lease is really in the
 database" is wrong - the check is that the lease is not in the database.

 !RenewReject test: see comment above about the ASSERT_TRUE check for the
 address.

 '''src/lib/dhcp/tests/option6_ia_unittest.cc'''[[BR]]
 "simple" test: s/Option6IA * ia/Option6IA* ia/

 "simple" test: Should check that T1 and T2 are not the values set by
 setT1/setT2 before calling these methods. As it is, the test would pass if
 setT1/setT2 did nothing, but t1 and t2 were initialized to 2345 and 3456.

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