BIND 10 #2355: DHCP v4 and v6 parsers' unification should be considered

[BIND 10 Development]

#2355: DHCP v4 and v6 parsers' unification should be considered
-------------------------------------+-------------------------------------
            Reporter:  tomek         |                        Owner:  tmark
                Type:  defect        |                       Status:
            Priority:  medium        |  reviewing
           Component:  dhcpconf      |                    Milestone:
            Keywords:                |  Sprint-DHCP-20130523
           Sensitive:  0             |                   Resolution:
         Sub-Project:  DHCP          |                 CVSS Scoring:
Estimated Difficulty:  0             |              Defect Severity:  N/A
         Total Hours:  0             |  Feature Depending on Ticket:
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by stephen):

 * owner:  stephen => tmark


Comment:

 Reviewed commit 36f7b7fa7bddd07da90c9346d5d62dbed77c2a49.

 >> On Ubuntu 11.10 (building with g++ 4.6.1), I got a failure in the unit
 tests:
 >> :
 > I do not get this failure on OS-X. A clean pull and build on Ubuntu VM,
 also passes unit tests. Something is amiss with your environment?
 For documentation purposes, the problem was down to the fact that the test
 failed in a 32-bit environment.  The way the test was structured - trying
 to coerce the maximum uint32_t number plus one into a long - meant that it
 was invalid on a system where the maximum "long" value is less than the
 maximum "uint32_t" value.

 A modification was added to skip the test on 32-bit systems.  The rest of
 this review is on the updated code.

 '''src/lib/dhcp4/config_parser.{cc,h}'''[[BR]]
 Subnet4ConfigParser constructor - need to remove the documentation for the
 now non-existent second argument.

 I think that globalContext() should be declared "!ParserContextPtr&
 globalContext()" and pass back a reference to the internal pointer. (Yes,
 I know I got it wrong in my example.)  The reason can be seen at lines
 503-506: the requirement is to reset the global pointer to the
 original_context value.  However, the pointer to the global context on
 which reset() is called is a copy of the static boost::shared_ptr declared
 in the globalContext() function, not the actual static instance itself.
 The reset() only affects the former, not the latter.


 '''src/lib/dhcpsrv/tests/config_parser_unittest.cc'''[[BR]]
 Can take out the "#if 0" block of code now that the method of getting
 global parser context has changed.

 '''src/bin/dhcp6/config_parser.{cc,h}'''[[BR]]
 Subnet6ConfigParser constructor - need to remove the documentation for the
 now non-existent second argument.

 Subnet6ListConfigParser::build() - trailing space after "subnet" when
 creating the new parser.

 globalContext() should be declared to return a reference.

 '''src/lib/dhcpsrv/dhcp_parsers.cc'''[[BR]]
 ParserContext::operator=(): I presume that the intermediate "tmp" object
 is created to overcome the problems of self-assignment (i.e. "a = a").  It
 would be faster to use a construct of the form:
 {{{
 if (this != &rhs) {
    // Do the assignment
    :
 }
 }}}

 '''src/lib/dhcpsrv/tests/cfgmgr_unittest.cc'''[[BR]]
 The !CfgMgrTest constructor has a commented-out line of code.

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