BIND 10 #1651: Integrate DHCP4 process startup into BIND 10

[BIND 10 Development]

#1651: Integrate DHCP4 process startup into BIND 10
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  stephen
  stephen                            |                Status:  reviewing
                       Type:  task   |             Milestone:  DHCP-
                   Priority:         |  Sprint-20120611
  medium                             |            Resolution:
                  Component:  dhcp4  |             Sensitive:  0
                   Keywords:         |           Sub-Project:  DHCP
            Defect Severity:  N/A    |  Estimated Difficulty:  0
Feature Depending on Ticket:         |           Total Hours:  0
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------
Changes (by tomek):

 * owner:  tomek => stephen


Comment:

 Replying to [comment:9 stephen]:
 > Reviewed commit afc4bd6b79b9469b5717d3a1b0ac729cf6aacef7
 >
 > '''src/bin/dhcp4/ctrl_dhcp4_srv.cc'''[[BR]]
 > Initialization of logging should be done in main() - see below.
 >
 > dhcp4CommandHandler(): Suggest outputting a message if "shutdown" is
 received and server_ is null - something must be wrong.
 Done.

 > establishSession():
 > Something along the lines of:
 > {{{
 > // Integrate the asynchronous I/O model of BIND 10 configuration
 > // control with the "select" model of the DHCP server.  This is
 > // fully explained in \ref dhcpv4Session.
 > }}}
 Thank you for contributting the text. Added it to establishSession()
 description.

 > disconnectSession(): closing the configuration session probably closes
 the file descriptor passed to the interface manager during initialization.
 Although this is part of shutdown, it might safest as part of session
 disconnection to clear the socket in the interface manager to prevent its
 further use.  (Incidentally, in iface_mgr.h, !InvalidSocket is probably
 better named INVALID_SOCKET to emphasise that it is a constant.)
 Renamed InvalidSocket to INVALID_SOCKET. disconnectSession() now also
 deregisters control socket.

 > > The code has been refactored significantly. Most of the code that used
 to be in main.cc is now part of the ControlledDhcpv4Srv class. There are
 no more global objects anymore.
 > Not strictly true - ControlledDhcpv4Srv::server_ is still global in that
 it can be referenced outside the file.
 That is technically true, but it is much easier to understand the context
 of this globally accessible class member.

 > execDhcpv4ServerCommand(): Argument to first "return" should be enclosed
 in parentheses.
 Done.
 >
 > '''src/bin/dhcp4/ctrl_dhcp4_srv.h'''[[BR]]
 > disconnectSession(): in the header comments, s/not/no/
 Done.

 > If only referenced by handlers within the ControlledDhcpv4Srv class,
 server_ could be declared private.
 Done.

 >
 > '''src/bin/dhcp4/main.cc'''[[BR]]
 > In main(), the declaration of "server" could be within the "try" block
 (and part of the initialization) as it is not referenced outside. (This
 also means that the setting of server to NULL could be removed.)
 Done.

 > The initialization of logging has been moved to ControlledDhcpv4Srv.  I
 think that should be left in main() and done as early as possible, else no
 messages can be output until the server is initialized (which prevents any
 debug messages stating that the server is initializing).
 Done.

 > Something missed on the last review: does the "getopt()" argument
 require the colon in the ":v" flags?  It - and the "case ':'" could be
 removed.
 Done.

 > The program should probably terminate if "usage()" is executed - it
 implies that the program has been incorrectly invoked.  (Or invoked
 interactively with the wrong flags - perhaps the user specified "-h"?)
 Note that exit() is called at the end of usage() function. Note that this
 part of the code was refactored as part of #1503 ticket (already merged).
 I'd like to keep changes in this part of the code to bare minimum to make
 the merge easier.

 > > I fail to imagine a situation, where control socket could be the first
 file descriptor opened by a process.
 > Never underestimate the influence of
 [http://en.wikipedia.org/wiki/Murphy%27s_law Murphy's Law] :-)
 Unfortunately, I know this gentleman too well already. Anyway, previous
 commit added specific check against InvalidSocket (now renamed to
 INVALID_SOCKET), so the code is safe.

 I believe this commit solves all outstanding issues. If there are no new
 comments, it is ready to merge.

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