From: Marcin Siodelski Date: Thu, 28 Nov 2013 17:46:53 +0000 (+0100) Subject: [2765] Gracefully handle socket opening errors in the IfaceMgr. X-Git-Tag: bind10-1.2.0beta1-release~102^2~5^2~12 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=c62586a2c2f8aecddbb2c1cfd0c00fb7cf41816a;p=thirdparty%2Fkea.git [2765] Gracefully handle socket opening errors in the IfaceMgr. Also, implemented missing unit tests for the Iface function which opens v4 sockets in IfaceMgr. --- diff --git a/src/lib/dhcp/iface_mgr.cc b/src/lib/dhcp/iface_mgr.cc index 845fd21b7d..986aadf189 100644 --- a/src/lib/dhcp/iface_mgr.cc +++ b/src/lib/dhcp/iface_mgr.cc @@ -292,7 +292,9 @@ void IfaceMgr::stubDetectIfaces() { addInterface(iface); } -bool IfaceMgr::openSockets4(const uint16_t port, const bool use_bcast) { +bool +IfaceMgr::openSockets4(const uint16_t port, const bool use_bcast, + IfaceMgrErrorMsgCallback error_handler) { int sock; int count = 0; @@ -339,26 +341,39 @@ bool 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) { - isc_throw(SocketConfigError, "SO_BINDTODEVICE socket option is" - << " not supported on this OS; therefore, DHCP" - << " server can only listen broadcast traffic on" - << " a single interface"); + handleSocketConfigError("SO_BINDTODEVICE socket option is" + " not supported on this OS;" + " therefore, DHCP server can only" + " listen broadcast traffic on a" + " single interface", + error_handler); } else { - // We haven't open any broadcast sockets yet, so we can - // open at least one more. - sock = openSocket(iface->getName(), *addr, port, true, true); - // Binding socket to an interface is not supported so we can't - // open any more broadcast sockets. Increase the number of - // opened broadcast sockets. + try { + // We haven't open any broadcast sockets yet, so we can + // open at least one more. + sock = openSocket(iface->getName(), *addr, port, + true, true); + } catch (const Exception& ex) { + handleSocketConfigError(ex.what(), error_handler); + + } + // Binding socket to an interface is not supported so we + // can't open any more broadcast sockets. Increase the + // number of open broadcast sockets. if (!bind_to_device) { ++bcast_num; } } } else { - // Not broadcast capable, do not set broadcast flags. - sock = openSocket(iface->getName(), *addr, port, false, false); + try { + // Not broadcast capable, do not set broadcast flags. + sock = openSocket(iface->getName(), *addr, port, + false, false); + } catch (const Exception& ex) { + handleSocketConfigError(ex.what(), error_handler); + } } if (sock < 0) { @@ -467,6 +482,21 @@ bool IfaceMgr::openSockets6(const uint16_t port) { return (count > 0); } +void +IfaceMgr::handleSocketConfigError(const std::string& errmsg, + IfaceMgrErrorMsgCallback handler) { + // If error handler is installed, we don't want to throw an exception, but + // rather call this handler. + if (handler != NULL) { + handler(errmsg); + + } else { + isc_throw(SocketConfigError, errmsg); + + } +} + + void IfaceMgr::printIfaces(std::ostream& out /*= std::cout*/) { for (IfaceCollection::const_iterator iface=ifaces_.begin(); diff --git a/src/lib/dhcp/iface_mgr.h b/src/lib/dhcp/iface_mgr.h index bae67cc426..77c9597bce 100644 --- a/src/lib/dhcp/iface_mgr.h +++ b/src/lib/dhcp/iface_mgr.h @@ -22,6 +22,7 @@ #include #include +#include #include #include #include @@ -373,6 +374,13 @@ public: bool inactive6_; }; +/// @brief This type describes the callback function invoked when error occurs +/// in the IfaceMgr. +/// +/// @param errmsg An error message. +typedef +boost::function IfaceMgrErrorMsgCallback; + /// @brief Handles network interfaces, transmission and reception. /// /// IfaceMgr is an interface manager class that detects available network @@ -620,15 +628,22 @@ public: bool openSockets6(const uint16_t port = DHCP6_SERVER_PORT); /// Opens IPv4 sockets on detected interfaces. - /// Will throw exception if socket creation fails. /// /// @param port specifies port number (usually DHCP4_SERVER_PORT) /// @param use_bcast configure sockets to support broadcast messages. - /// - /// @throw SocketOpenFailure if tried and failed to open socket. + /// @param errcb A pointer to a function which should be called everytime + /// a socket being opened failed. The presence of the callback function + /// (non NULL value) implies that an exception is not thrown when the + /// operation on the socket fails. The process of opening sockets will + /// continue after callback function returns. The socket which failed + /// to open will remain closed. + /// + /// @throw SocketOpenFailure if tried and failed to open socket and callback + /// function hasn't been specified. /// @return true if any sockets were open bool openSockets4(const uint16_t port = DHCP4_SERVER_PORT, - const bool use_bcast = true); + const bool use_bcast = true, + IfaceMgrErrorMsgCallback errcb = NULL); /// @brief Closes all open sockets. /// Is used in destructor, but also from Dhcpv4Srv and Dhcpv6Srv classes. @@ -855,6 +870,19 @@ private: getLocalAddress(const isc::asiolink::IOAddress& remote_addr, const uint16_t port); + /// @brief Handles an error which occurs during operation on the 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. + /// + /// @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. + void handleSocketConfigError(const std::string& errmsg, + IfaceMgrErrorMsgCallback handler); + /// Holds instance of a class derived from PktFilter, used by the /// IfaceMgr to open sockets and send/receive packets through these /// sockets. It is possible to supply custom object using diff --git a/src/lib/dhcp/tests/iface_mgr_unittest.cc b/src/lib/dhcp/tests/iface_mgr_unittest.cc index 3d2ed014b6..e7fb2bd606 100644 --- a/src/lib/dhcp/tests/iface_mgr_unittest.cc +++ b/src/lib/dhcp/tests/iface_mgr_unittest.cc @@ -20,6 +20,7 @@ #include #include +#include #include #include @@ -78,17 +79,34 @@ public: return (false); } - /// Pretends to open socket. Only records a call to this function. - /// This function returns fake socket descriptor (always the same). - /// Note that the returned value has been selected to be unique - /// (because real values are rather less than 255). Values greater - /// than 255 are not recommended because they cause warnings to be - /// reported by Valgrind when invoking close() on them. - virtual SocketInfo openSocket(const Iface&, + /// @brief Pretend to open a socket. + /// + /// This function doesn't open a real socket. It always returns the + /// same fake socket descriptor. It also records the fact that it has + /// been called in the public open_socket_called_ member. + /// As in the case of opening a real socket, this function will check + /// if there is another fake socket "bound" to the same address and port. + /// If there is, it will throw an exception. This allows to simulate the + /// conditions when one of the sockets can't be open because there is + /// a socket already open and test how IfaceMgr will handle it. + /// + /// @param iface An interface on which the socket is to be opened. + /// @param addr An address to which the socket is to be bound. + /// @param port A port to which the socket is to be bound. + virtual SocketInfo openSocket(const Iface& iface, const isc::asiolink::IOAddress& addr, const uint16_t port, const bool, const bool) { + // Check if there is any other socket bound to the specified address + // and port on this interface. + const Iface::SocketCollection& sockets = iface.getSockets(); + for (Iface::SocketCollection::const_iterator socket = sockets.begin(); + socket != sockets.end(); ++socket) { + if ((socket->addr_ == addr) && (socket->port_ == port)) { + isc_throw(SocketConfigError, "test socket bind error"); + } + } open_socket_called_ = true; return (SocketInfo(addr, port, 255)); } @@ -112,9 +130,83 @@ public: class NakedIfaceMgr: public IfaceMgr { // "Naked" Interface Manager, exposes internal fields public: + + /// @brief Constructor. NakedIfaceMgr() { } - IfaceCollection & getIfacesLst() { return ifaces_; } + + /// @brief Returns the collection of existing interfaces. + IfaceCollection& getIfacesLst() { return (ifaces_); } + + /// @brief This function creates fictious interfaces with fictious + /// addresses. + /// + /// These interfaces can be used in tests that don't actually try + /// to open the sockets on these interfaces. Some tests use mock + /// objects to mimic sockets being open. These interfaces are + /// suitable for such tests. + void createIfaces() { + + ifaces_.clear(); + + // local loopback + ifaces_.push_back(createIface("lo", 0, "127.0.0.1")); + // eth0 + ifaces_.push_back(createIface("eth0", 1, "10.0.0.1")); + // eth1 + ifaces_.push_back(createIface("eth1", 2, "192.0.2.3")); + } + + /// @brief Create an object representing interface. + /// + /// Apart from creating an interface, this function also sets the + /// interface flags: + /// - loopback flag if interface name is "lo" + /// - up always true + /// - running always true + /// - inactive always to false + /// + /// If one needs to modify the default flag settings, the setIfaceFlags + /// function should be used. + /// + /// @param name A name of the interface to be created. + /// @param ifindex An index of the interface to be created. + /// @param addr An IP address to be assigned to the interface. + /// + /// @return An object representing interface. + static Iface createIface(const std::string& name, const int ifindex, + const std::string& addr) { + Iface iface(name, ifindex); + iface.addAddress(IOAddress(addr)); + if (name == "lo") { + iface.flag_loopback_ = true; + } + iface.flag_up_ = true; + iface.flag_running_ = true; + iface.inactive4_ = false; + return (iface); + } + + /// @brief Modified flags on the interface. + /// + /// @param name A name of the interface. + /// @param loopback A new value of the loopback flag. + /// @param up A new value of the up flag. + /// @param running A new value of the running flag. + /// @param inactive A new value of the inactive flag. + void setIfaceFlags(const std::string& name, const bool loopback, + const bool up, const bool running, + const bool inactive) { + for (IfaceMgr::IfaceCollection::iterator iface = ifaces_.begin(); + iface != ifaces_.end(); ++iface) { + if (iface->getName() == name) { + iface->flag_loopback_ = loopback; + iface->flag_up_ = up; + iface->flag_running_ = running; + iface->inactive4_ = inactive; + } + } + } }; /// @brief A test fixture class for IfaceMgr. @@ -126,8 +218,9 @@ public: /// test failure path. class IfaceMgrTest : public ::testing::Test { public: - // These are empty for now, but let's keep them around - IfaceMgrTest() { + /// @brief Constructor. + IfaceMgrTest() + : errors_count_(0) { } ~IfaceMgrTest() { @@ -172,6 +265,26 @@ public: return (NULL); } + /// @brief Implements an IfaceMgr error handler. + /// + /// This function can be installed as an error handler for the + /// IfaceMgr::openSockets4 function. The error handler is invoked + /// when an attempt to open a particular socket fails for any reason. + /// Typically, the error handler will log a warning. When the error + /// handler returns, the openSockets4 function should continue opening + /// sockets on other interfaces. + /// + /// @param errmsg An error string indicating the reason for failure. + void ifaceMgrErrorHandler(const std::string&) { + // Increase the counter of invocations to this function. By checking + // this number, a test amy check if the expected number of errors + // has occurred. + ++errors_count_; + } + + /// Holds the invocation counter for ifaceMgrErrorHandler. + int errors_count_; + }; // We need some known interface to work reliably. Loopback interface is named @@ -1091,6 +1204,157 @@ TEST_F(IfaceMgrTest, socket4) { close(socket1); } +// This test verifies that IPv4 sockets are open on all interfaces (except +// loopback), when interfaces are up, running and active (not disabled from +// the DHCP configuration). +TEST_F(IfaceMgrTest, openSockets4) { + NakedIfaceMgr ifacemgr; + + // Remove all real interfaces and create a set of dummy interfaces. + ifacemgr.createIfaces(); + + // Use the custom packet filter object. This object mimics the socket + // opening operation - the real socket is not open. + boost::shared_ptr custom_packet_filter(new TestPktFilter()); + ASSERT_TRUE(custom_packet_filter); + ASSERT_NO_THROW(ifacemgr.setPacketFilter(custom_packet_filter)); + + // Simulate opening sockets using the dummy packet filter. + ASSERT_NO_THROW(ifacemgr.openSockets4(DHCP4_SERVER_PORT, true, NULL)); + + // Expect that the sockets are open on both eth0 and eth1. + EXPECT_EQ(1, ifacemgr.getIface("eth0")->getSockets().size()); + EXPECT_EQ(1, ifacemgr.getIface("eth1")->getSockets().size()); + // Socket shouldn't have been opened on loopback. + EXPECT_TRUE(ifacemgr.getIface("lo")->getSockets().empty()); +} + +// This test verifies that the socket is not open on the interface which is +// down, but sockets are open on all other non-loopback interfaces. +TEST_F(IfaceMgrTest, openSockets4IfaceDown) { + NakedIfaceMgr ifacemgr; + + // Remove all real interfaces and create a set of dummy interfaces. + ifacemgr.createIfaces(); + + boost::shared_ptr custom_packet_filter(new TestPktFilter()); + ASSERT_TRUE(custom_packet_filter); + ASSERT_NO_THROW(ifacemgr.setPacketFilter(custom_packet_filter)); + + // Boolean parameters specify that eth0 is: + // - not a loopback + // - is "down" (not up) + // - is not running + // - is active (is not inactive) + ifacemgr.setIfaceFlags("eth0", false, false, true, false); + ASSERT_FALSE(ifacemgr.getIface("eth0")->flag_up_); + ASSERT_NO_THROW(ifacemgr.openSockets4(DHCP4_SERVER_PORT, true, NULL)); + + // There should be no socket on eth0 open, because interface was down. + EXPECT_TRUE(ifacemgr.getIface("eth0")->getSockets().empty()); + // Expecting that the socket is open on eth1 because it was up, running + // and active. + EXPECT_EQ(1, ifacemgr.getIface("eth1")->getSockets().size()); + // Never open socket on loopback interface. + EXPECT_TRUE(ifacemgr.getIface("lo")->getSockets().empty()); +} + +// This test verifies that the socket is not open on the interface which is +// disabled from the DHCP configuration, but sockets are open on all other +// non-loopback interfaces. +TEST_F(IfaceMgrTest, openSockets4IfaceInactive) { + NakedIfaceMgr ifacemgr; + + // Remove all real interfaces and create a set of dummy interfaces. + ifacemgr.createIfaces(); + + boost::shared_ptr custom_packet_filter(new TestPktFilter()); + ASSERT_TRUE(custom_packet_filter); + ASSERT_NO_THROW(ifacemgr.setPacketFilter(custom_packet_filter)); + + // Boolean parameters specify that eth1 is: + // - not a loopback + // - is up + // - is running + // - is inactive + ifacemgr.setIfaceFlags("eth1", false, true, true, true); + ASSERT_TRUE(ifacemgr.getIface("eth1")->inactive4_); + ASSERT_NO_THROW(ifacemgr.openSockets4(DHCP4_SERVER_PORT, true, NULL)); + + // The socket on eth0 should be open because interface is up, running and + // active (not disabled through DHCP configuration, for example). + EXPECT_EQ(1, ifacemgr.getIface("eth0")->getSockets().size()); + // There should be no socket open on eth1 because it was marked inactive. + EXPECT_TRUE(ifacemgr.getIface("eth1")->getSockets().empty()); + // Sockets are not open on loopback interfaces too. + EXPECT_TRUE(ifacemgr.getIface("lo")->getSockets().empty()); +} + +// Test that exception is thrown when trying to bind a new socket to the port +// and address which is already in use by another socket. +TEST_F(IfaceMgrTest, openSockets4NoErrorHandler) { + NakedIfaceMgr ifacemgr; + + // Remove all real interfaces and create a set of dummy interfaces. + ifacemgr.createIfaces(); + + boost::shared_ptr custom_packet_filter(new TestPktFilter()); + ASSERT_TRUE(custom_packet_filter); + ASSERT_NO_THROW(ifacemgr.setPacketFilter(custom_packet_filter)); + + // Open socket on eth1. 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. + ASSERT_NO_THROW(ifacemgr.openSocket("eth1", IOAddress("192.0.2.3"), + DHCP4_SERVER_PORT)); + + // The function throws an exception when it tries to open a socket + // and bind it to the address in use. + EXPECT_THROW(ifacemgr.openSockets4(DHCP4_SERVER_PORT, true, NULL), + isc::dhcp::SocketConfigError); + +} + +// Test that the external error handler is called when trying to bind a new +// socket to the address and port being in use. The sockets on the other +// interfaces should open just fine.. +TEST_F(IfaceMgrTest, openSocket4ErrorHandler) { + NakedIfaceMgr ifacemgr; + + // Remove all real interfaces and create a set of dummy interfaces. + ifacemgr.createIfaces(); + + boost::shared_ptr custom_packet_filter(new TestPktFilter()); + 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. + ASSERT_NO_THROW(ifacemgr.openSocket("eth0", IOAddress("10.0.0.1"), + DHCP4_SERVER_PORT)); + + // Install an error handler before trying to open sockets. This handler + // 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.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. + EXPECT_EQ(1, errors_count_); + + // Reset errors count. + errors_count_ = 0; + + // Now that we have two sockets open, we can try this again but this time + // we should get two errors: one when opening a socket on eth0, another one + // when opening a socket on eth1. + ASSERT_NO_THROW(ifacemgr.openSockets4(DHCP4_SERVER_PORT, true, error_handler)); + EXPECT_EQ(2, errors_count_); + +} + + // Test the Iface structure itself TEST_F(IfaceMgrTest, iface) { boost::scoped_ptr iface; diff --git a/src/lib/dhcp/tests/pkt_filter_unittest.cc b/src/lib/dhcp/tests/pkt_filter_unittest.cc index bd48a63772..e677de9252 100644 --- a/src/lib/dhcp/tests/pkt_filter_unittest.cc +++ b/src/lib/dhcp/tests/pkt_filter_unittest.cc @@ -44,9 +44,9 @@ TEST_F(PktFilterBaseClassTest, openFallbackSocket) { // will handle it. PktFilterStub pkt_filter; ASSERT_NO_THROW(sock_info_.fallbackfd_ = - pkt_filter.openFallbackSocket(IOAddress("127.0.0.1"), PORT) - << "Failed to open fallback socket."; - ); + pkt_filter.openFallbackSocket(IOAddress("127.0.0.1"), PORT)) + << "Failed to open fallback socket."; + // Test that the socket has been successfully created. testDgramSocket(sock_info_.fallbackfd_);