BIND 10 #3171: PD: AllocationEngine support for Prefix Delegation, pt.2

[BIND 10 Development]

#3171: PD: AllocationEngine support for Prefix Delegation, pt.2
-------------------------------------+-------------------------------------
            Reporter:  tomek         |                        Owner:  tomek
                Type:  enhancement   |                       Status:
            Priority:  high          |  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 marcin):

 * owner:  marcin => tomek


Comment:

 Reviewed commit e09660860573d1b76c65a200d3682d3592406b67

 Please propose a !ChangeLog entry for this ticket.

 '''src/lib/dhcpsrv/alloc_engine.cc'''
 * increasePrefix:
   * Suggest that the error string !''Prefix operations are for IPv6
 only!'' is extended to include the string representation of the prefix
 passed to this function. It would be useful for the debugging purposes.
   * I was hoping that Mr Bean would explain what is going on between line
 104 and 106 but apparently he doesn't know DHCP very well. So, what
 exactly is going on there? :D I had to spend a while writting the
 variables state on the sheet of paper to understand how this code works.

 * pickAddress: If the pool found, doesn't cast to the Pool6 type, you
 should rather throw an Unexpected exception, not !InvalidParameter to
 indicate that there is something !''gravely wrong!''. Also, the exception
 string could indclude the pool (iterator) in the textual format (for
 debugging purposes).

 * pickAddress: There is no need to have pool6 in brackets in this
 statement:
 {{{
         next = increasePrefix(last, (pool6)->getLength());
 }}}

 '''src/lib/dhcpsrv/alloc_engine.h'''
 * pickAddress: Commentary should be exteded to indicate that not only does
 this function pick an IPv6 address but also IPv6 prefix which is not
 obvious.
 * Both increaseAddress and increasePrefix could be static instead of const
 as they don't modify the state of the class.
 * Sorry for the neat pick but both the brief of each increaseAddress and
 increasePrefix should start with the capital letter.
 * The prefix_len parameter in the increasePrefix could be declared const.
 * increasePrefix documentation: s/increase/increases
 * increaseAddress documentation: it would be useful to have an example of
 the !''increased!'' prefix, just like you did for increasePrefix.
 * Since allocateLease6 returns the collection of leases (as per @return
 tag), shouldn't function name be allocateLeases6 instead?
 * createLease6: the prefix_len (just like other parameters) could be
 declared const. The same is true for reuseExpiredLease
 * createLease6: since prefix_len is for PD only it should be mentioned
 what value it should be assigned when creating a lease for IA_NA. Or, is
 it simply ignored? The same is the case for reuseExpiredLease

 '''src/lib/dhcpsrv/libdhcpsrv.dox'''
 * s/At lease 3 allocators will be implemented/At least 3 allocators will
 be implemented/
 * s/The advantages of this approach are/The advantages of this approach
 are''':'''/
 * I would suggest that the first paragraph of the new section: !''Prefix
 Delegation support in Allocation Engine!'' is moved to a separate section.
 I would just leave the second section which is strictly about the PD
 support which this section should be about.
 * IMHO, the sentence: !''Prefixes are supported!'' is ambiguous. Please
 consider saying: !''The Allocation Engine supports allocation of the IPv6
 address prefixes!''.


 '''src/lib/dhcpsrv/subnet.cc'''
 * getPools: It may be worth to static_cast the type when returning
 exception string in the const variant too.

 '''src/lib/dhcpsrv/subnet.h'''
 * inPool documentation: perhaps it would be better to say: type of pools
 to iterate over. Otherwise it sounds like you're passing multiple lease
 types, not just one. Also, it sounds odd that Lease::Type argument
 identifies pool type but there is nothing we can now do about it.
 * getPool: Apparently the addr parameter could be passed by const
 reference. Also, assuming that inRange can be made const, the getPool
 could be made const too.
 * getPools: brief should start with capital letter.
 * getPools (variable variant): IMHO, the non-const variant would be more
 appropriate.
 * The non-const variant of getPools is used internally by the Subnet
 class. In this case, there is no need to make it public. Especially, that
 it leads to the situation that the class members may be modified
 externally when returned via non-const variant of the function. However,
 if you make it private you will end-up having one overload of getPools
 private and another one public which will cause the compilation to fail
 for a different reason: compiler will complain that the getPools is
 private even though you want to access the public (const) variant. I
 believe, that the reasonable way to overcome this problem is to give the
 private function a different name: say getPoolsWritable or something like
 that and use it internally within Subnet class.

 '''src/lib/dhcpsrv/tests/alloc_engine_unittest.cc'''
 * !NakedIterativeAllocator: This class appears to be the only one which
 doesn't have any commentary. Please consider adding some brief description
 of its purpose.
 * Is comment in line 149 still relevant? - !''this is IA_NA, not IA_PD!''
 * checkPrefixIncrease doesn't have a comment, while other functions do
 have.
 * simpleAlloc6Test: you may have meant to add @brief at the beginning of
 the function description. You may have meant to start the description with
 the capital letter :D
 * allocWithUsedHintTest function appears to have no description.
 * !IterativeAllocatorPrefixIncrease: should cover prefix length = 1 which
 is allowed by the function under test

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