From 6147e45a07ae9827d8e83b542a200ef34b8dc695 Mon Sep 17 00:00:00 2001 From: sigiesec Date: Wed, 23 Aug 2017 09:39:51 +0200 Subject: [PATCH] Problem: missing tests for zmq_timers_* corner cases, missing handling of such corner cases, code duplication, missing assertions in test code Solution: add tests, add checks to timers_t, add match_by_id functor, add assertions --- src/timers.cpp | 93 ++++++++++++++++++++++++++++--------------- src/timers.hpp | 2 + tests/test_timers.cpp | 55 +++++++++++++++++++++++-- 3 files changed, 114 insertions(+), 36 deletions(-) diff --git a/src/timers.cpp b/src/timers.cpp index c5471cb6..1474ac2a 100644 --- a/src/timers.cpp +++ b/src/timers.cpp @@ -31,11 +31,10 @@ along with this program. If not, see . #include "timers.hpp" #include "err.hpp" -zmq::timers_t::timers_t () : -tag (0xCAFEDADA), -next_timer_id (0) -{ +#include +zmq::timers_t::timers_t () : tag (0xCAFEDADA), next_timer_id (0) +{ } zmq::timers_t::~timers_t () @@ -49,20 +48,45 @@ bool zmq::timers_t::check_tag () return tag == 0xCAFEDADA; } -int zmq::timers_t::add (size_t interval_, timers_timer_fn handler_, void* arg_) +int zmq::timers_t::add (size_t interval_, timers_timer_fn handler_, void *arg_) { - uint64_t when = clock.now_ms() + interval_; + if (!handler_) { + errno = EFAULT; + return -1; + } + + uint64_t when = clock.now_ms () + interval_; timer_t timer = {++next_timer_id, interval_, handler_, arg_}; timers.insert (timersmap_t::value_type (when, timer)); return timer.timer_id; } +struct zmq::timers_t::match_by_id +{ + match_by_id (int timer_id_) : timer_id (timer_id_) {} + + bool operator() (timersmap_t::value_type const &entry) const + { + return entry.second.timer_id == timer_id; + } + + private: + int timer_id; +}; + int zmq::timers_t::cancel (int timer_id_) { - cancelled_timers_t::iterator it = cancelled_timers.find (timer_id_); + // check first if timer exists at all + if (timers.end () + == std::find_if (timers.begin (), timers.end (), + match_by_id (timer_id_))) { + errno = EINVAL; + return -1; + } - if (it != cancelled_timers.end ()) { + // check if timer was already canceled + if (cancelled_timers.count (timer_id_)) { errno = EINVAL; return -1; } @@ -74,32 +98,35 @@ int zmq::timers_t::cancel (int timer_id_) int zmq::timers_t::set_interval (int timer_id_, size_t interval_) { - for (timersmap_t::iterator it = timers.begin (); it != timers.end (); ++it) { - if (it->second.timer_id == timer_id_) { - timer_t timer = it->second; - timer.interval = interval_; - uint64_t when = clock.now_ms() + interval_; - timers.erase (it); - timers.insert (timersmap_t::value_type (when, timer)); + const timersmap_t::iterator end = timers.end (); + const timersmap_t::iterator it = + std::find_if (timers.begin (), end, match_by_id (timer_id_)); + if (it != end) { + timer_t timer = it->second; + timer.interval = interval_; + uint64_t when = clock.now_ms () + interval_; + timers.erase (it); + timers.insert (timersmap_t::value_type (when, timer)); - return 0; - } + return 0; } errno = EINVAL; return -1; } -int zmq::timers_t::reset (int timer_id_) { - for (timersmap_t::iterator it = timers.begin (); it != timers.end (); ++it) { - if (it->second.timer_id == timer_id_) { - timer_t timer = it->second; - uint64_t when = clock.now_ms() + timer.interval; - timers.erase (it); - timers.insert (timersmap_t::value_type (when, timer)); +int zmq::timers_t::reset (int timer_id_) +{ + const timersmap_t::iterator end = timers.end (); + const timersmap_t::iterator it = + std::find_if (timers.begin (), end, match_by_id (timer_id_)); + if (it != end) { + timer_t timer = it->second; + uint64_t when = clock.now_ms () + timer.interval; + timers.erase (it); + timers.insert (timersmap_t::value_type (when, timer)); - return 0; - } + return 0; } errno = EINVAL; @@ -110,10 +137,11 @@ long zmq::timers_t::timeout () { timersmap_t::iterator it = timers.begin (); - uint64_t now = clock.now_ms(); + uint64_t now = clock.now_ms (); while (it != timers.end ()) { - cancelled_timers_t::iterator cancelled_it = cancelled_timers.find (it->second.timer_id); + cancelled_timers_t::iterator cancelled_it = + cancelled_timers.find (it->second.timer_id); // Live timer, lets return the timeout if (cancelled_it == cancelled_timers.end ()) { @@ -123,7 +151,7 @@ long zmq::timers_t::timeout () return 0; } - // Let's remove it from the begining of the list + // Let's remove it from the beginning of the list timersmap_t::iterator old = it; ++it; timers.erase (old); @@ -138,10 +166,11 @@ int zmq::timers_t::execute () { timersmap_t::iterator it = timers.begin (); - uint64_t now = clock.now_ms(); + uint64_t now = clock.now_ms (); while (it != timers.end ()) { - cancelled_timers_t::iterator cancelled_it = cancelled_timers.find (it->second.timer_id); + cancelled_timers_t::iterator cancelled_it = + cancelled_timers.find (it->second.timer_id); // Dead timer, lets remove it and continue if (cancelled_it != cancelled_timers.end ()) { @@ -154,7 +183,7 @@ int zmq::timers_t::execute () // Map is ordered, if we have to wait for current timer we can stop. if (it->first > now) - break; + break; timer_t timer = it->second; diff --git a/src/timers.hpp b/src/timers.hpp index b4b40fc3..b7c5ce65 100644 --- a/src/timers.hpp +++ b/src/timers.hpp @@ -102,6 +102,8 @@ namespace zmq timers_t (const timers_t&); const timers_t &operator = (const timers_t&); + + struct match_by_id; }; } diff --git a/tests/test_timers.cpp b/tests/test_timers.cpp index 9babbd91..21ec8e6e 100644 --- a/tests/test_timers.cpp +++ b/tests/test_timers.cpp @@ -101,6 +101,45 @@ void test_null_timer_pointers () assert (rc == -1 && errno == EFAULT); } +void test_corner_cases () +{ + void *timers = zmq_timers_new (); + assert (timers); + + const size_t dummy_interval = SIZE_MAX; + const int dummy_timer_id = 1; + + // attempt to cancel non-existent timer + int rc = zmq_timers_cancel (timers, dummy_timer_id); + assert (rc == -1 && errno == EINVAL); + + // attempt to set interval of non-existent timer + rc = zmq_timers_set_interval (timers, dummy_timer_id, dummy_interval); + assert (rc == -1 && errno == EINVAL); + + // attempt to reset non-existent timer + rc = zmq_timers_reset (timers, dummy_timer_id); + assert (rc == -1 && errno == EINVAL); + + // attempt to add NULL handler + rc = zmq_timers_add (timers, dummy_interval, NULL, NULL); + assert (rc == -1 && errno == EFAULT); + + int timer_id = zmq_timers_add (timers, dummy_interval, handler, NULL); + assert (timer_id != -1); + + // attempt to cancel timer twice + // TODO should this case really be an error? canceling twice could be allowed + rc = zmq_timers_cancel (timers, timer_id); + assert (rc == 0); + + rc = zmq_timers_cancel (timers, timer_id); + assert (rc == -1 && errno == EINVAL); + + rc = zmq_timers_destroy (&timers); + assert (rc == 0); +} + int main (void) { setup_test_environment (); @@ -119,7 +158,9 @@ int main (void) assert (!timer_invoked); // Wait half the time and check again - msleep (zmq_timers_timeout (timers) / 2); + long timeout = zmq_timers_timeout(timers); + assert (rc != -1); + msleep (timeout / 2); rc = zmq_timers_execute (timers); assert (rc == 0); assert (!timer_invoked); @@ -131,7 +172,8 @@ int main (void) timer_invoked = false; // Wait half the time and check again - long timeout = zmq_timers_timeout (timers); + timeout = zmq_timers_timeout (timers); + assert (rc != -1); msleep (timeout / 2); rc = zmq_timers_execute (timers); assert (rc == 0); @@ -139,6 +181,7 @@ int main (void) // Reset timer and wait half of the time left rc = zmq_timers_reset (timers, timer_id); + assert (rc == 0); msleep (timeout / 2); rc = zmq_timers_execute (timers); assert (rc == 0); @@ -151,7 +194,8 @@ int main (void) timer_invoked = false; // reschedule - zmq_timers_set_interval (timers, timer_id, 50); + rc = zmq_timers_set_interval (timers, timer_id, 50); + assert (rc == 0); rc = sleep_and_execute(timers); assert (rc == 0); assert (timer_invoked); @@ -159,7 +203,9 @@ int main (void) // cancel timer timeout = zmq_timers_timeout (timers); - zmq_timers_cancel (timers, timer_id); + assert (rc != -1); + rc = zmq_timers_cancel (timers, timer_id); + assert (rc == 0); msleep (timeout * 2); rc = zmq_timers_execute (timers); assert (rc == 0); @@ -169,6 +215,7 @@ int main (void) assert (rc == 0); test_null_timer_pointers (); + test_corner_cases (); return 0; }