[Patch v4 4/9] lib: add "verbose" versions of notmuch_database_{open, create}
Tomi Ollila
tomi.ollila at iki.fi
Sat Mar 21 02:27:41 PDT 2015
On Sat, Mar 14 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 stdargs based infrastucture will be used in following commits for
infrastructure :D
> a more general logging mechanism.
>
> The changes to notmuch-{new,search} and test/symbol-test are just to
> make the test suite pass.
> ---
> lib/database.cc | 96 ++++++++++++++++++++++++++++++++++++++++++++---------
> lib/notmuch.h | 21 ++++++++++++
> notmuch-new.c | 8 +++--
> notmuch-search.c | 8 +++--
> test/symbol-test.cc | 6 +++-
> 5 files changed, 119 insertions(+), 20 deletions(-)
>
> diff --git a/lib/database.cc b/lib/database.cc
> index 3974e2e..b774edc 100644
> --- a/lib/database.cc
> +++ b/lib/database.cc
> @@ -349,6 +349,35 @@ notmuch_status_to_string (notmuch_status_t status)
> }
>
> static void
> +vlog_to_string (void *ctx,
> + char **status_string,
> + const char *format,
> + va_list va_args)
> +{
> + if (!status_string)
> + return;
Noticed just before sending: (! status_string) -- and one more deep down
below...
> +
> + if (*status_string)
> + talloc_free (*status_string);
> +
> + *status_string = talloc_vasprintf (ctx, format, va_args);
> +}
> +
> +static void
> +log_to_string (char **str,
> + const char *format,
> + ...)
> +{
> + va_list va_args;
> +
> + va_start (va_args, format);
> +
> + vlog_to_string (NULL, str, format, va_args);
> +
> + va_end (va_args);
> +}
To me it looks a bit peculiar log_to_string () not taking `ctx` as first
argument. I'd suggest
1) add that ctx to first arg and tediously add NULL to all callers
or
2) rename the function so something less similar and call that instead
(even leading _ could "mark" the function to be less generic interface to
vlog_to_string) .
Otherwise this (and rest of the series) looks pretty good to me.
Tomi
> +static void
> find_doc_ids_for_term (notmuch_database_t *notmuch,
> const char *term,
> Xapian::PostingIterator *begin,
> @@ -608,28 +637,37 @@ parse_references (void *ctx,
> notmuch_status_t
> notmuch_database_create (const char *path, notmuch_database_t **database)
> {
> + return notmuch_database_create_verbose (path, database, NULL);
> +}
> +
> +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");
> + log_to_string (&message, "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",
> + log_to_string (&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",
> + log_to_string (&message, "Error: Cannot create database at %s: Not a directory.\n",
> path);
> status = NOTMUCH_STATUS_FILE_ERROR;
> goto DONE;
> @@ -640,15 +678,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",
> + log_to_string (&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 +705,8 @@ notmuch_database_create (const char *path, notmuch_database_t **database)
> if (notmuch_path)
> talloc_free (notmuch_path);
>
> + if (message)
> + *status_string = strdup(message);
strdup (message);
> if (database)
> *database = notmuch;
> else
> @@ -767,37 +807,55 @@ 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);
Also, does this above (and few similar below) match the style elsewhere ?
(i personally like it but... `git grep fputs` does not reveal such done before)
> +
> + 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");
> + log_to_string (&message, "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");
> + log_to_string (&message, "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",
> + log_to_string (&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");
> + log_to_string (&message, "Out of memory\n");
> status = NOTMUCH_STATUS_OUT_OF_MEMORY;
> goto DONE;
> }
> @@ -837,7 +895,7 @@ notmuch_database_open (const char *path,
> * means a dramatically incompatible change. */
> version = notmuch_database_get_version (notmuch);
> if (version > NOTMUCH_DATABASE_VERSION) {
> - fprintf (stderr,
> + log_to_string (&message,
> "Error: Notmuch database at %s\n"
> " has a newer database format version (%u) than supported by this\n"
> " version of notmuch (%u).\n",
> @@ -856,7 +914,7 @@ notmuch_database_open (const char *path,
> version, mode == NOTMUCH_DATABASE_MODE_READ_WRITE ? 'w' : 'r',
> &incompat_features);
> if (incompat_features) {
> - fprintf (stderr,
> + log_to_string (&message,
> "Error: Notmuch database at %s\n"
> " requires features (%s)\n"
> " not supported by this version of notmuch.\n",
> @@ -906,7 +964,7 @@ 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",
> + log_to_string (&message, "A Xapian exception occurred opening database: %s\n",
> error.get_msg().c_str());
> notmuch_database_destroy (notmuch);
> notmuch = NULL;
> @@ -916,6 +974,9 @@ notmuch_database_open (const char *path,
> DONE:
> talloc_free (local);
>
> + if (status_string && message)
> + *status_string = strdup (message);
> +
> if (database)
> *database = notmuch;
> else
> @@ -1039,13 +1100,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..93b70bf 100644
> --- a/notmuch-new.c
> +++ b/notmuch-new.c
> @@ -985,9 +985,13 @@ 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);
> +
> 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..d012af3 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,12 @@ _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);
> 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..9f8eea7 100644
> --- a/test/symbol-test.cc
> +++ b/test/symbol-test.cc
> @@ -5,7 +5,11 @@
>
> 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);
> +
>
> 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