BIND 10 #2316: Extend "Subnet" with OptionCollection structure

[BIND 10 Development]

#2316: Extend "Subnet" with OptionCollection structure
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  stephen
  marcin                             |                Status:  reviewing
                       Type:  task   |             Milestone:  Sprint-
                   Priority:         |  DHCP-20121018
  medium                             |            Resolution:
                  Component:  dhcp   |             Sensitive:  0
                   Keywords:         |           Sub-Project:  DHCP
            Defect Severity:  N/A    |  Estimated Difficulty:  0
Feature Depending on Ticket:  2318   |           Total Hours:  0
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------
Changes (by marcin):

 * owner:  marcin => stephen


Comment:

 Replying to [comment:5 stephen]:
 > Reviewed commit 82c0737d3d63da1a1c2b001d0306a32a1e55f5f5
 >
 > '''src/lib/dhcp/subnet.cc'''[[BR]]
 > Subnet::addOption(), Subnet{4,6}::validateOption(): Suggest passing the
 "option" argument by reference (instead of value).  As !OptionPtr is a
 boost::shared_ptr, this saves the overhead associated with the copying of
 the shared_ptr object on entry (including the incrementing of the
 reference count) and its destruction (and decrement of the reference
 count) on exit.

 Agreed and fixed.

 >
 > Subnet{4,6}::validateOption(): as these methods don't alter the
 shared_ptr (nor the unerlying reference count), can the option argument be
 declared "const"?

 Yes, it should be const. Fixed.

 >
 > Subnet{4,6}::validateOption(): typo - "epected".

 Corrected.

 >
 > Subnet{4,6}::validateOption(): it occurs to me that if, within option.h,
 explicit numeric values were to be assigned to the members of the
 "Universe" enum:
 > {{{
 > enum Universe {
 >     V4 = 4,
 >     V6 = 6
 > };
 > }}}
 > ... then "validateOption()" could be put in the "Subnet" base class.  An
 extra argument for the universe expected would be used in the
 getUniverse() check, and the value of the argument, cast to an "int",
 would be used in the error message.

 This is one way to do this but it was my intention to force override of
 validateOption in the derived classes as I am expecting that there will be
 further specializations for Subnet6 and Subnet4. Also having the pure
 function truly prevents people from using Subnet class directly.

 >
 > '''src/lib/dhcp/subnet.h'''[[BR]]
 > !OptioNDescriptor constructor: header needs "@param" tags to describe
 the options.

 Fixed.

 >
 > !KeyFromKey header: not quite clear what is meant by "This class
 extracts one multi_index_container key with another key".  Does this mean
 that given one key, it returns a value that can then be sed as another
 key?

 I added some more comprehensive description.

 >
 > !KeyFromKey: as the members are private and only methods public, this is
 probably better declared as a "class" instead of a "struct".

 Changed.

 >
 > !KeyFromKey: thanks for the indented structure and detailed description
 of each part of the "index_by" template: it made it much easier to
 understand.  Just one suggestion: the comments refer to indexes !#1 and
 !#2: is the "sequenced<>" construct index !#0?  If so, I suggest that the
 comment mention it.

 I added some comment.

 >
 > getOptions(): should add in the "@return" comment that the reference
 returned is valid only for as long as the Subnet object remains in
 existence.

 Comment added.

 >
 > '''src/lib/dhcp/tests/subnet_unittest.cc'''[[BR]]
 > addPersistentOption test: probably want an unequal split the number of
 persistent and non-persistent options.  When testing a 5/5 split, it is
 possible that the both the function returning the count of persistent
 options and the one returning the non-persistent count are in fact
 returning the persistent count (or non-persistent count).

 Good catch. I changed the split to 3/7 and extended the comment how I am
 doing the split.

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