BIND 10 #2984: Add remaining (15+) hooks into the DHCPv6 Server

[BIND 10 Development]

#2984: Add remaining (15+) hooks into the DHCPv6 Server
-------------------------------------+-------------------------------------
            Reporter:  stephen       |                        Owner:  tomek
                Type:  enhancement   |                       Status:
            Priority:  medium        |  reviewing
           Component:  dhcp6         |                    Milestone:
            Keywords:                |  Sprint-DHCP-20130731
           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:

 Revieweed commit d165e1c0d34e34a24b4519106af41af74d92b197

 I've made modifications to a couple of files - please "pull" before
 continuing.


 '''General'''[[BR]]
 Reviewing this code (and working on the generalised "getCalloutHandle()"
 in #3062) suggests that there is a weakness the way the stored
 !CalloutHandle is accessed.

 getCalloutHandle() uses the address of the packet to identify the
 !CalloutHandle.  The server passes a shared pointer to the packet to the
 hooks callout.  If the callout modifies the packet ''in-situ'', there is
 no problem.  However, if the callout creates a new packet and updates the
 shared pointer, a new !CalloutHandle will be obtained the next time one is
 needed.  This is only an issue if callouts are using per-packet context
 but, if they are, loss of context could be surprising.

 Given the current "one packet at a time" nature of the server, it might be
 better to modify getCalloutHandle() not to check the packet address.
 Instead, add an explicit clear of the !CalloutHandle to the head of the
 "run()" loop and have getCalloutHandle() return the same handle regardless
 of packet passed to it. (If you agree, I'll update #3062.)


 '''src/bin/dhcp6/dhcp6_hooks.dox'''[[BR]]
 I've updated some of the text.

 It occurred to me that if you wanted to drop a packet where you could
 determine the fact in buffer6_receive, it would be most efficient to use
 ycallouts on two hooks: the buffer6_receive would detect the information,
 set a context flag in the !CalloutHandle, set the skip flag and return.
 The pkt6_receive callout would check the context flag and set the skip
 flag based on it.  That way you could drop the packet without the overhead
 of parsing it.  This would prove a useful real-world example and is
 perhaps one we should include somewhere.


 '''src/bin/dhcp6/dhcp6_messages.mes'''[[BR]]
 Minor corrections made to this file.

 '''src/bin/dhcp6/dhcp6_srv.cc'''[[BR]]
 Comment for the "buffer6_receive" callout: The comment here would be
 better if it explained in what context the hook is called, rather than
 just listing it's name, e.g.
 {{{
 The packet has just been received so contains the uninterpreted wire
 data; execute callouts registered for buffer6_receive.
 }}}
 and, at the packet6_receive callout, something like.
 {{{
 At this point the information in the packet has been unpacked into
 the various packet field; execte callouts registered for packet6_receive.
 }}}
 This applies to other hooks points as well.

 Comment for the "buffer6_receive" callout (the comment prior to the call
 to "getSkip()"): The comment is wrong: the next processing step after
 receiving the packet is unpacking it, and it is this step that is skipped
 if the flag is set.

 Would like a comment such as "Unpack the packet information unless the
 buffer6_receive callouts indicated they did it" before the "if
 (!skip_unpack)" line - it's easy to miss this significant step in the
 packet processing.

 In the "switch" for query type, the comment in the "default" clause is now
 incorrect - there is no debug statement before the "switch".  I suggest
 that a LOG_DEBUG statement be put there to output the unrecognised packet
 type.

 "specifies if server should do the packing" - please start comments with a
 capital letter.

 Comment is incorrect in the "packet6_send" hook code.  If the callouts set
 the skip flag, it tells the server to skip the packing step, not to drop
 the packet.

 renewIA_NA (packet6_send hook code): The next processing step is to update
 the lease, so "skip" at this stage does not mean "drop response".
 (Methinks there has been a fair bit of "copy and paste" with the hooks
 code ;-) ).

 releaseIA_NA: same command as above regarding comment when the "skip" flag
 is set.

 Comment in error: "did the removal as successful"


 '''src/bin/dhcp6/tests/dhcp6_test_utils.h'''[[BR]]
 > I'm somewhat uneasy with the content of the new dhcp6_test_utils.h
 header. It is almost exact copy of the class that used to reside in
 dhcp6_srv_unittest.cc. It has too much code for a header, but I did not
 want to split it further into cc+h without getting extra opinion. Please
 advise if it is ok to keep that much code in header or not.
 Create a ticket for this and leave it for now.

 I agree that there is a lot of code in the header and perhaps should be
 separated into a .cc file, but rather than deal with an individual case I
 think we should look at a bit for rationalisation.  For example, the code
 handling the fake packet sending in NakedDhcpv6Srv is more or less
 identical (apart from the packet type) to that in the DHCP4 NakedDhcpv6Srv
 class.  Although it is unrealistic to combine the Naked* classes, the fake
 packet processing could be abstracted into a template class shared between
 the two tests.  Some of the tests I wrote for the hooks code in #2981 are
 duplicated in the two servers. In the long term, we should probably create
 a DHCP server test directory into which we put common tests.

 '''src/bin/dhcp6/tests/hooks_unittest.cc'''[[BR]]
 "Hooks" test - could perhaps line up all the "= -1" under one another.

 HooksDhcpv6SrvTest::createOption: as payload is numeric, a data type of
 uint8_t would be semantically more correct.

 HooksDhcpv6SrvTest::buffer6_receive_change_client: spaces needed around
 the ">".  Also, what is special about "pkt->data_[8]"?

 Please start the first line of each of the callbackk headers with a
 capital letter.

 HooksDhcpv6SrvTest: if NakedDhcpv6Srv was declared as a scoped_ptr, its
 deletion in the destructor would not be needed.

 simple_pkt6_receive test: Please start every comment with a capital
 letter.

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