[3252] Addressed review comments.

src/lib/dhcp/iface_mgr.cc

@@ -60,14 +60,16 @@
 /// @param handler Error handler function to be called or NULL to indicate
 /// that exception should be thrown instead.
 /// @param stream stream object holding an error string.
-#define ifacemgr_error(ex_type, handler, stream) \
+#define IFACEMGR_ERROR(ex_type, handler, stream) \
+{ \
     std::ostringstream oss__; \
     oss__ << stream; \
     if (handler) { \
         handler(oss__.str()); \
     } else { \
         isc_throw(ex_type, oss__); \
-    }
+    } \
+} \
 
 using namespace std;
 using namespace isc::asiolink;
@@ -405,7 +407,7 @@ IfaceMgr::openSockets4(const uint16_t port, const bool use_bcast,
                 // bind to INADDR_ANY address but we can do it only once. Thus,
                 // if one socket has been bound we can't do it any further.
                 if (!bind_to_device && bcast_num > 0) {
-                    ifacemgr_error(SocketConfigError, error_handler,
+                    IFACEMGR_ERROR(SocketConfigError, error_handler,
                                    "SO_BINDTODEVICE socket option is"
                                    " not supported on this OS;"
                                    " therefore, DHCP server can only"
@@ -419,7 +421,7 @@ IfaceMgr::openSockets4(const uint16_t port, const bool use_bcast,
                         // open at least one more.
                         openSocket(iface->getName(), *addr, port, true, true);
                     } catch (const Exception& ex) {
-                        ifacemgr_error(SocketConfigError, error_handler,
+                        IFACEMGR_ERROR(SocketConfigError, error_handler,
                                        "failed to open socket on interface "
                                        << iface->getName() << ", reason: "
                                        << ex.what());
@@ -439,7 +441,7 @@ IfaceMgr::openSockets4(const uint16_t port, const bool use_bcast,
                     // Not broadcast capable, do not set broadcast flags.
                     openSocket(iface->getName(), *addr, port, false, false);
                 } catch (const Exception& ex) {
-                    ifacemgr_error(SocketConfigError, error_handler,
+                    IFACEMGR_ERROR(SocketConfigError, error_handler,
                                    "failed to open socket on interface "
                                    << iface->getName() << ", reason: "
                                    << ex.what());
@@ -479,7 +481,7 @@ IfaceMgr::openSockets6(const uint16_t port,
                 openSocket(iface->getName(), *addr, port);
 
             } catch (const Exception& ex) {
-                ifacemgr_error(SocketConfigError, error_handler,
+                IFACEMGR_ERROR(SocketConfigError, error_handler,
                                "Failed to open unicast socket on  interface "
                                << iface->getName() << ", reason: "
                                << ex.what());
@@ -517,13 +519,23 @@ IfaceMgr::openSockets6(const uint16_t port,
             // it for some odd use cases which may utilize non-multicast
             // interfaces. Perhaps a warning should be emitted if the
             // interface is not a multicast one.
+
+            // The sock variable will hold a socket descriptor. It may be
+            // used to close a socket if the function fails to bind to
+            // multicast address on Linux systems. Because we only bind
+            // a socket to multicast address on Linux, on other systems
+            // the sock variable will be initialized but unused. We have
+            // to suppress the cppcheck warning which shows up on non-Linux
+            // systems.
+            // cppcheck-suppress variableScope
             int sock;
             try {
+                // cppcheck-suppress unreadVariable
                 sock = openSocket(iface->getName(), *addr, port,
                                   iface->flag_multicast_);
 
             } catch (const Exception& ex) {
-                ifacemgr_error(SocketConfigError, error_handler,
+                IFACEMGR_ERROR(SocketConfigError, error_handler,
                                "Failed to open link-local socket on "
                                " interface " << iface->getName() << ": "
                                << ex.what());
@@ -545,7 +557,7 @@ IfaceMgr::openSockets6(const uint16_t port,
                 } catch (const Exception& ex) {
                     // Delete previously opened socket.
                     iface->delSocket(sock);
-                    ifacemgr_error(SocketConfigError, error_handler,
+                    IFACEMGR_ERROR(SocketConfigError, error_handler,
                                    "Failed to open multicast socket on"
                                    " interface " << iface->getName()
                                    << ", reason: " << ex.what());

src/lib/dhcp/iface_mgr.h

@@ -629,18 +629,33 @@ public:
 
     /// @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
-    /// other sockets from opening. We should strive to provide similar mechanism
-    /// that has been introduced for V4 sockets. If socket creation fails the
-    /// appropriate error handler is called and once the handler returns the
-    /// function contnues to open other sockets. The change in the IfaceMgr
-    /// is quite straight forward and it is proven to work for V4. However,
-    /// unit testing it is a bit involved, because for unit testing we need
-    /// a replacement of the openSocket6 function which will mimic the
-    /// behavior of the real socket opening. For the V4 we have the means to
-    /// to achieve that with the replaceable PktFilter class. For V6, the
-    /// implementation is hardcoded in the openSocket6.
+    /// 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 openSockets6 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).
+    ///
+    /// @warning This function does not check if there has been any sockets
+    /// already open by the @c IfaceMgr. Therefore a caller should call
+    /// @c IfaceMgr::closeSockets(AF_INET6) before calling this function.
+    /// If there are any sockets open, the function may either throw an
+    /// exception or invoke an error handler on attempt to bind the new socket
+    /// to the same address and port.
     ///
     /// @param port specifies port number (usually DHCP6_SERVER_PORT)
     /// @param error_handler A pointer to an error handler function which is
@@ -705,6 +720,13 @@ public:
     /// If the error handler is not installed (is NULL), the exception is thrown
     /// for each failure (default behavior).
     ///
+    /// @warning This function does not check if there has been any sockets
+    /// already open by the @c IfaceMgr. Therefore a caller should call
+    /// @c IfaceMgr::closeSockets(AF_INET) before calling this function.
+    /// If there are any sockets open, the function may either throw an
+    /// exception or invoke an error handler on attempt to bind the new socket
+    /// to the same address and port.
+    ///
     /// @param port specifies port number (usually DHCP4_SERVER_PORT)
     /// @param use_bcast configure sockets to support broadcast messages.
     /// @param error_handler A pointer to an error handler function which is

src/lib/dhcp/tests/iface_mgr_unittest.cc

@@ -1457,9 +1457,7 @@ TEST_F(IfaceMgrTest, openSocket4ErrorHandler) {
     ASSERT_TRUE(custom_packet_filter);
     ASSERT_NO_THROW(ifacemgr.setPacketFilter(custom_packet_filter));
 
-    // Open socket on eth0. The openSockets4 should detect that this
-    // socket has been already open and an attempt to open another socket
-    // and bind to this address and port should fail.
+    // Open socket on eth0.
     ASSERT_NO_THROW(ifacemgr.openSocket("eth0", IOAddress("10.0.0.1"),
                                         DHCP4_SERVER_PORT));
 
@@ -1467,6 +1465,9 @@ TEST_F(IfaceMgrTest, openSocket4ErrorHandler) {
     // should be called when the IfaceMgr fails to open socket on eth0.
     isc::dhcp::IfaceMgrErrorMsgCallback error_handler =
         boost::bind(&IfaceMgrTest::ifaceMgrErrorHandler, this, _1);
+    // The openSockets4 should detect that there is another socket already
+    // open and bound to the same address and port. An attempt to open
+    // another socket and bind to this address and port should fail.
     ASSERT_NO_THROW(ifacemgr.openSockets4(DHCP4_SERVER_PORT, true, error_handler));
     // We expect that an error occured when we tried to open a socket on
     // eth0, but the socket on eth1 should open just fine.
@@ -1849,9 +1850,7 @@ TEST_F(IfaceMgrTest, openSocket6ErrorHandler) {
     ASSERT_TRUE(filter);
     ASSERT_NO_THROW(ifacemgr.setPacketFilter(filter));
 
-    // Open socket on eth0. The openSockets6 should detect that this
-    // socket has been already open and an attempt to open another socket
-    // and bind to this address and port should fail.
+    // Open socket on eth0.
     ASSERT_NO_THROW(ifacemgr.openSocket("eth0",
                                         IOAddress("fe80::3a60:77ff:fed5:cdef"),
                                         DHCP6_SERVER_PORT));
@@ -1860,12 +1859,10 @@ TEST_F(IfaceMgrTest, openSocket6ErrorHandler) {
     // should be called when the IfaceMgr fails to open socket on eth0.
     isc::dhcp::IfaceMgrErrorMsgCallback error_handler =
         boost::bind(&IfaceMgrTest::ifaceMgrErrorHandler, this, _1);
-    //    ASSERT_NO_THROW(ifacemgr.openSockets6(DHCP6_SERVER_PORT, error_handler));
-    try {
-        ifacemgr.openSockets6(DHCP6_SERVER_PORT, error_handler);
-    } catch (const Exception& ex) {
-        std::cout << ex.what() << std::endl;
-    }
+    // The openSockets6 should detect that a socket has been already
+    // opened on eth0 and an attempt to open another socket and bind to
+    // the same address and port should fail.
+    ASSERT_NO_THROW(ifacemgr.openSockets6(DHCP6_SERVER_PORT, error_handler));
     // We expect that an error occured when we tried to open a socket on
     // eth0, but the socket on eth1 should open just fine.
     EXPECT_EQ(1, errors_count_);