BIND 10 #2983: Add remaining (15+) Hooks into the DHCPv4 Server

[BIND 10 Development]

#2983: Add remaining (15+) Hooks into the DHCPv4 Server
-------------------------------------+-------------------------------------
            Reporter:  stephen       |                        Owner:
                Type:  enhancement   |  marcin
            Priority:  medium        |                       Status:
           Component:  dhcp4         |  reviewing
            Keywords:                |                    Milestone:
           Sensitive:  0             |  Sprint-DHCP-20130904
         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 => marcin


Comment:

 Replying to [comment:12 marcin]:
 > Reviewed commit 8abbdb589f6205c51f461541778f40dd355db60b
 >
 > '''!ChangeLog'''
 > This is ok.
 >
 > src/bin/dhcp4/dhcp4_hooks.dox
 > pkt4_receive description: Suggest that the first sentence is modified:
 >
 > ''this callout is executed when an incoming DHCPv4 packet is received
 and its content '''has been''' parsed''
 > When using present tense it is confusing if the hook is called before or
 after parsing the message.
 Done.

 > pkt4_receive:
 > ''By the time this hook is reached, that information has '''been'''
 already parsed...''
 Done.

 > Suggest that this:
 > ''data_ contains the incoming packet as raw buffer. By the time this
 hook is reached, that information has already parsed and is available
 though other fields in the Pkt4 object. For this reason, it doesn't make
 sense to modify it.''
 >
 > is rephrased:
 > ''By the time this hook is reached, the contents of the data_ field has
 been already parsed and stored in other fields. Therefore, the
 modification in the data_ field has no effect.''
 > Or something like this....
 Done.

 > Also, this may sound better:
 > ''The callout should '''sanity check all modifications''' as the server
 will use that data as is with no further checking.''
 Done.

 > lease4_select:
 > ''a value of true '''indicates''' that the lease won't be inserted into
 the database (DISCOVER), a value of false '''indicates''' that it will
 (REQUEST).''
 Done.

 > ''It doesn't make sense to modify it at this time.'', I wonder if there
 are an (negative) implications of modifying it?
 Its value not used anymore.

 > ''this callout is executed when server's response is about to be
 '''sent''' back''
 Done.

 > ''object that contains the packet, with source and destination addresses
 '''set''', interface over which it will be '''sent and a''' list of all
 options and relay information ''
 Done.

 > ''All fields of the Pkt4 object can be modified at this time, except
 '''bufferOut_''' ''. The highlighted name does not conform to the naming
 convention for variables. Is it a typo in the Developer's Guide or in the
 code?
 That's how that field used to be named. Renamed to buffer_out_.

 > buffer4_send:
 > '' this callout is executed when server's response is about to be
 '''sent''' back to the client''
 Done.

 > '' object that contains the packet, with source and destination
 addresses ''''set''', interface over which it will be '''sent and''' list
 of all options and relay information''
 Done.
 >
 > ''The raw on-wire form is already prepared in '''bufferOut_''' '', again
 something is wrong with this name.
 Fixed in the code and in the docs as well.


 > '''src/bin/dhcp4/dhcp4_messages.mes'''
 > DHCP4_HOOK_LEASE4_RELEASE_SKIP: typo  in
 > ''If client requested release of '''multiple''' leases (by sending
 multiple IA options), the server will '''retain''' this particular lease
 and will proceed with other renewals as usual.''
 Good catch, but it's not what you meant. There are no multiple leases in
 DHCPv4 :) Removed the whole sentence altogether.


 > '''DHCP4_PACKET_DROP_NO_TYPE dropped packet received on interface %1:
 does not have msg-type option'''
 > It has to be rephrased because I read it as: ''There was a dropped
 packet on the interface X and then it turned out that this packet didn't
 have msg-type option'', while actually it is that ''The packet has been
 dropped and the reason for it was that it hadn't had the msg-type
 option''.
 Rephrased. Hopefully is reads better now.

 > Also, typo in: ''THis is a debug message''
 Fixed.

 > src/bin/dhcp4/dhcp4_srv.cc
 > General comment: I suggest that you stick to 80 lines. I haven't seen
 this documented anywhere that we can do more than 80. Even if there was an
 informal agreement that we can, there are a lot of cases when you actually
 could do 80 lines by wrapping the arguments of the function invocation. I
 am using emacs on my laptop and I often have two windows open side-by-
 side. The 80 line text is displayed just fine. Anything wide gets wrapped
 and makes the code unreadable until I close the other buffer.
 Wrapped some of the longer lines, at least those related to hooks and
 couple others. There are still some lines over 80 chars, but I don't want
 to turn this ticket into code beautification exercise.

 > Shouldn't !''Hooks!'' be named using lower case letters?
 > {{{
 > Dhcp4Hooks hooks;
 > }}}
 Yes and no. Hooks is a global variable. Had it been named with lower case
 letters, it would strongly suggest that it is a local variable, which is
 not. Using capital letter was my idea to signify that this is a global
 object. If you think that it is not a good idea and all lower case is
 better, I'll update it.

 >
 > With the recent changes the Dhcpv4Srv::run function is now over 250
 lines long. It makes it very hard to read. Therefore I suggest that the
 parts of this function which process the certain callouts are enclosed in
 the private functions. These functions could return the boolean flags
 indicating whether to skip the next step or not. I guess it could even be
 a sole private function taking parameters.
 I've created a ticket for this #3117. The same issue is with v6.

 > line 355: should be ''Execute all callouts registered for
 '''packet4_send''' ''
 > line 394: the same issue
 > line 525: the same issue
 Fixed.

 > process Discover and processRequest: Why is this? Why is this commented?
 And why it should be uncommented?
 > {{{
 >     /// @todo Uncomment this
 >     // sanityCheck(request, MANDATORY);
 >
 > }}}
 While implementing hooks, I made the callouts to modify incoming packets
 in a way that would later cause the server to drop the packet, because of
 my modification. I wanted to use the drop as a verification technique that
 hook's modification was indeed affecting the server. In the process I
 discovered that many methods do not call sanityCheck() function. However,
 once I added it around 10 tests started to fail. And this ticket is about
 implementing hooks and not improving sanity checking. I've added separate
 ticket for that (#3116).

 > line 922: packet6_send
 >
 > src/bin/dhcp4/tests/dhcp4_srv_unittest.cc
 >
 > Tests look good to me. The only thing is to rename the test cases so as,
 instead of:
 > !''simple_buffer4_receive!'' you'll have !''simpleBuffer4Receive!''.
 It was explained in one comment it was a normal camel notation,
 underscore, name of the callout. For example: changeClientId_pkt4_send.
 But I agree that it was different than all other test names, so I've
 updated them. It is now somewhat not obvious to get the callout names from
 the test names.

 > src/lib/dhcp/pkt4.h
 >
 > bufferOut_ should be renamed to buffer_out_.
 >
 > Also the description should start with capital letter.
 Renamed and capitalized the description.

 > data_
 > The description should start with capital letter.
 Description updated.

 > src/lib/dhcpsrv/alloc_engine.cc
 >
 > renewLease4: It should be ''Execute all callouts registered for
 '''packet4_send ''' ''
 Fixed.

 > The rollback which is at the end of the function (for memfile) will not
 be valid anymore when I finish the ticket number #3083. With this ticket,
 I am making a change in memfile to return a copy of the lease, so as the
 change to the returned structure doesn't affect the lease in the lease
 container. I suggest that you add a todo to this ticket saying that the
 rollback is removed once 3083 is merged.
 Added.

 I believe I addressed all he issues. Thanks for the review.

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