BIND 10 #2490: Option definitions: multiple data formats to be accepted to create option instance.

[BIND 10 Development]

#2490: Option definitions: multiple data formats to be accepted to create option
instance.
-------------------------------------+-------------------------------------
                   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:  N/A    |  Estimated Difficulty:  0
Feature Depending on Ticket:         |           Total Hours:  0
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------
Changes (by marcin):

 * owner:  marcin => stephen


Comment:

 Replying to [comment:5 stephen]:
 > Reviewed commit c84d8a4bcf3058d49c88962744102cb3efa7aa2c
 >
 > '''src/lib/dhcp/libdhcp++.cc'''[[BR]]
 > unpackOptions6(): suggest the comment about removing the "else if" in
 future be an "@todo".
 Added!

 >
 > initStdOptionDefs6(): Suggestion - unless the order matters, I'd suggest
 that the elements of the "params" array be ordered alphabetically by
 option name. (You may want to see if aligning the elements in columns
 makes it more readable.)

 They are sorted by their codes. This is intentional because it helps me to
 confront for which options definitions are already implemented, for which
 they aren't. I am using the DHCPv6 option list on iana.org as a reference.
 On this site they list options by codes.

 >
 > initStdOptionDefs6(): You say a validation error is a programming error
 and should never happen, so wouldn't throwing an exception be better?
 That way the caller is guaranteed that the return value is valid.

 Ok. Maybe throwing exception has more sense. I now throw exception.

 >
 > '''src/lib/dhcp/option_data_types.h'''[[BR]]
 > I'm not certain that setting "len" to the size of IOAddress or string is
 useful.  As far as I can see, "len" is only used to determine what
 function to use when writing data to a buffer.  It is unlikely - but
 possible - that some implementations of string have a size of 4 bytes. I
 would suggest setting their length to 0 (on the grounds that if data in a
 string has to be written, the amount of data won't be sizeof(string) -
 similarly for IOAddress) or choosing which function to use on the basis of
 !OptionDataTypeTraits<T>::type
 >

 Set to 0.

 >
 > '''src/lib/dhcp/option_definition.cc'''[[BR]]
 > writeToBuffer(): "case OPT_UINT8_TYPE" clause - the cast should be to
 uint8_t, not int8_t.
 >

 Good catch. Fixed.

 > !Address/String types: nice use of copy_backward to avoid the need to
 know how much the buffer had been extended by - I've not come across that
 STL function before.
 >
 Actually, I have realized that STL has this function quite recently.

 > OPT_STRING_TYPE: suggest value.size() is used instead of valid.length(),
 to be consistent with other uses.

 Changed.

 >
 > optionFactory: just a comment that the "try" block is a lot of solid
 lines of code.  I would suggest introducing a blank line before each "else
 if" statement in the main "if" block and seeing if it is easier to read.

 Blank lines added.

 Here is a Changelog entry I am proposing:
 {{{
 509.    [func]          marcin
         DHCPv6 option instances can be created using a collection of
 strings.
         Each string represents a value of a particular data field within
         an option. The data field values, given as strings, are validated
         against the actual types of option fields specified in options
         definitions.
         (Trac #2490, git c7defffb89bd0f3fdd7ad2437c78950bcb86ad37)

 }}}

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