BIND 10 #3146: PD: Abstract LeaseMgr support for Lease6 type PD

[BIND 10 Development]

#3146: PD: Abstract LeaseMgr support for Lease6 type PD
-------------------------------------+-------------------------------------
            Reporter:  tomek         |                        Owner:  tomek
                Type:  enhancement   |                       Status:
            Priority:  medium        |  reviewing
           Component:  dhcp6         |                    Milestone:
            Keywords:                |  Sprint-DHCP-20130918
           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 tmark):

 * owner:  tmark => tomek


Comment:

 Review Comments:

 General:

 If no two leases may have the same address, regardless of type, why do we
 pass in lease type on getLease6 methods that formerly only required an
 address?

 From a back end perspective, it would be inefficient to filter by lease
 type
 address is guaranteed to be unique, so at it would likely be ignored.

 ----------------------------------------------------------------------

 General:

 Not to be picky, but the v4 methods which return collections are not
 "getLeases4".  I think if you are going to make a semantic change in the
 names like this, you should do it for both v4 and v6, or not do it.
 Personally, I like the change, in fact it is almost too subtle,
 getLease6Collection would be even better.  Maybe open a ticket to rename
 the appropriate lease4 methods in similar fashion?

 ----------------------------------------------------------------------

 General unit tests:

 Tests such as TEST_F(!MySqlLeaseMgrTest, getLease6DuidIaid) are going to
 fail
 once the back ends implement filtering by type.   I think we need @todo's
 on
 these.

 ---------------------------------------------------------------------------

 /dhcpsrv/tests/test_utils.h

 Did you assigned specific values to !LeaseType enums solely for unit
 testing?  I see
 that !GenericLeaseMgrTest's constructor relies on the values being 0,1,2
 with the "i%3" trick.  This seems like an abuse of enums that you yourself
 would normally complain about it
 and is it even portable?  It also makes it less than obvious which lease
 type is in which test
 lease.

 All this really bought you was a cute for loop in !GenericLeaseMgrTest's
 constructor while having assigned them specific values conveys a false
 sense of purpose.  It makes it seem as though the precise value is
 required perhaps by the an RFC or some such.  Plus your tests fall over if
 anyone changes them to say 0, 2, 4.

 Unless there is some reason beyond unit testing for these values, why not
 just change leasetype6_ to be a static const member (or even a global)
 like this:

 {{{
     static const Lease6::LeaseType leasetype6_[] =
         { Lease6::LEASE_IA_NA, Lease6::LEASE_IA_TA, ... };

 }}}

 and initialize them at the top right after ADDRESS6.  This way you don't
 need or care about specific numeric values.

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