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

BIND 10 Development do-not-reply at isc.org
Thu Nov 22 18:53:53 UTC 2012


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

 * owner:  stephen => marcin


Comment:

 Reviewed commit c84d8a4bcf3058d49c88962744102cb3efa7aa2c

 '''src/lib/dhcp/libdhcp++.cc'''[[BR]]
 unpackOptions6(): suggest the comment about removing the "else if" in
 future be an "@todo".

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

 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.

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


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

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

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

 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.

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


More information about the bind10-tickets mailing list