[PATCH 2/2] lib: introduce notmuch_database_new for initializing a database handle

Subject: [PATCH 2/2] lib: introduce notmuch_database_new for initializing a database handle

Date: Sun, 1 Dec 2013 15:14:00 +0200

To: notmuch@notmuchmail.org

Cc:

From: Jani Nikula


There is a need for setting options before opening a database, such as
setting a logging function to use instead of writing to stdout or
stderr. It would be possible to do this by adding new parameters to
notmuch_database_create and notmuch_database_open, but maintaining a
backwards compatible API and ABI when new options are added becomes
burdensome.

Instead, split the opaque database object creation from
notmuch_database_create and notmuch_database_open into a new
notmuch_database_new call, to allow operations on the handle before
create and open. This creates API and ABI breakage now, but allows
easier future extensions.

The notmuch_database_new call becomes a natural pair to the already
existing notmuch_database_destroy, and it should be possible to call
open/close multiple times using an initialized handle.
---
 lib/database.cc      | 64 ++++++++++++++++++++++++++++------------------------
 lib/notmuch.h        | 52 ++++++++++++++++++++++++++++++++----------
 notmuch-compact.c    | 11 ++++++++-
 notmuch-count.c      | 10 ++++++--
 notmuch-dump.c       | 10 ++++++--
 notmuch-insert.c     | 10 ++++++--
 notmuch-new.c        | 14 +++++++-----
 notmuch-reply.c      | 10 ++++++--
 notmuch-restore.c    | 10 ++++++--
 notmuch-search.c     | 10 ++++++--
 notmuch-show.c       | 10 ++++++--
 notmuch-tag.c        | 10 ++++++--
 test/random-corpus.c | 10 ++++++--
 test/symbol-test.cc  |  3 ++-
 14 files changed, 166 insertions(+), 68 deletions(-)

diff --git a/lib/database.cc b/lib/database.cc
index 98e2c31..386b93a 100644
--- a/lib/database.cc
+++ b/lib/database.cc
@@ -539,10 +539,21 @@ parse_references (void *ctx,
 }
 
 notmuch_status_t
-notmuch_database_create (const char *path, notmuch_database_t **database)
+notmuch_database_new (notmuch_database_t **notmuch)
+{
+    /* Note: try to avoid error conditions! No error printing! */
+
+    *notmuch = talloc_zero (NULL, notmuch_database_t);
+    if (! *notmuch)
+	return NOTMUCH_STATUS_OUT_OF_MEMORY;
+
+    return NOTMUCH_STATUS_SUCCESS;
+}
+
+notmuch_status_t
+notmuch_database_create (notmuch_database_t *notmuch, const char *path)
 {
     notmuch_status_t status = NOTMUCH_STATUS_SUCCESS;
-    notmuch_database_t *notmuch = NULL;
     char *notmuch_path = NULL;
     struct stat st;
     int err;
@@ -579,25 +590,18 @@ notmuch_database_create (const char *path, notmuch_database_t **database)
 	goto DONE;
     }
 
-    status = notmuch_database_open (path,
-				    NOTMUCH_DATABASE_MODE_READ_WRITE,
-				    &notmuch);
+    status = notmuch_database_open (notmuch, path,
+				    NOTMUCH_DATABASE_MODE_READ_WRITE);
     if (status)
 	goto DONE;
     status = notmuch_database_upgrade (notmuch, NULL, NULL);
-    if (status) {
+    if (status)
 	notmuch_database_close(notmuch);
-	notmuch = NULL;
-    }
 
   DONE:
     if (notmuch_path)
 	talloc_free (notmuch_path);
 
-    if (database)
-	*database = notmuch;
-    else
-	talloc_free (notmuch);
     return status;
 }
 
@@ -612,14 +616,15 @@ _notmuch_database_ensure_writable (notmuch_database_t *notmuch)
     return NOTMUCH_STATUS_SUCCESS;
 }
 
+/*
+ * XXX: error handling should clean up *all* state created!
+ */
 notmuch_status_t
-notmuch_database_open (const char *path,
-		       notmuch_database_mode_t mode,
-		       notmuch_database_t **database)
+notmuch_database_open (notmuch_database_t *notmuch, const char *path,
+		       notmuch_database_mode_t mode)
 {
     notmuch_status_t status = NOTMUCH_STATUS_SUCCESS;
     void *local = talloc_new (NULL);
-    notmuch_database_t *notmuch = NULL;
     char *notmuch_path, *xapian_path;
     struct stat st;
     int err;
@@ -663,7 +668,6 @@ notmuch_database_open (const char *path,
 	initialized = 1;
     }
 
-    notmuch = talloc_zero (NULL, notmuch_database_t);
     notmuch->exception_reported = FALSE;
     notmuch->path = talloc_strdup (notmuch, path);
 
@@ -689,8 +693,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_destroy (notmuch);
-		notmuch = NULL;
+		notmuch_database_close (notmuch);
 		status = NOTMUCH_STATUS_FILE_ERROR;
 		goto DONE;
 	    }
@@ -752,21 +755,19 @@ 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_destroy (notmuch);
-	notmuch = NULL;
+	notmuch_database_close (notmuch);
 	status = NOTMUCH_STATUS_XAPIAN_EXCEPTION;
     }
 
   DONE:
     talloc_free (local);
 
-    if (database)
-	*database = notmuch;
-    else
-	talloc_free (notmuch);
     return status;
 }
 
+/*
+ * XXX: close should clean up *all* state created by open/create!
+ */
 notmuch_status_t
 notmuch_database_close (notmuch_database_t *notmuch)
 {
@@ -869,7 +870,8 @@ public:
  * compaction process to protect data integrity.
  */
 notmuch_status_t
-notmuch_database_compact (const char *path,
+notmuch_database_compact (notmuch_database_t *notmuch,
+			  const char *path,
 			  const char *backup_path,
 			  notmuch_compact_status_cb_t status_cb,
 			  void *closure)
@@ -877,7 +879,6 @@ notmuch_database_compact (const char *path,
     void *local;
     char *notmuch_path, *xapian_path, *compact_xapian_path;
     notmuch_status_t ret = NOTMUCH_STATUS_SUCCESS;
-    notmuch_database_t *notmuch = NULL;
     struct stat statbuf;
     notmuch_bool_t keep_backup;
 
@@ -885,7 +886,8 @@ notmuch_database_compact (const char *path,
     if (! local)
 	return NOTMUCH_STATUS_OUT_OF_MEMORY;
 
-    ret = notmuch_database_open (path, NOTMUCH_DATABASE_MODE_READ_WRITE, &notmuch);
+    ret = notmuch_database_open (notmuch, path,
+				 NOTMUCH_DATABASE_MODE_READ_WRITE);
     if (ret) {
 	goto DONE;
     }
@@ -971,8 +973,9 @@ notmuch_database_compact (const char *path,
     }
 
   DONE:
+    /* XXX: error handling */
     if (notmuch)
-	ret = notmuch_database_destroy (notmuch);
+	ret = notmuch_database_close (notmuch);
 
     talloc_free (local);
 
@@ -980,7 +983,8 @@ notmuch_database_compact (const char *path,
 }
 #else
 notmuch_status_t
-notmuch_database_compact (unused (const char *path),
+notmuch_database_compact (unused (notmuch_database_t *notmuch),
+			  unused (const char *path),
 			  unused (const char *backup_path),
 			  unused (notmuch_compact_status_cb_t status_cb),
 			  unused (void *closure))
diff --git a/lib/notmuch.h b/lib/notmuch.h
index dbdce86..cd58d15 100644
--- a/lib/notmuch.h
+++ b/lib/notmuch.h
@@ -149,6 +149,28 @@ typedef struct _notmuch_tags notmuch_tags_t;
 typedef struct _notmuch_directory notmuch_directory_t;
 typedef struct _notmuch_filenames notmuch_filenames_t;
 
+/* Initialize a new, empty database handle.
+ *
+ * The database handle is required for creating, opening, and
+ * compacting a database. For further database operations, the
+ * database needs to be created or opened.
+ *
+ * After a successful call to notmuch_database_new, the returned
+ * database handle will remain in memory, so the caller should call
+ * notmuch_database_destroy when finished with the database handle.
+ *
+ * In case of any failure, this function returns an error status and
+ * sets *notmuch to NULL.
+ *
+ * Return value:
+ *
+ * NOTMUCH_STATUS_SUCCESS: Successfully created the database object.
+ *
+ * NOTMUCH_STATUS_OUT_OF_MEMORY: Out of memory.
+ */
+notmuch_status_t
+notmuch_database_new (notmuch_database_t **notmuch);
+
 /* Create a new, empty notmuch database located at 'path'.
  *
  * The path should be a top-level directory to a collection of
@@ -156,9 +178,9 @@ typedef struct _notmuch_filenames notmuch_filenames_t;
  * create a new ".notmuch" directory within 'path' where notmuch will
  * store its data.
  *
- * After a successful call to notmuch_database_create, the returned
- * database will be open so the caller should call
- * notmuch_database_destroy when finished with it.
+ * After a successful call to notmuch_database_create, the database
+ * will be open so the caller should call notmuch_database_close (or
+ * notmuch_database_destroy) when finished with the database.
  *
  * The database will not yet have any data in it
  * (notmuch_database_create itself is a very cheap function). Messages
@@ -166,7 +188,8 @@ typedef struct _notmuch_filenames notmuch_filenames_t;
  * notmuch_database_add_message.
  *
  * In case of any failure, this function returns an error status and
- * sets *database to NULL (after printing an error message on stderr).
+ * the database will be closed (after printing an error message on
+ * stderr).
  *
  * Return value:
  *
@@ -183,7 +206,7 @@ typedef struct _notmuch_filenames notmuch_filenames_t;
  * NOTMUCH_STATUS_XAPIAN_EXCEPTION: A Xapian exception occurred.
  */
 notmuch_status_t
-notmuch_database_create (const char *path, notmuch_database_t **database);
+notmuch_database_create (notmuch_database_t *notmuch, const char *path);
 
 typedef enum {
     NOTMUCH_DATABASE_MODE_READ_ONLY = 0,
@@ -201,11 +224,13 @@ 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_destroy when finished with
- * this database.
+ * After a successful call to notmuch_database_open, the database will
+ * be open so the caller should call notmuch_database_close (or
+ * notmuch_database_destroy) when finished with the database.
  *
  * In case of any failure, this function returns an error status and
- * sets *database to NULL (after printing an error message on stderr).
+ * the database will be closed (after printing an error message on
+ * stderr).
  *
  * Return value:
  *
@@ -222,9 +247,8 @@ typedef enum {
  * NOTMUCH_STATUS_XAPIAN_EXCEPTION: A Xapian exception occurred.
  */
 notmuch_status_t
-notmuch_database_open (const char *path,
-		       notmuch_database_mode_t mode,
-		       notmuch_database_t **database);
+notmuch_database_open (notmuch_database_t *notmuch, const char *path,
+		       notmuch_database_mode_t mode);
 
 /* Close the given notmuch database.
  *
@@ -264,7 +288,8 @@ typedef void (*notmuch_compact_status_cb_t)(const char *message, void *closure);
  * 'closure' is passed verbatim to any callback invoked.
  */
 notmuch_status_t
-notmuch_database_compact (const char* path,
+notmuch_database_compact (notmuch_database_t *notmuch,
+			  const char* path,
 			  const char* backup_path,
 			  notmuch_compact_status_cb_t status_cb,
 			  void *closure);
@@ -272,6 +297,9 @@ notmuch_database_compact (const char* path,
 /* Destroy the notmuch database, closing it if necessary and freeing
  * all associated resources.
  *
+ * A database handle initialized with notmuch_database_new should be
+ * destroyed by calling notmuch_database_destroy.
+ *
  * Return value as in notmuch_database_close if the database was open;
  * notmuch_database_destroy itself has no failure modes.
  */
diff --git a/notmuch-compact.c b/notmuch-compact.c
index 8b820c0..626685e 100644
--- a/notmuch-compact.c
+++ b/notmuch-compact.c
@@ -29,6 +29,7 @@ status_update_cb (const char *msg, unused (void *closure))
 int
 notmuch_compact_command (notmuch_config_t *config, int argc, char *argv[])
 {
+    notmuch_database_t *notmuch = NULL;
     const char *path = notmuch_config_get_database_path (config);
     const char *backup_path = NULL;
     notmuch_status_t ret;
@@ -46,7 +47,13 @@ notmuch_compact_command (notmuch_config_t *config, int argc, char *argv[])
 
     if (! quiet)
 	printf ("Compacting database...\n");
-    ret = notmuch_database_compact (path, backup_path,
+
+    if (notmuch_database_new (&notmuch)) {
+	fprintf (stderr, "Out of memory\n");
+	return 1;
+    }
+
+    ret = notmuch_database_compact (notmuch, path, backup_path,
 				    quiet ? NULL : status_update_cb, NULL);
     if (ret) {
 	fprintf (stderr, "Compaction failed: %s\n", notmuch_status_to_string (ret));
@@ -60,5 +67,7 @@ notmuch_compact_command (notmuch_config_t *config, int argc, char *argv[])
 	printf ("Done.\n");
     }
 
+    notmuch_database_destroy (notmuch);
+
     return 0;
 }
diff --git a/notmuch-count.c b/notmuch-count.c
index 01e4e30..15c95c7 100644
--- a/notmuch-count.c
+++ b/notmuch-count.c
@@ -170,8 +170,14 @@ notmuch_count_command (notmuch_config_t *config, int argc, char *argv[])
 	return 1;
     }
 
-    if (notmuch_database_open (notmuch_config_get_database_path (config),
-			       NOTMUCH_DATABASE_MODE_READ_ONLY, &notmuch))
+    if (notmuch_database_new (&notmuch)) {
+	fprintf (stderr, "Out of memory\n");
+	return 1;
+    }
+
+    if (notmuch_database_open (notmuch,
+			       notmuch_config_get_database_path (config),
+			       NOTMUCH_DATABASE_MODE_READ_ONLY))
 	return 1;
 
     query_str = query_string_from_args (config, argc-opt_index, argv+opt_index);
diff --git a/notmuch-dump.c b/notmuch-dump.c
index 2024e30..73579bc 100644
--- a/notmuch-dump.c
+++ b/notmuch-dump.c
@@ -33,8 +33,14 @@ notmuch_dump_command (notmuch_config_t *config, int argc, char *argv[])
     notmuch_tags_t *tags;
     const char *query_str = "";
 
-    if (notmuch_database_open (notmuch_config_get_database_path (config),
-			       NOTMUCH_DATABASE_MODE_READ_ONLY, &notmuch))
+    if (notmuch_database_new (&notmuch)) {
+	fprintf (stderr, "Out of memory\n");
+	return 1;
+    }
+
+    if (notmuch_database_open (notmuch,
+			       notmuch_config_get_database_path (config),
+			       NOTMUCH_DATABASE_MODE_READ_ONLY))
 	return 1;
 
     char *output_file_name = NULL;
diff --git a/notmuch-insert.c b/notmuch-insert.c
index 2207b1e..4a8aad3 100644
--- a/notmuch-insert.c
+++ b/notmuch-insert.c
@@ -467,8 +467,14 @@ notmuch_insert_command (notmuch_config_t *config, int argc, char *argv[])
     action.sa_flags = 0;
     sigaction (SIGINT, &action, NULL);
 
-    if (notmuch_database_open (notmuch_config_get_database_path (config),
-			       NOTMUCH_DATABASE_MODE_READ_WRITE, &notmuch))
+    if (notmuch_database_new (&notmuch)) {
+	fprintf (stderr, "Out of memory\n");
+	return 1;
+    }
+
+    if (notmuch_database_open (notmuch,
+			       notmuch_config_get_database_path (config),
+			       NOTMUCH_DATABASE_MODE_READ_WRITE))
 	return 1;
 
     ret = insert_message (config, notmuch, STDIN_FILENO, maildir, tag_ops);
diff --git a/notmuch-new.c b/notmuch-new.c
index ba05cb4..f72a4de 100644
--- a/notmuch-new.c
+++ b/notmuch-new.c
@@ -914,6 +914,11 @@ notmuch_new_command (notmuch_config_t *config, int argc, char *argv[])
 	    return ret;
     }
 
+    if (notmuch_database_new (&notmuch)) {
+	fprintf (stderr, "Out of memory\n");
+	return 1;
+    }
+
     dot_notmuch_path = talloc_asprintf (config, "%s/%s", db_path, ".notmuch");
 
     if (stat (dot_notmuch_path, &st)) {
@@ -925,12 +930,12 @@ notmuch_new_command (notmuch_config_t *config, int argc, char *argv[])
 	    return 1;
 
 	printf ("Found %d total files (that's not much mail).\n", count);
-	if (notmuch_database_create (db_path, &notmuch))
+	if (notmuch_database_create (notmuch, db_path))
 	    return 1;
 	add_files_state.total_files = count;
     } else {
-	if (notmuch_database_open (db_path, NOTMUCH_DATABASE_MODE_READ_WRITE,
-				   &notmuch))
+	if (notmuch_database_open (notmuch, db_path,
+				   NOTMUCH_DATABASE_MODE_READ_WRITE))
 	    return 1;
 
 	if (notmuch_database_needs_upgrade (notmuch)) {
@@ -945,9 +950,6 @@ notmuch_new_command (notmuch_config_t *config, int argc, char *argv[])
 	add_files_state.total_files = 0;
     }
 
-    if (notmuch == NULL)
-	return 1;
-
     /* Setup our handler for SIGINT. We do this after having
      * potentially done a database upgrade we this interrupt handler
      * won't support. */
diff --git a/notmuch-reply.c b/notmuch-reply.c
index 9d6f843..7b80841 100644
--- a/notmuch-reply.c
+++ b/notmuch-reply.c
@@ -769,8 +769,14 @@ notmuch_reply_command (notmuch_config_t *config, int argc, char *argv[])
 	return 1;
     }
 
-    if (notmuch_database_open (notmuch_config_get_database_path (config),
-			       NOTMUCH_DATABASE_MODE_READ_ONLY, &notmuch))
+    if (notmuch_database_new (&notmuch)) {
+	fprintf (stderr, "Out of memory\n");
+	return 1;
+    }
+
+    if (notmuch_database_open (notmuch,
+			       notmuch_config_get_database_path (config),
+			       NOTMUCH_DATABASE_MODE_READ_ONLY))
 	return 1;
 
     query = notmuch_query_create (notmuch, query_string);
diff --git a/notmuch-restore.c b/notmuch-restore.c
index 1419621..fc37838 100644
--- a/notmuch-restore.c
+++ b/notmuch-restore.c
@@ -138,8 +138,14 @@ notmuch_restore_command (notmuch_config_t *config, int argc, char *argv[])
     int opt_index;
     int input_format = DUMP_FORMAT_AUTO;
 
-    if (notmuch_database_open (notmuch_config_get_database_path (config),
-			       NOTMUCH_DATABASE_MODE_READ_WRITE, &notmuch))
+    if (notmuch_database_new (&notmuch)) {
+	fprintf (stderr, "Out of memory\n");
+	return 1;
+    }
+
+    if (notmuch_database_open (notmuch,
+			       notmuch_config_get_database_path (config),
+			       NOTMUCH_DATABASE_MODE_READ_WRITE))
 	return 1;
 
     if (notmuch_config_get_maildir_synchronize_flags (config))
diff --git a/notmuch-search.c b/notmuch-search.c
index 7c973b3..50aace9 100644
--- a/notmuch-search.c
+++ b/notmuch-search.c
@@ -430,8 +430,14 @@ notmuch_search_command (notmuch_config_t *config, int argc, char *argv[])
 
     notmuch_exit_if_unsupported_format ();
 
-    if (notmuch_database_open (notmuch_config_get_database_path (config),
-			       NOTMUCH_DATABASE_MODE_READ_ONLY, &notmuch))
+    if (notmuch_database_new (&notmuch)) {
+	fprintf (stderr, "Out of memory\n");
+	return 1;
+    }
+
+    if (notmuch_database_open (notmuch,
+			       notmuch_config_get_database_path (config),
+			       NOTMUCH_DATABASE_MODE_READ_ONLY))
 	return 1;
 
     query_str = query_string_from_args (notmuch, argc-opt_index, argv+opt_index);
diff --git a/notmuch-show.c b/notmuch-show.c
index c07f887..bc44b2c 100644
--- a/notmuch-show.c
+++ b/notmuch-show.c
@@ -1201,8 +1201,14 @@ notmuch_show_command (notmuch_config_t *config, int argc, char *argv[])
 	return 1;
     }
 
-    if (notmuch_database_open (notmuch_config_get_database_path (config),
-			       NOTMUCH_DATABASE_MODE_READ_ONLY, &notmuch))
+    if (notmuch_database_new (&notmuch)) {
+	fprintf (stderr, "Out of memory\n");
+	return 1;
+    }
+
+    if (notmuch_database_open (notmuch,
+			       notmuch_config_get_database_path (config),
+			       NOTMUCH_DATABASE_MODE_READ_ONLY))
 	return 1;
 
     query = notmuch_query_create (notmuch, query_string);
diff --git a/notmuch-tag.c b/notmuch-tag.c
index 3b09df9..6e29799 100644
--- a/notmuch-tag.c
+++ b/notmuch-tag.c
@@ -254,8 +254,14 @@ notmuch_tag_command (notmuch_config_t *config, int argc, char *argv[])
 	}
     }
 
-    if (notmuch_database_open (notmuch_config_get_database_path (config),
-			       NOTMUCH_DATABASE_MODE_READ_WRITE, &notmuch))
+    if (notmuch_database_new (&notmuch)) {
+	fprintf (stderr, "Out of memory\n");
+	return 1;
+    }
+
+    if (notmuch_database_open (notmuch,
+			       notmuch_config_get_database_path (config),
+			       NOTMUCH_DATABASE_MODE_READ_WRITE))
 	return 1;
 
     if (notmuch_config_get_maildir_synchronize_flags (config))
diff --git a/test/random-corpus.c b/test/random-corpus.c
index 790193d..2b205e5 100644
--- a/test/random-corpus.c
+++ b/test/random-corpus.c
@@ -164,8 +164,14 @@ main (int argc, char **argv)
     if (config == NULL)
 	return 1;
 
-    if (notmuch_database_open (notmuch_config_get_database_path (config),
-			       NOTMUCH_DATABASE_MODE_READ_WRITE, &notmuch))
+    if (notmuch_database_new (&notmuch)) {
+	fprintf (stderr, "Out of memory\n");
+	return 1;
+    }
+
+    if (notmuch_database_open (notmuch,
+			       notmuch_config_get_database_path (config),
+			       NOTMUCH_DATABASE_MODE_READ_WRITE))
 	return 1;
 
     srandom (seed);
diff --git a/test/symbol-test.cc b/test/symbol-test.cc
index 3e96c03..47c5351 100644
--- a/test/symbol-test.cc
+++ b/test/symbol-test.cc
@@ -5,7 +5,8 @@
 
 int main() {
   notmuch_database_t *notmuch;
-  notmuch_database_open("fakedb", NOTMUCH_DATABASE_MODE_READ_ONLY, &notmuch);
+  notmuch_database_new (&notmuch);
+  notmuch_database_open (notmuch, "fakedb", NOTMUCH_DATABASE_MODE_READ_ONLY);
 
   try {
     (void) new Xapian::WritableDatabase("./nonexistant", Xapian::DB_OPEN);
-- 
1.8.4.2


Thread: