BIND 10 #1224: V4 packet library - basic framework

[BIND 10 Development]

#1224: V4 packet library - basic framework
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  stephen
  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 tomek):

 * owner:  tomek => stephen


Comment:

 Replying to [comment:6 stephen]:

 Thanks for the review.

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

 > '''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.
 That is probably true that we won't be doing any merges. Reworked most of
 the header text. I left defines that I don't know about. I don't want to
 guess names for enums just yet. I'll convert them to C++ enums as soon as
 I start using them.

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

 > Typographical error in the DHCPV4_PKT_HDR_LEN comment - "specifes".
 Fixed.

 > In the Pkt4 constructor used for message transmission, does the first
 line require an "@brief" tag?
 Added.
 >
 > Same constructor, the "data" element is really a pointer to the data to
 be written to the message, not a pointer to received data.
 Comment was confusing. It is hopefully more readable now. That constructor
 is used when receiving data. Clarified that input data will be copied to
 bufferIn_.

 > Header comments for pack()/unpack(): there are no members named data_ or
 data_len_.
 Copy-and-paste error. Clarified that bufferIn_ and bufferOut_ will be
 used.

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

 > 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.
 Done.
 >
 > 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.
 Done are requested. Note that is made use of this method both slower and
 more complicated. See changes in pkt4_unittest.cc.

 > 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.
 This function was implemented under the assumption that its user is a sane
 developer that understands that passing a pointer to a buffer and lying
 about its length will cause problems. As we do not know where MAC address
 date will come from (note that there could possibly be different sources
 of that information) at this stage, using const uint8_t* seems like a
 reasonable choice. Expanded comment to explain that and warn about passing
 hlen that is larger than buffer pointed by macAddr. Also added check if
 macAddr is NULL.

 >
 > Description of ifindex: "Windows" is spelt with a capital "W".  (You
 might not like it, but let's give it its proper name :-) ).
 How about wINDOWS? :-). Ok, fixed. It is now "MS Windows".
 >
 > 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?
 It is now less than 80 characters long. Also expanded this description a
 bit.
 BTW What happened to the discussion about extending allowed line length to
 100?

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

 > 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).
 I thought that we decided that on a high-level, hook user will have the
 ability to inject his changes on 2 levels: binary format and later at
 parsed options. There are valid cases when user may want to modify wire
 format - e.g. to fix fields for buggy clients, to recalculate checksums
 etc.

 It is true that user will have the ability to modify PktN& object, but we
 need to pass it twice. First, after we receive raw packet, we will pass it
 to buffer_rcvd(). Essential part is that hook can modify incoming packet.
 Therefore we must do parsing AFTER this hook returns. Then we parse this
 buffer, create options, set fields and so on. After parsing is complete,
 we call packet_rcvd() and user can tweak this packet further on a fields
 and options level.

 We could simplify this under the condition that there will be NO checks or
 any sort of processing between receiving buffer and parsing it. I can't
 specify any concrete examples at this moment, but I believe that one of
 useful functions of hooks at this stage would be conformance fix. Let me
 explain. Imagine a network with old devices that cannot be upgraded or
 migrated. Some of those older clients may send slightly malformed packets.
 We have first hand experience with cases when clients abused single domain
 option to send list of domains. One way of solving this problem is to
 implement hook that will fix such packets on the fly.

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

 > 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)?
 It could be. But hlen is a field in DHCPv4 packet. I prefer clean method
 that sets those fields as they are. Adding some logic that hlen will be
 set based on length of a vector is not the way to go here, especially
 since there are corner cases ahead. For example for Infiniband hlen is 20,
 but hlen field must be set to 0 and actual hardware address is stored in
 client-identifier option. htype, hlen and hardware address really should
 be set together in the simplest possible way.

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


 > '''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").
 Sorry for this recurring mistake. That's a notation I've been using for
 years and it's tricky to discard old habits.

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

 > Suggest starting comments with capital letters.
 Should comments form a complete sentence? I was trying to follow a logic
 that if a comment contains sentence, it should be formed as such (starting
 with capital letter and ending with a dot.). If it is just a short
 explanation, it is written in lower-case, without a dot.

 >
 > 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.
 I've never used back_inserter object. Could you commit code that does
 that?

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

 Fixes pushed to trac1224. Please re-review.

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