tal: remove TAL_TAKE in favor of ccan/take.
authorRusty Russell <rusty@rustcorp.com.au>
Wed, 21 Nov 2012 23:35:27 +0000 (10:05 +1030)
committerRusty Russell <rusty@rustcorp.com.au>
Wed, 21 Nov 2012 23:35:27 +0000 (10:05 +1030)
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
ccan/tal/tal.c
ccan/tal/tal.h
ccan/tal/test/run-overflow.c
ccan/tal/test/run-take.c
ccan/tal/test/run-test-backend.c

index 9ff6baf68a783dd42b97bd69e2ab4dd248be9761..ca0d18500595fb8b863e1d5416cdb5a856bb1967 100644 (file)
@@ -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;
        }
index 88bcb5ab6135257529eda0e999df024507c0386d..0b5e8e445674720f288819c9d2370e1a80d14c64 100644 (file)
@@ -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;
 }
index b047b3894c95786e71face627907506bb7a34ff9..3a0481455ae1da5356a8905820190d03702d47eb 100644 (file)
  */
 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);
index 6c4ccff4379dc97d4be669d8fe2c5224086c5618..989fcd2aba2eee5e9339e06abdd3be4eb7549644 100644 (file)
@@ -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();
 }
index 53c1a44a85390e419075268efb157617390be71d..a6e667bbae0875104725d36f676a95083d31d388 100644 (file)
@@ -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();
 }
index 2f9770a00b9c9804520e5f78cdcfcd2e3c120318..4663a5664a0bfedcb93cde5b8094791f2bf863f7 100644 (file)
@@ -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)