[ccan] [RFC] tal_stack

[Rusty Russell]

Delio Brignoli <brignoli.delio at gmail.com> writes:
> Hi All,
>
> tal_stack implements a (trivial) stack of tal contexts. Would this be a worthy addition to CCAN? (not necessarily in its current form).

Hi Delio,

        This is cute; I've seen similar used in Samba.  It's
particularly nice because if you miss freeing one frame, it gets freed
by the lower frame, so you can get pretty lazy and still have bounded
leakage.

You are missing a _info file: I would create that, and put your example
in an Example: section there.

Other random advice:
1) You should also document the tal_newframe function (particularly note
   that you're expected to tal_free the result, and that it will free
   any future unfreed frames).  And note that it's not threadsafe.
2) You probably want tal_newframe to be a macro, and hand file and line
   thought to the tal_alloc_ call.  That makes debugging nicer when
   you iterate the tree.
3) Consider whether you want to declare a dummy type 'struct tal_stack'.
   Probably pretty unnecessary since it's quite clear.

Cheers,
Rusty.

> ---
>  ccan/tal/tal_stack.c      | 24 +++++++++++++++++++++++
>  ccan/tal/tal_stack.h      | 50 +++++++++++++++++++++++++++++++++++++++++++++++
>  ccan/tal/test/run-stack.c | 37 +++++++++++++++++++++++++++++++++++
>  3 files changed, 111 insertions(+)
>  create mode 100644 ccan/tal/tal_stack.c
>  create mode 100644 ccan/tal/tal_stack.h
>  create mode 100644 ccan/tal/test/run-stack.c
>
> diff --git a/ccan/tal/tal_stack.c b/ccan/tal/tal_stack.c
> new file mode 100644
> index 0000000..95e24b0
> --- /dev/null
> +++ b/ccan/tal/tal_stack.c
> @@ -0,0 +1,24 @@
> +/* Licensed under BSD-MIT - see LICENSE file for details */
> +
> +#include <ccan/tal/tal_stack.h>
> +#include <assert.h>
> +
> +static tal_t *h = NULL;
> +
> +static void _free_frame(tal_t *o)
> +{
> +	h = tal_parent(o);
> +}
> +
> +tal_t *tal_newframe(void)
> +{
> +	h = tal_alloc_(h, 0, false, NULL);
> +	assert(h != NULL);
> +	tal_add_destructor(h, _free_frame);
> +	return h;
> +}
> +
> +tal_t *tal_curframe(void)
> +{
> +	return h;
> +}
> diff --git a/ccan/tal/tal_stack.h b/ccan/tal/tal_stack.h
> new file mode 100644
> index 0000000..7ea11e5
> --- /dev/null
> +++ b/ccan/tal/tal_stack.h
> @@ -0,0 +1,50 @@
> +/* Licensed under BSD-MIT - see LICENSE file for details */
> +#ifndef CCAN_TAL_STACK_H
> +#define CCAN_TAL_STACK_H
> +/*
> +
> +Implement a stack of tal contexts. A new (empty) context is pushed on top
> +of the stack using tal_newframe and it is popped/freed using tal_free().
> +tal_curframe() can be used to get the stack's top context.
> +
> +tal_stack can be used to implement per-function temporary allocation context
> +to help avoiding memory leaks, but unlike the plain tal approach it does not
> +require the caller to pass a destination context for returning allocated
> +values. Instead, allocated values are moved to the parent context using
> +tal_steal(tal_parent(tmp_ctx), ptr).
> +
> +int funcname(int **out)
> +{
> +	int retval = 0;
> +	tal_t *tmp_ctx = tal_newframe();
> +
> +	int *val = talz(tmp_ctx, int);
> +	assert(val != NULL)
> +
> +	int cnt = libc_call(...);
> +	if (cnt < 0) {
> +		retval = -errno;
> +		goto done;
> +	}
> +
> +	... do something with val ...
> +
> +done:
> +	if (retval >= 0) {
> +		tal_steal(tal_parent(tmp_ctx), val);
> +		if (*out)
> +			*out = val;
> +	} else {
> +		... undo side-effects ...
> +	}
> +	tal_free(tmp_ctx);
> +	return retval;
> +}
> +
> +*/
> +
> +#include <ccan/tal/tal.h>
> +
> +tal_t *tal_newframe(void);
> +tal_t *tal_curframe(void);
> +#endif /* CCAN_TAL_STACK_H */
> diff --git a/ccan/tal/test/run-stack.c b/ccan/tal/test/run-stack.c
> new file mode 100644
> index 0000000..cd6054d
> --- /dev/null
> +++ b/ccan/tal/test/run-stack.c
> @@ -0,0 +1,37 @@
> +#include <ccan/tal/tal.h>
> +#include <ccan/tal/tal.c>
> +#include <ccan/tal/tal_stack.h>
> +#include <ccan/tal/tal_stack.c>
> +#include <ccan/tap/tap.h>
> +
> +int main(void)
> +{
> +	tal_t *parent, *cur;
> +
> +	plan_tests(8);
> +
> +	/* initial frame is NULL */
> +	ok1(tal_curframe() == NULL);
> +
> +	/* create new frame and make sure all is OK */
> +	cur = tal_newframe();
> +	ok1(tal_curframe() == cur);
> +	ok1(tal_parent(cur) == NULL);
> +
> +	/* create another frame */
> +	parent = cur;
> +	cur = tal_newframe();
> +	ok1(tal_curframe() == cur);
> +	ok1(tal_parent(cur) == parent);
> +
> +	/* unwind */
> +	tal_free(cur);
> +	ok1(tal_curframe() == parent);
> +	cur = tal_curframe();
> +	ok1(tal_parent(cur) == NULL);
> +	tal_free(cur);
> +	ok1(tal_curframe() == NULL);
> +
> +	tal_cleanup();
> +	return exit_status();
> +}
> -- 
> 1.9.3 (Apple Git-50)
>
> _______________________________________________
> ccan mailing list
> ccan at lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/ccan