BIND 10 #2314: Namespace encapsulation by Option

[BIND 10 Development]

#2314: Namespace encapsulation by Option
-------------------------------------+-------------------------------------
            Reporter:  marcin        |                        Owner:
                Type:  task          |  stephen
            Priority:  medium        |                       Status:
           Component:  dhcp          |  reviewing
            Keywords:                |                    Milestone:
           Sensitive:  0             |  Sprint-DHCP-20130122
         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:9 stephen]:
 > '''src/bin/dhcp4/config_parser.cc'''[[BR]]
 > appendSubOptions: if the pointer is invalid, should an exception be
 thrown?

 For Config Parsers I used to use asserts to guard against programming
 error. In fact, the NULL pointer in the appendSubOptions functions is also
 a programming error because in theory the NULL pointer shouldn't be stored
 in the container. If there is one, this is a serious error. For this
 reason I added an assert.

 >
 > appendSubOptions: s/option definition fo the particular/option
 definition for the particular/

 Fixed.

 >
 >
 > '''src/bin/dhcp6/config_parser.cc'''[[BR]]
 > Same comments as for the V4 version.

 Fixed.

 >
 >
 > '''src/lib/dhcp/option_definition.cc'''[[BR]]
 > In the third constructor (starting at line 63), can't you initialize the
 value of type_ to OptionDataTypeUtil::getData(type) instead of setting it
 in the constructor?
 >

 Moved to the constructor initialization list.

 >
 > '''src/lib/dhcp/option_definition.h'''[[BR]]
 > Most BIND 10 code, when returning a string, returns a string by value.
 I suggest we do the same here.  (The argument is that a (futurte) caller
 would be able to set up a reference to an internal member variable, which
 would disappear when the object goes away.)  As an asie, I'm not sure that
 returning a reference in this case gives us much.  Compilers are allowed
 to do return value optimisation, so the overhead should be small.

 Fair enough. Now, functions return strings by value.

 >
 > '''Copyright Notices'''[[BR]]
 > Need to update the copyright date to 2013 in:
 > * src/lib/dhcp/option_custom.h
 > * src/lib/dhcp/option_definition.cc
 > * src/lib/dhcp/tests/option_definition_unittest.cc
 > * src/lib/dhcp/tests/libdhcp++_unittest.cc

 Updated.

 >
 > '''Documentation'''[[BR]]
 > The documentation can be added as part of #264.

 Was that #2642?

 Proposed !ChangeLog entry:
 {{{
 XXX.    [func]          marcin
         The encapsulated option space name can be specified for
         a DHCP option. It comprises sub-options being sent within
         an option that encapsulates this option space.
         (Trac #2314, git abcdef)
 }}}

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