TODO talloc: improve the new_parent detection for abandoned children

[metze/samba/wip.git] / lib / talloc / talloc.c

diff --git a/lib/talloc/talloc.c b/lib/talloc/talloc.c
index c2755822fa20af9527be7aca76c4ede95bcde81f..4d72263a8a54c8b866be15387b755fa099afd309 100644 (file)
--- a/lib/talloc/talloc.c
+++ b/lib/talloc/talloc.c
@@ -685,6 +685,7 @@ static void *_talloc_steal_internal(const void *new_ctx, const void *ptr);
 
 static inline void _talloc_free_children_internal(struct talloc_chunk *tc,
                                                  void *ptr,
+                                                 bool only_children,
                                                  const char *location);
 
 /* 
@@ -755,7 +756,7 @@ static inline int _talloc_free_internal(void *ptr, const char *location)
                if (tc->next) tc->next->prev = tc->prev;
        }
 
-       _talloc_free_children_internal(tc, ptr, location);
+       _talloc_free_children_internal(tc, ptr, false, location);
 
        tc->flags |= TALLOC_FLAG_FREE;
 
@@ -1174,42 +1175,87 @@ _PUBLIC_ void *talloc_init(const char *fmt, ...)
 
 static inline void _talloc_free_children_internal(struct talloc_chunk *tc,
                                                  void *ptr,
+                                                 bool only_children,
                                                  const char *location)
 {
        tc->flags |= TALLOC_FLAG_LOOP;
 
        while (tc->child) {
-               /* we need to work out who will own an abandoned child
-                  if it cannot be freed. In priority order, the first
-                  choice is owner of any remaining reference to this
-                  pointer, the second choice is our parent, and the
-                  final choice is the null context. */
+               /*
+                * we need to work out who will own an abandoned child
+                * if it cannot be freed. In priority order, the first
+                * choice is owner of any remaining reference to this
+                * pointer, the second choice is our parent followed
+                * by grandparents finally terminated by the null context.
+                *
+                * finding the parent here is potentially quite
+                * expensive, but the alternative, which is to change
+                * talloc to always have a valid tc->parent pointer,
+                * makes realloc more expensive where there are a
+                * large number of children.
+                *
+                * The reason we need the parent pointer here is that
+                * if _talloc_free_internal() fails due to references
+                * or a failing destructor we need to re-parent, but
+                * the free call can invalidate the prev pointer.
+                */
                void *child = TC_PTR_FROM_CHUNK(tc->child);
-               const void *new_parent = null_context;
-               struct talloc_chunk *old_parent = NULL;
+               const void *new_parent = NULL;
+               bool need_new_parent = false;
+
                if (unlikely(tc->child->refs)) {
-                       struct talloc_chunk *p = talloc_parent_chunk(tc->child->refs);
-                       if (p) new_parent = TC_PTR_FROM_CHUNK(p);
+                       struct talloc_reference_handle *next = tc->child->refs;
+                       const void *invalid_parent;
+
+                       if (only_children) {
+                               invalid_parent = child;
+                       } else {
+                               invalid_parent = ptr;
+                       };
+
+                       /*
+                        * if the child has references
+                        * we should find a new parent for it
+                        */
+                       need_new_parent = true;
+
+                       while (next) {
+                               struct talloc_chunk *p;
+                               int is_child;
+                               struct talloc_reference_handle *ref = next;
+                               next = ref->next;
+
+                               is_child = talloc_is_parent(ref, invalid_parent);
+                               if (is_child) {
+                                       continue;
+                               }
+
+                               p = talloc_parent_chunk(ref);
+                               if (p) {
+                                       new_parent = TC_PTR_FROM_CHUNK(p);
+                                       break;
+                               }
+                       }
+               } else if (unlikely(tc->child->destructor)) {
+                       /*
+                        * if there is a destructor is attached
+                        * to the child there is a change that
+                        * _talloc_free_internal() returns -1
+                        * so we need to find a parent for the
+                        * child.
+                        */
+                       need_new_parent = true;
                }
-               /* finding the parent here is potentially quite
-                  expensive, but the alternative, which is to change
-                  talloc to always have a valid tc->parent pointer,
-                  makes realloc more expensive where there are a
-                  large number of children.
-
-                  The reason we need the parent pointer here is that
-                  if _talloc_free_internal() fails due to references
-                  or a failing destructor we need to re-parent, but
-                  the free call can invalidate the prev pointer.
-               */
-               if (new_parent == null_context && (tc->child->refs || tc->child->destructor)) {
-                       old_parent = talloc_parent_chunk(ptr);
+
+               if (unlikely(need_new_parent && new_parent == NULL)) {
+                       if (only_children) {
+                               new_parent = ptr;
+                       } else {
+                               new_parent = talloc_parent(ptr);
+                       }
                }
+
                if (unlikely(_talloc_free_internal(child, location) == -1)) {
-                       if (new_parent == null_context) {
-                               struct talloc_chunk *p = old_parent;
-                               if (p) new_parent = TC_PTR_FROM_CHUNK(p);
-                       }
                        _talloc_steal_internal(new_parent, child);
                }
        }
@@ -1236,7 +1282,7 @@ _PUBLIC_ void talloc_free_children(void *ptr)
                return;
        }
 
-       _talloc_free_children_internal(tc, ptr, __location__);
+       _talloc_free_children_internal(tc, ptr, true, __location__);
 }
 
 /*