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/tests/handles_unittest.cc'''

 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.

 '''src/lib/util/tests/callout_handle_unittest.cc'''

 I googled aleph as soon as I spotted the name. The comment at the
 beginning
 of !CalloutHandleTest.!ComplexTypes that explained it almost spoiled
 the fun :) For a long time my only exposure to Hebrew alphabet was
 Aleph_0 - the designation used for cardinality of integer numbers
 set. In a broader sense it was a symbol of abstract algebra that was
 on the border of my comprehension capabilities. I felt chills on my
 spine when I saw Aleph2. If I remember correctly, Aleph2 was the next
 cardinal number after cardinality of real numbers set. Anyway, I never
 realized that it was the first letter from Hebrew alphabet, even though
 I've been to Israel and must have seen that letter many times. Very
 interesting.

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

 '''Extra comment about !CalloutHandle class'''

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

 If this require non-trivial amount of extra code, we should do it as
 part of a separate ticket. This ticket is already way too large.

 This concludes my review. I will now respond to your comment:8 and then
 reassign the ticket back to you.

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