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:5 stephen]:
 > '''src/lib/cc/tests/session_unittests.cc'''[[BR]]
 > The final EXPECT_TRUE(0 < socket) is better as EXPECT_LT(0, socket), as
 the error message is more informative in that it should print out the
 value of socket.
 Done.

 > '''src/lib/dhcp/iface_mgr.{h,cc}'''[[BR]]
 > General: please start comments with a capital letter.
 Done. I hope I spotted them all.
 >
 > receive4(): should the argument be of type uint32_t?
 No necessarily. uintXX_t formats are typically used when it is essential
 to maintain consistency between 32 and 64 bits. Here it is not a problem -
 64bit compilation could select for longer if there was nothing to do (e.g.
 empty lease database, so there is nothing to expire. 32bit would call
 select() for 4 billion seconds, 64 bit would call it for a lot longer). Of
 course it doesn't matter much, so I've changed it as requested.

 > receive4(): in the loop over the socket collection, suggest we replace:
 > {{{
 >     // We don't want IPv6 addresses here.
 >     if (s->addr_.getFamily() != AF_INET) {
 >         continue;
 >     }
 >
 >     names << s->sockfd_ << "(" << iface->getName() << ") ";
 >
 >     // add this socket to listening set
 >     FD_SET(s->sockfd_, &sockets);
 >     if (maxfd < s->sockfd_)
 >         maxfd = s->sockfd_;
 > }}}
 > with
 > {{{
 >     // Only deal with IPv4 addresses.
 >     if (s->addr_.getFamily() == AF_INET) {
 >        names << s->sockfd_ << "(" << iface->getName() << ") ";
 >
 >        // Add this socket to listening set
 >        FD_SET(s->sockfd_, &sockets);
 >        if (maxfd < s->sockfd_) {
 >            maxfd = s->sockfd_;
 >        }
 >     }
 > }}}
 > (Note also the braces around the single-line if statement.)
 Done. Personally I think first style, as there is one less indent level.
 The code readability was marginally better in my opinion. I don't have
 strong opinion, so I changed the code as requested.

 >
 > receive4(): when testing maxfd against session_socket_, there should be
 a brace around the single-line "if" statement.
 >
 > receive4(): this builds the socket set on each call. Do we need this?
 Can't the socket set be encapsulated in a separate object and stored as a
 member of the !IfaceMgr class?
 Yes, we need this. No, i can't be encapsulated. select() modifies passed
 set and sets those sockets, which have a data to read. We could probably
 define a class member of fd_set type, initialize it once and then use its
 copy every time we want to read something. There are 2 problems with that
 approach:
 1. fs_set is a bitfield. Its size my vary from system to system. It is a C
 structure, so there is no constructor. It could probably be copied with
 memcpy(dst,src, sizeof(fd_set)). The only allowed way of modifying fd_set
 is with provided FD_SET, FD_ISSET, FD_CLEAR, etc. macros. It is possible
 that on some systems that is not a structure, but an array of some sort.
 2. Once we implement support for dynamic interfaces (i.e. detecting that
 interface disappeared), we will need to update that structure.

 So the answer is: yes, it can be code, I just don't want to spend too much
 time on it at this stage. Added todo comment about possible marginal
 performance improvement.

 > receive4(): at the "Socket read error" message, is there a need to
 strncpy() the return from strerror() into a temporary buffer?  Can't a
 call to strerror() be incorporated in the cout call directly>
 Removed temporary buffer.

 > receive4(): the "if (result < 0) {" test at the "Socket read error"
 message is probably better as an "else if", since it is mutually exclusive
 with the preceding "if" clause.
 Fixed.

 > receive4(): rather than checking "if (session_socket_)", better is "if
 (session_socket_ > 0)" (as the socket is a number.)  Incidentally, as 0 is
 a valid file descriptor, wouldn't it be better to initialize
 session_socket_ to an invalid descriptor number, e.g. -1, to indicate that
 it has not been set?
 In theory session_socket_ could be 0, but that file descriptor is always
 taken by stdin. I fail to imagine a situation, where control socket could
 be the first file descriptor opened by a process. Anyway, Introduced
 InvalidSocket constant (with -1 value).

 Also added references to the common description.

 Ready for review.

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