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:
                Type:  enhancement   |  marcin
            Priority:  high          |                       Status:
           Component:  dhcp6         |  reviewing
            Keywords:                |                    Milestone:
           Sensitive:  0             |  Sprint-DHCP-20130918
         Sub-Project:  DHCP          |                   Resolution:
Estimated Difficulty:  0             |                 CVSS Scoring:
         Total Hours:  0             |              Defect Severity:  N/A
                                     |  Feature Depending on Ticket:
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by tomek):

 * owner:  tomek => marcin


Comment:

 Replying to [comment:3 marcin]:
 > 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.
 Done.

 >   * 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.
 Explanation is what kills even the best magic tricks :(
 I had to spend some time with a sheet of paper to write it. But it was on
 a plane, so I had nothing better to do at that time.

 > * 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).
 Yeah, except that there's no method that returns textual representation of
 a pool. Just added one with two simple unit-tests. The exception is now
 more verbose.

 > * pickAddress: There is no need to have pool6 in brackets in this
 statement:
 > {{{
 >         next = increasePrefix(last, (pool6)->getLength());
 > }}}
 Sorry. It was a leftover when I was using dereferenced iterator there.

 > '''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.
 Done.

 > * Both increaseAddress and increasePrefix could be static instead of
 const as they don't modify the state of the class.
 Done.

 > * Sorry for the neat pick but both the brief of each increaseAddress and
 increasePrefix should start with the capital letter.
 There's a lot of other comments that do not start with capital letters.
 Fixed some of those. I'm sorry to ruin your expectations, but I'm afraid
 that we'll have to do a demo with lowercase comments. :P

 > * The prefix_len parameter in the increasePrefix could be declared
 const.
 Done.

 > * increasePrefix documentation: s/increase/increases
 Done.

 > * increaseAddress documentation: it would be useful to have an example
 of the !''increased!'' prefix, just like you did for increasePrefix.
 Added.

 > * Since allocateLease6 returns the collection of leases (as per @return
 tag), shouldn't function name be allocateLeases6 instead?
 Renamed.

 > * createLease6: the prefix_len (just like other parameters) could be
 declared const. The same is true for reuseExpiredLease
 No. Because for leases other than PD prefix_len is overridden to be always
 128. Updated comment to cleary state that.

 > * 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
 Added explanation. For non-PD leases, the value is actually ignored and
 set to 128.

 > '''src/lib/dhcpsrv/libdhcpsrv.dox'''
 > * s/At lease 3 allocators will be implemented/At least 3 allocators will
 be implemented/
 Done:

 > * s/The advantages of this approach are/The advantages of this approach
 are''':'''/
 Done.

 > * 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.
 I didn't get that comment. Prefix Delegation support in AllocEngine *is* a
 separate section.

 > * IMHO, the sentence: !''Prefixes are supported!'' is ambiguous. Please
 consider saying: !''The Allocation Engine supports allocation of the IPv6
 address prefixes!''.
 Fixed.

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

 > '''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.
 Done.

 > Also, it sounds odd that Lease::Type argument identifies pool type but
 there is nothing we can now do about it.
 We've been over this before. Let's not reopen that discussion.

 > * 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.
 Done.

 > * getPools: brief should start with capital letter.
 Done.

 > * getPools (variable variant): IMHO, the non-const variant would be more
 appropriate.
 I don't get this comment. There are const and non-const variants.
 Depending on the case both are used.

 > * 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.
 C++ purity is not a deliverable on the demo. I can add a @todo to the code
 if you like, but it is unlikely that anyone would implement this before
 November.

 > '''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.
 Added.

 > * Is comment in line 149 still relevant? - !''this is IA_NA, not
 IA_PD!''
 It is not. Removed.

 > * checkPrefixIncrease doesn't have a comment, while other functions do
 have.
 Added.

 > * 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
 Capital letters/@brief tag added.

 > * allocWithUsedHintTest function appears to have no description.
 Added.

 > * !IterativeAllocatorPrefixIncrease: should cover prefix length = 1
 which is allowed by the function under test
 Added.

 The ticket is back with you, Marcin. Thanks for reviewing this.

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