From b71ae9b1a704a9631698d7d4a92b1dfdfc9a0f69 Mon Sep 17 00:00:00 2001 From: Paul Fertser Date: Wed, 11 Feb 2015 10:51:17 +0300 Subject: [PATCH] target: fix timer callbacks processing MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit Warning, behaviour change: before this patch if a timer callback returned an error, the other handlers in the list were not called. This patch fixes two different issues with the way timer callbacks are called: 1. The function is not designed to be reentrant but a nested call is possible via: target_handle timer event -> poll -> target events before/after reexaminantion -> script_command_run -> target_call_timer_callbacks_now . This patch makes function a no-op when called recursively; 2. The current code can deal with the case when calling a handler leads to its removal but not when it leads to removal of the next callback in the list. This patch defers actual removal to consolidate it with the calling loop. These bugs were exposed by Valgrind. Change-Id: Ia628a744634f5d2911eb329747e826cb9772e789 Signed-off-by: Paul Fertser Reviewed-on: http://openocd.zylin.com/2541 Tested-by: jenkins Reviewed-by: Stian Skjelstad Reviewed-by: Andreas Färber Reviewed-by: Spencer Oliver --- src/target/target.c | 60 +++++++++++++++++++++++++-------------------- src/target/target.h | 1 + 2 files changed, 35 insertions(+), 26 deletions(-) diff --git a/src/target/target.c b/src/target/target.c index fb141789..ef456856 100644 --- a/src/target/target.c +++ b/src/target/target.c @@ -1378,6 +1378,7 @@ int target_register_timer_callback(int (*callback)(void *priv), int time_ms, int (*callbacks_p)->callback = callback; (*callbacks_p)->periodic = periodic; (*callbacks_p)->time_ms = time_ms; + (*callbacks_p)->removed = false; gettimeofday(&now, NULL); (*callbacks_p)->when.tv_usec = now.tv_usec + (time_ms % 1000) * 1000; @@ -1438,24 +1439,18 @@ int target_unregister_reset_callback(int (*callback)(struct target *target, int target_unregister_timer_callback(int (*callback)(void *priv), void *priv) { - struct target_timer_callback **p = &target_timer_callbacks; - struct target_timer_callback *c = target_timer_callbacks; - if (callback == NULL) return ERROR_COMMAND_SYNTAX_ERROR; - while (c) { - struct target_timer_callback *next = c->next; + for (struct target_timer_callback *c = target_timer_callbacks; + c; c = c->next) { if ((c->callback == callback) && (c->priv == priv)) { - *p = next; - free(c); + c->removed = true; return ERROR_OK; - } else - p = &(c->next); - c = next; + } } - return ERROR_OK; + return ERROR_FAIL; } int target_call_event_callbacks(struct target *target, enum target_event event) @@ -1522,31 +1517,44 @@ static int target_call_timer_callback(struct target_timer_callback *cb, static int target_call_timer_callbacks_check_time(int checktime) { + static bool callback_processing; + + /* Do not allow nesting */ + if (callback_processing) + return ERROR_OK; + + callback_processing = true; + keep_alive(); struct timeval now; gettimeofday(&now, NULL); - struct target_timer_callback *callback = target_timer_callbacks; - while (callback) { - /* cleaning up may unregister and free this callback */ - struct target_timer_callback *next_callback = callback->next; + /* Store an address of the place containing a pointer to the + * next item; initially, that's a standalone "root of the + * list" variable. */ + struct target_timer_callback **callback = &target_timer_callbacks; + while (*callback) { + if ((*callback)->removed) { + struct target_timer_callback *p = *callback; + *callback = (*callback)->next; + free(p); + continue; + } - bool call_it = callback->callback && - ((!checktime && callback->periodic) || - now.tv_sec > callback->when.tv_sec || - (now.tv_sec == callback->when.tv_sec && - now.tv_usec >= callback->when.tv_usec)); + bool call_it = (*callback)->callback && + ((!checktime && (*callback)->periodic) || + now.tv_sec > (*callback)->when.tv_sec || + (now.tv_sec == (*callback)->when.tv_sec && + now.tv_usec >= (*callback)->when.tv_usec)); - if (call_it) { - int retval = target_call_timer_callback(callback, &now); - if (retval != ERROR_OK) - return retval; - } + if (call_it) + target_call_timer_callback(*callback, &now); - callback = next_callback; + callback = &(*callback)->next; } + callback_processing = false; return ERROR_OK; } diff --git a/src/target/target.h b/src/target/target.h index d46571e7..f709d6a5 100644 --- a/src/target/target.h +++ b/src/target/target.h @@ -294,6 +294,7 @@ struct target_timer_callback { int (*callback)(void *priv); int time_ms; int periodic; + bool removed; struct timeval when; void *priv; struct target_timer_callback *next; -- 2.39.5