From: Tomi Ollila Date: Sat, 28 Mar 2015 10:04:22 +0000 (+0200) Subject: Re: [Patch v6 4/8] lib: add "verbose" versions of notmuch_database_{open, create} X-Git-Url: http://git.tremily.us/?a=commitdiff_plain;h=c1cbc13257c2448d1d6ee5b7c2e595df8bab87ce;p=notmuch-archives.git Re: [Patch v6 4/8] lib: add "verbose" versions of notmuch_database_{open, create} --- diff --git a/c9/ed2968baf2ad645e10dc19d3ac6725964178e1 b/c9/ed2968baf2ad645e10dc19d3ac6725964178e1 new file mode 100644 index 000000000..dc9809299 --- /dev/null +++ b/c9/ed2968baf2ad645e10dc19d3ac6725964178e1 @@ -0,0 +1,449 @@ +Return-Path: +X-Original-To: notmuch@notmuchmail.org +Delivered-To: notmuch@notmuchmail.org +Received: from localhost (localhost [127.0.0.1]) + by olra.theworths.org (Postfix) with ESMTP id 07DD2431FB6 + for ; Sat, 28 Mar 2015 03:04:50 -0700 (PDT) +X-Virus-Scanned: Debian amavisd-new at olra.theworths.org +X-Spam-Flag: NO +X-Spam-Score: 2.438 +X-Spam-Level: ** +X-Spam-Status: No, score=2.438 tagged_above=-999 required=5 + tests=[DNS_FROM_AHBL_RHSBL=2.438] autolearn=disabled +Received: from olra.theworths.org ([127.0.0.1]) + by localhost (olra.theworths.org [127.0.0.1]) (amavisd-new, port 10024) + with ESMTP id 8r9hAleA44v3 for ; + Sat, 28 Mar 2015 03:04:46 -0700 (PDT) +Received: from guru.guru-group.fi (guru.guru-group.fi [46.183.73.34]) + by olra.theworths.org (Postfix) with ESMTP id 54D61431FAF + for ; Sat, 28 Mar 2015 03:04:46 -0700 (PDT) +Received: from guru.guru-group.fi (localhost [IPv6:::1]) + by guru.guru-group.fi (Postfix) with ESMTP id 71B8B100086; + Sat, 28 Mar 2015 12:04:22 +0200 (EET) +From: Tomi Ollila +To: David Bremner , notmuch@notmuchmail.org +Subject: Re: [Patch v6 4/8] lib: add "verbose" versions + of notmuch_database_{open, create} +In-Reply-To: <1427494320-1483-5-git-send-email-david@tethera.net> +References: <1427494320-1483-1-git-send-email-david@tethera.net> + <1427494320-1483-5-git-send-email-david@tethera.net> +User-Agent: Notmuch/0.19+92~g402df12 (http://notmuchmail.org) Emacs/24.3.1 + (x86_64-unknown-linux-gnu) +X-Face: HhBM'cA~ +MIME-Version: 1.0 +Content-Type: text/plain +X-BeenThere: notmuch@notmuchmail.org +X-Mailman-Version: 2.1.13 +Precedence: list +List-Id: "Use and development of the notmuch mail system." + +List-Unsubscribe: , + +List-Archive: +List-Post: +List-Help: +List-Subscribe: , + +X-List-Received-Date: Sat, 28 Mar 2015 10:04:50 -0000 + +On Sat, Mar 28 2015, David Bremner 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 +> +#include +> #include +> #include +> +> +> 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@notmuchmail.org +> http://notmuchmail.org/mailman/listinfo/notmuch