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 marcin):

 Replying to [comment:6 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.

 Yes, I am sorry for this. I will do my best to avoid this in the future.

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

 Removed

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

 Corrected.

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

 I implemented multipleSockets test that combines testing of getSockets()
 and closeSockets(). It will basically try to open more than one socket,
 check if list of sockets is matching expected socket descriptors. It will
 also try to use sockets (expecting success), close them with
 closeSockets() and try to use them again (expecting failure).
 [[BR]]
 In the socketFromRemoteAddress I added new section to test broadcast
 address like this:
 {{{
  int socket3  = 0;
     IOAddress bcastAddr("255.255.255.255");
     EXPECT_NO_THROW(
         socket3 = ifacemgr->openSocketFromRemoteAddress(bcastAddr, PORT2);
     );
     EXPECT_GT(socket3, 0);
     close(socket3);
 }}}



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

 Dot added.

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

 Added extra comment.

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

 I left it untouched for now. Writing SocketCollection::iterator or
 SocketCollection::const_iterator does not cost a lot comparing to
 SocketCollectionIter. Assuming that objects in SocketCollection are rather
 read only, clients should use const_iterator. Shouldn't it be
 SocketCollectionConstIter defined instead?

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

 It is planned that we create an additional ticket to update documentation
 of perfdhcp after code refactoring and integration. I suggest that I
 implement your suggestion as a part of this new ticket.
 >
 > '''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)?

 I did not want to complicate this new factory function. I assumed that the
 purpose of this function is to find if the appropriate factory function
 has been registered and pass the call to it.
 The necessity to copy the buffers occurs because of Option class
 constructor that takes the reference to the buffer. This buffer has to be
 created somewhere. Usually it will be created within the custom factory
 function or by the client class that calls Option::Factory(). Since it is
 passed by reference further on, it will not be copy constructed any
 further until Option constructor that initializes data_ member. In fact we
 have just one copy operation that we can't really avoid.

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

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

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

 I added requested checks to the test.

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

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