[PATCH] Allow talloc reparenting in a destructor.
Jeremy Allison
jra at samba.org
Thu Mar 5 09:38:08 MST 2015
On Thu, Mar 05, 2015 at 05:24:32PM +0100, Volker Lendecke wrote:
> On Wed, Mar 04, 2015 at 01:28:53PM -0800, Jeremy Allison wrote:
> > Here is a proposed addition to talloc (plus tests for it).
> >
> > If a destructor returns failure (-1) when freeing a child, talloc
> > reparents the child so the memory doesn't get lost.
> >
> > Firstly it tries the owner of any reference, next the parent of the
> > current object calling _talloc_free_children_internal(), and finally
> > the null context in the last resort.
> >
> > If a destructor reparents its own object, which can be a very
> > desirable thing to do (a destructor can make a decision it isn't
> > time to die yet, and as the parent may be going away it might
> > want to move itself to longer-term storage) then this new parent
> > gets overwritten by the existing reparenting logic.
> >
> > This patch checks when freeing a child if it already reparented
> > itself, and if it did then it doesn't overwrite the new parent.
> >
> > Please review and push.
>
> Pushed, but fails with
>
> [2750/4333] Compiling lib/talloc/testsuite.c
> cc1: warnings being treated as errors ../lib/talloc/testsuite.c: In function 'reparenting_destructor':
> ../lib/talloc/testsuite.c:991: error: value computed is not used
> Waf: Leaving directory `/memdisk/vlendec/a/b24052/samba/bin'
> Build failed: -> task failed (err #1):
> {task: cc testsuite.c -> testsuite_1.o}
> make: *** [all] Error 1
Ah - picky compilers on our build machine :-).
I need to explicitly ignore the return
from talloc_move() I think. Just added a (void).
Updated patch attached.
-------------- next part --------------
From 1fb46c4c3840db45870a034404cfdba93a65352a Mon Sep 17 00:00:00 2001
From: Jeremy Allison <jra at samba.org>
Date: Tue, 3 Mar 2015 17:02:47 -0800
Subject: [PATCH 1/2] lib: talloc: Allow destructors to reparent the object
they're called on.
If a destructor returns failure (-1) when freeing a child, talloc
must then reparent the child.
Firstly it tries the owner of any reference, next the parent of the
current object calling _talloc_free_children_internal(), and finally
the null context in the last resort.
If a destructor reparented its own object, which can be a very
desirable thing to do (a destructor can make a decision it isn't
time to die yet, and as the parent may be going away it might
want to move itself to longer-term storage) then this new parent
gets overwritten by the existing reparenting logic.
This patch checks when freeing a child if it already reparented
itself, and if it did doesn't then overwrite the new parent.
Makes destructors more flexible.
Signed-off-by: Jeremy Allison <jra at samba.org>
---
lib/talloc/talloc.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/lib/talloc/talloc.c b/lib/talloc/talloc.c
index fa56ea5..9e5ba11 100644
--- a/lib/talloc/talloc.c
+++ b/lib/talloc/talloc.c
@@ -1464,6 +1464,13 @@ static inline void _talloc_free_children_internal(struct talloc_chunk *tc,
if (p) new_parent = TC_PTR_FROM_CHUNK(p);
}
if (unlikely(_talloc_free_internal(child, location) == -1)) {
+ if (talloc_parent_chunk(child) != tc) {
+ /*
+ * Destructor already reparented this child.
+ * No further reparenting needed.
+ */
+ return;
+ }
if (new_parent == null_context) {
struct talloc_chunk *p = talloc_parent_chunk(ptr);
if (p) new_parent = TC_PTR_FROM_CHUNK(p);
--
2.2.0.rc0.207.ga3a616c
From d1eb6ca518651b834735e002e0dc47ea15db72f4 Mon Sep 17 00:00:00 2001
From: Jeremy Allison <jra at samba.org>
Date: Tue, 3 Mar 2015 17:12:32 -0800
Subject: [PATCH 2/2] lib: talloc: Test suite for the new destructor reparent
logic.
Signed-off-by: Jeremy Allison <jra at samba.org>
---
lib/talloc/testsuite.c | 82 ++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 82 insertions(+)
diff --git a/lib/talloc/testsuite.c b/lib/talloc/testsuite.c
index a878278..82e5b28 100644
--- a/lib/talloc/testsuite.c
+++ b/lib/talloc/testsuite.c
@@ -981,6 +981,84 @@ static bool test_free_parent_deny_child(void)
return true;
}
+struct new_parent {
+ void *new_parent;
+ char val[20];
+};
+
+static int reparenting_destructor(struct new_parent *np)
+{
+ (void)talloc_move(np->new_parent, &np);
+ return -1;
+}
+
+static bool test_free_parent_reparent_child(void)
+{
+ void *top = talloc_new(NULL);
+ char *level1;
+ char *alternate_level1;
+ char *level2;
+ struct new_parent *level3;
+
+ printf("test: free_parent_reparent_child\n# "
+ "TALLOC FREE PARENT REPARENT CHILD\n");
+
+ level1 = talloc_strdup(top, "level1");
+ alternate_level1 = talloc_strdup(top, "alternate_level1");
+ level2 = talloc_strdup(level1, "level2");
+ level3 = talloc(level2, struct new_parent);
+ level3->new_parent = alternate_level1;
+ memset(level3->val, 'x', sizeof(level3->val));
+
+ talloc_set_destructor(level3, reparenting_destructor);
+ talloc_free(level1);
+ talloc_set_destructor(level3, NULL);
+
+ CHECK_PARENT("free_parent_reparent_child",
+ level3, alternate_level1);
+
+ talloc_free(top);
+
+ printf("success: free_parent_reparent_child\n");
+ return true;
+}
+
+static bool test_free_parent_reparent_child_in_pool(void)
+{
+ void *top = talloc_new(NULL);
+ char *level1;
+ char *alternate_level1;
+ char *level2;
+ void *pool;
+ struct new_parent *level3;
+
+ printf("test: free_parent_reparent_child_in_pool\n# "
+ "TALLOC FREE PARENT REPARENT CHILD IN POOL\n");
+
+ pool = talloc_pool(top, 1024);
+ level1 = talloc_strdup(pool, "level1");
+ alternate_level1 = talloc_strdup(top, "alternate_level1");
+ level2 = talloc_strdup(level1, "level2");
+ level3 = talloc(level2, struct new_parent);
+ level3->new_parent = alternate_level1;
+ memset(level3->val, 'x', sizeof(level3->val));
+
+ talloc_set_destructor(level3, reparenting_destructor);
+ talloc_free(level1);
+ talloc_set_destructor(level3, NULL);
+
+ CHECK_PARENT("free_parent_reparent_child_in_pool",
+ level3, alternate_level1);
+
+ /* Even freeing alternate_level1 should leave pool alone. */
+ talloc_free(alternate_level1);
+ talloc_free(top);
+
+ printf("success: free_parent_reparent_child_in_pool\n");
+ return true;
+}
+
+
static bool test_talloc_ptrtype(void)
{
void *top = talloc_new(NULL);
@@ -1674,6 +1752,10 @@ bool torture_local_talloc(struct torture_context *tctx)
test_reset();
ret &= test_free_parent_deny_child();
test_reset();
+ ret &= test_free_parent_reparent_child();
+ test_reset();
+ ret &= test_free_parent_reparent_child_in_pool();
+ test_reset();
ret &= test_talloc_ptrtype();
test_reset();
ret &= test_talloc_free_in_destructor();
--
2.2.0.rc0.207.ga3a616c
More information about the samba-technical
mailing list