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

Comment (by tomek):

 Replying to [comment:8 stephen]:
 > '''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.
 Done.
 >
 > Can't the DHCP_OPTIONS_COOKIE be defined as a "static const char*"?
 No, it cannot. That causes "defined but not used" warning that is treated
 as error. Changed to static const char* as suggested, but also commented
 it out.

 >
 > 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.)
 Done.
 >
 > > 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.
 Yes, I understand. The problem is with naming the enums. I'm not sure what
 RAI or defines starting with RAI are. I could try to invent generic name
 like RaiTypes, but I don't want to do that. Message and option types are
 easy. I understand what they are meaning, so naming those enums
 DHCPMessageType or DHCPOptionType is adequate.

 Anyway, both remaining define groups (RAI_* and FQDN_*) are ifdefed 0 and
 commented appropriately.

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

 > load_addr_ and remote_addr_ in the Pkt4 constructors can also be
 initialised using DEFAULT_ADDRESS.
 Done.
 >
 > >> 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.
 I give up. const vector<uint8_t>& it is.

 > > 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.
 Changed the code as requested.

 > >> 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>&"?
 Note that hook document predates the actual implementation. Just recently
 nobody told me "Tomek, please drop the coding work for now and update the
 document". :-)

 > 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.
 Valid points. Hooks document should be updated then. As a convenience to
 Shawn, whoever updates the hooks document, should explain him, why we his
 recommendation to keep parameter formats unified between calls is no
 longer honored.

 > > 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.
 Agree.  Noting that obviously I'm not a native speaker, I try to follow
 the rules of written English. Please let me know if there are any comments
 that you want me fixed. Having said that, I can continue working on
 comments text to better adhere to proper English and improve my English
 skills in the process. Alternatively, we could consider possibility of
 accepting comments as they are now, and move on to working on a new code.
 :-)

 > >> 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);
 > }
 > }}}
 Thank you. Replaced old implementation with your code.

 Code changes pushed to trac1224. Please review.

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