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:6 stephen]:
 > Reviewed commit 9062cbabaeebead0b95bde1559d107fc35eeda1d
 >
 > '''src/lib/dhcp/option6_int.h'''[[BR]]
 > Option6Int first constructor: header comments do not mention exception
 thrown

 Fixed.

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

 Fixed.

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

 Fixed.

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

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

 I created ticket #2365 to fix this in all classes derived from Option. I
 think it is worth being separate task if this issue touches more than just
 Option6Int and Option6IntArray.

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

 Created ticket #2364.

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

 Fixed.

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

 I moved the comment to the other function.

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

 I was not guided by the coding standard here. Coding standard does not
 prohibit putting the explicit values AFAIK. I wanted to have the easy way
 to check that supplied enum value is within the range of expected values
 (using < operator). Although in most cases enum values will be assigned
 incrementally by the compiler if they are omitted in enum's declaration, I
 am not fully convinced that it is always the case. If I hadn't put
 explicit values I would have had to do checks on every supplied value
 against all defined enums to make sure that the value is correct.

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