BIND 10 #2237: Pool/Lease Configuration - IPv4

[BIND 10 Development]

#2237: Pool/Lease Configuration - IPv4
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  stephen
  stephen                            |                Status:  reviewing
                       Type:  task   |             Milestone:  Sprint-
                   Priority:         |  DHCP-20121018
  medium                             |            Resolution:
                  Component:         |             Sensitive:  0
  dhcpconf                           |           Sub-Project:  DHCP
                   Keywords:         |  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 => stephen


Comment:

 Replying to [comment:4 stephen]:
 > Reviewed commit 46c03d26e27417bdce132b90eef18686443bd5a7
 >
 > '''General'''[[BR]]
 > In a lot of calls, "prefix length" is passed as a uint8_t.  Unless there
 is a compelling reason for this, I would have thought that an "unsigned
 int" - using the natural word length of the machine - would be faster.
 In general, I try to use the minimal type that is able to hold maximum
 valid value. In this context, the ranges are 0-32 or 0-128, so both can be
 conveyed in uint8_t. You seem to prefer uint32_t. I've asked in bind10
 chatroom, and got response from vorner slightly preferring unsigned int
 (because internal arithmetics in C/C++ must be done on types at least as
 large as int) and Mark Andrews preferring uint8_t. Vorner later added that
 for consistency reasons it makes sense to stick with uint8_t.

 This gives the result of 2 vs 2, with a slight preference to uint8_t. I
 decided to stick with uint8_t, because:
 - the alternative would mean that we need to do a lot of changed with zero
 usability gains
 - from storage perspective uint8_t is better. Think about million users
 provided with prefixes.
 - using uint8_t for storage and uint32_t for calculations would be ugly
 and is error prone

 If you disagree with those arguments, please submit a new ticket for that
 work. Keep in mind that it affects most of the DHCP code, so it is much
 bigger than just changing parameters in addr_utilities

 > '''src/lib/dhcp/addr_utilities.cc'''[[BR]]
 > The .h file doesn't seem to have been updated with the function
 definitions.
 That's by design. Those new functions are private. The external interface
 is done via generic functions that accept both protocol families. I don't
 want anyone to call v4 and v6 functions directly as they lack sanity
 checks. Those sanity checks are done in generic, public versions.

 Added comments that explain that design.

 > Instead of using "static", put masks and bitMask into the anonymous
 namespace.
 Done. Also renamed bitMask to bitMask6 and masks to bitMask4.
 >
 > firstAddrInPrefix4: require spaces around the operator ">".  Also spaces
 around "-" in "32-len".
 Done.

 > {first,last}AddrInPrefix4: In both cases where "mask" is referenced, it
 is always "mask[32 - len]".  Why not reverse the order of the elements of
 in the initialization of "mask" and just refer to it as "mask[len]"?
 I wrote the array in that order and only later realized how I'm going to
 use it. It doesn't make much sense, so I've reverted the order as your
 suggested.

 > There are no unit tests for the new xxx4 functions.
 Implemented new tests.

 > '''src/lib/dhcp/cfgmgr.h'''[[BR]]
 > There is a definition for removeSubnets4() but not for removeSubnets6().
 That is done as part of #2269 work. I don't want to fix it in this ticket
 to make the merge more complicated than necessary. Added comment 6 in
 #2269.

 > Similarly, there is a getSubnet6() by interface ID, but not a
 getSubnet4() by interface ID.
 Aha! Because there is no interface-id concept in DHCPv4 :)

 > '''src/lib/dhcp/tests/cfgmgr_unittest.cc'''[[BR]]
 > test subnet4: Would add a comment to the test recording (a) adding more
 subnets and checking them and (b) attempting to obtain an address not in
 the subnet.
 >
 > test subnet4: Would also test what happens if you attempt to add a
 subnet that is already present.
 >
 > (Same comments go for the subnet6 test.)
 Such a test would fail. See @todo in CfgMgr::addSubnet4(). Also added
 #2346.

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