BIND 10 #3150: PD: CfgMgr must be able to store info about Prefix Delegation

[BIND 10 Development]

#3150: PD: CfgMgr must be able to store info about Prefix Delegation
-------------------------------------+-------------------------------------
            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:

 This is twisty stuff, I'm glad you know what we're doing ;)

 ------------------------------------------------------------------------
 dhcpsrv/pool.h

 In your commentary at line 187:

 {{{
     /// Obviously, prefix_len must define bigger prefix than
 delegated_len,
     /// so prefix_len < delegated_len. Note that it is slightly confusing:
     /// bigger (larger) prefix actually has smaller prefix length, e.g.
     /// /56 is a bigger prefix than /64.
     ///
 }}}

 "prefix_len must define bigger prefix", isn't quite true.  It must specify
 a SHORTER prefix.

 I think if you alter your choice of words a bit and discuss the "size" of
 prefixes as shorter or longer, while the lengths (_len values) are smaller
 or larger, it's a bit easier to follow.

 A smaller length yields a shorter prefix which describes a larger set of
 addresses.
 A larger length yields a longer prefix which describes a smaller set of
 addresses.

 A numerically larger _len value describes a longer prefix, which specifies
 more of the actual
 address.  The delegated length must yield a LONGER prefix the prefix
 length and therefore it's length value must be larger.

 There's nothing really wrong here, I just think if you adopt the above
 semantics it makes the discussion clearer.

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

 dhcpsrv/pool.cc:

 Per the above, your error message should say "must be longer than prefix
 length"

 line 115:
 {{{
     if (prefix_len > delegated_len) {
         isc_throw(BadValue, "Delegated length (" <<
 static_cast<int>(delegated_len)
                   << ") must be smaller than prefix length ("
                   << static_cast<int>(prefix_len) << ")");
     }
 }}}

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

 dhcpsrv/Allocator.h

 If you are going to use a new enumeration, !PoolType, for this then you
 should rename this
 instance member to pool_type_.  There is already Lease6::LeaseType and
 these values are not the quite same.

 {{{
         /// @brief defines lease type allocation
         Pool::PoolType lease_type_;
 }}}

 Perhaps we shouldn't have two sets of enums at all. I think it would be a
 cleaner, more  universal approach to extract !LeaseType out of Lease6 and
 add a value of LEASE_V4, and later we can add IA_PA etc.  I think we are
 confusing matters with having two sets of enums, that are this closely
 related, especially since you were inclined to name the member above
 "lease_type_".

 Regarding !PoolType, why did you uses specific values?

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

 dhcpsrv/subnet.h/cc

 Using a explicit instance members for last allocated address and pool
 collection per type seems a bit awkward.  For one thing the base class,
 Subnet, now has members that apply only to Subnet6.  It also means the
 same essential switch statement is appearing often.

 What if you were to replace:

 {{{
 typedef std::vector<PoolPtr> PoolCollection with a class:
 }}}

 with a class, something like this:

 {{{
 class PoolCollection {
     LeaseType/PoolType type_
     IOAddress last_allocated_addr_
     std::vector<PoolPtr> pools_

     void push_back(PoolPtr )  // wraps vector<>.push_back
     operator[]                // wraps vector [] operator
     }
 }
 }}}

 You could then add a map, to Subnet something like this:

 {{{
     std::map<PoolType, PoolCollection> pools_map_;
 }}}

 Implement an accessor to return the appropriate !PoolCollection instance
 from the map based on type or throw if not found.

 Derivations (Subnet4, Subnet6), then only add collections of supported
 types
 to their pools_map_.

 I'm not sure how the Hashed and Random allocators will work, but they
 might be
 able to leverage this as well, in the event they need additional stateful
 values
 per specific pooltype collection.

 Maybe as a ticket to refactor?

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

 subnet_unittest.cc

 In TEST(Subnet4Test, PoolType),  your calls to addPool() for the valid V4
 pools
 should be wrapped in EXPECT_NO_THROWs.

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

 subnet_unittest.cc

 In TEST(Subnet6Test, PoolType),  your calls to addPool() for the valid V6
 pools
 should be wrapped in EXPECT_NO_THROWs.

 ------------------------------------------------------------------------
 cppcheck and valgrind on unit tests were clean.

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