From: Alan T. DeKok Date: Mon, 14 Oct 2024 18:59:09 +0000 (-0400) Subject: remove unlang_module_timeout_add() X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=3b42a8aa31011ccd26512ef30382cb0be67dcfd0;p=thirdparty%2Ffreeradius-server.git remove unlang_module_timeout_add() and clean up the API for module timeouts / retries --- diff --git a/src/lib/unlang/module.c b/src/lib/unlang/module.c index f3107b90794..9331ff429eb 100644 --- a/src/lib/unlang/module.c +++ b/src/lib/unlang/module.c @@ -38,63 +38,6 @@ RCSID("$Id$") static unlang_action_t unlang_module_resume(rlm_rcode_t *p_result, request_t *request, unlang_stack_frame_t *frame); static void unlang_module_event_retry_handler(UNUSED fr_event_list_t *el, fr_time_t now, void *ctx); -/** Call the callback registered for a timeout event - * - * @param[in] el the event timer was inserted into. - * @param[in] now The current time, as held by the event_list. - * @param[in] ctx unlang_module_event_t structure holding callbacks. - * - */ -static void unlang_module_event_timeout_handler(UNUSED fr_event_list_t *el, fr_time_t now, void *ctx) -{ - unlang_frame_state_module_t *state = talloc_get_type_abort(ctx, unlang_frame_state_module_t); - - state->timeout(MODULE_CTX(state->mi, state->thread, state->env_data, state->timeout_rctx), state->request, now); -} - -/** Set a timeout for the request. - * - * Used when a module needs wait for an event. Typically the callback is set, and then the - * module returns unlang_module_yield(). - * - * @note The callback is automatically removed when the stack frame returns. - * - * param[in] request the current request. - * param[in] callback to call. - * param[in] rctx to pass to the callback. - * param[in] timeout when to call the timeout (i.e. now + timeout). - * @return - * - 0 on success. - * - <0 on error. - */ -int unlang_module_timeout_add(request_t *request, unlang_module_timeout_t callback, - void *rctx, fr_time_t when) -{ - unlang_stack_t *stack = request->stack; - unlang_stack_frame_t *frame = &stack->frame[stack->depth]; - unlang_module_t *m; - unlang_frame_state_module_t *state = talloc_get_type_abort(frame->state, unlang_frame_state_module_t); - - fr_assert(stack->depth > 0); - fr_assert(frame->instruction->type == UNLANG_TYPE_MODULE); - m = unlang_generic_to_module(frame->instruction); - - state->timeout = callback; - state->timeout_rctx = rctx; - - state->request = request; - state->mi = m->mmc.mi; - - if (fr_event_timer_at(request, unlang_interpret_event_list(request), &state->ev, - when, unlang_module_event_timeout_handler, state) < 0) { - RPEDEBUG("Failed inserting event"); - return -1; - } - - return 0; -} - - /** Push a module or submodule onto the stack for evaluation * * @param[out] p_result Where to write the result of calling the module. @@ -357,7 +300,7 @@ static unlang_action_t unlang_module_retry_resume(rlm_rcode_t *p_result, module_ */ talloc_const_free(state->ev); state->ev = NULL; - state->timeout = NULL; + state->retry_cb = NULL; state->retry.config = NULL; return state->retry_resume(p_result, mctx, request); @@ -384,7 +327,7 @@ static unlang_action_t unlang_module_retry_resume(rlm_rcode_t *p_result, module_ * - UNLANG_ACTION_YIELD on success * - UNLANG_ACTION_FAIL on failure */ -unlang_action_t unlang_module_yield_to_retry(request_t *request, module_method_t resume, unlang_module_timeout_t retry, +unlang_action_t unlang_module_yield_to_retry(request_t *request, module_method_t resume, unlang_module_retry_t retry, unlang_module_signal_t signal, fr_signal_t sigmask, void *rctx, fr_retry_config_t const *retry_cfg) { @@ -397,9 +340,9 @@ unlang_action_t unlang_module_yield_to_retry(request_t *request, module_method_t fr_assert(frame->instruction->type == UNLANG_TYPE_MODULE); m = unlang_generic_to_module(frame->instruction); - fr_assert(!state->timeout); + fr_assert(!state->retry_cb); - state->timeout = retry; + state->retry_cb = retry; state->retry_resume = resume; // so that we can cancel the retry timer state->rctx = rctx; @@ -696,34 +639,34 @@ static void unlang_module_event_retry_handler(UNUSED fr_event_list_t *el, fr_tim unlang_stack_frame_t *frame = &stack->frame[stack->depth]; unlang_frame_state_module_t *state = talloc_get_type_abort(frame->state, unlang_frame_state_module_t); - /* - * The module will get either a RETRY signal, or a - * TIMEOUT signal (also for max count). - * - * The signal handler should generally change the resume - * function, and mark the request as runnable. We - * probably don't want the module to do tons of work in - * the signal handler, as it's called from the event - * loop. And doing so could affect the other event - * timers. - * - * Note also that we call frame->signal(), and not - * unlang_interpret_signal(). That is because we want to - * signal only the module. We know that the other frames - * on the stack can't handle this particular signal. So - * there's no point in calling them. Or, if sections - * have their own retry handlers, then we don't want to - * signal those _other_ retry handles with _our_ signal. - */ switch (fr_retry_next(&state->retry, now)) { case FR_RETRY_CONTINUE: - /* - * There's a timeout / retry handler, call that with a cached "now". - */ - if (state->timeout) { - state->timeout(MODULE_CTX(state->mi, state->thread, state->env_data, state->rctx), state->request, now); + if (state->retry_cb) { + /* + * Call the module retry handler, with the state of the retry. On MRD / MRC, the + * module is made runnable again, and the "resume" function is called. + */ + state->retry_cb(MODULE_CTX(state->mi, state->thread, state->env_data, state->rctx), state->request, &state->retry); } else { - + /* + * For signals, the module will get either a RETRY + * signal, or a TIMEOUT signal (also for max count). + * + * The signal handler should generally change the resume + * function, and mark the request as runnable. We + * probably don't want the module to do tons of work in + * the signal handler, as it's called from the event + * loop. And doing so could affect the other event + * timers. + * + * Note also that we call frame->signal(), and not + * unlang_interpret_signal(). That is because we want to + * signal only the module. We know that the other frames + * on the stack can't handle this particular signal. So + * there's no point in calling them. Or, if sections + * have their own retry handlers, then we don't want to + * signal those _other_ retry handlers with _our_ signal. + */ frame->signal(request, frame, FR_SIGNAL_RETRY); } @@ -738,17 +681,29 @@ static void unlang_module_event_retry_handler(UNUSED fr_event_list_t *el, fr_tim return; case FR_RETRY_MRD: - REDEBUG("Reached max_rtx_duration (%pVs > %pVs) - sending timeout signal", + REDEBUG("Reached max_rtx_duration (%pVs > %pVs) - sending timeout", fr_box_time_delta(fr_time_sub(now, state->retry.start)), fr_box_time_delta(state->retry.config->mrd)); break; case FR_RETRY_MRC: - REDEBUG("Reached max_rtx_count (%u > %u) - sending timeout signal", + REDEBUG("Reached max_rtx_count (%u > %u) - sending timeout", state->retry.count, state->retry.config->mrc); break; } - frame->signal(request, frame, FR_SIGNAL_TIMEOUT); + /* + * Run the retry handler on MRD / MRC, too. + */ + if (state->retry_cb) { + state->retry_cb(MODULE_CTX(state->mi, state->thread, state->env_data, state->rctx), state->request, &state->retry); + } else { + frame->signal(request, frame, FR_SIGNAL_TIMEOUT); + } + + /* + * On final timeout, always mark the request as runnable. + */ + unlang_interpret_mark_runnable(request); } static unlang_action_t unlang_module(rlm_rcode_t *p_result, request_t *request, unlang_stack_frame_t *frame) diff --git a/src/lib/unlang/module.h b/src/lib/unlang/module.h index b783551f8cf..2156606217b 100644 --- a/src/lib/unlang/module.h +++ b/src/lib/unlang/module.h @@ -36,7 +36,7 @@ extern "C" { #include #include -/** A callback when the the timeout occurs +/** A callback when a retry happens * * Used when a module needs wait for an event. * Typically the callback is set, and then the module returns unlang_module_yield(). @@ -47,9 +47,9 @@ extern "C" { * @param[in] mctx calling context for the module. * Contains global, thread-specific, and call-specific data for a module. * @param[in] request the request. - * @param[in] fired the time the timeout event actually fired. + * @param[in] retry retry status. "now" is in retry->updated */ -typedef void (*unlang_module_timeout_t)(module_ctx_t const *mctx, request_t *request, fr_time_t fired); +typedef void (*unlang_module_retry_t)(module_ctx_t const *mctx, request_t *request, fr_retry_t const *retry); /** A callback when the FD is ready for reading * @@ -79,9 +79,6 @@ typedef void (*unlang_module_fd_event_t)(module_ctx_t const *mctx, request_t *re */ typedef void (*unlang_module_signal_t)(module_ctx_t const *mctx, request_t *request, fr_signal_t action); -int unlang_module_timeout_add(request_t *request, unlang_module_timeout_t callback, - void *rctx, fr_time_t when); - int unlang_module_push(rlm_rcode_t *p_result, request_t *request, module_instance_t *module_instance, module_method_t method, bool top_frame) CC_HINT(warn_unused_result) CC_HINT(nonnull(2,3,4)); @@ -105,7 +102,7 @@ unlang_action_t unlang_module_yield_to_tmpl(TALLOC_CTX *ctx, fr_value_box_list_t module_method_t resume, unlang_module_signal_t signal, fr_signal_t sigmask, void *rctx); -unlang_action_t unlang_module_yield_to_retry(request_t *request, module_method_t resume, unlang_module_timeout_t retry, +unlang_action_t unlang_module_yield_to_retry(request_t *request, module_method_t resume, unlang_module_retry_t retry_cb, unlang_module_signal_t signal, fr_signal_t sigmask, void *rctx, fr_retry_config_t const *retry_cfg); diff --git a/src/lib/unlang/module_priv.h b/src/lib/unlang/module_priv.h index 6f54499494f..f19bb00ad70 100644 --- a/src/lib/unlang/module_priv.h +++ b/src/lib/unlang/module_priv.h @@ -79,7 +79,7 @@ typedef struct { * @{ */ module_method_t retry_resume; //!< which stops retries on resume - unlang_module_timeout_t timeout; //!< callback to run on timeout + unlang_module_retry_t retry_cb; //!< callback to run on timeout void *timeout_rctx; //!< rctx data to pass to timeout callback module_instance_t const *mi; //!< Module instance to pass to callbacks. request_t *request; diff --git a/src/modules/rlm_delay/rlm_delay.c b/src/modules/rlm_delay/rlm_delay.c index cedadf80d3d..c3a48b2342b 100644 --- a/src/modules/rlm_delay/rlm_delay.c +++ b/src/modules/rlm_delay/rlm_delay.c @@ -47,13 +47,18 @@ static const conf_parser_t module_config[] = { CONF_PARSER_TERMINATOR }; +typedef struct { + fr_retry_config_t retry_cfg; + fr_time_t when; +} rlm_delay_retry_t; + /** Called when the timeout has expired * * Marks the request as resumable, and prints the delayed delay time. */ -static void _delay_done(module_ctx_t const *mctx, request_t *request, fr_time_t fired) +static void _delay_done(module_ctx_t const *mctx, request_t *request, fr_retry_t const *retry) { - fr_time_t *yielded = talloc_get_type_abort(mctx->rctx, fr_time_t); + rlm_delay_retry_t *yielded = talloc_get_type_abort(mctx->rctx, rlm_delay_retry_t); RDEBUG2("Delay done"); @@ -61,9 +66,7 @@ static void _delay_done(module_ctx_t const *mctx, request_t *request, fr_time_t * timeout should never be *before* the scheduled time, * if it is, something is very broken. */ - if (!fr_cond_assert(fr_time_gteq(fired, *yielded))) REDEBUG("Unexpected resume time"); - - unlang_interpret_mark_runnable(request); + if (!fr_cond_assert(fr_time_gteq(retry->updated, yielded->when))) REDEBUG("Unexpected resume time"); } static void _xlat_delay_done(xlat_ctx_t const *xctx, request_t *request, fr_time_t fired) @@ -117,12 +120,12 @@ static int delay_add(request_t *request, fr_time_t *resume_at, fr_time_t now, */ static unlang_action_t mod_delay_return(rlm_rcode_t *p_result, module_ctx_t const *mctx, request_t *request) { - fr_time_t *yielded = talloc_get_type_abort(mctx->rctx, fr_time_t); + rlm_delay_retry_t *yielded = talloc_get_type_abort(mctx->rctx, rlm_delay_retry_t); /* * Print how long the delay *really* was. */ - RDEBUG3("Request delayed by %pV", fr_box_time_delta(fr_time_sub(fr_time(), *yielded))); + RDEBUG3("Request delayed by %pV", fr_box_time_delta(fr_time_sub(fr_time(), yielded->when))); talloc_free(yielded); RETURN_MODULE_OK; @@ -132,7 +135,8 @@ static unlang_action_t CC_HINT(nonnull) mod_delay(rlm_rcode_t *p_result, module_ { rlm_delay_t const *inst = talloc_get_type_abort_const(mctx->mi->data, rlm_delay_t); fr_time_delta_t delay; - fr_time_t resume_at, *yielded_at; + rlm_delay_retry_t *yielded; + fr_time_t resume_at; if (inst->delay) { if (tmpl_aexpand_type(request, &delay, FR_TYPE_TIME_DELTA, @@ -147,26 +151,30 @@ static unlang_action_t CC_HINT(nonnull) mod_delay(rlm_rcode_t *p_result, module_ /* * Record the time that we yielded the request */ - MEM(yielded_at = talloc(unlang_interpret_frame_talloc_ctx(request), fr_time_t)); - *yielded_at = fr_time(); + MEM(yielded = talloc(unlang_interpret_frame_talloc_ctx(request), rlm_delay_retry_t)); + yielded->when = fr_time(); /* * Setup the delay for this request */ - if (delay_add(request, &resume_at, *yielded_at, delay, + if (delay_add(request, &resume_at, yielded->when, delay, inst->force_reschedule, inst->relative) != 0) { RETURN_MODULE_NOOP; } RDEBUG3("Current time %pVs, resume time %pVs", - fr_box_time(*yielded_at), fr_box_time(resume_at)); + fr_box_time(yielded->when), fr_box_time(resume_at)); - if (unlang_module_timeout_add(request, _delay_done, yielded_at, resume_at) < 0) { - RPEDEBUG("Adding event failed"); - RETURN_MODULE_FAIL; - } + /* + * + */ + yielded->retry_cfg = (fr_retry_config_t) { + .mrd = delay, + .mrc = 1, + }; - return unlang_module_yield(request, mod_delay_return, NULL, 0, yielded_at); + return unlang_module_yield_to_retry(request, mod_delay_return, _delay_done, NULL, 0, + yielded, &yielded->retry_cfg); } static xlat_action_t xlat_delay_resume(TALLOC_CTX *ctx, fr_dcursor_t *out,