From: Marcin Siodelski Date: Fri, 4 Sep 2015 07:47:25 +0000 (+0200) Subject: [3970] Addressed most of the review comments. X-Git-Tag: fd4o6_base~5^2~3 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=fe65dbb1b1b4dae88526e0ba8cfa5283d91041c9;p=thirdparty%2Fkea.git [3970] Addressed most of the review comments. --- diff --git a/src/lib/dhcpsrv/timer_mgr.cc b/src/lib/dhcpsrv/timer_mgr.cc index 956efed7a2..841dad282e 100644 --- a/src/lib/dhcpsrv/timer_mgr.cc +++ b/src/lib/dhcpsrv/timer_mgr.cc @@ -36,7 +36,8 @@ TimerMgr::instance() { } TimerMgr::TimerMgr() - : thread_(), registered_timers_() { + : io_service_(new IOService()), thread_(), + registered_timers_() { } TimerMgr::~TimerMgr() { @@ -81,18 +82,20 @@ TimerMgr::registerTimer(const std::string& timer_name, // create the instance if the IntervalTimer and WatchSocket. It will // also hold the callback, interval and scheduling mode parameters. // This may throw a WatchSocketError if the socket creation fails. - TimerInfo timer_info(getIOService(), callback, interval, scheduling_mode); + TimerInfoPtr timer_info(new TimerInfo(getIOService(), callback, + interval, scheduling_mode)); // Register the WatchSocket in the IfaceMgr and register our own callback // to be executed when the data is received over this socket. The only time // this may fail is when the socket failed to open which would have caused // an exception in the previous call. So we should be safe here. - IfaceMgr::instance().addExternalSocket(timer_info.watch_socket_->getSelectFd(), + IfaceMgr::instance().addExternalSocket(timer_info->watch_socket_.getSelectFd(), boost::bind(&TimerMgr::ifaceMgrCallback, this, timer_name)); // Actually register the timer. - registered_timers_.insert(std::pair(timer_name, timer_info)); + registered_timers_.insert(std::pair(timer_name, + timer_info)); } void @@ -119,10 +122,10 @@ TimerMgr::unregisterTimer(const std::string& timer_name) { // Cancel any pending asynchronous operation and stop the timer. cancel(timer_name); - TimerInfo& timer_info = timer_info_it->second; + const TimerInfoPtr& timer_info = timer_info_it->second; // Unregister the watch socket from the IfaceMgr. - IfaceMgr::instance().deleteExternalSocket(timer_info.watch_socket_->getSelectFd()); + IfaceMgr::instance().deleteExternalSocket(timer_info->watch_socket_.getSelectFd()); // Remove the timer. registered_timers_.erase(timer_info_it); @@ -168,10 +171,10 @@ TimerMgr::setup(const std::string& timer_name) { // Schedule the execution of the timer using the parameters supplied // during the registration. - const TimerInfo& timer_info = timer_info_it->second; - timer_info.interval_timer_->setup(boost::bind(&TimerMgr::timerCallback, this, timer_name), - timer_info.interval_, - timer_info.scheduling_mode_); + const TimerInfoPtr& timer_info = timer_info_it->second; + timer_info->interval_timer_.setup(boost::bind(&TimerMgr::timerCallback, this, timer_name), + timer_info->interval_, + timer_info->scheduling_mode_); } void @@ -188,9 +191,9 @@ TimerMgr::cancel(const std::string& timer_name) { "no such timer registered"); } // Cancel the timer. - timer_info_it->second.interval_timer_->cancel(); + timer_info_it->second->interval_timer_.cancel(); // Clear watch socket, if ready. - timer_info_it->second.watch_socket_->clearReady(); + timer_info_it->second->watch_socket_.clearReady(); } void @@ -218,7 +221,7 @@ TimerMgr::stopThread() { // Stop the IO Service. This will break the IOService::run executed in the // worker thread. The thread will now terminate. - getIOService().post(boost::bind(&IOService::stop, &getIOService())); + getIOService().stop(); // When the worker thread may be waiting on the call to // WatchSocket::markReady until main thread clears the socket. // To unblock the thread we have to clear all sockets to make @@ -235,62 +238,45 @@ TimerMgr::stopThread() { } IOService& TimerMgr::getIOService() const { - // The IO service is now created internally by the TimerMgr and we don't allow - // overriding it with anything as there are currently no use cases for it. - // It is possible that someone may want to obtain this instance to use it - // for something else too, so we return a reference to a static object. - // If we decide to allow setting the IO service object we will have to - // replace this static object with a shared pointer allocated in the - // class constructor. - static asiolink::IOService io_service; - return (io_service); + return (*io_service_); } void TimerMgr::timerCallback(const std::string& timer_name) { - // Run callback. Value of true says "mark socket ready". - watchSocketCallback(timer_name, true); + // Find the specified timer setup. + TimerInfoMap::iterator timer_info_it = registered_timers_.find(timer_name); + if (timer_info_it != registered_timers_.end()) { + // We will mark watch socket ready - write data to a socket to + // interrupt the execution of the select() function. This is + // executed from the worker thread. + const TimerInfoPtr& timer_info = timer_info_it->second; + timer_info->watch_socket_.markReady(); + } } void TimerMgr::ifaceMgrCallback(const std::string& timer_name) { - // Run callback. Value of false says "clear ready socket". - watchSocketCallback(timer_name, false); -} - -void -TimerMgr::watchSocketCallback(const std::string& timer_name, const bool mark_ready) { // Find the specified timer setup. TimerInfoMap::iterator timer_info_it = registered_timers_.find(timer_name); if (timer_info_it != registered_timers_.end()) { - TimerInfo& timer_info = timer_info_it->second; - // This is 'true' when we're executing a callback for the elapsed timer. - // It is supposed to mark watch socket ready - write data to a socket to - // interrupt the execution of the select() function. This is executed - // from the worker thrad and will likely block the thread until the socket - // is cleared. - if (mark_ready) { - timer_info.watch_socket_->markReady(); - } else { - - // We're executing a callback function from the Interface Manager. - // This callback function is executed when the call to select() is - // interrupted as a result of receiving some data over the watch - // socket. We have to clear the socket which has been marked as - // ready. Then execute the callback function supplied by the - // TimerMgr user to perform custom actions on the expiration of - // the given timer. - timer_info.watch_socket_->clearReady(); - - // Running user-defined operation for the timer. Logging it - // on the slightly lower debug level as there may be many - // such traces. - LOG_DEBUG(dhcpsrv_logger, DHCPSRV_DBG_TRACE_DETAIL, - DHCPSRV_TIMERMGR_RUN_TIMER_OPERATION) - .arg(timer_name); - timer_info.user_callback_(); - } + const TimerInfoPtr& timer_info = timer_info_it->second; + // We're executing a callback function from the Interface Manager. + // This callback function is executed when the call to select() is + // interrupted as a result of receiving some data over the watch + // socket. We have to clear the socket which has been marked as + // ready. Then execute the callback function supplied by the + // TimerMgr user to perform custom actions on the expiration of + // the given timer. + timer_info->watch_socket_.clearReady(); + + // Running user-defined operation for the timer. Logging it + // on the slightly lower debug level as there may be many + // such traces. + LOG_DEBUG(dhcpsrv_logger, DHCPSRV_DBG_TRACE_DETAIL, + DHCPSRV_TIMERMGR_RUN_TIMER_OPERATION) + .arg(timer_name); + timer_info->user_callback_(); } } @@ -298,7 +284,7 @@ void TimerMgr::clearReadySockets() { for (TimerInfoMap::iterator timer_info_it = registered_timers_.begin(); timer_info_it != registered_timers_.end(); ++timer_info_it) { - timer_info_it->second.watch_socket_->clearReady(); + timer_info_it->second->watch_socket_.clearReady(); } } diff --git a/src/lib/dhcpsrv/timer_mgr.h b/src/lib/dhcpsrv/timer_mgr.h index e6c4a1b57f..26e3c5b76a 100644 --- a/src/lib/dhcpsrv/timer_mgr.h +++ b/src/lib/dhcpsrv/timer_mgr.h @@ -52,8 +52,8 @@ namespace dhcp { /// performs the tasks to be executed periodically according to the timer's /// interval. /// -/// The registered timer is not executed until the @c TimerMgr::setup -/// method is called for it. +/// The registered timer's interval does not begin to elapse until the +/// @c TimerMgr::setup method is called for it. /// /// The @c TimerMgr uses worker thread to run the timers. The thread is /// started and stopped using the @c TimerMgr::startThread and @@ -70,10 +70,9 @@ namespace dhcp { /// function is invoked for the timer, it obtains the instance of the /// @c util::WatchSocket and marks it "ready". This call effectively /// writes the data to a socket (pipe) which interrupts the call -/// to the @c select() function in the main thread. Note that this -/// operation may block the worker thread until the socket is cleared. -/// When the @c IfaceMgr (in the main thread) detects data transmitted -/// over the external socket it will invoke a callback function +/// to the @c select() function in the main thread. When the +/// @c IfaceMgr (in the main thread) detects data transmitted over +/// the external socket it will invoke a callback function /// associated with this socket. This is the /// @c TimerMgr::ifaceMgrCallback associated with the socket when the /// timer is registered. This callback function is executed/ in the @@ -102,7 +101,6 @@ namespace dhcp { /// not unregistered before the application terminates this will /// likely result in segmentation fault on some systems. /// -/// All timers are associated with the callback function class TimerMgr : public boost::noncopyable { public: @@ -235,20 +233,13 @@ private: /// @param timer_name Unique timer name. void ifaceMgrCallback(const std::string& timer_name); - /// @brief Common method called by the @c TimerMgr::timerCallback and - /// @c TimerMgr::ifaceMgrCallback. - /// - /// @param timer_name Unique timer name. - /// @param mark_ready If 'true' it indicates that the watch socket should - /// be marked 'ready'. If 'false' the socket is cleared. In this case the - /// callback supplied during the timer registration is also executed. - void watchSocketCallback(const std::string& timer_name, - const bool mark_ready); - //@} void clearReadySockets(); + /// @brief Pointer to the io service. + asiolink::IOServicePtr io_service_; + /// @brief Pointer to the worker thread. /// /// This is initially set to NULL until the thread is started using the @@ -263,10 +254,10 @@ private: /// interval timer and other parameters pertaining to it. struct TimerInfo { /// @brief Instance of the watch socket. - util::WatchSocketPtr watch_socket_; + util::WatchSocket watch_socket_; /// @brief Instance of the interval timer. - asiolink::IntervalTimerPtr interval_timer_; + asiolink::IntervalTimer interval_timer_; /// @brief Holds the pointer to the callback supplied when registering /// the timer. @@ -290,15 +281,18 @@ private: const asiolink::IntervalTimer::Callback& user_callback, const long interval, const asiolink::IntervalTimer::Mode& mode) - : watch_socket_(new util::WatchSocket()), - interval_timer_(new asiolink::IntervalTimer(io_service)), + : watch_socket_(), + interval_timer_(io_service), user_callback_(user_callback), interval_(interval), scheduling_mode_(mode) { }; }; + /// @brief A type definition for the pointer to @c TimerInfo structure. + typedef boost::shared_ptr TimerInfoPtr; + /// @brief A type definition for the map holding timers configuration. - typedef std::map TimerInfoMap; + typedef std::map TimerInfoMap; /// @brief Holds mapping of the timer name to the watch socket, timer /// instance and other parameters pertaining to the timer. diff --git a/src/lib/util/watch_socket.cc b/src/lib/util/watch_socket.cc index 8bc3bd38ec..77246732f3 100644 --- a/src/lib/util/watch_socket.cc +++ b/src/lib/util/watch_socket.cc @@ -19,6 +19,7 @@ #include #include +#include #include #include @@ -122,6 +123,7 @@ WatchSocket::closeSocket(std::string& error_string) { // Clear error string. error_string.clear(); + std::ostringstream s; // Close the pipe fds. Technically a close can fail (hugely unlikely) // but there's no recovery for it either. If one does fail we log it // and go on. Plus this is called by the destructor and no one likes @@ -129,7 +131,7 @@ WatchSocket::closeSocket(std::string& error_string) { if (source_ != SOCKET_NOT_VALID) { if (close(source_)) { // An error occured. - error_string = strerror(errno); + s << "Could not close source: " << strerror(errno); } source_ = SOCKET_NOT_VALID; @@ -139,13 +141,15 @@ WatchSocket::closeSocket(std::string& error_string) { if (close(sink_)) { // An error occured. if (error_string.empty()) { - error_string = strerror(errno); + s << "could not close sink: " << strerror(errno); } } sink_ = SOCKET_NOT_VALID; } + error_string = s.str(); + // If any errors have been reported, return false. return (error_string.empty() ? true : false); }