v2 restore leak fix

v2 restore leak fix

[david]

This obsoletes 

     id:1355688997-19164-1-git-send-email-david@tethera.net

Actually the first patch in the series is now an unrelated bug fix, 
since it isn't needed anymore for 2 and 3.

[PATCH 1/3] notmuch-restore: fix return value propagation

[david]

From: David Bremner <bremner@debian.org>

Previously notmuch_restore_command returned 0 if tag_message returned
a non-zero (failure) value. This is wrong, since non-zero status
indicates something mysterious went wrong with retrieving the message,
or applying it.

There was also a failure to check or propagate the return value from
tag_op_list_apply in tag_message.
---
 notmuch-restore.c |   13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/notmuch-restore.c b/notmuch-restore.c
index 40596a8..5a02328 100644
--- a/notmuch-restore.c
+++ b/notmuch-restore.c
@@ -25,6 +25,9 @@
 
 static regex_t regex;
 
+/* Non-zero return indicates an error in retrieving the message,
+ * or in applying the tags.
+ */
 static int
 tag_message (unused (void *ctx),
 	     notmuch_database_t *notmuch,
@@ -48,7 +51,7 @@ tag_message (unused (void *ctx),
     /* In order to detect missing messages, this check/optimization is
      * intentionally done *after* first finding the message. */
     if ((flags & TAG_FLAG_REMOVE_ALL) || tag_op_list_size (tag_ops))
-	tag_op_list_apply (message, tag_ops, flags);
+	ret = tag_op_list_apply (message, tag_ops, flags);
 
     notmuch_message_destroy (message);
 
@@ -231,8 +234,12 @@ notmuch_restore_command (unused (void *ctx), int argc, char *argv[])
 	if (ret > 0)
 	    continue;
 
-	if (ret < 0 || tag_message (ctx, notmuch, query_string,
-				    tag_ops, flags))
+	if (ret < 0)
+	    break;
+
+	ret = tag_message (line_ctx, notmuch, query_string,
+			   tag_ops, flags);
+	if (ret)
 	    break;
 
     }  while ((line_len = getline (&line, &line_size, input)) != -1);
-- 
1.7.10.4

[PATCH 2/3] tag-utils: use the tag_opt_list_t as talloc context, if possible.

[david]

From: David Bremner <bremner@debian.org>

This code is no less correct than the previous version, since it does
not make sense for the array to live longer than the wrapping struct.

By not relying on the context passed into tag_parse_line, we can allow
tag_op_list_t structures to live longer than that context.
---
 notmuch-restore.c |    2 +-
 tag-util.c        |    9 ++++-----
 tag-util.h        |    3 +--
 3 files changed, 6 insertions(+), 8 deletions(-)

diff --git a/notmuch-restore.c b/notmuch-restore.c
index 5a02328..665373f 100644
--- a/notmuch-restore.c
+++ b/notmuch-restore.c
@@ -105,7 +105,7 @@ parse_sup_line (void *ctx, char *line,
 	    tok_len++;
 	}
 
-	if (tag_op_list_append (ctx, tag_ops, tok, FALSE))
+	if (tag_op_list_append (tag_ops, tok, FALSE))
 	    return -1;
     }
 
diff --git a/tag-util.c b/tag-util.c
index eab482f..705b7ba 100644
--- a/tag-util.c
+++ b/tag-util.c
@@ -109,7 +109,7 @@ parse_tag_line (void *ctx, char *line,
 	    goto DONE;
 	}
 
-	if (tag_op_list_append (ctx, tag_ops, tag, remove)) {
+	if (tag_op_list_append (tag_ops, tag, remove)) {
 	    ret = line_error (TAG_PARSE_OUT_OF_MEMORY, line_for_error,
 			      "aborting");
 	    goto DONE;
@@ -294,7 +294,7 @@ tag_op_list_create (void *ctx)
     list->size = TAG_OP_LIST_INITIAL_SIZE;
     list->count = 0;
 
-    list->ops = talloc_array (ctx, tag_operation_t, list->size);
+    list->ops = talloc_array (list, tag_operation_t, list->size);
     if (list->ops == NULL)
 	return NULL;
 
@@ -303,8 +303,7 @@ tag_op_list_create (void *ctx)
 
 
 int
-tag_op_list_append (void *ctx,
-		    tag_op_list_t *list,
+tag_op_list_append (tag_op_list_t *list,
 		    const char *tag,
 		    notmuch_bool_t remove)
 {
@@ -314,7 +313,7 @@ tag_op_list_append (void *ctx,
 
     if (list->count == list->size) {
 	list->size *= 2;
-	list->ops = talloc_realloc (ctx, list->ops, tag_operation_t,
+	list->ops = talloc_realloc (list, list->ops, tag_operation_t,
 				    list->size);
 	if (list->ops == NULL) {
 	    fprintf (stderr, "Out of memory.\n");
diff --git a/tag-util.h b/tag-util.h
index 99b0fa0..c07bfde 100644
--- a/tag-util.h
+++ b/tag-util.h
@@ -87,8 +87,7 @@ tag_op_list_create (void *ctx);
  */
 
 int
-tag_op_list_append (void *ctx,
-		    tag_op_list_t *list,
+tag_op_list_append (tag_op_list_t *list,
 		    const char *tag,
 		    notmuch_bool_t remove);
 
-- 
1.7.10.4

[PATCH 3/3] notmuch-restore: allocate a temporary talloc context for each line parsed.

[david]

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 665373f..9ed9b51 100644
--- a/notmuch-restore.c
+++ b/notmuch-restore.c
@@ -125,6 +125,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;
 
@@ -208,10 +209,14 @@ notmuch_restore_command (unused (void *ctx), int argc, char *argv[])
     do {
 	char *query_string;
 
+	if (line_ctx != NULL)
+	    talloc_free (line_ctx);
+
+	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) {
@@ -244,6 +249,9 @@ notmuch_restore_command (unused (void *ctx), int argc, char *argv[])
 
     }  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

Re: [PATCH 2/3] tag-utils: use the tag_opt_list_t as talloc context, if possible.

[Tomi Ollila]

On Mon, Dec 17 2012, david@tethera.net wrote:

> From: David Bremner <bremner@debian.org>
>
> This code is no less correct than the previous version, since it does
> not make sense for the array to live longer than the wrapping struct.
>
> By not relying on the context passed into tag_parse_line, we can allow
> tag_op_list_t structures to live longer than that context.
> ---

The whole patch series looks good to me -- just that this comment
gets a bit over my head...

Tomi


>  notmuch-restore.c |    2 +-
>  tag-util.c        |    9 ++++-----
>  tag-util.h        |    3 +--
>  3 files changed, 6 insertions(+), 8 deletions(-)
>
> diff --git a/notmuch-restore.c b/notmuch-restore.c
> index 5a02328..665373f 100644
> --- a/notmuch-restore.c
> +++ b/notmuch-restore.c
> @@ -105,7 +105,7 @@ parse_sup_line (void *ctx, char *line,
>  	    tok_len++;
>  	}
>  
> -	if (tag_op_list_append (ctx, tag_ops, tok, FALSE))
> +	if (tag_op_list_append (tag_ops, tok, FALSE))
>  	    return -1;
>      }
>  
> diff --git a/tag-util.c b/tag-util.c
> index eab482f..705b7ba 100644
> --- a/tag-util.c
> +++ b/tag-util.c
> @@ -109,7 +109,7 @@ parse_tag_line (void *ctx, char *line,
>  	    goto DONE;
>  	}
>  
> -	if (tag_op_list_append (ctx, tag_ops, tag, remove)) {
> +	if (tag_op_list_append (tag_ops, tag, remove)) {
>  	    ret = line_error (TAG_PARSE_OUT_OF_MEMORY, line_for_error,
>  			      "aborting");
>  	    goto DONE;
> @@ -294,7 +294,7 @@ tag_op_list_create (void *ctx)
>      list->size = TAG_OP_LIST_INITIAL_SIZE;
>      list->count = 0;
>  
> -    list->ops = talloc_array (ctx, tag_operation_t, list->size);
> +    list->ops = talloc_array (list, tag_operation_t, list->size);
>      if (list->ops == NULL)
>  	return NULL;
>  
> @@ -303,8 +303,7 @@ tag_op_list_create (void *ctx)
>  
>  
>  int
> -tag_op_list_append (void *ctx,
> -		    tag_op_list_t *list,
> +tag_op_list_append (tag_op_list_t *list,
>  		    const char *tag,
>  		    notmuch_bool_t remove)
>  {
> @@ -314,7 +313,7 @@ tag_op_list_append (void *ctx,
>  
>      if (list->count == list->size) {
>  	list->size *= 2;
> -	list->ops = talloc_realloc (ctx, list->ops, tag_operation_t,
> +	list->ops = talloc_realloc (list, list->ops, tag_operation_t,
>  				    list->size);
>  	if (list->ops == NULL) {
>  	    fprintf (stderr, "Out of memory.\n");
> diff --git a/tag-util.h b/tag-util.h
> index 99b0fa0..c07bfde 100644
> --- a/tag-util.h
> +++ b/tag-util.h
> @@ -87,8 +87,7 @@ tag_op_list_create (void *ctx);
>   */
>  
>  int
> -tag_op_list_append (void *ctx,
> -		    tag_op_list_t *list,
> +tag_op_list_append (tag_op_list_t *list,
>  		    const char *tag,
>  		    notmuch_bool_t remove);
>  
> -- 
> 1.7.10.4
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: v2 restore leak fix

[Jani Nikula]

On Mon, 17 Dec 2012, david@tethera.net wrote:
> This obsoletes 
>
>      id:1355688997-19164-1-git-send-email-david@tethera.net
>
> Actually the first patch in the series is now an unrelated bug fix, 
> since it isn't needed anymore for 2 and 3.

The series LGTM.

Jani.

Re: v2 restore leak fix

[David Bremner]

Jani Nikula <jani@nikula.org> writes:

> On Mon, 17 Dec 2012, david@tethera.net wrote:
>> This obsoletes 
>>
>>      id:1355688997-19164-1-git-send-email-david@tethera.net
>>
>> Actually the first patch in the series is now an unrelated bug fix, 
>> since it isn't needed anymore for 2 and 3.
>
> The series LGTM.
>
> Jani.

Pushed, with a Tomi approved commit message for patch 2, and one small
fix to patch 1, which used line_ctx before it was defined in a later
patch.

d