[PATCH v2] lib: add return status to database close and destroy

[PATCH v2] lib: add return status to database close and destroy

[Jani Nikula]

notmuch_database_close may fail in Xapian ->flush() or ->close(), so
report the status. Similarly for notmuch_database_destroy which calls
close.

This is required for notmuch insert to report error status if message
indexing failed.

---

v2 of
id:29b808bb6bf051fe21b6a72f12bb4ad1badfbf97.1385903109.git.jani@nikula.org
picked out of the series.
---
 lib/database.cc | 30 ++++++++++++++++++++++++------
 lib/notmuch.h   | 15 +++++++++++++--
 2 files changed, 37 insertions(+), 8 deletions(-)

diff --git a/lib/database.cc b/lib/database.cc
index f395061..46a9a8f 100644
--- a/lib/database.cc
+++ b/lib/database.cc
@@ -767,14 +767,17 @@ notmuch_database_open (const char *path,
     return status;
 }
 
-void
+notmuch_status_t
 notmuch_database_close (notmuch_database_t *notmuch)
 {
+    notmuch_status_t status = NOTMUCH_STATUS_SUCCESS;
+
     try {
 	if (notmuch->xapian_db != NULL &&
 	    notmuch->mode == NOTMUCH_DATABASE_MODE_READ_WRITE)
 	    (static_cast <Xapian::WritableDatabase *> (notmuch->xapian_db))->flush ();
     } catch (const Xapian::Error &error) {
+	status = NOTMUCH_STATUS_XAPIAN_EXCEPTION;
 	if (! notmuch->exception_reported) {
 	    fprintf (stderr, "Error: A Xapian exception occurred flushing database: %s\n",
 		     error.get_msg().c_str());
@@ -788,7 +791,9 @@ notmuch_database_close (notmuch_database_t *notmuch)
 	try {
 	    notmuch->xapian_db->close();
 	} catch (const Xapian::Error &error) {
-	    /* do nothing */
+	    /* don't clobber previous error status */
+	    if (status == NOTMUCH_STATUS_SUCCESS)
+		status = NOTMUCH_STATUS_XAPIAN_EXCEPTION;
 	}
     }
 
@@ -802,6 +807,8 @@ notmuch_database_close (notmuch_database_t *notmuch)
     notmuch->value_range_processor = NULL;
     delete notmuch->date_range_processor;
     notmuch->date_range_processor = NULL;
+
+    return status;
 }
 
 #if HAVE_XAPIAN_COMPACT
@@ -965,8 +972,15 @@ notmuch_database_compact (const char *path,
     }
 
   DONE:
-    if (notmuch)
-	notmuch_database_destroy (notmuch);
+    if (notmuch) {
+	notmuch_status_t ret2;
+
+	ret2 = notmuch_database_destroy (notmuch);
+
+	/* don't clobber previous error status */
+	if (ret == NOTMUCH_STATUS_SUCCESS && ret2 != NOTMUCH_STATUS_SUCCESS)
+	    ret = ret2;
+    }
 
     talloc_free (local);
 
@@ -984,11 +998,15 @@ notmuch_database_compact (unused (const char *path),
 }
 #endif
 
-void
+notmuch_status_t
 notmuch_database_destroy (notmuch_database_t *notmuch)
 {
-    notmuch_database_close (notmuch);
+    notmuch_status_t status;
+
+    status = notmuch_database_close (notmuch);
     talloc_free (notmuch);
+
+    return status;
 }
 
 const char *
diff --git a/lib/notmuch.h b/lib/notmuch.h
index 02604c5..7ac7118 100644
--- a/lib/notmuch.h
+++ b/lib/notmuch.h
@@ -287,8 +287,16 @@ notmuch_database_open (const char *path,
  *
  * notmuch_database_close can be called multiple times.  Later calls
  * have no effect.
+ *
+ * Return value:
+ *
+ * NOTMUCH_STATUS_SUCCESS: Successfully closed the database.
+ *
+ * NOTMUCH_STATUS_XAPIAN_EXCEPTION: A Xapian exception occurred; the
+ *	database has been closed but there are no guarantees the
+ *	changes to the database, if any, have been flushed to disk.
  */
-void
+notmuch_status_t
 notmuch_database_close (notmuch_database_t *database);
 
 /**
@@ -317,8 +325,11 @@ notmuch_database_compact (const char* path,
 /**
  * Destroy the notmuch database, closing it if necessary and freeing
  * all associated resources.
+ *
+ * Return value as in notmuch_database_close if the database was open;
+ * notmuch_database_destroy itself has no failure modes.
  */
-void
+notmuch_status_t
 notmuch_database_destroy (notmuch_database_t *database);
 
 /**
-- 
1.8.5.2

Re: [PATCH v2] lib: add return status to database close and destroy

[David Bremner]

Jani Nikula <jani@nikula.org> writes:

> -void
> +notmuch_status_t
>  notmuch_database_close (notmuch_database_t *notmuch)
>  {
> +    notmuch_status_t status = NOTMUCH_STATUS_SUCCESS;
> +
>      try {
>  	if (notmuch->xapian_db != NULL &&
>  	    notmuch->mode == NOTMUCH_DATABASE_MODE_READ_WRITE)
>  	    (static_cast <Xapian::WritableDatabase *> (notmuch->xapian_db))->flush ();
>      } catch (const Xapian::Error &error) {
> +	status = NOTMUCH_STATUS_XAPIAN_EXCEPTION;
>  	if (! notmuch->exception_reported) {

as I mentioned on IRC, I don't really know what this piece of code is
useful for, but that's orthogonal to this patch.

> -void
> +notmuch_status_t
>  notmuch_database_destroy (notmuch_database_t *notmuch)
>  {
> -    notmuch_database_close (notmuch);
> +    notmuch_status_t status;
> +
> +    status = notmuch_database_close (notmuch);
>      talloc_free (notmuch);
> +
> +    return status;
>  }

I guess we're implicitly claiming that talloc_free cannot possibly
return -1 in our use case? perhaps a cast to void and/or a comment would
be suitable.

>  const char *
> diff --git a/lib/notmuch.h b/lib/notmuch.h
> index 02604c5..7ac7118 100644
> --- a/lib/notmuch.h
> +++ b/lib/notmuch.h
> @@ -287,8 +287,16 @@ notmuch_database_open (const char *path,
>   *
>   * notmuch_database_close can be called multiple times.  Later calls
>   * have no effect.

Is it conceivable that the user might wish to retry closing if an
exception occurs? It feels like we ought to have an opinion here.

No functionality complaints...

d