BIND 10 #2324: Allocation engine v6: address assignment

[BIND 10 Development]

#2324: Allocation engine v6: address assignment
-------------------------------------+-------------------------------------
                   Reporter:  tomek  |                 Owner:  marcin
                       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 tomek):

 * owner:  tomek => marcin


Comment:

 Replying to [comment:4 marcin]:
 > '''alloc_engine.h'''
 > There is no need to
 > {{{
 > #include <iostream>
 > }}}
 Removed.

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

 > '''AllocEngine::Allocator''': the '''pickAddress''' declaration seems to
 have wrong indentation. Shouldn't it be aligned with the '''virtual'''
 keyword above?
 Emacs tries to be too smart sometimes. Fixed.

 > '''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).
 Removed.

 > '''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?
 It loops over and starts from the beginning. Comment clarified.

 > '''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?
 Renamed to fake_allocation and clarified comment.

 > '''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.
 Changed to shared_ptr. Although there is currently no way to create
 AllocEngine without creating Allocator, I decided to keep the check in
 place just in case.

 > 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
 > }}}
 Fixed.

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

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

 > '''AllocEngine::IterativeAllocator::pickAddress''': There is an unused
 variable declared:
 > {{{
 > Pool6Ptr pool = Pool6Ptr();
 > }}}
 Removed. I'm wondering why my compiler didn't warn/erred about it. Odd.
 >
 > '''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.
 Agree that it did't make much sense. However, with the shared_ptr being
 used now, it will prevent from using NULL pointer. Although the current
 code does not allow it, it is possible that some allocator_ manipulation
 methods will be implemented.

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

 > '''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.
 Clarified. It is somewhat convoluted as the work is progressing in
 parallel on two tickets: #2324 (this work) and #2342 (MySQL backend).
 Updated the description to cover what will be merged in #2342. The comment
 does not make much sense now, but will be ok once both tickets are merged.

 > '''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.
 Added.
 >
 > The following functions:
 > * getLastAllocated
 > * setLastAllocated
 > * getID
 Commented.

 > Also missing curly braces in '''return''' statement in '''getID'''.
 Done.
 >
 > '''getLastAllocated and getID could be declared '''const'''.
 They are now.

 > '''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.
 It is the double colon that confuses Doxygen. It thinks this is a
 namespace/class selection. I managed to work around this problem by using
 only numeric value after double colon. 2001::1234:abcd does not confuse my
 doxygen version anymore.

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

 > '''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.
 Converted to EXPECT_EQ where possible.

 > '''memfile_lease_mgr.h'''
 > '''addLease''': The Lease4Ptr and Lease6Ptr could be passed by const
 reference.
 This is already fixed in #2342. I've fixed it here as well, because I
 wanted to check if two changes introduced separately on two branches will
 create a conflict or not. Even if they do, it will be trivial to fix them.
 >
 > '''getLease4X''': IOAddress could be passed as const reference.
 Also updateLease4/6 was modified. Keep in mind that this is update the
 lease in the database, so the actual passed lease is not updated, so it
 may be constant.

 > '''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?.
 All functions that are not implemented are marked with todo. As this is a
 test code, there's no need for them to throw NotImplemented.

 Thanks for this review. Changes pushed. Is it ok to merge it?

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