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 C5D92431FC0 for ; Wed, 25 Mar 2015 09:39:49 -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 BgZaEHkKtE2x for ; Wed, 25 Mar 2015 09:39: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 49D86431FBC for ; Wed, 25 Mar 2015 09:39:46 -0700 (PDT) Received: from guru.guru-group.fi (localhost [IPv6:::1]) by guru.guru-group.fi (Postfix) with ESMTP id B3CA7100086; Wed, 25 Mar 2015 18:39:24 +0200 (EET) From: Tomi Ollila To: David Bremner , David Bremner , notmuch@notmuchmail.org Subject: Re: [Patch v5 4/8] lib: add "verbose" versions of notmuch_database_{open, create} In-Reply-To: <1427203451-1540-5-git-send-email-david@tethera.net> References: <1426352554-4383-10-git-send-email-david@tethera.net> <1427203451-1540-1-git-send-email-david@tethera.net> <1427203451-1540-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: Wed, 25 Mar 2015 16:39:49 -0000 On Tue, Mar 24 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 | 94 +++++++++++++++++++++++++++++++++++++---------------- > lib/notmuch.h | 21 ++++++++++++ > notmuch-new.c | 8 +++-- > notmuch-search.c | 8 +++-- > test/symbol-test.cc | 6 +++- > 5 files changed, 104 insertions(+), 33 deletions(-) > > diff --git a/lib/database.cc b/lib/database.cc > index 3974e2e..36849d7 100644 > --- a/lib/database.cc > +++ b/lib/database.cc > @@ -608,29 +608,39 @@ 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); Hmm, is is so that nothing is printed if creating database fails, should this provide &message to _verbose function and if message changed, fputs() it (and then free ())? ... after looking below -- notmuch_database_open () does it. > +} > + > +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 +650,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 +677,8 @@ notmuch_database_create (const char *path, notmuch_database_t **database) > if (notmuch_path) > talloc_free (notmuch_path); > > + if (message) > + *status_string = message; Hmm, what if status_string == NULL -- do we have a test for this (or am I missing something ?) > if (database) > *database = notmuch; > else > @@ -767,37 +779,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); and free (status_string); (and fputs (...) (i.e. space which is in all other hunks in this patch :) > + > + 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 +867,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 +886,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 +936,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 +946,9 @@ notmuch_database_open (const char *path, > DONE: > talloc_free (local); > > + if (status_string && message) > + *status_string = message; here it is !!! \o/ !!! :D > + > if (database) > *database = notmuch; > else > @@ -1039,13 +1072,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; > } Is this ever printing the message (if any?) > > 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); and 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..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); diiitto (or do we skip freeing 'small' allocations just before exit) > 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); free... (for consistency) > + > > try { > (void) new Xapian::WritableDatabase("./nonexistant", Xapian::DB_OPEN); > -- > 2.1.4