[Patch v5 4/8] lib: add "verbose" versions of notmuch_database_{open, create}

Tomi Ollila tomi.ollila at iki.fi
Wed Mar 25 09:39:24 PDT 2015


On Tue, Mar 24 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     | 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,
> -				    &notmuch);
> +    status = notmuch_database_open_verbose (path,
> +					    NOTMUCH_DATABASE_MODE_READ_WRITE,
> +					    &notmuch, &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, &notmuch);
> +    ret = notmuch_database_open_verbose (path,
> +					 NOTMUCH_DATABASE_MODE_READ_WRITE,
> +					 &notmuch,
> +					 &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,
> -				   &notmuch))
> +	char *status_string = NULL;
> +	if (notmuch_database_open_verbose (db_path, NOTMUCH_DATABASE_MODE_READ_WRITE,
> +					   &notmuch, &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, &notmuch);
> +  char *message = NULL;
> +
> +  if (notmuch_database_open_verbose  ("fakedb", NOTMUCH_DATABASE_MODE_READ_ONLY, &notmuch, &message))
> +      if (message) fputs (message, stderr);

free... (for consistency)

> +
>  
>    try {
>      (void) new Xapian::WritableDatabase("./nonexistant", Xapian::DB_OPEN);
> -- 
> 2.1.4


More information about the notmuch mailing list