tal_stack
authorDelio Brignoli <brignoli.delio@gmail.com>
Sun, 12 Apr 2015 13:27:25 +0000 (15:27 +0200)
committerRusty Russell <rusty@rustcorp.com.au>
Mon, 13 Apr 2015 02:11:00 +0000 (11:41 +0930)
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
ccan/tal/stack/LICENSE [new symlink]
ccan/tal/stack/_info [new file with mode: 0644]
ccan/tal/stack/stack.c [new file with mode: 0644]
ccan/tal/stack/stack.h [new file with mode: 0644]
ccan/tal/stack/test/run-stack.c [new file with mode: 0644]

index cfaa0a9a93c65fced874d66db850bf3ccad2e113..315962cf4c8ec7118d71d5fb87e994b3a3521609 100644 (file)
@@ -100,6 +100,7 @@ MODS_WITH_SRC := antithread \
        tal/grab_file \
        tal/link \
        tal/path \
+       tal/stack \
        tal/str \
        tal/talloc \
        talloc \
diff --git a/ccan/tal/stack/LICENSE b/ccan/tal/stack/LICENSE
new file mode 120000 (symlink)
index 0000000..2b1feca
--- /dev/null
@@ -0,0 +1 @@
+../../../licenses/BSD-MIT
\ No newline at end of file
diff --git a/ccan/tal/stack/_info b/ccan/tal/stack/_info
new file mode 100644 (file)
index 0000000..803a005
--- /dev/null
@@ -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;
+}
diff --git a/ccan/tal/stack/stack.c b/ccan/tal/stack/stack.c
new file mode 100644 (file)
index 0000000..9b949e7
--- /dev/null
@@ -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;
+}
diff --git a/ccan/tal/stack/stack.h b/ccan/tal/stack/stack.h
new file mode 100644 (file)
index 0000000..24d909a
--- /dev/null
@@ -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 */
diff --git a/ccan/tal/stack/test/run-stack.c b/ccan/tal/stack/test/run-stack.c
new file mode 100644 (file)
index 0000000..0e5093c
--- /dev/null
@@ -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();
+}