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:
                Type:  defect        |  stephen
            Priority:  medium        |                       Status:
           Component:  dhcpconf      |  reviewing
            Keywords:                |                    Milestone:
           Sensitive:  0             |  Sprint-DHCP-20130523
         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 tmark):

 * owner:  tmark => stephen


Comment:

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

 Got it.

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

 Good catch. Got it.


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

 Got it.  That does it, I'm writing a "checker" script to look for TKM and
 #if 0,
 two things I use when working code, to make sure I don't miss them.

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

 Got it.

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

 Done.


 > globalContext() should be declared to return a reference.

 Done.

 >
 > '''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
 >    :
 > }
 > }}}

 Done.

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

 Done.

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