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

 * owner:  stephen => tomek


Comment:

 From comment:6

 '''src/lib/util/tests/callout_manager_unittest.cc'''[[BR]]
 > !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.
 Changed to 10 libraries.

 > You do not set callout_value_ in constructor.
 Now initialized to 0.

 > 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.
 Renamed to callout_X.

 > !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...
 I've followed the suggestion I made in my last comment and to which you
 agreed: the error return from the callout is ignored, and the
 SUCCESS/COMPLETE status codes have been removed. (Currently there is a
 "todo" in the code to remind me to add the logging of errors when I deal
 with the next ticket.)

 > Skip flag is not tested.
 Test has been added to the handles_unittest set of tests. (The basic
 operation of the "skip" flag is tested in the callout_handle unit tests.)

 > 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).
 Added !MultipleCalloutsLibrariesHooks test.

 From comment:7

 '''src/lib/util/tests/handles_unittest.cc'''[[BR]]
 > There are no dedicated tests to check if argument set/changed by one
 callout are passed to the next callout (it is tested to some degree - many
 of the existing tests use getArgument). It would be good to have a
 dedicated test for that purpose.
 Good catch. !CheckModifiedArgument test added for that purpose.

 '''src/lib/util/tests/callout_handle_unittest.cc'''[[BR]]
 > !CalloutHandleTest.!ContextItemNames: what's the value variable is for?
 It is increased for every setArgument, but then never checked. If it not
 really important (because we check names only), please add a short comment
 about it.
 Removed and constants used instead.

 '''Extra comment about !CalloutHandle class'''[[BR]]
 > Final comment is about get/setArgument. We need getConstArgument() (to
 be used by library) and setConstArgument() (to be used by server only). If
 this can be achived with use of existing get/setArgument template, such
 capability should be demonstrated in tests (and later documented). That is
 needed to pass configuration parameters. For example in select_subnet hook
 point, I need to provide available subnets, so the callout can choose.
 However, the callout must not modify that configuration data (there's
 another interface for changing configuration data - bindctl and its
 restful api).
 >
 > Please note that tests done in !CalloutHandleTest.!PointerTypes are not
 sufficient. const Beta* x only makes the pointer constant, so it cannot be
 changed, however the object it points to can be changed. We need Beta*
 const x; (non-const pointer to a const object).

 You are incorrect in your pointer types:

 * <type>* ptr - variable pointer to variable data.
 * const <type>* ptr - variable pointer to constant data.
 * <type>* const ptr - constant pointer to variable data.
 * const <type>* const ptr - constant pointer to constant data.

 (e.g. see http://duramecho.com/ComputerInformation/WhyHowCppConst.html or
 http://jriddell.org/const-in-cpp.html)

 The argument list can distinguish between "<type>* ptr" and "const <type>*
 ptr" and the tests check this, i.e. that if the server were to pass a
 "const <type>*" pointer, the user library could only retrieve the data
 into a "const <type>*" pointer. So you can pass a pointer to data that the
 user library cannot modify (although there is nothing to stop the user
 library calling const_cast to discard the const attribute in order to
 modify the data).

 Note that it doesn't make sense to copy a "const" object (as opposed to a
 pointer/reference to a const object) into the argument list. You are
 effectively passing it by value, and it is up to the user library what
 they do with it: to avoid the value being altered in the server, don't
 copy the value back from the argument list when the callout returns.  (The
 issue of data modification is discussed in the [wiki:Bind10HooksFramework
 design document].) Besides, there are issues in that objects are copied
 into the argument list and copied out from them: "const" objects can only
 be set at initialization, copying into them is forbidden.

 '''!ChangeLog'''[[BR]]
 Suggested !ChangeLog entry:
 {{{
 626.    [func]          stephen
         Added initial part of hooks framework (all but library loading).
         (Trac #2974, git xxx)
 }}}

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