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-20130717
           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
-------------------------------------+-------------------------------------
Changes (by stephen):

 * owner:  stephen => tomek


Comment:

 == Replying to comment:4 ==

 > "Bit larger" is an understatement.
 Sorry.

 > '''src/lib/hooks/hooks_component_developer.dox'''[[BR]]
 > General comment: the document should stay at the very beginning that it
 is intended for BIND10 developers, not for user-side hook developers.
 The "Introduction" section has been altered to emphasise this.

 > (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).
 It's already been written and the first draft is awaiting review (#2982).
 (It's at first draft stage because some details - such as how to configure
 the which libraries are to be loaded - have yet to be written.)


 > "The hook points need to be defined" => " and registered."
 Done.

 > "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:"
 Done.

 > Please use numbered list - it would be easier to reference to specific
 steps.
 Done.


 > 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.
 This bit has been rewritten.  As this is a guide for component developers,
 callout registration is not really an issue (and where it is, they are
 pointed to the user-development guide which describes it in more detail).

 > The first example code in Defining the Hook Points has extra colon after
 #include <hooks/hooks_manager.h>.
 Do you mean the one below the #include?  That's supposed to indicate
 omitted code.

 > Yes. We need to solve that problem somehow anyway to call the proper
 version(), load() and unload() functions.
 I don't know whether that is possible because dlsym() gives no way  to
 restrict the scope of the search - it's through all symbols in the loaded
 library or in libraries referenced by that library.  As far as I know,
 version, load and unload are not functions in any Unix library, although
 we have the flexibility to rename them before release. (I hesitate to
 suggest adding a prefix like b10hooks_ until a determination has been made
 about the rebranding of BIND 10.)

 > 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.
 I've added a note about multiple callouts, but that text belongs more in
 the hooks user guide (which is the guide concerned with writing callouts).

 > 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".
 I think's I've addressed that.


 > '''server_hooks.h'''
 > Please fix a typo in !ServerHooks() ctor: ggetServerHooks() =>
 getServerHooks().
 Fixed.

 > I see that the !HookRegistrationFunction class disappeared. Was it moved
 somewhere or just removed? I'm ok with that if it is gone completely.
 The class was there when the !ServerHooks was not a singleton and had to
 be explicitly created by the server.  It allowed the creation of a list of
 registration functions that the server could execute once the !ServerHooks
 had been created.  With !ServerHooks now a singleton, there is no reason
 for a registration function not to access it directly.  As hooks can be
 registered on a per-module basis at startup (using the technique described
 in the server developer's guide), there was no need for this.

 > '''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.
 The paragraph referred to the !LibraryHandle accessed through the
 !CalloutHandle, which does allow such modifications.  I've updated the
 text to make it clearer.


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

 > '''callout_handle_unittest.cc'''
 > There are no tests for getHookName().
 They are in the handles_unittest.cc file.

 == Replying to comment:5 ==

 > '''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
 Done.

 > The comment for setNumLibraries() says that the initial value is only
 estimate...
 The initial value is set when the number of libraries is known, and the
 updated value set when the number of libraries that were loaded
 successfully is determined.  However, this method will be removed. While
 doing the work for #2981, I realised that after setting the list of
 libraries in bindctl, the change to the configuration has to be validated
 before it is committed.  As it makes sense for the validation to be "all
 or nothing" (i.e the configuration is only valid if all libraries can be
 located, opened and are at the correct version), there is no use-case for
 allowing library loading to continue if some fail to load.

 For this reason, I'd like to leave it as the code will be altered in
 #2981.


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


 > '''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...
 I've removed the "else" because in this case both the "if" and "else"
 clauses return from the method - there is no way to get out of the
 "if/else" block (which seems wrong somehow).  I would have preferred to
 have code along the lines of:
 {{{
 if (condition not satisifed) {
    throw
 }
 return
 }}}
 ... having the end of the function being the return point, but negating
 the condition makes the logic a lot less clear.


 > 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...
 It's up to the server author if they choose to do a calloutsPresent()
 check before they prepare the arguments.  We can't enforce it. In this
 particular case, the reason for the call is to check that hook_index is
 within range (which forms the bulk of the code in calloutsPresent()). I
 could separate that check out into a separate method (it's the same for
 both calloutsPresent() and callCallouts()) but the overhead of calling
 empty() on a vector seemed sufficiently small for it not to be worth it.

 > 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.
 It's feasible that a server may call callCallouts() without calling
 calloutsPresent() first. For example, to gather statistics of number of
 packets received, the code could contain a hook call without arguments -
 calling the hook indicates "packet received".  Or a server might be
 structured such that each callout takes the same argument. It could put a
 pointer to a received packet in a !CalloutHandle argument list as the
 first step of processing: there is no point in cluttering up the server
 code with calls to calloutsPresent() as callCallouts() is a no-op with no
 callouts registered.

 > 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.
 It's not just installing callouts, it's removing them as well.  It's quite
 possible that a callout may want to remove itself - it's just how flexible
 we want the system to be. As I said before, the overhead is only incurred
 if any callouts are actually called and will be be small compared with the
 overhead added by the callout. Let's leave it in and measure the overhead.

 > '''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.
 That's a Doxygen configuration issue - no test classes are included in the
 Doxygen documentation.

 == Replying to comment:6 ==

 > '''library_manager.h'''
 > !LibraryManager? class comment: "On unload, it calls the "unload" method
 if present, clears the callouts.
 Comment corrected.

 > "Should this happens" => "Should this happen'
 Fixed.

 > I would use stronger language in checkVersion() and point out that the
 version() method is really mandatory.
 Done.

 > I would like to see registerStandardCallouts() returing bool if any
 callouts were registered. Right now it is possible to make an empty
 library that has version() function and nothing else in it. We should at
 least print out a warning about that (or more likely an error). That's the
 only way we can catch spelling errors in user library function names, so
 it is rather important.
 What it won't catch (which is more likely) is a case where there are
 several standard callouts with one misspelled; the misspelled one won't be
 registered. I had thought of returning an "int" indicating the number of
 callouts successfully registered but when would you print it?  The only
 possibility is as a debug message, in which case the names of the callouts
 that are loaded (which is already printed) is of more use.


 > '''library_manager.cc'''
 > I would like to see LOAD_FUNCTION_NAME, UNLOAD_FUNCTION_NAME and
 VERSION_FUNCTION_NAME moved to a header. A header is a natural place as it
 will be used by a user library developer.
 Moved to the general "hooks.h" file.  Also moved there are typdefs for
 pointers to the functions.

 > checkVersion() method: It would be good to check that
 version_function_ptr and void* are of the same size.
 Whether they are or not doesn't matter - it is up to the machine and
 compiler.  There is an issue between converting from "void*" (returned by
 dlsym()) and a pointer to a function, which is documented in the reference
 in the code.  The suggested way round this is the union, which is why it
 was done.  The zeroing of both elements of the union as a way of ensuring
 that everything is set to 0 seemed faster than a memset() call (although
 this has now been changed as a result of a later comment).

 > You may also consider wrapping call to user libraries() with try ...
 catch.
 I presume you mean the framework functions (it's already done for the
 callouts).  Good catch - try/catch clause added.

 > I think HOOKS_REGISTER_STD_CALLOUT should print out function pointer. It
 doesn't give much, but that will at least give some pointers if we get
 bizarre segfaults.
 Good idea, done.

 > Question about checkVersion(), runLoad() and runUnload(). How did you
 solve the problem of dlsym() returning a pointer to a function not from
 the library? E.g. you load library1 then when you call dlsym("version")
 for library2 you can get version() from library1 (which is already
 loaded). I don't see any code for that. I admit that I don't have any
 experience with dynamically loaded libraries.
 It appears that when looking up a symbol in a library, it only looks for
 the symbol in the library being loaded and the libraries linked against
 it, not in previously loaded libraries.  I explicitly test for this in the
 !LoadMultipleLibraries test; the
 test loads a library with all the framework functions (which return
 success), then loads a library with no framework functions. If dlsym()
 searches currently loaded libraries, it should pick up the symbols from
 the first library.  The test checks that the second library fails to load,
 indicating that dlsym() didn't find the framework functions in it.

 In general, there appears to be limitations on what the basic dlsym() can
 do.  Some operating system provide more control with non-standard flags.

 > Return codes for load() and unload() should be defined in a header
 somewhere. It may be just one constant for 0 with the explanation that
 non-zero status codes are errors and are library dependent.
 The documentation (like most Unix documentation) states that 0 is a
 success status, non-zero is an error indication. As we would only be
 adding a single symbolic constant, this just adds complication for no real
 gain.

 > The code for union of pointers and dlsym() calls are repeated in 4
 places. Would it be possible to refactor them into a separate method? It's
 an honest question.
 Good suggestion.  I've created a simple !PointerConverter class to
 encapsulate the union and handle the initialization.  Member methods
 return the appropriate type of pointer.


 > loadLibrary(): This is a bit confusing. If version check fails, you run
 closeLibrary(). If runLoad() fails, you don't. I think in the first "else"
 there should be unloadLibrary() and closeLibrary().
 Well spotted.  The closeLibrary() call should not have been within the
 "else" block of the "checkVersion()" conditional statement.


 > unloadLibrary(): I think there's a bug in the code. closeLibrary() will
 be called only if runUnload() succeeded. Is that what you wanted? I think
 that we should call closeLibrary() every time, so it should be:
 > result = closeLibrary() && result;

 Good catch!  Corrected.

 > '''tests/test_libraries.h.in'''
 > "Take carse of" => "Take care of"
 Fixed.

 > '''tests/Makefile.am'''
 > There are unnecessary spaces in lines 32 and 37.
 Removed.

 > '''tests/library_manager_unittest.cc'''
 > What are the r and d parameters of executeCallCallouts() mean? I presume
 it is a short > for data and result, right?
 Yes - I've added a short explanation.

 > This is used in several tests. I understand that the callouts perform
 simple mathematical operations, but a bit of explanation would be handy.
 Also, that's a tricky approach as with all results can be achieved with
 different combinations of addition and multiplication.
 The explanation can be found in common_test_class.h.


 > library_handle.h
 > The comment about manager parameter passed to constructor being a raw
 pointer being safe. Although this may be true, there's not guarantee that
 !LibraryHandle will not be created somewhere else. You would get that
 guarantee if constructor was protected and !CalloutManager was defined a
 friend. This may possibly complicate some tests (which violate that
 guarantee).
 I'd like to leave that for a moment as it does add more complication.  It
 is possible for !LibraryHandle to be created elsewhere but the creator
 would need to supply a suitable !CalloutManager.  However, to reduce the
 risk of a dangling pointer, I've made the !LibraryHandle copy constructor
 and assignment operator private.  User-code  can only get a reference to
 an existing object, they can't take a copy of it.

 > '''hooks/'hook_messages.mes'''
 > The different between HOOKS_CALLOUT_REMOVED and HOOKS_DEREGISTER_CALLOUT
 are not clear (I understand the difference, but it not clearly explained).
 Two comments about log message naming. Firstly, some hooks have noun_verb
 names, which others have verb_nound. Secondly, some are in present and
 some in past tense. I don't have any preference, but I think it should be
 consistent.
 Improved the explanation and updated the message symbols to be noun_verb.
 This has meant that the messages are not in alphabetic order; I've left it
 like that for ease of review and will reorder them before merging.

 > tests/basic_callout_library.cc
 > Please update the naming to reflect what the callouts are for. lm_one is
 a wrong name. It is not library manager, it is a callout. Also, it would
 be great if the name reflected what the callout actually does, so
 callout_add seems like a good name.
 They are only tests and their functionality is incidental - it is only
 something to prove that the callout worked.  However, I've renamed the
 hook points "hookpt_one", "hookpt_two" and "hookpt_three".

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