From: Marcin Siodelski Date: Tue, 14 Jan 2014 13:07:05 +0000 (+0100) Subject: [3252] Addressed review comments. X-Git-Tag: bind10-1.2.0beta1-release~143^2 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=68b6c2b5001d19eb88ea10bb1c40744cbe1d33e5;p=thirdparty%2Fkea.git [3252] Addressed review comments. --- diff --git a/src/lib/dhcp/iface_mgr.cc b/src/lib/dhcp/iface_mgr.cc index 741b798b75..8b8e16276c 100644 --- a/src/lib/dhcp/iface_mgr.cc +++ b/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()); diff --git a/src/lib/dhcp/iface_mgr.h b/src/lib/dhcp/iface_mgr.h index bdae0af35a..09afa68835 100644 --- a/src/lib/dhcp/iface_mgr.h +++ b/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 diff --git a/src/lib/dhcp/tests/iface_mgr_unittest.cc b/src/lib/dhcp/tests/iface_mgr_unittest.cc index 00ab342e5a..d538dc8168 100644 --- a/src/lib/dhcp/tests/iface_mgr_unittest.cc +++ b/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_);