From: Jani Nikula <jani@nikula.org>
To: david@tethera.net, notmuch@notmuchmail.org
Subject: Re: [PATCH 3/3] notmuch-restore: allocate a temporary talloc context
	for each line parsed.
In-Reply-To: <1355688997-19164-4-git-send-email-david@tethera.net>
References: <1355688997-19164-1-git-send-email-david@tethera.net>
	<1355688997-19164-4-git-send-email-david@tethera.net>
User-Agent: Notmuch/0.14+138~g7041c56 (http://notmuchmail.org) Emacs/23.4.1
	(i686-pc-linux-gnu)
Date: Mon, 17 Dec 2012 00:41:12 +0200
Message-ID: <87ip81pqlz.fsf@nikula.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Cc: David Bremner <bremner@debian.org>
Precedence: list

On Sun, 16 Dec 2012, david@tethera.net wrote:
> From: David Bremner <bremner@debian.org>
>
> This lets the high level code in notmuch restore be ignorant about
> what the lower level code is doing as far as allocating memory.
> ---
>  notmuch-restore.c |   12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/notmuch-restore.c b/notmuch-restore.c
> index 4b76d83..52e7ecb 100644
> --- a/notmuch-restore.c
> +++ b/notmuch-restore.c
> @@ -122,6 +122,7 @@ notmuch_restore_command (unused (void *ctx), int argc, char *argv[])
>      char *input_file_name = NULL;
>      FILE *input = stdin;
>      char *line = NULL;
> +    void *line_ctx = NULL;
>      size_t line_size;
>      ssize_t line_len;
>  
> @@ -205,10 +206,11 @@ notmuch_restore_command (unused (void *ctx), int argc, char *argv[])
>      do {
>  	char *query_string;

This patch only works on top of the batch tagging series, because
there's still one continue statement in the "id:" prefix check. But you
could make it work for both, *and* keep the slightly more intuitive ret
checking order if you did (yes, the slightly counter-intuitive):

	if (line_ctxt != NULL)
	  talloc_free (line_ctx);

right here, and...

> +	line_ctx = talloc_new (ctx);
>  	if (input_format == DUMP_FORMAT_SUP) {
> -	    ret = parse_sup_line (ctx, line, &query_string, tag_ops);
> +	    ret = parse_sup_line (line_ctx, line, &query_string, tag_ops);
>  	} else {
> -	    ret = parse_tag_line (ctx, line, TAG_FLAG_BE_GENEROUS,
> +	    ret = parse_tag_line (line_ctx, line, TAG_FLAG_BE_GENEROUS,
>  				  &query_string, tag_ops);
>  
>  	    if (ret == 0) {
> @@ -238,8 +240,14 @@ notmuch_restore_command (unused (void *ctx), int argc, char *argv[])
>  		break;
>  	}
>  
> +	talloc_free (line_ctx);
> +	/* setting to NULL is important to avoid a double free */
> +	line_ctx = NULL;

...removed the above lines here.

Otherwise I think you'll need a temporary goto until the batch tagging
series. (I'm fine with that too.)

Overall the series LGTM, and I like how this dodges the query_string
alloc/free problem altogether.

BR,
Jani.

>      }  while ((line_len = getline (&line, &line_size, input)) != -1);
>  
> +    if (line_ctx != NULL)
> +	talloc_free (line_ctx);
> +
>      if (input_format == DUMP_FORMAT_SUP)
>  	regfree (&regex);
>  
> -- 
> 1.7.10.4
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch