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

[BIND 10 Development]

#1651: Integrate DHCP4 process startup into BIND 10
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  tomek
  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 stephen):

 * owner:  stephen => tomek


Comment:

 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.

 establishSession():
 >> The logic behind extracting the control socket and plugging it into the
 interface manager should be explained in a comment. (In fact, a fuller
 description of the interaction between asio and select() should be
 explained in a header to the file, as it is not limited to a single
 function.)
 > It is described thoroughly in doc/devel/02-dhcp.dox. I'm not sure if you
 haven't seen this description, saying that it is not detailed enough or
 located in the wrong place?

 Locating the detailed explanation in the .dox file is fine.  (The contents
 are fine, although we probably need to run it - and all other .dox files -
 through an editing pass at some time.)  All I was suggesting is adding a
 short comment explaining why the control socket is retrieved from the
 configuration session and set in the interface manager at the point where
 this takes place (and the reason is not obvious).  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.
 }}}

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

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

 execDhcpv4ServerCommand(): Argument to first "return" should be enclosed
 in parentheses.


 '''src/bin/dhcp4/ctrl_dhcp4_srv.h'''[[BR]]
 disconnectSession(): in the header comments, s/not/no/

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


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

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

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

 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"?)

 '''src/lib/dhcp/iface_mgr.cc'''[[BR]]
 > Yes, we need this. No, it can't be encapsulated.
 You're right, I missed the bit that select() modifies the list of file
 descriptors.

 > 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] :-)

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