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:  stephen => tomek


Comment:

 '''src/lib/asiolink/tests/io_address_unittest.cc'''[[BR]]
 Just noticed - the "uint32" test should also check to see that an
 exception is raised if an attempt is made to convert a V6 address to a
 uint32_t.

 '''src/lib/dhcp/libdhcp.h'''[[BR]]
 >> unpackOptions4 needs its own header.
 > Why? This is part of the library. Traditionally, you provide a single
 header that exposes all methods offered by a library. User is not expected
 to use all methods, just the ones that are suitable for specific use.
 Mainly for the Doxygen documentation.  Without an individual header, the
 entry for the method in the LibDHCP Doxygen page contains just the bare
 function signature.

 Also, it would be useful to have a summary of what the LibDHCP class does
 (again for the Doxygen page).

 '''src/lib/dhcp/option.{cc,h}'''[[BR]]
 Changes OK.  However...

 In the constructor taking the first/last arguments and the one taking an
 offset and length, data_ can be initialized on the declaration line.
 Currently it is initialized in the body of the constructor via copy
 constructor.

 Also, the checking of the arguments is the same in all constructors.  This
 could be extracted into a private method and each constructor call that
 method.

 When checking the arguments, the test "u == V4" is carried out twice.
 Would be clearer to have:
 {{{
 if (universe_ == V4) {
     if (data_.size() > 255) {
         ...
     } else if (type_ > 255) {
         ...
     }
 }
 }}}

 >> '''src/lib/dhcp/option.cc'''[[BR]]
 >> Spaces around binary operators.
 > Done. I hope I picked them all.
 There is an extraneous space in the declaration of "uint8_t * ptr" in
 pack6() :-)

 '''src/lib/dhcp/tests/option_unittest.cc'''[[BR]]
 This now fails to compile.  The problem seems to be line 90:
 {{{
 EXPECT_EQ(optData, data);
 }}}
 This fails because in case of inequality, the macro attempts to output the
 first argument using operator<< and this is not defined for a vector.
 Suggested is:
 {{{
 EXPECT_TRUE(optData == data);
 }}}

 >> '''src/lib/util/buffer.h'''[[BR]]
 > Clever. Done. Fix in buffer.h went as a separate checkin in case someone
 wants cherry-pick this on his branch.
 There was no need to leave the test in that method, as it will be carried
 out in the call to the other readData() method.  (Although the test does
 guarantee that the input vector is not changed if an exception is thrown.)

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