[1651] Changes after review in dhcp4 and IfaceMgr

src/bin/dhcp4/ctrl_dhcp4_srv.cc

@@ -13,32 +13,19 @@
 // PERFORMANCE OF THIS SOFTWARE.
 
 #include <config.h>
-
-//#include <sys/types.h>
-//#include <sys/socket.h>
-//#include <sys/select.h>
-//#include <netdb.h>
-//#include <netinet/in.h>
-//#include <stdlib.h>
-//#include <errno.h>
-
 #include <cassert>
 #include <iostream>
 
 #include <cc/session.h>
 #include <cc/data.h>
-
 #include <exceptions/exceptions.h>
 #include <cc/session.h>
 #include <config/ccsession.h>
-
 #include <util/buffer.h>
 #include <log/dummylog.h>
-
 #include <dhcp4/spec_config.h>
 #include <dhcp4/ctrl_dhcp4_srv.h>
 #include <dhcp/iface_mgr.h>
-
 #include <asiolink/asiolink.h>
 #include <log/logger_support.h>
 

src/bin/dhcp4/ctrl_dhcp4_srv.h

@@ -45,13 +45,16 @@ public:
     ControlledDhcpv4Srv(uint16_t port = DHCP4_SERVER_PORT,
                         bool verbose = false);
 
+    /// @brief Destructor.
+    ~ControlledDhcpv4Srv();
+
     /// @brief Establishes msgq session.
     ///
     /// Creates session that will be used to receive commands and updated
     /// configuration from boss (or indirectly from user via bindctl).
     void establishSession();
 
-    /// @brief Terminates existing session.
+    /// @brief Terminates existing msgq session.
     ///
     /// This method terminates existing session with msgq. After calling
     /// it, not further messages over msgq (commands or configuration updates)
@@ -63,8 +66,6 @@ public:
     /// @brief Initiates shutdown procedure for the whole DHCPv4 server.
     void shutdown();
 
-    ~ControlledDhcpv4Srv();
-
     /// @brief Session callback, processes received commands.
     ///
     /// @param command_id text represenation of the command (e.g. "shutdown")
@@ -84,6 +85,9 @@ protected:
 
     /// @brief A callback for handling incoming configuration updates.
     ///
+    /// As pointer to this method is used a callback in ASIO used in
+    /// ModuleCCSession, it has to be static.
+    ///
     /// @param new_config textual representation of the new configuration
     ///
     /// @return status of the config update
@@ -99,16 +103,16 @@ protected:
     static isc::data::ConstElementPtr
     dhcp4CommandHandler(const std::string& command, isc::data::ConstElementPtr args);
 
-    /// @brief callback that will be called from iface_mgr when command/config arrives
+    /// @brief Callback that will be called from iface_mgr when command/config arrives.
     ///
     /// This static callback method is called from IfaceMgr::receive4() method,
     /// when there is a new command or configuration sent over msgq.
     static void sessionReader(void);
 
-    /// @brief IOService object, used for all ASIO operations
+    /// @brief IOService object, used for all ASIO operations.
     isc::asiolink::IOService io_service_;
 
-    /// @brief Helper session object that represents raw connection to msgq
+    /// @brief Helper session object that represents raw connection to msgq.
     isc::cc::Session* cc_session_;
 
     /// @brief Session that receives configuation and commands

src/lib/cc/tests/session_unittests.cc

@@ -247,5 +247,5 @@ TEST_F(SessionTest, get_socket_descr) {
     EXPECT_NO_THROW(socket = sess.getSocketDesc());
 
     // expect actual socket handle to be returned, not 0
-    EXPECT_TRUE(0 < socket);
+    EXPECT_LT(0, socket);
 }

src/lib/dhcp/iface_mgr.cc

@@ -123,7 +123,7 @@ bool IfaceMgr::Iface::delSocket(uint16_t sockfd) {
 IfaceMgr::IfaceMgr()
     :control_buf_len_(CMSG_SPACE(sizeof(struct in6_pktinfo))),
      control_buf_(new char[control_buf_len_]),
-     session_socket_(0), session_callback_(NULL)
+     session_socket_(InvalidSocket), session_callback_(NULL)
 {
 
     cout << "IfaceMgr initialization." << endl;
@@ -685,7 +685,7 @@ IfaceMgr::send(const Pkt4Ptr& pkt)
 
 
 boost::shared_ptr<Pkt4>
-IfaceMgr::receive4(unsigned int timeout) {
+IfaceMgr::receive4(uint32_t timeout) {
 
     const SocketInfo* candidate = 0;
     IfaceCollection::const_iterator iface;
@@ -696,27 +696,29 @@ IfaceMgr::receive4(unsigned int timeout) {
 
     stringstream names;
 
+    /// @todo: marginal performance optimization. We could create the set once
+    /// and then use its copy for select(). Please note that select() modifies
+    /// provided set to indicated which sockets have something to read.
     for (iface = ifaces_.begin(); iface != ifaces_.end(); ++iface) {
 
         for (SocketCollection::const_iterator s = iface->sockets_.begin();
              s != iface->sockets_.end(); ++s) {
 
-            // We don't want IPv6 addresses here.
-            if (s->addr_.getFamily() != AF_INET) {
-                continue;
-            }
-
-            names << s->sockfd_ << "(" << iface->getName() << ") ";
+            // 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_;
+                // Add this socket to listening set
+                FD_SET(s->sockfd_, &sockets);
+                if (maxfd < s->sockfd_) {
+                    maxfd = s->sockfd_;
+                }
+            }
         }
     }
 
     // if there is session socket registered...
-    if (session_socket_) {
+    if (session_socket_ != InvalidSocket) {
         // at it to the set as well
         FD_SET(session_socket_, &sockets);
         if (maxfd < session_socket_)
@@ -737,11 +739,8 @@ IfaceMgr::receive4(unsigned int timeout) {
     if (result == 0) {
         // nothing received and timeout has been reached
         return (Pkt4Ptr()); // NULL
-    }
-    if (result < 0) {
-        char buf[512];
-        strncpy(buf, strerror(errno), 512);
-        cout << "Socket read error: " << buf << endl;
+    } else if (result < 0) {
+        cout << "Socket read error: " << strerror(errno) << endl;
 
         /// @todo: perhaps throw here?
         return (Pkt4Ptr()); // NULL
@@ -749,7 +748,7 @@ IfaceMgr::receive4(unsigned int timeout) {
 
     // Let's find out which socket has the data
 
-    if (session_socket_ && (FD_ISSET(session_socket_, &sockets))) {
+    if ((session_socket_ != InvalidSocket) && (FD_ISSET(session_socket_, &sockets))) {
         // something received over session socket
         cout << "BIND10 command or config available over session socket." << endl;
 

src/lib/dhcp/iface_mgr.h

@@ -357,7 +357,7 @@ public:
     /// @param timeout specifies timeout (in seconds)
     ///
     /// @return Pkt4 object representing received packet (or NULL)
-    Pkt4Ptr receive4(unsigned int timeout);
+    Pkt4Ptr receive4(uint32_t timeout);
 
     /// Opens UDP/IP socket and binds it to address, interface and port.
     ///
@@ -414,6 +414,9 @@ public:
         session_callback_ = callback;
     }
 
+    /// A value of socket descriptor representing "not specified" state.
+    static const int InvalidSocket = -1;
+
     // don't use private, we need derived classes in tests
 protected: