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

 Replying to [comment:8 marcin]:
 > Replying to [comment:6 tomek]:
 > > 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).
 This comment was not responded. I've added appropriate @todo in the code.

 > > openSocket6()
 > > if (addr.toText() != "::1") was moved to the previous line. It should
 be in a separate line.
 > Corrected.
 It still looked ugly for me. "if" statement was in the same line as
 previous statement. Furthermore, there was no curly braces after it. I've
 pushed fixed code.

 > 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);
 > }}}
 It is all that can be done now, but it does not address the main issue.
 How do you specify which interface should be used if you have eth0 and
 eth1? Broadcast is valid on both. You have no control over which interface
 is going to be used.

 That's why I said that it is not easily solvable now. I've added a @todo
 in the code and consider the issue closed. We will have to get back to it
 eventually, but it will not be in a near future.

 > > 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?
 Ok. I've added a @todo in the text. That's yet another thing that we will
 do some time in the future.

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

 > > '''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.
 My point was that Option class should also take pointers (or iterators) to
 first and last byte of the buffer, so no copying actually occurs. But this
 is a generic problem of libdhcp, so fixing it in test control ticket
 wouldn't be appropriate. Added @todo for this.

 Option(const Option& buf) format is much easier to use in creating options
 and in tests.
 Option(const Option::const_iterator begin, const Option::const_iterator
 end) is much easier to use in receiving options. I believe we will
 eventually need both constructors.

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

 Ok, I consider all comments raised in part 1 addressed. Now, on to part
 two.

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