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:
                Type:  enhancement   |  stephen
            Priority:  medium        |                       Status:
           Component:  Unclassified  |  reviewing
            Keywords:                |                    Milestone:
           Sensitive:  0             |  Sprint-DHCP-20130703
         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 tomek):

 * owner:  tomek => stephen
 * milestone:  Sprint-DHCP-20130717 => Sprint-DHCP-20130703


Comment:

 Replying to [comment:2 stephen]:
 > * Look at library_manager.*, library_manager_collection.* and
 hooks_manager.* in that order. The first handles the loading and unloading
 of a user library, the second handles the fact that there can be multiple
 libraries, and the last collects all the disparate hooks framework objects
 in one place and provides an easy interface for BIND 10 component
 developers.
 '''library_manager.h'''
 LibraryManager class comment: "On unload, it calls the "unload" method if
 present, clears the callouts
  all hooks and closes the library.". There seems to be something missing
 between "callouts" and "all hooks".

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

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

 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.

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

 checkVersion() method: It would be good to check that version_function_ptr
 and void* are of the same size. I think this particular check should be
 done in the production code, not in tests (not everyone runs tests). You
 may also consider wrapping call to user libraries() with try ... catch.
 Libraries still could fail arbitrarily bad, up to and including segfault,
 so it won't be bullet-proof, just a bit resistant against more benign
 failures.

 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.

 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.

 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 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. It's a tricky part of code and there will be much
 debugging conducted in that area in years to come (most 3rd party library
 debugging will probably start here), so perhaps keeping the code simple as
 it is may be the right way to go.

 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().

 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;
 }}}

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

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

 '''tests/test_libraries.h'''
 Good work with the test_libraries.h.in and the libraries it points to.
 Very thorough.

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

 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 changes to library_handle.* are minor and are related to providing
 a way for the server to register its own callouts (as desribed in the dox
 file).
 '''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).

 > * hooks_log.*/hooks_messages.mes are standard logging and message files.
 '''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.

 > In the test directories:
 >
 > * The *library.cc files are each compiled into a separate shared
 library.  These contain relatively simple code and are loaded and unloaded
 during the unit tests.
 '''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.

 It seems that the same applies to other test libraries as well. I did not
 go through them all.

 Finally, I think we should consider adding a separate version function for
 libraries. Right now the version() returns BIND10 Hooks API version. We
 should have another one for versioning library itself. This would be
 called upon load and returned string would be logged. We should do this as
 a separate ticket. Please do not complicate this ticket any further.

 > * common_test_class.h provides common test functionality for a number of
 test files.
 >
 > The remaining files should be reasonably well documented.

 Phew! That's the end of it. I must admit that the size of the code to
 cover made my review not as thorough as I'd like it to be. It was very
 difficult to stay focused when looking at fifth library that is only a bit
 different that previous four. Anyway, this code looks good. I hope the
 things I have missed in review are minor or will come up in testing.

 I'd like to repeat what I said when I began this review - this ticket is
 too large. The next time I receive such a large ticket, I will simply
 refuse to review it.

 The ticket is back with you, Stephen.

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