batch 5, API exception handling cleanup

batch 5, API exception handling cleanup

[David Bremner]

This is a followup and extension to

     id:20200709001709.449217-1-david@tethera.net

Since that finished with message.cc, this one starts on database.cc

There is one non-boring patch here, namely

      [PATCH 3/4] lib: move deallocation of memory from n_d_close to n_d_destroy

It (of course) passes the test suite, but it represents a non-trivial
behaviour change for the few people calling notmuch_database_close
directly rather than via notmuch_database_destroy

[PATCH 1/4] test: regression tests for n_d_status_string and n_d_get_path

[David Bremner]

These do not crash on a closed database, and we want to keep it that
way.

Start a new file of tests as T560-lib-error was starting to get unwieldy.
---
 test/T562-lib-database.sh | 70 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 70 insertions(+)
 create mode 100755 test/T562-lib-database.sh

diff --git a/test/T562-lib-database.sh b/test/T562-lib-database.sh
new file mode 100755
index 00000000..c869341a
--- /dev/null
+++ b/test/T562-lib-database.sh
@@ -0,0 +1,70 @@
+#!/usr/bin/env bash
+test_description="error reporting for library"
+
+. $(dirname "$0")/test-lib.sh || exit 1
+
+add_email_corpus
+
+test_begin_subtest "building database"
+test_expect_success "NOTMUCH_NEW"
+
+cat <<EOF > c_head
+#include <stdio.h>
+#include <notmuch.h>
+#include <notmuch-test.h>
+int main (int argc, char** argv)
+{
+   notmuch_database_t *db;
+   notmuch_status_t stat;
+   char *msg = NULL;
+
+   stat = notmuch_database_open_verbose (argv[1], NOTMUCH_DATABASE_MODE_READ_WRITE, &db, &msg);
+   if (stat != NOTMUCH_STATUS_SUCCESS) {
+     fprintf (stderr, "error opening database: %d %s\n", stat, msg ? msg : "");
+     exit (1);
+   }
+EOF
+
+cat <<'EOF' > c_tail
+   if (stat) {
+       const char *stat_str = notmuch_database_status_string (db);
+       if (stat_str)
+           fputs (stat_str, stderr);
+    }
+
+}
+EOF
+
+test_begin_subtest "get status_string with closed db"
+cat c_head - c_tail <<'EOF' | test_C ${MAIL_DIR}
+    {
+        const char *str;
+        EXPECT0(notmuch_database_close (db));
+        str = notmuch_database_status_string (db);
+        printf("%d\n", str == NULL);
+    }
+EOF
+cat <<EOF > EXPECTED
+== stdout ==
+1
+== stderr ==
+EOF
+test_expect_equal_file EXPECTED OUTPUT
+
+test_begin_subtest "get path with closed db"
+cat c_head - c_tail <<'EOF' | test_C ${MAIL_DIR}
+    {
+        const char *path;
+        EXPECT0(notmuch_database_close (db));
+        path = notmuch_database_get_path (db);
+        printf("%s\n", path);
+    }
+EOF
+cat <<EOF > EXPECTED
+== stdout ==
+MAIL_DIR
+== stderr ==
+EOF
+test_expect_equal_file EXPECTED OUTPUT
+
+test_done
-- 
2.27.0

[PATCH 2/4] test: add known broken test for n_d_get_version on closed db

[David Bremner]

This should not crash, but it does currently.
---
 test/T562-lib-database.sh | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/test/T562-lib-database.sh b/test/T562-lib-database.sh
index c869341a..b8fba7d6 100755
--- a/test/T562-lib-database.sh
+++ b/test/T562-lib-database.sh
@@ -1,5 +1,5 @@
 #!/usr/bin/env bash
-test_description="error reporting for library"
+test_description="notmuch_database_* API"
 
 . $(dirname "$0")/test-lib.sh || exit 1
 
@@ -67,4 +67,21 @@ MAIL_DIR
 EOF
 test_expect_equal_file EXPECTED OUTPUT
 
+test_begin_subtest "get version with closed db"
+test_subtest_known_broken
+cat c_head - c_tail <<'EOF' | test_C ${MAIL_DIR}
+    {
+        unsigned int version;
+        EXPECT0(notmuch_database_close (db));
+        version = notmuch_database_get_version (db);
+        printf ("%u\n", version);
+    }
+EOF
+cat <<EOF > EXPECTED
+== stdout ==
+0
+== stderr ==
+EOF
+test_expect_equal_file EXPECTED OUTPUT
+
 test_done
-- 
2.27.0

[PATCH 3/4] lib: move deallocation of memory from n_d_close to n_d_destroy

[David Bremner]

In order to mimic the "best effort" API of Xapian to provide
information from a closed database when possible, do not
destroy the Xapian database object too early.

Because the pointer to a Xapian database is no longer nulled on close,
introduce a flag to track whether the notmuch database is open or not.
---
 lib/database-private.h |  2 +-
 lib/database.cc        | 35 ++++++++++++++++++++---------------
 2 files changed, 21 insertions(+), 16 deletions(-)

diff --git a/lib/database-private.h b/lib/database-private.h
index 76359007..d2c25313 100644
--- a/lib/database-private.h
+++ b/lib/database-private.h
@@ -194,7 +194,7 @@ struct _notmuch_database {
     /* true if changes have been made in this atomic section */
     bool atomic_dirty;
     Xapian::Database *xapian_db;
-
+    bool open;
     /* Bit mask of features used by this database.  This is a
      * bitwise-OR of NOTMUCH_FEATURE_* values (above). */
     enum _notmuch_features features;
diff --git a/lib/database.cc b/lib/database.cc
index 2f794164..cfb19ccb 100644
--- a/lib/database.cc
+++ b/lib/database.cc
@@ -1076,6 +1076,10 @@ notmuch_database_open_verbose (const char *path,
 	*database = notmuch;
     else
 	talloc_free (notmuch);
+
+    if (notmuch)
+	notmuch->open = true;
+
     return status;
 }
 
@@ -1087,7 +1091,7 @@ notmuch_database_close (notmuch_database_t *notmuch)
     /* Many Xapian objects (and thus notmuch objects) hold references to
      * the database, so merely deleting the database may not suffice to
      * close it.  Thus, we explicitly close it here. */
-    if (notmuch->xapian_db != NULL) {
+    if (notmuch->open) {
 	try {
 	    /* If there's an outstanding transaction, it's unclear if
 	     * closing the Xapian database commits everything up to
@@ -1110,20 +1114,7 @@ notmuch_database_close (notmuch_database_t *notmuch)
 	    }
 	}
     }
-
-    delete notmuch->term_gen;
-    notmuch->term_gen = NULL;
-    delete notmuch->query_parser;
-    notmuch->query_parser = NULL;
-    delete notmuch->xapian_db;
-    notmuch->xapian_db = NULL;
-    delete notmuch->value_range_processor;
-    notmuch->value_range_processor = NULL;
-    delete notmuch->date_range_processor;
-    notmuch->date_range_processor = NULL;
-    delete notmuch->last_mod_range_processor;
-    notmuch->last_mod_range_processor = NULL;
-
+    notmuch->open = false;
     return status;
 }
 
@@ -1336,6 +1327,20 @@ notmuch_database_destroy (notmuch_database_t *notmuch)
     notmuch_status_t status;
 
     status = notmuch_database_close (notmuch);
+
+    delete notmuch->term_gen;
+    notmuch->term_gen = NULL;
+    delete notmuch->query_parser;
+    notmuch->query_parser = NULL;
+    delete notmuch->xapian_db;
+    notmuch->xapian_db = NULL;
+    delete notmuch->value_range_processor;
+    notmuch->value_range_processor = NULL;
+    delete notmuch->date_range_processor;
+    notmuch->date_range_processor = NULL;
+    delete notmuch->last_mod_range_processor;
+    notmuch->last_mod_range_processor = NULL;
+
     talloc_free (notmuch);
 
     return status;
-- 
2.27.0

[PATCH 4/4] lib/n_d_get_version: catch exceptions and clarify the API

[David Bremner]

notmuch_database_get_version previously returned 0 on some errors, but
did not document this. Luckily 0 is not a valid database version.
---
 lib/database.cc           | 19 ++++++++++++++++++-
 lib/notmuch.h             |  3 +++
 test/T562-lib-database.sh |  1 -
 3 files changed, 21 insertions(+), 2 deletions(-)

diff --git a/lib/database.cc b/lib/database.cc
index cfb19ccb..27861970 100644
--- a/lib/database.cc
+++ b/lib/database.cc
@@ -58,6 +58,17 @@ typedef struct {
 #define DB_ACTION Xapian::DB_CREATE_OR_OPEN
 #endif
 
+#define LOG_XAPIAN_EXCEPTION(message, error) _log_xapian_exception (__location__, message, error)
+
+static void
+_log_xapian_exception (const char *where, notmuch_database_t *notmuch,  const Xapian::Error error) {
+    _notmuch_database_log (notmuch,
+			   "A Xapian exception occurred %s accessing %s : %s\n",
+			   where,
+			   error.get_msg ().c_str ());
+    notmuch->exception_reported = true;
+}
+
 /* Here's the current schema for our database (for NOTMUCH_DATABASE_VERSION):
  *
  * We currently have three different types of documents (mail, ghost,
@@ -1360,7 +1371,13 @@ notmuch_database_get_version (notmuch_database_t *notmuch)
     const char *str;
     char *end;
 
-    version_string = notmuch->xapian_db->get_metadata ("version");
+    try {
+	version_string = notmuch->xapian_db->get_metadata ("version");
+    } catch (const Xapian::Error &error) {
+	LOG_XAPIAN_EXCEPTION (notmuch, error);
+	return 0;
+    }
+
     if (version_string.empty ())
 	return 0;
 
diff --git a/lib/notmuch.h b/lib/notmuch.h
index 97ebc17d..7ee0507a 100644
--- a/lib/notmuch.h
+++ b/lib/notmuch.h
@@ -431,6 +431,8 @@ notmuch_database_get_path (notmuch_database_t *database);
 
 /**
  * Return the database format version of the given database.
+ *
+ * @retval 0 on error
  */
 unsigned int
 notmuch_database_get_version (notmuch_database_t *database);
@@ -444,6 +446,7 @@ notmuch_database_get_version (notmuch_database_t *database);
  * fail with NOTMUCH_STATUS_UPGRADE_REQUIRED.  This always returns
  * FALSE for a read-only database because there's no way to upgrade a
  * read-only 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 b8fba7d6..c9705b13 100755
--- a/test/T562-lib-database.sh
+++ b/test/T562-lib-database.sh
@@ -68,7 +68,6 @@ EOF
 test_expect_equal_file EXPECTED OUTPUT
 
 test_begin_subtest "get version with closed db"
-test_subtest_known_broken
 cat c_head - c_tail <<'EOF' | test_C ${MAIL_DIR}
     {
         unsigned int version;
-- 
2.27.0

Re: [PATCH 4/4] lib/n_d_get_version: catch exceptions and clarify the API

[David Bremner]

David Bremner <david@tethera.net> writes:

> notmuch_database_get_version previously returned 0 on some errors, but
> did not document this. Luckily 0 is not a valid database version.
> +static void
> +_log_xapian_exception (const char *where, notmuch_database_t *notmuch,  const Xapian::Error error) {
> +    _notmuch_database_log (notmuch,
> +			   "A Xapian exception occurred %s accessing %s : %s\n",
> +			   where,
> +			   error.get_msg ().c_str ());
> +    notmuch->exception_reported = true;
> +}
> +

I realized this has the same extra '%s' as fixed in

  id:20200722120905.472562-2-david@tethera.net

for message.cc. So this patch will need to be updated, and possibly some
of the tests as well.

Re: [PATCH 4/4] lib/n_d_get_version: catch exceptions and clarify the API

[David Bremner]

David Bremner <david@tethera.net> writes:

> David Bremner <david@tethera.net> writes:
>
>> notmuch_database_get_version previously returned 0 on some errors, but
>> did not document this. Luckily 0 is not a valid database version.
>> +static void
>> +_log_xapian_exception (const char *where, notmuch_database_t *notmuch,  const Xapian::Error error) {
>> +    _notmuch_database_log (notmuch,
>> +			   "A Xapian exception occurred %s accessing %s : %s\n",
>> +			   where,
>> +			   error.get_msg ().c_str ());
>> +    notmuch->exception_reported = true;
>> +}
>> +
>
> I realized this has the same extra '%s' as fixed in
>
>   id:20200722120905.472562-2-david@tethera.net
>
> for message.cc. So this patch will need to be updated, and possibly some
> of the tests as well.

updated version applied to master.

d