BIND 10 #2526: V4 option definitions

[BIND 10 Development]

#2526: V4 option definitions
-------------------------------------+-------------------------------------
            Reporter:  stephen       |                        Owner:
                Type:  task          |  stephen
            Priority:  medium        |                       Status:
           Component:  dhcp4         |  reviewing
            Keywords:                |                    Milestone:
           Sensitive:  0             |  Sprint-DHCP-20130103
         Sub-Project:  DHCP          |                   Resolution:
Estimated Difficulty:  0             |                 CVSS Scoring:
         Total Hours:  0             |              Defect Severity:  N/A
                                     |  Feature Depending on Ticket:
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by marcin):

 * owner:  marcin => stephen


Comment:

 Replying to [comment:5 stephen]:
 > Reviewed commit 1ee4d989baee8e0fe013aa0c1e63f4e6a429d114
 >
 > I have made a change to a couple of method header comments and pushed to
 the repository.
 Thanks.
 >
 > '''src/lib/dhcp/libdhcp++.cc'''[[BR]]
 > unpackOption6(): rather than use "end" in unpackOptions6(), "length" is
 more descriptive of what buf.size() returns.
 Renamed.
 >
 > unpackOption6(): the "while" loop needs a comment preamble explaining
 that the buffer comprises a set of options, each starting with a two-byte
 type code and a two-byte length field.  It's clear when you read the code
 - and if you understand what's going on - but a short explanation for the
 non-familiar reader would be helpful.
 Comment added.
 >
 > unpackOption6(): s/offset +4/offset + 4/
 Changed.
 >
 > unpackOption6(): rather than perform the bit arithmetic to get the
 option type and length, consider using readUint16() in
 util/io_utilities.h.
 I used utility functions.
 >
 > unpackOption6(): Does the call to LibDHCP::getOptionDefs() have to be
 within the "while" loop?  Also, can option_defs be declared "const"?
 Good catch. I have taken it out from the loop. Also the call that returns
 index has been taken out of the loop.

 >
 > '''src/lib/dhcp/libdhcp++.h'''[[BR]]
 > Typo (lines 143, 152): s/is incorrect/are incorrect/

 Fixed.

 >
 >
 > '''src/lib/dhcp/std_option_defs.h'''[[BR]]
 > OK, but remember you will have to make the number of elements in each
 "struct" initialization the same as the number of elements in the
 "struct".

 I cherry-picked the similar change I had done on master for DHCPv6 and
 added initialization of all struct fields for DHCPv4 options.

 >
 > '''src/lib/dhcp/tests/libdhcp++_unittest.cc'''[[BR]]
 > The declaration of "packed" line 153 should have some comment stating
 whether these are packed V4 or V6 options.

 I added the comment and renamed the const.

 >
 >
 > '''src/lib/dhcp/tests/option6_int_unittest.cc'''[[BR]]
 > unpackSuboptions4/6: the memcpy at the start of the test is being done
 without checking the allocated size of buf_: suggest using an ASSERT_XXX
 to check that buf_.size() is large enough.  Even better though: if the
 data already in buf_ is not used, clear buf_ and add the data using
 vector<T>::insert().

 I added an assert to make sure that the buffer's length is sufficient.
 However, the buf_ is initialized with 255 values for each test case so it
 should  have sufficient length as long as you don't create a larger table
 in the test scope (which is not the case here).

 >
 > '''src/lib/dhcp/tests/pkt4_unittest.cc'''[[BR]]
 > Line 547: looks as if a debug statement is still in the code.

 Removed.

 Proposed !ChangeLog entry:
 {{{
 XXX.    [func]          marcin
         Implemented definitions for DHCPv4 option definitions identified
         by option codes: 1 to 63, 77, 81-82, 90-92, 118-119, 124-125.
         These definitions are now used by the DHCPv4 server to parse
         options received from a client.
         (Trac #2526, git XYZ)

 }}}

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