[PATCH 1/2] lib: add return status to database close and destroy

[Jani Nikula]

On Wed, 04 Dec 2013, Austin Clements <amdragon at MIT.EDU> wrote:
> 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?

Good point, will fix.

BR,
Jani.


>
>>  
>>      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 at 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).