BIND 10 #1224: V4 packet library - basic framework

[BIND 10 Development]

#1224: V4 packet library - basic framework
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  tomek
  stephen                            |                Status:  reviewing
                       Type:  task   |             Milestone:  Sprint-
                   Priority:  major  |  DHCP-20111026
                  Component:  dhcp   |            Resolution:
                   Keywords:         |             Sensitive:  0
            Defect Severity:  N/A    |           Sub-Project:  DHCP
Feature Depending on Ticket:         |  Estimated Difficulty:  0
        Add Hours to Ticket:  0      |           Total Hours:  0
                  Internal?:  0      |
-------------------------------------+-------------------------------------
Changes (by stephen):

 * owner:  stephen => tomek


Comment:

 '''src/lib/dhcp/Makefile.am'''[[BR]]
 Should add dhcp4.h to libdhcp_la_SOURCES

 '''src/lib/dhcp/dhcp4.h'''[[BR]]
 Rather than "format kept for easier merge" I think that the real reason is
 that DHCPv4 is still being developed and that this file may change if the
 code is extended.

 Having said that, the DHCPv4 protocol is fairly fixed, so I think there is
 a very strong case for moving to a C++ paradigm.  In particular, defining
 the different constants as enums would ensure that C++ type checking
 checked that constants were appropriate for the fields.  At this stage of
 the work, it should not be too much of an overhead.

 '''src/lib/dhcp/pkt4.h'''[[BR]]
 Public constants in Pkt4 want a Doxygen-style comment before them.

 Typographical error in the DHCPV4_PKT_HDR_LEN comment - "specifes".

 In the Pkt4 constructor used for message transmission, does the first line
 require an "@brief" tag?

 Same constructor, the "data" element is really a pointer to the data to be
 written to the message, not a pointer to received data.

 Header comments for pack()/unpack(): there are no members named data_ or
 data_len_.

 Header for unpack(): doesn't this parse a DHCPv4 packet, not a v6 one?
 Also, it should return true if the unpacking was successful, not if the
 "build" was successful.

 len(): a better description would be that len() returns the size of the
 required buffer to build the packet with the current set of packet
 options.

 getSname()/getFile(): I think it would be safer to return a "const
 vector<uint8_t>" unless there is a very strong guarantee that the Pkt4
 structure will remain in existence while the the data is accessed.

 setHWAddr(): there is the assumption that the macAddr argument points to
 an array that is long enough.  Passing in a "const vector<uint8_t>&" would
 be better in that the length can be checked.

 Description of ifindex: "Windows" is spelt with a capital "W".  (You might
 not like it, but let's give it its proper name :-) ).

 op_ description (first line): This is the only line in the file that is
 more than 80 characters long (and 80 characters is the default width at
 which my editor opens).  Can we move the last word to the next line?

 chaddr_/sname_/file_ fields.  The declarations of the size of these arrays
 should use the constants defined earlier in the class.

 bufferIn_: We need to look again at the hooks interface.  I'm thinking
 that perhaps we don't need the buffer_rcvd() or buffer_end() callbacks -
 only let the users modify data in the form of a PktN& obejcts.  (besides,
 the DhcpHooks document advises against modifying the input or output wire
 data.) In this case, we are not passing a raw data buffer to the hooks and
 so bufferIn_ can be an !InputBuffer structure. (Not: if adopted, this
 requires a modification to DhcpHooks).

 '''src/lib/dhcp/pkt6.cc'''[[BR]]
 Constructors: for members that are of type IOAddress, there is no need to
 create an intermediate IOAddress object (i.e. ciaddr_("0.0.0.0") will work
 as well as ciaddr_(IOAddress("0.0.0.0")).  The former is initializing via
 a direct constructor, the latter is creating an intermediate object and
 initializing ciaddr_ via a copy constructor.

 Having said that, and at the risk of premature optimisation, there is a
 lot of parsing of "0.0.0.0" every time a Pkt4 object is created.  Would it
 be better to declare an external variable like:
 {{{
 namespace {
 const IOAddress DEFAULT_ADDRESS("0.0.0.0");
 }
 }}}
 ... and to initialize the address objects with a copy of DEFAULT_ADDRESS?
 That way the parsing is only done once.

 setHWAddr: should be spaces around binary operators (i.e. "hlen >
 MAC_CHADDR_LEN").

 setHWAddr: can the macAddr be encapsulated in a vector (in which case
 there is no need to pass the hlen argument)?

 setFile(): comment at the end of the method shows a cut and paste :-)


 '''src/lib/dhcp/tests/pkt4_unittest.cc'''[[BR]]
 constructor: use the form "type* variable" instead of "type * variable".

 constructor: surround binary operators with spaces ("i = 0" instead of
 "i=0").

 constructor: even single-line "for" loops should have the statement
 enclosed in "{" and "}".

 Suggest starting comments with capital letters.

 generateTestPacket1(): Space between parentheses in setFlags() call.

 generateTestPacket2(): I must admit I would have used a vector<> and a
 std::copy using a back_inserter object to copy data into it.  That way
 there is no chance of overflowing the buffer.  As it is:
 * In the memcpy calls, use the constants as the length argument instead of
 the numbers.  (If numbers are used, then it would be worth checking their
 value with an EXPECT_EQ check).
 * On exit from the function, we should expect that (offset - &buf[0]) is
 equal to DHCPV4_PKT_HDR_LEN.

 fixedFields test - would prefer "EXPECT_EQ(0, memcmp(...)) to
 EXPECT_FALSE().

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