BIND 10 #2304: implement OptionDefinition class

[BIND 10 Development]

#2304: implement OptionDefinition class
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  stephen
  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 marcin):

 * owner:  marcin => stephen


Comment:

 Replying to [comment:4 stephen]:
 > 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).

 Changed.

 >
 > Member variable comment: typo - "cabveyed".

 Fixed.

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

 I have added comment and also I added unit tests for signed types to check
 that it actually works.

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

 Comment added.

 >
 > 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_".

 Fixed.

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

 I can't do this. The Option::pack() is packing the whole input buffer
 (with option data) and then sub-options. In the derived class I am packing
 option data myself. Calling pack() on the superclass will pack the input
 buffer once again (data_) and then sub-options. Calling
 LibDHCP::packOptions6 packs only options which is what I want. We could
 maybe add some new method Option::packOptions(...) and use it in derived
 classes. However this is not a change to be done in Option6Int and
 Option6IntArray only but in all derived classes.

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

 Fixed.

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

 No it is not. The array-type options should not include sub-options. I
 added some comment about it. The array-type may have variable number of
 fields (the number of fields is not specified). For this reason there is
 no way to distinguish the option data and sub-options position when such
 option is received.

 >
 > '''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);
 > }
 > }}}

 Changed.

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

 Thanks!

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

 Do you mean UNKNOWN_TYPE should be explicitly set to 13? If so, I have
 done it.

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

 Comment added.

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

 All factory functions need the same declaration. We declare Factory
 function type as follows (option.h, line 70):
 {{{
 typedef OptionPtr Factory(Option::Universe u, uint16_t type, const
 OptionBuffer& buf);
 }}}

 and then we use it to return pointers to various factory functions using
 this typedef. For example, see OptionDefinition::getFactory(), also see
 libdhcp++.h v4_factories_ declaration.

 I wanted to separate factoryAddrList4 and factoryAddrList6. My intention
 was that if user specifies '''ipv4-address''' as option type he gets
 dedicated factory function which will always create '''V4''' type of
 option.
 Otherwise someone may specify the option definition type as
 '''ipv4-address''', get the generic factory function and then create the
 V6 option instance through it. This is not a big deal but guarantees some
 protection against typos like "V6 vs V4" or "ipv4-address vs
 ipv6-address".

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

 I added some checks there.

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

 I wanted to check if readUintX will work well for signed types so I added
 some tests for signed integers.

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