Date: Wed, 4 Dec 2013 11:31:32 -0500
From: Austin Clements <amdragon@MIT.EDU>
To: Jani Nikula <jani@nikula.org>
Subject: Re: [PATCH 1/2] lib: add return status to database close and destroy
Message-ID: <20131204163132.GC8854@mit.edu>
References: <cover.1385903109.git.jani@nikula.org>
	<29b808bb6bf051fe21b6a72f12bb4ad1badfbf97.1385903109.git.jani@nikula.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To:
 <29b808bb6bf051fe21b6a72f12bb4ad1badfbf97.1385903109.git.jani@nikula.org>
User-Agent: Mutt/1.5.21 (2010-09-15)
Cc: notmuch@notmuchmail.org
Precedence: list

Quoth Jani Nikula on Dec 01 at  3:13 pm:
> 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.
> 
> Bump the notmuch version to allow clients to conditional build against
> both the current release and the next release (current git master).
> ---
>  lib/database.cc | 18 ++++++++++++++----
>  lib/notmuch.h   | 17 ++++++++++++++---
>  2 files changed, 28 insertions(+), 7 deletions(-)
> 
> diff --git a/lib/database.cc b/lib/database.cc
> index f395061..98e2c31 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());
> @@ -789,6 +792,7 @@ notmuch_database_close (notmuch_database_t *notmuch)
>  	    notmuch->xapian_db->close();
>  	} catch (const Xapian::Error &error) {
>  	    /* do nothing */
> +	    status = NOTMUCH_STATUS_XAPIAN_EXCEPTION;
>  	}
>      }
>  
> @@ -802,6 +806,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
> @@ -966,7 +972,7 @@ notmuch_database_compact (const char *path,
>  
>    DONE:
>      if (notmuch)
> -	notmuch_database_destroy (notmuch);
> +	ret = notmuch_database_destroy (notmuch);

This will clobber the error code on most of the error paths.  Maybe
you want to only set ret if it's currently NOTMUCH_STATUS_SUCCESS?

>  
>      talloc_free (local);
>  
> @@ -984,11 +990,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 7c3a30c..dbdce86 100644
> --- a/lib/notmuch.h
> +++ b/lib/notmuch.h
> @@ -42,7 +42,7 @@ NOTMUCH_BEGIN_DECLS
>  #endif
>  
>  #define NOTMUCH_MAJOR_VERSION	0
> -#define NOTMUCH_MINOR_VERSION	17
> +#define NOTMUCH_MINOR_VERSION	18
>  #define NOTMUCH_MICRO_VERSION	0

This is actually what got me thinking about
id:1386173986-9624-1-git-send-email-amdragon@mit.edu (which obviously
conflicts).  In particular, the Python bindings don't have access to
these macros, and can only use the soname version.  (I think the Go
ones can use the macros; the Ruby ones certainly can.)

>  
>  /*
> @@ -236,8 +236,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);
>  
>  /* A callback invoked by notmuch_database_compact to notify the user
> @@ -263,8 +271,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);
>  
>  /* Return the database path of the given database.

Regarding bindings (since you asked me to think about them), these
should be a safe changes from an ABI perspective (though obviously the
bindings will need changes to take advantage of the new return code).