BIND 10 #2980: Implement Hooks Framework (part 2) - library loading and unloading

[BIND 10 Development]

#2980: Implement Hooks Framework (part 2) - library loading and unloading
-------------------------------------+-------------------------------------
            Reporter:  stephen       |                        Owner:  tomek
                Type:  enhancement   |                       Status:
            Priority:  medium        |  reviewing
           Component:  Unclassified  |                    Milestone:
            Keywords:                |  Sprint-DHCP-20130703
           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
-------------------------------------+-------------------------------------

Comment (by tomek):

 Replying to [comment:2 stephen]:
 > * The largest change to callout_manager.h is an expansion of the
 documentation describing how it treats callouts and the valid values of
 library index.  The .cc file has fewer changes.
 '''callout_manager.h'''
 I would add a single sentence somewhere in the beginning of the class
 description that this class is part of server implementation and user
 library developers should not

 The comment for setNumLibraries() says that the initial value is only
 estimate. It is not. It's just out of the blue value that
 will be set to proper value once the configuration is loaded.

 There's extra space between std::vector<CalloutVector>  and hook_vector_;

 '''callout_manager.cc'''

 checkLibraryIndex()
 I would prefer to not have the else clause, just plain isc_throw(). That
 way the code is simpler to read. It doesn't have any real difference as
 both code versions will likely to be compiled to exactly the same code. I
 remember that you disagreed before, so I'm ok if you keep the code as it
 is now.

 callCallouts()
 I'm somewhat against doing the calloutsPresent() check. This should be
 done far earlier in the server code when the hook point is reached. If the
 code flow reached callCallouts() and there are no callouts present, that's
 actually a coding error. And a performance penalty as well - the server
 code that needlessly executed callCallouts already prepared arguments.

 I think it would be better to not call calloutsPresent() at all here.
 Alternatively, if you really want to, you can call it, but then throw
 exception if someone used callCallouts() without checking
 calloutsPresent() first. In a way that would enforce proper use of
 calloutsPresent() in the server code.

 I'd like to repeat my previous comment about unnecessary copy of
 hook_vector_[hook_index] to callouts. That's repeated for every hook point
 for every packet, just to be safe against (unlikely) case of a library
 installing callout on a hook point during callout execution on the same
 hook point. The simpler way would be to forbid installing callouts on a
 hook point while the callouts for that hook point are processed.

 '''tests/common_test_class.h'''
 I can't find Doxygen documentation for that class. The tags are in the
 file, but it doesn't appear on the Doxygen classes (or files) list.

 The review will continue tomorrow.

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