BIND 10 #1959: Implement perfdhcp control logic

[BIND 10 Development]

#1959: Implement perfdhcp control logic
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  tomek
  stephen                            |                Status:  reviewing
                       Type:         |             Milestone:  Sprint-
  enhancement                        |  DHCP-20120903
                   Priority:         |            Resolution:
  medium                             |             Sensitive:  0
                  Component:         |           Sub-Project:  DHCP
  perfdhcp                           |  Estimated Difficulty:  0
                   Keywords:         |           Total Hours:  0
            Defect Severity:  N/A    |
Feature Depending on Ticket:         |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------

Comment (by tomek):

 First, let me say that this ticket is huge (the diff is 5400 lines long).
 In future tickets, it would be reasonable to split the work into separate
 tickets to make it more manageable.

 '''src/lib/dhcp/iface_mgr.cc'''
 Please remove cout statement in closeSockets(). There's a separate ticket
 for removing cout in the whole libdhcp library. Let's not add new work to
 it.

 getLocalAddress()
 How is the code for broadcast is going to work on a device with multiple
 interfaces? 255.255.255.255 is a local network broadcast address. If your
 host is connected to 2 networks, it is valid on both. There should be a
 way to specify which interface is going to be used. I'm not sure how to do
 it with boost (and IPv4), but there is a field in6_scope_id that can be
 used for scope in IPv6. Perhaps something similar is available for IPv4.
 If it isn't, we should at least mention it in the docs. (od add @todo note
 somewhere).

 openSocket6()
 if (addr.toText() != "::1") was moved to the previous line. It should be
 in a separate line.

 There are no tests for new code introduced in iface_mgr.{cc|h}. In
 particular, closeSockets() and getSockets() should be tests (easy) and the
 change about broadcast address in getLocalAddress() (may be tricky). It
 would be good to have tests for IfaceMgr::openSocketFromRemoteAddress()
 that check the broadcast path. One possible thing to consider here is to
 use the same approach BIND9 is using. They have a script that is expected
 to run (with root privileges) before tests. That script sets some well
 known addresses on the loopback interface. Then the tests check if the
 expected addresses are there, then skip some cases if they aren't. All
 BIND9 developers had run this script once and they now have some specific
 address setup on their machines and can run those additional tests. Of
 course, if you can come up with a simpler approach, that would be
 preferable.

 '''src/lib/dhcp/iface_mgr.h'''
 Nit-pick: The comment for closeSockets() should end with a dot.

 The comment for getSockets() should caution that the returned reference is
 only valid as long as the object that returned it. It was introduced in
 commit 4ae9b5ea.

 Since SocketsCollection is now used outside of the Iface class, perhaps
 SocketsCollectionIter type should be defined? I don't have strong opinion
 here, so it may be left as it is now.

 '''Documentation'''
 I see that there's quite a bit of new code in the iface_mgr (added mostly
 in previous tickets). Some description about how the socket management is
 done should be added to doc/devel/02-dhcp.dox. In fact, the 20-dhcp.dox
 can probably be renamed, split and perhaps moved to more suitable
 directory. Or it can stay in doc/devel - it is up to you.

 The key point with the documentation here is that not huge multi-page is
 needed. It is addressed to C++ developers, who can read the code and
 understand what each method does separately. It should present "the big
 picture".

 '''libdhcp++.cc'''
 I just wanted to say that it is great that the code is being written
 there. It will be very useful for option definition framework.

 What happens if the buffer passed to optionFactory() is not consumed
 entirely, e.g. there are 20 bytes for an option that conveys a single IPv4
 address? There's no way to detect that or report somehow where the parsing
 ended.

 A packet is received and put into some buffer. How is this mechanism going
 to be used without copying the data to a separate OptionBuffer, just to
 call optionFactory() that will create an object that will copy the data
 again (that copy is unavoidable)?

 '''libdhcp++.h'''
 There is an extra empty line after optionFactory(). Methods should be
 separated with a single line only.

 '''option.h'''
 There is an extra empty line after factory() method.

 '''tests/libdhcp++_unittest.cc'''
 The test for optionFactory() is not thorough enough. Dummy function
 "return OptionPtr(1);" would have passed it. Additional checks for
 returned option being the right type of right length and having the right
 content is needed.

 Review of changes in tests/tools/perfdhcp will follow.

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