incrementally improve error handling in notmuch_message*

incrementally improve error handling in notmuch_message*

[David Bremner]

There is more that can be done, but this is a start. The series
introduces a new test primitive test_private_C, for directly unit
testing non-exported functions, and new macro NODISCARD to get the
compiler to help tracking down places a particular function's return
value is discarded.

As a bonus, drop some dead code.

[PATCH 1/8] test: define test_private_C

[David Bremner]

When testing error handling, it is sometimes difficult to cover a
particular error path deterministically. Introduce a test function to
allow calling lower level functions directly.
---
 configure        | 16 ++++++++++++++++
 test/test-lib.sh | 13 +++++++++++++
 2 files changed, 29 insertions(+)

diff --git a/configure b/configure
index 30fee6ab..5247e05a 100755
--- a/configure
+++ b/configure
@@ -1566,9 +1566,17 @@ cat > sh.config <<EOF
 
 NOTMUCH_SRCDIR='${NOTMUCH_SRCDIR}'
 
+# Flags needed to compile and link against Xapian
+NOTMUCH_XAPIAN_CXXFLAGS="${xapian_cxxflags}"
+NOTMUCH_XAPIAN_LDFLAGS="${xapian_ldflags}"
+
 # Whether to have Xapian retry lock
 NOTMUCH_HAVE_XAPIAN_DB_RETRY_LOCK=${WITH_RETRY_LOCK}
 
+# Flags needed to compile and link against GMime
+NOTMUCH_GMIME_CFLAGS="${gmime_cflags}"
+NOTMUCH_GMIME_LDFLAGS="${gmime_ldflags}"
+
 # Whether GMime can verify X.509 certificate validity
 NOTMUCH_GMIME_X509_CERT_VALIDITY=${gmime_x509_cert_validity}
 
@@ -1578,6 +1586,10 @@ NOTMUCH_GMIME_EMITS_ANGLE_BRACKETS=${gmime_emits_angle_brackets}
 # Whether GMime can verify signatures when decrypting with a session key:
 NOTMUCH_GMIME_VERIFY_WITH_SESSION_KEY=${gmime_verify_with_session_key}
 
+# Flags needed to compile and link against zlib
+NOTMUCH_ZLIB_CFLAGS="${zlib_cflags}"
+NOTMUCH_ZLIB_LDFLAGS="${zlib_ldflags}"
+
 # Does the C compiler support the address sanitizer
 NOTMUCH_HAVE_ASAN=${have_asan}
 
@@ -1614,6 +1626,10 @@ NOTMUCH_HAVE_PYTHON3_PYTEST=${have_python3_pytest}
 # Is the sfsexp library available?
 NOTMUCH_HAVE_SFSEXP=${have_sfsexp}
 
+# And if so, flags needed at compile/link time for sfsexp
+NOTMUCH_SFSEXP_CFLAGS="${sfsexp_cflags}"
+NOTMUCH_SFSEXP_LDFLAGS="${sfsexp_ldflags}"
+
 # Platform we are run on
 PLATFORM=${platform}
 EOF
diff --git a/test/test-lib.sh b/test/test-lib.sh
index 59b6079d..75a62214 100644
--- a/test/test-lib.sh
+++ b/test/test-lib.sh
@@ -872,6 +872,19 @@ test_C () {
     notmuch_dir_sanitize OUTPUT.stdout OUTPUT.stderr | notmuch_exception_sanitize | notmuch_debug_sanitize > OUTPUT
 }
 
+test_private_C () {
+    local exec_file test_file
+    exec_file="test${test_count}"
+    test_file="${exec_file}.c"
+    echo '#include <notmuch-private.h>' > ${test_file}
+    cat >> ${test_file}
+    ${TEST_CC} ${TEST_CFLAGS} -I${NOTMUCH_SRCDIR}/test -I${NOTMUCH_SRCDIR}/lib -I${NOTMUCH_SRCDIR}/util -I${NOTMUCH_SRCDIR}/compat ${NOTMUCH_GMIME_CFLAGS} -o ${exec_file} ${test_file} ${NOTMUCH_BUILDDIR}/lib/libnotmuch.a ${NOTMUCH_GMIME_LDFLAGS} ${NOTMUCH_XAPIAN_LDFLAGS} ${NOTMUCH_BUILDDIR}/util/libnotmuch_util.a ${NOTMUCH_SFSEXP_LDFLAGS} ${NOTMUCH_BUILDDIR}/parse-time-string/libparse-time-string.a -ltalloc -lstdc++
+    echo "== stdout ==" > OUTPUT.stdout
+    echo "== stderr ==" > OUTPUT.stderr
+    ./${exec_file} "$@" 1>>OUTPUT.stdout 2>>OUTPUT.stderr
+    notmuch_dir_sanitize OUTPUT.stdout OUTPUT.stderr | notmuch_exception_sanitize | notmuch_debug_sanitize > OUTPUT
+}
+
 make_shim () {
     local base_name test_file shim_file
     base_name="$1"
-- 
2.35.2

[PATCH 2/8] lib/message: catch exceptions in _n_m_add_term

[David Bremner]

Some code movement is needed to make sure the cache is only
invalidated when the Xapian operation succeeds.
---
 lib/message.cc           | 27 +++++++++++++++++----------
 test/T566-lib-message.sh | 17 +++++++++++++++++
 2 files changed, 34 insertions(+), 10 deletions(-)

diff --git a/lib/message.cc b/lib/message.cc
index 63b216b6..bf3a3419 100644
--- a/lib/message.cc
+++ b/lib/message.cc
@@ -1489,24 +1489,30 @@ _notmuch_message_add_term (notmuch_message_t *message,
 {
 
     char *term;
+    notmuch_private_status_t status = NOTMUCH_PRIVATE_STATUS_SUCCESS;
 
     if (value == NULL)
 	return NOTMUCH_PRIVATE_STATUS_NULL_POINTER;
 
     term = talloc_asprintf (message, "%s%s",
 			    _find_prefix (prefix_name), value);
+    if (strlen (term) > NOTMUCH_TERM_MAX) {
+	status = NOTMUCH_PRIVATE_STATUS_TERM_TOO_LONG;
+	goto DONE;
+    }
 
-    if (strlen (term) > NOTMUCH_TERM_MAX)
-	return NOTMUCH_PRIVATE_STATUS_TERM_TOO_LONG;
-
-    message->doc.add_term (term, 0);
-    message->modified = true;
+    try {
+	message->doc.add_term (term, 0);
+	message->modified = true;
+	_notmuch_message_invalidate_metadata (message, prefix_name);
+    } catch (Xapian::Error &error) {
+	LOG_XAPIAN_EXCEPTION (message, error);
+	status = NOTMUCH_PRIVATE_STATUS_XAPIAN_EXCEPTION;
+    }
 
+  DONE:
     talloc_free (term);
-
-    _notmuch_message_invalidate_metadata (message, prefix_name);
-
-    return NOTMUCH_PRIVATE_STATUS_SUCCESS;
+    return status;
 }
 
 /* Parse 'text' and add a term to 'message' for each parsed word. Each
@@ -1570,11 +1576,12 @@ _notmuch_message_remove_term (notmuch_message_t *message,
     try {
 	message->doc.remove_term (term);
 	message->modified = true;
-    } catch (const Xapian::InvalidArgumentError) {
+    } catch (const Xapian::InvalidArgumentError &error) {
 	/* We'll let the philosophers try to wrestle with the
 	 * question of whether failing to remove that which was not
 	 * there in the first place is failure. For us, we'll silently
 	 * consider it all good. */
+	LOG_XAPIAN_EXCEPTION (message, error);
     }
 
     talloc_free (term);
diff --git a/test/T566-lib-message.sh b/test/T566-lib-message.sh
index 8b61d182..08ab2765 100755
--- a/test/T566-lib-message.sh
+++ b/test/T566-lib-message.sh
@@ -305,6 +305,23 @@ cat <<EOF > EXPECTED
 EOF
 test_expect_equal_file EXPECTED OUTPUT
 
+test_begin_subtest "_notmuch_message_add_term catches exceptions"
+cat c_head0 - c_tail <<'EOF' | test_private_C ${MAIL_DIR}
+    {
+	notmuch_private_status_t status;
+	/* This relies on Xapian throwing an exception for adding empty terms */
+	status = _notmuch_message_add_term (message, "body", "");
+	printf("%d\n%d\n", message != NULL, status != NOTMUCH_STATUS_SUCCESS );
+    }
+EOF
+cat <<EOF > EXPECTED
+== stdout ==
+1
+1
+== stderr ==
+EOF
+test_expect_equal_file EXPECTED OUTPUT
+
 test_begin_subtest "Handle removing all tags with closed db"
 cat c_head - c_tail <<'EOF' | test_C ${MAIL_DIR}
     {
-- 
2.35.2

[PATCH 3/8] test: _notmuch_message_remove_term catches exceptions.

[David Bremner]

Unfortunately we can't differentiate between the two distinct error
conditions for Document::remove_term.
---
 test/T566-lib-message.sh | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/test/T566-lib-message.sh b/test/T566-lib-message.sh
index 08ab2765..5b6c937e 100755
--- a/test/T566-lib-message.sh
+++ b/test/T566-lib-message.sh
@@ -322,6 +322,24 @@ cat <<EOF > EXPECTED
 EOF
 test_expect_equal_file EXPECTED OUTPUT
 
+test_begin_subtest "_notmuch_message_remove_term catches exceptions"
+cat c_head0 - c_tail <<'EOF' | test_private_C ${MAIL_DIR}
+    {
+	notmuch_private_status_t status;
+	/* Xapian throws the same exception for empty and non-existent terms;
+	 * error string varies between Xapian versions. */
+	status = _notmuch_message_remove_term (message, "tag", "nonexistent");
+	printf("%d\n%d\n", message != NULL, status == NOTMUCH_STATUS_SUCCESS );
+    }
+EOF
+cat <<EOF > EXPECTED
+== stdout ==
+1
+1
+== stderr ==
+EOF
+test_expect_equal_file EXPECTED OUTPUT
+
 test_begin_subtest "Handle removing all tags with closed db"
 cat c_head - c_tail <<'EOF' | test_C ${MAIL_DIR}
     {
-- 
2.35.2

[PATCH 4/8] lib/message: drop _notmuch_message_get_thread_id_only

[David Bremner]

This function has been unused since commit 4083fd8.
---
 lib/message.cc        | 17 -----------------
 lib/notmuch-private.h |  3 ---
 2 files changed, 20 deletions(-)

diff --git a/lib/message.cc b/lib/message.cc
index bf3a3419..09e5660b 100644
--- a/lib/message.cc
+++ b/lib/message.cc
@@ -340,23 +340,6 @@ _notmuch_message_get_term (notmuch_message_t *message,
     return value;
 }
 
-/*
- * For special applications where we only want the thread id, reading
- * in all metadata is a heavy I/O penalty.
- */
-const char *
-_notmuch_message_get_thread_id_only (notmuch_message_t *message)
-{
-
-    Xapian::TermIterator i = message->doc.termlist_begin ();
-    Xapian::TermIterator end = message->doc.termlist_end ();
-
-    message->thread_id = _notmuch_message_get_term (message, i, end,
-						    _find_prefix ("thread"));
-    return message->thread_id;
-}
-
-
 static void
 _notmuch_message_ensure_metadata (notmuch_message_t *message, void *field)
 {
diff --git a/lib/notmuch-private.h b/lib/notmuch-private.h
index 3cc79bc4..c63cfe5e 100644
--- a/lib/notmuch-private.h
+++ b/lib/notmuch-private.h
@@ -586,9 +586,6 @@ _notmuch_message_add_reply (notmuch_message_t *message,
 void
 _notmuch_message_remove_unprefixed_terms (notmuch_message_t *message);
 
-const char *
-_notmuch_message_get_thread_id_only (notmuch_message_t *message);
-
 size_t _notmuch_message_get_thread_depth (notmuch_message_t *message);
 
 void
-- 
2.35.2

[PATCH 5/8] lib: define macro NODISCARD

[David Bremner]

In either C++17 (or later) mode, or when running cppcheck, this can be
used to selectively generate warnings about discarded return values.
---
 lib/notmuch-private.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/lib/notmuch-private.h b/lib/notmuch-private.h
index c63cfe5e..1c2290b2 100644
--- a/lib/notmuch-private.h
+++ b/lib/notmuch-private.h
@@ -750,6 +750,12 @@ _notmuch_talloc_steal (const void *new_ctx, const T *ptr)
 #undef talloc_steal
 #define talloc_steal _notmuch_talloc_steal
 #endif
+
+#if __cplusplus >= 201703L || __cppcheck__
+#define NODISCARD [[nodiscard]]
+#else
+#define NODISCARD /**/
+#endif
 #endif
 
 #endif
-- 
2.35.2

[PATCH 6/8] lib/message: check return status of _n_m_{add,remove}_term

[David Bremner]

Xapian exceptions are not something that can be ignored, in general.
---
 lib/message.cc | 43 ++++++++++++++++++++++++++++++++++---------
 1 file changed, 34 insertions(+), 9 deletions(-)

diff --git a/lib/message.cc b/lib/message.cc
index 09e5660b..94be0f76 100644
--- a/lib/message.cc
+++ b/lib/message.cc
@@ -827,7 +827,10 @@ _notmuch_message_add_folder_terms (notmuch_message_t *message,
 	*folder = '\0';
     }
 
-    _notmuch_message_add_term (message, "folder", folder);
+    if (notmuch_status_t status = COERCE_STATUS (_notmuch_message_add_term (message, "folder",
+									    folder),
+						 "adding folder term"))
+	return status;
 
     talloc_free (folder);
 
@@ -842,8 +845,13 @@ static notmuch_status_t
 _notmuch_message_add_path_terms (notmuch_message_t *message,
 				 const char *directory)
 {
+    notmuch_status_t status;
+
     /* Add exact "path:" term. */
-    _notmuch_message_add_term (message, "path", directory);
+    status = COERCE_STATUS (_notmuch_message_add_term (message, "path", directory),
+			    "adding path term");
+    if (status)
+	return status;
 
     if (strlen (directory)) {
 	char *path, *p;
@@ -856,7 +864,10 @@ _notmuch_message_add_path_terms (notmuch_message_t *message,
 	for (p = path + strlen (path) - 1; p > path; p--) {
 	    if (*p == '/') {
 		strcpy (p, RECURSIVE_SUFFIX);
-		_notmuch_message_add_term (message, "path", path);
+		status = COERCE_STATUS (_notmuch_message_add_term (message, "path", path),
+					"adding path term");
+		if (status)
+		    return status;
 	    }
 	}
 
@@ -864,7 +875,10 @@ _notmuch_message_add_path_terms (notmuch_message_t *message,
     }
 
     /* Recursive all-matching path:** for consistency. */
-    _notmuch_message_add_term (message, "path", "**");
+    status = COERCE_STATUS (_notmuch_message_add_term (message, "path", "**"),
+			    "adding path term");
+    if (status)
+	return status;
 
     return NOTMUCH_STATUS_SUCCESS;
 }
@@ -943,7 +957,10 @@ _notmuch_message_add_filename (notmuch_message_t *message,
 
     /* New file-direntry allows navigating to this message with
      * notmuch_directory_get_child_files() . */
-    _notmuch_message_add_term (message, "file-direntry", direntry);
+    status = COERCE_STATUS (_notmuch_message_add_term (message, "file-direntry", direntry),
+			    "adding file-direntry term");
+    if (status)
+	return status;
 
     _notmuch_message_add_folder_terms (message, directory);
     _notmuch_message_add_path_terms (message, directory);
@@ -1465,7 +1482,7 @@ _notmuch_message_close (notmuch_message_t *message)
  *
  * This change will not be reflected in the database until the next
  * call to _notmuch_message_sync. */
-notmuch_private_status_t
+NODISCARD notmuch_private_status_t
 _notmuch_message_add_term (notmuch_message_t *message,
 			   const char *prefix_name,
 			   const char *value)
@@ -1540,7 +1557,7 @@ _notmuch_message_gen_terms (notmuch_message_t *message,
  *
  * This change will not be reflected in the database until the next
  * call to _notmuch_message_sync. */
-notmuch_private_status_t
+NODISCARD notmuch_private_status_t
 _notmuch_message_remove_term (notmuch_message_t *message,
 			      const char *prefix_name,
 			      const char *value)
@@ -2273,7 +2290,11 @@ notmuch_message_reindex (notmuch_message_t *message,
 	if (thread_id == NULL)
 	    thread_id = orig_thread_id;
 
-	_notmuch_message_add_term (message, "thread", thread_id);
+	ret = COERCE_STATUS (_notmuch_message_add_term (message, "thread", thread_id),
+			     "adding thread term");
+	if (ret)
+	    goto DONE;
+
 	/* Take header values only from first filename */
 	if (found == 0)
 	    _notmuch_message_set_header_values (message, date, from, subject);
@@ -2291,7 +2312,11 @@ notmuch_message_reindex (notmuch_message_t *message,
     }
     if (found == 0) {
 	/* put back thread id to help cleanup */
-	_notmuch_message_add_term (message, "thread", orig_thread_id);
+	ret = COERCE_STATUS (_notmuch_message_add_term (message, "thread", orig_thread_id),
+			     "adding thread term");
+	if (ret)
+	    goto DONE;
+
 	ret = _notmuch_message_delete (message);
     } else {
 	_notmuch_message_sync (message);
-- 
2.35.2

[PATCH 7/8] lib/message: check return status from _n_m_add_{path,folder}_terms

[David Bremner]

Mainly to propagate information about Xapian exceptions.
---
 lib/message.cc | 23 +++++++++++++++++------
 1 file changed, 17 insertions(+), 6 deletions(-)

diff --git a/lib/message.cc b/lib/message.cc
index 94be0f76..ae152df7 100644
--- a/lib/message.cc
+++ b/lib/message.cc
@@ -791,7 +791,7 @@ is_maildir (const char *p)
 }
 
 /* Add "folder:" term for directory. */
-static notmuch_status_t
+NODISCARD static notmuch_status_t
 _notmuch_message_add_folder_terms (notmuch_message_t *message,
 				   const char *directory)
 {
@@ -841,7 +841,7 @@ _notmuch_message_add_folder_terms (notmuch_message_t *message,
 #define RECURSIVE_SUFFIX "/**"
 
 /* Add "path:" terms for directory. */
-static notmuch_status_t
+NODISCARD static notmuch_status_t
 _notmuch_message_add_path_terms (notmuch_message_t *message,
 				 const char *directory)
 {
@@ -897,6 +897,7 @@ _notmuch_message_add_directory_terms (void *ctx, notmuch_message_t *message)
 	const char *direntry, *directory;
 	char *colon;
 	const std::string &term = *i;
+	notmuch_status_t term_status;
 
 	/* Terminate loop at first term without desired prefix. */
 	if (strncmp (term.c_str (), direntry_prefix, direntry_prefix_len))
@@ -917,8 +918,13 @@ _notmuch_message_add_directory_terms (void *ctx, notmuch_message_t *message)
 							  message->notmuch,
 							  directory_id);
 
-	_notmuch_message_add_folder_terms (message, directory);
-	_notmuch_message_add_path_terms (message, directory);
+	term_status = _notmuch_message_add_folder_terms (message, directory);
+	if (term_status)
+	    return term_status;
+
+	term_status = _notmuch_message_add_path_terms (message, directory);
+	if (term_status)
+	    return term_status;
     }
 
     return status;
@@ -962,8 +968,13 @@ _notmuch_message_add_filename (notmuch_message_t *message,
     if (status)
 	return status;
 
-    _notmuch_message_add_folder_terms (message, directory);
-    _notmuch_message_add_path_terms (message, directory);
+    status = _notmuch_message_add_folder_terms (message, directory);
+    if (status)
+	return status;
+
+    status = _notmuch_message_add_path_terms (message, directory);
+    if (status)
+	return status;
 
     talloc_free (local);
 
-- 
2.35.2

[PATCH 8/8] test: error handling in _n_message_{add,remove}_filename

[David Bremner]

Use a closed database to force throwing a Xapian exception. Leave the
check vague to accomodate a potential explicit check for open database.
---
 test/T566-lib-message.sh | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/test/T566-lib-message.sh b/test/T566-lib-message.sh
index 5b6c937e..30040634 100755
--- a/test/T566-lib-message.sh
+++ b/test/T566-lib-message.sh
@@ -340,6 +340,38 @@ cat <<EOF > EXPECTED
 EOF
 test_expect_equal_file EXPECTED OUTPUT
 
+test_begin_subtest "_notmuch_message_add_filename on closed db"
+cat c_head - c_tail <<'EOF' | test_private_C ${MAIL_DIR}
+    {
+	notmuch_private_status_t status;
+	status = _notmuch_message_add_filename (message, "some-filename");
+	printf("%d\n%d\n", message != NULL, status != NOTMUCH_STATUS_SUCCESS);
+    }
+EOF
+cat <<EOF > EXPECTED
+== stdout ==
+1
+1
+== stderr ==
+EOF
+test_expect_equal_file EXPECTED OUTPUT
+
+test_begin_subtest "_notmuch_message_remove_filename on closed db"
+cat c_head - c_tail <<'EOF' | test_private_C ${MAIL_DIR}
+    {
+	notmuch_private_status_t status;
+	status = _notmuch_message_remove_filename (message, "some-filename");
+	printf("%d\n%d\n", message != NULL, status != NOTMUCH_STATUS_SUCCESS);
+    }
+EOF
+cat <<EOF > EXPECTED
+== stdout ==
+1
+1
+== stderr ==
+EOF
+test_expect_equal_file EXPECTED OUTPUT
+
 test_begin_subtest "Handle removing all tags with closed db"
 cat c_head - c_tail <<'EOF' | test_C ${MAIL_DIR}
     {
-- 
2.35.2

Re: incrementally improve error handling in notmuch_message*

[David Bremner]

David Bremner <david@tethera.net> writes:

> There is more that can be done, but this is a start. The series
> introduces a new test primitive test_private_C, for directly unit
> testing non-exported functions, and new macro NODISCARD to get the
> compiler to help tracking down places a particular function's return
> value is discarded.
>
> As a bonus, drop some dead code.
>

Series applied to master.