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>
tal/grab_file \
tal/link \
tal/path \
+ tal/stack \
tal/str \
tal/talloc \
talloc \
--- /dev/null
+../../../licenses/BSD-MIT
\ No newline at end of file
--- /dev/null
+#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;
+}
--- /dev/null
+/* 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;
+}
--- /dev/null
+/* 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 */
--- /dev/null
+#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();
+}
projects / ccan / commitdiff