From: Marcin Siodelski Date: Mon, 19 Jan 2015 13:31:50 +0000 (+0100) Subject: [3668] Addressed review comments. X-Git-Tag: trac3712_base~63^2~3 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=1f4950f59e12e714aa07edb79e5563b0f5369bb3;p=thirdparty%2Fkea.git [3668] Addressed review comments. --- diff --git a/src/bin/d2/d2_process.cc b/src/bin/d2/d2_process.cc index 970543bccd..73e10fb458 100644 --- a/src/bin/d2/d2_process.cc +++ b/src/bin/d2/d2_process.cc @@ -1,4 +1,4 @@ -// Copyright (C) 2013-2014 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2013-2015 Internet Systems Consortium, Inc. ("ISC") // // Permission to use, copy, modify, and/or distribute this software for any // purpose with or without fee is hereby granted, provided that the above diff --git a/src/bin/d2/d2_process.h b/src/bin/d2/d2_process.h index 96d860e299..fa624ddb52 100644 --- a/src/bin/d2/d2_process.h +++ b/src/bin/d2/d2_process.h @@ -1,4 +1,4 @@ -// Copyright (C) 2013 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2013, 2015 Internet Systems Consortium, Inc. ("ISC") // // Permission to use, copy, modify, and/or distribute this software for any // purpose with or without fee is hereby granted, provided that the above diff --git a/src/bin/d2/d2_update_mgr.cc b/src/bin/d2/d2_update_mgr.cc index da84945b26..b72a5ec552 100644 --- a/src/bin/d2/d2_update_mgr.cc +++ b/src/bin/d2/d2_update_mgr.cc @@ -1,4 +1,4 @@ -// Copyright (C) 2013 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2013, 2015 Internet Systems Consortium, Inc. ("ISC") // // Permission to use, copy, modify, and/or distribute this software for any // purpose with or without fee is hereby granted, provided that the above diff --git a/src/bin/d2/tests/io_service_signal_unittests.cc b/src/bin/d2/tests/io_service_signal_unittests.cc index 4d06c2122a..88e6d24d7d 100644 --- a/src/bin/d2/tests/io_service_signal_unittests.cc +++ b/src/bin/d2/tests/io_service_signal_unittests.cc @@ -1,4 +1,4 @@ -// Copyright (C) 2014-2016 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2014-2015 Internet Systems Consortium, Inc. ("ISC") // // Permission to use, copy, modify, and/or distribute this software for any // purpose with or without fee is hereby granted, provided that the above diff --git a/src/bin/dhcp4/dhcp4_messages.mes b/src/bin/dhcp4/dhcp4_messages.mes index 4d16ba913e..19b88bdbbc 100644 --- a/src/bin/dhcp4/dhcp4_messages.mes +++ b/src/bin/dhcp4/dhcp4_messages.mes @@ -1,4 +1,4 @@ -# Copyright (C) 2012-2014 Internet Systems Consortium, Inc. ("ISC") +# Copyright (C) 2012-2015 Internet Systems Consortium, Inc. ("ISC") # # Permission to use, copy, modify, and/or distribute this software for any # purpose with or without fee is hereby granted, provided that the above @@ -187,7 +187,7 @@ specified client after receiving a REQUEST message from it. There are many possible reasons for such a failure. Additional messages will indicate the reason. -% DHCP4_LEASE_DATABASE_TIMERS_EXEC_FAIL failed to execute the timers for lease database: %1 +% DHCP4_LEASE_DATABASE_TIMERS_EXEC_FAIL failed to execute timer-based functions for lease database: %1 A warning message executed when a server process is unable to execute the periodic actions for the lease database. An example of the periodic action is a Lease File Cleanup. One of the reasons for the failure is diff --git a/src/bin/dhcp4/dhcp4_srv.cc b/src/bin/dhcp4/dhcp4_srv.cc index 8b23755449..d6c016bdec 100644 --- a/src/bin/dhcp4/dhcp4_srv.cc +++ b/src/bin/dhcp4/dhcp4_srv.cc @@ -150,26 +150,21 @@ Dhcpv4Srv::sendPacket(const Pkt4Ptr& packet) { bool Dhcpv4Srv::run() { while (!shutdown_) { - /// @todo Currently we're using the fixed value of the timeout for - /// select. This value shouldn't be changed. Keeping it at 1s - /// guarantees that the main loop will be executed at least once - /// a seconds allowing for executing the interval timers associated - /// with the lease database backend in use. The intervals for these - /// timers are configured using the unit of 1 second. Bumping up - /// the select timeout would cause the timers to go out of sync - /// with the configured value. - /// Probing for the packets at this pace should not cause a - /// significant rise of the CPU usage. However, in the future we - /// should adjust the select timeout to the value reported by the - /// lease database backend as a maximum poll interval. - //cppcheck-suppress variableScope This is temporary anyway - const int timeout = 1; - // client's message and server's response Pkt4Ptr query; Pkt4Ptr rsp; try { + // The lease database backend may install some timers for which + // the handlers need to be executed periodically. Retrieve the + // maximum interval at which the handlers must be executed from + // the lease manager. + uint32_t timeout = LeaseMgrFactory::instance().getIOServiceExecInterval(); + // If the returned value is zero it means that there are no + // timers installed, so use a default value. + if (timeout == 0) { + timeout = 1000; + } query = receivePacket(timeout); } catch (const SignalInterruptOnSelect) { @@ -197,7 +192,7 @@ Dhcpv4Srv::run() { // Execute ready timers for the lease database, e.g. Lease File Cleanup. try { - LeaseMgrFactory::instance().getIOService()->get_io_service().poll(); + LeaseMgrFactory::instance().getIOService()->poll(); } catch (const std::exception& ex) { LOG_WARN(dhcp4_logger, DHCP4_LEASE_DATABASE_TIMERS_EXEC_FAIL) diff --git a/src/bin/dhcp6/dhcp6_messages.mes b/src/bin/dhcp6/dhcp6_messages.mes index 9caa7338d6..f72b80a8ca 100644 --- a/src/bin/dhcp6/dhcp6_messages.mes +++ b/src/bin/dhcp6/dhcp6_messages.mes @@ -275,7 +275,7 @@ failed to grant a non-temporary address lease for the client. There may be many reasons for such failure. Each failure is logged in a separate log entry. -% DHCP6_LEASE_DATABASE_TIMERS_EXEC_FAIL failed to execute the timers for lease database: %1 +% DHCP6_LEASE_DATABASE_TIMERS_EXEC_FAIL failed to execute timer-based functions for lease database: %1 A warning message executed when a server process is unable to execute the periodic actions for the lease database. An example of the periodic action is a Lease File Cleanup. One of the reasons for the failure is diff --git a/src/bin/dhcp6/dhcp6_srv.cc b/src/bin/dhcp6/dhcp6_srv.cc index a8d6ef82c7..1db773de98 100644 --- a/src/bin/dhcp6/dhcp6_srv.cc +++ b/src/bin/dhcp6/dhcp6_srv.cc @@ -230,26 +230,21 @@ Dhcpv6Srv::testUnicast(const Pkt6Ptr& pkt) const { bool Dhcpv6Srv::run() { while (!shutdown_) { - /// @todo Currently we're using the fixed value of the timeout for - /// select. This value shouldn't be changed. Keeping it at 1s - /// guarantees that the main loop will be executed at least once - /// a seconds allowing for executing the interval timers associated - /// with the lease database backend in use. The intervals for these - /// timers are configured using the unit of 1 second. Bumping up - /// the select timeout would cause the timers to go out of sync - /// with the configured value. - /// Probing for the packets at this pace should not cause a - /// significant rise of the CPU usage. However, in the future we - /// should adjust the select timeout to the value reported by the - /// lease database backend as a maximum poll interval. - //cppcheck-suppress variableScope This is temporary anyway - const int timeout = 1; - // client's message and server's response Pkt6Ptr query; Pkt6Ptr rsp; try { + // The lease database backend may install some timers for which + // the handlers need to be executed periodically. Retrieve the + // maximum interval at which the handlers must be executed from + // the lease manager. + uint32_t timeout = LeaseMgrFactory::instance().getIOServiceExecInterval(); + // If the returned value is zero it means that there are no + // timers installed, so use a default value. + if (timeout == 0) { + timeout = 1000; + } query = receivePacket(timeout); } catch (const SignalInterruptOnSelect) { @@ -276,7 +271,7 @@ bool Dhcpv6Srv::run() { // Execute ready timers for the lease database, e.g. Lease File Cleanup. try { - LeaseMgrFactory::instance().getIOService()->get_io_service().poll(); + LeaseMgrFactory::instance().getIOService()->poll(); } catch (const std::exception& ex) { LOG_WARN(dhcp6_logger, DHCP6_LEASE_DATABASE_TIMERS_EXEC_FAIL) diff --git a/src/lib/asiolink/interval_timer.cc b/src/lib/asiolink/interval_timer.cc index 12ee89b7d8..41efba9bc4 100644 --- a/src/lib/asiolink/interval_timer.cc +++ b/src/lib/asiolink/interval_timer.cc @@ -1,4 +1,4 @@ -// Copyright (C) 2011, 2104 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2011, 2014 Internet Systems Consortium, Inc. ("ISC") // // Permission to use, copy, modify, and/or distribute this software for any // purpose with or without fee is hereby granted, provided that the above diff --git a/src/lib/asiolink/io_service.cc b/src/lib/asiolink/io_service.cc index f6cedaae23..7c8272ccdb 100644 --- a/src/lib/asiolink/io_service.cc +++ b/src/lib/asiolink/io_service.cc @@ -1,4 +1,4 @@ -// Copyright (C) 2011,2013 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2011, 2013, 2015 Internet Systems Consortium, Inc. ("ISC") // // Permission to use, copy, modify, and/or distribute this software for any // purpose with or without fee is hereby granted, provided that the above @@ -59,14 +59,26 @@ public: /// /// This method does not return control to the caller until /// the \c stop() method is called via some handler. - void run() { io_service_.run(); }; + void run() { + io_service_.run(); + }; /// \brief Run the underlying event loop for a single event. /// /// This method return control to the caller as soon as the /// first handler has completed. (If no handlers are ready when /// it is run, it will block until one is.) - void run_one() { io_service_.run_one();} ; + void run_one() { + io_service_.run_one(); + }; + + /// \brief Run the underlying event loop for a ready events. + /// + /// This method executes handlers for all ready events and returns. + /// It will return immediately if there are no ready events. + void poll() { + io_service_.poll(); + }; /// \brief Stop the underlying event loop. /// @@ -107,6 +119,11 @@ IOService::run_one() { io_impl_->run_one(); } +void +IOService::poll() { + io_impl_->poll(); +} + void IOService::stop() { io_impl_->stop(); diff --git a/src/lib/asiolink/io_service.h b/src/lib/asiolink/io_service.h index 2b7e4d6f95..f319d1d309 100644 --- a/src/lib/asiolink/io_service.h +++ b/src/lib/asiolink/io_service.h @@ -1,4 +1,4 @@ -// Copyright (C) 2011,2013 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2011, 2013, 2015 Internet Systems Consortium, Inc. ("ISC") // // Permission to use, copy, modify, and/or distribute this software for any // purpose with or without fee is hereby granted, provided that the above @@ -59,6 +59,12 @@ public: /// it is run, it will block until one is.) void run_one(); + /// \brief Run the underlying event loop for a ready events. + /// + /// This method executes handlers for all ready events and returns. + /// It will return immediately if there are no ready events. + void poll(); + /// \brief Stop the underlying event loop. /// /// This will return the control to the caller of the \c run() method. diff --git a/src/lib/asiolink/tests/io_service_unittest.cc b/src/lib/asiolink/tests/io_service_unittest.cc index 2fe4f12722..5e3e840670 100644 --- a/src/lib/asiolink/tests/io_service_unittest.cc +++ b/src/lib/asiolink/tests/io_service_unittest.cc @@ -1,4 +1,4 @@ -// Copyright (C) 2013 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2013, 2015 Internet Systems Consortium, Inc. ("ISC") // // Permission to use, copy, modify, and/or distribute this software for any // purpose with or without fee is hereby granted, provided that the above @@ -34,6 +34,7 @@ TEST(IOService, post) { // Post two events service.post(boost::bind(&postedEvent, &called, 1)); service.post(boost::bind(&postedEvent, &called, 2)); + service.post(boost::bind(&postedEvent, &called, 3)); // They have not yet been called EXPECT_TRUE(called.empty()); // Process two events @@ -43,6 +44,11 @@ TEST(IOService, post) { ASSERT_EQ(2, called.size()); EXPECT_EQ(1, called[0]); EXPECT_EQ(2, called[1]); + + // Test that poll() executes the last handler. + service.poll(); + ASSERT_EQ(3, called.size()); + EXPECT_EQ(3, called[2]); } } diff --git a/src/lib/dhcpsrv/dhcpsrv_messages.mes b/src/lib/dhcpsrv/dhcpsrv_messages.mes index 5118807e8b..35746a00b3 100644 --- a/src/lib/dhcpsrv/dhcpsrv_messages.mes +++ b/src/lib/dhcpsrv/dhcpsrv_messages.mes @@ -303,13 +303,13 @@ A debug message issued when DHCPv6 lease is being loaded from the file to memory. % DHCPSRV_MEMFILE_LFC_SETUP setting up the Lease File Cleanup interval to %1 sec -An info message logged when the Memfile lease database backend configures -the LFC to be executed periodically. An argument holds the interval in seconds -in which the LFC will be executed. +An informational message logged when the Memfile lease database backend +configures the LFC to be executed periodically. The argument holds the +interval in seconds in which the LFC will be executed. % DHCPSRV_MEMFILE_LFC_START starting Lease File Cleanup -An info message issued when the Memfile lease database backend starts the -periodic Lease File Cleanup. +An informational message issued when the Memfile lease database backend +starts the periodic Lease File Cleanup. % DHCPSRV_MEMFILE_NO_STORAGE running in non-persistent mode, leases will be lost after restart A warning message issued when writes of leases to disk have been disabled diff --git a/src/lib/dhcpsrv/lease_mgr.h b/src/lib/dhcpsrv/lease_mgr.h index f3e8096fc9..07a83ab165 100644 --- a/src/lib/dhcpsrv/lease_mgr.h +++ b/src/lib/dhcpsrv/lease_mgr.h @@ -391,6 +391,25 @@ public: /// @brief returns value of the parameter virtual std::string getParameter(const std::string& name) const; + /// @brief Returns the interval at which the @c IOService events should + /// be released. + /// + /// The implementations of this class may install the timers which + /// periodically trigger event handlers defined for them. Depending + /// on the intervals specified for these timers the @c IOService::poll, + /// @c IOService::run etc. have to be executed to allow the timers + /// for checking whether they have already expired and the handler + /// must be executed. Running the @c IOService with a lower interval + /// would cause the desynchronization of timers with the clock. + /// + /// @return A maximum interval in seconds at which the @c IOService + /// should be executed. A value of 0 means that no timers are installed + /// and that there is no requirement for the @c IOService to be + /// executed at any specific interval. + virtual uint32_t getIOServiceExecInterval() const { + return (0); + } + /// @brief Returns a reference to the @c IOService object used /// by the Lease Manager. const asiolink::IOServicePtr& getIOService() const { diff --git a/src/lib/dhcpsrv/memfile_lease_mgr.cc b/src/lib/dhcpsrv/memfile_lease_mgr.cc index 0b08fca94a..1086f177a3 100644 --- a/src/lib/dhcpsrv/memfile_lease_mgr.cc +++ b/src/lib/dhcpsrv/memfile_lease_mgr.cc @@ -49,7 +49,7 @@ Memfile_LeaseMgr::Memfile_LeaseMgr(const ParameterMap& parameters) LOG_WARN(dhcpsrv_logger, DHCPSRV_MEMFILE_NO_STORAGE); } else { - initTimers(universe == "4" ? V4 : V6); + initTimers(); } } @@ -412,6 +412,11 @@ Memfile_LeaseMgr::rollback() { DHCPSRV_MEMFILE_ROLLBACK); } +uint32_t +Memfile_LeaseMgr::getIOServiceExecInterval() const { + return (static_cast(lfc_timer_.getInterval() / 1000)); +} + std::string Memfile_LeaseMgr::getDefaultLeaseFilePath(Universe u) const { std::ostringstream s; @@ -472,7 +477,7 @@ Memfile_LeaseMgr::initLeaseFilePath(Universe u) { } void -Memfile_LeaseMgr::initTimers(const Universe& universe) { +Memfile_LeaseMgr::initTimers() { std::string lfc_interval_str = "0"; try { lfc_interval_str = getParameter("lfc-interval"); diff --git a/src/lib/dhcpsrv/memfile_lease_mgr.h b/src/lib/dhcpsrv/memfile_lease_mgr.h index 6568fbe417..b1b70477a6 100644 --- a/src/lib/dhcpsrv/memfile_lease_mgr.h +++ b/src/lib/dhcpsrv/memfile_lease_mgr.h @@ -323,6 +323,19 @@ public: /// support transactions, this is a no-op. virtual void rollback(); + /// @brief Returns the interval at which the @c IOService events should + /// be released. + /// + /// The Memfile backend may install a timer to execute the Lease File + /// Cleanup periodically. If this timer is installed, the method returns + /// the LFC interval in milliseconds. + /// + /// @return A maximum interval (in seconds) at which the @c IOService + /// should be executed. A value of 0 means that no timers are installed + /// and that there is no requirement for the @c IOService to be + /// executed at any specific interval. + virtual uint32_t getIOServiceExecInterval() const; + /// @brief Returns default path to the lease file. /// /// @param u Universe (V4 or V6). @@ -430,9 +443,7 @@ private: /// /// Currently only one timer is supported. This timer executes the /// Lease File Cleanup periodically. - /// - /// @param universe A V4 or V6 value. - void initTimers(const Universe& universe); + void initTimers(); // This is a multi-index container, which holds elements that can // be accessed using different search indexes. diff --git a/src/lib/dhcpsrv/tests/memfile_lease_mgr_unittest.cc b/src/lib/dhcpsrv/tests/memfile_lease_mgr_unittest.cc index 4916d6e7b2..22c8072bf8 100644 --- a/src/lib/dhcpsrv/tests/memfile_lease_mgr_unittest.cc +++ b/src/lib/dhcpsrv/tests/memfile_lease_mgr_unittest.cc @@ -288,6 +288,9 @@ TEST_F(MemfileLeaseMgrTest, lfcTimer) { boost::scoped_ptr lease_mgr(new LFCMemfileLeaseMgr(pmap)); + // Check that the interval is correct. + EXPECT_EQ(1, lease_mgr->getIOServiceExecInterval()); + io_service_ = lease_mgr->getIOService(); // Run the test for at most 2.9 seconds. @@ -314,6 +317,8 @@ TEST_F(MemfileLeaseMgrTest, lfcTimerDisabled) { boost::scoped_ptr lease_mgr(new LFCMemfileLeaseMgr(pmap)); + EXPECT_EQ(0, lease_mgr->getIOServiceExecInterval()); + io_service_ = lease_mgr->getIOService(); // Run the test for at most 1.9 seconds. @@ -326,6 +331,31 @@ TEST_F(MemfileLeaseMgrTest, lfcTimerDisabled) { EXPECT_EQ(0, lease_mgr->getLFCCount()); } +// Test that the backend returns a correct value of the interval +// at which the IOService must be executed to run the handlers +// for the installed timers. +TEST_F(MemfileLeaseMgrTest, getIOServiceExecInterval) { + LeaseMgr::ParameterMap pmap; + pmap["type"] = "memfile"; + pmap["universe"] = "4"; + pmap["name"] = getLeaseFilePath("leasefile4_0.csv"); + + // The lfc-interval is not set, so the returned value should be 0. + boost::scoped_ptr lease_mgr(new LFCMemfileLeaseMgr(pmap)); + EXPECT_EQ(0, lease_mgr->getIOServiceExecInterval()); + + // lfc-interval = 10 + pmap["lfc-interval"] = 10; + lease_mgr.reset(new LFCMemfileLeaseMgr(pmap)); + EXPECT_EQ(10, lease_mgr->getIOServiceExecInterval()); + + // lfc-interval = 20 + pmap["lfc-interval"] = 20; + lease_mgr.reset(new LFCMemfileLeaseMgr(pmap)); + EXPECT_EQ(10, lease_mgr->getIOServiceExecInterval()); + +} + // Checks that adding/getting/deleting a Lease6 object works. TEST_F(MemfileLeaseMgrTest, addGetDelete6) { startBackend(V6);