From: Ronnie Sahlberg Date: Thu, 9 Aug 2007 04:08:59 +0000 (+1000) Subject: change the mem hierarchy for trees. let the node be owned by the data X-Git-Tag: tevent-0.9.20~348^2~2442^2~3 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=adb49f02f08818e8588d114c14499d61e31e7324;p=thirdparty%2Fsamba.git change the mem hierarchy for trees. let the node be owned by the data we store in the tree and use a node destructor so that when the data is talloc_free()d we also remove the node from the tree. (This used to be ctdb commit b8dabd1811ebd85ee031563e95085f720a2fa04d) --- diff --git a/ctdb/common/rb_tree.c b/ctdb/common/rb_tree.c index a06cd02edbf..9876f654a32 100644 --- a/ctdb/common/rb_tree.c +++ b/ctdb/common/rb_tree.c @@ -26,14 +26,69 @@ }} while (0) +static void +tree_destructor_traverse_node(TALLOC_CTX *mem_ctx, trbt_node_t *node) +{ + talloc_set_destructor(node, NULL); + if (node->left) { + tree_destructor_traverse_node(mem_ctx, node->left); + } + if (node->right) { + tree_destructor_traverse_node(mem_ctx, node->right); + } + talloc_steal(mem_ctx, node); +} + +/* + destroy a tree and remove all its nodes + */ +static int tree_destructor(trbt_tree_t *tree) +{ + TALLOC_CTX *tmp_ctx; + trbt_node_t *node; + + if (tree == NULL) { + return 0; + } + + node=tree->root; + if (node == NULL) { + return 0; + } + + /* traverse the tree and remove the node destructor and steal + the node to the temporary context. + we dont want to use the existing destructor for the node + since that will remove the nodes one by one from the tree. + since the entire tree will be completely destroyed we dont care + if it is inconsistent or unbalanced while freeing the + individual nodes + */ + tmp_ctx = talloc_new(NULL); + tree_destructor_traverse_node(tmp_ctx, node); + talloc_free(tmp_ctx); + + return 0; +} + + +/* create a red black tree */ trbt_tree_t * -trbt_create(TALLOC_CTX *memctx) +trbt_create(TALLOC_CTX *memctx, uint32_t flags) { trbt_tree_t *tree; tree = talloc_zero(memctx, trbt_tree_t); NO_MEMORY_FATAL(tree); + /* If the tree is freed, we must walk over all entries and steal the + node from the stored data pointer and release the node. + Note, when we free the tree we only free the tree and not any of + the data stored in the tree. + */ + talloc_set_destructor(tree, tree_destructor); + tree->flags = flags; + return tree; } @@ -392,9 +447,10 @@ trbt_delete_case1(trbt_node_t *node) } static void -delete_node(trbt_node_t *node) +delete_node(trbt_node_t *node, BOOL from_destructor) { trbt_node_t *parent, *child, dc; + trbt_node_t *temp = NULL; /* This node has two child nodes, then just copy the content from the next smaller node with this node and delete the @@ -407,7 +463,7 @@ delete_node(trbt_node_t *node) if (node->left != NULL && node->right != NULL) { /* This node has two children, just copy the data */ /* find the predecessor */ - trbt_node_t *temp = node->left; + temp = node->left; while (temp->right != NULL) { temp = temp->right; @@ -416,15 +472,18 @@ delete_node(trbt_node_t *node) /* swap the predecessor data and key with the node to be deleted. */ - talloc_free(node->data); - node->data = talloc_steal(node, temp->data); node->key32 = temp->key32; + node->data = temp->data; + /* now we let node hang off the new data */ + talloc_steal(node->data, node); + temp->data = NULL; temp->key32 = -1; /* then delete the temp node. - this node is guaranteed to have at least one leaf child */ - delete_node(temp); - return; + this node is guaranteed to have at least one leaf + child */ + delete_node(temp, from_destructor); + goto finished; } @@ -495,10 +554,63 @@ delete_node(trbt_node_t *node) } } - talloc_free(node); +finished: + if (!from_destructor) { + talloc_free(node); + } + + /* if we came from a destructor and temp!=NULL this means we + did the node-swap but now the tree still contains the old + node which was freed in the destructor. Not good. + */ + if (from_destructor && temp) { + temp->key32 = node->key32; + temp->rb_color = node->rb_color; + + temp->data = node->data; + talloc_steal(temp->data, temp); + + temp->parent = node->parent; + if (temp->parent) { + if (temp->parent->left == node) { + temp->parent->left = temp; + } else { + temp->parent->right = temp; + } + } + + temp->left = node->left; + if (temp->left) { + temp->left->parent = temp; + } + temp->right = node->right; + if (temp->right) { + temp->right->parent = temp; + } + + if (temp->tree->root == node) { + temp->tree->root = temp; + } + } + + if ( (node->tree->flags & TRBT_AUTOFREE) + && (node->tree->root == NULL) ) { + talloc_free(node->tree); + } + return; } +/* + destroy a node and remove it from its tree + */ +static int node_destructor(trbt_node_t *node) +{ + delete_node(node, True); + + return 0; +} + static inline trbt_node_t * trbt_create_node(trbt_tree_t *tree, trbt_node_t *parent, uint32_t key, void *data) { @@ -513,7 +625,13 @@ trbt_create_node(trbt_tree_t *tree, trbt_node_t *parent, uint32_t key, void *dat node->left=NULL; node->right=NULL; node->key32=key; - node->data=talloc_steal(node, data); + node->data = data; + + /* let this node hang off data so that it is removed when + data is freed + */ + talloc_steal(data, node); + talloc_set_destructor(node, node_destructor); return node; } @@ -549,7 +667,11 @@ trbt_insert32(trbt_tree_t *tree, uint32_t key, void *data) void *old_data; old_data = node->data; - node->data = talloc_steal(node, data); + node->data = data; + /* Let the node now be owned by the new data + so the node is freed when the enw data is released + */ + talloc_steal(node->data, node); return old_data; } @@ -611,6 +733,10 @@ trbt_lookup32(trbt_tree_t *tree, uint32_t key) return NULL; } + +/* This deletes a node from the tree. + Note that this does not release the data that the node points to +*/ void trbt_delete32(trbt_tree_t *tree, uint32_t key) { @@ -620,7 +746,7 @@ trbt_delete32(trbt_tree_t *tree, uint32_t key) while(node){ if(key==node->key32){ - delete_node(node); + delete_node(node, False); return; } if(keykey32){ @@ -655,12 +781,11 @@ trbt_insert32_callback(trbt_tree_t *tree, uint32_t key, void *(*callback)(void * * insert this new leaf. */ while(1){ - /* this node already exists, replace data and return the - old data + /* this node already exists, replace it */ if(key==node->key32){ - node->data = talloc_steal(node, - callback(param, node->data)); + node->data = callback(param, node->data); + talloc_steal(node->data, node); return; } @@ -702,7 +827,6 @@ trbt_insert32_callback(trbt_tree_t *tree, uint32_t key, void *(*callback)(void * } - struct trbt_array_param { void *(*callback)(void *param, void *data); void *param; @@ -729,8 +853,11 @@ static void *array_insert_callback(void *p, void *data) and we must create one. */ if (data == NULL) { - /* create a new subtree and hang it off our current tree */ - tree = trbt_create(param->tree); + /* create a new subtree and hang it off our current tree + set it to autofree so that the tree is freed when + the last node in it has been released. + */ + tree = trbt_create(param->tree, TRBT_AUTOFREE); } else { /* we already have a subtree for this path */ tree = (trbt_tree_t *)data; @@ -765,7 +892,6 @@ trbt_insertarray32_callback(trbt_tree_t *tree, uint32_t keylen, uint32_t *key, v trbt_insert32_callback(tree, key[0], array_insert_callback, &tap); } - /* lookup the tree using an array of uint32 as a key */ void * trbt_lookuparray32(trbt_tree_t *tree, uint32_t keylen, uint32_t *key) @@ -789,41 +915,6 @@ trbt_lookuparray32(trbt_tree_t *tree, uint32_t keylen, uint32_t *key) } -/* delete a node from the tree using an array of uint32 as a key */ -void -trbt_deletearray32(trbt_tree_t *tree, uint32_t keylen, uint32_t *key) -{ - trbt_tree_t *next_tree; - - /* if we have reached the leaftree we can just delete the node - if it exists - */ - if (keylen == 1) { - trbt_delete32(tree, key[0]); - return; - } - - - /* find the next subtree and recurse into it if it exists */ - next_tree = trbt_lookup32(tree, key[0]); - if (next_tree == NULL) { - return; - } - - trbt_deletearray32(next_tree, keylen-1, &key[1]); - - - /* when we returned from recursing into the the subtree, - that subtree might have become empty in which case we can delete it - as well - */ - if (next_tree->root == NULL) { - trbt_delete32(tree, key[0]); - } -} - - - /* traverse a tree starting at node */ static void trbt_traversearray32_node(trbt_node_t *node, uint32_t keylen, @@ -869,10 +960,9 @@ trbt_traversearray32(trbt_tree_t *tree, uint32_t keylen, trbt_traversearray32_node(node, keylen-1, callback, param); } - -# if 0 +#if 0 static void printtree(trbt_node_t *node, int levels) { int i; @@ -880,7 +970,7 @@ static void printtree(trbt_node_t *node, int levels) printtree(node->left, levels+1); for(i=0;ikey32,node->rb_color==TRBT_BLACK?"BLACK":"RED"); + printf("key:%d COLOR:%s (node:0x%08x parent:0x%08x left:0x%08x right:0x%08x)\n",node->key32,node->rb_color==TRBT_BLACK?"BLACK":"RED",(int)node,(int)node->parent, (int)node->left,(int)node->right); printtree(node->right, levels+1); printf("\n"); @@ -888,18 +978,19 @@ static void printtree(trbt_node_t *node, int levels) void print_tree(trbt_tree_t *tree) { - if(tree->tree==NULL){ + if(tree->root==NULL){ printf("tree is empty\n"); return; } printf("---\n"); - printtree(tree->tree->left, 1); - printf("root node key:%d COLOR:%s\n",tree->tree->key32,tree->tree->rb_color==TRBT_BLACK?"BLACK":"RED"); - printtree(tree->tree->right, 1); + printtree(tree->root->left, 1); + printf("root node key:%d COLOR:%s (node:0x%08x left:0x%08x right:0x%08x)\n",tree->root->key32,tree->root->rb_color==TRBT_BLACK?"BLACK":"RED",(int)tree->root,(int)tree->root->left,(int)tree->root->right); + printtree(tree->root->right, 1); printf("===\n"); } +#endif - +# if 0 void test_tree(void) { diff --git a/ctdb/common/rb_tree.h b/ctdb/common/rb_tree.h index 9c470aee964..09f73e649bf 100644 --- a/ctdb/common/rb_tree.h +++ b/ctdb/common/rb_tree.h @@ -33,12 +33,15 @@ typedef struct _trbt_node_t { typedef struct _trbt_tree_t { trbt_node_t *root; +/* automatically free the tree when the last node has been deleted */ +#define TRBT_AUTOFREE 0x00000001 + uint32_t flags; } trbt_tree_t; /* Create a RB tree */ -trbt_tree_t *trbt_create(TALLOC_CTX *memctx); +trbt_tree_t *trbt_create(TALLOC_CTX *memctx, uint32_t flags); /* Lookup a node in the tree and return a pointer to data or NULL */ void *trbt_lookup32(trbt_tree_t *tree, uint32_t key); @@ -71,9 +74,5 @@ void trbt_insertarray32_callback(trbt_tree_t *tree, uint32_t keylen, uint32_t *k and return a pointer to data or NULL */ void *trbt_lookuparray32(trbt_tree_t *tree, uint32_t keylen, uint32_t *key); -/* Delete a node in the tree with a key based on an array of uint32 - and return a pointer to data or NULL */ -void trbt_deletearray32(trbt_tree_t *tree, uint32_t keylen, uint32_t *key); - /* Traverse a tree with a key based on an array of uint32 */ void trbt_traversearray32(trbt_tree_t *tree, uint32_t keylen, void (*callback)(void *param, void *data), void *param); diff --git a/ctdb/server/ctdb_takeover.c b/ctdb/server/ctdb_takeover.c index 44dfc3914aa..9a904f9dd0b 100644 --- a/ctdb/server/ctdb_takeover.c +++ b/ctdb/server/ctdb_takeover.c @@ -992,7 +992,7 @@ static void capture_tcp_handler(struct event_context *ev, struct fd_event *fde, ctdb_sys_send_tcp(killtcp->sending_fd, &con->dst, &con->src, ack_seq, seq, 1); - trbt_deletearray32(killtcp->connections, 4, key); + talloc_free(con); } @@ -1059,23 +1059,6 @@ static int ctdb_killtcp_destructor(struct ctdb_kill_tcp *killtcp) return 0; } -/* - destroy a killtcp connection structure - */ -static int ctdb_killtcp_connection_destructor(struct ctdb_killtcp_con *con) -{ - uint32_t key[4]; - - key[0] = con->src.sin_addr.s_addr; - key[1] = con->dst.sin_addr.s_addr; - key[2] = con->src.sin_port; - key[3] = con->dst.sin_port; - - trbt_deletearray32(con->killtcp->connections, 4, key); - - return 0; -} - /* nothing fancy here, just unconditionally replace any existing connection structure with the new one. @@ -1109,7 +1092,7 @@ static int ctdb_killtcp_add_connection(struct ctdb_context *ctdb, killtcp->ctdb = ctdb; killtcp->capture_fd = -1; killtcp->sending_fd = -1; - killtcp->connections= trbt_create(killtcp); + killtcp->connections= trbt_create(killtcp, 0); ctdb->killtcp = killtcp; talloc_set_destructor(killtcp, ctdb_killtcp_destructor); @@ -1126,7 +1109,7 @@ static int ctdb_killtcp_add_connection(struct ctdb_context *ctdb, con->dst = *dst; con->count = 0; con->killtcp = killtcp; - talloc_set_destructor(con, ctdb_killtcp_connection_destructor); + key[0] = con->src.sin_addr.s_addr; key[1] = con->dst.sin_addr.s_addr; @@ -1166,7 +1149,7 @@ static int ctdb_killtcp_add_connection(struct ctdb_context *ctdb, /* We also need to set up some events to tickle all these connections until they are all reset */ - event_add_timed(ctdb->ev, killtcp, timeval_zero(), + event_add_timed(ctdb->ev, killtcp, timeval_current_ofs(1, 0), ctdb_tickle_sentenced_connections, killtcp); } diff --git a/ctdb/tests/rb_test.c b/ctdb/tests/rb_test.c index 1d95c8f84cb..20143974c0b 100644 --- a/ctdb/tests/rb_test.c +++ b/ctdb/tests/rb_test.c @@ -42,16 +42,16 @@ static double end_timer(void) (tp1.tv_sec + (tp1.tv_usec*1.0e-6)); } -int num_records; +int num_records=5; -void *callback(void *param, void *d) +void *callback(void *p, void *d) { uint32_t *data = (uint32_t *)d; - if(!data){ - data = talloc(NULL, uint32_t); - *data = 0; + if (d==NULL) { + data = (uint32_t *)p; } + (*data)++; return data; @@ -59,9 +59,6 @@ void *callback(void *param, void *d) void *random_add(void *p, void *d) { - if(d){ - talloc_free(d); - } return p; } @@ -102,6 +99,8 @@ int main(int argc, const char *argv[]) uint32_t key2[3] = {0,10,21}; uint32_t key3[3] = {0,11,20}; uint32_t key4[3] = {2,10,20}; + TALLOC_CTX *memctx; + uint32_t **u32array; pc = poptGetContext(argv[0], argc, argv, popt_options, POPT_CONTEXT_KEEP_FIRST); @@ -125,9 +124,13 @@ int main(int argc, const char *argv[]) printf("testing trbt_insert32_callback for %d records\n", num_records); - tree = trbt_create(NULL); + memctx = talloc_new(NULL); + u32array = talloc_array(memctx, uint32_t, num_records); + tree = trbt_create(memctx, 0); for (i=0; i