batch 7, API exception handline cleanup.

batch 7, API exception handline cleanup.

[David Bremner]

These follow the series

      id:20200718150751.4106125-1-david@tethera.net

Probably the most interesting change is

         [PATCH 7/8] lib: fix error return bug with n_d_set_config.

since that fixes a real bug with error handling.

Patch 8/8 identifies a similar issue with error values being swallowed.
I'm hoping Daniel can help me with this, and I'll follow
up with a seperate email.

[PATCH 1/8] test: add known broken test for notmuch_database_remove_message

[David Bremner]

The current error message is a bit confusing; fix in next commit.
---
 test/T562-lib-database.sh | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/test/T562-lib-database.sh b/test/T562-lib-database.sh
index 77af313f..56cc849c 100755
--- a/test/T562-lib-database.sh
+++ b/test/T562-lib-database.sh
@@ -282,5 +282,21 @@ Error opening /dev/zero: path outside mail root
 EOF
 test_expect_equal_file EXPECTED OUTPUT
 
+test_begin_subtest "remove message file with a closed db"
+test_subtest_known_broken
+cat c_head - c_tail <<'EOF' | test_C ${MAIL_DIR}
+    {
+        EXPECT0(notmuch_database_close (db));
+        stat = notmuch_database_remove_message (db, "01:2,");
+        printf ("%d\n", stat == NOTMUCH_STATUS_XAPIAN_EXCEPTION);
+    }
+EOF
+cat <<EOF > EXPECTED
+== stdout ==
+1
+== stderr ==
+A Xapian exception occurred finding/creating a directory: Database has been closed.
+EOF
+test_expect_equal_file EXPECTED OUTPUT
 
 test_done
-- 
2.27.0

[PATCH 2/8] lib: rename _n_d_create to _n_d_find_or_create

[David Bremner]

The error message and name were confusing when called in some "read
only" context.
---
 lib/database.cc           | 10 +++++-----
 lib/directory.cc          | 10 +++++-----
 lib/notmuch-private.h     |  8 ++++----
 test/T560-lib-error.sh    |  2 +-
 test/T562-lib-database.sh |  3 +--
 5 files changed, 16 insertions(+), 17 deletions(-)

diff --git a/lib/database.cc b/lib/database.cc
index 25d34253..f81f28f8 100644
--- a/lib/database.cc
+++ b/lib/database.cc
@@ -1605,8 +1605,8 @@ notmuch_database_upgrade (notmuch_database_t *notmuch,
 		mtime = Xapian::sortable_unserialise (
 		    document.get_value (NOTMUCH_VALUE_TIMESTAMP));
 
-		directory = _notmuch_directory_create (notmuch, term.c_str () + 10,
-						       NOTMUCH_FIND_CREATE, &status);
+		directory = _notmuch_directory_find_or_create (notmuch, term.c_str () + 10,
+							       NOTMUCH_FIND_CREATE, &status);
 		notmuch_directory_set_mtime (directory, mtime);
 		notmuch_directory_destroy (directory);
 
@@ -1878,7 +1878,7 @@ _notmuch_database_find_directory_id (notmuch_database_t *notmuch,
 	return NOTMUCH_STATUS_SUCCESS;
     }
 
-    directory = _notmuch_directory_create (notmuch, path, flags, &status);
+    directory = _notmuch_directory_find_or_create (notmuch, path, flags, &status);
     if (status || ! directory) {
 	*directory_id = -1;
 	return status;
@@ -1988,8 +1988,8 @@ notmuch_database_get_directory (notmuch_database_t *notmuch,
     *directory = NULL;
 
     try {
-	*directory = _notmuch_directory_create (notmuch, path,
-						NOTMUCH_FIND_LOOKUP, &status);
+	*directory = _notmuch_directory_find_or_create (notmuch, path,
+							NOTMUCH_FIND_LOOKUP, &status);
     } catch (const Xapian::Error &error) {
 	_notmuch_database_log (notmuch, "A Xapian exception occurred getting directory: %s.\n",
 			       error.get_msg ().c_str ());
diff --git a/lib/directory.cc b/lib/directory.cc
index af71f402..09b49245 100644
--- a/lib/directory.cc
+++ b/lib/directory.cc
@@ -94,10 +94,10 @@ find_directory_document (notmuch_database_t *notmuch,
  * NOTMUCH_STATUS_SUCCESS and this returns NULL.
  */
 notmuch_directory_t *
-_notmuch_directory_create (notmuch_database_t *notmuch,
-			   const char *path,
-			   notmuch_find_flags_t flags,
-			   notmuch_status_t *status_ret)
+_notmuch_directory_find_or_create (notmuch_database_t *notmuch,
+				   const char *path,
+				   notmuch_find_flags_t flags,
+				   notmuch_status_t *status_ret)
 {
     Xapian::WritableDatabase *db;
     notmuch_directory_t *directory;
@@ -187,7 +187,7 @@ _notmuch_directory_create (notmuch_database_t *notmuch,
 	    directory->doc.get_value (NOTMUCH_VALUE_TIMESTAMP));
     } catch (const Xapian::Error &error) {
 	_notmuch_database_log (notmuch,
-			       "A Xapian exception occurred creating a directory: %s.\n",
+			       "A Xapian exception occurred finding/creating a directory: %s.\n",
 			       error.get_msg ().c_str ());
 	notmuch->exception_reported = true;
 	notmuch_directory_destroy (directory);
diff --git a/lib/notmuch-private.h b/lib/notmuch-private.h
index 28ced3a2..2bbbb293 100644
--- a/lib/notmuch-private.h
+++ b/lib/notmuch-private.h
@@ -251,10 +251,10 @@ _notmuch_database_filename_to_direntry (void *ctx,
 /* directory.cc */
 
 notmuch_directory_t *
-_notmuch_directory_create (notmuch_database_t *notmuch,
-			   const char *path,
-			   notmuch_find_flags_t flags,
-			   notmuch_status_t *status_ret);
+_notmuch_directory_find_or_create (notmuch_database_t *notmuch,
+				   const char *path,
+				   notmuch_find_flags_t flags,
+				   notmuch_status_t *status_ret);
 
 unsigned int
 _notmuch_directory_get_document_id (notmuch_directory_t *directory);
diff --git a/test/T560-lib-error.sh b/test/T560-lib-error.sh
index fda1f170..908bb9d8 100755
--- a/test/T560-lib-error.sh
+++ b/test/T560-lib-error.sh
@@ -275,7 +275,7 @@ sed 's/^\(A Xapian exception [^:]*\):.*$/\1/' < OUTPUT > OUTPUT.clean
 cat <<'EOF' >EXPECTED
 == stdout ==
 == stderr ==
-A Xapian exception occurred creating a directory
+A Xapian exception occurred finding/creating a directory
 EOF
 test_expect_equal_file EXPECTED OUTPUT.clean
 restore_database
diff --git a/test/T562-lib-database.sh b/test/T562-lib-database.sh
index 56cc849c..5ebaf64c 100755
--- a/test/T562-lib-database.sh
+++ b/test/T562-lib-database.sh
@@ -228,7 +228,7 @@ cat <<EOF > EXPECTED
 == stdout ==
 1
 == stderr ==
-A Xapian exception occurred creating a directory: Database has been closed.
+A Xapian exception occurred finding/creating a directory: Database has been closed.
 EOF
 test_expect_equal_file EXPECTED OUTPUT
 
@@ -283,7 +283,6 @@ EOF
 test_expect_equal_file EXPECTED OUTPUT
 
 test_begin_subtest "remove message file with a closed db"
-test_subtest_known_broken
 cat c_head - c_tail <<'EOF' | test_C ${MAIL_DIR}
     {
         EXPECT0(notmuch_database_close (db));
-- 
2.27.0

[PATCH 3/8] lib: add regression test for n_d_find_message_by_filename

[David Bremner]

At least this Xapian exception is caught. Make sure it stays that way.
---
 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 5ebaf64c..fea77f78 100755
--- a/test/T562-lib-database.sh
+++ b/test/T562-lib-database.sh
@@ -298,4 +298,21 @@ A Xapian exception occurred finding/creating a directory: Database has been clos
 EOF
 test_expect_equal_file EXPECTED OUTPUT
 
+test_begin_subtest "find message by filename with a closed db"
+cat c_head - c_tail <<'EOF' | test_C ${MAIL_DIR}
+    {
+        notmuch_message_t *msg;
+        EXPECT0(notmuch_database_close (db));
+        stat = notmuch_database_find_message_by_filename (db, "01:2,", &msg);
+        printf ("%d\n", stat == NOTMUCH_STATUS_XAPIAN_EXCEPTION);
+    }
+EOF
+cat <<EOF > EXPECTED
+== stdout ==
+1
+== stderr ==
+A Xapian exception occurred finding/creating a directory: Database has been closed.
+EOF
+test_expect_equal_file EXPECTED OUTPUT
+
 test_done
-- 
2.27.0

[PATCH 4/8] lib: add regresion test for n_d_get_all_tags

[David Bremner]

Existing error handling seems adequate, if not ideal.
---
 test/T562-lib-database.sh | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/test/T562-lib-database.sh b/test/T562-lib-database.sh
index fea77f78..bb454ea5 100755
--- a/test/T562-lib-database.sh
+++ b/test/T562-lib-database.sh
@@ -315,4 +315,20 @@ A Xapian exception occurred finding/creating a directory: Database has been clos
 EOF
 test_expect_equal_file EXPECTED OUTPUT
 
+test_begin_subtest "Handle getting tags from closed database"
+cat c_head - c_tail <<'EOF' | test_C ${MAIL_DIR}
+    {
+        notmuch_tags_t *result;
+        EXPECT0(notmuch_database_close (db));
+        result = notmuch_database_get_all_tags (db);
+        printf("%d\n",  result == NULL);
+    }
+EOF
+cat <<EOF > EXPECTED
+== stdout ==
+1
+== stderr ==
+EOF
+test_expect_equal_file EXPECTED OUTPUT
+
 test_done
-- 
2.27.0

[PATCH 5/8] test: add regression test for n_d_get_config

[David Bremner]

Xapian exceptions seem to handled OK, at least for this case.
---
 test/T562-lib-database.sh | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/test/T562-lib-database.sh b/test/T562-lib-database.sh
index bb454ea5..20b7eca6 100755
--- a/test/T562-lib-database.sh
+++ b/test/T562-lib-database.sh
@@ -331,4 +331,20 @@ cat <<EOF > EXPECTED
 EOF
 test_expect_equal_file EXPECTED OUTPUT
 
+test_begin_subtest "get config from closed database"
+cat c_head - c_tail <<'EOF' | test_C ${MAIL_DIR}
+    {
+        const char *result;
+        EXPECT0(notmuch_database_close (db));
+        stat = notmuch_database_get_config (db, "foo", &result);
+        printf("%d\n",  stat == NOTMUCH_STATUS_XAPIAN_EXCEPTION);
+    }
+EOF
+cat <<EOF > EXPECTED
+== stdout ==
+1
+== stderr ==
+Error: A Xapian exception occurred getting metadata: Database has been closed
+EOF
+test_expect_equal_file EXPECTED OUTPUT
 test_done
-- 
2.27.0

[PATCH 6/8] test: add known broken test for n_d_set_config

[David Bremner]

Error status is currently lost.
---
 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 20b7eca6..46a1b843 100755
--- a/test/T562-lib-database.sh
+++ b/test/T562-lib-database.sh
@@ -347,4 +347,22 @@ cat <<EOF > EXPECTED
 Error: A Xapian exception occurred getting metadata: Database has been closed
 EOF
 test_expect_equal_file EXPECTED OUTPUT
+
+test_begin_subtest "set config in closed database"
+test_subtest_known_broken
+cat c_head - c_tail <<'EOF' | test_C ${MAIL_DIR}
+    {
+        EXPECT0(notmuch_database_close (db));
+        stat = notmuch_database_set_config (db, "foo", "bar");
+        printf("%d\n",  stat == NOTMUCH_STATUS_XAPIAN_EXCEPTION);
+    }
+EOF
+cat <<EOF > EXPECTED
+== stdout ==
+1
+== stderr ==
+Error: A Xapian exception occurred setting metadata: Database has been closed
+EOF
+test_expect_equal_file EXPECTED OUTPUT
+
 test_done
-- 
2.27.0

[PATCH 7/8] lib: fix error return bug with n_d_set_config.

[David Bremner]

The catch block either needs to return, or the function needs to
return "status". Choose the latter for consistency with
n_d_get_config.
---
 lib/config.cc             | 2 +-
 test/T562-lib-database.sh | 1 -
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/lib/config.cc b/lib/config.cc
index 292f0288..20036471 100644
--- a/lib/config.cc
+++ b/lib/config.cc
@@ -60,7 +60,7 @@ notmuch_database_set_config (notmuch_database_t *notmuch,
 	_notmuch_database_log (notmuch, "Error: A Xapian exception occurred setting metadata: %s\n",
 			       error.get_msg ().c_str ());
     }
-    return NOTMUCH_STATUS_SUCCESS;
+    return status;
 }
 
 static notmuch_status_t
diff --git a/test/T562-lib-database.sh b/test/T562-lib-database.sh
index 46a1b843..81d5c540 100755
--- a/test/T562-lib-database.sh
+++ b/test/T562-lib-database.sh
@@ -349,7 +349,6 @@ EOF
 test_expect_equal_file EXPECTED OUTPUT
 
 test_begin_subtest "set config in closed database"
-test_subtest_known_broken
 cat c_head - c_tail <<'EOF' | test_C ${MAIL_DIR}
     {
         EXPECT0(notmuch_database_close (db));
-- 
2.27.0

[PATCH 8/8] test: add known broken test for n_d_get_default_indexopts

[David Bremner]

Xapian exceptions are swallowed and turned into default return value.
---
 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 81d5c540..a3ecd9ba 100755
--- a/test/T562-lib-database.sh
+++ b/test/T562-lib-database.sh
@@ -364,4 +364,21 @@ Error: A Xapian exception occurred setting metadata: Database has been closed
 EOF
 test_expect_equal_file EXPECTED OUTPUT
 
+test_begin_subtest "get indexopts from closed database"
+test_subtest_known_broken
+cat c_head - c_tail <<'EOF' | test_C ${MAIL_DIR}
+    {
+        notmuch_indexopts_t *result;
+        EXPECT0(notmuch_database_close (db));
+        result = notmuch_database_get_default_indexopts (db);
+        printf("%d\n",  result == NULL);
+    }
+EOF
+cat <<EOF > EXPECTED
+== stdout ==
+1
+== stderr ==
+EOF
+test_expect_equal_file EXPECTED OUTPUT
+
 test_done
-- 
2.27.0

Re: [PATCH 8/8] test: add known broken test for n_d_get_default_indexopts

[David Bremner]

David Bremner <david@tethera.net> writes:

> Xapian exceptions are swallowed and turned into default return value.

There is not much guidance in the API docs, so I'm hoping Daniel can
clarify what the desired behaviour is when a Xapian exception is
detected in notmuch_database_get_config.  The choice in many places in
the API would be to return NULL.

d

[PATCH] lib: return NULL from n_d_get_default_indexopts on error

[David Bremner]

This is a rare and probably serious programming error, so better not
to silently return a default value.
---
 lib/indexopts.c           | 2 +-
 lib/notmuch.h             | 1 +
 test/T562-lib-database.sh | 1 -
 3 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/indexopts.c b/lib/indexopts.c
index 1e8d180e..82a0026f 100644
--- a/lib/indexopts.c
+++ b/lib/indexopts.c
@@ -32,7 +32,7 @@ notmuch_database_get_default_indexopts (notmuch_database_t *db)
     char *decrypt_policy;
     notmuch_status_t err = notmuch_database_get_config (db, "index.decrypt", &decrypt_policy);
     if (err)
-	return ret;
+	return NULL;
 
     if (decrypt_policy) {
 	if ((! (strcasecmp (decrypt_policy, "true"))) ||
diff --git a/lib/notmuch.h b/lib/notmuch.h
index 1fe62914..f3cb0fe2 100644
--- a/lib/notmuch.h
+++ b/lib/notmuch.h
@@ -2325,6 +2325,7 @@ notmuch_config_list_destroy (notmuch_config_list_t *config_list);
  * added to the index.  At the moment it is a featureless stub.
  *
  * @since libnotmuch 5.1 (notmuch 0.26)
+ * @retval NULL in case of error
  */
 notmuch_indexopts_t *
 notmuch_database_get_default_indexopts (notmuch_database_t *db);
diff --git a/test/T562-lib-database.sh b/test/T562-lib-database.sh
index 815c9856..d6097418 100755
--- a/test/T562-lib-database.sh
+++ b/test/T562-lib-database.sh
@@ -373,7 +373,6 @@ EOF
 test_expect_equal_file EXPECTED OUTPUT
 
 test_begin_subtest "get indexopts from closed database"
-test_subtest_known_broken
 cat c_head - c_tail <<'EOF' | test_C ${MAIL_DIR}
     {
         notmuch_indexopts_t *result;
-- 
2.27.0

Re: batch 7, API exception handline cleanup.

[David Bremner]

David Bremner <david@tethera.net> writes:

> These follow the series
>
>       id:20200718150751.4106125-1-david@tethera.net
>
> Probably the most interesting change is
>
>          [PATCH 7/8] lib: fix error return bug with n_d_set_config.
>

batch applied to master.

d