From: James Jones Date: Thu, 25 Feb 2021 22:05:27 +0000 (-0600) Subject: Make rbtree_walk() users use iterators instead. (#3959) X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=9c005cc13f65664cde1f707e0726a9484d6ac167;p=thirdparty%2Ffreeradius-server.git Make rbtree_walk() users use iterators instead. (#3959) That makes rbtree_walk() unused, so it goes away, too. In turn, since rbtree_walk() is the only caller of the walk_*_order() functions, they go away as well, and it means the various "compare" functions (that do what's usually called "visiting" nodes in the context of tree traversal) will typically be inlined into the one place they're used, in the iterator loop, and control flow that's implied by what the "compare" functions return can just be explicitly done in said loop. --- diff --git a/src/bin/radclient.c b/src/bin/radclient.c index 21f37e108ec..e6ea3dd0ab7 100644 --- a/src/bin/radclient.c +++ b/src/bin/radclient.c @@ -735,16 +735,6 @@ static int filename_cmp(void const *one, void const *two) return strcmp(a->filters, b->filters); } -static int filename_walk(void *data, UNUSED void *uctx) -{ - rc_file_pair_t *files = data; - - /* - * Read request(s) from the file. - */ - return radclient_init(files, files); -} - /* * Deallocate packet ID, etc. */ @@ -1424,9 +1414,18 @@ int main(int argc, char **argv) /* * Walk over the list of filenames, creating the requests. */ - if (rbtree_walk(filename_tree, RBTREE_IN_ORDER, filename_walk, NULL) != 0) { - ERROR("Failed parsing input files"); - fr_exit_now(1); + { + fr_rb_tree_iter_inorder_t iter; + rc_file_pair_t *files; + + for (files = rbtree_iter_init_inorder(&iter, filename_tree); + files; + files = rbtree_iter_next_inorder(&iter)) { + if (radclient_init(files, files)) { + ERROR("Failed parsing input files"); + fr_exit_now(1); + } + } } /* diff --git a/src/lib/io/network.c b/src/lib/io/network.c index 3dc6cb81af4..2d2e8647f1d 100644 --- a/src/lib/io/network.c +++ b/src/lib/io/network.c @@ -387,27 +387,22 @@ int fr_network_listen_inject(fr_network_t *nr, fr_listen_t *li, uint8_t const *p return fr_control_message_send(nr->control, rb, FR_CONTROL_ID_INJECT, &my_inject, sizeof(my_inject)); } -static int apply(void *data, void *uctx) -{ - fr_network_socket_t *socket = data; - - fr_event_update_t *update = uctx; - - fr_event_filter_update(socket->nr->el, socket->listen->fd, FR_EVENT_FILTER_IO, update); - - return 0; -} - static void fr_network_suspend(fr_network_t *nr) { static fr_event_update_t pause_read[] = { FR_EVENT_SUSPEND(fr_event_io_func_t, read), { 0 } }; + fr_rb_tree_iter_inorder_t iter; + fr_network_socket_t *socket; if (nr->suspended) return; - (void) rbtree_walk(nr->sockets, RBTREE_IN_ORDER, apply, pause_read); + for (socket = rbtree_iter_init_inorder(&iter, nr->sockets); + socket; + socket = rbtree_iter_next_inorder(&iter)) { + fr_event_filter_update(socket->nr->el, socket->listen->fd, FR_EVENT_FILTER_IO, pause_read); + } nr->suspended = true; } @@ -417,10 +412,16 @@ static void fr_network_unsuspend(fr_network_t *nr) FR_EVENT_RESUME(fr_event_io_func_t, read), { 0 } }; + fr_rb_tree_iter_inorder_t iter; + fr_network_socket_t *socket; if (!nr->suspended) return; - (void) rbtree_walk(nr->sockets, RBTREE_IN_ORDER, apply, resume_read); + for (socket = rbtree_iter_init_inorder(&iter, nr->sockets); + socket; + socket = rbtree_iter_next_inorder(&iter)) { + fr_event_filter_update(socket->nr->el, socket->listen->fd, FR_EVENT_FILTER_IO, resume_read); + } nr->suspended = false; } @@ -1742,27 +1743,23 @@ static int cmd_stats_self(FILE *fp, UNUSED FILE *fp_err, void *ctx, UNUSED fr_cm return 0; } -static int socket_list(void *data, void *uctx) -{ - FILE *fp = uctx; - fr_network_socket_t *s = data; - - if (!s->listen->app_io->get_name) { - fprintf(fp, "%s\n", s->listen->app_io->name); - return 0; - } - - fprintf(fp, "%d\t%s\n", s->number, s->listen->app_io->get_name(s->listen)); - return 0; -} - static int cmd_socket_list(FILE *fp, UNUSED FILE *fp_err, void *ctx, UNUSED fr_cmd_info_t const *info) { - fr_network_t const *nr = ctx; + fr_network_t const *nr = ctx; + fr_rb_tree_iter_inorder_t iter; + fr_network_socket_t *socket; // @todo - note that this isn't thread-safe! - (void) rbtree_walk(nr->sockets, RBTREE_IN_ORDER, socket_list, fp); + for (socket = rbtree_iter_init_inorder(&iter, nr->sockets); + socket; + socket = rbtree_iter_next_inorder(&iter)) { + if (!socket->listen->app_io->get_name) { + fprintf(fp, "%s\n", socket->listen->app_io->name); + } else { + fprintf(fp, "%d\t%s\n", socket->number, socket->listen->app_io->get_name(socket->listen)); + } + } return 0; } diff --git a/src/lib/redis/cluster.c b/src/lib/redis/cluster.c index 8260fd16654..885e923fd4a 100644 --- a/src/lib/redis/cluster.c +++ b/src/lib/redis/cluster.c @@ -1175,28 +1175,6 @@ static fr_redis_cluster_rcode_t cluster_redirect(fr_redis_cluster_node_t **out, return FR_REDIS_CLUSTER_RCODE_SUCCESS; } -/** Walk all used pools adding them to the live node list - * - * @param[in] uctx Where to write the node we found. - * @param[in] data node to check. - * @return - * - 0 continue walking. - * - -1 found suitable node. - */ -static int _cluster_pool_walk(void *data, void *uctx) -{ - cluster_nodes_live_t *live = uctx; - fr_redis_cluster_node_t *node = data; - - fr_assert(node->pool); - - if (live->skip == node->id) return 0; /* Skip the dead node */ - - live->node[live->next].pool_state = fr_pool_state(node->pool); - live->node[live->next++].id = node->id; - - return 0; -} /** Try to determine the health of a cluster node passively by examining its pool state * @@ -1322,6 +1300,8 @@ static int cluster_node_find_live(fr_redis_cluster_node_t **live_node, fr_redis_ cluster_nodes_live_t *live; fr_time_t now; + fr_rb_tree_iter_inorder_t iter; + fr_redis_cluster_node_t *node; RDEBUG2("Searching for live cluster nodes"); @@ -1335,7 +1315,15 @@ static int cluster_node_find_live(fr_redis_cluster_node_t **live_node, fr_redis_ live->skip = skip->id; pthread_mutex_lock(&cluster->mutex); - rbtree_walk(cluster->used_nodes, RBTREE_IN_ORDER, _cluster_pool_walk, live); + for (node = rbtree_iter_init_inorder(&iter, cluster->used_nodes); + node; + node = rbtree_iter_next_inorder(&iter)) { + fr_assert(node->pool); + if (live->skip == node->id) continue; /* Skip dead nodes */ + + live->node[live->next].pool_state = fr_pool_state(node->pool); + live->node[live->next++].id = node->id; + } pthread_mutex_unlock(&cluster->mutex); fr_assert(live->next); /* There should be at least one */ @@ -1348,7 +1336,6 @@ static int cluster_node_find_live(fr_redis_cluster_node_t **live_node, fr_redis_ */ for (i = 0; (i < cluster->conf->max_alt) && live->next; i++) { fr_redis_conn_t *conn; - fr_redis_cluster_node_t *node; uint8_t j; int first, last, pivot; /* Must be signed for BS */ unsigned int find, cumulative = 0; @@ -2081,35 +2068,6 @@ int fr_redis_cluster_pool_by_node_addr(fr_pool_t **pool, fr_redis_cluster_t *clu return 0; } -/** Private ctx structure to pass to _cluster_role_walk - * - */ -typedef struct { - bool is_master; - bool is_slave; - uint8_t count; - fr_socket_t *found; -} addr_by_role_ctx_t; - -/** Walk all used pools, recording the IP addresses of ones matching the filter - * - * @param[in] uctx Where to write the node we found. - * @param[in] data node to check. - * @return - * - 0 continue walking. - * - -1 found suitable node. - */ -static int _cluster_role_walk(void *data, void *uctx) -{ - addr_by_role_ctx_t *ctx = uctx; - fr_redis_cluster_node_t *node = data; - - if ((ctx->is_master && node->is_master) || (ctx->is_slave && !node->is_master)) { - ctx->found[ctx->count++] = node->addr; - } - return 0; -} - /** Return an array of IP addresses belonging to masters or slaves * * @note We return IP addresses as they're safe to use across cluster remaps. @@ -2125,37 +2083,45 @@ static int _cluster_role_walk(void *data, void *uctx) ssize_t fr_redis_cluster_node_addr_by_role(TALLOC_CTX *ctx, fr_socket_t *out[], fr_redis_cluster_t *cluster, bool is_master, bool is_slave) { - addr_by_role_ctx_t context; size_t in_use = rbtree_num_elements(cluster->used_nodes); + fr_rb_tree_iter_inorder_t iter; + fr_redis_cluster_node_t *node; + uint8_t count; + fr_socket_t *found; if (in_use == 0) { *out = NULL; return 0; } - context.is_master = is_master; - context.is_slave = is_slave; - context.count = 0; - context.found = talloc_zero_array(ctx, fr_socket_t, in_use); - if (!context.found) { + count = 0; + found = talloc_zero_array(ctx, fr_socket_t, in_use); + if (!found) { fr_strerror_const("Out of memory"); return -1; } pthread_mutex_lock(&cluster->mutex); - rbtree_walk(cluster->used_nodes, RBTREE_IN_ORDER, _cluster_role_walk, &context); - *out = context.found; + + for (node = rbtree_iter_init_inorder(&iter, cluster->used_nodes); + node; + node = rbtree_iter_next_inorder(&iter)) { + if ((is_master && node->is_master) || (is_slave && !node->is_master)) { + found[count++] = node->addr; + } + } + pthread_mutex_unlock(&cluster->mutex); - if (context.count == 0) { + if (count == 0) { *out = NULL; - talloc_free(context.found); + talloc_free(found); return 0; } - *out = context.found; + *out = found; - return context.count; + return count; } /** Destroy mutex associated with cluster slots structure @@ -2170,43 +2136,6 @@ static int _fr_redis_cluster_free(fr_redis_cluster_t *cluster) return 0; } -/** Walk all used pools checking their versions - * - * @param[in] uctx Where to write the node we found. - * @param[in] data node to check. - * @return - * - 0 continue walking. - * - -1 found suitable node. - */ -static int _cluster_version_walk(void *data, void *uctx) -{ - char const *min_version = uctx; - fr_redis_cluster_node_t *node = data; - fr_redis_conn_t *conn; - int ret; - char buffer[40]; - - conn = fr_pool_connection_get(node->pool, NULL); - if (!conn) return 0; - - /* - * We don't care if we can't get the version - * as we don't want to prevent the server from - * starting if start == 0. - */ - ret = fr_redis_get_version(buffer, sizeof(buffer), conn); - fr_pool_connection_release(node->pool, NULL, conn); - if (ret < 0) return 0; - - if (fr_redis_version_num(buffer) < fr_redis_version_num(min_version)) { - fr_strerror_printf("Redis node %s:%i (currently v%s) needs update to >= v%s", - node->name, node->addr.inet.dst_port, buffer, min_version); - return -1; - } - - return 0; -} - /** Check if members of the cluster are above a certain version * * @param cluster to perform check on. @@ -2219,16 +2148,42 @@ static int _cluster_version_walk(void *data, void *uctx) */ bool fr_redis_cluster_min_version(fr_redis_cluster_t *cluster, char const *min_version) { - int ret; - char *p; - - memcpy(&p, &min_version, sizeof(p)); + fr_redis_cluster_node_t *node; + fr_rb_tree_iter_inorder_t iter; + fr_redis_conn_t *conn; + int ret; + char buffer[40]; + bool all_above = true; pthread_mutex_lock(&cluster->mutex); - ret = rbtree_walk(cluster->used_nodes, RBTREE_IN_ORDER, _cluster_version_walk, p); + + for (node = rbtree_iter_init_inorder(&iter, cluster->used_nodes); + node; + node = rbtree_iter_next_inorder(&iter)) { + conn = fr_pool_connection_get(node->pool, NULL); + if (!conn) continue; + + /* + * We don't care if we can't get the version + * as we don't want to prevent the server from + * starting if start == 0. + */ + ret = fr_redis_get_version(buffer, sizeof(buffer), conn); + fr_pool_connection_release(node->pool, NULL, conn); + if (ret < 0) continue; + + if (fr_redis_version_num(buffer) < fr_redis_version_num(min_version)) { + fr_strerror_printf("Redis node %s:%i (currently v%s) needs update to >= v%s", + node->name, node->addr.inet.dst_port, buffer, min_version); + all_above = false; + rbtree_iter_done(&iter); + break; + } + } + pthread_mutex_unlock(&cluster->mutex); - return ret < 0 ? false : true; + return all_above; } /** Allocate and initialise a new cluster structure diff --git a/src/lib/server/cf_file.c b/src/lib/server/cf_file.c index 944d6311e26..d22cec0b0e9 100644 --- a/src/lib/server/cf_file.c +++ b/src/lib/server/cf_file.c @@ -698,45 +698,6 @@ bool cf_file_check(CONF_SECTION *cs, char const *filename, bool check_perms) return true; } -typedef struct { - int rcode; - fr_rb_walker_t callback; - CONF_SECTION *modules; -} cf_file_callback_t; - -/* - * Return 0 for keep going, 1 for stop. - */ -static int _file_callback(void *data, void *uctx) -{ - cf_file_callback_t *cb = uctx; - cf_file_t *file = data; - struct stat buf; - - /* - * The file doesn't exist or we can no longer read it. - */ - if (stat(file->filename, &buf) < 0) { - cb->rcode = CF_FILE_ERROR; - return 1; - } - - /* - * The file changed, we'll need to re-read it. - */ - if (buf.st_mtime != file->buf.st_mtime) { - if (cb->callback(cb->modules, file->cs)) { - cb->rcode |= CF_FILE_MODULE; - DEBUG3("HUP: Changed module file %s", file->filename); - } else { - DEBUG3("HUP: Changed config file %s", file->filename); - cb->rcode |= CF_FILE_CONFIG; - } - } - - return 0; -} - /* * Do variable expansion in pass2. * @@ -2417,21 +2378,47 @@ void cf_file_check_user(uid_t uid, gid_t gid) */ int cf_file_changed(CONF_SECTION *cs, fr_rb_walker_t callback) { - CONF_SECTION *top; - cf_file_callback_t cb; - rbtree_t *tree; + CONF_SECTION *top, *modules; + rbtree_t *tree; + cf_file_t *file; + int rcode; + struct stat buf; + fr_rb_tree_iter_inorder_t iter; top = cf_root(cs); tree = cf_data_value(cf_data_find(top, rbtree_t, "filename")); - if (!tree) return true; + if (!tree) return CF_FILE_ERROR; + + rcode = CF_FILE_NONE; + modules = cf_section_find(cs, "modules", NULL); - cb.rcode = CF_FILE_NONE; - cb.callback = callback; - cb.modules = cf_section_find(cs, "modules", NULL); + for (file = rbtree_iter_init_inorder(&iter, tree); + file; + file = rbtree_iter_next_inorder(&iter)) { + /* + * The file doesn't exist or we can no longer read it. + */ + if (stat(file->filename, &buf) < 0) { + rcode = CF_FILE_ERROR; + rbtree_iter_done(&iter); + break; + } - (void) rbtree_walk(tree, RBTREE_IN_ORDER, _file_callback, &cb); + /* + * The file changed, we'll need to re-read it. + */ + if (buf.st_mtime != file->buf.st_mtime) { + if (callback(modules, file->cs)) { + rcode |= CF_FILE_MODULE; + DEBUG3("HUP: Changed module file %s", file->filename); + } else { + DEBUG3("HUP: Changed config file %s", file->filename); + rcode |= CF_FILE_CONFIG; + } + } + } - return cb.rcode; + return rcode; } diff --git a/src/lib/server/cf_util.c b/src/lib/server/cf_util.c index 897898e37b6..30c8ac48f88 100644 --- a/src/lib/server/cf_util.c +++ b/src/lib/server/cf_util.c @@ -1673,39 +1673,6 @@ void *_cf_data_remove(CONF_ITEM *parent, CONF_DATA const *cd) return data; } -/** ctx data for a _cf_data_walk_call - * - */ -typedef struct { - char const *type; //!< of CONF_DATA we're iterating over. - cf_walker_t cb; //!< cb to process CONF_DATA. - void *ctx; //!< to pass to cb. -} cf_data_walk_ctx_t; - -/** Wrap a cf_walker_t in an fr_rb_walker_t - * - * @param[in] data A CONF_DATA entry. - * @param[in] uctx A cf_data_walk_ctx_t. - */ -static int _cf_data_walk_cb(void *data, void *uctx) -{ - cf_data_walk_ctx_t *cd_ctx = uctx; - CONF_DATA *cd = data; - CONF_ITEM *ci = data; - int ret; - - /* - * We're walking ident2, not all of the items will be data - */ - if (ci->type != CONF_ITEM_DATA) return 0; - - if ((cd->type != cd_ctx->type) && (strcmp(cd->type, cd_ctx->type) != 0)) return 0; - - ret = cd_ctx->cb(UNCONST(void *, cd->data), cd_ctx->ctx); - - return ret; -} - /** Walk over a specific type of CONF_DATA * * @param[in] ci containing the CONF_DATA to walk over. @@ -1718,15 +1685,31 @@ static int _cf_data_walk_cb(void *data, void *uctx) */ int _cf_data_walk(CONF_ITEM *ci, char const *type, cf_walker_t cb, void *ctx) { - cf_data_walk_ctx_t cd_ctx = { - .type = type, - .cb = cb, - .ctx = ctx - }; + CONF_DATA *cd; + fr_rb_tree_iter_inorder_t iter; + int ret = 0; if (!ci->ident2) return 0; - return rbtree_walk(ci->ident2, RBTREE_IN_ORDER, _cf_data_walk_cb, &cd_ctx); + for (ci = rbtree_iter_init_inorder(&iter, ci->ident2); + ci; + ci = rbtree_iter_next_inorder(&iter)) { + /* + * We're walking ident2, not all of the items will be data + */ + if (ci->type != CONF_ITEM_DATA) continue; + + cd = (void *) ci; + if ((cd->type != type) && (strcmp(cd->type, type) != 0)) continue; + + ret = cb(UNCONST(void *, cd->data), ctx); + if (ret) { + rbtree_iter_done(&iter); + break; + } + } + + return ret; } static inline CC_HINT(nonnull) void truncate_filename(char const **e, char const **p, int *len, char const *filename) diff --git a/src/lib/server/dl_module.c b/src/lib/server/dl_module.c index 579bb283ce7..aa505c2cea0 100644 --- a/src/lib/server/dl_module.c +++ b/src/lib/server/dl_module.c @@ -566,27 +566,25 @@ int dl_module_instance(TALLOC_CTX *ctx, dl_module_inst_t **out, return 0; } -#ifndef NDEBUG -static int _dl_inst_walk_print(void *data, UNUSED void *uctx) -{ - dl_module_inst_t *dl_inst = talloc_get_type_abort(data, dl_module_inst_t); - - WARN(" %s (%s)", dl_inst->module->dl->name, dl_inst->name); - - return 0; -} -#endif - static int _dl_module_loader_free(dl_module_loader_t *dl_module_l) { int ret = 0; if (rbtree_num_elements(dl_module_l->inst_data_tree) > 0) { - ret = -1; #ifndef NDEBUG + fr_rb_tree_iter_inorder_t iter; + void *data; + WARN("Refusing to cleanup dl loader, the following module instances are still in use:"); - rbtree_walk(dl_module_l->inst_data_tree, RBTREE_IN_ORDER, _dl_inst_walk_print, NULL); + for (data = rbtree_iter_init_inorder(&iter, dl_module_l->inst_data_tree); + data; + data = rbtree_iter_next_inorder(&iter)) { + dl_module_inst_t *dl_inst = talloc_get_type_abort(data, dl_module_inst_t); + + WARN(" %s (%s)", dl_inst->module->dl->name, dl_inst->name); + } #endif + ret = -1; goto finish; } diff --git a/src/lib/server/module.c b/src/lib/server/module.c index b4ab1da1ff0..7ce86a9c667 100644 --- a/src/lib/server/module.c +++ b/src/lib/server/module.c @@ -58,7 +58,7 @@ static rbtree_t *module_instance_data_tree; */ static fr_cmd_table_t cmd_module_table[]; -static int _module_instantiate(void *instance, UNUSED void *ctx); +static int _module_instantiate(void *instance); /* * Ordered by component @@ -83,59 +83,49 @@ static int cmd_show_module_config(FILE *fp, UNUSED FILE *fp_err, void *ctx, UNUS return 0; } -typedef struct { - char const *text; - int count; - int max_expansions; - char const **expansions; -} module_tab_expand_t; - - -static int _module_tab_expand(void *instance, void *ctx) -{ - module_instance_t *mi = talloc_get_type_abort(instance, module_instance_t); - module_tab_expand_t *mt = ctx; - - if (mt->count >= mt->max_expansions) return 1; - - if (fr_command_strncmp(mt->text, mi->name)) { - mt->expansions[mt->count] = strdup(mi->name); - mt->count++; - } - - return 0; -} - static int module_name_tab_expand(UNUSED TALLOC_CTX *talloc_ctx, UNUSED void *uctx, fr_cmd_info_t *info, int max_expansions, char const **expansions) { - module_tab_expand_t mt; + fr_rb_tree_iter_inorder_t iter; + void *instance; + char const *text; + int count; if (info->argc <= 0) return 0; - mt.text = info->argv[info->argc - 1]; - mt.count = 0; - mt.max_expansions = max_expansions; - mt.expansions = expansions; + text = info->argv[info->argc - 1]; + count = 0; - (void) rbtree_walk(module_instance_name_tree, RBTREE_IN_ORDER, _module_tab_expand, &mt); + for (instance = rbtree_iter_init_inorder(&iter, module_instance_name_tree); + instance; + instance = rbtree_iter_next_inorder(&iter)) { + module_instance_t *mi = talloc_get_type_abort(instance, module_instance_t); - return mt.count; + if (count >= max_expansions) { + rbtree_iter_done(&iter); + break; + } + if (fr_command_strncmp(text, mi->name)) { + expansions[count] = strdup(mi->name); + count++; + } + } + + return count; } -static int _module_list(void *instance, void *uctx) +static int cmd_show_module_list(FILE *fp, UNUSED FILE *fp_err, UNUSED void *uctx, UNUSED fr_cmd_info_t const *info) { - module_instance_t *mi = talloc_get_type_abort(instance, module_instance_t); - FILE *fp = uctx; + fr_rb_tree_iter_inorder_t iter; + void *instance; - fprintf(fp, "\t%s\n", mi->name); + for (instance = rbtree_iter_init_inorder(&iter, module_instance_name_tree); + instance; + instance = rbtree_iter_next_inorder(&iter)) { + module_instance_t *mi = talloc_get_type_abort(instance, module_instance_t); - return 0; -} - -static int cmd_show_module_list(FILE *fp, UNUSED FILE *fp_err, UNUSED void *uctx, UNUSED fr_cmd_info_t const *info) -{ - (void) rbtree_walk(module_instance_name_tree, RBTREE_IN_ORDER, _module_list, fp); + fprintf(fp, "\t%s\n", mi->name); + } return 0; } @@ -422,7 +412,7 @@ int module_sibling_section_find(CONF_SECTION **out, CONF_SECTION *module, char c parent = tmp; } while (true); - _module_instantiate(module_by_name(NULL, inst_name), NULL); + _module_instantiate(module_by_name(NULL, inst_name)); } /* @@ -1120,22 +1110,21 @@ void module_free(module_instance_t *mi) } -static int _module_instance_free_walker(void *data, UNUSED void *uctx) -{ - module_instance_t *mi = data; - - mi->in_name_tree = false; /* about to be deleted */ - talloc_free(mi); - return 2; -} - - /** Free all modules loaded by the server */ void modules_free(void) { if (module_instance_name_tree) { - rbtree_walk(module_instance_name_tree, RBTREE_DELETE_ORDER, _module_instance_free_walker, NULL); + fr_rb_tree_iter_inorder_t iter; + module_instance_t *mi; + + for (mi = rbtree_iter_init_inorder(&iter, module_instance_name_tree); + mi; + mi = rbtree_iter_next_inorder(&iter)) { + mi->in_name_tree = false; /* about to be deleted */ + talloc_free(mi); + rbtree_iter_delete_inorder(&iter); + } TALLOC_FREE(module_instance_name_tree); } @@ -1177,61 +1166,6 @@ static int _module_thread_inst_array_free(module_thread_instance_t **array) return 0; } -typedef struct { - module_thread_instance_t **array; //!< Containing the thread instances. - fr_event_list_t *el; //!< Event list for this thread. -} _thread_intantiate_ctx_t; - -/** Setup thread specific instance data for a module - * - * @param[in] uctx additional arguments to pass to a module's thread_instantiate function. - * @param[in] instance of module to perform thread instantiation for. - * @return - * - 0 on success. - * - -1 on failure. - */ -static int _module_thread_instantiate(void *instance, void *uctx) -{ - module_instance_t *mi = talloc_get_type_abort(instance, module_instance_t); - module_thread_instance_t *ti; - _thread_intantiate_ctx_t *thread_inst_ctx = uctx; - int ret; - - MEM(ti = talloc_zero(thread_inst_ctx->array, module_thread_instance_t)); - ti->el = thread_inst_ctx->el; - ti->module = mi->module; - ti->mod_inst = mi->dl_inst->data; /* For efficient lookups */ - - if (mi->module->thread_inst_size) { - MEM(ti->data = talloc_zero_array(ti, uint8_t, mi->module->thread_inst_size)); - - /* - * Fixup the type name, incase something calls - * talloc_get_type_abort() on it... - */ - if (!mi->module->thread_inst_type) { - talloc_set_name(ti->data, "rlm_%s_thread_t", mi->module->name); - } else { - talloc_set_name(ti->data, "%s", mi->module->thread_inst_type); - } - } - - DEBUG4("Worker alloced %s thread instance data (%p/%p)", ti->module->name, ti, ti->data); - if (mi->module->thread_instantiate) { - ret = mi->module->thread_instantiate(mi->dl_inst->conf, mi->dl_inst->data, - thread_inst_ctx->el, ti->data); - if (ret < 0) { - PERROR("Thread instantiation failed for module \"%s\"", mi->name); - return -1; - } - } - - fr_assert(mi->number < talloc_array_length(thread_inst_ctx->array)); - thread_inst_ctx->array[mi->number] = ti; - - return 0; -} - /** Creates per-thread instance data for modules which need it * * Must be called by any new threads before attempting to execute unlang sections. @@ -1245,6 +1179,9 @@ static int _module_thread_instantiate(void *instance, void *uctx) */ int modules_thread_instantiate(TALLOC_CTX *ctx, fr_event_list_t *el) { + void *instance; + fr_rb_tree_iter_inorder_t iter; + /* * Initialise the thread specific tree if this is the first time through */ @@ -1253,10 +1190,43 @@ int modules_thread_instantiate(TALLOC_CTX *ctx, fr_event_list_t *el) talloc_set_destructor(module_thread_inst_array, _module_thread_inst_array_free); } - if (rbtree_walk(module_instance_name_tree, RBTREE_IN_ORDER, _module_thread_instantiate, - &(_thread_intantiate_ctx_t){ .el = el, .array = module_thread_inst_array }) < 0) { - TALLOC_FREE(module_thread_inst_array); - return -1; + for (instance = rbtree_iter_init_inorder(&iter, module_instance_name_tree); + instance; + instance = rbtree_iter_next_inorder(&iter)) { + module_instance_t *mi = talloc_get_type_abort(instance, module_instance_t); + module_thread_instance_t *ti; + + MEM(ti = talloc_zero(module_thread_inst_array, module_thread_instance_t)); + ti->el = el; + ti->module = mi->module; + ti->mod_inst = mi->dl_inst->data; /* For efficient lookups */ + + if (mi->module->thread_inst_size) { + MEM(ti->data = talloc_zero_array(ti, uint8_t, mi->module->thread_inst_size)); + + /* + * Fixup the type name, incase something calls + * talloc_get_type_abort() on it... + */ + if (!mi->module->thread_inst_type) { + talloc_set_name(ti->data, "rlm_%s_thread_t", mi->module->name); + } else { + talloc_set_name(ti->data, "%s", mi->module->thread_inst_type); + } + } + + DEBUG4("Worker alloced %s thread instance data (%p/%p)", ti->module->name, ti, ti->data); + if (mi->module->thread_instantiate) { + if (mi->module->thread_instantiate(mi->dl_inst->conf, mi->dl_inst->data, el, ti->data) < 0) { + PERROR("Thread instantiation failed for module \"%s\"", mi->name); + rbtree_iter_done(&iter); + TALLOC_FREE(module_thread_inst_array); + return -1; + } + } + + fr_assert(mi->number < talloc_array_length(module_thread_inst_array)); + module_thread_inst_array[mi->number] = ti; } return 0; @@ -1276,12 +1246,11 @@ void modules_thread_detach(void) /** Complete module setup by calling its instantiate function * * @param[in] instance of module to complete instantiation for. - * @param[in] ctx modules section, containing instance data. * @return * - 0 on success. * - -1 on failure. */ -static int _module_instantiate(void *instance, UNUSED void *ctx) +static int _module_instantiate(void *instance) { module_instance_t *mi = talloc_get_type_abort(instance, module_instance_t); @@ -1346,9 +1315,19 @@ static int _module_instantiate(void *instance, UNUSED void *ctx) */ int modules_instantiate(void) { + void *instance; + fr_rb_tree_iter_inorder_t iter; + DEBUG2("#### Instantiating modules ####"); - if (rbtree_walk(module_instance_name_tree, RBTREE_IN_ORDER, _module_instantiate, NULL) < 0) return -1; + for (instance = rbtree_iter_init_inorder(&iter, module_instance_name_tree); + instance; + instance = rbtree_iter_next_inorder(&iter)) { + if (_module_instantiate(instance) < 0) { + rbtree_iter_done(&iter); + return -1; + } + } return 0; } diff --git a/src/lib/unlang/xlat_builtin.c b/src/lib/unlang/xlat_builtin.c index 9a6e3cd08a3..1952293501d 100644 --- a/src/lib/unlang/xlat_builtin.c +++ b/src/lib/unlang/xlat_builtin.c @@ -475,21 +475,19 @@ void xlat_unregister(char const *name) } -static int _xlat_unregister_callback(void *data, void *mod_inst) -{ - xlat_t *c = (xlat_t *) data; - - if (c->mod_inst != mod_inst) return 0; /* keep walking */ - - return 2; /* delete it */ -} - - void xlat_unregister_module(void *instance) { + xlat_t *c; + fr_rb_tree_iter_inorder_t iter; + if (!xlat_root) return; /* All xlats have already been freed */ - rbtree_walk(xlat_root, RBTREE_DELETE_ORDER, _xlat_unregister_callback, instance); + for (c = rbtree_iter_init_inorder(&iter, xlat_root); + c; + c = rbtree_iter_next_inorder(&iter)) { + if (c->mod_inst != instance) continue; + rbtree_iter_delete_inorder(&iter); + } } diff --git a/src/lib/unlang/xlat_inst.c b/src/lib/unlang/xlat_inst.c index 742fe1b5404..9dcbe5795f6 100644 --- a/src/lib/unlang/xlat_inst.c +++ b/src/lib/unlang/xlat_inst.c @@ -349,7 +349,8 @@ xlat_thread_inst_t *xlat_thread_instance_find(xlat_exp_t const *node) */ int xlat_thread_instantiate(TALLOC_CTX *ctx) { - int ret; + fr_rb_tree_iter_preorder_t iter; + void *data; if (!xlat_inst_tree) return 0; @@ -361,10 +362,14 @@ int xlat_thread_instantiate(TALLOC_CTX *ctx) /* * Walk the inst tree, creating thread specific instances. */ - ret = rbtree_walk(xlat_inst_tree, RBTREE_PRE_ORDER, _xlat_thread_instantiate, xlat_thread_inst_tree); - if (ret < 0) { - TALLOC_FREE(xlat_thread_inst_tree); - return -1; + for (data = rbtree_iter_init_preorder(&iter, xlat_inst_tree); + data; + data = rbtree_iter_next_preorder(&iter)) { + if (_xlat_thread_instantiate(data, xlat_thread_inst_tree) < 0) { + TALLOC_FREE(xlat_thread_inst_tree); + rbtree_iter_done(&iter); + return -1; + } } return 0; @@ -380,21 +385,6 @@ void xlat_thread_detach(void) TALLOC_FREE(xlat_thread_inst_tree); } -/** Walk over #xlat_exp_t that require instantiation - * - * @param[in] uctx UNUSED. - * @param[in] data node to perform - */ -static int _xlat_instantiate_walker(void *data, UNUSED void *uctx) -{ - xlat_inst_t *inst = talloc_get_type_abort(data, xlat_inst_t); - - if (inst->node->call.func->instantiate && - (inst->node->call.func->instantiate(inst->data, inst->node, inst->node->call.func->uctx) < 0)) return -1; - - return 0; -} - /** Initialise the xlat inst code * */ @@ -415,9 +405,24 @@ static int xlat_instantiate_init(void) */ int xlat_instantiate(void) { + fr_rb_tree_iter_preorder_t iter; + void *data; + if (!xlat_inst_tree) xlat_instantiate_init(); - return rbtree_walk(xlat_inst_tree, RBTREE_PRE_ORDER, _xlat_instantiate_walker, NULL); + for (data = rbtree_iter_init_preorder(&iter, xlat_inst_tree); + data; + data = rbtree_iter_next_preorder(&iter)) { + xlat_inst_t *inst = talloc_get_type_abort(data, xlat_inst_t); + + if (inst->node->call.func->instantiate && + (inst->node->call.func->instantiate(inst->data, inst->node, inst->node->call.func->uctx) < 0)) { + rbtree_iter_done(&iter); + return -1; + } + } + + return 0; } /** Callback for creating "permanent" instance data for a #xlat_exp_t diff --git a/src/lib/util/dl.c b/src/lib/util/dl.c index 388879ba1e7..89c25c8d541 100644 --- a/src/lib/util/dl.c +++ b/src/lib/util/dl.c @@ -616,17 +616,6 @@ int dl_free(dl_t const *dl) return talloc_decrease_ref_count(talloc_get_type_abort_const(dl, dl_t)); } -#ifndef NDEBUG -static int _dl_walk_print(void *data, UNUSED void *uctx) -{ - dl_t *dl = talloc_get_type_abort(data, dl_t); - - fr_strerror_printf_push(" %s (%zu)", dl->name, talloc_reference_count(dl)); - - return 0; -} -#endif - static int _dl_loader_free(dl_loader_t *dl_loader) { int ret = 0; @@ -642,15 +631,25 @@ static int _dl_loader_free(dl_loader_t *dl_loader) * should still be active. */ if (rbtree_num_elements(dl_loader->tree) > 0) { - ret = -1; #ifndef NDEBUG + fr_rb_tree_iter_inorder_t iter; + void *data; + /* * Yes, this is the correct call order */ - rbtree_walk(dl_loader->tree, RBTREE_IN_ORDER, _dl_walk_print, NULL); + for (data = rbtree_iter_init_inorder(&iter, dl_loader->tree); + data; + data = rbtree_iter_next_inorder(&iter)) { + dl_t *dl = talloc_get_type_abort(data, dl_t); + + fr_strerror_printf_push(" %s (%zu)", dl->name, talloc_reference_count(dl)); + } + fr_strerror_printf_push("Refusing to cleanup dl loader, the following dynamically loaded " "libraries are still in use:"); #endif + ret = -1; goto finish; } diff --git a/src/lib/util/event.c b/src/lib/util/event.c index 94bce66bad5..b80e9cee8de 100644 --- a/src/lib/util/event.c +++ b/src/lib/util/event.c @@ -2383,13 +2383,6 @@ static int event_timer_location_cmp(void const *one, void const *two) return (a->line > b->line) - (a->line < b->line); } -static int event_timer_print_location(void *node, UNUSED void *uctx) -{ - fr_event_counter_t *counter = talloc_get_type_abort(node, fr_event_counter_t); - - EVENT_DEBUG(" : %u allocd at %s[%u]", counter->count, counter->file, counter->line); - return 0; -} /** Print out information about the number of events in the event loop * @@ -2457,6 +2450,9 @@ void fr_event_report(fr_event_list_t *el, fr_time_t now, void *uctx) EVENT_DEBUG(" num timer events : %u", fr_event_list_num_timers(el)); for (i = 0; i < NUM_ELEMENTS(decades); i++) { + fr_rb_tree_iter_inorder_t iter; + void *node; + if (!array[i]) continue; if (i == 0) { @@ -2467,7 +2463,14 @@ void fr_event_report(fr_event_list_t *el, fr_time_t now, void *uctx) EVENT_DEBUG(" events %5s - %5s : %zu", decade_names[i - 1], decade_names[i], array[i]); } - rbtree_walk(locations[i], RBTREE_IN_ORDER, event_timer_print_location, NULL); + for (node = rbtree_iter_init_inorder(&iter, locations[i]); + node; + node = rbtree_iter_next_inorder(&iter)) { + fr_event_counter_t *counter = talloc_get_type_abort(node, fr_event_counter_t); + + EVENT_DEBUG(" : %u allocd at %s[%u]", + counter->count, counter->file, counter->line); + } } pthread_mutex_unlock(&print_lock); diff --git a/src/modules/rlm_cache/drivers/rlm_cache_rbtree/rlm_cache_rbtree.c b/src/modules/rlm_cache/drivers/rlm_cache_rbtree/rlm_cache_rbtree.c index 96d5a6172b2..16d091ea3c8 100644 --- a/src/modules/rlm_cache/drivers/rlm_cache_rbtree/rlm_cache_rbtree.c +++ b/src/modules/rlm_cache/drivers/rlm_cache_rbtree/rlm_cache_rbtree.c @@ -66,21 +66,6 @@ static int8_t cache_heap_cmp(void const *one, void const *two) return (a->expires > b->expires) - (a->expires < b->expires); } -/** Walk over the cache rbtree - * - * Used to free any entries left in the tree on detach. - * - * @param[in] data to free. - * @param[in] uctx unused. - * @return 2 - */ -static int _cache_entry_free(void *data, UNUSED void *uctx) -{ - talloc_free(data); - - return 2; -} - /** Cleanup a cache_rbtree instance * */ @@ -89,7 +74,15 @@ static int mod_detach(void *instance) rlm_cache_rbtree_t *driver = talloc_get_type_abort(instance, rlm_cache_rbtree_t); if (driver->cache) { - rbtree_walk(driver->cache, RBTREE_DELETE_ORDER, _cache_entry_free, NULL); + fr_rb_tree_iter_inorder_t iter; + void *data; + + for (data = rbtree_iter_init_inorder(&iter, driver->cache); + data; + data = rbtree_iter_next_inorder(&iter)) { + talloc_free(data); + rbtree_iter_delete_inorder(&iter); + } talloc_free(driver->cache); } diff --git a/src/protocols/radius/list.c b/src/protocols/radius/list.c index a0f78426544..ac3303bba1c 100644 --- a/src/protocols/radius/list.c +++ b/src/protocols/radius/list.c @@ -664,20 +664,6 @@ bool fr_packet_list_id_free(fr_packet_list_t *pl, return true; } -/* - * We always walk RBTREE_DELETE_ORDER, which is like RBTREE_IN_ORDER, except that - * <0 means error, stop - * 0 means OK, continue - * 1 means delete current node and stop - * 2 means delete current node and continue - */ -int fr_packet_list_walk(fr_packet_list_t *pl, fr_rb_walker_t callback, void *uctx) -{ - if (!pl || !callback) return 0; - - return rbtree_walk(pl->tree, RBTREE_DELETE_ORDER, callback, uctx); -} - int fr_packet_list_fd_set(fr_packet_list_t *pl, fd_set *set) { int i, maxfd; diff --git a/src/protocols/radius/list.h b/src/protocols/radius/list.h index f6647b147cd..273682f0424 100644 --- a/src/protocols/radius/list.h +++ b/src/protocols/radius/list.h @@ -54,7 +54,6 @@ bool fr_packet_list_socket_add(fr_packet_list_t *pl, int sockfd, int proto, bool fr_packet_list_socket_del(fr_packet_list_t *pl, int sockfd); bool fr_packet_list_socket_freeze(fr_packet_list_t *pl, int sockfd); bool fr_packet_list_socket_thaw(fr_packet_list_t *pl, int sockfd); -int fr_packet_list_walk(fr_packet_list_t *pl, fr_rb_walker_t callback, void *uctx); int fr_packet_list_fd_set(fr_packet_list_t *pl, fd_set *set); fr_radius_packet_t *fr_packet_list_recv(fr_packet_list_t *pl, fd_set *set, uint32_t max_attributes, bool require_ma); diff --git a/src/tests/rbmonkey.c b/src/tests/rbmonkey.c index d715791e85b..eb39777fc68 100644 --- a/src/tests/rbmonkey.c +++ b/src/tests/rbmonkey.c @@ -30,12 +30,6 @@ static int print_cb(void *i, UNUSED void *uctx) static int cb_stored = 0; static fr_rb_test_node_t rvals[MAXSIZE]; -static int store_cb(void *i, UNUSED void *uctx) -{ - rvals[cb_stored++].num = ((fr_rb_test_node_t const *)i)->num; - return 0; -} - static uint32_t mask; static int filter_cb(void *i, void *uctx) @@ -136,6 +130,8 @@ int main(UNUSED int argc, UNUSED char *argv[]) fr_rb_test_node_t thresh = {}; int n, rep; fr_rb_test_node_t vals[MAXSIZE]; + fr_rb_tree_iter_inorder_t iter; + fr_rb_test_node_t const *node; memset(&vals, 0, sizeof(vals)); @@ -177,13 +173,21 @@ again: * */ - (void) rbtree_walk(t, RBTREE_DELETE_ORDER, filter_cb, &thresh); + for (node = rbtree_iter_init_inorder(&iter, t); + node; + node = rbtree_iter_next_inorder(&iter)) { + if ((node->num & mask) == (thresh.num & mask)) rbtree_iter_delete_inorder(&iter); + } i = rbcount(t); fprintf(stderr,"After delete rbcount is %i\n", i); if (i < 0) return i; cb_stored = 0; - rbtree_walk(t, RBTREE_IN_ORDER, &store_cb, NULL); + for (node = rbtree_iter_init_inorder(&iter, t); + node; + node = rbtree_iter_next_inorder(&iter)) { + rvals[cb_stored++].num = node->num; + } for (j = i = 0; i < n; i++) { if (i && (vals[i-1].num == vals[i].num)) continue;