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:
                Type:  enhancement   |  stephen
            Priority:  medium        |                       Status:
           Component:  dhcp6         |  reviewing
            Keywords:                |                    Milestone:
           Sensitive:  0             |  Sprint-DHCP-20130731
         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:5 stephen]:
 > 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.)
 Agree. I haven't thought about it. I see that #3062 is ready for review. I
 will review it soon.

 > '''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.
 It is possible use case, but rather infrequently used. I think that the
 most common place to implement access control would be pkt6_receive as the
 library can easily access client-id, server-id and other options. We had
 similar discussion before - the most useful example would be something
 related to access control, e.g. check if received client-id is on white
 /black-list and then continue processing or drop it. Easy to implement,
 yet sufficiently complex to showcase many hook features.

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

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

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

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

 > "specifies if server should do the packing" - please start comments with
 a capital letter.
 Fixed that and many other comments.

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

 > 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 ;-) ).
 There is. I should be more careful when copying the code.

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

 > Comment in error: "did the removal as successful"
 Fixed.

 > '''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.
 Created ticket #3085.

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

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

 > HooksDhcpv6SrvTest::buffer6_receive_change_client: spaces needed around
 the ">".  Also, what is special about "pkt->data_[8]"?
 It's the first byte of the first option. We need to modify something and
 the first byte of a first option is a good choice. The code now uses
 constants and the purpose of that modification is explained.

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

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

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

 Ok, the ticket is back with you. Thanks for the review.

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