BIND 10 #1955: Implement perfdhcp IPv4 packet handling

[BIND 10 Development]

#1955: Implement perfdhcp IPv4 packet handling
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  marcin
  stephen                            |                Status:  reviewing
                       Type:         |             Milestone:  DHCP-
  enhancement                        |  Sprint-20120611
                   Priority:         |            Resolution:
  medium                             |             Sensitive:  0
                  Component:         |           Sub-Project:  DHCP
  perfdhcp                           |  Estimated Difficulty:  32
                   Keywords:         |           Total Hours:  20
            Defect Severity:  N/A    |
Feature Depending on Ticket:         |
        Add Hours to Ticket:  20     |
                  Internal?:  0      |
-------------------------------------+-------------------------------------
Changes (by stephen):

 * owner:  stephen => marcin


Comment:

 Reviewed commit 53dfacddb52280654910206f96325c531a480ce0

 I've corrected the comments in some of the method headers and pushed to
 the repository - "git pull" before making any more changes.

 '''All .cc/.h files in tests/tools/perfdhcp and
 tests/tools/perfdhcp/tests'''[[BR]]
 These should be copyright 2012.  Suggest these be fixed as part of this
 ticket.


 '''tests/tools/perfdhcp/perf_pkt{4,6}.{h,cc}'''[[BR]]
 Constructor: Looking at the code, the first constructor is redundant - the
 same can be achieved using the third constructor if the transid_offset
 argument is given a default value of 1.

 Constructor: neither the first nor third constructor set the transid_
 value, which implies that the value is not important and should probably
 be zero. (This is in fact what it is set to when the "Pkt4(buf, len)"
 constructor is called, although it is set to a random value in the
 equivalent Pkt6 constructor.)  In this case, all constructors could be
 collapsed into a single constructor with a signature of:
 {{{
 PerfPkt{4|6}(const uint8_t* buf, size_t len,
              size_t transid_offset = 1, uint32_t transid = 0)
 }}}



 '''Protected Variables'''[[BR]]
 I'm usually against use of "protected", but here - where the PerfPkt4 and
 PerfPkt6 classes need to manipulate the internals of the Pkt4 and Pkt6
 classes to customise packets for the performance tests - I don't really
 see an easy way round it.  The PerfPktN classes manipulate internal
 variables which have no public interface to allow them to be modified.

 In part mitigation, I suggest that the public accessor methods of the PktN
 classes be used where possible (e.g. use getTransid() instead of accessing
 transid_ directly).  These methods could be extended in some cases, e.g. a
 PktN::setTransid() method seems logical.

 For the remainder, protected access is OK, but the comments in the
 protected section PktN should be extended to note that these variables are
 being accessed from classes in the perfdhcp program (and the reason for
 it).  This is imperfect, but should serve as a warning to anyone modifying
 the PktN classes that changes may have an impact on the perfdhcp code.


 '''tests/tools/perfdhcp/pkt_transform.cc'''[[BR]]
 pack(): Need some comment about the check on transaction ID to state that
 the transaction ID must fit in the packet with some space to spare. Also,
 the check that
 {{{
 (transid_offset + transid_len + 1 > in_buffer.size())
 }}}
 ... could be simplified to:
 {{{
 (transid_offset + transid_len >= in_buffer.size())
 }}}

 pack(): Identical comments to those in ticket #1956 about the rawPack()
 function in perf_pkt6.cc.

 unpack(): When reading a four-byte transaction ID, it would be easier to
 use InputBuffer::setPosition followed by InputBuffer::readUint32.  (In
 fact, we could also use it for reading the three-byte transaction ID in
 the V4 universe: as we know that the offset of the transaction ID is not
 zero (this was checked in the "validate transaction ID" check), it would
 be possible to reads a four-byte value from offset "transaction ID - 1"
 and mask out the high-byte. If this is done, it would require a comment as
 the logic is not obvious.)

 packOptions(): if for some reason the option in the !OptionCollection is
 not a !LocalizedOption, the program will segfault when it receives a null
 pointer through the boost::dynamic_pointer_cast call.  Perhaps an
 additional test that the pointer returned is not null?

 PerfPkt4Test::capture(): The code is fine, although I woudn't have
 bothered with trying to optimise the initialization of "buf".  This is
 test code after all, and the additional overhead of reinitializing "buf"
 on every call is negligible.

 Constructor test: Even though it doesn't make much difference here, in
 general "++i" is preferred to "i++": it avoids use of temporaries when "i"
 is a complex object.

 !RawPack test: suggest adding a comment to say what the data in the
 buf_hostname and buf_file_size arrays are (i.e. option code, length,
 data).  People modifying this code in the future may not have a deep
 knowledge of DHCP. (Another possibility is to use the DHO_ code instead of
 the option number in the array initialization.)

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