BIND 10 #2974: Implement Hooks Framework (part 1) - all but library loading and unloading

[BIND 10 Development]

#2974: Implement Hooks Framework (part 1) - all but library loading and unloading
-------------------------------------+-------------------------------------
            Reporter:  stephen       |                        Owner:  tomek
                Type:  task          |                       Status:
            Priority:  medium        |  reviewing
           Component:  Unclassified  |                    Milestone:
            Keywords:                |  Sprint-DHCP-20130620
           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):

 '''src/lib/util/hooks/callout_manager.h'''
 I would have preferred if that was done as a separate ticket, but since it
 is here, I'll review it.

 The design does not allow for extra libraries to be loaded at runtime.
 This is something we need to be ready for. You start a server, then
 configure it to use one library, then configure it to use another one. You
 can't assume that the number of libraries is constant.

 Who is intended user of the public methods in !CallOutManager: server-side
 code or user library?
 I assume it is server code, right? Again, I have trouble understanding how
 the intended usage model looks like. Who is going to call
 setLibraryIndex()? I'm concerned with the fact that the registerCallout()
 may be called by the user library, right? Yet at the same time in the same
 public scope we have method setLibraryIndex() that user library must not
 call. You can add applicability statements, but they are not worth
 anything. There will always be someone who thinks he is smart by playing
 tricks ("my library must always be called first, so I'll set library index
 to 0. Ha!").

 Please make it private and define as a friend for
 !CalloutHandle::getContextForLibrary(). Otherwise library developers will
 mess with it.

 The functions in .h file are getting bigger and bigger. It is more
 difficult to understand the code if it is split between .cc and .h (for
 example when I seach where function X is called I need to do it twice,
 once for each file). And of course it is less efficient from the
 compilation perspective. The same code is included many, many times.

 '''src/lib/util/hooks/callout_manager.cc'''
 CalloutManager::registerCallout() Isn't the
 checkLibraryIndex(current_library_) check redundant? It was already
 checked when setting up current_library_.

 Why do you require to have the callouts from the same library be grouped
 together? Michal gave specific example when he wants them to be out of
 order: the encryption + signing. I would remove the whole for loop from
 registerCallout().

 callCallouts() - There's a saying that the disign is not complete when
 there is noting else to add, but when there is nothing else left to be
 taken away. This method has almost good design :) In the current code,
 you'll do vector copy every time. There's 27 hook points for DHCPv6,
 multiplied by 16000 (current number of transactions per second) gives you
 432 thousand copies per second. I can't let that happen. Fortunately,
 there's a very easy solution:

 Forbid installing new callout on a hook point from the callout called from
 the same hook point.

 Easy to communicate to library developers and easy to enforce. In
 callCallouts() store i value in current_hook_being_processed_. In
 registerCallout() add a check if someone tries to install a callout on the
 hook point that is currently being process, then throw exception. The same
 should be true for deregistering callout.

 deregisterCallout() - that's a tricky removal code. I think I understand
 how it works, but isn't there a simpler way to remove something from a
 vector?

 '''src/lib/util/tests/callout_manager_unittest.cc'''
 !CalloutManagerTest constructor. I don't like the fact that there are 4
 hook points defined and there is a maximum of 4 libraries. One of those
 values should be changed to avoid confusion. You do not set callout_value_
 in constructor.

 Brilliant trick for recording callouts order :)

 Why are those callouts named manager_X? I think they should be called
 callout_X (or userlib_X). They represent callouts from a user library.
 Naming them manager is a bit misleading. There will always be just one
 manager.

 !CalloutManagerTest.!CallCalloutsError: I'm somewhat uneasy with the fact
 that the first library that returns an error will cause other libraries to
 not be called. What if the first library is logger and it reports error,
 because partition for logs is full? Then the code will stop calling
 libraries for access control, host reservations etc. That would
 essentially disable hooks and the server would continue to run in its
 "vanilla" form. This would make a very fragile system, with easily
 triggered domino effect. The first library that falls over will trip all
 others as well.

 Skip flag is not tested.

 There are no tests that check that the same callout can be
 registered/called/deregistered on different hook points.
 (!CalloutManagerTest.!CallCalloutsSuccess checks that to some degree)

 Outstanding tests (callout_handle, handles_unittest) will be reviewed in
 the next comment.

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