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 9062cbabaeebead0b95bde1559d107fc35eeda1d

 '''src/lib/dhcp/option6_int.h'''[[BR]]
 Option6Int first constructor: header comments do not mention exception
 thrown

 Option6Int second constructor: header comments mention !OutOfRange
 exception but the constructor actually throws an !InvalidDataType
 exception.

 pack(): header comments states it throws a !BadValue exception but it
 actually throws !InvalidDataType.

 unpack(): header comments only menthion the !OutOfRange excaption: it also
 throws !InvalidDataType.


 >> 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.
 Option::pack() calls either Option::pack6() or Option::pack4() depending
 on the "universe_" option.  Each of these writes type, length and size
 before calling libDHCP::packOptions().  A way round this would be to
 extend the declaration of Option::pack/pack4/pack6 with an optional
 boolean "pack_header" argument (default to true): this code calls
 Option::pack() with a second argument of "false", indicating that the
 header has already been set.  In this way, you avoid referencing the
 superclass's options_ variable.

 (Incidentally, I notice that Option::pack4() includes code for the V6
 universe.  This seems to be a mistake - if you concur, can you open a
 ticket to remove it?)

 '''src/lib/dhcp/option6_int_array.h'''[[BR]]
 General: check the exceptions documented in the function headers with
 those thrown by the code -in a number of cases they are wrong or omitted.

 '''src/lib/dhcp/option_definition.cc'''[[BR]]
 The comment in haveIAx6Format really belongs in haveIA6Format.  Either
 that or it should be modified to indicate what the general check is doing.

 >> 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.
 Apologies - short-circuit somewhere between brain and keyboard.  What I
 really meant to say was that there is no need to explicitly set the values
 for the elements of an enum - the BIND 10 coding standards were changed on
 this point.

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