From e03d7ecb505cb73ff244708323b0f1a5a0c5cd7a Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Thu, 22 Nov 2012 10:05:27 +1030 Subject: [PATCH] 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 --- ccan/tal/_info | 1 + ccan/tal/tal.c | 41 +++++++++++++++++++++----------- ccan/tal/tal.h | 41 +++++++++----------------------- ccan/tal/test/run-overflow.c | 39 +++++++++++++++--------------- ccan/tal/test/run-take.c | 29 ++++++++++++++++------ ccan/tal/test/run-test-backend.c | 3 ++- 6 files changed, 83 insertions(+), 71 deletions(-) diff --git a/ccan/tal/_info b/ccan/tal/_info index 9ff6baf6..ca0d1850 100644 --- a/ccan/tal/_info +++ b/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; } diff --git a/ccan/tal/tal.c b/ccan/tal/tal.c index 88bcb5ab..0b5e8e44 100644 --- a/ccan/tal/tal.c +++ b/ccan/tal/tal.c @@ -3,6 +3,7 @@ #include #include #include +#include #include #include #include @@ -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; } diff --git a/ccan/tal/tal.h b/ccan/tal/tal.h index b047b389..3a048145 100644 --- a/ccan/tal/tal.h +++ b/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); diff --git a/ccan/tal/test/run-overflow.c b/ccan/tal/test/run-overflow.c index 6c4ccff4..989fcd2a 100644 --- a/ccan/tal/test/run-overflow.c +++ b/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(); } diff --git a/ccan/tal/test/run-take.c b/ccan/tal/test/run-take.c index 53c1a44a..a6e667bb 100644 --- a/ccan/tal/test/run-take.c +++ b/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(); } diff --git a/ccan/tal/test/run-test-backend.c b/ccan/tal/test/run-test-backend.c index 2f9770a0..4663a566 100644 --- a/ccan/tal/test/run-test-backend.c +++ b/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) -- 2.39.2