BIND 10 #2312: Custom Option implementation

[BIND 10 Development]

#2312: Custom Option implementation
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  stephen
  marcin                             |                Status:  reviewing
                       Type:  task   |             Milestone:  Sprint-
                   Priority:         |  DHCP-20121129
  medium                             |            Resolution:
                  Component:  dhcp   |             Sensitive:  0
                   Keywords:         |           Sub-Project:  DHCP
            Defect Severity:         |  Estimated Difficulty:  0
  Medium                             |           Total Hours:  0
Feature Depending on Ticket:         |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------
Changes (by marcin):

 * owner:  marcin => stephen


Comment:

 Replying to [comment:7 stephen]:
 > Reviewed commit 863da09e4d4ef50693860dd95948ae5171ba5116
 >
 > '''General'''[[BR]]
 > I've corrected a few typos and missing Doxygen keywords, and pushed the
 modified files back to the repository.

 Ok. Thanks!

 >
 > '''src/lib/dhcp/option_custom.h'''[[BR]]
 > !OptionCustom constructors: If !OptionCustom is used by the server, then
 supplying iterators to the first and last elements that should be copied
 as constructor arguments seems to assume that I know the data and that
 data is going to be sent.  However, the header indicates that that data
 supplied to the constructor in this manner is used for received options.
 A bit more explanation would be helpful.

 I updated the doxygen comments and I am hoping now it is clearer.

 >
 > readAddress(): more a question of style, but wouldn't it be more logical
 to have readAddress return an IOAddress through the return value rather
 than passing back through an already-created IOAddress object?

 I thought that it is better to avoid additional copy construction of the
 object but finally
 I decided to follow your suggestion for consistency and ease of use.

 >
 > readString(): would it be better to return the string through the
 function return value?

 Same as above.

 >
 > '''src/lib/dhcp/option_custom.cc'''[[BR]]
 > Constructors: looking at the definition of the constructors, they appear
 more or less equivalent.  In which case, how is the send/receive
 distinction referred to in the header comments (in the .h file) justified?

 As I mentioned above. I have put some additional comment in a header file.

 >
 > createBuffers() (line 103): the comments does not make sense.

 I updated the comment. Hopefully it has more sense now.

 >
 > createBuffers() (line 129): at the end of the do...while loop, no
 exception is thrown if the data is truncated. However, an exception is
 thrown in other places where data is truncated.

 I assume that a buffer is truncated to create an option with array of
 values if you can't make at least one valid value out of it. Otherwise it
 is ok that the buffer length is not divisible by the size of the element.
 The remaining part is ignored by intent.

 >
 > createBuffer() (line 134-136): This is a no-op if the data type is
 OPT_EMPTY_TYPE.  It would be logical to put this check (and the following
 if/else clause at lines 136-140) within a check of data_type.

 Ok. Instead of putting it on the beginning I changed the global if
 statements so as they exclude OPT_EMPTY_TYPE. That's pretty much the same
 but there was no sense to create if statement for no-op.

 >
 > pack4()/pack6(): are we sure that an !OptionBuffer can't be empty?  If
 it is, then the reference to (*it)[0] could be invalid.

 In theory it shouldn't because it is being validated by createBuffers but
 ... I added a check to be sure.

 >
 > dataFieldToText()/toText(): would prefer that the string stream variable
 be named something like "text" than "tmp".  "tmp" tends to be used in
 throwaway code or in situations where a variable is needed for a couple of
 lines.  This variable persists through the entire method.

 Renamed.

 >
 > toText(): I suggest removing the "= 0" in the parameter list. The
 default value of the "indent" parameter does not affect the operation of
 the method.

 Removed as it was copied from option.cc implementation. However I am not
 exactly sure your motivation to remove it. IMHO, It does affect the
 operation of the method because the text will be printed out in a
 different position.

 >
 > '''option_data_types.h'''[[BR]]
 > !OptionDataTypeUtil: Suggest that the class header include an
 explanation that the class is a singleton, but is only accessed through
 static class methods.

 Added.

 >
 > readAddress(): would not this be better returning an IOAddress through
 the function return value?

 Already discussed above.

 >
 > writeXxxx(): The header comments should make it clear that these methods
 append the data to the end of the passed buffer rather than overwriting
 the entire buffer.

 Added.

 >
 > writeInt(): should add a comment about converting to an unsigned integer
 will be OK even for signed types.

 Added in the function body.

 >
 > readString(): why not return a std::string as much of the rest of the
 BIND 10 code does in similar situations?

 Discussed above.

 >
 > '''src/lib/dhcp/option_data_types.cc'''[[BR]]
 > !OptionDataTypeUtil constructor: there is a mapping for OPT_UNKNOWN_TYPE
 -> "unknown", but not vice-versa.

 That is ok. I added a comment to explain why.

 >
 > getDataTypeLen(): suggest adding a blank line before each set of "case"
 statements (apart from the first) - it looks clearer.

 Added.

 >
 > readAddress(): error at line 124 - the check should be "else if (family
 == AF_INET6)"

 Added.

 >
 > writeAddress(): The functionality of converting an IP address to a
 string of bytes really belongs in the IOAddress class.  We already have a
 ticket for this - #2396, which adds an "IOAddress::toBytes()" method.
 Once this is done, writeAddress() becomes relatively trivial.  I suggest
 we add #2396 to the sprint, implement that, then use it in this method.

 That functionality will be useful indeed to simplify this method. However
 I don't want to rely on #2396 to merge this code to master so I added todo
 statement and once this function is implemented it can be changed.

 >
 > writeBool(): This could be simplified to a single line:
 > {{{
 > buf.push_back(static_cast<uint8_t>(value ? 1 : 0));
 > }}}

 Changed.

 >
 > readString()/writeString(): these are essentially doing the same task
 (appending an array of bytes to another array) but are doing it in
 different ways.  They should be consistent.

 They are now consistent.

 >
 > '''src/lib/dhcp/option_definition.cc'''[[BR]]
 > addRecordField() (line 68): as the code is making an assumption about
 the ordering of the OPT_xxx symbols (i.e. that there are no simply types
 with a value higher than OPT_RECORD_TYPE), a comment to that effect should
 be added to the defintion of !OptionDataType in option_data_types.h.

 I added a comment.

 >
 > writeToBuffer() (line 352 on): suggest using IOAddress::getFamily()
 rather than extracting the underlying asio::ip address type.
 (Alternatively, #2396 could be extended to add isV4() and isV6() methods
 to IOAddress.)

 I am now using getFamily function.

 >
 > '''src/lib/dhcp/tests/option_custom_unittest.cc'''[[BR]]
 >
 > All tests for different data types should include a check that an
 exception is thrown if the buffer is too short. (It's done in a couple of
 places, but not everywhere - e.g. checking that the parsing of a V6
 address will thrown the exception if the buffer is too short).

 I added checks in every test case.

 >
 > ipv4AddressDataArray test (line 487): not sure why the end is written as
 "buf.begin() + 13" instead of buf.end(). (Similar comment in the
 ipv6AddressDataArray test.)  Both tests should also check what happens if
 the array size is not a multiple of the address size - does it throw a
 "data truncated" exception?
 >

 Yes, this is an error. I now use the constructor that takes whole buffer
 rather than a pair of iterators. I use a pair of iterators when I want to
 simulate truncated buffer.

 The proposed ChangeLog entry:
 {{{
 XXX.    [func]          marcin
         Implemented an OptionCustom class for DHCPv4 and DHCPv6.
         This class represents an option which has a defined
         structure: set of data fields of specific types and order.
         Those fields can be accessed using indexes, where index 0
         represents first data field and index equal to 'number
         of fields' - 1 represents the last data field. This class
         is used to represent those options that can't be represented
         by any other specialized class.
         (Trac #2312, git ...)
 }}}

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