BIND 10 #2270: Configuration parser for DHCPv4 server

[BIND 10 Development]

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

 * owner:  marcin => tomek


Comment:

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

 You have done it for DHCPv6 only.

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

 Instead of using C-style UINT32_MAX macro you could have used the
 '''numeric_limits''' trait class for integers. So, in Uint32Parser::build
 you would have:
 {{{
 if (check > std::numeric_limits<uint32_t>::max()) {
     isc_throw(BadValue, "Value " << value->str() << "is too large"
               << " for unsigned 32-bit integer.");
 }
 }}}
 This is a C++ style approach and does not require things like (line 15,
 config_parser.cc):
 {{{
 // We want UINT32_MAX macro to be defined in stdint.h
 #define __STDC_LIMIT_MACROS
 }}}

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

 FYI... I had discovered this issue in V6 config parser some time ago and
 fixed it. This means that since you have done similar change you are going
 to have merge conflicts in V6.

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

 This will not show up as a warning but rather a duplicated documentation
 in the html. You have two '''@brief''' tags (one in config_parser.h,
 another in config_parser.cc) that describe the same function:
 configureDhcp4Server. The html output for this function looks like this:
 {{{
 configures DHCPv4 server

 Configure DHCPv4 server (Dhcpv4Srv) with a set of configuration values.

 ...
 }}}
 I suggest that @brief tag is removed from .cc file. If you want to keep
 some description of this function in a .cc file anyway you could make it
 non-doxygen documentation by removing one slash.

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

 You still have '''bugus_command''' and '''subnet_global_defaults'''.

 > > Will be any changelog entry for this?
 > There is one now.

 Changelog looks good.

 ----

 Additional comments after the second review:
 '''config_parser.cc'''
 You now clear the global storage for uint32 and string values every time
 the configureDhcp4Server is called. I already had similar idea for DHCPv6
 and had to back off changes because this causes problems when multiple
 config requests are issued. Note that the configuration string is
 '''partial''' so if you clear the storage you remove the configuration
 that you had entered earlier and only the recently configured parameter is
 up-to-date. I suggest that containers are not cleared but their values are
 rather overriden in a map with [] operator.

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