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]:
 > Ready to review. This is a bit larger than anticipated (and may look a
 bit daunting), so I suggest the following be done first:
 "Bit larger" is an understatement. The complete diff for trac2980 is 12229
 lines long. Even without the first commit that moves data between
 src/lib/util/hooks to src/lib/hooks, the diff is 5850 lines long. That's
 definitely too large. Please take that into consideration the next time
 you ask why are we doing so few tickets per sprint.

 > * An overview of the hooks framework for BIND 10 developers who wish to
 use hooks in a BIND 10 component is in
 hooks/hooks_component_developer.dox.  I suggest that you run doxygen and
 read the formatted output.
 '''src/lib/hooks/hooks_component_developer.dox'''
 General comment: the document should stay at the very beginning that it is
 intended for BIND10 developers, not for user-side hook developers. (I
 imagine that such a document will be created later). It would be good to
 add a stub now to show that such a document is planned. This also applies
 to main page (doc/devel/mainpage.dox).

 Basic Ideas section
 "The hook points need to be defined" => " and registered."

 "At each hook point, the component needs to:" => "At each hook point, the
 component needs to complete the following steps to execute callouts
 registered by user-library:"

 Please use numbered list - it would be easier to reference to specific
 steps. Very important first step is omitted - check if there are any user
 callouts registered on a given hook point. If there aren't any, skip all
 remaining steps. That's important step from performance perspective.

 Defining the Hook Points section
 It should be clarified that hook registration and callout registration are
 two completely separate things. When I just started implementing first
 hooks, I confused them couple of times. Something along the following text
 would be helpful "There are two types of registrations. The first is hook
 registration. Server engine uses this procedure to expose which hook
 points are available (i.e. where users can install their callouts). the
 second type is callout registration. That procedure is used to register a
 callout on a previously defined hook point and is typically used by the
 user library."

 The first example code in Defining the Hook Points has extra colon after
 #include <hooks/hooks_manager.h>.

 "TBD: do we still want this given the possibility of confusion with
 functions in system libraries?"
 Yes. We need to solve that problem somehow anyway to call the proper
 version(), load() and unload() functions.

 Calling Callouts on a Hook
 The text should mention how to check if there are any callouts installed.

 The text should make clear that the CalloutHandle is passed to all
 callouts installed on a given hook point. Callouts are executed in order
 (explain the order or point out to a text that explains the order) and
 mention that any argument value updated by callout1 will be seen by
 callout2.

 The example should not have "..." instead of the call the callouts. Also,
 the comment ("call the hook code...") is incorrect. It should be something
 like "execute installed callouts".

 "SHOULD WE GET RID OF THE SKIP FLAG AND WHERE APPROPRIATE, SIGNAL SUCH
 PROCESSING THROUGH AN ARGUMENT?"
 I ny opinion no. The boolean flag names skip is simple enough. The design
 is not complete when there's nothing else to add, but when there's nothing
 left to take away. You can't get any simpler than a boolean flag. It is
 also very easy to understand in most contexts what the flag does. For int
 status code passed via arguments, you'd have to define an elaborate status
 code tables that could change their meaning between hook points. I think
 we should leave the code as it is now. If there are strong objections from
 users/customers backed by real life examples, we may reconsider our
 decision.

 Oh, I see that there is a section about conditionally calling callouts.
 You can disregard my previous comments on such need.

 > For the rest of the code in hooks, the following review order is
 suggested, as each item uses the functions of previous ones:
 >
 > * First review the differences in server_hooks.{cc,h}.  These are a
 follow-on from the previous hooks ticket (#2974) and involve making the
 !ServerHooks object a singleton.
 '''server_hooks.h'''
 Please fix a typo in ServerHooks() ctor: ggetServerHooks() =>
 getServerHooks().

 I see that the HookRegistrationFunction class disappeared. Was it moved
 somewhere or just removed? I'm ok with that if it is gone completely.

 > * callout_handle.* contain only minor changes.  These allow a callout to
 get the name of the hook from which it is being called, and store a
 pointer to a library collection object for lifetime purposes (described in
 the .dox file).
 '''callout_handle.h'''
 The comment for CalloutHandle (the last bullet, starting with "Per-library
 handle" suggests that CalloutHandle allows user libraries to register or
 deregister callouts. That's not the case. I think the whole paragraph can
 be safely removed.

 '''callout_handle.cc'''
 Comment in ~CalloutHandle: typo in "lm_collection_, wn action".

 '''callout_handle_unittest.cc'''
 There are no tests for getHookName().

 The review will continue tomorrow.

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