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

Comment (by tomek):

 Replying to [comment:6 stephen]:
 > '''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.
 Added constructor:
  Option(Universe u, uint16_t type,
         std::vector<uint8_t>::const_iterator first,
         std::vector<uint8_t>::const_iterator last);
 It is ok for now, but it should be migrated to templated version
 eventually. Added TODO text in comments that suggest that. There are
 currently 2 constructors that take different parameters. One takes the
 whole vector (easier to use), while the second takes two iterators (more
 flexible). I think both are useful and should stay.

 > '''src/lib/dhcp/option.cc'''[[BR]]
 > Spaces around binary operators.
 Done. I hope I picked them all.

 > 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())".
 Done.

 > '''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.
 Done that in options test. However, in HWAddr test, I really want vector
 length to be kept fixed at field length.

 >
 > '''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
 > }}}
 Clever. Done. Fix in buffer.h went as a separate checkin in case someone
 wants cherry-pick this on his branch.

 Note: Also added ChangeLog entry.

 Please review.

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