BIND 10 #2414: Use AllocationEngine to actually handle v6 clients' messages

[BIND 10 Development]

#2414: Use AllocationEngine to actually handle v6 clients' messages
-------------------------------------+-------------------------------------
                   Reporter:  tomek  |                 Owner:  marcin
                       Type:  task   |                Status:  reviewing
                   Priority:         |             Milestone:  Sprint-
  medium                             |  DHCP-20121115
                  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 stephen):

 * owner:  stephen => marcin


Comment:

 '''dhcp6_srv_unittest.cc'''[[BR]]
 > advertiseOptions: There are some comments needed in the new code that
 sets up the Solicit message. In particular: why do we have to set the link
 local address: fe80::abcd? What are the magic numbers 234, 1500, 3000?
 I don't think they are anything special: the fact that the server is
 started with a port of 0 means that no sockets are opened, so fe80::abcd
 is just an arbitrary address.  And the others are just random numbers of
 the test.  However, Tomek wrote the test, so he is best placed to give the
 reasons.  Added #2474 to cover this.

 > Also extra space before
 > Pkt6Ptr sol = Pkt6Ptr(...);
 Removed.

 '''dhcp6_srv.cc'''[[BR]]
 > Dhcp6Srv: Is there any reason for not initializing the Allocation Engine
 in the constructor's initialization list but in the constructor's body?
 Comments in the header file indicate that it should be a pointer with the
 intention that it can be changed at runtime.  I've interpreted this to
 mean that at some point, a function call will be needed to extract
 allocation engine  parameters, something that will have to be done in the
 constructor body.  For that reason, I left the initialization there,
 rather than move it to the initialization list.

 >Does it have to be initialized after other objects, i.e. logger, option
 defs etc?
 I think the order is not relevant.

 '''dhcp6_srv.h'''[[BR]]
 > Dhcpv6Srv header: There is nothing in Dhcpv6Srv that makes it singleton
 class. Doxygen comment should be changed.
 Comment updated.

 '''dhcp_srv_unittest.cc'''[[BR]]
 > Dhcp6SrvTest constructor: Technically the subnet_ and pool_ could be
 initialized with constructor's initialization list not in the body.
 Agreed, although as the initialization statements are a bit long, I think
 it would look a bit clumsy. I suggest we leave it for now.


 > checkIAAddr: It would be good to comment why we compare:
 > EXPECT_EQ(expected_addr.toText(), addr->getAddress().toText());
 > instead of
 > EXPECT_EQ(expected_addr, addr->getAddress());
 > I presume this is because the == operator is not working properly for
 IOAddress because of the presence of uint32 operator?
 It is because IOAddress can't be streamed to an ostream using operator<<()
 (used by EXPECT_EQ to report an error). It is possible to use
 EXPECT_TRUE(address1 == address2), as OPAddress does support operator==():
 however, this gives less information in the case of a failure.  A comment
 explaining this has been added.

 > checkIAAddr: the addr parameter could be passed by const reference.
 Changed.

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