BIND 10 #1956: Implement perfdhcp IPv6 packet handling

[BIND 10 Development]

#1956: Implement perfdhcp IPv6 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:  40
                   Keywords:         |           Total Hours:  37.5
            Defect Severity:  N/A    |
Feature Depending on Ticket:         |
        Add Hours to Ticket:  37.5   |
                  Internal?:  0      |
-------------------------------------+-------------------------------------
Changes (by stephen):

 * owner:  stephen => marcin


Comment:

 Reviewed commit bff3d4786c50018143abe2316c9e837f24c52e81

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

 '''src/lib/dhcp/option.{cc,h}'''[[BR]]
 In setData():
 * Use "std::distance" to calculate the distance between two iterators.  It
 does not make any assumptions about the type of iterators.
 * The "while" loop can be replaced by a call to std::copy().
 * The function arguments are of type "!OptionBufferConstIterator" in the
 function declaration, but "const !OptionBufferConstIterator" in the
 function definition.

 Also

 * The unit tests should be extended to test this method.


 '''src/lib/dhcp/tests/pkt{4,6}_unittest.cc'''[[BR]]
 If the code were such that the timestamp was initialized when the packet
 was created and updateTimestamp() did nothing, the tests would still pass.
 A check needs to be made that the timestamp is invalid after packet
 construction.

 Also, if the ASSERT_FALSE failed, the test would have a memory leak, as
 pkt would not be deleted when the function exits.  Suggest encapsulating
 pkt in a boost::scoped_ptr and getting rid of the "delete".


 '''tests/tools/perfdhcp/localized_option.h'''[[BR]]
 The first and third constructors are redundant: just specify a default
 value of zero for the last (offset) parameter in the argument list of the
 second and fourth constructors.

 '''tests/tools/perfdhcp/perf_pkt6.cc'''[[BR]]
 Constructor: I would prefer that a method be added to set the transaction
 ID to the Pkt6 class and used, rather than modifying a member variable of
 the parent directly. (I ''think'' that variables in the parent class are
 set protected for test use only.)

 rawPack(): A comment should be added to the check on transaction id.  It
 is not clear (to me) what the restrictions on the transaction ID offset
 are.

 rawPack(): There is a second call to bufferOut_.clear() ("Seek to the
 transaction id position in output buffer"). At the moment, it looks as if
 all the data just written to the buffer is destroyed (although I see
 looking at the code for !OutputBuffer that it just clears the internal
 pointer.) As it is feasible that a future change to buffer could erase the
 data if clear() is called (perhaps as a debugging aid), this should be
 altered.  Perhaps write the data in three chunks: upto the transaction ID,
 the transaction ID, and the part after the ID?

 rawPack(): suggest you liaise with Tomek to define the offsets for the
 message type and transaction ID in the dhcp6.h file.  (Admittedly,
 assuming that the message type is the first octet in the buffer is pretty
 safe.  All the same, hard-wiring "magic numbers" in the code is usually a
 bad idea.)

 '''tests/tools/perfdhcp/perf_pkt6.h'''[[BR]]
 Class Offset: the first constructor can be removed - specify a default
 value of 1 for the offset argument of the second constructor.

 Also, I'm not convinced that use of the Offset class does give any real
 advantage.  The comments suggest that explicitly specifying offset reduces
 the chances of getting arguments mixed up with the offset: but what about
 getting other arguments mixed up?

 '''tests/tools/perfdhcp/tests/perf_pkt6_unittest.cc'''[[BR]]
 Constructor: The size of the data in pkt1/pkt2 is checked against 6 - that
 test will fail if ever the static initialization of "data" is changed.
 Perhaps check against "sizeof(data)"?

 !RawUnpack: buf_elapsed_time is declared without an explicit size,
 buf_duid with an explicit size.  This is an inconsistency.

 !RawUnpack: creation of !OptionBuffers from static data - using
 "sizeof(<buffer>)" instead of explicitly putting the buffer size in the
 constructor guards against a future change of the buffer sizes.

 !RawUnpack: "magic numbers" 4 and 86 would be better declared as "const
 int" symbols.

 !PackTransactionId/!UnpackTransactionId: The initialization of data[100]
 will only initialize the first element, as there is only one constant.
 (True the rest of the array may well be initialized to zeroes, but that is
 down to implementation of the compiler.)  Safer would be to use memset()
 or std::fill().

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