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:6 marcin]:
 > > > 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:
 > You have done it for DHCPv6 only.
 Uh oh. Ok, it is fixed now. Protected access is no longer used in in
 config_parser anymore in either dhcp4 nor dhcp6.

 > 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.");
 > }
 I didn't know that. Macro removed and the code now uses numeric_limits
 class.

 > > 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.
 Ok. Removed that clear. The code now uses [] operator, so it is no longer
 necessary.

 I fixed this in dhcp4 only to avoid merging problems in v6 code.

 > 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.
 Ok, removed.

 > You still have '''bugus_command''' and '''subnet_global_defaults'''.
 Not anymore.

 > 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.
 Good point. I've updated both dhcp4 and dhcp6, even it that means extra
 work with merging.

 Ok, I hope I haven't missed anything this time.

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