From: Arran Cudbard-Bell Date: Fri, 16 Apr 2021 23:19:07 +0000 (-0500) Subject: Add "old" argument to fr_*_replace to return the old data X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=7846459984d9336f1a2bd121aac2a47d9cd3fbf3;p=thirdparty%2Ffreeradius-server.git Add "old" argument to fr_*_replace to return the old data This is to avoid multiple lookups in pair.c --- diff --git a/src/lib/server/map_proc.c b/src/lib/server/map_proc.c index b285a37e5cd..ff40ab68a2e 100644 --- a/src/lib/server/map_proc.c +++ b/src/lib/server/map_proc.c @@ -134,7 +134,7 @@ int map_proc_register(void *mod_inst, char const *name, strlcpy(proc->name, name, sizeof(proc->name)); proc->length = strlen(proc->name); - if (fr_rb_replace(map_proc_root, proc) < 0) { + if (fr_rb_replace(NULL, map_proc_root, proc) < 0) { talloc_free(proc); return -1; } diff --git a/src/lib/server/virtual_servers.c b/src/lib/server/virtual_servers.c index dfefd560083..1db8dcff75e 100644 --- a/src/lib/server/virtual_servers.c +++ b/src/lib/server/virtual_servers.c @@ -1236,7 +1236,7 @@ int virtual_namespace_register(char const *namespace, fr_dict_t const *dict, } } - if (fr_rb_replace(vns_tree, vns) < 0) { + if (fr_rb_replace(NULL, vns_tree, vns) < 0) { ERROR("Failed inserting namespace into tree"); return -1; } diff --git a/src/lib/unlang/xlat_builtin.c b/src/lib/unlang/xlat_builtin.c index 749921b7575..fba3ca6ed99 100644 --- a/src/lib/unlang/xlat_builtin.c +++ b/src/lib/unlang/xlat_builtin.c @@ -221,7 +221,7 @@ xlat_t *xlat_register_legacy(void *mod_inst, char const *name, DEBUG3("%s: %s", __FUNCTION__, c->name); - if (is_new && (fr_rb_replace(xlat_root, c) < 0)) { + if (is_new && (fr_rb_replace(NULL, xlat_root, c) < 0)) { ERROR("Failed inserting xlat registration for %s", c->name); talloc_free(c); @@ -292,7 +292,7 @@ xlat_t *xlat_register(TALLOC_CTX *ctx, char const *name, xlat_func_t func, bool talloc_set_destructor(c, _xlat_func_talloc_free); DEBUG3("%s: %s", __FUNCTION__, c->name); - if (fr_rb_replace(xlat_root, c) < 0) { + if (fr_rb_replace(NULL, xlat_root, c) < 0) { ERROR("%s: Failed inserting xlat registration for %s", __FUNCTION__, c->name); talloc_free(c); return NULL; diff --git a/src/lib/util/dict_util.c b/src/lib/util/dict_util.c index 4ff9629d70d..3e0562842c4 100644 --- a/src/lib/util/dict_util.c +++ b/src/lib/util/dict_util.c @@ -827,7 +827,7 @@ int dict_vendor_add(fr_dict_t *dict, char const *name, unsigned int num) * files, but when we're printing them, (and looking up * by value) we want to use the NEW name. */ - if (fr_hash_table_replace(dict->vendors_by_num, vendor) < 0) { + if (fr_hash_table_replace(NULL, dict->vendors_by_num, vendor) < 0) { fr_strerror_printf("%s: Failed inserting vendor %s", __FUNCTION__, name); return -1; } @@ -1049,7 +1049,7 @@ int dict_attr_add_to_namespace(fr_dict_attr_t const *parent, fr_dict_attr_t *da) * dictionary but entry in the name hash table is * updated to point to the new definition. */ - if (fr_hash_table_replace(namespace, da) < 0) { + if (fr_hash_table_replace(NULL, namespace, da) < 0) { fr_strerror_const("Internal error storing attribute"); goto error; } @@ -1316,7 +1316,7 @@ int dict_attr_enum_add_name(fr_dict_attr_t *da, char const *name, * take care of that here. */ if (takes_precedence) { - if (fr_hash_table_replace(ext->name_by_value, enumv) < 0) { + if (fr_hash_table_replace(NULL, ext->name_by_value, enumv) < 0) { fr_strerror_printf("%s: Failed inserting value %s", __FUNCTION__, name); return -1; } diff --git a/src/lib/util/hash.c b/src/lib/util/hash.c index a65b5099121..454e1fc1078 100644 --- a/src/lib/util/hash.c +++ b/src/lib/util/hash.c @@ -503,22 +503,33 @@ bool fr_hash_table_insert(fr_hash_table_t *ht, void const *data) /** Replace old data with new data, OR insert if there is no old * + * @param[out] old data that was replaced. If this argument + * is not NULL, then the old data will not + * be freed, even if a free function is + * configured. * @param[in] ht to insert data into. * @param[in] data to replace. Will be passed to the * hashing function. * @return - * - 1 if data was inserted. - * - 0 if data was replaced. + * - 1 if data was replaced. + * - 0 if data was inserted. * - -1 if we failed to replace data */ -int fr_hash_table_replace(fr_hash_table_t *ht, void const *data) +int fr_hash_table_replace(void **old, fr_hash_table_t *ht, void const *data) { fr_hash_entry_t *node; node = hash_table_find(ht, ht->hash(data), data); - if (!node) return fr_hash_table_insert(ht, data) ? 1 : -1; + if (!node) { + if (old) *old = NULL; + return fr_hash_table_insert(ht, data) ? 1 : -1; + } - if (ht->free) ht->free(node->data); + if (old) { + *old = node->data; + } else if (ht->free) { + ht->free(node->data); + } node->data = UNCONST(void *, data); diff --git a/src/lib/util/hash.h b/src/lib/util/hash.h index d00fe5cefa0..0b3d75c887a 100644 --- a/src/lib/util/hash.h +++ b/src/lib/util/hash.h @@ -66,7 +66,7 @@ void *fr_hash_table_find_by_key(fr_hash_table_t *ht, uint32_t key, void const * bool fr_hash_table_insert(fr_hash_table_t *ht, void const *data) CC_HINT(nonnull); -int fr_hash_table_replace(fr_hash_table_t *ht, void const *data) CC_HINT(nonnull); +int fr_hash_table_replace(void **old, fr_hash_table_t *ht, void const *data) CC_HINT(nonnull(2,3)); void *fr_hash_table_remove(fr_hash_table_t *ht, void const *data) CC_HINT(nonnull); diff --git a/src/lib/util/htrie.h b/src/lib/util/htrie.h index df35998e461..da723844114 100644 --- a/src/lib/util/htrie.h +++ b/src/lib/util/htrie.h @@ -38,7 +38,7 @@ typedef void *(*fr_htrie_find_t)(fr_htrie_t *ht, void const *data); typedef bool (*fr_htrie_insert_t)(fr_htrie_t *ht, void const *data); -typedef int (*fr_htrie_replace_t)(fr_htrie_t *ht, void const *data); +typedef int (*fr_htrie_replace_t)(void **old, fr_htrie_t *ht, void const *data); typedef void *(*fr_htrie_remove_t)(fr_htrie_t *ht, void const *data); @@ -99,9 +99,9 @@ static inline CC_HINT(nonnull) bool fr_htrie_insert(fr_htrie_t *ht, void const * /** Replace data in a htrie, freeing previous data if free_data cb was passed to fr_htrie_alloc * */ -static inline CC_HINT(nonnull) int fr_htrie_replace(fr_htrie_t *ht, void const *data) +static inline CC_HINT(nonnull(2,3)) int fr_htrie_replace(void **old, fr_htrie_t *ht, void const *data) { - return ht->funcs.replace(ht->store, data); + return ht->funcs.replace(old, ht->store, data); } /** Remove data from a htrie without freeing it diff --git a/src/lib/util/rb.c b/src/lib/util/rb.c index 35d23235d47..ef7c8081bfe 100644 --- a/src/lib/util/rb.c +++ b/src/lib/util/rb.c @@ -33,7 +33,7 @@ static fr_rb_node_t sentinel = { NIL, NIL, NULL, NULL, BLACK, false }; # define RB_MAGIC (0x5ad09c42) #endif -static fr_rb_node_t *insert_node(fr_rb_tree_t *tree, void *data) CC_HINT(nonnull); +static int insert_node(fr_rb_node_t **existing, fr_rb_tree_t *tree, void *data) CC_HINT(nonnull); static inline CC_HINT(always_inline) void node_data_free(fr_rb_tree_t const *tree, fr_rb_node_t *node) { @@ -333,8 +333,16 @@ static inline CC_HINT(always_inline) void insert_fixup(fr_rb_tree_t *tree, fr_rb /** Insert an element into the tree * + * @param[out] existing if a node exists, and existing is not NULL + * this will be populated with the node. + * @param[in] tree to search in. + * @param[in] data to search for. + * @return + * - 1 on existing (with existing populated). + * - 0 on success. + * - -1 on failure. */ -static fr_rb_node_t *insert_node(fr_rb_tree_t *tree, void *data) +static int insert_node(fr_rb_node_t **existing, fr_rb_tree_t *tree, void *data) { fr_rb_node_t *current, *parent, *x; @@ -354,7 +362,10 @@ static fr_rb_node_t *insert_node(fr_rb_tree_t *tree, void *data) * See if two entries are identical. */ result = tree->data_cmp(data, current->data); - if (result == 0) return NULL; + if (result == 0) { + if (existing) *existing = current; + return 1 + } parent = current; current = (result < 0) ? current->left : current->right; @@ -362,7 +373,7 @@ static fr_rb_node_t *insert_node(fr_rb_tree_t *tree, void *data) /* setup new node */ x = tree->node_alloc(tree, data); - if (unlikely(!x)) return NULL; + if (unlikely(!x)) return -1; *x = (fr_rb_node_t){ .data = data, @@ -387,7 +398,7 @@ static fr_rb_node_t *insert_node(fr_rb_tree_t *tree, void *data) tree->num_elements++; - return x; + return 0; } /** Maintain RED-BLACK tree balance after deleting node x @@ -573,6 +584,35 @@ void *fr_rb_find(fr_rb_tree_t *tree, void const *data) return x->data; } +/** Attempt to find current data in the tree, if it does not exist insert it + * + * @param[out] found Pre-existing data we found. + * @param[in] tree to search/insert into. + * @param[in] data to find. + * @return + * - 1 if existing data was found, found will be populated. + * - 0 if no existing data was found. + * - -1 on insert error. + */ +int fr_rb_find_or_insert(void **found, fr_rb_tree_t *tree, void const *data) +{ + fr_rb_node_t *existing; + + switch (insert_node(existing, tree, UNCONST(void *, data))) { + case 1: + if (found) *found = existing->data; + return 1; + + case 0: + if (found) *found = NULL; + return 0; + + default: + if (found) *found = NULL; + return -1; + } +} + /** Insert data into a tree * * @param[in] tree to insert data into. @@ -583,44 +623,61 @@ void *fr_rb_find(fr_rb_tree_t *tree, void const *data) */ bool fr_rb_insert(fr_rb_tree_t *tree, void const *data) { - if (insert_node(tree, UNCONST(void *, data))) return true; + if (insert_node(NULL, tree, UNCONST(void *, data)) == 0) return true; return false; } /** Replace old data with new data, OR insert if there is no old * + * @param[out] old data that was replaced. If this argument + * is not NULL, then the old data will not + * be freed, even if a free function is + * configured. * @param[in] tree to insert data into. * @param[in] data to replace. * @return - * - 1 if data was inserted. - * - 0 if data was replaced. + * - 1 if data was replaced. + * - 0 if data was inserted. * - -1 if we failed to replace data */ -int fr_rb_replace(fr_rb_tree_t *tree, void const *data) +int fr_rb_replace(void **old, fr_rb_tree_t *tree, void const *data) { fr_rb_node_t *node; - void *old_data; - node = find_node(tree, UNCONST(void *, data)); - if (!node) return insert_node(tree, UNCONST(void *, data)) ? 1 : -1; - old_data = node->data; + switch (insert_node(&node, tree, UNCONST(void *, data))) { + case 1: /* Something exists */ + { + void *old_data = node->data; - /* - * If the fr_node_t is inline with the - * data structure, we need to delete - * the old node out of the tree, and - * perform a normal insert operation. - */ - if (tree->node_alloc == _node_inline_alloc) { - delete_internal(tree, node, false); - insert_node(tree, UNCONST(void *, data)); - } else { - node->data = UNCONST(void *, data); + /* + * If the fr_node_t is inline with the + * data structure, we need to delete + * the old node out of the tree, and + * perform a normal insert operation. + */ + if (tree->node_alloc == _node_inline_alloc) { + delete_internal(tree, node, false); + insert_node(NULL, tree, UNCONST(void *, data)); + } else { + node->data = UNCONST(void *, data); + } + + if (old) { + *old = old_data; + } else if (tree->data_free) { + tree->data_free(old_data); + } + return 1; } - if (tree->data_free) tree->data_free(old_data); + case 0: /* New node was inserted - There was no pre-existing node */ + if (old) *old = NULL; + return 0; - return 0; + default: + if (old) *old = NULL; + return -1; + } } /** Remove an entry from the tree, without freeing the data diff --git a/src/lib/util/rb.h b/src/lib/util/rb.h index 0509f29a284..84df3ae3168 100644 --- a/src/lib/util/rb.h +++ b/src/lib/util/rb.h @@ -287,9 +287,11 @@ fr_rb_tree_t *_fr_rb_alloc(TALLOC_CTX *ctx, ssize_t offset, char const *type, /** @hidecallergraph */ void *fr_rb_find(fr_rb_tree_t *tree, void const *data) CC_HINT(nonnull); +int fr_rb_find_or_insert(void **found, fr_rb_tree_t *tree, void const *data) CC_HINT(nonnull); + bool fr_rb_insert(fr_rb_tree_t *tree, void const *data) CC_HINT(nonnull); -int fr_rb_replace(fr_rb_tree_t *tree, void const *data) CC_HINT(nonnull); +int fr_rb_replace(void **old, fr_rb_tree_t *tree, void const *data) CC_HINT(nonnull(2,3)); void *fr_rb_remove(fr_rb_tree_t *tree, void const *data) CC_HINT(nonnull); diff --git a/src/lib/util/trie.c b/src/lib/util/trie.c index dc15e886263..ba11b045963 100644 --- a/src/lib/util/trie.c +++ b/src/lib/util/trie.c @@ -2685,20 +2685,24 @@ bool fr_trie_insert(fr_trie_t *ft, void const *data) /** Replace old data with new data, OR insert if there is no old * + * @param[out] old data that was replaced. If this argument + * is not NULL, then the old data will not + * be freed, even if a free function is + * configured. * @param[in] ft to insert data into. * @param[in] data to replace. * @return - * - 1 if data was inserted. - * - 0 if data was replaced. + * - 1 if data was replaced. + * - 0 if data was inserted. * - -1 if we failed to replace data */ -int fr_trie_replace(fr_trie_t *ft, void const *data) +int fr_trie_replace(void **old, fr_trie_t *ft, void const *data) { - fr_trie_user_t *user = (fr_trie_user_t *) ft; - fr_trie_ctx_t *uctx = talloc_get_type_abort(user->data, fr_trie_ctx_t); - uint8_t *key; - size_t keylen; - void *found; + fr_trie_user_t *user = (fr_trie_user_t *) ft; + fr_trie_ctx_t *uctx = talloc_get_type_abort(user->data, fr_trie_ctx_t); + uint8_t *key; + size_t keylen; + void *found; key = &uctx->buffer[0]; keylen = sizeof(uctx->buffer) * 8; @@ -2709,13 +2713,18 @@ int fr_trie_replace(fr_trie_t *ft, void const *data) found = trie_key_match(ft, key, 0, keylen, true); /* do exact match */ if (found) { + if (old) *old = found; if (fr_trie_remove_by_key(ft, key, keylen) != found) return -1; + } else { + if (old) *old = NULL; } /* * Insert the new key. */ - return fr_trie_insert_by_key(ft, key, keylen, data); + if (fr_trie_insert_by_key(ft, key, keylen, data) < 0) return -1; + + return found ? 1 : 0; } /** Remove an entry, without freeing the data diff --git a/src/lib/util/trie.h b/src/lib/util/trie.h index b7c11b00451..b20b44960b5 100644 --- a/src/lib/util/trie.h +++ b/src/lib/util/trie.h @@ -74,7 +74,7 @@ void *fr_trie_find(fr_trie_t *ft, void const *data) CC_HINT(nonnull); bool fr_trie_insert(fr_trie_t *ft, void const *data) CC_HINT(nonnull); -int fr_trie_replace(fr_trie_t *ft, void const *data) CC_HINT(nonnull); +int fr_trie_replace(void **old, fr_trie_t *ft, void const *data) CC_HINT(nonnull(2, 3)); void *fr_trie_remove(fr_trie_t *ft, void const *data) CC_HINT(nonnull);