BIND 10 #2526: V4 option definitions

[BIND 10 Development]

#2526: V4 option definitions
-------------------------------------+-------------------------------------
            Reporter:  stephen       |                        Owner:
                Type:  task          |  marcin
            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 stephen):

 * owner:  stephen => marcin


Comment:

 Reviewed commit 1ee4d989baee8e0fe013aa0c1e63f4e6a429d114

 I have made a change to a couple of method header comments and pushed to
 the repository.

 '''src/lib/dhcp/libdhcp++.cc'''[[BR]]
 unpackOption6(): rather than use "end" in unpackOptions6(), "length" is
 more descriptive of what buf.size() returns.

 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.

 unpackOption6(): s/offset +4/offset + 4/

 unpackOption6(): rather than perform the bit arithmetic to get the option
 type and length, consider using readUint16() in util/io_utilities.h.

 unpackOption6(): Does the call to LibDHCP::getOptionDefs() have to be
 within the "while" loop?  Also, can option_defs be declared "const"?

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


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

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


 '''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().

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

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