BIND 10 #2312: Custom Option implementation

[BIND 10 Development]

#2312: Custom Option implementation
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  marcin
  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 stephen):

 * owner:  stephen => marcin


Comment:

 Reviewed commit 863da09e4d4ef50693860dd95948ae5171ba5116

 '''General'''[[BR]]
 I've corrected a few typos and missing Doxygen keywords, and pushed the
 modified files back to the repository.

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

 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?

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

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

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

 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.

 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.

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

 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.

 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.

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

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

 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.

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

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

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

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

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

 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.

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

 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.

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

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

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

 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?

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