* talloc leak debugging
@ 2012-12-17 3:24 david
2012-12-17 3:24 ` [PATCH 1/3] CLI: add talloc leak report, controlled by an environment variable david
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: david @ 2012-12-17 3:24 UTC (permalink / raw)
To: notmuch
I was playing a bit with adding talloc leak debugging to notmuch
(since it seems to be essentially free, performance-wise). I got a bit
discouraged about modifying the argument handling in notmuch.c, so I
hacked it in via an environment variable.
^ permalink raw reply [flat|nested] 9+ messages in thread* [PATCH 1/3] CLI: add talloc leak report, controlled by an environment variable. 2012-12-17 3:24 talloc leak debugging david @ 2012-12-17 3:24 ` david 2012-12-20 15:40 ` Tomi Ollila 2012-12-21 1:55 ` Austin Clements 2012-12-17 3:24 ` [PATCH 2/3] util: add xtalloc.[ch] david 2012-12-17 3:24 ` [PATCH 3/3] notmuch-restore: use xtalloc version of strndup david 2 siblings, 2 replies; 9+ messages in thread From: david @ 2012-12-17 3:24 UTC (permalink / raw) To: notmuch; +Cc: David Bremner From: David Bremner <bremner@debian.org> The argument handling in notmuch.c seems due for an overhaul, but until then use an environment variable to specify a location to write the talloc leak report to. This is only enabled for the (interesting) case where some notmuch subcommand is invoked. --- notmuch.c | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/notmuch.c b/notmuch.c index 9516dfb..fb49c5a 100644 --- a/notmuch.c +++ b/notmuch.c @@ -322,8 +322,20 @@ main (int argc, char *argv[]) for (i = 0; i < ARRAY_SIZE (commands); i++) { command = &commands[i]; - if (strcmp (argv[1], command->name) == 0) - return (command->function) (local, argc - 1, &argv[1]); + if (strcmp (argv[1], command->name) == 0) { + int ret; + char *talloc_report; + + ret = (command->function) (local, argc - 1, &argv[1]); + + talloc_report=getenv ("NOTMUCH_TALLOC_REPORT"); + if (talloc_report && strcmp(talloc_report, "") != 0) { + FILE *report = fopen (talloc_report, "w"); + talloc_report_full (NULL, report); + } + + return ret; + } } fprintf (stderr, "Error: Unknown command '%s' (see \"notmuch help\")\n", -- 1.7.10.4 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/3] CLI: add talloc leak report, controlled by an environment variable. 2012-12-17 3:24 ` [PATCH 1/3] CLI: add talloc leak report, controlled by an environment variable david @ 2012-12-20 15:40 ` Tomi Ollila 2012-12-20 16:58 ` David Bremner 2012-12-21 1:55 ` Austin Clements 1 sibling, 1 reply; 9+ messages in thread From: Tomi Ollila @ 2012-12-20 15:40 UTC (permalink / raw) To: david, notmuch; +Cc: David Bremner On Mon, Dec 17 2012, david@tethera.net wrote: > From: David Bremner <bremner@debian.org> > > The argument handling in notmuch.c seems due for an overhaul, but > until then use an environment variable to specify a location to write > the talloc leak report to. This is only enabled for the (interesting) > case where some notmuch subcommand is invoked. > --- I'd still suggest to have similar command-line interface option(s) as samba4 use: --leak-report and/or --leak-report-full (search --leak-r in talloc(3) namual page) Tomi > notmuch.c | 16 ++++++++++++++-- > 1 file changed, 14 insertions(+), 2 deletions(-) > > diff --git a/notmuch.c b/notmuch.c > index 9516dfb..fb49c5a 100644 > --- a/notmuch.c > +++ b/notmuch.c > @@ -322,8 +322,20 @@ main (int argc, char *argv[]) > for (i = 0; i < ARRAY_SIZE (commands); i++) { > command = &commands[i]; > > - if (strcmp (argv[1], command->name) == 0) > - return (command->function) (local, argc - 1, &argv[1]); > + if (strcmp (argv[1], command->name) == 0) { > + int ret; > + char *talloc_report; > + > + ret = (command->function) (local, argc - 1, &argv[1]); > + > + talloc_report=getenv ("NOTMUCH_TALLOC_REPORT"); > + if (talloc_report && strcmp(talloc_report, "") != 0) { > + FILE *report = fopen (talloc_report, "w"); > + talloc_report_full (NULL, report); > + } > + > + return ret; > + } > } > > fprintf (stderr, "Error: Unknown command '%s' (see \"notmuch help\")\n", > -- > 1.7.10.4 > > _______________________________________________ > notmuch mailing list > notmuch@notmuchmail.org > http://notmuchmail.org/mailman/listinfo/notmuch ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/3] CLI: add talloc leak report, controlled by an environment variable. 2012-12-20 15:40 ` Tomi Ollila @ 2012-12-20 16:58 ` David Bremner 0 siblings, 0 replies; 9+ messages in thread From: David Bremner @ 2012-12-20 16:58 UTC (permalink / raw) To: Tomi Ollila, notmuch Tomi Ollila <tomi.ollila@iki.fi> writes: > On Mon, Dec 17 2012, david@tethera.net wrote: > >> From: David Bremner <bremner@debian.org> >> >> The argument handling in notmuch.c seems due for an overhaul, but >> until then use an environment variable to specify a location to write >> the talloc leak report to. This is only enabled for the (interesting) >> case where some notmuch subcommand is invoked. >> --- > > I'd still suggest to have similar command-line interface option(s) as > samba4 use: --leak-report and/or --leak-report-full > > (search --leak-r in talloc(3) namual page) > Sure sounds fine, when someone (TM) redoes the top level argument handling. I think it would make sense to ditch the alias handling completely; both current aliases are deprecated for a long time. d ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/3] CLI: add talloc leak report, controlled by an environment variable. 2012-12-17 3:24 ` [PATCH 1/3] CLI: add talloc leak report, controlled by an environment variable david 2012-12-20 15:40 ` Tomi Ollila @ 2012-12-21 1:55 ` Austin Clements 1 sibling, 0 replies; 9+ messages in thread From: Austin Clements @ 2012-12-21 1:55 UTC (permalink / raw) To: david; +Cc: notmuch, David Bremner Quoth david@tethera.net on Dec 16 at 11:24 pm: > From: David Bremner <bremner@debian.org> > > The argument handling in notmuch.c seems due for an overhaul, but > until then use an environment variable to specify a location to write > the talloc leak report to. This is only enabled for the (interesting) > case where some notmuch subcommand is invoked. > --- > notmuch.c | 16 ++++++++++++++-- > 1 file changed, 14 insertions(+), 2 deletions(-) > > diff --git a/notmuch.c b/notmuch.c > index 9516dfb..fb49c5a 100644 > --- a/notmuch.c > +++ b/notmuch.c > @@ -322,8 +322,20 @@ main (int argc, char *argv[]) > for (i = 0; i < ARRAY_SIZE (commands); i++) { > command = &commands[i]; > > - if (strcmp (argv[1], command->name) == 0) > - return (command->function) (local, argc - 1, &argv[1]); > + if (strcmp (argv[1], command->name) == 0) { > + int ret; > + char *talloc_report; > + > + ret = (command->function) (local, argc - 1, &argv[1]); > + > + talloc_report=getenv ("NOTMUCH_TALLOC_REPORT"); Missing spaces around =. I think hacking this in as an environment variable is fine, but maybe there should be a comment here saying that it would be better to follow Samba's talloc command-line argument conventions? > + if (talloc_report && strcmp(talloc_report, "") != 0) { Missing space before paren. > + FILE *report = fopen (talloc_report, "w"); > + talloc_report_full (NULL, report); Maybe I'm missing something here, but don't you have to call talloc_enable_leak_report_full before the first use of talloc to get a complete leak report? > + } > + > + return ret; > + } > } > > fprintf (stderr, "Error: Unknown command '%s' (see \"notmuch help\")\n", ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 2/3] util: add xtalloc.[ch] 2012-12-17 3:24 talloc leak debugging david 2012-12-17 3:24 ` [PATCH 1/3] CLI: add talloc leak report, controlled by an environment variable david @ 2012-12-17 3:24 ` david 2012-12-20 15:42 ` Tomi Ollila 2012-12-21 2:03 ` Austin Clements 2012-12-17 3:24 ` [PATCH 3/3] notmuch-restore: use xtalloc version of strndup david 2 siblings, 2 replies; 9+ messages in thread From: david @ 2012-12-17 3:24 UTC (permalink / raw) To: notmuch; +Cc: David Bremner From: David Bremner <bremner@debian.org> These are intended to be simple wrappers to provide slightly better debugging information than what talloc currently provides natively. --- notmuch-client.h | 2 +- util/Makefile.local | 2 +- util/xtalloc.c | 15 +++++++++++++++ util/xtalloc.h | 18 ++++++++++++++++++ 4 files changed, 35 insertions(+), 2 deletions(-) create mode 100644 util/xtalloc.c create mode 100644 util/xtalloc.h diff --git a/notmuch-client.h b/notmuch-client.h index d7b352e..60be030 100644 --- a/notmuch-client.h +++ b/notmuch-client.h @@ -58,7 +58,7 @@ typedef GMimeCipherContext notmuch_crypto_context_t; #include <errno.h> #include <signal.h> -#include <talloc.h> +#include "xtalloc.h" #define unused(x) x __attribute__ ((unused)) diff --git a/util/Makefile.local b/util/Makefile.local index a11e35b..8a62c00 100644 --- a/util/Makefile.local +++ b/util/Makefile.local @@ -4,7 +4,7 @@ dir := util extra_cflags += -I$(srcdir)/$(dir) libutil_c_srcs := $(dir)/xutil.c $(dir)/error_util.c $(dir)/hex-escape.c \ - $(dir)/string-util.c + $(dir)/string-util.c $(dir)/xtalloc.c libutil_modules := $(libutil_c_srcs:.c=.o) diff --git a/util/xtalloc.c b/util/xtalloc.c new file mode 100644 index 0000000..22834bd --- /dev/null +++ b/util/xtalloc.c @@ -0,0 +1,15 @@ +#include <string.h> +#include "xtalloc.h" + +char * +xtalloc_strndup_named_const (void *ctx, const char *str, + size_t len, const char *name) +{ + char *ptr = talloc_named_const (ctx, len + 1, name); + + if (ptr) { + memcpy (ptr, str, len); + *(ptr + len) = '\0'; + } + return ptr; +} diff --git a/util/xtalloc.h b/util/xtalloc.h new file mode 100644 index 0000000..3cc1179 --- /dev/null +++ b/util/xtalloc.h @@ -0,0 +1,18 @@ +#ifndef _XTALLOC_H +#define _XTALLOC_H + +#include <talloc.h> + +/* Like talloc_strndup, but take an extra parameter for the internal talloc + * name (for debugging) */ + +char * +xtalloc_strndup_named_const (void *ctx, const char *str, + size_t len, const char *name); + +/* use the __location__ macro from talloc.h to name a string according to its + * source location */ + +#define xtalloc_strndup(ctx, str, len) xtalloc_strndup_named_const (ctx, str, len, __location__) + +#endif -- 1.7.10.4 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 2/3] util: add xtalloc.[ch] 2012-12-17 3:24 ` [PATCH 2/3] util: add xtalloc.[ch] david @ 2012-12-20 15:42 ` Tomi Ollila 2012-12-21 2:03 ` Austin Clements 1 sibling, 0 replies; 9+ messages in thread From: Tomi Ollila @ 2012-12-20 15:42 UTC (permalink / raw) To: david, notmuch; +Cc: David Bremner On Mon, Dec 17 2012, david@tethera.net wrote: > From: David Bremner <bremner@debian.org> > > These are intended to be simple wrappers to provide slightly better > debugging information than what talloc currently provides natively. > --- AFAIC it's been customary to have memory allocation functions starting with 'x' to abort execution if memory allocation fails. See util/xutil.c in notmuch source for reference. IMHO it would be confusing to use the 'x' prefix for some other use. Tomi > notmuch-client.h | 2 +- > util/Makefile.local | 2 +- > util/xtalloc.c | 15 +++++++++++++++ > util/xtalloc.h | 18 ++++++++++++++++++ > 4 files changed, 35 insertions(+), 2 deletions(-) > create mode 100644 util/xtalloc.c > create mode 100644 util/xtalloc.h > > diff --git a/notmuch-client.h b/notmuch-client.h > index d7b352e..60be030 100644 > --- a/notmuch-client.h > +++ b/notmuch-client.h > @@ -58,7 +58,7 @@ typedef GMimeCipherContext notmuch_crypto_context_t; > #include <errno.h> > #include <signal.h> > > -#include <talloc.h> > +#include "xtalloc.h" > > #define unused(x) x __attribute__ ((unused)) > > diff --git a/util/Makefile.local b/util/Makefile.local > index a11e35b..8a62c00 100644 > --- a/util/Makefile.local > +++ b/util/Makefile.local > @@ -4,7 +4,7 @@ dir := util > extra_cflags += -I$(srcdir)/$(dir) > > libutil_c_srcs := $(dir)/xutil.c $(dir)/error_util.c $(dir)/hex-escape.c \ > - $(dir)/string-util.c > + $(dir)/string-util.c $(dir)/xtalloc.c > > libutil_modules := $(libutil_c_srcs:.c=.o) > > diff --git a/util/xtalloc.c b/util/xtalloc.c > new file mode 100644 > index 0000000..22834bd > --- /dev/null > +++ b/util/xtalloc.c > @@ -0,0 +1,15 @@ > +#include <string.h> > +#include "xtalloc.h" > + > +char * > +xtalloc_strndup_named_const (void *ctx, const char *str, > + size_t len, const char *name) > +{ > + char *ptr = talloc_named_const (ctx, len + 1, name); > + > + if (ptr) { > + memcpy (ptr, str, len); > + *(ptr + len) = '\0'; > + } > + return ptr; > +} > diff --git a/util/xtalloc.h b/util/xtalloc.h > new file mode 100644 > index 0000000..3cc1179 > --- /dev/null > +++ b/util/xtalloc.h > @@ -0,0 +1,18 @@ > +#ifndef _XTALLOC_H > +#define _XTALLOC_H > + > +#include <talloc.h> > + > +/* Like talloc_strndup, but take an extra parameter for the internal talloc > + * name (for debugging) */ > + > +char * > +xtalloc_strndup_named_const (void *ctx, const char *str, > + size_t len, const char *name); > + > +/* use the __location__ macro from talloc.h to name a string according to its > + * source location */ > + > +#define xtalloc_strndup(ctx, str, len) xtalloc_strndup_named_const (ctx, str, len, __location__) > + > +#endif > -- > 1.7.10.4 > > _______________________________________________ > notmuch mailing list > notmuch@notmuchmail.org > http://notmuchmail.org/mailman/listinfo/notmuch ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/3] util: add xtalloc.[ch] 2012-12-17 3:24 ` [PATCH 2/3] util: add xtalloc.[ch] david 2012-12-20 15:42 ` Tomi Ollila @ 2012-12-21 2:03 ` Austin Clements 1 sibling, 0 replies; 9+ messages in thread From: Austin Clements @ 2012-12-21 2:03 UTC (permalink / raw) To: david; +Cc: notmuch, David Bremner Quoth david@tethera.net on Dec 16 at 11:24 pm: > From: David Bremner <bremner@debian.org> > > These are intended to be simple wrappers to provide slightly better > debugging information than what talloc currently provides natively. > --- > notmuch-client.h | 2 +- > util/Makefile.local | 2 +- > util/xtalloc.c | 15 +++++++++++++++ > util/xtalloc.h | 18 ++++++++++++++++++ > 4 files changed, 35 insertions(+), 2 deletions(-) > create mode 100644 util/xtalloc.c > create mode 100644 util/xtalloc.h > > diff --git a/notmuch-client.h b/notmuch-client.h > index d7b352e..60be030 100644 > --- a/notmuch-client.h > +++ b/notmuch-client.h > @@ -58,7 +58,7 @@ typedef GMimeCipherContext notmuch_crypto_context_t; > #include <errno.h> > #include <signal.h> > > -#include <talloc.h> > +#include "xtalloc.h" > > #define unused(x) x __attribute__ ((unused)) > > diff --git a/util/Makefile.local b/util/Makefile.local > index a11e35b..8a62c00 100644 > --- a/util/Makefile.local > +++ b/util/Makefile.local > @@ -4,7 +4,7 @@ dir := util > extra_cflags += -I$(srcdir)/$(dir) > > libutil_c_srcs := $(dir)/xutil.c $(dir)/error_util.c $(dir)/hex-escape.c \ > - $(dir)/string-util.c > + $(dir)/string-util.c $(dir)/xtalloc.c > > libutil_modules := $(libutil_c_srcs:.c=.o) > > diff --git a/util/xtalloc.c b/util/xtalloc.c > new file mode 100644 > index 0000000..22834bd > --- /dev/null > +++ b/util/xtalloc.c > @@ -0,0 +1,15 @@ > +#include <string.h> > +#include "xtalloc.h" > + > +char * > +xtalloc_strndup_named_const (void *ctx, const char *str, > + size_t len, const char *name) > +{ > + char *ptr = talloc_named_const (ctx, len + 1, name); > + > + if (ptr) { > + memcpy (ptr, str, len); This isn't safe. If the string at ptr is actually shorter than len, this may read past allocated memory and crash. Maybe this should just call talloc_strndup and talloc_set_name_const? > + *(ptr + len) = '\0'; > + } > + return ptr; > +} > diff --git a/util/xtalloc.h b/util/xtalloc.h > new file mode 100644 > index 0000000..3cc1179 > --- /dev/null > +++ b/util/xtalloc.h > @@ -0,0 +1,18 @@ > +#ifndef _XTALLOC_H > +#define _XTALLOC_H > + > +#include <talloc.h> > + > +/* Like talloc_strndup, but take an extra parameter for the internal talloc > + * name (for debugging) */ > + > +char * > +xtalloc_strndup_named_const (void *ctx, const char *str, > + size_t len, const char *name); I agree with Tomi that these shouldn't be named with 'x'. For this one, it seems fine to simply drop the 'x', since the name is fully descriptive of what it does. > + > +/* use the __location__ macro from talloc.h to name a string according to its > + * source location */ > + > +#define xtalloc_strndup(ctx, str, len) xtalloc_strndup_named_const (ctx, str, len, __location__) For this, what about talloc_strndup_debug? > + > +#endif ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 3/3] notmuch-restore: use xtalloc version of strndup 2012-12-17 3:24 talloc leak debugging david 2012-12-17 3:24 ` [PATCH 1/3] CLI: add talloc leak report, controlled by an environment variable david 2012-12-17 3:24 ` [PATCH 2/3] util: add xtalloc.[ch] david @ 2012-12-17 3:24 ` david 2 siblings, 0 replies; 9+ messages in thread From: david @ 2012-12-17 3:24 UTC (permalink / raw) To: notmuch; +Cc: David Bremner From: David Bremner <bremner@debian.org> This gives line numbers for better debugging. --- notmuch-restore.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/notmuch-restore.c b/notmuch-restore.c index 40596a8..0cc9c9f 100644 --- a/notmuch-restore.c +++ b/notmuch-restore.c @@ -85,9 +85,10 @@ parse_sup_line (void *ctx, char *line, return 1; } - *query_str = talloc_strndup (ctx, line + match[1].rm_so, - match[1].rm_eo - match[1].rm_so); - file_tags = talloc_strndup (ctx, line + match[2].rm_so, + *query_str = xtalloc_strndup (ctx, line + match[1].rm_so, + match[1].rm_eo - match[1].rm_so); + + file_tags = xtalloc_strndup (ctx, line + match[2].rm_so, match[2].rm_eo - match[2].rm_so); char *tok = file_tags; -- 1.7.10.4 ^ permalink raw reply related [flat|nested] 9+ messages in thread
end of thread, other threads:[~2012-12-21 2:04 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-12-17 3:24 talloc leak debugging david 2012-12-17 3:24 ` [PATCH 1/3] CLI: add talloc leak report, controlled by an environment variable david 2012-12-20 15:40 ` Tomi Ollila 2012-12-20 16:58 ` David Bremner 2012-12-21 1:55 ` Austin Clements 2012-12-17 3:24 ` [PATCH 2/3] util: add xtalloc.[ch] david 2012-12-20 15:42 ` Tomi Ollila 2012-12-21 2:03 ` Austin Clements 2012-12-17 3:24 ` [PATCH 3/3] notmuch-restore: use xtalloc version of strndup david
Code repositories for project(s) associated with this public inbox https://yhetil.org/notmuch.git/ This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).