Date: Sun, 22 Apr 2012 14:01:59 -0400
From: Austin Clements <amdragon@MIT.EDU>
To: Justus Winter <4winter@informatik.uni-hamburg.de>
Subject: Re: [PATCH 1/7] Split notmuch_database_close into two functions
Message-ID: <20120422180159.GG17810@mit.edu>
References: <20120422120620.26225.40778@thinkbox.jade-hamburg.de>
	<1335096477-27537-1-git-send-email-4winter@informatik.uni-hamburg.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To:
 <1335096477-27537-1-git-send-email-4winter@informatik.uni-hamburg.de>
User-Agent: Mutt/1.5.21 (2010-09-15)
Cc: notmuch@notmuchmail.org
Precedence: list

Quoth Justus Winter on Apr 22 at  2:07 pm:
> Formerly notmuch_database_close closed the xapian database and
> destroyed the talloc structure associated with the notmuch database
> object. Split notmuch_database_close into notmuch_database_close and
> notmuch_database_destroy.
> 
> This makes it possible for long running programs to close the xapian
> database and thus release the lock associated with it without
> destroying the data structures obtained from it.
> 
> This also makes the api more consistent since every other data
> structure has a destructor function.
> 
> Signed-off-by: Justus Winter <4winter@informatik.uni-hamburg.de>
> ---
>  lib/database.cc |   14 ++++++++++++--
>  lib/notmuch.h   |   15 +++++++++++----
>  2 files changed, 23 insertions(+), 6 deletions(-)
> 
> diff --git a/lib/database.cc b/lib/database.cc
> index 16c4354..2fefcad 100644
> --- a/lib/database.cc
> +++ b/lib/database.cc
> @@ -642,7 +642,7 @@ notmuch_database_open (const char *path,
>  			 "       read-write mode.\n",
>  			 notmuch_path, version, NOTMUCH_DATABASE_VERSION);
>  		notmuch->mode = NOTMUCH_DATABASE_MODE_READ_ONLY;
> -		notmuch_database_close (notmuch);
> +		notmuch_database_destroy (notmuch);
>  		notmuch = NULL;
>  		goto DONE;
>  	    }
> @@ -702,7 +702,7 @@ notmuch_database_open (const char *path,
>      } catch (const Xapian::Error &error) {
>  	fprintf (stderr, "A Xapian exception occurred opening database: %s\n",
>  		 error.get_msg().c_str());
> -	notmuch_database_close (notmuch);
> +	notmuch_database_destroy (notmuch);
>  	notmuch = NULL;
>      }
>  
> @@ -738,9 +738,19 @@ 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;
> +}
> +
> +void
> +notmuch_database_destroy (notmuch_database_t *notmuch)
> +{
> +    notmuch_database_close (notmuch);
>      talloc_free (notmuch);
>  }
>  
> diff --git a/lib/notmuch.h b/lib/notmuch.h
> index 673c423..84c9265 100644
> --- a/lib/notmuch.h
> +++ b/lib/notmuch.h
> @@ -133,7 +133,7 @@ typedef struct _notmuch_filenames notmuch_filenames_t;
>   *
>   * After a successful call to notmuch_database_create, the returned
>   * database will be open so the caller should call
> - * notmuch_database_close when finished with it.
> + * notmuch_database_destroy when finished with it.
>   *
>   * The database will not yet have any data in it
>   * (notmuch_database_create itself is a very cheap function). Messages
> @@ -165,7 +165,7 @@ typedef enum {
>   * An existing notmuch database can be identified by the presence of a
>   * directory named ".notmuch" below 'path'.
>   *
> - * The caller should call notmuch_database_close when finished with
> + * The caller should call notmuch_database_destroy when finished with
>   * this database.
>   *
>   * In case of any failure, this function returns NULL, (after printing
> @@ -175,11 +175,18 @@ notmuch_database_t *
>  notmuch_database_open (const char *path,
>  		       notmuch_database_mode_t mode);
>  
> -/* Close the given notmuch database, freeing all associated
> - * resources. See notmuch_database_open. */
> +/* Close the given notmuch database.
> + *
> + * This function is called by notmuch_database_destroy and can be
> + * called multiple times. */

This needs a comment explaining the implications of closing the
database.  Perhaps something like this (modeled on
Xapian::Database::close's documentation)

/* Close the given notmuch database.
 *
 * After notmuch_database_close has been called, calls to other
 * functions on objects derived from this database may either behave
 * as if the database had not been closed (e.g., if the required data
 * has been cached) or may fail with a
 * NOTMUCH_STATUS_XAPIAN_EXCEPTION.
 *
 * notmuch_database_close can be called multiple times.  Later calls
 * have no affect. 
 */

>  void
>  notmuch_database_close (notmuch_database_t *database);
>  
> +/* Destroy the notmuch database freeing all associated
> + * resources */

This should mention that this also closes the database (currently you
mention this in the doc for notmuch_database_close, but it's a reverse
reference there; that could probably even be omitted).  Perhaps

/* Destroy the notmuch database, closing it if necessary and freeing
 * all associated resources. */

> +void
> +notmuch_database_destroy (notmuch_database_t *database);
> +
>  /* Return the database path of the given database.
>   *
>   * The return value is a string owned by notmuch so should not be