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:

 Regarding testing:

 > We could write them now, but really don't know how to simulate commands.
 Do the tests in src/bin/auth/tests give any clue to this?

 '''Review'''[[BR]]
 To avoid confusion with the number of files changed where "master" was
 merged into this branch, the review was done in two stages:
 * changes between commits 18eda2228413b1bd5764ad23dd5ab45a2d9c1809 and
 66e868079f7a572809e5030d64de9caa4c550f73
 * changes between commits 4a09f5d1d17004095f18b9ada885cbdf61e8f088 and
 d1accb3e7b04acca3711181ab3e9cc5833e92f0f

 '''src/bin/dhcp4/dhcp4_srv.h'''[[BR]]
 "instructs" should start with capital I.

 '''src/bin/dhcp4/main.cc'''[[BR]]
 verbose_mode should have a comment descibing what it is.

 I don't like global objects either.  However, if you put the declaration
 of "dhcp4" in the anonymous namespace (preferred over declaring it static)
 it is scope-limited to the main.cc file, so I would not be too concerned
 about it.

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

 Function names should be likeThis (not like_this).

 disconnect_session (or disconnectSession) needs a header comment.

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

 '''src/lib/dhcp/iface_mgr.{h,cc}'''[[BR]]
 General: please start comments with a capital letter.

 receive4(): should the argument be of type uint32_t?

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

 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?

 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>

 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.

 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?

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