BIND 10 #2981: Implement Hooks Framework (part 3) - BIND 10 configuration

[BIND 10 Development]

#2981: Implement Hooks Framework (part 3) - BIND 10 configuration
-------------------------------------+-------------------------------------
            Reporter:  stephen       |                        Owner:
                Type:  enhancement   |  stephen
            Priority:  medium        |                       Status:
           Component:  Unclassified  |  reviewing
            Keywords:                |                    Milestone:
           Sensitive:  0             |  Sprint-DHCP-20130821
         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 marcin):

 * owner:  marcin => stephen


Comment:

 Reviewed commit a6cd22451be6684f4bebbc34d5344371afdeaa4b

 Mostly minor issues below.

 All comments which refer to DHCP4, are also valid for DHCPv6.

 '''src/bin/dhcp4/ctrl_dhcp4_srv.cc'''
 configureDhcp4Server: Typo in !''...the same risk of failurre as doing the
 change.)!''. It should be !''failure!''.

 Regarding the following code:
 {{{
     // Now commit changes that have been validated but not yet committed,
     // and which can't be rolled back.
     if (hooks_parser_) {
         hooks_parser_->commit();
     }
 }}}

 Is this exception-free? I can see that the !HooksManager reports errors
 through the boolean status code. However, I am wondering if there is a
 guarantee that future changes to !HooksManager and !HooksParser will not
 cause it throw exceptions which we don't catch here. Thus, wouldn't it be
 better to try-catch here and log an error?

 Also, current code assumes that hooks_parser_->commit() is always
 successful (assuming that it is exception safe and errors are not
 propagated from !HooksManager to this function). Is this intended? This
 would mean that if the commit() fails the configuration is accepted. If
 this is intended, it deserves some comprehensive comment in the code.

 '''src/bin/dhcp4/ctrl_dhcp4_srv.cc'''
 dhcp4CommandHandler: Neat pick: if-else would be more readable if there
 was an empty line before !''else!''.

 '''src/bin/dhcp4/dhcp4_messages.mes'''
 DHCP4_RELOAD_FAIL: I suggest that this error message is renamed to
 something that better reflects what is being reloaded. For example:
 DHCP4_HOOKS_LIBS_RELOAD_FAIL. The current name suggest that DHCP4 server
 is being reloaded/restarted.

 '''src/bin/dhcp4/tests/callout_library_common.h'''
 The following statement in the file description needs to be fixed: !''The
 functions append a single to the single line in the file, creating the
 file if need be.!''. What !''single!'' do they append?

 Should be a new line added before the returned type and the function name
 for !''load!'' and !''unload!'', just like for other functions?

 '''src/bin/dhcp4/tests/config_parser_unittest.cc'''
 In most of the cases we start names of the tests with the lower case
 letter. However, I know that it is not followed everywhere.

 There is a common initialization for hooks tests:
 {{{
     std::vector<std::string> libraries = HooksManager::getLibraryNames();
 }}}
 Technically, it could be moved to the test fixture class constructor.

 '''src/lib/dhcpsrv/dhcp_parsers.cc'''
 !HooksLibrariesParser class: The !''brief!'' description should start with
 capital letter.

 !HooksLibrariesParser Constructor description: The name of the parameter
 is !''hooks-libraries!'', not !''hooks_libraries!''.

 HooksLibrariesParser::getLibraries: The doxygen convention to mark the
 parameter !''out!'' is:
 {{{
 /// @param [out] libraries List of libraries that were specified in the
 ///        new configuration.

 }}}

 There are two doxygen warnings which may be related to this ticket:
 {{{
 /home/marcin/devel/bind10/src/bin/dhcp6/dhcp6_hooks.dox:24: warning:
 unable to resolve reference to `hooksDevelopersGuide' for \ref command
 /home/marcin/devel/bind10/src/bin/dhcp4/dhcp4_hooks.dox:24: warning:
 unable to resolve reference to `hooksDevelopersGuide' for \ref command
 }}}

 Should this ticket have !ChangeLog entry?

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