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:  tmark
                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 tomek):

 * owner:  tomek => tmark


Comment:

 Replying to [comment:5 tmark]:
 > Review Comments:
 >
 > This is twisty stuff, I'm glad you know what we're doing ;)
 I don't, but a good mix of acting skills and self-confidence does wonders
 ;)

 > ------------------------------------------------------------------------
 > 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.
 Yup, that's confusing. I tried to clarify it a bit, using parts of your
 comments. Let me know if it is better now.

 > dhcpsrv/pool.cc:
 >
 > Per the above, your error message should say "must be longer than prefix
 length"
 Done.

 > dhcpsrv/Allocator.h
 Eeh, no such file :)

 > 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.
 They are similar, but different. I agree that its name was not the best
 one, so I renamed it to pool_type_. There are 4 pool types, but there's
 only 3 lease6 types. If we decide to stick v4 and v6 addresses in one
 table, we may have the same enum. But I think it would be a grave mistake
 - lots of problems for very little gain.

 > 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
 But there's no such thing as V4 lease6.

 > Regarding !PoolType, why did you uses specific values?
 Just for the pleasure of maintaining a tradition of abusing enums. Ok,
 removed.

 > ------------------------------------------------------------------------
 > 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.
 >
 > ...
 >
 > Maybe as a ticket to refactor?
 Added ticket #3168, referencing your comment. I agree that the code is not
 the most elegant stuff.

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

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

 > cppcheck and valgrind on unit tests were clean.
 Thanks for checking.

 Ticket is back with you. Thanks for very quick review.

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