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

Comment (by stephen):

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

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

 Well spotted.  After some consideration, I've put this at the end of the
 rest of the special-case commits (in both dhcp6 and dhcp6).

 The reason that this parser is treated differently is that when the commit
 occurs, the only way to roll back the changes is to do the operation again
 - load the old set of libraries to replace the new ones.  This may not
 work; for example, if the old libraries were deleted on disk while open
 (so were finally deleted when the new libraries were loaded) and are now
 not available for loading.

 However, there is a longer-term problem because I've just realized that
 the parser for the MySQL data is of the same type - the "commit" closes
 the current database and opens a new one. At the moment the rollback for
 this is ignored, but it should be treated in the same way - but then how
 do we handle the case when we have two parsers whose commit() action can't
 be rolled back?  We may want to open a ticket for this.


 > '''src/bin/dhcp4/ctrl_dhcp4_srv.cc'''
 > dhcp4CommandHandler: Neat pick: if-else would be more readable if there
 was an empty line before ''else''.
 Added (in both dhcp6 and dhcp6).

 > '''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.
 Good point, changed as suggested.

 > '''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?
 Fixed.

 > Should be a new line added before the returned type and the function
 name for ''load'' and ''unload'', just like for other functions?
 Yes - changed.

 > '''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.
 We might want to raise a ticket for this.

 > 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.
 Really its a check on a pre-condition for the test, rather than an
 initialization.  However, you are right, it could be done at the start of
 each test.  As the GTest documentation does not make it clear that
 predicates can be used in test fixture constructors, the check has been
 placed in the fixture's !SetUp() method.

 > !HooksLibrariesParser class: The ''brief'' description should start with
 capital letter.
 Hoist by my own petard! Corrected.

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

 > HooksLibrariesParser::getLibraries: The doxygen convention to mark the
 parameter ''out'' is:
 Corrected.

 > There are two doxygen warnings which may be related to this ticket:
 These were corrected when #2982 was merged; this ticket was branched from
 master before then.

 > Should this ticket have !ChangeLog entry?
 Yes.  Suggested text is:
 {{{
 Added capability to configure the hooks libraries through the BIND 10
 configuration mechanism.
 }}}

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