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/dhcp4.h'''[[BR]]
 > I left defines that I don't know about.
 OK, can you add a comment to that effect in the file?  Also, commenting
 them out (#if 0 ... #endif) would be a check that they aren't used
 unexpectedly.

 Can't the DHCP_OPTIONS_COOKIE be defined as a "static const char*"?

 Not BIND 10 convention to indent fields within a namespace.  (Also, to be
 consistent, the trailing brace of the BOOTPTypes should be on a new line.)

 > I don't want to guess names for enums just yet
 If I understand you, I think you mean that you don't want to change the
 names of members of the enums (e.g. change DHO_PAD to "Pad" etc.).  If
 this is the case, I don't think there is any reason to; if nothing else,
 leaving the names the same as DHCPv4 will make it easier to read the code.

 The reason for wanting enums was type safety.  If you have method that
 takes a DHCPOptionType (say), but when calling that method incorrectly
 call it with the argument of DHCPDISCOVER (for example), you will get a
 compile-time error.  If the methods were to just accept an int, this would
 go unnoticed unless it caused some form of run-time failure.

 '''src/lib/dhcp/pkt4.cc'''[[BR]]
 I've just noticed this and I'm a bit surprised that it has compiled.  The
 Pkt4 class declaration in pkt4.h is in the namespace isc::dhcp, but the
 definitions in this file are in the namespace isc.

 Also, the trailing brace of a namespace doesn't need a semi-colon after
 it.

 load_addr_ and remote_addr_ in the Pkt4 constructors can also be
 initialised using DEFAULT_ADDRESS.

 >> 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.
 I think that's assumed.  But as Jinmei has pointed out on a related
 ticket, errors can happen and we all make them.

 > 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.
 Leave it for now, but I do think we will need to revisit this later.

 >> bufferIn_: We need to look again at the hooks interface...
 Your comments about modifying a packet for conformance are well-made.  OK,
 we need to have some way to vary the raw packet data.  As the packet is
 raw, why are we passing a Pkt6 object to buffer_rcvd()/buffer_sent()?
 Wouldn't it be better to pass a "vector<uint8_t>&"?  That way the called
 function can quite happily modify the data and the length of the data?  It
 also has the advantage in that the correct allocator for the vector should
 be used, which avoids the problems that arise on some operating systems
 where memory is allocated in one DLL and freed in another.

 > Should comments form a complete sentence?
 I admit I sometimes omit trailing periods on one-line comments which I
 really shouldn't do.  The reason I like capital letters at the start of
 comments (and spaces around binary operators etc.) is that I believe that
 the code and comments are easier to read if they conform as closely as
 possible to the rules of written English.

 >> 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?

 Here's a modified version of generateTestPacket2():
 {{{
 // Returns a vector containing a DHCPv4 packet header.
 vector<uint8_t>
 generateTestPacket2() {

     // That is only part of the header. It contains all "short" fields,
     // larger fields are constructed separately.
     uint8_t hdr[] = {
         1, 6, 6, 13,            // op, htype, hlen, hops,
         0x12, 0x34, 0x56, 0x78, // transaction-id
         0, 42, 0xff, 0xff,      // 42 secs, 0xffff flags
         192, 0, 2, 1,           // ciaddr
         1, 2, 3, 4,             // yiaddr
         192, 0, 2, 255,         // siaddr
         255, 255, 255, 255,     // giaddr
     };

     // Initialize the vector with the header fields defined above.
     vector<uint8_t> buf(hdr, hdr + sizeof(hdr));

     // Append the large header fields.
     copy(dummyMacAddr, dummyMacAddr + Pkt4::MAX_CHADDR_LEN,
 back_inserter(buf));
     copy(dummySname, dummySname + Pkt4::MAX_SNAME_LEN,
 back_inserter(buf));
     copy(dummyFile, dummyFile + Pkt4::MAX_FILE_LEN, back_inserter(buf));

     // Should now have all the header, so check.  The "static_cast" is
 used
     // to get round an odd bug whereby the linker appears not to find the
     // definition of DHCPV4_PKT_HDR_LEN if it appears within an
 EXPECT_EQ().
     EXPECT_EQ(static_cast<size_t>(Pkt4::DHCPV4_PKT_HDR_LEN), buf.size());

     return (buf);
 }
 }}}

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