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

 On Ubuntu 11.10 (building with g++ 4.6.1), I got a failure in the unit
 tests:
 {{{
 [ RUN      ] DhcpParserTest.uint32ParserTest
 dhcp_parsers_unittest.cc:164: Failure
 Expected: parser.build(int_element) throws an exception of type
 isc::BadValue.
   Actual: it throws nothing.
 }}}

 A review of the code led to the following comments:

 '''src/bin/dhcp4/config_parser.cc'''[[BR]]
 Rather than create the global variable global_context_ptr pointing to a
 parser context, it would be better to create a function that returns the
 variable, i.e.
 {{{
 ParserContextPtr globalContext() {
    static ParserContextPtr global_context_ptr(new
 ParserContext(Option::V4));
    return (global_context_ptr);
 }
 }}}
 This avoids any possibility of the "static initialization fiasco" by
 deferring initialization of global_context_ptr until everything else has
 initialized.  (As an aside, this also eliminates the need for the
 getGlobalParserContext() function for unit tests.)

 !ParserContext: a non-default copy constructor has been provided.  As
 these usually go hand-in-hand with the assignment operator, should one be
 provided here?  Or should at least an assignment operator be declared
 private to prohibit its use?

 Dhcp4OptionDataParser::factory - typo in the header: s/fo/of/

 Dhcp4OptionDataParser::constructor & factory - if the parameter is the
 global context for the DHCPv4 option data - and if that context is given
 by the global symbol global_context_ptr - why pass it as an argument?

 Pool4Parser::poolMaker - appears to be a missing line in the header
 description of the dummy parameter.

 Pool4Parser::poolMaker - opening brace should be on same line as method
 definition.

 Subnet4ConfigParser constructor: again, if the context is the global
 context, why pass it through as an argument?

 Subnet4ConfigParser commit: the dynamic pointer cast could return null if
 subnet_ is an incorrect type.  Perhaps throw an exception in this case?

 Subnet4ConfigParser::createSubnetConfigParser: minor thing - when breaking
 a function argument list across lines, subsequent lines are aligned under
 the character after the opening parenthesis, not under the parenthesis
 itself. (Also, the "config_id.compare" lines should line up.)

 Subnet4ListConfigParser constructor: typo in header s/ingored/ignored/


 '''src/bin/dhcp6/config_parser.cc'''
 Similar comments to dhcp4/config_parser.cc

 As an aside, a side-by-side comparison between dhcp4/config_parser.cc and
 dhcp6/config_parser.cc shows a lot of similarities.  Further merging of
 the code could be made - another ticket?



 '''src/lib/dhcpsrv/dhcp_parsers.cc'''[[BR]]
 The BIND 10 standard for function definitions is to put the return type on
 the line preceding the definition (see examples in the
 [wiki:CodingGuidelines coding guidelines]. (This refers to the declaration
 of the !ValueParser specialisations.)


 '''src/lib/dhcpsrv/dhcp_parsers.h'''[[BR]]
 Typos:
 s/paramaters/parameters/
 s/to  option/to option/ (twice)

 !ParserContext: a copy constructor usually goes hand-in-hand with an
 assignment operator.  If an assignment operator is explicitly not used,
 perhaps declarating it private?

 !ValueParser - What does TKM mean in the "@brief" line.

 !ValueParser - header is wrong - this is a template class, not the parser
 for the boolean type.

 It would be more logical (and easier to read) if the class !ValueStorage
 were defined earlier in the file, before !ValueParser.

 !OptionDataListParser constructor - variables should be aligned below one
 another, not below the parenthesis.

 A change in several classes has been to alter the declaration of the
 storage for !XxxxStorage to !XxxxStoragePtr.  Why the additional level of
 indirection?

 !PoolParser header. s/Otherwise exception/Ownerwise an exception/


 '''src/lib/dhcpsrv/subnet.{cc,h}'''[[BR]]
 Just as an aside, I would consider the the getIface() and setIface()
 methods sufficiently trivial to be put in the header.


 '''src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc'''[[BR]]
 booleanParserTest: after asserting that the parser.build() method works
 correctly, it would be logical to test that the value has been set
 correctly.

 booleanParserTest: "bool actual_value = ~test_value".  The "~" operator is
 a bit-wise complement operator.  As both actual_value and test_value are
 declared "bool", the correct operator is the "not" operator, "!".

 stringParserTest: after verifying that the parser builds with a non-string
 element, it would be worth checking what the value in the storage actually
 is after that step.

 uint32ParserTest: after building the parser with various values, it would
 be worth checking what the parser thinks the value is at that point.

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