From: Alan T. DeKok <aland@freeradius.org>
Date: Fri, 27 Sep 2019 18:56:46 +0000 (-0400)
Subject: make UNLANG_TYPE_MODULE skip using resumable frames
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=6d07606bea1db146bea239372e11a2a663a86602;p=thirdparty%2Ffreeradius-server.git

make UNLANG_TYPE_MODULE skip using resumable frames

The basic tests seem to work.  So let's hope that the rest works.
---

diff --git a/src/lib/unlang/interpret.c b/src/lib/unlang/interpret.c
index e09806d1007..8389298f29f 100644
--- a/src/lib/unlang/interpret.c
+++ b/src/lib/unlang/interpret.c
@@ -77,6 +77,7 @@ static void frame_dump(REQUEST *request, unlang_stack_frame_t *frame)
 	RDEBUG2("repeat         %s", is_repeatable(frame) ? "yes" : "no");
 	RDEBUG2("break_point    %s", is_break_point(frame) ? "yes" : "no");
 	RDEBUG2("return_point   %s", is_return_point(frame) ? "yes" : "no");
+	RDEBUG2("resumable      %s", is_resumable(frame) ? "yes" : "no");
 	REXDENT();
 }
 
@@ -438,6 +439,7 @@ static inline void frame_cleanup(unlang_stack_frame_t *frame)
 	repeatable_clear(frame);
 	break_point_clear(frame);
 	return_point_clear(frame);
+	resumable_clear(frame);
 	if (frame->state) TALLOC_FREE(frame->state);
 }
 
@@ -610,8 +612,10 @@ static inline unlang_frame_action_t frame_eval(REQUEST *request, unlang_stack_fr
 
 				return UNLANG_FRAME_ACTION_YIELD;
 
+			case UNLANG_TYPE_MODULE: /* until we get rid of UNLANG_TYPE_RESUME */
 			case UNLANG_TYPE_RESUME:
 				repeatable_set(frame);
+				resumable_set(frame);
 				RDEBUG4("** [%i] %s - yielding with current (%s %d)", stack->depth, __FUNCTION__,
 					fr_table_str_by_value(mod_rcode_table, frame->result, "<invalid>"),
 					frame->priority);
@@ -1128,16 +1132,30 @@ static void frame_signal(REQUEST *request, fr_state_signal_t action, int limit)
 		/*
 		 *	Be gracious in errors.
 		 */
-		if (frame->instruction->type != UNLANG_TYPE_RESUME) continue;
+		if (!is_resumable(frame)) continue;
 
-		mr = unlang_generic_to_resume(frame->instruction);
+		switch (frame->instruction->type) {
+		case UNLANG_TYPE_MODULE: /* until we get rid of UNLANG_TYPE_RESUME */
+			if (!frame->signal) continue;
 
-		/*
-		 *	No signal handler for this frame type
-		 */
-		if (!unlang_ops[mr->parent->type].signal) continue;
+			frame->signal(request, NULL, action);
+			break;
+
+		case UNLANG_TYPE_RESUME:
+			mr = unlang_generic_to_resume(frame->instruction);
+
+			/*
+			 *	No signal handler for this frame type
+			 */
+			if (!unlang_ops[mr->parent->type].signal) continue;
+
+			unlang_ops[mr->parent->type].signal(request, mr->rctx, action);
+			break;
 
-		unlang_ops[mr->parent->type].signal(request, mr->rctx, action);
+		default:
+			rad_assert(0);
+			break;
+		}
 	}
 	stack->depth = depth;				/* Reset */
 }
@@ -1199,26 +1217,39 @@ void unlang_interpret_resumable(REQUEST *request)
 	unlang_stack_frame_t		*frame = &stack->frame[stack->depth];
 
 	/*
+	 *	The request hasn't yield, OR it's already been marked
+	 *	as runnable.  Don't do anything.
+	 *
 	 *	The IO code, or children have no idea where they're
-	 *	being called from.  They just mark the parent
+	 *	being called from.  They just ask to mark the parent
 	 *	resumable when they're done.  So we have to check here
-	 *	for a RESUME frame.  If the parent called the child
-	 *	directly, then there's no RESUME frame.  When the
-	 *	child is done, the parent will automatically continue
-	 *	running.  We threfore don't need to insert the parent
-	 *	into the backlog.
+	 *	if this request is resumable.
+	 *
+	 *	If the parent called the child directly, then the
+	 *	parent hasn't yielded, so it isn't resumable.  When
+	 *	the child is done, the parent will automatically
+	 *	continue running.  We therefore don't need to insert
+	 *	the parent into the backlog.
+	 *
+	 *	Multiple child request may also mark a parent request
+	 *	runnable, before the parent request starts running.
+	 */
+	if (!is_resumable(frame) || (request->runnable_id >= 0)) return;
+
+	/*
 	 */
-	if (frame->instruction->type != UNLANG_TYPE_RESUME) {
+	switch (frame->instruction->type) {
+	case UNLANG_TYPE_MODULE:
+	case UNLANG_TYPE_RESUME:
+		break;
+
+	default:
 		return;
 	}
 
 	rad_assert(request->backlog != NULL);
 
-	/*
-	 *	Multiple child request may mark a request runnable,
-	 *	before it is enabled for running.
-	 */
-	if (request->runnable_id < 0) fr_heap_insert(request->backlog, request);
+	fr_heap_insert(request->backlog, request);
 }
 
 /** Callback for handling resumption frames
diff --git a/src/lib/unlang/module.c b/src/lib/unlang/module.c
index bd359276968..d4d3bda3e95 100644
--- a/src/lib/unlang/module.c
+++ b/src/lib/unlang/module.c
@@ -336,7 +336,7 @@ void unlang_module_push(rlm_rcode_t *out, REQUEST *request,
 	unlang_frame_state_module_t	*ms;
 	unlang_stack_t			*stack = request->stack;
 	unlang_stack_frame_t		*frame;
-	unlang_module_t			*mi;
+	unlang_module_t			*sp;
 
 	static unlang_t module_instruction = {
 		.type = UNLANG_TYPE_MODULE,
@@ -363,17 +363,17 @@ void unlang_module_push(rlm_rcode_t *out, REQUEST *request,
 	ms->thread = module_thread(module_instance);
 	rad_assert(ms->thread != NULL);
 
-	MEM(mi = talloc_zero(ms, unlang_module_t));	/* Free at the same time as the state */
-	mi->self = module_instruction;
-	mi->self.name = module_instance->name;
-	mi->self.debug_name = mi->self.name;
-	mi->module_instance = module_instance;
-	mi->method = method;
+	MEM(sp = talloc_zero(ms, unlang_module_t));	/* Free at the same time as the state */
+	sp->self = module_instruction;
+	sp->self.name = module_instance->name;
+	sp->self.debug_name = sp->self.name;
+	sp->module_instance = module_instance;
+	sp->method = method;
 
 	/*
 	 *	Push a new module frame onto the stack
 	 */
-	unlang_interpret_push(request, unlang_module_to_generic(mi), RLM_MODULE_UNKNOWN, UNLANG_NEXT_STOP, top_frame);
+	unlang_interpret_push(request, unlang_module_to_generic(sp), RLM_MODULE_UNKNOWN, UNLANG_NEXT_STOP, top_frame);
 	frame = &stack->frame[stack->depth];
 	frame->state = ms;
 }
@@ -507,6 +507,17 @@ rlm_rcode_t unlang_module_yield_to_section(REQUEST *request, CONF_SECTION *subcs
 	return RLM_MODULE_YIELD;
 }
 
+/*
+ *	@todo - hacks until we get rid of UNLANG_TYPE_RESUME
+ */
+static unlang_action_t unlang_module_resume(REQUEST *request, rlm_rcode_t *presult, UNUSED void *rctx);
+
+static unlang_action_t unlang_module_process_resume(REQUEST *request, rlm_rcode_t *presult)
+{
+	return unlang_module_resume(request, presult, NULL);
+}
+
+
 /** Yield a request back to the interpreter from within a module
  *
  * This passes control of the request back to the unlang interpreter, setting
@@ -531,38 +542,18 @@ rlm_rcode_t unlang_module_yield(REQUEST *request,
 {
 	unlang_stack_t			*stack = request->stack;
 	unlang_stack_frame_t		*frame = &stack->frame[stack->depth];
-	unlang_resume_t			*mr;
-
-	rad_assert(stack->depth > 0);
+	unlang_frame_state_module_t	*ms = talloc_get_type_abort(frame->state,
+								    unlang_frame_state_module_t);
 
 	REQUEST_VERIFY(request);	/* Check the yielded request is sane */
 
-	switch (frame->instruction->type) {
-	case UNLANG_TYPE_MODULE:
-		mr = unlang_interpret_resume_alloc(request, (void *)resume, (void *)signal, rctx);
-		if (!fr_cond_assert(mr)) {
-			return RLM_MODULE_FAIL;
-		}
-		return RLM_MODULE_YIELD;
+	ms->rctx = rctx;
+	ms->resume = resume;
+	ms->signal = signal;
 
-	case UNLANG_TYPE_RESUME:
-		mr = talloc_get_type_abort(frame->instruction, unlang_resume_t);
-		rad_assert(mr->parent->type == UNLANG_TYPE_MODULE);
+	frame->process = unlang_module_process_resume;
 
-		/*
-		 *	Re-use the current RESUME frame, but over-ride
-		 *	the callbacks and context.
-		 */
-		mr->resume = (void *)resume;
-		mr->signal = (void *)signal;
-		mr->rctx = rctx;
-
-		return RLM_MODULE_YIELD;
-
-	default:
-		rad_assert(0);
-		return RLM_MODULE_FAIL;
-	}
+	return RLM_MODULE_YIELD;
 }
 
 /*
@@ -699,30 +690,20 @@ done:
  * @param[in] rctx		createed by #unlang_module.
  * @param[in] action		to signal.
  */
-static void unlang_module_signal(REQUEST *request, void *rctx, fr_state_signal_t action)
+static void unlang_module_signal(REQUEST *request, UNUSED void *rctx, fr_state_signal_t action)
 {
-	unlang_stack_frame_t		*frame;
 	unlang_stack_t			*stack = request->stack;
-	unlang_resume_t			*mr;
-	unlang_module_t		*mc;
+	unlang_stack_frame_t		*frame = &stack->frame[stack->depth];
+	unlang_frame_state_module_t	*ms = talloc_get_type_abort(frame->state, unlang_frame_state_module_t);
+	unlang_module_t			*sp = unlang_generic_to_module(frame->instruction);
 	char const 			*caller;
 
-	unlang_frame_state_module_t	*ms = NULL;
-
-	rad_assert(stack->depth > 0);
-
-	frame = &stack->frame[stack->depth];
-
-	mr = unlang_generic_to_resume(frame->instruction);
-	if (!mr->signal) return;
-
-	mc = unlang_generic_to_module(mr->parent);
-	ms = talloc_get_type_abort(frame->state, unlang_frame_state_module_t);
+	if (!ms->signal) return;
 
 	caller = request->module;
-	request->module = mc->module_instance->name;
-	((fr_unlang_module_signal_t)mr->signal)(mc->module_instance->dl_inst->data, ms->thread->data, request,
-						rctx, action);
+	request->module = sp->module_instance->name;
+	ms->signal(sp->module_instance->dl_inst->data, ms->thread->data, request,
+		   ms->rctx, action);
 	request->module = caller;
 }
 
@@ -730,33 +711,28 @@ static unlang_action_t unlang_module_resume(REQUEST *request, rlm_rcode_t *presu
 {
 	unlang_stack_t			*stack = request->stack;
 	unlang_stack_frame_t		*frame = &stack->frame[stack->depth];
-	unlang_t			*instruction = frame->instruction;
-	unlang_resume_t			*mr = unlang_generic_to_resume(instruction);
-	unlang_module_t			*mc = unlang_generic_to_module(mr->parent);
-	int				stack_depth = stack->depth;
-	char const			*caller;
+	unlang_frame_state_module_t	*ms = talloc_get_type_abort(frame->state, unlang_frame_state_module_t);
+	unlang_module_t			*sp = unlang_generic_to_module(frame->instruction);
+	char const 			*caller;
 	rlm_rcode_t			rcode;
+	int				stack_depth = stack->depth;
 
-	unlang_frame_state_module_t	*ms = NULL;
-
-	rad_assert(mr->parent->type == UNLANG_TYPE_MODULE);
-
-	ms = talloc_get_type_abort(frame->state, unlang_frame_state_module_t);
+	rad_assert(ms->resume != NULL);
 
 	/*
 	 *	Lock is noop unless instance->mutex is set.
 	 */
 	caller = request->module;
-	request->module = mc->module_instance->name;
-	safe_lock(mc->module_instance);
-	rcode = request->rcode = ((fr_unlang_module_resume_t)mr->resume)(mc->module_instance->dl_inst->data,
-									 ms->thread->data, request, mr->rctx);
-	safe_unlock(mc->module_instance);
+	request->module = sp->module_instance->name;
+	safe_lock(sp->module_instance);
+	rcode = request->rcode = ms->resume(sp->module_instance->dl_inst->data,
+					    ms->thread->data, request, ms->rctx);
+	safe_unlock(sp->module_instance);
 	request->module = caller;
 
 	if (rcode != RLM_MODULE_YIELD) ms->thread->active_callers--;
 
-	RDEBUG2("%s (%s)", instruction->name ? instruction->name : "",
+	RDEBUG2("%s (%s)", frame->instruction->name ? frame->instruction->name : "",
 		fr_table_str_by_value(mod_rcode_table, rcode, "<invalid>"));
 
 	switch (rcode) {
diff --git a/src/lib/unlang/module_priv.h b/src/lib/unlang/module_priv.h
index 3c026ecf3ea..47c3c414866 100644
--- a/src/lib/unlang/module_priv.h
+++ b/src/lib/unlang/module_priv.h
@@ -45,6 +45,9 @@ typedef struct {
 typedef struct {
 	rlm_rcode_t			*presult;		//!< Where to store the result.
 	module_thread_instance_t 	*thread;		//!< thread-local data for this module
+	void				*rctx;			//!< for resume / signal
+	fr_unlang_module_resume_t	resume;			//!< resumption handler
+	fr_unlang_module_signal_t	signal;			//!< for signal handlers
 } unlang_frame_state_module_t;
 
 static inline unlang_module_t *unlang_generic_to_module(unlang_t *p)
diff --git a/src/lib/unlang/unlang_priv.h b/src/lib/unlang/unlang_priv.h
index 83b58f87995..e7e28f66d4b 100644
--- a/src/lib/unlang/unlang_priv.h
+++ b/src/lib/unlang/unlang_priv.h
@@ -293,21 +293,25 @@ typedef struct {
 #define UNWIND_FLAG_BREAK_POINT		0x04			//!< 'break' stops here.
 #define UNWIND_FLAG_RETURN_POINT	0x08      		//!< 'return' stops here.
 #define UNWIND_FLAG_NO_CLEAR		0x10			//!< Keep unwinding, don't clear the unwind flag.
+#define UNWIND_FLAG_RESUMABLE		0x20			//!< Resume point, temporary until we get rid of UNLANG_TYPE_RESUME
 
 static inline void repeatable_set(unlang_stack_frame_t *frame)		{ frame->uflags |= UNWIND_FLAG_REPEAT; }
 static inline void top_frame_set(unlang_stack_frame_t *frame) 		{ frame->uflags |= UNWIND_FLAG_TOP_FRAME; }
 static inline void break_point_set(unlang_stack_frame_t *frame)		{ frame->uflags |= UNWIND_FLAG_BREAK_POINT; }
 static inline void return_point_set(unlang_stack_frame_t *frame)	{ frame->uflags |= UNWIND_FLAG_RETURN_POINT; }
+static inline void resumable_set(unlang_stack_frame_t *frame)		{ frame->uflags |= UNWIND_FLAG_RESUMABLE; }
 
 static inline void repeatable_clear(unlang_stack_frame_t *frame)	{ frame->uflags &= ~UNWIND_FLAG_REPEAT; }
 static inline void top_frame_clear(unlang_stack_frame_t *frame)		{ frame->uflags &= ~UNWIND_FLAG_TOP_FRAME; }
 static inline void break_point_clear(unlang_stack_frame_t *frame)	{ frame->uflags &= ~UNWIND_FLAG_BREAK_POINT; }
 static inline void return_point_clear(unlang_stack_frame_t *frame) 	{ frame->uflags &= ~UNWIND_FLAG_RETURN_POINT; }
+static inline void resumable_clear(unlang_stack_frame_t *frame) 	{ frame->uflags &= ~UNWIND_FLAG_RESUMABLE; }
 
 static inline bool is_repeatable(unlang_stack_frame_t *frame)		{ return frame->uflags & UNWIND_FLAG_REPEAT; }
 static inline bool is_top_frame(unlang_stack_frame_t *frame)		{ return frame->uflags & UNWIND_FLAG_TOP_FRAME; }
 static inline bool is_break_point(unlang_stack_frame_t *frame)		{ return frame->uflags & UNWIND_FLAG_BREAK_POINT; }
 static inline bool is_return_point(unlang_stack_frame_t *frame) 	{ return frame->uflags & UNWIND_FLAG_RETURN_POINT; }
+static inline bool is_resumable(unlang_stack_frame_t *frame) 		{ return frame->uflags & UNWIND_FLAG_RESUMABLE; }
 
 static inline unlang_action_t unwind_to_break(unlang_stack_t *stack)
 {