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-20130509
         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:

 This update includes the new changes as well as corrections to Stephen's
 review comments. First the new changes:

 ------------------------- NEW CHANGES --------------------------


 Replaced individual global storage variables, with single instance of new
 ParserContext class.  This is passed down into parsers rather than have
 them access global variables.

 Created Abstract classes for PoolParser, SubnetConfigParser in dhcpsrv,
 Added support of Dhcp4/subnet/interface configuration parameter. (Note,
 this allows it to be specified, does not implement listening on it, same
 as with Dhcp6, that needs to be done under a new ticket)

 Replaced use of raw pointers in most of the code.  Added Element type
 checks to Boolean and String parsers, corrected some commentary.

 Added basic parsers units tests in new file, dhcp_parsers_unittest.cc.
 There is preliminary support for higher order parsers (OptionDataParser
 ,OptionDefinitionParser), These could be expanded with more permutations.

 Heavier testing of the higher order parsers requires more thorough
 integration as they are reliant on each other as well as server-specific
 derivations which must be replaced with suitable stubs.  The tests that do
 exercise this level of testing exist as unit tests under both DHCP
 servers, however they are tied tightly to the server-specific derivations.


 Now the review response:
 -------------------------------------------------------------------------


 Replying to [comment:6 stephen]:

 > Reviewed commit 53ceb71e36d310a2a4cd5fef7ae9fa5d410f078a.
 >
 > '''src/bin/dhcp4/config_parser.cc'''[[BR]]
 > Please start comments with a capital letter.
 >
 Fixed.

 > Subnet4ConfigParser constructor: all parameters (even unused ones)
 should be listed in the header.
 >
 Fixed.

 > Subnet4ConfigParser::createSubnet4ConfigParser: in the "else if" block,
 the "else" should be on the same line as the preceding closing brace, i.e.
 "} else if (...) {"
 >
 Fixed.

 > Subnet4ConfigParser::createSubnet4ConfigParser: please extend the
 @return comment in the header to note that the caller is responsible for
 deleting the returned parser once it is no longer needed.
 >
 Fixed.

 > configureDhcp4Server: there are several debug statements outputting to
 cout here.
 >
 Fixed.

 > Dhcp4OptionDataParser methods: "return" statements should enclose the
 returned value in parentheses.
 >
 Fixed.

 > createGlobalDhcp4ConfigParser: in the "else if" block, the "else" should
 be on the same line as the preceding closing brace, i.e. "} else if (...)
 {"
 >
 >
 Fixed.

 > '''src/bin/dhcp4/config_parser.h'''[[BR]]

 Note, I moved Dhcp4OptionDataParser definition into config_parser.cc

 > Dhcp4OptionDataParser constructor: all arguments should be documented.

 Fixed.
 >
 > Dhcp4OptionDataParser constructor and "factory" method: attach the
 asterisk of a pointer declaration to the data type rather than the
 variable (i.e. use "type* var" rather than "type *var" - see below).
 >

 Replaced use of raw pointers with boost::shared_ptrs.

 > Dhcp4OptionDataParser: Do you need to define a private default
 constructor if a non-default one has been defined?
 >

 Have removed private default constructors throughout.

 > '''src/bin/dhcp6/config_parser.cc'''[[BR]]
 > Please start comments with a capital letter.

 Done.

 >
 > !PoolParser constructors: arguments (even unused arguments) should be
 described in the header.
 >
 Done.

 > Subnet6ConfigParser constructor: arguments (even unused arguments)
 should be described in the header.
 >
 Done.

 > Subnet6ConfigParser::createSubnet6ConfigParser: in the "else if" block,
 the "else" should be on the same line as the preceding closing brace, i.e.
 "} else if (...) {"
 >
 Done.

 > Subnet6ConfigParser::createSubnet6ConfigParser: please extend the
 @return comment in the header to note that the caller is responsible for
 deleting the returned parser once it is no longer needed.

 >
 Done.

 > Dhcp6OptionDataParser methods: "return" statements should enclose the
 returned value in parentheses.
 >
 Done.

 > Subnet6ListConfigParser constructor: arguments (even unused arguments)
 should be described in the header.
 >
 Done.

 > Dhcp6OptionDataParser methods: "return" statements should enclose the
 returned value in parentheses.
 >
 Done.

 > createGlobalDhcpConfigParser: in the "else if" block, the "else" should
 be on the same line as the preceding closing brace, i.e. "} else if (...)
 {"
 >
 Done.

 > createGlobalDhcpConfigParser: for symmetry, should consider renaming
 this createGlobalDhcp6ConfigParser.
 >
 Done. (FYI, I didn't pick this name ;))

 >
 > '''src/bin/dhcp6/config_parser.h'''[[BR]]
 As with dhcp4, I moved Dhcp6OptionsDataParser definition into
 config_parser.cc

 > Try breaking lines at 80 columns - there are only a couple of places
 where this is violated.

 Done. (It wasn't me!)
 >
 > Dhcp6OptionDataParser constructor and "factory" method: attach the
 asterisk of a pointer declaration to the data type rather than the
 variable (i.e. use "type* var" rather than "type *var" - see below).
 >

 Replaced use of raw pointers with boost::shared_ptrs.

 > Dhcp6OptionDataParser constructor: all arguments should be documented.
 >
 > Dhcp6OptionDataParser: Do you need to define a private default
 constructor if a non-default one has been defined?
 >

 Private default constructors removed throughout.
 >
 > '''src/lib/dhcpsrv/dhcp_parsers.h'''[[BR]]
 > !BooleanParser, !Uint32Parser and !StringParser appear to be identical
 apart from the data type of the storage_ member (and !XxxStorage argument
 to the constructor).  They could be replaced by a template class plus a
 set of typedefs.

 Will address this under ticket a separate ticket to reduce the number of
 deltas for this ticket.

 >
 > Multiple declarations: the de-facto standard when declaring pointers is
 to attach the asterisk to the data type, e.g. use
 > {{{
 > Uint32Storage* storage;
 > }}}
 > instead of the C-standard:
 > {{{
 > Uint32Storage *storage;
 > }}}
 > (This fails if you declare multiple variables on the same line, but our
 standard is not to do that.)  This happens at various points in the code.
 >

 This has been corrected throughout.  Most instances were replaced with
 switch to using boost::share_ptrs instead of raw pointers anyway.


 > !XxxParser constructor: second argument should be described in the
 header. Since a raw pointer is being passed, the description should also
 describe who is responsible for destroying the pointed-to object and
 whether the storage object has to remain in scope while an object derived
 from the parser class is active.
 >

 This has been corrected throughout.  Most instances were replaced with
 switch to using boost::shared_ptrs instead of raw pointers. There are only
 a few uses of raw pointers remaining that are part of the original
 implementation. These are creating parser and definition instances that
 are immediately wrapped in shared_ptrs, so they appear to be safe.


 > !OptionDataParser constructor: all parameters must be described in the
 header.
 >

 Fixed.

 > OptionDataParser::commit: suggest wrapping the header comments at 80
 characters.
 > src/lib/dhcpsrv/dhcp_parsers.h
 >

 Done.

 > Multiple classes: if a non-default constructor has been defined, do you
 need to define the default constructor? Isn't that only added if no
 constructor is present?
 >

 Private default constructors have been removed throughout.

 >
 >
 > '''src/lib/dhcpsrv/dhcp_parsers.cc'''[[BR]]
 > !XxxParser constructor: as the logic of checking whether the parameter
 name is empty is the same in each of the specialisations, putting the
 check into a private method of the template class would avoid the
 repetition of the error message in the code.
 >
 > XxxParser::commit(): these are all the same, so can be in the general
 template class.
 >
 > XxxParser::build(): these can be specialisations of the template.
 >
 >

 This applies primarily to the simple value type parsers
 (Uint32,Boolean,String). This effort will be done under a separate ticket
 to reduce the scope of deltas for this ticket.

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