From: Arran Cudbard-Bell Date: Mon, 5 Apr 2021 20:49:52 +0000 (+0100) Subject: Add explicit request types (external, internal, detached) X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=70e837fba271125c353cb98f01da5d176ce1fd96;p=thirdparty%2Ffreeradius-server.git Add explicit request types (external, internal, detached) Add stop and detach interpreter callbacks to fix state issues --- diff --git a/src/bin/unit_test_module.c b/src/bin/unit_test_module.c index 44ea4f2e66..726264cdd3 100644 --- a/src/bin/unit_test_module.c +++ b/src/bin/unit_test_module.c @@ -136,7 +136,7 @@ static request_t *request_from_file(TALLOC_CTX *ctx, FILE *fp, RADCLIENT *client /* * Create and initialize the new request. */ - request = request_local_alloc(ctx, NULL); + request = request_local_alloc_external(ctx, NULL); /* * FIXME - Should be less RADIUS centric, but everything * else assumes RADIUS at the moment so we can fix this later. @@ -339,12 +339,12 @@ static bool do_xlats(char const *filename, FILE *fp) char *p; char input[8192]; char output[8192]; - request_t *request; + request_t *request; /* * Create and initialize the new request. */ - request = request_alloc(NULL, NULL); + request = request_alloc_external(NULL, NULL); request->log.dst = talloc_zero(request, log_dst_t); request->log.dst->func = vlog_request; @@ -464,7 +464,7 @@ static request_t *request_clone(request_t *old) { request_t *request; - request = request_alloc(NULL, NULL); + request = request_alloc_external(NULL, NULL); if (!request) return NULL; if (!request->packet) request->packet = fr_radius_packet_alloc(request, false); diff --git a/src/lib/eap/chbind.c b/src/lib/eap/chbind.c index 9e09488b61..cb25e0acdd 100644 --- a/src/lib/eap/chbind.c +++ b/src/lib/eap/chbind.c @@ -185,7 +185,7 @@ fr_radius_packet_code_t chbind_process(request_t *request, CHBIND_REQ *chbind) (chbind->response == NULL)); /* Set-up the fake request */ - fake = request_alloc(request, &(request_init_args_t){ .parent = request }); + fake = request_alloc_internal(request, &(request_init_args_t){ .parent = request }); MEM(fr_pair_prepend_by_da(fake->request_ctx, &vp, &fake->request_pairs, attr_freeradius_proxied_to) >= 0); fr_pair_value_from_str(vp, "127.0.0.1", sizeof("127.0.0.1"), '\0', false); diff --git a/src/lib/io/listen.h b/src/lib/io/listen.h index 4c184d0941..ed98640957 100644 --- a/src/lib/io/listen.h +++ b/src/lib/io/listen.h @@ -64,7 +64,4 @@ struct fr_async_s { uint32_t priority; //!< higher == higher priority }; -#define request_is_external(_x) (_x->async && (_x->async->listen != NULL)) -#define request_is_internal(_x) (!request_is_external(_x)) - int fr_io_listen_free(fr_listen_t *li); diff --git a/src/lib/io/worker.c b/src/lib/io/worker.c index 97210b03d7..cabb4db90a 100644 --- a/src/lib/io/worker.c +++ b/src/lib/io/worker.c @@ -372,14 +372,9 @@ static void worker_nak(fr_worker_t *worker, fr_channel_data_t *cd, fr_time_t now * * @param[in] request_p Pointer to the request to cancel. * Will be set to NULL. - * @param[in] now The current time. */ -static void worker_stop_request(request_t **request_p, fr_time_t now) +static void worker_stop_request(request_t **request_p) { - if ((*request_p)->async->tracking.state == FR_TIME_TRACKING_YIELDED) { - fr_time_tracking_resume(&(*request_p)->async->tracking, now); - } - /* * Also marks the request as done and runs * the internal/external callbacs. @@ -421,7 +416,7 @@ static void worker_max_request_time(UNUSED fr_event_list_t *el, UNUSED fr_time_t * Waiting too long, delete it. */ REDEBUG("Request has reached max_request_time - signalling it to stop"); - worker_stop_request(&request, now); + worker_stop_request(&request); } /* @@ -682,7 +677,7 @@ static void worker_request_bootstrap(fr_worker_t *worker, fr_channel_data_t *cd, if (fr_heap_num_elements(worker->time_order) >= (uint32_t) worker->config.max_requests) goto nak; - ctx = request = request_alloc(NULL, NULL); + ctx = request = request_alloc_external(NULL, NULL); if (!request) goto nak; worker_request_init(worker, request, now); @@ -815,7 +810,7 @@ nak: */ RWARN("Got conflicting packet for request (%" PRIu64 "), telling old request to stop", old->number); - worker_stop_request(&old, now); + worker_stop_request(&old); worker->stats.dropped++; insert_new: @@ -879,7 +874,6 @@ void fr_worker_destroy(fr_worker_t *worker) { int i, count; request_t *request; - fr_time_t now = fr_time(); // WORKER_VERIFY; @@ -899,7 +893,7 @@ void fr_worker_destroy(fr_worker_t *worker) DEBUG("Worker is exiting - telling request %s to stop", request->name); count++; } - worker_stop_request(&request, now); + worker_stop_request(&request); } fr_assert(fr_heap_num_elements(worker->runnable) == 0); @@ -926,8 +920,8 @@ void fr_worker_destroy(fr_worker_t *worker) */ static void _worker_request_internal_init(request_t *request, void *uctx) { + fr_worker_t *worker = talloc_get_type_abort(uctx, fr_worker_t); fr_time_t now = fr_time(); - fr_worker_t *worker = uctx; worker_request_init(worker, request, now); @@ -939,40 +933,18 @@ static void _worker_request_internal_init(request_t *request, void *uctx) worker_request_time_tracking_start(worker, request, now); } -/** Internal request (i.e. one generated by the interpreter) is now complete - * - * Whatever generated the request is now responsible for freeing it. - */ -static void _worker_request_internal_done(request_t *request, UNUSED rlm_rcode_t rcode, void *uctx) -{ - fr_time_t now = fr_time(); - fr_worker_t *worker = uctx; - - worker_request_time_tracking_end(worker, request, now); - - fr_assert(!fr_heap_entry_inserted(request->runnable_id)); - fr_assert(!fr_heap_entry_inserted(request->time_order_id)); - - /* - * Detached requests have to be freed by us - * as nothing else can free them. - * - * All other requests must be freed by the - * code which allocated them. - */ - if (!request->parent) talloc_free(request); -} /** External request is now complete * */ -static void _worker_request_external_done(request_t *request, UNUSED rlm_rcode_t rcode, void *uctx) +static void _worker_request_done_external(request_t *request, UNUSED rlm_rcode_t rcode, void *uctx) { - fr_worker_t *worker = uctx; + fr_worker_t *worker = talloc_get_type_abort(uctx, fr_worker_t); /* * All external requests MUST have a listener. */ + fr_assert(request_is_external(request)); fr_assert(request->async->listen != NULL); /* @@ -1005,6 +977,94 @@ static void _worker_request_external_done(request_t *request, UNUSED rlm_rcode_t talloc_free(request); } +/** Internal request (i.e. one generated by the interpreter) is now complete + * + * Whatever generated the request is now responsible for freeing it. + */ +static void _worker_request_done_internal(request_t *request, UNUSED rlm_rcode_t rcode, void *uctx) +{ + fr_worker_t *worker = talloc_get_type_abort(uctx, fr_worker_t); + + worker_request_time_tracking_end(worker, request, fr_time()); + + fr_assert(!fr_heap_entry_inserted(request->runnable_id)); + fr_assert(!fr_heap_entry_inserted(request->time_order_id)); +} + +/** Detached request (i.e. one generated by the interpreter with no parent) is now complete + * + * As the request has no parent, then there's nothing to free it + * so we have to. + */ +static void _worker_request_done_detached(request_t *request, UNUSED rlm_rcode_t rcode, UNUSED void *uctx) +{ + /* + * No time tracking for detached requests + * so we don't need to call + * worker_request_time_tracking_end. + */ + fr_assert(!fr_heap_entry_inserted(request->runnable_id)); + fr_assert(!fr_heap_entry_inserted(request->time_order_id)); + + /* + * Detached requests have to be freed by us + * as nothing else can free them. + * + * All other requests must be freed by the + * code which allocated them. + */ + talloc_free(request); +} + + +/** Make us responsible for running the request + * + */ +static void _worker_request_detach(request_t *request, void *uctx) +{ + fr_worker_t *worker = talloc_get_type_abort(uctx, fr_worker_t); + + RDEBUG3("Request is detached"); + fr_assert(request_is_detached(request)); + + /* + * End the time tracking... We don't track detached requests, + * because they don't contribute for the time consumed by an + * external request. + */ + worker_request_time_tracking_end(worker, request, fr_time()); + + return; +} + +/** This is called by the interpreter when it wants to stop a request + * + * The idea is to get the request into the same state it would be in + * if the interpreter had just finished with it. + */ +static void _worker_request_stop(request_t *request, void *uctx) +{ + fr_worker_t *worker = talloc_get_type_abort(uctx, fr_worker_t); + + RDEBUG3("Cleaning up request execution state"); + + /* + * Make sure time tracking is always in a + * consistent state when we mark the request + * as done. + */ + if (request->async->tracking.state == FR_TIME_TRACKING_YIELDED) { + fr_time_tracking_resume(&request->async->tracking, fr_time()); + } + + /* + * If the request is in the runnable queue + * yank it back out, so it's not "runnable" + * when we call request done. + */ + if (fr_heap_entry_inserted(worker->runnable)) fr_heap_extract(worker->runnable, request); +} + /** Request is now runnable * */ @@ -1080,7 +1140,7 @@ static inline CC_HINT(always_inline) void worker_run_request(fr_worker_t *worker * just stop the request and free it. */ if (request->async->channel && !fr_channel_active(request->async->channel)) { - worker_stop_request(&request, now); + worker_stop_request(&request); return; } @@ -1195,11 +1255,17 @@ nomem: worker->intp = unlang_interpret_init(worker, el, &(unlang_request_func_t){ .init_internal = _worker_request_internal_init, - .done_internal = _worker_request_internal_done, - .done_external = _worker_request_external_done, - .mark_runnable = _worker_request_runnable, + + .done_external = _worker_request_done_external, + .done_internal = _worker_request_done_internal, + .done_detached = _worker_request_done_detached, + + .detach = _worker_request_detach, + .stop = _worker_request_stop, .yield = _worker_request_yield, .resume = _worker_request_resume, + .mark_runnable = _worker_request_runnable, + .scheduled = _worker_request_scheduled }, worker); diff --git a/src/lib/server/pair_server_tests.c b/src/lib/server/pair_server_tests.c index d11ccbbb2e..776b58ef03 100644 --- a/src/lib/server/pair_server_tests.c +++ b/src/lib/server/pair_server_tests.c @@ -198,7 +198,7 @@ static request_t *request_fake_alloc(void) /* * Create and initialize the new request. */ - request = request_local_alloc(autofree, NULL); + request = request_local_alloc_external(autofree, NULL); request->packet = fr_radius_packet_alloc(request, false); TEST_CHECK(request->packet != NULL); diff --git a/src/lib/server/request.c b/src/lib/server/request.c index 08111dd189..e76d862bd0 100644 --- a/src/lib/server/request.c +++ b/src/lib/server/request.c @@ -123,6 +123,7 @@ static inline CC_HINT(always_inline) int request_detachable_init(request_t *chil static inline CC_HINT(always_inline) int request_child_init(request_t *child, request_t *parent) { child->number = parent->child_number++; + if (!child->dict) child->dict = parent->dict; if ((parent->seq_start == 0) || (parent->number == parent->seq_start)) { child->name = talloc_typed_asprintf(child, "%s.%" PRIu64, parent->name, child->number); @@ -132,8 +133,6 @@ static inline CC_HINT(always_inline) int request_child_init(request_t *child, re } child->seq_start = 0; /* children always start with their own sequence */ child->parent = parent; - child->dict = parent->dict; - child->client = parent->client; /* * For new server support. @@ -154,30 +153,6 @@ static inline CC_HINT(always_inline) int request_child_init(request_t *child, re return -1; } - /* - * Fill in the child request. - */ - child->packet->socket = parent->packet->socket; - child->packet->socket.inet.dst_port = 0; - child->packet->socket.fd = -1; - - /* - * This isn't STRICTLY required, as the child request MUST NEVER - * be put into the request list. However, it's still reasonable - * practice. - */ - child->packet->id = child->number & 0xff; - child->packet->code = parent->packet->code; - child->packet->timestamp = parent->packet->timestamp; - - /* - * Fill in the child reply, based on the child request. - */ - fr_socket_addr_swap(&child->reply->socket, &child->packet->socket); - child->reply->id = child->packet->id; - child->reply->code = 0; /* UNKNOWN code */ - child->reply->socket.fd = -1; - return 0; } @@ -186,24 +161,52 @@ static inline CC_HINT(always_inline) int request_child_init(request_t *child, re * @param[in] file the request was allocated in. * @param[in] line the request was allocated on. * @param[in] request to (re)-initialise. + * @param[in] type of request to initialise. + * @param[in] args Other optional arguments. */ static inline CC_HINT(always_inline) int request_init(char const *file, int line, - request_t *request, request_init_args_t const *args) + request_t *request, request_type_t type, + request_init_args_t const *args) { + /* + * Sanity checks for different requests types + */ + switch (type) { + case REQUEST_TYPE_EXTERNAL: + if (!fr_cond_assert_msg(!args->parent, "External requests must NOT have a parent")) return -1; + break; + + case REQUEST_TYPE_INTERNAL: + if (!fr_cond_assert_msg(args->parent, + "Internal requests must have a parent (args->parent == NULL)")) return -1; + break; + + case REQUEST_TYPE_DETACHED: + fr_assert_fail("Detached requests should start as type == REQUEST_TYPE_INTERNAL, " + "args->detachable and be detached later"); + return -1; + } + *request = (request_t){ #ifndef NDEBUG .magic = REQUEST_MAGIC, #endif + .type = type, .master_state = REQUEST_ACTIVE, .dict = args->namespace, .component = "", + .flags = { + .detachable = args->detachable + }, .runnable_id = -1, .time_order_id = -1, + .alloc_file = file, .alloc_line = line }; + /* * Initialise the stack */ @@ -418,12 +421,14 @@ static inline CC_HINT(always_inline) request_t *request_alloc_pool(TALLOC_CTX *c * @param[in] file where the request was allocated. * @param[in] line where the request was allocated. * @param[in] ctx to bind the request to. + * @param[in] type what type of request to alloc. * @param[in] args Optional arguments. * @return * - A request on success. * - NULL on error. */ -request_t *_request_alloc(char const *file, int line, TALLOC_CTX *ctx, request_init_args_t const *args) +request_t *_request_alloc(char const *file, int line, TALLOC_CTX *ctx, + request_type_t type, request_init_args_t const *args) { request_t *request; fr_dlist_head_t *free_list; @@ -458,7 +463,7 @@ request_t *_request_alloc(char const *file, int line, TALLOC_CTX *ctx, request_i fr_dlist_remove(free_list, request); } - if (request_init(file, line, request, args) < 0) { + if (request_init(file, line, request, type, args) < 0) { talloc_free(request); return NULL; } @@ -524,14 +529,15 @@ static int _request_local_free(request_t *request) * which needs to be outside of the normal free list, so that it can be freed * when the module requires, not when the thread destructor runs. */ -request_t *_request_local_alloc(char const *file, int line, TALLOC_CTX *ctx, request_init_args_t const *args) +request_t *_request_local_alloc(char const *file, int line, TALLOC_CTX *ctx, + request_type_t type, request_init_args_t const *args) { request_t *request; if (!args) args = &default_args; request = request_alloc_pool(ctx); - if (request_init(file, line, request, args) < 0) return NULL; + if (request_init(file, line, request, type, args) < 0) return NULL; talloc_set_destructor(request, _request_local_free); diff --git a/src/lib/server/request.h b/src/lib/server/request.h index 16c0154fd6..c963248164 100644 --- a/src/lib/server/request.h +++ b/src/lib/server/request.h @@ -140,6 +140,18 @@ typedef struct { fr_pair_t *state; //!< Pair containing the state list. } request_pair_lists_t; +typedef enum { + REQUEST_TYPE_EXTERNAL = 0, //!< A request received on the wire. + REQUEST_TYPE_INTERNAL, //!< A request generated internally. + REQUEST_TYPE_DETACHED //!< A request that was generated internally, but is now detached + ///< (not associated with a parent request.) +} request_type_t; + +#define request_is_external(_x) ((_x)->type == REQUEST_TYPE_EXTERNAL) +#define request_is_internal(_x) ((_x)->type == REQUEST_TYPE_INTERNAL) +#define request_is_detached(_x) ((_x)->type == REQUEST_TYPE_DETACHED) +#define request_is_detachable(_x) ((_x)->flags.detachable == 1) + struct request_s { #ifndef NDEBUG uint32_t magic; //!< Magic number used to detect memory corruption, @@ -147,6 +159,8 @@ struct request_s { #endif void *stack; //!< unlang interpreter stack. + request_type_t type; //!< What type of request this is. + request_t *parent; //!< Request that generated this request. uint64_t number; //!< Monotonically increasing request number. Reset on server restart. @@ -169,7 +183,14 @@ struct request_s { */ request_pair_lists_t pair_list; //!< Structure containing all pair lists. - fr_dlist_head_t data; //!< Request metadata. + fr_dlist_head_t data; //!< Request data. + + /** Capabilities flags for this request + * + */ + struct { + uint8_t detachable : 1; //!< This request may be detached from its parent.. + } flags; /** Logging information * @@ -251,21 +272,47 @@ typedef struct { #define RAD_REQUEST_OPTION_CTX (1 << 1) #define RAD_REQUEST_OPTION_DETAIL (1 << 2) -/** Allocate a new request +/** Allocate a new external request + * + * Use for requests produced by listeners + * + * @param[in] _ctx Talloc ctx to bind the request to. + * @param[in] _args Optional arguments that control how the request is initialised. + */ +#define request_alloc_external(_ctx, _args) \ + _request_alloc( __FILE__, __LINE__, (_ctx), REQUEST_TYPE_EXTERNAL, (_args)) + +/** Allocate a new internal request + * + * Use for requests produced by modules and unlang * * @param[in] _ctx Talloc ctx to bind the request to. * @param[in] _args Optional arguments that control how the request is initialised. */ -#define request_alloc(_ctx, _args) _request_alloc( __FILE__, __LINE__, (_ctx), (_args)) -request_t *_request_alloc(char const *file, int line, TALLOC_CTX *ctx, request_init_args_t const *args); +#define request_alloc_internal(_ctx, _args) \ + _request_alloc( __FILE__, __LINE__, (_ctx), REQUEST_TYPE_INTERNAL, (_args)) + +request_t *_request_alloc(char const *file, int line, TALLOC_CTX *ctx, + request_type_t type, request_init_args_t const *args); -/** Allocate a new request outside of the request pool +/** Allocate a new external request outside of the request pool * * @param[in] _ctx Talloc ctx to allocate the request in. * @param[in] _args Optional arguments that control how the request is initialised. */ -#define request_local_alloc(_ctx, _args) _request_local_alloc(__FILE__, __LINE__, (_ctx), (_args)) -request_t *_request_local_alloc(char const *file, int line, TALLOC_CTX *ctx, request_init_args_t const *args); +#define request_local_alloc_external(_ctx, _args) \ + _request_local_alloc(__FILE__, __LINE__, (_ctx), REQUEST_TYPE_EXTERNAL, (_args)) + +/** Allocate a new internal request outside of the request pool + * + * @param[in] _ctx Talloc ctx to allocate the request in. + * @param[in] _args Optional arguments that control how the request is initialised. + */ +#define request_local_alloc_internal(_ctx, _args) \ + _request_local_alloc(__FILE__, __LINE__, (_ctx), REQUEST_TYPE_INTERNAL, (_args)) + +request_t *_request_local_alloc(char const *file, int line, TALLOC_CTX *ctx, + request_type_t type, request_init_args_t const *args); int request_detach(request_t *child); diff --git a/src/lib/server/trigger.c b/src/lib/server/trigger.c index 480ae58f2d..ea0897a66c 100644 --- a/src/lib/server/trigger.c +++ b/src/lib/server/trigger.c @@ -370,7 +370,7 @@ int trigger_exec(unlang_interpret_t *intp, request_t *request, /* * radius_exec_program always needs a request. */ - child = request_alloc(NULL, (&(request_init_args_t){ .parent = request, .detachable = true })); + child = request_alloc_internal(NULL, (&(request_init_args_t){ .parent = request, .detachable = true })); /* * Add the args to the request data, so they can be picked up by the diff --git a/src/lib/tls/session.c b/src/lib/tls/session.c index 62f8d02603..b98a3d2f04 100644 --- a/src/lib/tls/session.c +++ b/src/lib/tls/session.c @@ -1613,7 +1613,7 @@ fr_tls_session_t *fr_tls_session_init_client(TALLOC_CTX *ctx, fr_tls_conf_t *con return NULL; } - request = request_alloc(session, NULL); + request = request_alloc_internal(session, NULL); fr_tls_session_request_bind(request, session->ssl); diff --git a/src/lib/unlang/interpret.c b/src/lib/unlang/interpret.c index 38b6fefad4..afc1e07e93 100644 --- a/src/lib/unlang/interpret.c +++ b/src/lib/unlang/interpret.c @@ -843,6 +843,57 @@ static inline CC_HINT(always_inline) void frame_signal(request_t *request, fr_st stack->depth = depth; /* Reset */ } +/** Indicate to the caller of the interpreter that this request is complete + * + */ +void unlang_interpret_request_done(request_t *request) +{ + unlang_stack_t *stack = request->stack; + unlang_interpret_t *intp; + + if (!fr_cond_assert(stack != NULL)) return; + + intp = stack->intp; + + switch (request->type) { + case REQUEST_TYPE_EXTERNAL: + intp->funcs.done_external(request, stack->result, intp->uctx); + break; + + case REQUEST_TYPE_INTERNAL: + intp->funcs.done_internal(request, stack->result, intp->uctx); + break; + + case REQUEST_TYPE_DETACHED: + intp->funcs.done_detached(request, stack->result, intp->uctx); + break; + } +} + +static inline CC_HINT(always_inline) +void unlang_interpret_request_stop(request_t *request) +{ + unlang_stack_t *stack = request->stack; + unlang_interpret_t *intp; + + if (!fr_cond_assert(stack != NULL)) return; + + intp = stack->intp; + intp->funcs.stop(request, intp->uctx); +} + +static inline CC_HINT(always_inline) +void unlang_interpret_request_detach(request_t *request) +{ + unlang_stack_t *stack = request->stack; + unlang_interpret_t *intp; + + if (!fr_cond_assert(stack != NULL)) return; + + intp = stack->intp; + intp->funcs.detach(request, intp->uctx); +} + /** Send a signal (usually stop) to a request * * This is typically called via an "async" action, i.e. an action @@ -857,11 +908,26 @@ void unlang_interpret_signal(request_t *request, fr_state_signal_t action) { unlang_stack_t *stack = request->stack; + switch (action) { /* * If we're stopping, then mark the request as stopped. * Then, call the frame signal handler. */ - if (action == FR_SIGNAL_CANCEL) request->master_state = REQUEST_STOP_PROCESSING; + case FR_SIGNAL_CANCEL: + request->master_state = REQUEST_STOP_PROCESSING; + break; + + case FR_SIGNAL_DETACH: + /* + * Ensure the request is able to be detached + * else don't signal. + */ + if (!fr_cond_assert(request_is_detachable(request))) return; + break; + + default: + break; + } /* * Requests that haven't been run through the interpret @@ -870,7 +936,19 @@ void unlang_interpret_signal(request_t *request, fr_state_signal_t action) */ if (stack && (stack->depth > 0)) frame_signal(request, action, 0); - if (action == FR_SIGNAL_CANCEL) unlang_interpret_request_done(request); + switch (action) { + case FR_SIGNAL_CANCEL: + unlang_interpret_request_stop(request); /* Stop gets the request in a consistent state */ + unlang_interpret_request_done(request); /* Done signals the request is complete */ + break; + + case FR_SIGNAL_DETACH: + unlang_interpret_request_detach(request); /* Tell our caller that the request is being detached */ + break; + + default: + break; + } } /** Return the depth of the request's stack @@ -924,25 +1002,6 @@ bool unlang_interpret_is_resumable(request_t *request) return is_yielded(frame); } -/** Indicate to the caller of the interpreter that this request is complete - * - */ -void unlang_interpret_request_done(request_t *request) -{ - unlang_stack_t *stack = request->stack; - unlang_interpret_t *intp; - - if (!fr_cond_assert(stack != NULL)) return; - - intp = stack->intp; - - if (request_is_internal(request)) { - intp->funcs.done_internal(request, stack->result, intp->uctx); - } else { - intp->funcs.done_external(request, stack->result, intp->uctx); - } -} - /** Mark a request as resumable. * * It's not called "unlang_interpret", because it doesn't actually @@ -1155,6 +1214,19 @@ unlang_interpret_t *unlang_interpret_init(TALLOC_CTX *ctx, { unlang_interpret_t *intp; + fr_assert(funcs->init_internal); + + fr_assert(funcs->done_internal); + fr_assert(funcs->done_detached); + fr_assert(funcs->done_external); + + fr_assert(funcs->detach); + fr_assert(funcs->stop); + fr_assert(funcs->yield); + fr_assert(funcs->resume); + fr_assert(funcs->mark_runnable); + fr_assert(funcs->scheduled); + MEM(intp = talloc(ctx, unlang_interpret_t)); *intp = (unlang_interpret_t){ .el = el, diff --git a/src/lib/unlang/interpret.h b/src/lib/unlang/interpret.h index 9082c40e66..518a5e8d7c 100644 --- a/src/lib/unlang/interpret.h +++ b/src/lib/unlang/interpret.h @@ -54,6 +54,12 @@ typedef void (*unlang_request_init_t)(request_t *request, void *uctx); */ typedef void (*unlang_request_done_t)(request_t *request, rlm_rcode_t rcode, void *uctx); +/** Stop a request from running + * + * This is called whenever a request has been signalled to stop + */ +typedef void (*unlang_request_stop_t)(request_t *request, void *uctx); + /** Signal the owner of the interpreter that a request has yielded * * This is called whenever a request has given control back to the interpeter. @@ -91,9 +97,19 @@ typedef bool (*unlang_request_scheduled_t)(request_t const *request, void *uctx) * request management than FeeRADIUS worker threads. */ typedef struct { - unlang_request_init_t init_internal; //!< Function called to initialise a request. + /* + * There's no init_external as this is done + * before the external request is handed off + * to the interpreter. + */ + unlang_request_init_t init_internal; //!< Function called to initialise an internal request. + + unlang_request_done_t done_external; //!< Function called when a external request completes. unlang_request_done_t done_internal; //!< Function called when an internal request completes. - unlang_request_done_t done_external; //!< Function called when an internal request completes. + unlang_request_done_t done_detached; //!< Function called when a detached request completes. + + unlang_request_init_t detach; //!< Function called when a request is detached. + unlang_request_stop_t stop; //!< function called when a request is signalled to stop. unlang_request_yield_t yield; //!< Function called when a request yields. unlang_request_resume_t resume; //!< Function called when a request is resumed. unlang_request_runnable_t mark_runnable; //!< Function called when a request needs to be diff --git a/src/lib/unlang/interpret_synchronous.c b/src/lib/unlang/interpret_synchronous.c index 85527f3074..043ef4bba4 100644 --- a/src/lib/unlang/interpret_synchronous.c +++ b/src/lib/unlang/interpret_synchronous.c @@ -25,6 +25,7 @@ #include "interpret_priv.h" #include +#include typedef struct { fr_heap_t *runnable; @@ -33,7 +34,6 @@ typedef struct { unlang_interpret_t *intp; } unlang_interpret_synchronous_t; - /** Internal request (i.e. one generated by the interpreter) is now complete * */ @@ -41,19 +41,11 @@ static void _request_init_internal(request_t *request, void *uctx) { unlang_interpret_synchronous_t *intps = uctx; - RDEBUG3("Initialising synchronous request"); + RDEBUG3("Initialising internal synchronous request"); unlang_interpret_set(request, intps->intp); fr_heap_insert(intps->runnable, request); } -/** Internal request (i.e. one generated by the interpreter) is now complete - * - */ -static void _request_done_internal(request_t *request, UNUSED rlm_rcode_t rcode, UNUSED void *uctx) -{ - RDEBUG3("Done synchronous request"); -} - /** External request is now complete * */ @@ -78,6 +70,48 @@ static void _request_done_external(request_t *request, UNUSED rlm_rcode_t rcode, RDEBUG3("Synchronous done external request"); } +/** Internal request (i.e. one generated by the interpreter) is now complete + * + */ +static void _request_done_internal(request_t *request, UNUSED rlm_rcode_t rcode, UNUSED void *uctx) +{ + RDEBUG3("Done synchronous internal request"); + + /* Request will be cleaned up by whatever created it */ +} + +static void _request_done_detached(request_t *request, UNUSED rlm_rcode_t rcode, UNUSED void *uctx) +{ + RDEBUG3("Done synchronous detached request"); + + /* + * If we're the top level interpreter + * then free the detached request. + */ + if (unlang_interpret_stack_depth(request) == 0) talloc_free(request); +} + +/** We don't need to do anything for internal -> detached + * + */ +static void _request_detach(request_t *request, UNUSED void *uctx) +{ + RDEBUG3("Synchronous request detached"); +} + +/** Request has been stopped + * + * Clean up execution state + */ +static void _request_stop(request_t *request, void *uctx) +{ + unlang_interpret_synchronous_t *intps = uctx; + + RDEBUG3("Stopped detached request"); + + fr_heap_extract(intps->runnable, request); +} + /** Request is now runnable * */ @@ -126,11 +160,16 @@ static unlang_interpret_synchronous_t *unlang_interpret_synchronous_alloc(TALLOC MEM(intps->intp = unlang_interpret_init(intps, intps->el, &(unlang_request_func_t){ .init_internal = _request_init_internal, - .done_internal = _request_done_internal, + .done_external = _request_done_external, - .mark_runnable = _request_runnable, + .done_internal = _request_done_internal, + .done_detached = _request_done_detached, + + .detach = _request_detach, + .stop = _request_stop, .yield = _request_yield, .resume = _request_resume, + .mark_runnable = _request_runnable, .scheduled = _request_scheduled }, intps)); diff --git a/src/lib/unlang/io.c b/src/lib/unlang/io.c index 38f2473b4a..cd8bdb6a17 100644 --- a/src/lib/unlang/io.c +++ b/src/lib/unlang/io.c @@ -40,12 +40,12 @@ request_t *unlang_io_subrequest_alloc(request_t *parent, fr_dict_t const *namesp { request_t *child; - child = request_alloc(detachable ? NULL : parent, - (&(request_init_args_t){ - .parent = parent, - .namespace = namespace, - .detachable = detachable - })); + child = request_alloc_internal(detachable ? NULL : parent, + (&(request_init_args_t){ + .parent = parent, + .namespace = namespace, + .detachable = detachable + })); if (!child) return NULL; /* diff --git a/src/lib/unlang/subrequest_child.c b/src/lib/unlang/subrequest_child.c index cdcd312c7b..60b795936d 100644 --- a/src/lib/unlang/subrequest_child.c +++ b/src/lib/unlang/subrequest_child.c @@ -142,12 +142,6 @@ static void unlang_subrequest_child_signal(request_t *request, fr_state_signal_t state = talloc_get_type_abort(frame_current(request->parent), unlang_frame_state_subrequest_t); - /* - * Ignore signals if the parent didn't want the child - * to be detached. - */ - if (!state->detachable) return; - /* * Place child's state back inside the parent */ diff --git a/src/modules/proto_bfd/proto_bfd.c b/src/modules/proto_bfd/proto_bfd.c index 1a274cf11c..2cdc164574 100644 --- a/src/modules/proto_bfd/proto_bfd.c +++ b/src/modules/proto_bfd/proto_bfd.c @@ -399,7 +399,7 @@ static void bfd_request(bfd_state_t *session, request_t *request, fr_radius_pack static void bfd_trigger(bfd_state_t *session) { fr_radius_packet_t packet; - request_t *request = request_local_alloc(session, NULL); + request_t *request = request_local_alloc_external(session, NULL); char buffer[256]; snprintf(buffer, sizeof(buffer), "server.bfd.%s", @@ -1343,7 +1343,7 @@ static int bfd_process(bfd_state_t *session, bfd_packet_t *bfd) request_t *request; fr_radius_packet_t *packet, *reply; - request = request_alloc(session, NULL); + request = request_alloc_external(session, NULL); packet = fr_radius_packet_alloc(request, 0); reply = fr_radius_packet_alloc(request, 0); diff --git a/src/modules/rlm_eap/types/rlm_eap_fast/eap_fast.c b/src/modules/rlm_eap/types/rlm_eap_fast/eap_fast.c index 38a87a8fb3..c2ed3e3428 100644 --- a/src/modules/rlm_eap/types/rlm_eap_fast/eap_fast.c +++ b/src/modules/rlm_eap/types/rlm_eap_fast/eap_fast.c @@ -579,7 +579,7 @@ static fr_radius_packet_code_t eap_fast_eap_payload(request_t *request, eap_sess /* * Allocate a fake request_t structure. */ - fake = request_alloc(request, &(request_init_args_t){ .parent = request }); + fake = request_alloc_internal(request, &(request_init_args_t){ .parent = request }); fr_assert(fr_pair_list_empty(&fake->request_pairs)); t = talloc_get_type_abort(tls_session->opaque, eap_fast_tunnel_t); @@ -691,7 +691,7 @@ static fr_radius_packet_code_t eap_fast_eap_payload(request_t *request, eap_sess /* * FIXME: Actually proxy stuff */ - request->proxy = request_alloc(request, &(request_init_args_t){ .parent = request }); + request->proxy = request_alloc_internal(request, &(request_init_args_t){ .parent = request }); request->proxy->packet = talloc_steal(request->proxy, fake->packet); memset(&request->proxy->packet->src_ipaddr, 0, diff --git a/src/modules/rlm_eap/types/rlm_eap_peap/peap.c b/src/modules/rlm_eap/types/rlm_eap_peap/peap.c index 8b17a447b9..35d5b0dab9 100644 --- a/src/modules/rlm_eap/types/rlm_eap_peap/peap.c +++ b/src/modules/rlm_eap/types/rlm_eap_peap/peap.c @@ -548,7 +548,7 @@ unlang_action_t eap_peap_process(rlm_rcode_t *p_result, request_t *request, break; case PEAP_STATUS_WAIT_FOR_SOH_RESPONSE: - fake = request_alloc(request, &(request_init_args_t){ .parent = request }); + fake = request_alloc_internal(request, &(request_init_args_t){ .parent = request }); fr_assert(fr_pair_list_empty(&fake->request_pairs)); eap_peap_soh_verify(fake, data, data_len); setup_fake_request(request, fake, t); @@ -647,7 +647,7 @@ unlang_action_t eap_peap_process(rlm_rcode_t *p_result, request_t *request, goto finish; } - fake = request_alloc(request, &(request_init_args_t){ .parent = request }); + fake = request_alloc_internal(request, &(request_init_args_t){ .parent = request }); fr_assert(fr_pair_list_empty(&fake->request_pairs)); switch (t->status) { diff --git a/src/modules/rlm_radius/rlm_radius_udp.c b/src/modules/rlm_radius/rlm_radius_udp.c index fcb675ab26..e5594b8cdd 100644 --- a/src/modules/rlm_radius/rlm_radius_udp.c +++ b/src/modules/rlm_radius/rlm_radius_udp.c @@ -359,7 +359,7 @@ static void CC_HINT(nonnull) status_check_alloc(fr_event_list_t *el, udp_handle_ * head before the module destructor * runs. */ - request = request_local_alloc(u, NULL); + request = request_local_alloc_external(u, NULL); request->async = talloc_zero(request, fr_async_t); talloc_const_free(request->name); request->name = talloc_strdup(request, h->module_name);