From: Arran Cudbard-Bell Date: Thu, 3 Jun 2021 21:06:57 +0000 (-0500) Subject: Don't automatically make the frame repeatable on yield X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=f6e29e1396bd5e7e53dd55e8585ec33cd02f500c;p=thirdparty%2Ffreeradius-server.git Don't automatically make the frame repeatable on yield This allows modules to push things onto the stack and remove themselves from the return path. ...also clear the repeatable flag before calling the process function. This lets modules set resume functions, that'll always be called, no matter what the unlang action. --- diff --git a/src/lib/unlang/interpret.c b/src/lib/unlang/interpret.c index 670b88f46e2..af0871cc732 100644 --- a/src/lib/unlang/interpret.c +++ b/src/lib/unlang/interpret.c @@ -374,6 +374,17 @@ unlang_frame_action_t frame_eval(request_t *request, unlang_stack_frame_t *frame unlang_ops[instruction->type].name); fr_assert(frame->process != NULL); + + /* + * Clear the repeatable flag so this frame + * won't get executed again unless it specifically + * requests it. + * + * The flag may still be set again during the + * process function to indicate that the frame + * should be evaluated again. + */ + repeatable_clear(frame); action = frame->process(result, request, frame); RDEBUG4("** [%i] %s << %s (%d)", stack->depth, __FUNCTION__, @@ -417,7 +428,6 @@ unlang_frame_action_t frame_eval(request_t *request, unlang_stack_frame_t *frame * called the interpret. */ case UNLANG_ACTION_YIELD: - repeatable_set(frame); yielded_set(frame); RDEBUG4("** [%i] %s - yielding with current (%s %d)", stack->depth, __FUNCTION__, fr_table_str_by_value(mod_rcode_table, frame->result, ""), @@ -433,8 +443,6 @@ unlang_frame_action_t frame_eval(request_t *request, unlang_stack_frame_t *frame case UNLANG_ACTION_CALCULATE_RESULT: fr_assert(*result != RLM_MODULE_UNKNOWN); - repeatable_clear(frame); - if (unlang_ops[instruction->type].debug_braces) { REXDENT(); diff --git a/src/lib/unlang/map.c b/src/lib/unlang/map.c index 84f7dfb7dbc..9850dddd5eb 100644 --- a/src/lib/unlang/map.c +++ b/src/lib/unlang/map.c @@ -131,6 +131,8 @@ static unlang_action_t list_mod_create(rlm_rcode_t *p_result, request_t *request for (map = fr_dcursor_current(&update_state->maps); map; map = fr_dcursor_next(&update_state->maps)) { + repeatable_set(frame); /* Call us again when done */ + switch (update_state->state) { case UNLANG_UPDATE_MAP_INIT: update_state->state = UNLANG_UPDATE_MAP_EXPANDED_LHS; @@ -166,7 +168,7 @@ static unlang_action_t list_mod_create(rlm_rcode_t *p_result, request_t *request fr_assert(0); error: TALLOC_FREE(frame->state); - + repeatable_clear(frame); *p_result = RLM_MODULE_FAIL; return UNLANG_ACTION_CALCULATE_RESULT; diff --git a/src/lib/unlang/module.c b/src/lib/unlang/module.c index 12b33aa8489..467efad6075 100644 --- a/src/lib/unlang/module.c +++ b/src/lib/unlang/module.c @@ -54,6 +54,8 @@ typedef struct { fr_event_timer_t const *ev; //!< Event in this worker's event heap. } unlang_module_event_t; +static unlang_action_t unlang_module_resume(rlm_rcode_t *p_result, request_t *request, unlang_stack_frame_t *frame); + /** Call the callback registered for a read I/O event * * @param[in] el containing the event (not passed to the callback). @@ -549,6 +551,47 @@ unlang_action_t unlang_module_yield_to_section(rlm_rcode_t *p_result, return UNLANG_ACTION_PUSHED_CHILD; } +/** Yield a request back to the interpreter from within a module + * + * This passes control of the request back to the unlang interpreter, setting + * callbacks to execute when the request is 'signalled' asynchronously, or whatever + * timer or I/O event the module was waiting for occurs. + * + * @note The module function which calls #unlang_module_yield should return control + * of the C stack to the unlang interpreter immediately after calling #unlang_module_yield. + * A common pattern is to use ``return unlang_module_yield(...)``. + * + * @param[in] request The current request. + * @param[in] resume Called on unlang_interpret_mark_runnable(). + * @param[in] signal Called on unlang_action(). + * @param[in] rctx to pass to the callbacks. + * @return + * - UNLANG_ACTION_YIELD. + */ +unlang_action_t unlang_module_yield(request_t *request, + unlang_module_resume_t resume, unlang_module_signal_t signal, void *rctx) +{ + unlang_stack_t *stack = request->stack; + 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); + + REQUEST_VERIFY(request); /* Check the yielded request is sane */ + + state->rctx = rctx; + state->resume = resume; + state->signal = signal; + + /* + * We set the repeatable flag here, + * so that the resume function is always + * called going back up the stack. + */ + frame->process = unlang_module_resume; + repeatable_set(frame); + + return UNLANG_ACTION_YIELD; +} + /* * Lock the mutex for the module */ @@ -606,18 +649,66 @@ static void unlang_module_signal(request_t *request, unlang_stack_frame_t *frame } } +/** Cleanup after a module completes + * + */ +static unlang_action_t unlang_module_done(rlm_rcode_t *p_result, request_t *request, unlang_stack_frame_t *frame) +{ + unlang_frame_state_module_t *state = talloc_get_type_abort(frame->state, unlang_frame_state_module_t); + rlm_rcode_t rcode = state-> set_rcode ? state->rcode : *p_result; + +#ifndef NDEBUG + fr_assert(state->unlang_indent == request->log.unlang_indent); +#endif + + fr_assert(rcode >= RLM_MODULE_REJECT); + fr_assert(rcode < RLM_MODULE_UNKNOWN); + + RDEBUG("%s (%s)", frame->instruction->name ? frame->instruction->name : "", + fr_table_str_by_value(mod_rcode_table, rcode, "")); + + request->rcode = rcode; + if (state->p_result) *state->p_result = rcode; /* Inform our caller if we have one */ + *p_result = rcode; + + return UNLANG_ACTION_CALCULATE_RESULT; +} + +/** Cleanup after a yielded module completes + * + */ +static unlang_action_t unlang_module_resume_done(rlm_rcode_t *p_result, request_t *request, unlang_stack_frame_t *frame) +{ + unlang_frame_state_module_t *state = talloc_get_type_abort(frame->state, unlang_frame_state_module_t); + + state->thread->active_callers--; + + return unlang_module_done(p_result, request, frame); +} + /** Wrapper to call a module's resumption function * + * This is called _after_ the module first yields, and again after any + * other yields. */ static unlang_action_t unlang_module_resume(rlm_rcode_t *p_result, request_t *request, unlang_stack_frame_t *frame) { unlang_frame_state_module_t *state = talloc_get_type_abort(frame->state, unlang_frame_state_module_t); unlang_module_t *mc = unlang_generic_to_module(frame->instruction); + unlang_module_resume_t resume; char const *caller; - rlm_rcode_t rcode = *p_result; - unlang_action_t ua; + /* + * Update the rcode from any child calls that + * may have been performed. The module still + * has a chance to override this rcode if it + * wants, but process modules in particular + * expect to see the result of child + * evaluations available to them in p_result. + */ + state->rcode = *p_result < RLM_MODULE_NUMCODES ? *p_result : RLM_MODULE_NOOP; + fr_assert(state->resume != NULL); /* @@ -626,15 +717,23 @@ static unlang_action_t unlang_module_resume(rlm_rcode_t *p_result, request_t *re caller = request->module; request->module = mc->instance->name; + resume = state->resume; + /* + * The module *MUST* explicitly set the resume + * function when yielding or pushing children + * if it wants to be called again later. + */ + state->resume = NULL; + safe_lock(mc->instance); - ua = state->resume(&rcode, - &(module_ctx_t){ - .instance = mc->instance->dl_inst->data, - .thread = state->thread->data - }, request, state->rctx); + ua = resume(&state->rcode, + &(module_ctx_t){ + .instance = mc->instance->dl_inst->data, + .thread = state->thread->data + }, request, state->rctx); safe_unlock(mc->instance); - request->rcode = rcode; + request->rcode = state->rcode; request->module = caller; if (request->master_state == REQUEST_STOP_PROCESSING) ua = UNLANG_ACTION_STOP_PROCESSING; @@ -642,22 +741,45 @@ static unlang_action_t unlang_module_resume(rlm_rcode_t *p_result, request_t *re switch (ua) { case UNLANG_ACTION_STOP_PROCESSING: RWARN("Module %s or worker signalled to stop processing request", mc->instance->module->name); - if (state->p_result) *state->p_result = rcode; - if (state->signal) state->thread->active_callers--; - - *p_result = rcode; + if (state->p_result) *state->p_result = state->rcode; + state->thread->active_callers--; + *p_result = state->rcode; return UNLANG_ACTION_STOP_PROCESSING; case UNLANG_ACTION_YIELD: + /* + * The module yielded but didn't set a + * resume function, this means it's done + * and when the I/O operation completes + * it shouldn't be called again. + */ + if (!state->resume) { + frame->process = unlang_module_resume_done; + } else { + frame->process = unlang_module_resume; + } + repeatable_set(frame); return UNLANG_ACTION_YIELD; /* - * The module is done (for now). But, running it pushed one or - * more asynchronous calls onto the stack. These need to - * be run before the next module runs. + * The module is done (for now). + * But, running it pushed one or more asynchronous + * calls onto the stack for evaluation. + * These need to be run before the module resumes + * or the next unlang instruction is processed. */ case UNLANG_ACTION_PUSHED_CHILD: - state->rcode = rcode; + /* + * The module pushed a child and didn't + * set a resume function, this means + * it's done, and we won't call it again + * but we still need to do some cleanup + * after the child returns. + */ + if (!state->resume) { + frame->process = unlang_module_resume_done; + state->set_rcode = false; /* Preserve the child rcode */ + } repeatable_set(frame); return UNLANG_ACTION_PUSHED_CHILD; @@ -665,82 +787,30 @@ static unlang_action_t unlang_module_resume(rlm_rcode_t *p_result, request_t *re case UNLANG_ACTION_UNWIND: break; - case UNLANG_ACTION_EXECUTE_NEXT: + case UNLANG_ACTION_EXECUTE_NEXT: /* Not valid */ fr_assert(0); *p_result = RLM_MODULE_FAIL; break; } - RDEBUG2("%s (%s)", frame->instruction->name ? frame->instruction->name : "", - fr_table_str_by_value(mod_rcode_table, rcode, "")); - - state->thread->active_callers--; - - request->rcode = rcode; - if (state->p_result) *state->p_result = rcode; - - *p_result = rcode; + unlang_module_resume_done(p_result, request, frame); return ua; } -/** Yield a request back to the interpreter from within a module - * - * This passes control of the request back to the unlang interpreter, setting - * callbacks to execute when the request is 'signalled' asynchronously, or whatever - * timer or I/O event the module was waiting for occurs. - * - * @note The module function which calls #unlang_module_yield should return control - * of the C stack to the unlang interpreter immediately after calling #unlang_module_yield. - * A common pattern is to use ``return unlang_module_yield(...)``. - * - * @param[in] request The current request. - * @param[in] resume Called on unlang_interpret_mark_runnable(). - * @param[in] signal Called on unlang_action(). - * @param[in] rctx to pass to the callbacks. - * @return - * - UNLANG_ACTION_YIELD. - */ -unlang_action_t unlang_module_yield(request_t *request, - unlang_module_resume_t resume, unlang_module_signal_t signal, void *rctx) -{ - unlang_stack_t *stack = request->stack; - 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); - - REQUEST_VERIFY(request); /* Check the yielded request is sane */ - - state->rctx = rctx; - state->resume = resume; - state->signal = signal; - - /* - * We set the repeatable flag here, so that the resume - * function is always called going back up the stack. - * This setting is normally done in the intepreter. - * However, the caller of this function may call us, and - * then push *other* things onto the stack. Which means - * that the interpreter never gets a chance to set this - * flag. - */ - frame->process = unlang_module_resume; - repeatable_set(frame); - - return UNLANG_ACTION_YIELD; -} - static unlang_action_t unlang_module(rlm_rcode_t *p_result, request_t *request, unlang_stack_frame_t *frame) { unlang_module_t *mc; unlang_frame_state_module_t *state = talloc_get_type_abort(frame->state, unlang_frame_state_module_t); char const *caller; - rlm_rcode_t rcode = RLM_MODULE_NOOP; unlang_action_t ua; + state->rcode = RLM_MODULE_NOOP; + state->set_rcode = true; + #ifndef NDEBUG - int unlang_indent = request->log.unlang_indent; + state->unlang_indent = request->log.unlang_indent; #endif - /* * Process a stand-alone child, and fall through * to dealing with it's parent. @@ -762,7 +832,7 @@ static unlang_action_t unlang_module(rlm_rcode_t *p_result, request_t *request, * Return administratively configured return code */ if (mc->instance->force) { - rcode = mc->instance->code; + state->rcode = mc->instance->code; ua = UNLANG_ACTION_CALCULATE_RESULT; goto done; } @@ -780,7 +850,7 @@ static unlang_action_t unlang_module(rlm_rcode_t *p_result, request_t *request, caller = request->module; request->module = mc->instance->name; safe_lock(mc->instance); /* Noop unless instance->mutex set */ - ua = mc->method(&rcode, + ua = mc->method(&state->rcode, &(module_ctx_t){ .instance = mc->instance->dl_inst->data, .thread = state->thread->data @@ -798,15 +868,25 @@ static unlang_action_t unlang_module(rlm_rcode_t *p_result, request_t *request, */ case UNLANG_ACTION_STOP_PROCESSING: RWARN("Module %s became unblocked", mc->instance->module->name); - if (state->p_result) *state->p_result = rcode; - - *p_result = rcode; + if (state->p_result) *state->p_result = state->rcode; + *p_result = state->rcode; return UNLANG_ACTION_STOP_PROCESSING; case UNLANG_ACTION_YIELD: state->thread->active_callers++; + + /* + * The module yielded but didn't set a + * resume function, this means it's done + * and when the I/O operation completes + * it shouldn't be called again. + */ + if (!state->resume) { + frame->process = unlang_module_resume_done; + } else { + frame->process = unlang_module_resume; + } repeatable_set(frame); - frame->process = unlang_module_resume; return UNLANG_ACTION_YIELD; /* @@ -815,7 +895,17 @@ static unlang_action_t unlang_module(rlm_rcode_t *p_result, request_t *request, * be run before the next module runs. */ case UNLANG_ACTION_PUSHED_CHILD: - state->rcode = rcode; + /* + * The module pushed a child and didn't + * set a resume function, this means + * it's done, and we won't call it again + * but we still need to do some cleanup + * after the child returns. + */ + if (!state->resume) { + frame->process = unlang_module_done; + state->set_rcode = false; /* Preserve the child rcode */ + } repeatable_set(frame); return UNLANG_ACTION_PUSHED_CHILD; @@ -829,21 +919,8 @@ static unlang_action_t unlang_module(rlm_rcode_t *p_result, request_t *request, break; } - /* - * Must be left at RDEBUG() level otherwise RDEBUG becomes pointless - */ - RDEBUG("%s (%s)", frame->instruction->name ? frame->instruction->name : "", - fr_table_str_by_value(mod_rcode_table, rcode, "")); - done: - fr_assert(unlang_indent == request->log.unlang_indent); - fr_assert(rcode >= RLM_MODULE_REJECT); - fr_assert(rcode < RLM_MODULE_NUMCODES); - - request->rcode = rcode; - if (state->p_result) *state->p_result = rcode; - - *p_result = rcode; + unlang_module_done(p_result, request, frame); return ua; } diff --git a/src/lib/unlang/module_priv.h b/src/lib/unlang/module_priv.h index 262bac1bdd3..af1aa3a5d8a 100644 --- a/src/lib/unlang/module_priv.h +++ b/src/lib/unlang/module_priv.h @@ -49,11 +49,17 @@ typedef struct { ///< shared between all threads, so we can't ///< cache thread-specific data in the #unlang_t. +#ifndef NDEBUG + int unlang_indent; //!< Record what this was when we entered the module. +#endif + /** @name rcode output * @{ */ rlm_rcode_t *p_result; //!< Where to store the result. rlm_rcode_t rcode; //!< the result, only for unlang_module_resume_final. + bool set_rcode; //!< Overwrite the current rcode for the section with + ///< the module rcode. /** @} */ /** @name Resumption and signalling diff --git a/src/lib/unlang/parallel.c b/src/lib/unlang/parallel.c index 4d3a4796f85..958988ca33d 100644 --- a/src/lib/unlang/parallel.c +++ b/src/lib/unlang/parallel.c @@ -405,6 +405,7 @@ static unlang_action_t unlang_parallel_process(rlm_rcode_t *p_result, request_t * of the children. */ frame->process = unlang_parallel_resume; + repeatable_set(frame); /* * Yield to the children diff --git a/src/lib/unlang/subrequest_child.c b/src/lib/unlang/subrequest_child.c index 1a6c5d2639a..e4f4273e84d 100644 --- a/src/lib/unlang/subrequest_child.c +++ b/src/lib/unlang/subrequest_child.c @@ -264,6 +264,7 @@ unlang_action_t unlang_subrequest_child_run(UNUSED rlm_rcode_t *p_result, UNUSED * Don't run this function again on resumption */ if (frame->process == unlang_subrequest_child_run) frame->process = unlang_subrequest_calculate_result; + repeatable_set(frame); return UNLANG_ACTION_YIELD; } diff --git a/src/lib/unlang/tmpl.c b/src/lib/unlang/tmpl.c index cd2d6bdb6c9..be36d124358 100644 --- a/src/lib/unlang/tmpl.c +++ b/src/lib/unlang/tmpl.c @@ -520,6 +520,7 @@ static unlang_action_t unlang_tmpl_exec_wait_resume(rlm_rcode_t *p_result, reque } frame->process = unlang_tmpl_exec_wait_final; + repeatable_set(frame); return UNLANG_ACTION_YIELD; } diff --git a/src/lib/unlang/xlat.c b/src/lib/unlang/xlat.c index 55e86e89ba2..12cb83798f8 100644 --- a/src/lib/unlang/xlat.c +++ b/src/lib/unlang/xlat.c @@ -253,6 +253,7 @@ static unlang_action_t unlang_xlat_repeat(rlm_rcode_t *p_result, request_t *requ RWDEBUG("Missing call to unlang_xlat_yield()"); goto fail; } + repeatable_set(frame); return UNLANG_ACTION_YIELD; case XLAT_ACTION_DONE: @@ -306,6 +307,7 @@ static unlang_action_t unlang_xlat(rlm_rcode_t *p_result, request_t *request, un RWDEBUG("Missing call to unlang_xlat_yield()"); goto fail; } + repeatable_set(frame); return UNLANG_ACTION_YIELD; case XLAT_ACTION_DONE: @@ -379,6 +381,7 @@ static unlang_action_t unlang_xlat_resume(rlm_rcode_t *p_result, request_t *requ request, &state->rhead, state->rctx); switch (xa) { case XLAT_ACTION_YIELD: + repeatable_set(frame); return UNLANG_ACTION_YIELD; case XLAT_ACTION_DONE: