[PATCH] test: add known_broken test for dumping large stored queries

[PATCH] test: add known_broken test for dumping large stored queries

[David Bremner]

'qsx' reported a bug on #notmuch with notmuch-dump and large stored
queries. This test will pass (on my machine) if the value of `repeat'
is made smaller.
---
 test/T240-dump-restore.sh | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/test/T240-dump-restore.sh b/test/T240-dump-restore.sh
index 0870ff92..374db5c2 100755
--- a/test/T240-dump-restore.sh
+++ b/test/T240-dump-restore.sh
@@ -322,6 +322,19 @@ EOF
 
 test_expect_equal_file EXPECTED OUTPUT
 
+test_begin_subtest 'dumping large queries'
+test_subtest_known_broken
+# This value repeat was found experimentally by binary search. The
+# config value after URL encoding is exactly 4096 bytes, which
+# suggests a buffer size bug.
+repeat=1329
+notmuch config set query.big "$(seq -s' ' $repeat)"
+notmuch dump --include=config > OUTPUT
+notmuch config set query.big ''
+printf "#notmuch-dump batch-tag:3 config\n#@ query.big " > EXPECTED
+seq -s'%20'  $repeat >> EXPECTED
+test_expect_equal_file EXPECTED OUTPUT
+
 test_begin_subtest 'roundtripping random message-ids and tags'
 
     ${TEST_DIRECTORY}/random-corpus --config-path=${NOTMUCH_CONFIG} \
-- 
2.25.1

Re: [PATCH] test: add known_broken test for dumping large stored queries

[David Bremner]

David Bremner <david@tethera.net> writes:

> 'qsx' reported a bug on #notmuch with notmuch-dump and large stored
> queries. This test will pass (on my machine) if the value of `repeat'
> is made smaller.

I believe the underlying problem is explained by this comment in zlib.h

   The number of uncompressed bytes written is limited to 8191, or
   one less than the buffer size given to gzbuffer().  The caller should assure
   that this limit is not exceeded.  If it is exceeded, then gzprintf() will
   return an error (0) with nothing written.

So calling gzbuffer can increase this limit, but you still have to guess
before writing for the first time.

One way of interpreting this is that we should avoid using gzprintf
where we cannot predict the output size.

d

(no subject)

[David Bremner]

Here's a not too ambitious attempt to clean up the error handling on
zlib output. It's bit gross to treat any error reported by zlib as
fatal, but it's a step up from ignoring them, and it's in the client
code, not in the library.

The first patch splits out the first fix of
id:20200410173039.yextmxk4wtgpl4ud@siegel.lan

The last patch is a fix for the bug reported by qsx.

[PATCH 1/5] util/zlib-extra.c: don't pass NULL to gzerror.

[David Bremner]

Although (as of 1.2.11) zlib checks this parameter before writing to
it, the docs don't promise to keep doing so, so be safe.
---
 util/zlib-extra.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/util/zlib-extra.c b/util/zlib-extra.c
index f691cccf..e17a9731 100644
--- a/util/zlib-extra.c
+++ b/util/zlib-extra.c
@@ -79,8 +79,10 @@ gz_getline (void *talloc_ctx, char **bufptr, ssize_t *bytes_read, gzFile stream)
 const char *
 gz_error_string (util_status_t status, gzFile file)
 {
+    int dummy;
+
     if (status == UTIL_GZERROR)
-	return gzerror (file, NULL);
+	return gzerror (file, &dummy);
     else
 	return util_error_string (status);
 }
-- 
2.25.1

[PATCH 2/5] status: add print_status_gzbytes

[David Bremner]

This is in the client code, rather than libnotmuch_util, because it
prints to stderr. Also it in pretends to generate notmuch status
codes.
---
 notmuch-client.h |  8 +++++++-
 status.c         | 14 ++++++++++++++
 2 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/notmuch-client.h b/notmuch-client.h
index 89e15ba6..467e1d84 100644
--- a/notmuch-client.h
+++ b/notmuch-client.h
@@ -49,6 +49,7 @@
 #include <errno.h>
 #include <signal.h>
 #include <ctype.h>
+#include <zlib.h>
 
 #include "talloc-extra.h"
 #include "crypto.h"
@@ -469,7 +470,7 @@ notmuch_database_dump (notmuch_database_t *notmuch,
 		       dump_include_t include,
 		       bool gzip_output);
 
-/* If status is non-zero (i.e. error) print appropriate
+/* If status indicates error print appropriate
  * messages to stderr.
  */
 
@@ -491,6 +492,11 @@ print_status_database (const char *loc,
 int
 status_to_exit (notmuch_status_t status);
 
+notmuch_status_t
+print_status_gzbytes (const char *loc,
+		      gzFile file,
+		      int bytes);
+
 #include "command-line-arguments.h"
 
 extern const char *notmuch_requested_db_uuid;
diff --git a/status.c b/status.c
index d0ae47f4..09d82a17 100644
--- a/status.c
+++ b/status.c
@@ -72,3 +72,17 @@ status_to_exit (notmuch_status_t status)
 	return EXIT_FAILURE;
     }
 }
+
+notmuch_status_t
+print_status_gzbytes (const char *loc, gzFile file, int bytes)
+{
+    if (bytes <= 0) {
+	int errnum;
+	const char *errstr = gzerror (file, &errnum);
+	fprintf (stderr, "%s: zlib error %s (%d)\n", loc, errstr, errnum);
+	return NOTMUCH_STATUS_FILE_ERROR;
+    } else {
+	return NOTMUCH_STATUS_SUCCESS;
+    }
+}
+
-- 
2.25.1

[PATCH 3/5] cli/dump: define GZPRINTF macro and use it in place of gzprintf

[David Bremner]

This will at least catch errors, and can be replaced with more
sophisticated error handling where appropriate.
---
 notmuch-client.h |  3 +++
 notmuch-dump.c   | 24 ++++++++++++------------
 2 files changed, 15 insertions(+), 12 deletions(-)

diff --git a/notmuch-client.h b/notmuch-client.h
index 467e1d84..55d4d526 100644
--- a/notmuch-client.h
+++ b/notmuch-client.h
@@ -497,6 +497,9 @@ print_status_gzbytes (const char *loc,
 		      gzFile file,
 		      int bytes);
 
+#define ASSERT_GZBYTES(file, bytes) ( (print_status_gzbytes(__location__, file, bytes)) ?  exit(1) : 0 )
+#define GZPRINTF(file, fmt, ...) ASSERT_GZBYTES(file, gzprintf (file, fmt, ##__VA_ARGS__));
+
 #include "command-line-arguments.h"
 
 extern const char *notmuch_requested_db_uuid;
diff --git a/notmuch-dump.c b/notmuch-dump.c
index 65e02639..ca45d885 100644
--- a/notmuch-dump.c
+++ b/notmuch-dump.c
@@ -42,7 +42,7 @@ database_dump_config (notmuch_database_t *notmuch, gzFile output)
 		     notmuch_config_list_key (list));
 	    goto DONE;
 	}
-	gzprintf (output, "#@ %s", buffer);
+	GZPRINTF (output, "#@ %s", buffer);
 
 	if (hex_encode (notmuch, notmuch_config_list_value (list),
 			&buffer, &buffer_size) != HEX_SUCCESS) {
@@ -51,7 +51,7 @@ database_dump_config (notmuch_database_t *notmuch, gzFile output)
 	    goto DONE;
 	}
 
-	gzprintf (output, " %s\n", buffer);
+	GZPRINTF (output, " %s\n", buffer);
     }
 
     ret = EXIT_SUCCESS;
@@ -71,7 +71,7 @@ print_dump_header (gzFile output, int output_format, int include)
 {
     const char *sep = "";
 
-    gzprintf (output, "#notmuch-dump %s:%d ",
+    GZPRINTF (output, "#notmuch-dump %s:%d ",
 	      (output_format == DUMP_FORMAT_SUP) ? "sup" : "batch-tag",
 	      NOTMUCH_DUMP_VERSION);
 
@@ -80,11 +80,11 @@ print_dump_header (gzFile output, int output_format, int include)
 	sep = ",";
     }
     if (include & DUMP_INCLUDE_PROPERTIES) {
-	gzprintf (output, "%sproperties", sep);
+	GZPRINTF (output, "%sproperties", sep);
 	sep = ",";
     }
     if (include & DUMP_INCLUDE_TAGS) {
-	gzprintf (output, "%stags", sep);
+	GZPRINTF (output, "%stags", sep);
     }
     gzputs (output, "\n");
 }
@@ -115,7 +115,7 @@ dump_properties_message (void *ctx,
 		fprintf (stderr, "Error: failed to hex-encode message-id %s\n", message_id);
 		return 1;
 	    }
-	    gzprintf (output, "#= %s", *buffer_p);
+	    GZPRINTF (output, "#= %s", *buffer_p);
 	    first = false;
 	}
 
@@ -126,18 +126,18 @@ dump_properties_message (void *ctx,
 	    fprintf (stderr, "Error: failed to hex-encode key %s\n", key);
 	    return 1;
 	}
-	gzprintf (output, " %s", *buffer_p);
+	GZPRINTF (output, " %s", *buffer_p);
 
 	if (hex_encode (ctx, val, buffer_p, size_p) != HEX_SUCCESS) {
 	    fprintf (stderr, "Error: failed to hex-encode value %s\n", val);
 	    return 1;
 	}
-	gzprintf (output, "=%s", *buffer_p);
+	GZPRINTF (output, "=%s", *buffer_p);
     }
     notmuch_message_properties_destroy (list);
 
     if (! first)
-	gzprintf (output, "\n", *buffer_p);
+	GZPRINTF (output, "\n", *buffer_p);
 
     return 0;
 }
@@ -165,7 +165,7 @@ dump_tags_message (void *ctx,
     }
 
     if (output_format == DUMP_FORMAT_SUP) {
-	gzprintf (output, "%s (", message_id);
+	GZPRINTF (output, "%s (", message_id);
     }
 
     for (notmuch_tags_t *tags = notmuch_message_get_tags (message);
@@ -187,7 +187,7 @@ dump_tags_message (void *ctx,
 			 tag_str);
 		return EXIT_FAILURE;
 	    }
-	    gzprintf (output, "+%s", *buffer_p);
+	    GZPRINTF (output, "+%s", *buffer_p);
 	}
     }
 
@@ -200,7 +200,7 @@ dump_tags_message (void *ctx,
 		     message_id, strerror (errno));
 	    return EXIT_FAILURE;
 	}
-	gzprintf (output, " -- %s\n", *buffer_p);
+	GZPRINTF (output, " -- %s\n", *buffer_p);
     }
     return EXIT_SUCCESS;
 }
-- 
2.25.1

[PATCH 4/5] cli/dump: define GZPUTS and use it in notmuch-dump

[David Bremner]

Similarly to GZPRINTF, this is a drop in replacement that can be
improved where needd.
---
 notmuch-client.h |  1 +
 notmuch-dump.c   | 10 +++++-----
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/notmuch-client.h b/notmuch-client.h
index 55d4d526..7efcb06f 100644
--- a/notmuch-client.h
+++ b/notmuch-client.h
@@ -499,6 +499,7 @@ print_status_gzbytes (const char *loc,
 
 #define ASSERT_GZBYTES(file, bytes) ( (print_status_gzbytes(__location__, file, bytes)) ?  exit(1) : 0 )
 #define GZPRINTF(file, fmt, ...) ASSERT_GZBYTES(file, gzprintf (file, fmt, ##__VA_ARGS__));
+#define GZPUTS(file, str) ASSERT_GZBYTES(file, gzputs (file, str));
 
 #include "command-line-arguments.h"
 
diff --git a/notmuch-dump.c b/notmuch-dump.c
index ca45d885..fb322237 100644
--- a/notmuch-dump.c
+++ b/notmuch-dump.c
@@ -76,7 +76,7 @@ print_dump_header (gzFile output, int output_format, int include)
 	      NOTMUCH_DUMP_VERSION);
 
     if (include & DUMP_INCLUDE_CONFIG) {
-	gzputs (output, "config");
+	GZPUTS (output, "config");
 	sep = ",";
     }
     if (include & DUMP_INCLUDE_PROPERTIES) {
@@ -86,7 +86,7 @@ print_dump_header (gzFile output, int output_format, int include)
     if (include & DUMP_INCLUDE_TAGS) {
 	GZPRINTF (output, "%stags", sep);
     }
-    gzputs (output, "\n");
+    GZPUTS (output, "\n");
 }
 
 static int
@@ -174,12 +174,12 @@ dump_tags_message (void *ctx,
 	const char *tag_str = notmuch_tags_get (tags);
 
 	if (! first)
-	    gzputs (output, " ");
+	    GZPUTS (output, " ");
 
 	first = 0;
 
 	if (output_format == DUMP_FORMAT_SUP) {
-	    gzputs (output, tag_str);
+	    GZPUTS (output, tag_str);
 	} else {
 	    if (hex_encode (ctx, tag_str,
 			    buffer_p, size_p) != HEX_SUCCESS) {
@@ -192,7 +192,7 @@ dump_tags_message (void *ctx,
     }
 
     if (output_format == DUMP_FORMAT_SUP) {
-	gzputs (output, ")\n");
+	GZPUTS (output, ")\n");
     } else {
 	if (make_boolean_term (ctx, "id", message_id,
 			       buffer_p, size_p)) {
-- 
2.25.1

[PATCH 5/5] cli/dump: replace use of gzprintf with gzputs for config values

[David Bremner]

These can be large, and hit buffer limitations of gzprintf.
---
 notmuch-dump.c            | 4 +++-
 test/T240-dump-restore.sh | 1 -
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/notmuch-dump.c b/notmuch-dump.c
index fb322237..23d7d20a 100644
--- a/notmuch-dump.c
+++ b/notmuch-dump.c
@@ -51,7 +51,9 @@ database_dump_config (notmuch_database_t *notmuch, gzFile output)
 	    goto DONE;
 	}
 
-	GZPRINTF (output, " %s\n", buffer);
+	GZPUTS (output, " ");
+	GZPUTS (output, buffer);
+	GZPUTS (output, "\n");
     }
 
     ret = EXIT_SUCCESS;
diff --git a/test/T240-dump-restore.sh b/test/T240-dump-restore.sh
index 374db5c2..3b6ab8fd 100755
--- a/test/T240-dump-restore.sh
+++ b/test/T240-dump-restore.sh
@@ -323,7 +323,6 @@ EOF
 test_expect_equal_file EXPECTED OUTPUT
 
 test_begin_subtest 'dumping large queries'
-test_subtest_known_broken
 # This value repeat was found experimentally by binary search. The
 # config value after URL encoding is exactly 4096 bytes, which
 # suggests a buffer size bug.
-- 
2.25.1

Re: [PATCH] test: add known_broken test for dumping large stored queries

[Tomi Ollila]

On Sun, Apr 12 2020, David Bremner wrote:

> 'qsx' reported a bug on #notmuch with notmuch-dump and large stored
> queries. This test will pass (on my machine) if the value of `repeat'
> is made smaller.
> ---
>  test/T240-dump-restore.sh | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
>
> diff --git a/test/T240-dump-restore.sh b/test/T240-dump-restore.sh
> index 0870ff92..374db5c2 100755
> --- a/test/T240-dump-restore.sh
> +++ b/test/T240-dump-restore.sh
> @@ -322,6 +322,19 @@ EOF
>  
>  test_expect_equal_file EXPECTED OUTPUT
>  
> +test_begin_subtest 'dumping large queries'
> +test_subtest_known_broken
> +# This value repeat was found experimentally by binary search. The
> +# config value after URL encoding is exactly 4096 bytes, which
> +# suggests a buffer size bug.
> +repeat=1329
> +notmuch config set query.big "$(seq -s' ' $repeat)"
> +notmuch dump --include=config > OUTPUT
> +notmuch config set query.big ''
> +printf "#notmuch-dump batch-tag:3 config\n#@ query.big " > EXPECTED

> +seq -s'%20'  $repeat >> EXPECTED

Extra space, otherwise tolerable =D

Tomi

> +test_expect_equal_file EXPECTED OUTPUT
> +
>  test_begin_subtest 'roundtripping random message-ids and tags'
>  
>      ${TEST_DIRECTORY}/random-corpus --config-path=${NOTMUCH_CONFIG} \
> -- 
> 2.25.1

Re: [PATCH 3/5] cli/dump: define GZPRINTF macro and use it in place of gzprintf

[Tomi Ollila]

On Sun, Apr 12 2020, David Bremner wrote:

> This will at least catch errors, and can be replaced with more
> sophisticated error handling where appropriate.
> ---
>  notmuch-client.h |  3 +++
>  notmuch-dump.c   | 24 ++++++++++++------------
>  2 files changed, 15 insertions(+), 12 deletions(-)
>
> diff --git a/notmuch-client.h b/notmuch-client.h
> index 467e1d84..55d4d526 100644
> --- a/notmuch-client.h
> +++ b/notmuch-client.h
> @@ -497,6 +497,9 @@ print_status_gzbytes (const char *loc,
>  		      gzFile file,
>  		      int bytes);
>  
> +#define ASSERT_GZBYTES(file, bytes) ( (print_status_gzbytes(__location__, file, bytes)) ?  exit(1) : 0 )

__location__ ?? never seen such a (preprocessor?) definition. could not
find any reference by search...

there are also some suspiciously looking like inconsistent spaces in that
macro above

rest of the series (before and after) look good 

Tomi

Re: [PATCH] test: add known_broken test for dumping large stored queries

[Thomas Schneider]

David Bremner <david@tethera.net> writes:

> 'qsx' reported a bug on #notmuch with notmuch-dump and large stored
> queries. This test will pass (on my machine) if the value of `repeat'
> is made smaller.

Feel free to add 'Reported-By: Thomas Schneider <qsx@chaotikum.eu>', if
you like.

	--Thomas

Re: [PATCH 3/5] cli/dump: define GZPRINTF macro and use it in place of gzprintf

[David Bremner]

Tomi Ollila <tomi.ollila@iki.fi> writes:

> __location__ ?? never seen such a (preprocessor?) definition. could not
> find any reference by search...

I have to admit, it's just cargo culted from util/error_util.h

>
> there are also some suspiciously looking like inconsistent spaces in that
> macro above

OK, I'll have a look, thanks.

>
> rest of the series (before and after) look good 
>
> Tomi

Re: [PATCH 3/5] cli/dump: define GZPRINTF macro and use it in place of gzprintf

[David Bremner]

David Bremner <david@tethera.net> writes:

> Tomi Ollila <tomi.ollila@iki.fi> writes:
>
>> __location__ ?? never seen such a (preprocessor?) definition. could not
>> find any reference by search...
>
> I have to admit, it's just cargo culted from util/error_util.h
>
OIC, it's defined in talloc.h. Probably that deserves a comment.

[PATCH 1/6] test: add known_broken test for dumping large stored queries

[David Bremner]

'qsx' reported a bug on #notmuch with notmuch-dump and large stored
queries. This test will pass (on my machine) if the value of `repeat'
is made smaller.

Reported-By: Thomas Schneider <qsx@chaotikum.eu>
---
 test/T600-named-queries.sh | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/test/T600-named-queries.sh b/test/T600-named-queries.sh
index abaee3b7..852f7530 100755
--- a/test/T600-named-queries.sh
+++ b/test/T600-named-queries.sh
@@ -36,6 +36,22 @@ cat<<EOF > QUERIES.BEFORE
 EOF
 test_expect_equal_file QUERIES.BEFORE OUTPUT
 
+test_begin_subtest 'dumping large queries'
+test_subtest_known_broken
+# This value is just large enough to trigger a limitation of gzprintf
+# to 8191 bytes in total (by default).
+repeat=1329
+notmuch config set query.big "$(seq -s' ' $repeat)"
+notmuch dump --include=config > OUTPUT
+notmuch config set query.big ''
+printf "#notmuch-dump batch-tag:3 config\n#@ query.big " > EXPECTED
+seq -s'%20' $repeat >> EXPECTED
+cat <<EOF >> EXPECTED
+#@ query.test date%3a2009-11-18..2009-11-18%20and%20tag%3aunread
+#@ query.test2 query%3atest%20and%20subject%3aMaildir
+EOF
+test_expect_equal_file EXPECTED OUTPUT
+
 test_begin_subtest "delete named queries"
 notmuch dump > BEFORE
 notmuch config set query.test
-- 
2.25.1

[PATCH 2/6] util/zlib-extra.c: don't pass NULL to gzerror.

[David Bremner]

Although (as of 1.2.11) zlib checks this parameter before writing to
it, the docs don't promise to keep doing so, so be safe.
---
 notmuch-dump.c    | 6 +++---
 notmuch-restore.c | 2 +-
 util/zlib-extra.c | 2 +-
 util/zlib-extra.h | 5 +++++
 4 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/notmuch-dump.c b/notmuch-dump.c
index 65e02639..af346ba2 100644
--- a/notmuch-dump.c
+++ b/notmuch-dump.c
@@ -21,7 +21,7 @@
 #include "notmuch-client.h"
 #include "hex-escape.h"
 #include "string-util.h"
-#include <zlib.h>
+#include "zlib-extra.h"
 
 static int
 database_dump_config (notmuch_database_t *notmuch, gzFile output)
@@ -316,7 +316,7 @@ notmuch_database_dump (notmuch_database_t *notmuch,
 
     ret = gzflush (output, Z_FINISH);
     if (ret) {
-	fprintf (stderr, "Error flushing output: %s\n", gzerror (output, NULL));
+	fprintf (stderr, "Error flushing output: %s\n", gzerror_str (output));
 	goto DONE;
     }
 
@@ -332,7 +332,7 @@ notmuch_database_dump (notmuch_database_t *notmuch,
     ret = gzclose_w (output);
     if (ret) {
 	fprintf (stderr, "Error closing %s: %s\n", name_for_error,
-		 gzerror (output, NULL));
+		 gzerror_str (output));
 	ret = EXIT_FAILURE;
 	output = NULL;
 	goto DONE;
diff --git a/notmuch-restore.c b/notmuch-restore.c
index 4b509d95..9a8b7fb5 100644
--- a/notmuch-restore.c
+++ b/notmuch-restore.c
@@ -450,7 +450,7 @@ notmuch_restore_command (notmuch_config_t *config, int argc, char *argv[])
 
     if (input && gzclose_r (input)) {
 	fprintf (stderr, "Error closing %s: %s\n",
-		 name_for_error, gzerror (input, NULL));
+		 name_for_error, gzerror_str (input));
 	ret = EXIT_FAILURE;
     }
 
diff --git a/util/zlib-extra.c b/util/zlib-extra.c
index f691cccf..623f6d62 100644
--- a/util/zlib-extra.c
+++ b/util/zlib-extra.c
@@ -80,7 +80,7 @@ const char *
 gz_error_string (util_status_t status, gzFile file)
 {
     if (status == UTIL_GZERROR)
-	return gzerror (file, NULL);
+	return gzerror_str (file);
     else
 	return util_error_string (status);
 }
diff --git a/util/zlib-extra.h b/util/zlib-extra.h
index 209fa998..296dc914 100644
--- a/util/zlib-extra.h
+++ b/util/zlib-extra.h
@@ -27,6 +27,11 @@ gz_getline (void *ctx, char **lineptr, ssize_t *bytes_read, gzFile stream);
 const char *
 gz_error_string (util_status_t status, gzFile stream);
 
+/* Call gzerror with a dummy errno argument, the docs don't promise to
+ * support the NULL case */
+inline const char *
+gzerror_str(gzFile file) { int dummy; return gzerror (file, &dummy); }
+
 #ifdef __cplusplus
 }
 #endif
-- 
2.25.1

[PATCH 3/6] status: add print_status_gzbytes

[David Bremner]

This is in the client code, rather than libnotmuch_util, because it
prints to stderr. Also it in pretends to generate notmuch status
codes.
---
 notmuch-client.h |  8 +++++++-
 status.c         | 14 ++++++++++++++
 2 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/notmuch-client.h b/notmuch-client.h
index 89e15ba6..467e1d84 100644
--- a/notmuch-client.h
+++ b/notmuch-client.h
@@ -49,6 +49,7 @@
 #include <errno.h>
 #include <signal.h>
 #include <ctype.h>
+#include <zlib.h>
 
 #include "talloc-extra.h"
 #include "crypto.h"
@@ -469,7 +470,7 @@ notmuch_database_dump (notmuch_database_t *notmuch,
 		       dump_include_t include,
 		       bool gzip_output);
 
-/* If status is non-zero (i.e. error) print appropriate
+/* If status indicates error print appropriate
  * messages to stderr.
  */
 
@@ -491,6 +492,11 @@ print_status_database (const char *loc,
 int
 status_to_exit (notmuch_status_t status);
 
+notmuch_status_t
+print_status_gzbytes (const char *loc,
+		      gzFile file,
+		      int bytes);
+
 #include "command-line-arguments.h"
 
 extern const char *notmuch_requested_db_uuid;
diff --git a/status.c b/status.c
index d0ae47f4..09d82a17 100644
--- a/status.c
+++ b/status.c
@@ -72,3 +72,17 @@ status_to_exit (notmuch_status_t status)
 	return EXIT_FAILURE;
     }
 }
+
+notmuch_status_t
+print_status_gzbytes (const char *loc, gzFile file, int bytes)
+{
+    if (bytes <= 0) {
+	int errnum;
+	const char *errstr = gzerror (file, &errnum);
+	fprintf (stderr, "%s: zlib error %s (%d)\n", loc, errstr, errnum);
+	return NOTMUCH_STATUS_FILE_ERROR;
+    } else {
+	return NOTMUCH_STATUS_SUCCESS;
+    }
+}
+
-- 
2.25.1

[PATCH 4/6] cli/dump: define GZPRINTF macro and use it in place of gzprintf

[David Bremner]

This will at least catch errors, and can be replaced with more
sophisticated error handling where appropriate.
---
 notmuch-client.h |  4 ++++
 notmuch-dump.c   | 24 ++++++++++++------------
 2 files changed, 16 insertions(+), 12 deletions(-)

diff --git a/notmuch-client.h b/notmuch-client.h
index 467e1d84..01f5101a 100644
--- a/notmuch-client.h
+++ b/notmuch-client.h
@@ -497,6 +497,10 @@ print_status_gzbytes (const char *loc,
 		      gzFile file,
 		      int bytes);
 
+/* the __location__ macro is defined in talloc.h */
+#define ASSERT_GZBYTES(file, bytes) ((print_status_gzbytes (__location__, file, bytes)) ?  exit (1) : 0)
+#define GZPRINTF(file, fmt, ...) ASSERT_GZBYTES (file, gzprintf (file, fmt, ##__VA_ARGS__));
+
 #include "command-line-arguments.h"
 
 extern const char *notmuch_requested_db_uuid;
diff --git a/notmuch-dump.c b/notmuch-dump.c
index af346ba2..6c5c1433 100644
--- a/notmuch-dump.c
+++ b/notmuch-dump.c
@@ -42,7 +42,7 @@ database_dump_config (notmuch_database_t *notmuch, gzFile output)
 		     notmuch_config_list_key (list));
 	    goto DONE;
 	}
-	gzprintf (output, "#@ %s", buffer);
+	GZPRINTF (output, "#@ %s", buffer);
 
 	if (hex_encode (notmuch, notmuch_config_list_value (list),
 			&buffer, &buffer_size) != HEX_SUCCESS) {
@@ -51,7 +51,7 @@ database_dump_config (notmuch_database_t *notmuch, gzFile output)
 	    goto DONE;
 	}
 
-	gzprintf (output, " %s\n", buffer);
+	GZPRINTF (output, " %s\n", buffer);
     }
 
     ret = EXIT_SUCCESS;
@@ -71,7 +71,7 @@ print_dump_header (gzFile output, int output_format, int include)
 {
     const char *sep = "";
 
-    gzprintf (output, "#notmuch-dump %s:%d ",
+    GZPRINTF (output, "#notmuch-dump %s:%d ",
 	      (output_format == DUMP_FORMAT_SUP) ? "sup" : "batch-tag",
 	      NOTMUCH_DUMP_VERSION);
 
@@ -80,11 +80,11 @@ print_dump_header (gzFile output, int output_format, int include)
 	sep = ",";
     }
     if (include & DUMP_INCLUDE_PROPERTIES) {
-	gzprintf (output, "%sproperties", sep);
+	GZPRINTF (output, "%sproperties", sep);
 	sep = ",";
     }
     if (include & DUMP_INCLUDE_TAGS) {
-	gzprintf (output, "%stags", sep);
+	GZPRINTF (output, "%stags", sep);
     }
     gzputs (output, "\n");
 }
@@ -115,7 +115,7 @@ dump_properties_message (void *ctx,
 		fprintf (stderr, "Error: failed to hex-encode message-id %s\n", message_id);
 		return 1;
 	    }
-	    gzprintf (output, "#= %s", *buffer_p);
+	    GZPRINTF (output, "#= %s", *buffer_p);
 	    first = false;
 	}
 
@@ -126,18 +126,18 @@ dump_properties_message (void *ctx,
 	    fprintf (stderr, "Error: failed to hex-encode key %s\n", key);
 	    return 1;
 	}
-	gzprintf (output, " %s", *buffer_p);
+	GZPRINTF (output, " %s", *buffer_p);
 
 	if (hex_encode (ctx, val, buffer_p, size_p) != HEX_SUCCESS) {
 	    fprintf (stderr, "Error: failed to hex-encode value %s\n", val);
 	    return 1;
 	}
-	gzprintf (output, "=%s", *buffer_p);
+	GZPRINTF (output, "=%s", *buffer_p);
     }
     notmuch_message_properties_destroy (list);
 
     if (! first)
-	gzprintf (output, "\n", *buffer_p);
+	GZPRINTF (output, "\n", *buffer_p);
 
     return 0;
 }
@@ -165,7 +165,7 @@ dump_tags_message (void *ctx,
     }
 
     if (output_format == DUMP_FORMAT_SUP) {
-	gzprintf (output, "%s (", message_id);
+	GZPRINTF (output, "%s (", message_id);
     }
 
     for (notmuch_tags_t *tags = notmuch_message_get_tags (message);
@@ -187,7 +187,7 @@ dump_tags_message (void *ctx,
 			 tag_str);
 		return EXIT_FAILURE;
 	    }
-	    gzprintf (output, "+%s", *buffer_p);
+	    GZPRINTF (output, "+%s", *buffer_p);
 	}
     }
 
@@ -200,7 +200,7 @@ dump_tags_message (void *ctx,
 		     message_id, strerror (errno));
 	    return EXIT_FAILURE;
 	}
-	gzprintf (output, " -- %s\n", *buffer_p);
+	GZPRINTF (output, " -- %s\n", *buffer_p);
     }
     return EXIT_SUCCESS;
 }
-- 
2.25.1

[PATCH 5/6] cli/dump: define GZPUTS and use it in notmuch-dump

[David Bremner]

Similarly to GZPRINTF, this is a drop in replacement that can be
improved where needd.
---
 notmuch-client.h |  1 +
 notmuch-dump.c   | 10 +++++-----
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/notmuch-client.h b/notmuch-client.h
index 01f5101a..b59f3c81 100644
--- a/notmuch-client.h
+++ b/notmuch-client.h
@@ -500,6 +500,7 @@ print_status_gzbytes (const char *loc,
 /* the __location__ macro is defined in talloc.h */
 #define ASSERT_GZBYTES(file, bytes) ((print_status_gzbytes (__location__, file, bytes)) ?  exit (1) : 0)
 #define GZPRINTF(file, fmt, ...) ASSERT_GZBYTES (file, gzprintf (file, fmt, ##__VA_ARGS__));
+#define GZPUTS(file, str) ASSERT_GZBYTES(file, gzputs (file, str));
 
 #include "command-line-arguments.h"
 
diff --git a/notmuch-dump.c b/notmuch-dump.c
index 6c5c1433..52a88283 100644
--- a/notmuch-dump.c
+++ b/notmuch-dump.c
@@ -76,7 +76,7 @@ print_dump_header (gzFile output, int output_format, int include)
 	      NOTMUCH_DUMP_VERSION);
 
     if (include & DUMP_INCLUDE_CONFIG) {
-	gzputs (output, "config");
+	GZPUTS (output, "config");
 	sep = ",";
     }
     if (include & DUMP_INCLUDE_PROPERTIES) {
@@ -86,7 +86,7 @@ print_dump_header (gzFile output, int output_format, int include)
     if (include & DUMP_INCLUDE_TAGS) {
 	GZPRINTF (output, "%stags", sep);
     }
-    gzputs (output, "\n");
+    GZPUTS (output, "\n");
 }
 
 static int
@@ -174,12 +174,12 @@ dump_tags_message (void *ctx,
 	const char *tag_str = notmuch_tags_get (tags);
 
 	if (! first)
-	    gzputs (output, " ");
+	    GZPUTS (output, " ");
 
 	first = 0;
 
 	if (output_format == DUMP_FORMAT_SUP) {
-	    gzputs (output, tag_str);
+	    GZPUTS (output, tag_str);
 	} else {
 	    if (hex_encode (ctx, tag_str,
 			    buffer_p, size_p) != HEX_SUCCESS) {
@@ -192,7 +192,7 @@ dump_tags_message (void *ctx,
     }
 
     if (output_format == DUMP_FORMAT_SUP) {
-	gzputs (output, ")\n");
+	GZPUTS (output, ")\n");
     } else {
 	if (make_boolean_term (ctx, "id", message_id,
 			       buffer_p, size_p)) {
-- 
2.25.1

[PATCH 6/6] cli/dump: replace use of gzprintf with gzputs for config values

[David Bremner]

These can be large, and hit buffer limitations of gzprintf.
---
 notmuch-dump.c             | 4 +++-
 test/T600-named-queries.sh | 1 -
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/notmuch-dump.c b/notmuch-dump.c
index 52a88283..887ef7f0 100644
--- a/notmuch-dump.c
+++ b/notmuch-dump.c
@@ -51,7 +51,9 @@ database_dump_config (notmuch_database_t *notmuch, gzFile output)
 	    goto DONE;
 	}
 
-	GZPRINTF (output, " %s\n", buffer);
+	GZPUTS (output, " ");
+	GZPUTS (output, buffer);
+	GZPUTS (output, "\n");
     }
 
     ret = EXIT_SUCCESS;
diff --git a/test/T600-named-queries.sh b/test/T600-named-queries.sh
index 852f7530..421a11d4 100755
--- a/test/T600-named-queries.sh
+++ b/test/T600-named-queries.sh
@@ -37,7 +37,6 @@ EOF
 test_expect_equal_file QUERIES.BEFORE OUTPUT
 
 test_begin_subtest 'dumping large queries'
-test_subtest_known_broken
 # This value is just large enough to trigger a limitation of gzprintf
 # to 8191 bytes in total (by default).
 repeat=1329
-- 
2.25.1

Re: [PATCH 4/6] cli/dump: define GZPRINTF macro and use it in place of gzprintf

[Tomi Ollila]

On Mon, Apr 13 2020, David Bremner wrote:

> This will at least catch errors, and can be replaced with more
> sophisticated error handling where appropriate.
> ---
>  notmuch-client.h |  4 ++++
>  notmuch-dump.c   | 24 ++++++++++++------------
>  2 files changed, 16 insertions(+), 12 deletions(-)
>
> diff --git a/notmuch-client.h b/notmuch-client.h
> index 467e1d84..01f5101a 100644
> --- a/notmuch-client.h
> +++ b/notmuch-client.h
> @@ -497,6 +497,10 @@ print_status_gzbytes (const char *loc,
>  		      gzFile file,
>  		      int bytes);
>  
> +/* the __location__ macro is defined in talloc.h */
> +#define ASSERT_GZBYTES(file, bytes) ((print_status_gzbytes (__location__, file, bytes)) ?  exit (1) : 0)

The 2 spaces before exit (1) I tried to communicate ;) well, perhaps the
__location__ thing broke the flow...

can be amended if no other comments to be seen

Tomi

Re: [PATCH 1/6] test: add known_broken test for dumping large stored queries

[David Bremner]

David Bremner <david@tethera.net> writes:

> 'qsx' reported a bug on #notmuch with notmuch-dump and large stored
> queries. This test will pass (on my machine) if the value of `repeat'
> is made smaller.
>
> Reported-By: Thomas Schneider <qsx@chaotikum.eu>

series pushed, with one space deleted to please Tomi ;P

d