tal_stack

Hi Rusty,

Thanks for reviewing the patch. V2 is attached, see my comments below.

> On 31 Mar 2015, at 02:36, Rusty Russell <rusty@rustcorp.com.au> wrote:
>
> Delio Brignoli <brignoli.delio@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).

[…]

>        This is cute; I’ve seen similar used in Samba.  It's

Indeed, it was inspired by talloc_stack.h ;-)

[…]

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

I moved the module and tests under can/tal/stack and added a LICENSE and _info.

> 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.

Done.

> 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.

Done. The macro is calling a tal_newframe_() function because I’d rather not make the module’s stack variable ‘public’.

> 3) Consider whether you want to declare a dummy type 'struct tal_stack'.
>   Probably pretty unnecessary since it’s quite clear.

Skipped this one. We can declare it later if we change our minds.

Thanks
—
Delio

From c2ceb9258d97b0dcb72e7b6986cfd2bd394b254e Mon Sep 17 00:00:00 2001
From: Delio Brignoli <dbrignoli@audioscience.com>
Date: Sun, 15 Mar 2015 13:26:40 +0100
Subject: [PATCH] tal_stack: new module - V2

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

Makefile-ccan

@@ -100,6 +100,7 @@ MODS_WITH_SRC := antithread \
 	tal/grab_file \
 	tal/link \
 	tal/path \
+	tal/stack \
 	tal/str \
 	tal/talloc \
 	talloc \

ccan/tal/stack/LICENSE

@@ -0,0 +1 @@
+../../../licenses/BSD-MIT

ccan/tal/stack/_info

@@ -0,0 +1,68 @@
+#include "config.h"
+#include <stdio.h>
+#include <string.h>
+
+/**
+ * tal/stack - stack of tal contextes (inspired by talloc_stack)
+ *
+ * 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 mitigating 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).
+ *
+ * Example:
+ *	#include <assert.h>
+ *	#include <ccan/tal/stack/stack.h>
+ *
+ *	static int *do_work(void)
+ *	{
+ *		int *retval = NULL;
+ *		tal_t *tmp_ctx = tal_newframe();
+ *
+ *		int *val = talz(tmp_ctx, int);
+ *		assert(val != NULL);
+ *
+ *		// ... do something with val ...
+ *
+ *		if (retval >= 0) {
+ *			// steal to parent cxt so it survives tal_free()
+ *			tal_steal(tal_parent(tmp_ctx), val);
+ *			retval = val;
+ *		}
+ *		tal_free(tmp_ctx);
+ *		return retval;
+ *	}
+ *
+ *	int main(int argc, char *argv[])
+ *	{
+ *		tal_t *tmp_ctx = tal_newframe();
+ *		int *val = do_work();
+ *		if (val) {
+ *			// ... do something with val ...
+ *		}
+ *		// val is eventually freed
+ *		tal_free(tmp_ctx);
+ *		return 0;
+ *	}
+ *
+ * License: BSD-MIT
+ * Author: Delio Brignoli <brignoli.delio@gmail.com>
+ */
+int main(int argc, char *argv[])
+{
+	/* Expect exactly one argument */
+	if (argc != 2)
+		return 1;
+
+	if (strcmp(argv[1], "depends") == 0) {
+		printf("ccan/tal\n");
+		return 0;
+	}
+
+	return 1;
+}

ccan/tal/stack/stack.c

@@ -0,0 +1,24 @@
+/* Licensed under BSD-MIT - see LICENSE file for details */
+
+#include <ccan/tal/stack/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_(const char *label)
+{
+	h = tal_alloc_(h, 0, false, label);
+	assert(h != NULL);
+	tal_add_destructor(h, _free_frame);
+	return h;
+}
+
+tal_t *tal_curframe(void)
+{
+	return h;
+}

ccan/tal/stack/stack.h

@@ -0,0 +1,34 @@
+/* Licensed under BSD-MIT - see LICENSE file for details */
+#ifndef CCAN_TAL_STACK_H
+#define CCAN_TAL_STACK_H
+
+#include <ccan/tal/tal.h>
+
+/**
+ * tal_newframe - allocate and return a new nested tal context
+ *
+ * Allocates and push a new tal context on top of the stack.
+ * The context must be freed using tal_free() which will also pop it
+ * off the stack, which will also free all its nested contextes, if any.
+ *
+ * NOTE: this function is not threadsafe.
+ *
+ * Example:
+ *	tal_t *ctx = tal_newframe();
+ *      // ... do something with ctx ...
+ *	tal_free(ctx);
+ */
+#define tal_newframe(void) tal_newframe_(TAL_LABEL(tal_stack, ""));
+
+tal_t *tal_newframe_(const char *label);
+
+/**
+ * tal_curframe - return the current 'tal_stack frame'
+ *
+ * Returns the context currently on top of the stack. The initial context
+ * (before any tal_newframe() call) is the tal 'NULL' context.
+ *
+ * NOTE: this function is not threadsafe.
+ */
+tal_t *tal_curframe(void);
+#endif /* CCAN_TAL_STACK_H */

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

@@ -0,0 +1,35 @@
+#include <ccan/tal/stack/stack.h>
+#include <ccan/tal/stack/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();
+}