BIND 10 #2324: Allocation engine v6: address assignment

[BIND 10 Development]

#2324: Allocation engine v6: address assignment
-------------------------------------+-------------------------------------
                   Reporter:  tomek  |                 Owner:  tomek
                       Type:  task   |                Status:  reviewing
                   Priority:         |             Milestone:  Sprint-
  medium                             |  DHCP-20121101
                  Component:  dhcp6  |            Resolution:
                   Keywords:         |             Sensitive:  0
            Defect Severity:  N/A    |           Sub-Project:  DHCP
Feature Depending on Ticket:         |  Estimated Difficulty:  0
        Add Hours to Ticket:  0      |           Total Hours:  0
                  Internal?:  0      |
-------------------------------------+-------------------------------------
Changes (by marcin):

 * owner:  marcin => tomek


Comment:

 Reviewed commit cb330a06847987bfacacc7a9333b70666750b66d


 ----

 '''alloc_engine.h'''
 There is no need to
 {{{
 #include <iostream>
 }}}

 '''AllocFailed class constructor''': lack of indentation.

 '''AllocEngine::Allocator''': the '''pickAddress''' declaration seems to
 have wrong indentation. Shouldn't it be aligned with the '''virtual'''
 keyword above?

 '''AllocEngine::Alocator''': there is no need to provide protected
 constructor because '''Allocator''' class is abstract so instance of it
 can't be created anyway (and should be detected in compile time).

 '''AllocEngine::IterativeAllocator''': Suggest that you mention in doxygen
 documentation what happens if the iterative allocator algorithm comes to
 an end of the pool. Would that start allocating from the first available
 address?

 '''AllocEngine::allocAddress6''': I don't understand the description of
 '''fake''' parameter. In particular it would useful to have true and false
 values mapping to the REQUEST  or SOLICIT. Also '''fake''' is a little
 misleading. Maybe something like "fake_allocation" is better?

 '''AllocEngine::allocator_''': I think it is better if you make this
 member a '''boost::shared_ptr<Allocator>''' as it is generally safer and
 more convenient:
 * You would not have to delete object in the destructor
 * if not explicitly initialized in the constructor, the allocator_ member
 may have some garbage value and testing it against NULL will not reveal
 the fact that it hasn't been initialized (see
 '''AllocEngine::allocateAddress6'''):
 {{{
 if (!allocator_) {
     isc_throw(InvalidOperation, "No allocator selected");
 }
 }}}
 eventually program will go segmentation fault.
 I admit that this code is properly working as long you create an object in
 the constructor and destroy it in the destructor and don't create any
 additional pointers in between. So, if you don't want to change this to
 smart pointer you may simply want to remove the check given above because
 it does not make sense at all.

 Last line of the file:
 {{{
 #endif // DHCP6_SRV_H
 }}}
 seems to be copied over from another file and should be changed to:
 {{{
 #endif // ALLOC_ENGINE_H
 }}}

 ----

 '''alloc_engine.cc'''

 It is not necessary to include <io_address.h> as it is included in
 '''alloc_engine.h'''

 '''AllocEngine::IterativeAllocator::increaseAddress''': It does not make
 much difference here but it is generally faster to use '''++packed[i]'''
 instead of '''packed[i]++'''.

 '''AllocEngine::IterativeAllocator::pickAddress''': There is an unused
 variable declared:
 {{{
 Pool6Ptr pool = Pool6Ptr();
 }}}

 '''AllocEngine::allocateAddress6''': The check for '''allocator_''' is not
 neccessary because it will not detect that the variable is not initialized
 even if it isn't. It could be removed.

 ----

 '''lease_mgr.cc'''
 '''Lease6::Lease6''': The following:
 {{{
 if (duid == DuidPtr()) {
 ...
 }
 }}}
 could be replaced with
 {{{
 if (!duid) {
 }
 }}}
 That way you would avoid creating another object with NULL pointer.

 ----

 '''lease_mgr.h'''
 '''instance''': in doxygen comment you instruct that '''instantiate()'''
 method should be used to create the instance of the class before you can
 call '''instance()''' but the '''instantiate()''' method is nowhere
 around. Rather than this I can see constructor() creating the instance.

 ----

 '''subnet.h'''
 You added new virtual function: '''inPool()'''. Having at least one
 virtual function in a class implies that you have to define virtual
 destructor for this class too.

 The following functions:
 * getLastAllocated
 * setLastAllocated
 * getID

 have no doxygen documentation. Also missing curly braces in '''return'''
 statement in '''getID'''.

 '''getLastAllocated and getID could be declared '''const'''.

 '''inPool doxygen documentation''': The example address '''2001::abc'''
 causes doxygen warnings as it probably tries to reference the abc as a
 member of a class. Admittedly I could not find any doxygen tag to walk
 around it. Maybe turning abc to some valid IPv6 will work.

 ----

 '''subnet.cc'''
 It is not necessary to include <pool.h> as it is already included from
 '''subnet.h'''

 ----

 '''lease_mgr_unittest.cc'''
 Why do you use '''EXPECT_TRUE''' when comparing two values like this:
 {{{
 EXPECT_TRUE(x->t1_ == 50);
 }}}
 ?
 how about
 {{{
 EXPECT_EQ(50, x->t1_)
 }}}
 ?
 This has an advantage over EXPECT_TRUE that it will show the actual value
 of '''x->t1_''' if  check failed.

 ----

 '''memfile_lease_mgr.h'''
 '''addLease''': The Lease4Ptr and Lease6Ptr could be passed by const
 reference.

 '''getLease4X''': IOAddress could be passed as const reference.

 The same applies to other functions within the class.

 ----

 '''memfile_lease_mgr.cc'''
 The getLeaseX look like stub functions which is contrary to their doxygen
 descriptions in header file. Shouldn't we mention in the documentation
 that these functions currently return NULL pointers because IPv6 is of our
 interest for now or add some TODO comments there?.

 The same applies to other functions that do nothing.

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