[Patch v6 4/8] lib: add "verbose" versions of notmuch_database_{open, create}
Tomi Ollila
tomi.ollila at iki.fi
Sat Mar 28 03:04:22 PDT 2015
On Sat, Mar 28 2015, David Bremner <david at tethera.net> wrote:
> The compatibility wrapper ensures that clients calling
> notmuch_database_open will receive consistent output for now.
>
> The changes to notmuch-{new,search} and test/symbol-test are just to
> make the test suite pass.
>
> The use of IGNORE_RESULT is justified by two things. 1) I don't know
> what else to do. 2) asprintf guarantees the output string is NULL if
> an error occurs, so at least we are not passing garbage back.
> ---
> lib/database.cc | 109 ++++++++++++++++++++++++++++++++++++++--------------
> lib/notmuch.h | 21 ++++++++++
> notmuch-new.c | 11 +++++-
> notmuch-search.c | 13 ++++++-
> test/symbol-test.cc | 9 ++++-
> 5 files changed, 130 insertions(+), 33 deletions(-)
>
> diff --git a/lib/database.cc b/lib/database.cc
> index 3974e2e..5d973b9 100644
> --- a/lib/database.cc
> +++ b/lib/database.cc
> @@ -608,29 +608,50 @@ parse_references (void *ctx,
> notmuch_status_t
> notmuch_database_create (const char *path, notmuch_database_t **database)
> {
> + char *status_string = NULL;
> + notmuch_status_t status;
> +
> + status = notmuch_database_create_verbose (path, database,
> + &status_string);
> +
> + if (status_string) {
> + fputs (status_string, stderr);
> + free (status_string);
> + }
> +
> + return status;
> +}
> +
> +notmuch_status_t
> +notmuch_database_create_verbose (const char *path,
> + notmuch_database_t **database,
> + char **status_string)
> +{
> notmuch_status_t status = NOTMUCH_STATUS_SUCCESS;
> notmuch_database_t *notmuch = NULL;
> char *notmuch_path = NULL;
> + char *message = NULL;
> struct stat st;
> int err;
>
> if (path == NULL) {
> - fprintf (stderr, "Error: Cannot create a database for a NULL path.\n");
> + message = strdup ("Error: Cannot create a database for a NULL path.\n");
> status = NOTMUCH_STATUS_NULL_POINTER;
> goto DONE;
> }
>
> err = stat (path, &st);
> if (err) {
> - fprintf (stderr, "Error: Cannot create database at %s: %s.\n",
> - path, strerror (errno));
> + IGNORE_RESULT (asprintf (&message, "Error: Cannot create database at %s: %s.\n",
> + path, strerror (errno)));
> status = NOTMUCH_STATUS_FILE_ERROR;
> goto DONE;
> }
>
> if (! S_ISDIR (st.st_mode)) {
> - fprintf (stderr, "Error: Cannot create database at %s: Not a directory.\n",
> - path);
> + IGNORE_RESULT (asprintf (&message, "Error: Cannot create database at %s: "
> + "Not a directory.\n",
> + path));
> status = NOTMUCH_STATUS_FILE_ERROR;
> goto DONE;
> }
> @@ -640,15 +661,15 @@ notmuch_database_create (const char *path, notmuch_database_t **database)
> err = mkdir (notmuch_path, 0755);
>
> if (err) {
> - fprintf (stderr, "Error: Cannot create directory %s: %s.\n",
> - notmuch_path, strerror (errno));
> + IGNORE_RESULT (asprintf (&message, "Error: Cannot create directory %s: %s.\n",
> + notmuch_path, strerror (errno)));
> status = NOTMUCH_STATUS_FILE_ERROR;
> goto DONE;
> }
>
> - status = notmuch_database_open (path,
> - NOTMUCH_DATABASE_MODE_READ_WRITE,
> - ¬much);
> + status = notmuch_database_open_verbose (path,
> + NOTMUCH_DATABASE_MODE_READ_WRITE,
> + ¬much, &message);
> if (status)
> goto DONE;
>
> @@ -667,6 +688,9 @@ notmuch_database_create (const char *path, notmuch_database_t **database)
> if (notmuch_path)
> talloc_free (notmuch_path);
>
> + if (status_string && message)
> + *status_string = message;
This series looks good and tests pass (I had confusion there before seeing
one of the last patch moving one output from stderr to stdout of where
the test status_cb prints it output)
Just that this small piece of code above git pass my sparse sieve, perhaps
this could be amended to:
if (message) {
if (status_string)
*status_string = message;
else
free(message);
}
IMO either amend or leave to followup patch; just for that I don't
want to go through full patch series ;D
> +
> if (database)
> *database = notmuch;
> else
> @@ -767,37 +791,58 @@ notmuch_database_open (const char *path,
> notmuch_database_mode_t mode,
> notmuch_database_t **database)
> {
> + char *status_string = NULL;
> + notmuch_status_t status;
> +
> + status = notmuch_database_open_verbose (path, mode, database,
> + &status_string);
> +
> + if (status_string) {
> + fputs (status_string, stderr);
> + free (status_string);
> + }
> +
> + return status;
> +}
> +
> +notmuch_status_t
> +notmuch_database_open_verbose (const char *path,
> + notmuch_database_mode_t mode,
> + notmuch_database_t **database,
> + char **status_string)
> +{
> notmuch_status_t status = NOTMUCH_STATUS_SUCCESS;
> void *local = talloc_new (NULL);
> notmuch_database_t *notmuch = NULL;
> char *notmuch_path, *xapian_path, *incompat_features;
> + char *message = NULL;
> struct stat st;
> int err;
> unsigned int i, version;
> static int initialized = 0;
>
> if (path == NULL) {
> - fprintf (stderr, "Error: Cannot open a database for a NULL path.\n");
> + message = strdup ("Error: Cannot open a database for a NULL path.\n");
> status = NOTMUCH_STATUS_NULL_POINTER;
> goto DONE;
> }
>
> if (! (notmuch_path = talloc_asprintf (local, "%s/%s", path, ".notmuch"))) {
> - fprintf (stderr, "Out of memory\n");
> + message = strdup ("Out of memory\n");
> status = NOTMUCH_STATUS_OUT_OF_MEMORY;
> goto DONE;
> }
>
> err = stat (notmuch_path, &st);
> if (err) {
> - fprintf (stderr, "Error opening database at %s: %s\n",
> - notmuch_path, strerror (errno));
> + IGNORE_RESULT (asprintf (&message, "Error opening database at %s: %s\n",
> + notmuch_path, strerror (errno)));
> status = NOTMUCH_STATUS_FILE_ERROR;
> goto DONE;
> }
>
> if (! (xapian_path = talloc_asprintf (local, "%s/%s", notmuch_path, "xapian"))) {
> - fprintf (stderr, "Out of memory\n");
> + message = strdup ("Out of memory\n");
> status = NOTMUCH_STATUS_OUT_OF_MEMORY;
> goto DONE;
> }
> @@ -837,11 +882,11 @@ notmuch_database_open (const char *path,
> * means a dramatically incompatible change. */
> version = notmuch_database_get_version (notmuch);
> if (version > NOTMUCH_DATABASE_VERSION) {
> - fprintf (stderr,
> - "Error: Notmuch database at %s\n"
> - " has a newer database format version (%u) than supported by this\n"
> - " version of notmuch (%u).\n",
> - notmuch_path, version, NOTMUCH_DATABASE_VERSION);
> + IGNORE_RESULT (asprintf (&message,
> + "Error: Notmuch database at %s\n"
> + " has a newer database format version (%u) than supported by this\n"
> + " version of notmuch (%u).\n",
> + notmuch_path, version, NOTMUCH_DATABASE_VERSION));
> notmuch->mode = NOTMUCH_DATABASE_MODE_READ_ONLY;
> notmuch_database_destroy (notmuch);
> notmuch = NULL;
> @@ -856,11 +901,11 @@ notmuch_database_open (const char *path,
> version, mode == NOTMUCH_DATABASE_MODE_READ_WRITE ? 'w' : 'r',
> &incompat_features);
> if (incompat_features) {
> - fprintf (stderr,
> - "Error: Notmuch database at %s\n"
> - " requires features (%s)\n"
> - " not supported by this version of notmuch.\n",
> - notmuch_path, incompat_features);
> + IGNORE_RESULT (asprintf (&message,
> + "Error: Notmuch database at %s\n"
> + " requires features (%s)\n"
> + " not supported by this version of notmuch.\n",
> + notmuch_path, incompat_features));
> notmuch->mode = NOTMUCH_DATABASE_MODE_READ_ONLY;
> notmuch_database_destroy (notmuch);
> notmuch = NULL;
> @@ -906,8 +951,8 @@ notmuch_database_open (const char *path,
> notmuch->query_parser->add_prefix (prefix->name, prefix->prefix);
> }
> } catch (const Xapian::Error &error) {
> - fprintf (stderr, "A Xapian exception occurred opening database: %s\n",
> - error.get_msg().c_str());
> + IGNORE_RESULT (asprintf (&message, "A Xapian exception occurred opening database: %s\n",
> + error.get_msg().c_str()));
> notmuch_database_destroy (notmuch);
> notmuch = NULL;
> status = NOTMUCH_STATUS_XAPIAN_EXCEPTION;
> @@ -916,6 +961,9 @@ notmuch_database_open (const char *path,
> DONE:
> talloc_free (local);
>
> + if (status_string && message)
> + *status_string = message;
Ditto.
> +
> if (database)
> *database = notmuch;
> else
> @@ -1039,13 +1087,18 @@ notmuch_database_compact (const char *path,
> notmuch_database_t *notmuch = NULL;
> struct stat statbuf;
> notmuch_bool_t keep_backup;
> + char *message = NULL;
>
> local = talloc_new (NULL);
> if (! local)
> return NOTMUCH_STATUS_OUT_OF_MEMORY;
>
> - ret = notmuch_database_open (path, NOTMUCH_DATABASE_MODE_READ_WRITE, ¬much);
> + ret = notmuch_database_open_verbose (path,
> + NOTMUCH_DATABASE_MODE_READ_WRITE,
> + ¬much,
> + &message);
> if (ret) {
> + if (status_cb) status_cb (message, closure);
> goto DONE;
> }
>
> diff --git a/lib/notmuch.h b/lib/notmuch.h
> index f066245..c671d82 100644
> --- a/lib/notmuch.h
> +++ b/lib/notmuch.h
> @@ -230,6 +230,16 @@ notmuch_status_t
> notmuch_database_create (const char *path, notmuch_database_t **database);
>
> /**
> + * Like notmuch_database_create, except optionally return an error
> + * message. This message is allocated by malloc and should be freed by
> + * the caller.
> + */
> +notmuch_status_t
> +notmuch_database_create_verbose (const char *path,
> + notmuch_database_t **database,
> + char **error_message);
> +
> +/**
> * Database open mode for notmuch_database_open.
> */
> typedef enum {
> @@ -279,6 +289,17 @@ notmuch_status_t
> notmuch_database_open (const char *path,
> notmuch_database_mode_t mode,
> notmuch_database_t **database);
> +/**
> + * Like notmuch_database_open, except optionally return an error
> + * message. This message is allocated by malloc and should be freed by
> + * the caller.
> + */
> +
> +notmuch_status_t
> +notmuch_database_open_verbose (const char *path,
> + notmuch_database_mode_t mode,
> + notmuch_database_t **database,
> + char **error_message);
>
> /**
> * Commit changes and close the given notmuch database.
> diff --git a/notmuch-new.c b/notmuch-new.c
> index ddf42c1..e6c283e 100644
> --- a/notmuch-new.c
> +++ b/notmuch-new.c
> @@ -985,9 +985,16 @@ notmuch_new_command (notmuch_config_t *config, int argc, char *argv[])
> return EXIT_FAILURE;
> add_files_state.total_files = count;
> } else {
> - if (notmuch_database_open (db_path, NOTMUCH_DATABASE_MODE_READ_WRITE,
> - ¬much))
> + char *status_string = NULL;
> + if (notmuch_database_open_verbose (db_path, NOTMUCH_DATABASE_MODE_READ_WRITE,
> + ¬much, &status_string)) {
> + if (status_string) {
> + fputs (status_string, stderr);
> + free (status_string);
> + }
> +
> return EXIT_FAILURE;
> + }
>
> if (notmuch_database_needs_upgrade (notmuch)) {
> time_t now = time (NULL);
> diff --git a/notmuch-search.c b/notmuch-search.c
> index a591d45..b81ac01 100644
> --- a/notmuch-search.c
> +++ b/notmuch-search.c
> @@ -545,6 +545,7 @@ _notmuch_search_prepare (search_context_t *ctx, notmuch_config_t *config, int ar
> {
> char *query_str;
> unsigned int i;
> + char *status_string = NULL;
>
> switch (ctx->format_sel) {
> case NOTMUCH_FORMAT_TEXT:
> @@ -570,9 +571,17 @@ _notmuch_search_prepare (search_context_t *ctx, notmuch_config_t *config, int ar
>
> notmuch_exit_if_unsupported_format ();
>
> - if (notmuch_database_open (notmuch_config_get_database_path (config),
> - NOTMUCH_DATABASE_MODE_READ_ONLY, &ctx->notmuch))
> + if (notmuch_database_open_verbose (
> + notmuch_config_get_database_path (config),
> + NOTMUCH_DATABASE_MODE_READ_ONLY, &ctx->notmuch, &status_string)) {
> +
> + if (status_string) {
> + fputs (status_string, stderr);
> + free (status_string);
> + }
> +
> return EXIT_FAILURE;
> + }
>
> query_str = query_string_from_args (ctx->notmuch, argc, argv);
> if (query_str == NULL) {
> diff --git a/test/symbol-test.cc b/test/symbol-test.cc
> index 3e96c03..d979f83 100644
> --- a/test/symbol-test.cc
> +++ b/test/symbol-test.cc
> @@ -1,11 +1,18 @@
> #include <stdio.h>
> +#include <stdlib.h>
> #include <xapian.h>
> #include <notmuch.h>
>
>
> int main() {
> notmuch_database_t *notmuch;
> - notmuch_database_open("fakedb", NOTMUCH_DATABASE_MODE_READ_ONLY, ¬much);
> + char *message = NULL;
> +
> + if (notmuch_database_open_verbose ("fakedb", NOTMUCH_DATABASE_MODE_READ_ONLY, ¬much, &message))
> + if (message) {
> + fputs (message, stderr);
> + free (message);
> + }
>
> try {
> (void) new Xapian::WritableDatabase("./nonexistant", Xapian::DB_OPEN);
> --
> 2.1.4
>
> _______________________________________________
> notmuch mailing list
> notmuch at notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch
More information about the notmuch
mailing list