BIND 10 #1228: V4 packet library - option processing

[BIND 10 Development]

#1228: V4 packet library - option processing
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  stephen
  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 tomek):

 * owner:  tomek => stephen


Comment:

 Replying to [comment:10 stephen]:
 > '''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).
 I see your point. Providing documentation for libdhcp is a task on its
 own. It's not a matter of explaining what each method does (that is
 complete), but rather to show how to use them together. Created ticket
 #1367 for that. In its description I included a link to Doxygen
 documentation for Dibbler.

 >
 > '''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.
 Ok, fixed that. I don't like it much as we copy the data even when invalid
 type is defined. On the other hand, this means that code is broken anyway,
 so one extra vector copy is not a big deal.

 > 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.
 Ok, done. Now it makes sense, when class variables are set.

 >
 > 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) {
 >         ...
 >     }
 > }
 > }}}
 Done. see void check() method.


 > >> '''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() :-)
 BTW do we want space after unary operator '!' Should it be

     {{{if (! data_.empty())}}}

 or

     {{{if (!data_.empty())}}}

 > '''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);
 > }}}
 Fixed. I'm wondering why this does compile for me. I really need to switch
 to a different version of gcc/g++. I'm currently using g++ 4.5.2 (the
 default in Ubuntu 11.04). It seems very liberal in what it accepts. This
 caused problems more than once...

 > >> '''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.)
 Hmmm. Maybe better approach would be to modify this method a bit, so len
 would mean "give me at most X bytes". If there are less bytes available,
 just send that.

 In the current form, I prefer to leave the test as it is. Otherwise we
 don't have such guarantee you mentioned. This operation is sufficiently
 trivial to consider is atomic - completely succeeds or completely fails.
 The only think we would have gained in removing this if statement is just
 a couple of bytes of compiled code.

 Changes pushed. commit-id 5d6c71aeb2575883488b2cde87501aa84260b1ab

 Please review.

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