BIND 10 #2304: implement OptionDefinition class

[BIND 10 Development]

#2304: implement OptionDefinition class
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  marcin
  marcin                             |                Status:  reviewing
                       Type:  task   |             Milestone:  Sprint-
                   Priority:  high   |  DHCP-20121018
                  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 stephen):

 * owner:  stephen => marcin


Comment:

 Reviewed commit 85627bca2677b4e68d67c8ecd35fbfe409eda251

 (I've made some trivial changes to a class header and pushed.)

 '''src/lib/dhcp/option6_int.h'''[[BR]]
 Option6Int constructors: a better error message associated with the
 throwing of the exception would be "non-integer type" (as "float" or
 "double" are numeric types but would also trigger the exception).

 Member variable comment: typo - "cabveyed".

 pack()/unpack(): need to add a comment saying that {read,write}UintXX will
 work for signed data (!OptionDataTypes supports signed integers as well).
 (Actually, you should check that this is the case.)

 unpack(): I would suggest adding a "todo" comment to say that we need to
 decide what to do if the option data is longer than the data type.  For
 now, ignoring the excess is OK.

 I'm not really keen on using protected member variables - it makes the
 class very vulnerable to changes in the superclass.  I suggest using
 "getType()" instead of "type_".

 Rather than call the static function LibDHCP::packOptions6() (and thereby
 having to reference the superclass's options_ variable), can't you just
 call the superclass "pack()" method?  The same goes for calling
 unpackOptions6() (the superclass has an unpack() method as well).

 '''src/lib/dhcp/option6_int_array.h'''[[BR]]
 Same comment as above regarding "non-numeric type" message.

 Is the pack() method missing a call to the superclass's pack() method?
 (This is done for Option6Int, but not here.)  The same applies to
 unpack().

 '''src/lib/dhcp/option_data_types.h'''[[BR]]
 Nice idea!

 '''src/lib/dhcp/option_definition.cc'''[[BR]]
 haveIA6Format() and haveIAAddr6Format() could be combined and simplified:
 {{{
 bool
 optionDefinition::haveIAx6Format(OptionDefinition::DataType first_type)
 const {
    return (haveType(RECORD_TYPE) &&
            record_fields_.size() == 3 &&
            record_fields_[0] == first_type &&
            record_fields_[1] == UINT32_TYPE &&
            record_fields_[2] == UINT32_TYPE);
 }

 bool
 optionDefinition::haveIA6Format() const {
    return (haveIAx6Format(UINT32_TYPE);
 }

 bool
 optionDefinition::haveIAAddr66Format() const {
    return (haveIAx6Format(IPV6_ADDRESS_TYPE);
 }
 }}}

 '''src/lib/dhcp/option_definition.h'''[[BR]]
 I've tidied up the function header a bit and pushed the changes.

 The !DataType enum values need to be explicitly specified (the BIND 10
 coding guidelines for this were changes).

 Should note that !DataTypeUtil constructor is private to ensure that only
 one instance can be created.

 If factoryAddrListX()'s "u" argument must be a given value, why supply it?
 And what happens if it isn't - is an exception thrown?  Wouldn't it be
 better to make these methods private, and supply a generic
 factoryAddrList() whose "u" argument choses between the universe-specific
 methods?

 '''src/lib/dhcp/tests/option6_int_array_unittest.cc'''[[BR]]
 bufferToUintXX test: comment says that constructor may throw if the buffer
 is too short: it would be useful to check that.

 '''src/lib/dhcp/tests/option6_int_unittest.cc'''[[BR]]
 Agree that the tests should be extended for signed types.

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