BIND 10 #2270: Configuration parser for DHCPv4 server

[BIND 10 Development]

#2270: Configuration parser for DHCPv4 server
-------------------------------------+-------------------------------------
                   Reporter:  tomek  |                 Owner:  tomek
                       Type:  task   |                Status:  reviewing
                   Priority:         |             Milestone:  Sprint-
  medium                             |  DHCP-20121018
                  Component:         |            Resolution:
  dhcpconf                           |             Sensitive:  0
                   Keywords:         |           Sub-Project:  DHCP
            Defect Severity:  N/A    |  Estimated Difficulty:  0
Feature Depending on Ticket:         |           Total Hours:  0
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------
Changes (by marcin):

 * owner:  marcin => tomek


Comment:

 Reviewed commit 142825f0caeba214a4d1884be4067c62d1cc3551

 I also fixed some minor typos in a few files and corrected the copyright
 header in '''config_parser.cc'''. These changes have been pushed to the
 branch.

 '''General (applies to all files)'''
 According to BIND10 coding guidelines, whenever possible we should be
 including project headers first, then library headers and finally system
 headers.
 See: http://bind10.isc.org/wiki/CodingGuidelines#OrderingIncludeFiles

 This is not a critical, so you may drop this for now. However it may be
 worth to consider applying this order in the future work.

 '''cfgmgr.h and cfgmgr.cc'''
 Minor issue: but we used to make files comprising !''manager!'' classes
 with underscore. So we have lease_mgr.h, iface_mgr.h etc. This is not
 followed by Configuration Manager, so we have '''cfgmgr.h and cfgmgr.cc'''
 instead of '''cfg_mgr.h and cfg_mgr.cc'''. Is there any particular reason
 for this?

 '''config_parser.h'''
 Lack of indentation in the definition of '''Dhcp4ConfigError'''
 constructor.

 No doxygen documentation for '''DhcpConfigParser''' class.

 I can see that there is !''todo!'' comment saying that
 '''bin/dhcp4/config_parser.h''' and '''bin/dhcp6/config_parser.h''' will
 be merged. However I am suggesting that if it is going to take !''long!''
 it would be better to rename DhcpConfigParser classes to something like
 Dhcp4ConfigParser and Dhcp6ConfigParser for DHCPv4 and DHCPv6 server
 respectively. If it is going to be merged soon it should be ok to leave it
 as is to avoid overhead of refactoring. The primary reason for renaming is
 that the doxygen tries to combine the documentation for
 '''DhcpConfigParser''' classes and you end up having descripton of each
 member function duplicated.
 For example: open doxygen documentation -> Files -> dhcp4/config_parser.h
 -> class isc::dhcp::DhcpConfigParser. You're taken to the documentation of
 the class that comprises documentation of both headers while you would
 expect documentation of the class that is declared in DHCPv4 header.
 Again, it is not !''must have!'' for this review.

 Why do you:
 {{{
 #include <string>
 }}}
 ?

 There is no reference to std::string in the header file.

 '''config_parser.cc'''
 Copyright header updated with the current year '''2012'''.

 I am wondering if there is a need to include all headers you're including.
 I did not go through all of them but as an example... You include
 '''subnet.h''' that itself includes:
 {{{
 #include <boost/shared_ptr.hpp>
 #include <asiolink/io_address.h>
 #include <dhcp/pool.h>
 #include <dhcp/triplet.h>
 }}}

 Inclusion of those could have been avoided in '''config_parser.cc'''

 I understand the motivation to make some class members and methods
 '''protected''' instead of '''private''' for unit testing. However, there
 are some places in the code where you don't need to do it. The following
 members in DebugParser could be '''private''' with no effect on unit-
 testing:
 {{{
     /// name of the parsed parameter
     std::string param_name_;
     /// pointer to the actual value of the parameter
     ConstElementPtr value_;
 }}}
 The same applies to other parser classes.

 There are lots of warnings produced by doxygen about lack of reference to
 '''dhcpv4-config-inherit''' and '''dhcpv6-config-inherit''':
 {{{
 dhcp4/config_parser.cc:145: warning: unable to resolve reference to
 `dhcpv4-config-inherit' for \ref command
 }}}

 There are also some other doxygen warnings that origin in config_parser.cc
 files.

 I am not sure if this is enforced in any coding/documentation guidelines
 but it helps a lot when a list of thrown exceptions is provided along with
 the function documentation. For example Uint32Parser::build could have
 something like this included in its documentation:
 {{{
 \throw isc::BadValue if supplied value could not be cast to uint32_t
 }}}
 Treat this as a suggestion, not a !''must have!''.

 Is this possible that the value supplied as '''value''' into
 StringParser::build is empty? If so, do we want to have sanity check on
 this?

 Constructor InterfaceListConfigParser: I think that '''isc::BadValue''' is
 more suitable when supplied value is different than expected.

 PoolParser::build: I think that isc::InvalidOperation is more suitable
 when '''pools_''' member is not initialized.

 Subnet4ConfigParser::build: the variables: uintParser, stringParser and
 poolParser should be renamed to uint_parser, string_parser and pool_parser
 to conform with the BIND10 coding style.

 Uint32Parser::build: The lexical_cast<uint32_t> and bad_lexical_cast
 exception do not guard you against someone passing negative value in the
 configuration. In other words, if I specify:
 {{{
 "valid-lifetime": -4000
 }}}
 this will be accepted by the Uint32Parser and the '''valid lifetime'''
 value will be configured to 4294963296.
 I suggest that given value is first cast to signed integer (maybe even
 int64) and appropriate sanity check is done before you cast to uint32_t.
 Since this is a configuration that is expected to change rarely the
 performance impact will be negligible.
 BTW, test cases should be extended to cover this as well. I modified one
 of the existing tests to pass negative value and this modified test
 passed.

 The documentation for '''configureDhcp4Server''' in '''config_parser.cc'''
 is redundant as it is already documented in '''config_parser.h'''. BTW,
 the doxygen produces warnings about the fact that you're documenting
 !''server!'' argument that is actually omitted in implementation. This
 would not happen if you just remove documentation from
 '''config_parser.cc'''

 '''config_parser_unittest.cc'''
 AFAIK the parseAnswer(...) may throw exception so you might want to add
 something like this in all tests where you don't want it to throw:
 {{{
 ASSERT_NO_THROW(comment_ = parseAnswer(rcode, x))
 }}}

 Naming of test cases: the typical naming convention we used to follow is:
 {{{
 TEST_F(Dhcp4ParserTest, subnetLocal)
 }}}
 while you have:
 {{{
 TEST_F(Dhcp4ParserTest, subnet_local)
 }}}
 The same comment applies to all other test cases.

 Dhcp4ParserTest::pool_prefix_len: You seem to have invalid comment here:
 {{{
 // returned value must be 1 (configuration parse error)
 ASSERT_TRUE(x);
 comment_ = parseAnswer(rcode_, x);
 EXPECT_EQ(0, rcode_);
 }}}
 The comment says that you expect parse error but you actually expect
 rcode_ == 0.

 '''dhcp4.dox'''
 Why do you say: ''DHCPv4 server component does not use BIND10 logging
 yet.'' ?
 AFAIK it does use.

 Will be any changelog entry for this?

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