From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by olra.theworths.org (Postfix) with ESMTP id B46FE429E21 for ; Mon, 3 Oct 2011 10:40:51 -0700 (PDT) X-Virus-Scanned: Debian amavisd-new at olra.theworths.org X-Spam-Flag: NO X-Spam-Score: -0.7 X-Spam-Level: X-Spam-Status: No, score=-0.7 tagged_above=-999 required=5 tests=[RCVD_IN_DNSWL_LOW=-0.7] 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 d+MARKky+BqA for ; Mon, 3 Oct 2011 10:40:50 -0700 (PDT) Received: from dmz-mailsec-scanner-4.mit.edu (DMZ-MAILSEC-SCANNER-4.MIT.EDU [18.9.25.15]) by olra.theworths.org (Postfix) with ESMTP id 46143431FB6 for ; Mon, 3 Oct 2011 10:40:50 -0700 (PDT) X-AuditID: 1209190f-b7f6e6d0000008df-b7-4e89f3a0facd Received: from mailhub-auth-1.mit.edu ( [18.9.21.35]) by dmz-mailsec-scanner-4.mit.edu (Symantec Messaging Gateway) with SMTP id 27.B0.02271.0A3F98E4; Mon, 3 Oct 2011 13:40:48 -0400 (EDT) Received: from outgoing.mit.edu (OUTGOING-AUTH.MIT.EDU [18.7.22.103]) by mailhub-auth-1.mit.edu (8.13.8/8.9.2) with ESMTP id p93HemLv012854; Mon, 3 Oct 2011 13:40:48 -0400 Received: from awakening.csail.mit.edu (awakening.csail.mit.edu [18.26.4.91]) (authenticated bits=0) (User authenticated as amdragon@ATHENA.MIT.EDU) by outgoing.mit.edu (8.13.6/8.12.4) with ESMTP id p93Hekw5028698 (version=TLSv1/SSLv3 cipher=AES256-SHA bits=256 verify=NOT); Mon, 3 Oct 2011 13:40:47 -0400 (EDT) Received: from amthrax by awakening.csail.mit.edu with local (Exim 4.72) (envelope-from ) id 1RAmXk-0003SK-Nc; Mon, 03 Oct 2011 13:43:08 -0400 Date: Mon, 3 Oct 2011 13:43:08 -0400 From: Austin Clements To: Ali Polatel Subject: Re: [PATCH v1 1/1] lib: make find_message{,by_filename) report errors Message-ID: <20111003174308.GD17905@mit.edu> References: <1218582065f35bfcc5b84dfc1fbce21fc05034a6.1317660324.git.alip@exherbo.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1218582065f35bfcc5b84dfc1fbce21fc05034a6.1317660324.git.alip@exherbo.org> User-Agent: Mutt/1.5.20 (2009-06-14) X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFmpmleLIzCtJLcpLzFFi42IR4hRV1l3wudPP4MUSA4vrN2cyW/Tt+cbq wOSxc9Zddo9nq24xBzBFcdmkpOZklqUW6dslcGXMf3+GpeB7RcW3d9uYGxhnxHUxcnJICJhI zJ7wmwXCFpO4cG89WxcjF4eQwD5GiduXrkI56xkluhb8YYRwTjBJfG15yQLhLGGUWPlxPxtI P4uAisSWS1fAbDYBDYlt+5cDdXBwiAgoS/RtTwQJMwsYSRw+fQ5snbBAgMTJI3sYQWxeAR2J I/23WSFmNjBKvHz2jg0iIShxcuYTFohmLYkb/14ygcxkFpCWWP6PA8TkFAiXuNCvAFIhCnTB tf3tbBMYhWYhaZ6FpHkWQvMCRuZVjLIpuVW6uYmZOcWpybrFyYl5ealFuiZ6uZkleqkppZsY wWEtyb+D8dtBpUOMAhyMSjy8iuKdfkKsiWXFlbmHGCU5mJREeVd8AArxJeWnVGYkFmfEF5Xm pBYfYpTgYFYS4d1+GyjHm5JYWZValA+TkuZgURLnbdzh4CckkJ5YkpqdmlqQWgSTleHgUJLg nfgJqFGwKDU9tSItM6cEIc3EwQkynAdo+H6QGt7igsTc4sx0iPwpRkUpcd6pIAkBkERGaR5c LyztvGIUB3pFmHcvSBUPMGXBdb8CGswENDj1LtjgkkSElFQD4wK/J+zSpSeY7vuXLziW+OOW zPvtmmvjFVzzfh3juZzfr93W8WzXW+aGmolrk3UY+21tV/y7GbMu82m0WkXmi7rpjOdY9W3u bXiRzvR7j+/cmAbDiXenXZ7u+fbBiWV2AWHMj0+rTXPdfGR155OqH3euvd+/qNOi/+8ng6v3 dxYFXvzP/vTHsiglluKMREMt5qLiRACkjqgzFgMAAA== Cc: Notmuch Mailing List 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: Mon, 03 Oct 2011 17:40:51 -0000 All of the code looks good to me (and improving error handling is always a good thing). Some style nits are inline below. The changes to _resolve_message_id_to_thread_id and _notmuch_database_link_message_to_parents seem like just plain better error handling and unrelated to the find_message API changes. Perhaps these should be in a separate patch? Quoth Ali Polatel on Oct 03 at 7:49 pm: > Previously, the functions notmuch_database_find_message() and > notmuch_database_find_message_by_filename() functions did not properly > report error condition to the library user. > > For more information, read the thread on the notmuch mailing list > starting with my mail "id:871uv2unfd.fsf@gmail.com" > > Make these functions accept a pointer to 'notmuch_message_t' as argument > and return notmuch_status_t which may be used to check for any error > condition. > > restore: Modify for the new notmuch_database_find_message() > new: Modify for the new notmuch_database_find_message_by_filename() > --- > lib/database.cc | 90 ++++++++++++++++++++++++++++++++++------------------ > lib/message.cc | 6 ++-- > lib/notmuch.h | 61 +++++++++++++++++++++++++---------- > notmuch-new.c | 4 ++- > notmuch-restore.c | 11 ++++-- > 5 files changed, 115 insertions(+), 57 deletions(-) > > diff --git a/lib/database.cc b/lib/database.cc > index 9299c8d..1705232 100644 > --- a/lib/database.cc > +++ b/lib/database.cc > @@ -360,13 +360,17 @@ _message_id_compressed (void *ctx, const char *message_id) > return compressed; > } > > -notmuch_message_t * > +notmuch_status_t > notmuch_database_find_message (notmuch_database_t *notmuch, > - const char *message_id) > + const char *message_id, > + notmuch_message_t **message) Perhaps this should be message_ret to clearly distinguish it as an out-argument? > { > notmuch_private_status_t status; > unsigned int doc_id; > > + if (message == NULL) > + return NOTMUCH_STATUS_NULL_POINTER; > + > if (strlen (message_id) > NOTMUCH_MESSAGE_ID_MAX) > message_id = _message_id_compressed (notmuch, message_id); > > @@ -375,14 +379,21 @@ notmuch_database_find_message (notmuch_database_t *notmuch, > message_id, &doc_id); > > if (status == NOTMUCH_PRIVATE_STATUS_NO_DOCUMENT_FOUND) > - return NULL; > + *message = NULL; > + else { > + *message = _notmuch_message_create (notmuch, notmuch, doc_id, > + NULL); > + if (*message == NULL) > + return NOTMUCH_STATUS_OUT_OF_MEMORY; > + } > > - return _notmuch_message_create (notmuch, notmuch, doc_id, NULL); > + return NOTMUCH_STATUS_SUCCESS; > } catch (const Xapian::Error &error) { > fprintf (stderr, "A Xapian exception occurred finding message: %s.\n", > error.get_msg().c_str()); > notmuch->exception_reported = TRUE; > - return NULL; > + *message = NULL; > + return NOTMUCH_STATUS_XAPIAN_EXCEPTION; > } > } > > @@ -1311,7 +1322,8 @@ _get_metadata_thread_id_key (void *ctx, const char *message_id) > > /* Find the thread ID to which the message with 'message_id' belongs. > * > - * Always returns a newly talloced string belonging to 'ctx'. > + * Note: 'thread_id' must not be NULL! > + * On success '*thread_id' is set to a newly talloced string belonging to 'ctx'. > * > * Note: If there is no message in the database with the given > * 'message_id' then a new thread_id will be allocated for this > @@ -1319,25 +1331,29 @@ _get_metadata_thread_id_key (void *ctx, const char *message_id) > * thread ID can be looked up if the message is added to the database > * later). > */ > -static const char * > +static notmuch_status_t > _resolve_message_id_to_thread_id (notmuch_database_t *notmuch, > void *ctx, > - const char *message_id) > + const char *message_id, > + const char **thread_id) thread_id_ret? > { > + notmuch_status_t status; > notmuch_message_t *message; > string thread_id_string; > - const char *thread_id; > char *metadata_key; > Xapian::WritableDatabase *db; > > - message = notmuch_database_find_message (notmuch, message_id); > + status = notmuch_database_find_message (notmuch, message_id, &message); > + > + if (status) > + return status; > > if (message) { > - thread_id = talloc_steal (ctx, notmuch_message_get_thread_id (message)); > + *thread_id = talloc_steal (ctx, notmuch_message_get_thread_id (message)); > > notmuch_message_destroy (message); > > - return thread_id; > + return NOTMUCH_STATUS_SUCCESS; > } > > /* Message has not been seen yet. > @@ -1351,15 +1367,15 @@ _resolve_message_id_to_thread_id (notmuch_database_t *notmuch, > thread_id_string = notmuch->xapian_db->get_metadata (metadata_key); > > if (thread_id_string.empty()) { > - thread_id = _notmuch_database_generate_thread_id (notmuch); > - db->set_metadata (metadata_key, thread_id); > + *thread_id = talloc_strdup (ctx, _notmuch_database_generate_thread_id (notmuch)); > + db->set_metadata (metadata_key, *thread_id); > } else { > - thread_id = thread_id_string.c_str(); > + *thread_id = talloc_strdup(ctx, thread_id_string.c_str()); Missing space after talloc_strdup. > } > > talloc_free (metadata_key); > > - return talloc_strdup (ctx, thread_id); > + return NOTMUCH_STATUS_SUCCESS; > } > > static notmuch_status_t > @@ -1446,9 +1462,12 @@ _notmuch_database_link_message_to_parents (notmuch_database_t *notmuch, > _notmuch_message_add_term (message, "reference", > parent_message_id); > > - parent_thread_id = _resolve_message_id_to_thread_id (notmuch, > - message, > - parent_message_id); > + ret = _resolve_message_id_to_thread_id (notmuch, > + message, > + parent_message_id, > + &parent_thread_id); > + if (ret) > + goto DONE; > > if (*thread_id == NULL) { > *thread_id = talloc_strdup (message, parent_thread_id); > @@ -1759,11 +1778,12 @@ notmuch_status_t > notmuch_database_remove_message (notmuch_database_t *notmuch, > const char *filename) > { > - notmuch_message_t *message = > - notmuch_database_find_message_by_filename (notmuch, filename); > - notmuch_status_t status = NOTMUCH_STATUS_SUCCESS; > + notmuch_status_t status; > + notmuch_message_t *message; > > - if (message) { > + status = notmuch_database_find_message_by_filename (notmuch, filename, &message); > + > + if (status == NOTMUCH_STATUS_SUCCESS && message) { > status = _notmuch_message_remove_filename (message, filename); > if (status == NOTMUCH_STATUS_SUCCESS) > _notmuch_message_delete (message); This isn't a problem with your patch, but shouldn't this function be notmuch_message_destroy'ing message? > @@ -1774,24 +1794,27 @@ notmuch_database_remove_message (notmuch_database_t *notmuch, > return status; > } > > -notmuch_message_t * > +notmuch_status_t > notmuch_database_find_message_by_filename (notmuch_database_t *notmuch, > - const char *filename) > + const char *filename, > + notmuch_message_t **message) message_ret? > { > void *local; > const char *prefix = _find_prefix ("file-direntry"); > char *direntry, *term; > Xapian::PostingIterator i, end; > - notmuch_message_t *message = NULL; > notmuch_status_t status; > > + if (message == NULL) > + return NOTMUCH_STATUS_NULL_POINTER; > + > local = talloc_new (notmuch); > > try { > status = _notmuch_database_filename_to_direntry (local, notmuch, > filename, &direntry); > if (status) > - return NULL; > + goto DONE; > > term = talloc_asprintf (local, "%s%s", prefix, direntry); > > @@ -1800,19 +1823,24 @@ notmuch_database_find_message_by_filename (notmuch_database_t *notmuch, > if (i != end) { > notmuch_private_status_t private_status; > > - message = _notmuch_message_create (notmuch, notmuch, > - *i, &private_status); > + *message = _notmuch_message_create (notmuch, notmuch, > + *i, &private_status); > + if (*message == NULL) > + status = NOTMUCH_STATUS_OUT_OF_MEMORY; > } > } catch (const Xapian::Error &error) { > fprintf (stderr, "Error: A Xapian exception occurred finding message by filename: %s\n", > error.get_msg().c_str()); > notmuch->exception_reported = TRUE; > - message = NULL; > + status = NOTMUCH_STATUS_OUT_OF_MEMORY; > } > > + DONE: > talloc_free (local); > > - return message; > + if (status) > + *message = NULL; > + return status; > } > > notmuch_string_list_t * > diff --git a/lib/message.cc b/lib/message.cc > index 531d304..2a85744 100644 > --- a/lib/message.cc > +++ b/lib/message.cc > @@ -216,11 +216,11 @@ _notmuch_message_create_for_message_id (notmuch_database_t *notmuch, > unsigned int doc_id; > char *term; > > - *status_ret = NOTMUCH_PRIVATE_STATUS_SUCCESS; > - > - message = notmuch_database_find_message (notmuch, message_id); > + *status_ret = (notmuch_private_status_t) notmuch_database_find_message (notmuch, message_id, &message); Long line, should probably be wrapped. > if (message) > return talloc_steal (notmuch, message); > + else if (*status_ret) > + return NULL; > > term = talloc_asprintf (NULL, "%s%s", > _find_prefix ("id"), message_id); > diff --git a/lib/notmuch.h b/lib/notmuch.h > index 6d7a99f..08b4ce2 100644 > --- a/lib/notmuch.h > +++ b/lib/notmuch.h > @@ -347,35 +347,60 @@ notmuch_database_remove_message (notmuch_database_t *database, > > /* Find a message with the given message_id. > * > - * If the database contains a message with the given message_id, then > - * a new notmuch_message_t object is returned. The caller should call > - * notmuch_message_destroy when done with the message. > + * If message with the given message_id is found then, on successful return "If a message" > + * (NOTMUCH_STATUS_SUCCESS) '*message' will be initialized to a message object. > + * The user should call notmuch_message_destroy when done with the message. Probably s/user/caller/, since "the user" is the person sitting at the keyboard. (notmuch.h isn't very good about this, but it does use "caller" more often than "user"). > - * This function returns NULL in the following situations: > + * On any failure or when the message is not found, this function initializes > + * '*message' to NULL. This means, when NOTMUCH_STATUS_SUCCESS is returned, the > + * user is supposed to check '*message' for NULL to find out whether the > + * message with the given message_id was found. > * > - * * No message is found with the given message_id > - * * An out-of-memory situation occurs > - * * A Xapian exception occurs > + * Note: The argument 'message' must not be NULL! Since it's clearly specified what happens if 'message' is NULL, this note isn't necessary. > + * > + * Return value: > + * > + * NOTMUCH_STATUS_SUCCESS: Successful return, check '*message'. > + * > + * NOTMUCH_STATUS_NULL_POINTER: The given 'message' argument is NULL > + * > + * NOTMUCH_STATUS_OUT_OF_MEMORY: Out of memory, creating message object > + * > + * NOTMUCH_STATUS_XAPIAN_EXCEPTION: A Xapian exception occurred > */ > -notmuch_message_t * > +notmuch_status_t > notmuch_database_find_message (notmuch_database_t *database, > - const char *message_id); > + const char *message_id, > + notmuch_message_t **message); > > /* Find a message with the given filename. > * > - * If the database contains a message with the given filename, then a > - * new notmuch_message_t object is returned. The caller should call > - * notmuch_message_destroy when done with the message. > + * If the database contains a message with the given filename then, on > + * successful return (NOTMUCH_STATUS_SUCCESS) '*message' will be initialized to > + * a message object. The user should call notmuch_message_destroy when done > + * with the message. Same comment about "user" as above. > * > - * This function returns NULL in the following situations: > + * On any failure or when the message is not found, this function initializes > + * '*message' to NULL. This means, when NOTMUCH_STATUS_SUCCESS is returned, the > + * user is supposed to check '*message' for NULL to find out whether the > + * message with the given filename is found. > * > - * * No message is found with the given filename > - * * An out-of-memory situation occurs > - * * A Xapian exception occurs > + * Note: The argument 'message' must not be NULL! Same comment about this note as above. > + * > + * Return value: > + * > + * NOTMUCH_STATUS_SUCCESS: Successful return, check '*message' > + * > + * NOTMUCH_STATUS_NULL_POINTER: The given 'message' argument is NULL > + * > + * NOTMUCH_STATUS_OUT_OF_MEMORY: Out of memory, creating the message object > + * > + * NOTMUCH_STATUS_XAPIAN_EXCEPTION: A Xapian exception occurred > */ > -notmuch_message_t * > +notmuch_status_t > notmuch_database_find_message_by_filename (notmuch_database_t *notmuch, > - const char *filename); > + const char *filename, > + notmuch_message_t **message); > > /* Return a list of all tags found in the database. > * > diff --git a/notmuch-new.c b/notmuch-new.c > index e79593c..96a1e31 100644 > --- a/notmuch-new.c > +++ b/notmuch-new.c > @@ -743,7 +743,9 @@ remove_filename (notmuch_database_t *notmuch, > status = notmuch_database_begin_atomic (notmuch); > if (status) > return status; > - message = notmuch_database_find_message_by_filename (notmuch, path); > + status = notmuch_database_find_message_by_filename (notmuch, path, &message); > + if (status || message == NULL) > + return status; > status = notmuch_database_remove_message (notmuch, path); > if (status == NOTMUCH_STATUS_DUPLICATE_MESSAGE_ID) { > add_files_state->renamed_messages++; > diff --git a/notmuch-restore.c b/notmuch-restore.c > index f095f64..e4a5355 100644 > --- a/notmuch-restore.c > +++ b/notmuch-restore.c > @@ -87,10 +87,13 @@ notmuch_restore_command (unused (void *ctx), int argc, char *argv[]) > file_tags = xstrndup (line + match[2].rm_so, > match[2].rm_eo - match[2].rm_so); > > - message = notmuch_database_find_message (notmuch, message_id); > - if (message == NULL) { > - fprintf (stderr, "Warning: Cannot apply tags to missing message: %s\n", > - message_id); > + status = notmuch_database_find_message (notmuch, message_id, &message); > + if (status || message == NULL) { > + fprintf (stderr, "Warning: Cannot apply tags to %smessage: %s\n", > + message ? "" : "missing ", message_id); > + if (status) > + fprintf (stderr, "%s\n", > + notmuch_status_to_string(status)); > goto NEXT_LINE; > } > -- Austin Clements MIT/'06/PhD/CSAIL amdragon@mit.edu http://web.mit.edu/amdragon Somewhere in the dream we call reality you will find me, searching for the reality we call dreams.