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


Comment:

 Replying to [comment:8 stephen]:
 > > 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.
 I was not aware of such notation. I think that ellipsis (3 dots) is more
 common for that purpose.

 > > '''callout_handle_unittest.cc'''
 > > There are no tests for getHookName().
 > They are in the handles_unittest.cc file.
 Sorry, I missed that. Ok, those tests are fine.

 > == Replying to comment:5 ==
 > > 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
 Misusing hooks as a statistics module would be a very bad practice.


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

 Anyway, that was only a suggestion for something to consider. Let's leave
 the code as it is now.
 Agree - once the hooks code is merged (this and 2995), we should repeat
 performance measurements and see what the performance impact is (without
 any libraries installed, just to measure hooks framework 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.
 Ok, so it's a separate matter then. Can you create a ticket for it? That
 ticket should probably get an opinion from someone from DNS.

 > == Replying to comment:6 ==

 > > 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.
 Good idea. I've created #3037 with this concept. Although nice to have, it
 is not required now and should not hold back our development. The idea for
 #3037 is to have such nice to have improvements listed there.

 >
 >
 > > '''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.
 Excellent! I like that header a lot - simple, short and a good starting
 point for implementing a user library.

 > In general, there appears to be limitations on what the basic dlsym()
 can do.  Some operating system provide more control with non-standard
 flags.
 I read dlsym man page for Linux and BSD. They are quite different. Ok,
 let's keep the code as it is now.

 > > 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.
 Fair enough. The wiki version of Hooks Framework (Bind10HooksFramework) is
 clear that non-zero status means error. I presume the Doxygen version does
 the same.

 > > 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.
 Nice class. It is somewhat stunning how much code is required in C++ to do
 seemingly a trivial task.

 > > 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.
 And what would happen if I call LibraryHandle(NULL)? There is no code that
 protects against it.

 Finally, there's no ChangeLog entry for this work.


 Ok, that's it. I think the only two outstanding things are to create a
 ticket for Doxygen to cover test classes and add a check if parameter to
 LibraryHandle() is not NULL. As they are very simple, I don't need to see
 this ticket again. You can review changelog on chatroom if you want to.
 Please merge once you deal with those two minor issues.

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