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          |  stephen
            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 marcin):

 * owner:  marcin => stephen


Comment:

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

 I created a new header file '''std_option_defs.h''' where I created a
 static table holding parameters that are used to create option
 definitions. For definitions being 'records' I added two additional fields
 (per definition) holding pointers to an array of data types carried by an
 option and the number of elements in this array. Also I added two macros,
 that I think, make the code a little more readable.
 The same header file will be used to define V4 option definitions.

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

 I have submitted #2548.

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

 > >> '''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 have submitted #2549.

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

 Here is a proposed ChangeLog entry:
 {{{
 XXX.    [func]          marcin
         Implemented definitions for DHCPv6 standard options identified
         by codes up to 48. These definitions are now used by the DHCPv6
         server to create instances of options being sent to a client.
         (Trac #2491, git XYZ)
 }}}

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