BIND 10 #2491: Option Definitions: Implement option definitions for the rest of DHCPv6 standard options.

[BIND 10 Development]

#2491: Option Definitions: Implement option definitions for the rest of DHCPv6
standard options.
-------------------------------------+-------------------------------------
            Reporter:  marcin        |                        Owner:
                Type:  task          |  marcin
            Priority:  medium        |                       Status:
           Component:  Unclassified  |  reviewing
            Keywords:                |                    Milestone:
           Sensitive:  0             |  Sprint-DHCP-20121213
         Sub-Project:  DHCP          |                   Resolution:
Estimated Difficulty:  0             |                 CVSS Scoring:
         Total Hours:  0             |              Defect Severity:  N/A
                                     |  Feature Depending on Ticket:
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by stephen):

 * owner:  stephen => marcin


Comment:

 Reviewed commit 9430b1fef5b2cc99e97b4cfaf6ccd518bcd76198

 > '''src/lib/dhcp/libdhcp++.cc'''
 > This is nice idea but my concern is that the complete option definition
 consists of the parameters passed to the !OptionDefinition's constructor
 and the definition of record fields (obviously, for options containing
 record of fields). In such case you would have to look into the header
 file to get the general definition and then to initStdDefs6 to find out
 the record fields types. That's confusing.
 In that case, why not put both the definition of the record fields and the
 data (in an anonymous namespace) in a header file? (Incidentally, if you
 do this, then change the "name" field to "const char*" to avoid the
 possibility of a static initialisation fiasco.)

 I'm really thinking that when extending the option list, it is easier (and
 looks "better") to update a header file comprising little more that the
 data than it is to edit a static array within a function some 250 lines
 down inside a file.  However, I'm not dogmatic about this - I'll leave it
 up to you.

 > '''src/lib/dhcp/option_custom.cc'''
 > I have been thinking about it for a while and in fact I couldn't find
 any name that would give me 100% satisfaction. The !OptionRecord may be
 misleading because you may think that this class supports only those
 options that are of the OPT_RECORD_TYPE type while this class supports the
 whole variety of option types. I disagree with !OptionRecord. I tend to
 think about renaming it to !OptionGeneric or !OptionIndexed.
 !OptionGeneric seems best: !OptionIndexed suggests some form of indexing
 mechanism.  If we decide to rename, do it in a separate ticket.

 > '''src/lib/dhcp/option_data_types.cc'''
 > Agreed. I removed the part of the comment about efficiency.
 Also suggest instead of "Copy the data from a buffer to an
 !InputBuffer..." the comment starts "Set up an !InputBuffer" as there is
 no copy operation.

 >> '''src/lib/dhcp/tests/option_data_types_unittest.cc'''
 >> writeAddress(): if #2396 is reviewed and completed before finishing
 this ticket, it provides "IOAddress::toBytes()" that will eliminate the
 need for this method.
 > I think it is worth to do this little change in a merge time. As long as
 the review is in progress let's avoid merging master to trac2491 branch so
 as the list of commits on the branch is not polluted by changes on master.
 Fair enough: #2396 has been merged now.  On reflection, I suggest making
 any changes related to the new IOAddress methods in a separate ticket.

 > I haven't implemented all option definitions so I am wondering if there
 is any sense to add !ChangeLog at this time. Maybe we could defer this
 until all option definitions are implemented? Comments?
 I would add a !ChangeLog saying that some option definitions has been
 implemented.  The subsequent !ChangeLog can just mention that additional
 option definitions have been added.  Can you propose a !ChangeLog entry
 for approval?

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