BIND 10 #1228: V4 packet library - option processing

[BIND 10 Development]

#1228: V4 packet library - option processing
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  tomek
  stephen                            |                Status:  reviewing
                       Type:  task   |             Milestone:  Sprint-
                   Priority:  major  |  DHCP-20111109
                  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:  UnAssigned => tomek


Comment:

 Reviewed commit bfab5a33ceabe3f0d31bd465d13308c8b84adf68

 '''src/lib/dhcp/libdhcp.h'''[[BR]]
 unpackOptions4 needs its own header.

 Should unpackOptions4() be taking an !InputBuffer object?

 packOptions - should this be called packOptions4?

 '''src/lib/dhcp/libdhcp.cc'''[[BR]]
 Regarding the "TODO" in unpackOptions4: Looking at this and the definition
 of Option, perhaps the easiest thing would be to alter the Option
 constructor to something like:
 {{{
 Option::Option(Universe u, uint16_t type, InputIterator first,
                InputIterator last)
 }}}
 ... and pass &buf[offet] and &buf[offset + opt_len] as the "first" and
 "last" arguments.

 '''src/lib/dhcp/option.h'''[[BR]]
 See above comments regarding Option constructor.


 '''src/lib/dhcp/option.cc'''[[BR]]
 Spaces around binary operators.

 pack6(): If checking an STL container for being empty, it's best to use
 the empty() method.  For some STL containers the time taken to perform a
 size() depends on the number of elements in the container.  Having said
 that, for a vector, the time to perform an empty() should be about the
 same as that for a size().  But I think "if (! data_.empty())" is clearer
 than "if (data_.size())".

 '''src/lib/dhcp/tests/libdhcp_unittest.cc'''[[BR]]
 packOptions4 test: there is no need to resize a vector before adding
 elements to the end of it.  The use of push_back() to append an element to
 the end of a vector is less error-prone.

 '''src/lib/util/buffer.h'''[[BR]]
 readData(): The problem here is that if the vector is too long, you end up
 with a vector of which the first "len" bytes hold the data and the
 remainder are junk.  Also, when filling the data, an intermediate vector
 is created then copied by the assignment operator.  I would suggest the is
 better:
 {{{
 data.resize(len);               // Ensure vector is correct length
 readData(&data[0], len);        // Copy data into it using overloaded
 method
 }}}

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