From: Alan T. DeKok Date: Fri, 11 Aug 2017 13:15:37 +0000 (+0200) Subject: allocate status_check rr when we open the socket X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=b1cc8af5fd29242560f368db873cffa9f9768a9a;p=thirdparty%2Ffreeradius-server.git allocate status_check rr when we open the socket not when we initialize the module --- diff --git a/src/modules/rlm_radius/TODO.md b/src/modules/rlm_radius/TODO.md index 55809a7af18..2ba82a3a027 100644 --- a/src/modules/rlm_radius/TODO.md +++ b/src/modules/rlm_radius/TODO.md @@ -105,6 +105,10 @@ signaling if necessary. We probaby want the network + worker to be able to send IDs of 0/0, which means "no tracking", as that will likely be the common case. +We also need the same thing for conflicting packets... we need a way +to tell the end modules to stop retransmitting the packet, as no one +cares about it any more. + ## miscellaneous * Check on packet lifetime timers in network side? @@ -116,3 +120,31 @@ We should move to a "must_signal" approach, as with the network side The worker should suppress signals if it sees that the ACKs from the other end haven't caught up to it's sent packets. Otherwise, it must signal. + +this whole thing is wrong... we end up signaling on every damned packet in real life... + +we need worker-side de-dup for dup / conflicting packets. Which lets +us stop old packets while processing new ones. + +OK... fix the damned channel to use queue depth instead of ACKs +which makes them less general, but better. The worker can NAK a packet, send a reply, or mark it ask discarded + +DATA N -> W: (packet + queue 1, active) + +DATA N <- W (packet + queue is now 0, inactive) + +DISCARD N <- W (no packet, queue is now 0, inactive) + +SLEEPING N <- W (no packet, queue is 1, inactive) + +We also need an "must_signal" flag, for if the other end is +sleeping... the network always sets it, I guess.. + +What else... + +* the debug output is still a bit too complex to understand fully +* Status-Server packets (etc.) need to have priorities associated with them + * maybe the proto_radius stuff needs to take Status-Server and just respond itself?? + * for now, that's probably the best idea... +* need to move to queue depth / active flag in channels / network / worker + * the current ACK, and "my_view_of_their_shit" is just too complex diff --git a/src/modules/rlm_radius/rlm_radius_udp.c b/src/modules/rlm_radius/rlm_radius_udp.c index f1021f6f232..b063d62ab21 100644 --- a/src/modules/rlm_radius/rlm_radius_udp.c +++ b/src/modules/rlm_radius/rlm_radius_udp.c @@ -1203,6 +1203,24 @@ static void conn_close(int fd, void *uctx) c->fd = -1; } +/** Free an rlm_radius_udp_request_t + * + * Unlink the packet from the connection, and remove any tracking + * entries. + */ +static int udp_request_free(rlm_radius_udp_request_t *u) +{ + fr_dlist_remove(&u->entry); + + if (u->rr) { + (void) rr_track_delete(u->c->id, u->rr); + u->rr = NULL; + } + + return 0; +} + + /** Process notification that fd is open * */ @@ -1241,6 +1259,70 @@ static fr_connection_state_t conn_open(UNUSED fr_event_list_t *el, UNUSED int fd fr_heap_insert(t->active, c); c->state = CONN_ACTIVE; + /* + * Status-Server checks. Manually build the packet, and + * all of it's associated glue. + */ + if (c->inst->parent->status_check) { + rlm_radius_link_t *link; + rlm_radius_udp_request_t *u; + REQUEST *request; + + link = talloc_zero(c, rlm_radius_link_t); + u = talloc_zero(c, rlm_radius_udp_request_t); + request = request_alloc(link); + + // @todo - if we call unlang, we need to set a whole lot more... see worker.c + request->el = c->thread->el; + request->packet = fr_radius_alloc(request, false); + request->reply = fr_radius_alloc(request, false); + + /* + * Create the packet contents. + * + * @todo - different packet contents for + * Access-Request, Accounting-Request, etc. + */ + pair_make_request("NAS-Identifier", "status check - are you alive?", T_OP_EQ); + pair_make_request("Event-Timestamp", "0", T_OP_EQ); + + /* + * Initialize the link. Note that we don't set + * destructors. + */ + FR_DLIST_INIT(link->entry); + link->request = request; + link->request_io_ctx = u; + + /* + * Unitialize the UDP link. + */ + FR_DLIST_INIT(u->entry); + u->code = c->inst->parent->status_check; + request->packet->code = u->code; + u->c = c; + u->link = link; + + /* + * Reserve a permanent ID for the packet. This + * is because we need to be able to send an ID on + * demand. If the proxied packets use all of the + * IDs, then we can't send a Status-Server check. + */ + u->rr = rr_track_alloc(c->id, request, u->code, link); + if (!u->rr) { + ERROR("%s failed allocating status_check ID for new connection %s", + c->inst->parent->name, c->name); + talloc_free(u); + talloc_free(link); + } else { + DEBUG3("%s allocated %s ID %u for status checks on connection %s", + c->inst->parent->name, fr_packet_codes[u->code], u->rr->id, c->name); + talloc_set_destructor(u, udp_request_free); + c->status_u = u; + } + } + /* * Now that we're open, also push pending requests from * the main thread queue onto the queue for this @@ -1408,24 +1490,6 @@ static int conn_free(rlm_radius_udp_connection_t *c) } -/** Free an rlm_radius_udp_request_t - * - * Unlink the packet from the connection, and remove any tracking - * entries. - */ -static int udp_request_free(rlm_radius_udp_request_t *u) -{ - fr_dlist_remove(&u->entry); - - if (u->rr) { - (void) rr_track_delete(u->c->id, u->rr); - u->rr = NULL; - } - - return 0; -} - - /** Allocate a new connection and set it up. * */ @@ -1475,68 +1539,6 @@ static void mod_connection_alloc(rlm_radius_udp_t *inst, rlm_radius_udp_thread_t FR_DLIST_INIT(c->queued); FR_DLIST_INIT(c->sent); - /* - * Status-Server checks. Manually build the packet, and - * all of it's associated glue. - */ - if (inst->parent->status_check) { - rlm_radius_link_t *link; - rlm_radius_udp_request_t *u; - REQUEST *request; - - link = talloc_zero(c, rlm_radius_link_t); - u = talloc_zero(c, rlm_radius_udp_request_t); - request = request_alloc(link); - - // @todo - if we call unlang, we need to set a whole lot more... see worker.c - request->el = c->thread->el; - request->packet = fr_radius_alloc(request, false); - request->reply = fr_radius_alloc(request, false); - - /* - * Create the packet contents. - * - * @todo - different packet contents for - * Access-Request, Accounting-Request, etc. - */ - pair_make_request("NAS-Identifier", "status check - are you alive?", T_OP_EQ); - pair_make_request("Event-Timestamp", "0", T_OP_EQ); - - /* - * Initialize the link. Note that we don't set - * destructors. - */ - FR_DLIST_INIT(link->entry); - link->request = request; - link->request_io_ctx = u; - - /* - * Unitialize the UDP link. - */ - FR_DLIST_INIT(u->entry); - u->code = inst->parent->status_check; - request->packet->code = u->code; - u->c = c; - u->link = link; - - /* - * Reserve a permanent ID for the packet. This - * is because we need to be able to send an ID on - * demand. If the proxied packets use all of the - * IDs, then we can't send a Status-Server check. - */ - u->rr = rr_track_alloc(c->id, request, inst->parent->status_check, link); - if (!u->rr) { - cf_log_err(inst->config, "%s failed allocating status_check ID for new connection", - inst->parent->name); - talloc_free(c); - return; - } - - talloc_set_destructor(u, udp_request_free); - c->status_u = u; - } - c->conn = fr_connection_alloc(c, t->el, &inst->parent->connection_timeout, &inst->parent->reconnection_delay, conn_init, conn_open, conn_close, inst->parent->name, c); if (!c->conn) {