BIND 10 #2270: Configuration parser for DHCPv4 server

[BIND 10 Development]

#2270: Configuration parser for DHCPv4 server
-------------------------------------+-------------------------------------
            Reporter:  tomek         |                        Owner:
                Type:  task          |  marcin
            Priority:  medium        |                       Status:
           Component:  dhcpconf      |  reviewing
            Keywords:                |                    Milestone:
           Sensitive:  0             |  Sprint-DHCP-20121213
         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 tomek):

 * owner:  tomek => marcin


Comment:

 Replying to [comment:4 marcin]:
 > 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.
 Thanks.

 > '''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.
 I hope I have fixed all of them in src/bin/dhcp4. I do not want to touch
 src/bin/dhcp in this ticket, as this dir was split into dhcp and dhcpsrv
 already, so any extra changes here will cause the merge to be more
 difficult.

 > '''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?
 No. This will be fixed in a separate ticket. See #2540.

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

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

 > 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.
 Renamed to Dhcp4ConfigParser. The merge is planned after January release.

 > Why do you:
 > {{{
 > #include <string>
 > }}}
 > ?
 >
 > There is no reference to std::string in the header file.
 There is now. Uint32Storage and StringStorage types are moved to header
 file, so the containers can be used in tests.

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

 > I am wondering if there is a need to include all headers you're
 including.
 Some of the headers were removed. If there are others that can be removed,
 please let me know.

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

 > There are lots of warnings produced by doxygen about lack of reference
 to '''dhcpv4-config-inherit''' and '''dhcpv6-config-inherit''':
 They are gone now. I fixed some of the errors in dhcpv6 dir, but I did not
 want to fix them all to avoid merging issues. Created #2540 for this.
 >
 > There are also some other doxygen warnings that origin in
 config_parser.cc files.
 All doxygen warnings from src/bin/dhcp4 directory were fixed.

 > 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!''.
 This is tedious work, that ideally could be automated by doxygen. In
 theory it could parse the code and enumerate all possible exception types.
 Anyway, I've added them. I hope I didn't missed anything.

 > 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?
 Empty is a valid string. Higher level parsers should throw something if
 empty string is not acceptable in a given context.

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

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

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

 > 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.
 It wasn't as simple as it first looks.

 > 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.
 I've implemented new tests and actually found a bug with them. The code
 call insert() on uint32_defaults, but silently discards it if such a value
 exist already. So if a test (or a real server for that matter) call
 configureDhcpv4Server() only the first call will store defaults, the next
 one will just try and since the defaults are already there, new default
 values will be ignored.

 This applies to both v4 and v6. Fixed that in both.

 > 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'''
 I think I've fixed all Doxygen warnings in that directory.

 > '''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))
 > }}}
 Not really. If there is an unexpected exception thrown, the test fails
 anyway. ASSERT_NO_THROW is only useful if we want to continue the test
 (which is not the case here).

 > 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.
 Updated test names.

 > Dhcp4ParserTest::pool_prefix_len: You seem to have invalid comment here:
 > The comment says that you expect parse error but you actually expect
 rcode_ == 0.
 Fixed.

 > '''dhcp4.dox'''
 > Why do you say: ''DHCPv4 server component does not use BIND10 logging
 yet.'' ?
 > AFAIK it does use.
 It was left there for historical reasons. Removed.

 > Will be any changelog entry for this?
 There is one now. Thanks for the thorough review.

 All fixes pushed.

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