tal: remove TAL_TAKE in favor of ccan/take.

TAL_TAKE is awkward to use, particularly on functions which take multiple
arguments.  Instead, record annotations for pointers due to be freed in
the callee.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

ccan/tal/_info

@@ -96,6 +96,7 @@ int main(int argc, char *argv[])
 		printf("ccan/likely\n");
 		printf("ccan/list\n");
 		printf("ccan/str\n");
+		printf("ccan/take\n");
 		printf("ccan/typesafe_cb\n");
 		return 0;
 	}

ccan/tal/tal.c

@@ -3,6 +3,7 @@
 #include <ccan/compiler/compiler.h>
 #include <ccan/hash/hash.h>
 #include <ccan/list/list.h>
+#include <ccan/take/take.h>
 #include <assert.h>
 #include <stdio.h>
 #include <stdarg.h>
@@ -103,12 +104,14 @@ static struct group *next_group(struct group *group)
 	return list_entry(group->list.n.next, struct group, list.n);
 }
 
-static bool atexit_set = false;
+static bool initialized = false;
+
 /* This means valgrind can see leaks. */
-static void unlink_null(void)
+static void tal_cleanup(void)
 {
 	struct group *i, *next;
 
+	/* Unlink null_parent. */
 	for (i = next_group(&null_parent.c.group);
 	     i != &null_parent.c.group;
 	     i = next) {
@@ -116,6 +119,15 @@ static void unlink_null(void)
 		freefn(i);
 	}
 	null_parent.c.group.first_child = NULL;
+
+	/* Cleanup any taken pointers. */
+	take_cleanup();
+}
+
+/* For allocation failures inside ccan/take */
+static void take_alloc_failed(const void *p)
+{
+	tal_free(p);
 }
 
 #ifndef NDEBUG
@@ -332,9 +344,10 @@ static bool add_child(struct tal_hdr *parent, struct tal_hdr *child)
 	if (unlikely(!group->first_child)) {
 		assert(group == &children->group);
 		/* This hits on first child appended to null parent. */
-		if (unlikely(!atexit_set)) {
-			atexit(unlink_null);
-			atexit_set = true;
+		if (unlikely(!initialized)) {
+			atexit(tal_cleanup);
+			take_allocfail(take_alloc_failed);
+			initialized = true;
 		}
 		/* Link group into this child, make it the first one. */
 		group->hdr.next = child->prop;
@@ -728,15 +741,19 @@ void *tal_dup_(const tal_t *ctx, const void *p, size_t n, size_t extra,
 	/* Beware overflow! */
 	if (n + extra < n || n + extra + sizeof(struct tal_hdr) < n) {
 		call_error("dup size overflow");
-		if (ctx == TAL_TAKE)
+		if (taken(p))
 			tal_free(p);
 		return NULL;
 	}
 
-	if (ctx == TAL_TAKE) {
+	if (taken(p)) {
 		if (unlikely(!p))
 			return NULL;
-		if (!tal_resize_((void **)&p, n + extra)) {
+		if (unlikely(!tal_resize_((void **)&p, n + extra))) {
+			tal_free(p);
+			return NULL;
+		}
+		if (unlikely(!tal_steal(ctx, p))) {
 			tal_free(p);
 			return NULL;
 		}
@@ -766,11 +783,7 @@ char *tal_vasprintf(const tal_t *ctx, const char *fmt, va_list ap)
 	char *buf;
 	int ret;
 
-	if (ctx == TAL_TAKE)
-		buf = tal_arr(tal_parent(fmt), char, max);
-	else
-		buf = tal_arr(ctx, char, max);
-
+	buf = tal_arr(ctx, char, max);
 	while (buf) {
 		va_list ap2;
 
@@ -785,7 +798,7 @@ char *tal_vasprintf(const tal_t *ctx, const char *fmt, va_list ap)
 			buf = NULL;
 		}
 	}
-	if (ctx == TAL_TAKE)
+	if (taken(fmt))
 		tal_free(fmt);
 	return buf;
 }

ccan/tal/tal.h

@@ -18,18 +18,6 @@
  */
 typedef void tal_t;
 
-/**
- * TAL_TAKE - fake tal_t to indicate function will own arguments.
- *
- * Various functions take a context on which to allocate: if you use
- * TAL_TAKE there instead, it means that the argument(s) are actually
- * tal objects.  The returned value will share the same parent; it may
- * even be the same pointer as the arguments.  The arguments themselves
- * will be reused, freed, or made a child of the return value: they are
- * no longer valid for external use.
- */
-#define TAL_TAKE ((tal_t *)-2L)
-
 /**
  * tal - basic allocator function
  * @ctx: NULL, or tal allocated object to be parent.
@@ -123,10 +111,9 @@ void tal_free(const tal_t *p);
  *
  * This may need to perform an allocation, in which case it may fail; thus
  * it can return NULL, otherwise returns @ptr.
- *
- * Note: weird macro avoids gcc's 'warning: value computed is not used'.
  */
 #if HAVE_STATEMENT_EXPR
+/* Weird macro avoids gcc's 'warning: value computed is not used'. */
 #define tal_steal(ctx, ptr) \
 	({ (tal_typeof(ptr) tal_steal_((ctx),(ptr))); })
 #else
@@ -189,9 +176,9 @@ tal_t *tal_parent(const tal_t *ctx);
 
 /**
  * tal_dup - duplicate an array.
- * @ctx: NULL, or tal allocated object to be parent (or TAL_TAKE).
+ * @ctx: The tal allocated object to be parent of the result (may be NULL).
  * @type: the type (should match type of @p!)
- * @p: the array to copy
+ * @p: the array to copy (or resized & reparented if take())
  * @n: the number of sizeof(type) entries to copy.
  * @extra: the number of extra sizeof(type) entries to allocate.
  */
@@ -203,15 +190,15 @@ tal_t *tal_parent(const tal_t *ctx);
 
 /**
  * tal_strdup - duplicate a string
- * @ctx: NULL, or tal allocated object to be parent (or TAL_TAKE).
- * @p: the string to copy
+ * @ctx: NULL, or tal allocated object to be parent.
+ * @p: the string to copy (can be take()).
  */
 char *tal_strdup(const tal_t *ctx, const char *p);
 
 /**
  * tal_strndup - duplicate a limited amount of a string.
- * @ctx: NULL, or tal allocated object to be parent (or TAL_TAKE).
- * @p: the string to copy
+ * @ctx: NULL, or tal allocated object to be parent.
+ * @p: the string to copy (can be take()).
  * @n: the maximum length to copy.
  *
  * Always gives a nul-terminated string, with strlen() <= @n.
@@ -220,22 +207,16 @@ char *tal_strndup(const tal_t *ctx, const char *p, size_t n);
 
 /**
  * tal_asprintf - allocate a formatted string
- * @ctx: NULL, or tal allocated object to be parent (or TAL_TAKE).
- * @fmt: the printf-style format.
- *
- * If @ctx is TAL_TAKE, @fmt is freed and its parent will be the parent
- * of the return value.
+ * @ctx: NULL, or tal allocated object to be parent.
+ * @fmt: the printf-style format (can be take()).
  */
 char *tal_asprintf(const tal_t *ctx, const char *fmt, ...) PRINTF_FMT(2,3);
 
 /**
  * tal_vasprintf - allocate a formatted string (va_list version)
- * @ctx: NULL, or tal allocated object to be parent (or TAL_TAKE).
- * @fmt: the printf-style format.
+ * @ctx: NULL, or tal allocated object to be parent.
+ * @fmt: the printf-style format (can be take()).
  * @va: the va_list containing the format args.
- *
- * If @ctx is TAL_TAKE, @fmt is freed and its parent will be the parent
- * of the return value.
  */
 char *tal_vasprintf(const tal_t *ctx, const char *fmt, va_list ap)
 	PRINTF_FMT(2,0);

ccan/tal/test/run-overflow.c

@@ -28,57 +28,58 @@ int main(void)
 
 	/* Now try overflow cases for tal_dup. */
 	error_count = 0;
-	pi = origpi = tal_arr(NULL, int, 100);
-	ok1(pi);
+	origpi = tal_arr(NULL, int, 100);
+	ok1(origpi);
 	ok1(error_count == 0);
-	pi = tal_dup(NULL, int, pi, (size_t)-1, 0);
+	pi = tal_dup(NULL, int, origpi, (size_t)-1, 0);
 	ok1(!pi);
 	ok1(error_count == 1);
-	pi = tal_dup(NULL, int, pi, 0, (size_t)-1);
+	pi = tal_dup(NULL, int, origpi, 0, (size_t)-1);
 	ok1(!pi);
 	ok1(error_count == 2);
 
-	pi = tal_dup(NULL, int, pi, (size_t)-1UL / sizeof(int),
+	pi = tal_dup(NULL, int, origpi, (size_t)-1UL / sizeof(int),
 		     (size_t)-1UL / sizeof(int));
 	ok1(!pi);
 	ok1(error_count == 3);
 	/* This will still overflow when tal_hdr is added. */
-	pi = tal_dup(NULL, int, pi, (size_t)-1UL / sizeof(int) / 2,
+	pi = tal_dup(NULL, int, origpi, (size_t)-1UL / sizeof(int) / 2,
 		     (size_t)-1UL / sizeof(int) / 2);
 	ok1(!pi);
 	ok1(error_count == 4);
+	ok1(tal_first(NULL) == origpi && !tal_next(NULL, origpi));
+	tal_free(origpi);
 
-	/* Now, check that with TAL_TAKE we free old one on failure. */
-	pi = tal_arr(NULL, int, 100);
+	/* Now, check that with taltk() we free old one on failure. */
+	origpi = tal_arr(NULL, int, 100);
 	error_count = 0;
-	pi = tal_dup(TAL_TAKE, int, pi, (size_t)-1, 0);
+	pi = tal_dup(NULL, int, take(origpi), (size_t)-1, 0);
 	ok1(!pi);
 	ok1(error_count == 1);
-	ok1(tal_first(NULL) == origpi && !tal_next(NULL, origpi));
 
-	pi = tal_arr(NULL, int, 100);
+	origpi = tal_arr(NULL, int, 100);
 	error_count = 0;
-	pi = tal_dup(TAL_TAKE, int, pi, 0, (size_t)-1);
+	pi = tal_dup(NULL, int, take(origpi), 0, (size_t)-1);
 	ok1(!pi);
 	ok1(error_count == 1);
-	ok1(tal_first(NULL) == origpi && !tal_next(NULL, origpi));
+	ok1(!tal_first(NULL));
 
-	pi = tal_arr(NULL, int, 100);
+	origpi = tal_arr(NULL, int, 100);
 	error_count = 0;
-	pi = tal_dup(TAL_TAKE, int, pi, (size_t)-1UL / sizeof(int),
+	pi = tal_dup(NULL, int, take(origpi), (size_t)-1UL / sizeof(int),
 		     (size_t)-1UL / sizeof(int));
 	ok1(!pi);
 	ok1(error_count == 1);
-	ok1(tal_first(NULL) == origpi && !tal_next(NULL, origpi));
+	ok1(!tal_first(NULL));
 
-	pi = tal_arr(NULL, int, 100);
+	origpi = tal_arr(NULL, int, 100);
 	error_count = 0;
 	/* This will still overflow when tal_hdr is added. */
-	pi = tal_dup(TAL_TAKE, int, pi, (size_t)-1UL / sizeof(int) / 2,
+	pi = tal_dup(NULL, int, take(origpi), (size_t)-1UL / sizeof(int) / 2,
 		     (size_t)-1UL / sizeof(int) / 2);
 	ok1(!pi);
 	ok1(error_count == 1);
-	ok1(tal_first(NULL) == origpi && !tal_next(NULL, origpi));
+	ok1(!tal_first(NULL));
 
 	return exit_status();
 }

ccan/tal/test/run-take.c

@@ -6,40 +6,54 @@ int main(void)
 {
 	char *parent, *c;
 
-	plan_tests(15);
+	plan_tests(24);
+
+	/* We can take NULL. */
+	ok1(take(NULL) == NULL);
+	ok1(taken(NULL)); /* Undoes take() */
+	ok1(!taken(NULL));
 
 	parent = tal(NULL, char);
 	ok1(parent);
 
+	ok1(take(parent) == parent);
+	ok1(taken(parent)); /* Undoes take() */
+	ok1(!taken(parent));
+
 	c = tal_strdup(parent, "hello");
 
-	c = tal_strdup(TAL_TAKE, c);
+	c = tal_strdup(parent, take(c));
 	ok1(strcmp(c, "hello") == 0);
 	ok1(tal_parent(c) == parent);
 
-	c = tal_strndup(TAL_TAKE, c, 5);
+	c = tal_strndup(parent, take(c), 5);
 	ok1(strcmp(c, "hello") == 0);
 	ok1(tal_parent(c) == parent);
 
-	c = tal_strndup(TAL_TAKE, c, 3);
+	c = tal_strndup(parent, take(c), 3);
 	ok1(strcmp(c, "hel") == 0);
 	ok1(tal_parent(c) == parent);
 
-	c = tal_dup(TAL_TAKE, char, c, 1, 0);
+	c = tal_dup(parent, char, take(c), 1, 0);
 	ok1(c[0] == 'h');
 	ok1(tal_parent(c) == parent);
 
-	c = tal_dup(TAL_TAKE, char, c, 1, 2);
+	c = tal_dup(parent, char, take(c), 1, 2);
 	ok1(c[0] == 'h');
 	strcpy(c, "hi");
 	ok1(tal_parent(c) == parent);
 
+	/* dup must reparent child. */
+	c = tal_dup(NULL, char, take(c), 1, 0);
+	ok1(c[0] == 'h');
+	ok1(tal_parent(c) == NULL);
+
 	/* No leftover allocations. */
 	tal_free(c);
 	ok1(tal_first(parent) == NULL);
 
 	c = tal_strdup(parent, "hello %s");
-	c = tal_asprintf(TAL_TAKE, c, "there");
+	c = tal_asprintf(parent, take(c), "there");
 	ok1(strcmp(c, "hello there") == 0);
 	ok1(tal_parent(c) == parent);
 	/* No leftover allocations. */
@@ -47,6 +61,7 @@ int main(void)
 	ok1(tal_first(parent) == NULL);
 
 	tal_free(parent);
+	ok1(!taken_any());
 
 	return exit_status();
 }

ccan/tal/test/run-test-backend.c

@@ -12,7 +12,8 @@ static void *my_alloc(size_t len)
 
 static void my_free(void *p)
 {
-	return free((char *)p - 16);
+	if (p)
+		free((char *)p - 16);
 }
 
 static void *my_realloc(void *old, size_t new_size)