batch 6, API exception handling cleanup.

batch 6, API exception handling cleanup.

[David Bremner]

These follow the series at

      id:20200714224119.717845-1-david@tethera.net

Probably most interesting is 

     [PATCH 10/10] test: add known broken test for indexing relative path

That test is also part of the bug report
          
     id:87sgdqo0rz.fsf@tethera.net

[PATCH 01/10] test: regression test for closing a closed database

[David Bremner]

This does not currently throw an error, and it should stay that way.
---
 test/T562-lib-database.sh | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/test/T562-lib-database.sh b/test/T562-lib-database.sh
index c9705b13..0dae0096 100755
--- a/test/T562-lib-database.sh
+++ b/test/T562-lib-database.sh
@@ -83,4 +83,19 @@ cat <<EOF > EXPECTED
 EOF
 test_expect_equal_file EXPECTED OUTPUT
 
+test_begin_subtest "re-close a closed db"
+cat c_head - c_tail <<'EOF' | test_C ${MAIL_DIR}
+    {
+        EXPECT0(notmuch_database_close (db));
+        stat = notmuch_database_close (db);
+        printf ("%d\n", stat);
+    }
+EOF
+cat <<EOF > EXPECTED
+== stdout ==
+0
+== stderr ==
+EOF
+test_expect_equal_file EXPECTED OUTPUT
+
 test_done
-- 
2.27.0

[PATCH 02/10] test: add regression tests for notmuch database destroy

[David Bremner]

Either an open or closed database should be ok to destroy
---
 test/T562-lib-database.sh | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/test/T562-lib-database.sh b/test/T562-lib-database.sh
index 0dae0096..db7a34f3 100755
--- a/test/T562-lib-database.sh
+++ b/test/T562-lib-database.sh
@@ -98,4 +98,35 @@ cat <<EOF > EXPECTED
 EOF
 test_expect_equal_file EXPECTED OUTPUT
 
+test_begin_subtest "destroy a closed db"
+cat c_head - c_tail <<'EOF' | test_C ${MAIL_DIR}
+    {
+        unsigned int version;
+        EXPECT0(notmuch_database_close (db));
+        stat = notmuch_database_destroy (db);
+        printf ("%d\n", stat);
+    }
+EOF
+cat <<EOF > EXPECTED
+== stdout ==
+0
+== stderr ==
+EOF
+test_expect_equal_file EXPECTED OUTPUT
+
+test_begin_subtest "destroy an open db"
+cat c_head - c_tail <<'EOF' | test_C ${MAIL_DIR}
+    {
+        unsigned int version;
+        stat = notmuch_database_destroy (db);
+        printf ("%d\n", stat);
+    }
+EOF
+cat <<EOF > EXPECTED
+== stdout ==
+0
+== stderr ==
+EOF
+test_expect_equal_file EXPECTED OUTPUT
+
 test_done
-- 
2.27.0

[PATCH 03/10] test: add known broken test for n_d_needs_upgrade

[David Bremner]

It's a bit arbitrary which value to return for errors, but the same
argument as for read only databases applies for errors.
---
 test/T562-lib-database.sh | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/test/T562-lib-database.sh b/test/T562-lib-database.sh
index db7a34f3..0af7587a 100755
--- a/test/T562-lib-database.sh
+++ b/test/T562-lib-database.sh
@@ -129,4 +129,22 @@ cat <<EOF > EXPECTED
 EOF
 test_expect_equal_file EXPECTED OUTPUT
 
+test_begin_subtest "check a closed db for upgrade"
+test_subtest_known_broken
+cat c_head - c_tail <<'EOF' | test_C ${MAIL_DIR}
+    {
+        notmuch_bool_t ret;
+
+        EXPECT0(notmuch_database_close (db));
+        ret = notmuch_database_needs_upgrade (db);
+        printf ("%d\n", ret == FALSE);
+    }
+EOF
+cat <<EOF > EXPECTED
+== stdout ==
+1
+== stderr ==
+EOF
+test_expect_equal_file EXPECTED OUTPUT
+
 test_done
-- 
2.27.0

[PATCH 04/10] lib/n_d_needs_upgrade: handle error return from n_d_get_version

[David Bremner]

Also clarify documentation of error return from n_d_needs_upgrade.
---
 lib/database.cc           | 14 +++++++++++---
 lib/notmuch.h             |  2 ++
 test/T562-lib-database.sh |  1 -
 3 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/lib/database.cc b/lib/database.cc
index 27861970..25d34253 100644
--- a/lib/database.cc
+++ b/lib/database.cc
@@ -1395,9 +1395,17 @@ notmuch_database_get_version (notmuch_database_t *notmuch)
 notmuch_bool_t
 notmuch_database_needs_upgrade (notmuch_database_t *notmuch)
 {
-    return notmuch->mode == NOTMUCH_DATABASE_MODE_READ_WRITE &&
-	   ((NOTMUCH_FEATURES_CURRENT & ~notmuch->features) ||
-	    (notmuch_database_get_version (notmuch) < NOTMUCH_DATABASE_VERSION));
+    unsigned int version;
+
+    if (notmuch->mode != NOTMUCH_DATABASE_MODE_READ_WRITE)
+	return FALSE;
+
+    if (NOTMUCH_FEATURES_CURRENT & ~notmuch->features)
+	return TRUE;
+
+    version = notmuch_database_get_version (notmuch);
+
+    return (version > 0 && version < NOTMUCH_DATABASE_VERSION);
 }
 
 static volatile sig_atomic_t do_progress_notify = 0;
diff --git a/lib/notmuch.h b/lib/notmuch.h
index 45ecc59b..b8b89a4a 100644
--- a/lib/notmuch.h
+++ b/lib/notmuch.h
@@ -447,6 +447,8 @@ notmuch_database_get_version (notmuch_database_t *database);
  * FALSE for a read-only database because there's no way to upgrade a
  * read-only database.
  *
+ * Also returns FALSE if an error occurs accessing the database.
+ *
  */
 notmuch_bool_t
 notmuch_database_needs_upgrade (notmuch_database_t *database);
diff --git a/test/T562-lib-database.sh b/test/T562-lib-database.sh
index 0af7587a..4bcbc805 100755
--- a/test/T562-lib-database.sh
+++ b/test/T562-lib-database.sh
@@ -130,7 +130,6 @@ EOF
 test_expect_equal_file EXPECTED OUTPUT
 
 test_begin_subtest "check a closed db for upgrade"
-test_subtest_known_broken
 cat c_head - c_tail <<'EOF' | test_C ${MAIL_DIR}
     {
         notmuch_bool_t ret;
-- 
2.27.0

[PATCH 05/10] test: regression test for n_d_upgrade

[David Bremner]

The logic is that if it's acceptable to return SUCCESS for read only
database, it's acceptable for a closed one.
---
 test/T562-lib-database.sh | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/test/T562-lib-database.sh b/test/T562-lib-database.sh
index 4bcbc805..d975f109 100755
--- a/test/T562-lib-database.sh
+++ b/test/T562-lib-database.sh
@@ -146,4 +146,21 @@ cat <<EOF > EXPECTED
 EOF
 test_expect_equal_file EXPECTED OUTPUT
 
+test_begin_subtest "upgrade a closed db"
+cat c_head - c_tail <<'EOF' | test_C ${MAIL_DIR}
+    {
+        notmuch_bool_t ret;
+
+        EXPECT0(notmuch_database_close (db));
+        stat = notmuch_database_upgrade (db, NULL, NULL);
+        printf ("%d\n", ret == NOTMUCH_STATUS_SUCCESS);
+    }
+EOF
+cat <<EOF > EXPECTED
+== stdout ==
+1
+== stderr ==
+EOF
+test_expect_equal_file EXPECTED OUTPUT
+
 test_done
-- 
2.27.0

[PATCH 06/10] test: add regression test for n_d_{begin,end}_atomic

[David Bremner]

Xapian currently succeeds to begin/end a transaction on a closed database,
or at least does not throw an exception. Make the test robust against
this changing.
---
 test/T562-lib-database.sh | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/test/T562-lib-database.sh b/test/T562-lib-database.sh
index d975f109..94e0e06a 100755
--- a/test/T562-lib-database.sh
+++ b/test/T562-lib-database.sh
@@ -163,4 +163,37 @@ cat <<EOF > EXPECTED
 EOF
 test_expect_equal_file EXPECTED OUTPUT
 
+test_begin_subtest "begin atomic section for a closed db"
+cat c_head - c_tail <<'EOF' | test_C ${MAIL_DIR}
+    {
+        EXPECT0(notmuch_database_close (db));
+        stat = notmuch_database_begin_atomic (db);
+        printf ("%d\n", stat == NOTMUCH_STATUS_SUCCESS ||
+                        stat == NOTMUCH_STATUS_XAPIAN_EXCEPTION);
+    }
+EOF
+cat <<EOF > EXPECTED
+== stdout ==
+1
+== stderr ==
+EOF
+test_expect_equal_file EXPECTED OUTPUT
+
+test_begin_subtest "end atomic section for a closed db"
+cat c_head - c_tail <<'EOF' | test_C ${MAIL_DIR}
+    {
+        EXPECT0(notmuch_database_close (db));
+        EXPECT0(notmuch_database_begin_atomic (db));
+        stat = notmuch_database_end_atomic (db);
+        printf ("%d\n", stat == NOTMUCH_STATUS_SUCCESS ||
+                        stat == NOTMUCH_STATUS_XAPIAN_EXCEPTION);
+    }
+EOF
+cat <<EOF > EXPECTED
+== stdout ==
+1
+== stderr ==
+EOF
+test_expect_equal_file EXPECTED OUTPUT
+
 test_done
-- 
2.27.0

[PATCH 07/10] test: regression test for n_d_get_revision

[David Bremner]

This function only accesses data cached by notmuch, so being closed is
not a problem.
---
 test/T562-lib-database.sh | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/test/T562-lib-database.sh b/test/T562-lib-database.sh
index 94e0e06a..e9f00726 100755
--- a/test/T562-lib-database.sh
+++ b/test/T562-lib-database.sh
@@ -196,4 +196,22 @@ cat <<EOF > EXPECTED
 EOF
 test_expect_equal_file EXPECTED OUTPUT
 
+test_begin_subtest "get revision for a closed db"
+cat c_head - c_tail <<'EOF' | test_C ${MAIL_DIR}
+    {
+        const char *uuid;
+        unsigned long rev;
+
+        EXPECT0(notmuch_database_close (db));
+        rev = notmuch_database_get_revision (db, &uuid);
+        printf ("%d\n", rev, uuid);
+    }
+EOF
+cat <<EOF > EXPECTED
+== stdout ==
+53
+== stderr ==
+EOF
+test_expect_equal_file EXPECTED OUTPUT
+
 test_done
-- 
2.27.0

[PATCH 08/10] test: add regression test for n_d_get_directory

[David Bremner]

At least this exception is already handled correctly.
---
 test/T562-lib-database.sh | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/test/T562-lib-database.sh b/test/T562-lib-database.sh
index e9f00726..06569291 100755
--- a/test/T562-lib-database.sh
+++ b/test/T562-lib-database.sh
@@ -214,4 +214,21 @@ cat <<EOF > EXPECTED
 EOF
 test_expect_equal_file EXPECTED OUTPUT
 
+test_begin_subtest "get directory for a closed db"
+cat c_head - c_tail <<'EOF' | test_C ${MAIL_DIR}
+    {
+        notmuch_directory_t *dir;
+        EXPECT0(notmuch_database_close (db));
+        stat = notmuch_database_get_directory (db, "/nonexistent", &dir);
+        printf ("%d\n", stat == NOTMUCH_STATUS_XAPIAN_EXCEPTION);
+    }
+EOF
+cat <<EOF > EXPECTED
+== stdout ==
+1
+== stderr ==
+A Xapian exception occurred creating a directory: Database has been closed.
+EOF
+test_expect_equal_file EXPECTED OUTPUT
+
 test_done
-- 
2.27.0

[PATCH 09/10] test: regression test for n_d_index_file closed db

[David Bremner]

Current exception handling seems OK, at least for this case.
---
 test/T562-lib-database.sh | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/test/T562-lib-database.sh b/test/T562-lib-database.sh
index 06569291..6a51002a 100755
--- a/test/T562-lib-database.sh
+++ b/test/T562-lib-database.sh
@@ -12,6 +12,7 @@ cat <<EOF > c_head
 #include <stdio.h>
 #include <notmuch.h>
 #include <notmuch-test.h>
+#include <talloc.h>
 int main (int argc, char** argv)
 {
    notmuch_database_t *db;
@@ -231,4 +232,23 @@ A Xapian exception occurred creating a directory: Database has been closed.
 EOF
 test_expect_equal_file EXPECTED OUTPUT
 
+# XXX TODO: test with relative path
+test_begin_subtest "index file with a closed db"
+cat c_head - c_tail <<'EOF' | test_C ${MAIL_DIR}
+    {
+        notmuch_message_t *msg;
+        const char *path = talloc_asprintf(db, "%s/01:2,", argv[1]);
+        EXPECT0(notmuch_database_close (db));
+        stat = notmuch_database_index_file (db, path, NULL, &msg);
+        printf ("%d\n", stat == NOTMUCH_STATUS_XAPIAN_EXCEPTION);
+    }
+EOF
+cat <<EOF > EXPECTED
+== stdout ==
+1
+== stderr ==
+A Xapian exception occurred finding message: Database has been closed.
+EOF
+test_expect_equal_file EXPECTED OUTPUT
+
 test_done
-- 
2.27.0

[PATCH 10/10] test: add known broken test for indexing relative path

[David Bremner]

The current code seems to look for the path relative to the current
working directory, rather than the mail store root.
---
 test/T562-lib-database.sh | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/test/T562-lib-database.sh b/test/T562-lib-database.sh
index 6a51002a..c9d7c748 100755
--- a/test/T562-lib-database.sh
+++ b/test/T562-lib-database.sh
@@ -251,4 +251,21 @@ A Xapian exception occurred finding message: Database has been closed.
 EOF
 test_expect_equal_file EXPECTED OUTPUT
 
+generate_message '[filename]=relative_path'
+test_subtest_known_broken
+test_begin_subtest "index file (relative path)"
+cat c_head - c_tail <<'EOF' | test_C ${MAIL_DIR}
+    {
+        notmuch_message_t *msg;
+        stat = notmuch_database_index_file (db, "relative_path", NULL, &msg);
+        printf ("%d\n", stat == NOTMUCH_STATUS_SUCCESS);
+    }
+EOF
+cat <<EOF > EXPECTED
+== stdout ==
+1
+== stderr ==
+EOF
+test_expect_equal_file EXPECTED OUTPUT
+
 test_done
-- 
2.27.0

[PATCH] lib: convert relative filenames to absolute in n_d_index_file

[David Bremner]

The API docs promise to handle relative filenames, but the code did
not do it.

Also check for files outside the mail root, as implied by the API
description.

This fixes the bug reported at

     id:87sgdqo0rz.fsf@tethera.net
---
 lib/message-file.c        | 24 ++++++++++++++++++++----
 test/T562-lib-database.sh | 18 +++++++++++++++++-
 2 files changed, 37 insertions(+), 5 deletions(-)

diff --git a/lib/message-file.c b/lib/message-file.c
index e1db26fb..311bd478 100644
--- a/lib/message-file.c
+++ b/lib/message-file.c
@@ -64,21 +64,37 @@ _notmuch_message_file_open_ctx (notmuch_database_t *notmuch,
     if (unlikely (message == NULL))
 	return NULL;
 
-    message->filename = talloc_strdup (message, filename);
+    const char *prefix = notmuch_database_get_path (notmuch);
+    if (prefix == NULL)
+	goto FAIL;
+
+    if (*filename == '/') {
+	if (strncmp (filename, prefix, strlen(prefix)) != 0) {
+	    _notmuch_database_log (notmuch, "Error opening %s: path outside mail root\n",
+				   filename);
+	    errno = 0;
+	    goto FAIL;
+	}
+	message->filename = talloc_strdup (message, filename);
+    } else {
+	message->filename = talloc_asprintf(message, "%s/%s", prefix, filename);
+    }
+
     if (message->filename == NULL)
 	goto FAIL;
 
     talloc_set_destructor (message, _notmuch_message_file_destructor);
 
-    message->stream = g_mime_stream_gzfile_open (filename);
+    message->stream = g_mime_stream_gzfile_open (message->filename);
     if (message->stream == NULL)
 	goto FAIL;
 
     return message;
 
   FAIL:
-    _notmuch_database_log (notmuch, "Error opening %s: %s\n",
-			   filename, strerror (errno));
+    if (errno)
+	_notmuch_database_log (notmuch, "Error opening %s: %s\n",
+			       filename, strerror (errno));
     _notmuch_message_file_close (message);
 
     return NULL;
diff --git a/test/T562-lib-database.sh b/test/T562-lib-database.sh
index 4e225ccc..301eeb73 100755
--- a/test/T562-lib-database.sh
+++ b/test/T562-lib-database.sh
@@ -253,7 +253,6 @@ test_expect_equal_file EXPECTED OUTPUT
 
 generate_message '[filename]=relative_path'
 test_begin_subtest "index file (relative path)"
-test_subtest_known_broken
 cat c_head - c_tail <<'EOF' | test_C ${MAIL_DIR}
     {
         notmuch_message_t *msg;
@@ -268,4 +267,21 @@ cat <<EOF > EXPECTED
 EOF
 test_expect_equal_file EXPECTED OUTPUT
 
+test_begin_subtest "index file (absolute path outside mail root)"
+cat c_head - c_tail <<'EOF' | test_C ${MAIL_DIR}
+    {
+        notmuch_message_t *msg;
+        stat = notmuch_database_index_file (db, "/dev/zero", NULL, &msg);
+        printf ("%d\n", stat == NOTMUCH_STATUS_FILE_ERROR);
+    }
+EOF
+cat <<EOF > EXPECTED
+== stdout ==
+1
+== stderr ==
+Error opening /dev/zero: path outside mail root
+EOF
+test_expect_equal_file EXPECTED OUTPUT
+
+
 test_done
-- 
2.27.0

Re: [PATCH 10/10] test: add known broken test for indexing relative path

[David Bremner]

David Bremner <david@tethera.net> writes:

> The current code seems to look for the path relative to the current
> working directory, rather than the mail store root.
> ---
>  test/T562-lib-database.sh | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)

Series applied to master

d

Re: batch 6, API exception handling cleanup.

[David Bremner]

David Bremner <david@tethera.net> writes:

> These follow the series at
>
>       id:20200714224119.717845-1-david@tethera.net
>
> Probably most interesting is 
>
>      [PATCH 10/10] test: add known broken test for indexing relative path
>
> That test is also part of the bug report
>           
>      id:87sgdqo0rz.fsf@tethera.net

Queued, with some minor updates:

diff --git a/test/T562-lib-database.sh b/test/T562-lib-database.sh
index d2749a80..e64f0f12 100755
--- a/test/T562-lib-database.sh
+++ b/test/T562-lib-database.sh
@@ -140,12 +140,14 @@ cat c_head - c_tail <<'EOF' | test_C ${MAIL_DIR}
         EXPECT0(notmuch_database_close (db));
         ret = notmuch_database_needs_upgrade (db);
         printf ("%d\n", ret == FALSE);
+        stat = NOTMUCH_STATUS_XAPIAN_EXCEPTION;
     }
 EOF
 cat <<EOF > EXPECTED
 == stdout ==
 1
 == stderr ==
+A Xapian exception occurred at lib/database.cc:XXX: Database has been closed
 EOF
 test_expect_equal_file EXPECTED OUTPUT
 
@@ -173,6 +175,7 @@ cat c_head - c_tail <<'EOF' | test_C ${MAIL_DIR}
         stat = notmuch_database_begin_atomic (db);
         printf ("%d\n", stat == NOTMUCH_STATUS_SUCCESS ||
                         stat == NOTMUCH_STATUS_XAPIAN_EXCEPTION);
+        stat = NOTMUCH_STATUS_SUCCESS;
     }
 EOF
 cat <<EOF > EXPECTED
@@ -190,6 +193,7 @@ cat c_head - c_tail <<'EOF' | test_C ${MAIL_DIR}
         stat = notmuch_database_end_atomic (db);
         printf ("%d\n", stat == NOTMUCH_STATUS_SUCCESS ||
                         stat == NOTMUCH_STATUS_XAPIAN_EXCEPTION);
+        stat = NOTMUCH_STATUS_SUCCESS;
     }
 EOF
 cat <<EOF > EXPECTED
@@ -234,7 +238,6 @@ A Xapian exception occurred creating a directory: Database has been closed.
 EOF
 test_expect_equal_file EXPECTED OUTPUT
 
-# XXX TODO: test with relative path
 test_begin_subtest "index file with a closed db"
 cat c_head - c_tail <<'EOF' | test_C ${MAIL_DIR}
     {