[2765] Addressed review comments.

src/lib/dhcp/iface_mgr.cc

@@ -89,7 +89,7 @@ Iface::closeSockets(const uint16_t family) {
             // next one.
             close(sock->sockfd_);
             // Close fallback socket if open.
-            if (sock->fallbackfd_) {
+            if (sock->fallbackfd_ >= 0) {
                 close(sock->fallbackfd_);
             }
             sockets_.erase(sock++);
@@ -153,7 +153,7 @@ bool Iface::delSocket(uint16_t sockfd) {
         if (sock->sockfd_ == sockfd) {
             close(sockfd);
             // Close fallback socket if open.
-            if (sock->fallbackfd_) {
+            if (sock->fallbackfd_ >= 0) {
                 close(sock->fallbackfd_);
             }
             sockets_.erase(sock);

src/lib/dhcp/iface_mgr.h

@@ -617,7 +617,7 @@ public:
                                     const uint16_t port);
 
 
-    /// Opens IPv6 sockets on detected interfaces.
+    /// @brief Opens IPv6 sockets on detected interfaces.
     ///
     /// @todo This function will throw an exception immediately when a socket
     /// fails to open. This is undersired behavior because it will preclude
@@ -638,16 +638,64 @@ public:
     /// @return true if any sockets were open
     bool openSockets6(const uint16_t port = DHCP6_SERVER_PORT);
 
-    /// Opens IPv4 sockets on detected interfaces.
+    /// @brief Opens IPv4 sockets on detected interfaces.
+    ///
+    /// This function attempts to open sockets on all interfaces which have been
+    /// detected by @c IfaceMgr and meet the following conditions:
+    /// - interface is not a local loopback,
+    /// - interface is running (connected),
+    /// - interface is up,
+    /// - interface is active, e.g. selected from the configuration to be used
+    /// to listen DHCPv4 messages,
+    /// - interface has an IPv4 address assigned.
+    ///
+    /// The type of the socket being open depends on the selected Packet Filter
+    /// represented by a class derived from @c isc::dhcp::PktFilter abstract
+    /// class.
+    ///
+    /// It is possible to specify whether sockets should be broadcast capable.
+    /// In most of the cases, the sockets should support broadcast traffic, e.g.
+    /// DHCPv4 server and relay need to listen to broadcast messages sent by
+    /// clients. If the socket has to be open on the particular interface, this
+    /// interface must have broadcast flag set. If this condition is not met,
+    /// the socket will be created in the unicast-only mode. If there are
+    /// multiple broadcast-capable interfaces present, they may be all open
+    /// in a broadcast mode only if the OS supports SO_BINDTODEVICE (bind socket
+    /// to a device) socket option. If this option is not supported, only the
+    /// first broadcast-capable socket will be opened in the broadcast mode.
+    /// The error will be reported for sockets being opened on other interfaces.
+    /// If the socket is bound to a device (interface), the broadcast traffic
+    /// sent to this interface will be received on this interface only.
+    /// This allows the DHCPv4 server or relay to detect the interface on which
+    /// the broadcast message has been received. This interface is later used
+    /// to send a response.
+    ///
+    /// On the systems with multiple interfaces, it is often desired that the
+    /// failure to open a socket on a particular interface doesn't cause a
+    /// fatal error and sockets should be opened on remaining interfaces.
+    /// However, the warning about the failure for the particular socket should
+    /// be communicated to the caller. The libdhcp++ is a common library with
+    /// no logger associated with it. Most of the functions in this library
+    /// communicate errors via exceptions. In case of openSockets4 function
+    /// exception must not be thrown if the function is supposed to continue
+    /// opening sockets, despite an error. Therefore, if such a behavior is
+    /// desired, the error handler function can be passed as a parameter.
+    /// This error handler is called (if present) with an error string.
+    /// Typically, error handler will simply log an error using an application
+    /// logger, but it can do more sophisticated error handling too.
+    ///
+    /// @todo It is possible that additional parameters will have to be added
+    /// to the error handler, e.g. Iface if it was really supposed to do
+    /// some more sophisticated error handling.
+    ///
+    /// If the error handler is not installed (is NULL), the exception is thrown
+    /// for each failure (default behavior).
     ///
     /// @param port specifies port number (usually DHCP4_SERVER_PORT)
     /// @param use_bcast configure sockets to support broadcast messages.
-    /// @param error_handler A pointer to a callback function which is called
-    /// by the openSockets4 when it fails to open a socket. This parameter
-    /// can be NULL to indicate that the callback should not be used. In such
-    /// case the @c isc::dhcp::SocketConfigError exception is thrown instead.
-    /// When a callback is installed the function will continue when callback
-    /// returns control.
+    /// @param error_handler A pointer to an error handler function which is
+    /// called by the openSockets4 when it fails to open a socket. This
+    /// parameter can be NULL to indicate that the callback should not be used.
     ///
     /// @throw SocketOpenFailure if tried and failed to open socket and callback
     /// function hasn't been specified.
@@ -883,13 +931,17 @@ private:
     getLocalAddress(const isc::asiolink::IOAddress& remote_addr,
                     const uint16_t port);
 
-    /// @brief Handles an error which occurs during operation on the socket.
+    /// @brief Handles an error which occurs during configuration of a socket.
     ///
     /// If the handler callback is specified (non-NULL), this handler is
     /// called and the specified error message is passed to it. If the
     /// handler is not specified, the @c isc::dhcpSocketConfigError exception
     /// is thrown with the specified message.
     ///
+    /// This function should be called to handle errors which occur during
+    /// socket opening, binding or configuration (e.g. setting socket options
+    /// etc).
+    ///
     /// @param errmsg An error message to be passed to a handlder function or
     /// to the @c isc::dhcp::SocketConfigError exception.
     /// @param handler An error handler function or NULL.

src/lib/dhcp/pkt_filter.cc

@@ -28,8 +28,9 @@ PktFilter::openFallbackSocket(const isc::asiolink::IOAddress& addr,
     // Create socket.
     int sock = socket(AF_INET, SOCK_DGRAM, 0);
     if (sock < 0) {
-        isc_throw(SocketConfigError, "failed to create fallback socket for address "
-                  << addr.toText() << ", port " << port);
+        isc_throw(SocketConfigError, "failed to create fallback socket for"
+                  " address " << addr.toText() << ", port " << port
+                  << ", reason: " << strerror(errno));
     }
     // Bind the socket to a specified address and port.
     struct sockaddr_in addr4;
@@ -38,21 +39,23 @@ PktFilter::openFallbackSocket(const isc::asiolink::IOAddress& addr,
     addr4.sin_addr.s_addr = htonl(addr);
     addr4.sin_port = htons(port);
 
-    if (bind(sock, reinterpret_cast<struct sockaddr*>(&addr4), sizeof(addr4)) < 0) {
+    if (bind(sock, reinterpret_cast<struct sockaddr*>(&addr4),
+             sizeof(addr4)) < 0) {
         // Remember to close the socket if we failed to bind it.
         close(sock);
-        isc_throw(SocketConfigError, "failed to bind fallback socket to address "
-                  << addr.toText() << ", port " << port << " - is another DHCP "
-                  "server running?");
+        isc_throw(SocketConfigError, "failed to bind fallback socket to"
+                  " address " << addr.toText() << ", port " << port
+                  << ", reason: " << strerror(errno)
+                  << " - is another DHCP server running?");
     }
 
-    // Set socket to non-blocking mode. This is to prevent the read from the fallback
-    // socket to block message processing on the primary socket.
+    // Set socket to non-blocking mode. This is to prevent the read from the
+    // fallback socket to block message processing on the primary socket.
     if (fcntl(sock, F_SETFL, O_NONBLOCK) != 0) {
         close(sock);
         isc_throw(SocketConfigError, "failed to set SO_NONBLOCK option on the"
                   " fallback socket, bound to " << addr.toText() << ", port "
-                  << port);
+                  << port << ", reason: " << strerror(errno));
     }
     // Successfully created and bound a fallback socket. Return a descriptor.
     return (sock);

src/lib/dhcp/pkt_filter_inet.h

@@ -53,6 +53,8 @@ public:
     /// @param send_bcast Configure socket to send broadcast messages.
     ///
     /// @return A structure describing a primary and fallback socket.
+    /// @throw isc::dhcp::SocketConfigError if error occurs when opening,
+    /// binding or configuring the socket.
     virtual SocketInfo openSocket(const Iface& iface,
                                   const isc::asiolink::IOAddress& addr,
                                   const uint16_t port,
@@ -65,6 +67,10 @@ public:
     /// @param socket_info structure holding socket information
     ///
     /// @return Received packet
+    /// @throw isc::dhcp::SocketReadError if an error occurs during reception
+    /// of the packet.
+    /// @throw An execption thrown by the isc::dhcp::Pkt4 object if DHCPv4
+    /// message parsing fails.
     virtual Pkt4Ptr receive(const Iface& iface, const SocketInfo& socket_info);
 
     /// @brief Send packet over specified socket.
@@ -74,6 +80,8 @@ public:
     /// @param pkt packet to be sent
     ///
     /// @return result of sending a packet. It is 0 if successful.
+    /// @throw isc::dhcp::SocketWriteError if an error occures during sending
+    /// a DHCP message through the socket.
     virtual int send(const Iface& iface, uint16_t sockfd,
                      const Pkt4Ptr& pkt);
 

src/lib/dhcp/pkt_filter_lpf.cc

@@ -157,8 +157,7 @@ PktFilterLPF::openSocket(const Iface& iface,
                   << "' to interface '" << iface.getName() << "'");
     }
 
-    SocketInfo sock_desc(addr, port, sock, fallback);
-    return (sock_desc);
+    return (SocketInfo(addr, port, sock, fallback));
 
 }
 
@@ -171,10 +170,18 @@ PktFilterLPF::receive(const Iface& iface, const SocketInfo& socket_info) {
     // end after receiving one packet. The call to recv returns immediately
     // when there is no data left on the socket because the socket is
     // non-blocking.
+    // @todo In the normal conditions, both the primary socket and the fallback
+    // socket are in sync as they are set to receive packets on the same
+    // address and port. The reception of packets on the fallback socket
+    // shouldn't cause significant lags in packet reception. If we find in the
+    // future that it does, the sort of threshold could be set for the maximum
+    // bytes received on the fallback socket in a single round. Further
+    // optimizations would include an asynchronous read from the fallback socket
+    // when the DHCP server is idle.
     int datalen;
     do {
         datalen = recv(socket_info.fallbackfd_, raw_buf, sizeof(raw_buf), 0);
-    } while (datalen >= 0);
+    } while (datalen > 0);
 
     // Now that we finished getting data from the fallback socket, we
     // have to get the data from the raw socket too.

src/lib/dhcp/tests/iface_mgr_unittest.cc

@@ -1107,7 +1107,7 @@ TEST_F(IfaceMgrTest, setMatchingPacketFilter) {
 
 TEST_F(IfaceMgrTest, checkPacketFilterLPFSocket) {
     IOAddress loAddr("127.0.0.1");
-    int socket1 = 0, socket2 = 0;
+    int socket1 = -1, socket2 = -1;
     // Create two instances of IfaceMgr.
     boost::scoped_ptr<NakedIfaceMgr> iface_mgr1(new NakedIfaceMgr());
     ASSERT_TRUE(iface_mgr1);
@@ -1138,6 +1138,8 @@ TEST_F(IfaceMgrTest, checkPacketFilterLPFSocket) {
     // to prevent resource leak.
     if (socket2 >= 0) {
         close(socket2);
+        ADD_FAILURE() << "Two sockets opened and bound to the same IP"
+            " address and UDP port";
     }
 
     if (socket1 >= 0) {