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 8FF0B431FD0 for ; Thu, 29 Sep 2011 23:47:21 -0700 (PDT) X-Virus-Scanned: Debian amavisd-new at olra.theworths.org X-Spam-Flag: NO X-Spam-Score: 1.7 X-Spam-Level: * X-Spam-Status: No, score=1.7 tagged_above=-999 required=5 tests=[DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, FREEMAIL_FROM=0.001, FREEMAIL_REPLY=2.499, 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 On99ozF7R-7T for ; Thu, 29 Sep 2011 23:47:20 -0700 (PDT) Received: from mail-fx0-f53.google.com (mail-fx0-f53.google.com [209.85.161.53]) (using TLSv1 with cipher RC4-SHA (128/128 bits)) (No client certificate requested) by olra.theworths.org (Postfix) with ESMTPS id A2465431FB6 for ; Thu, 29 Sep 2011 23:47:19 -0700 (PDT) Received: by fxh2 with SMTP id 2so2766800fxh.26 for ; Thu, 29 Sep 2011 23:47:17 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=date:from:to:cc:subject:message-id:mail-followup-to:references :mime-version:content-type:content-disposition:in-reply-to :organization:user-agent; bh=9ekuy4Vjd2tLYH+VlnBP5NtLtE+GLEf8HsViLiCTDP0=; b=N/mCnz28IW14npGwgodLyDTcZM8SoX1VvK7zoDWzLQsDxo3RiwDhpHOtCI44cxUA2T s4zzqtStdMCWG0u24SMg1UvDxbh46h8nZcKZm5C9bEL2KjpvUyvDDdTcX5YAYmzRDa64 bRArnfOHiGjtHhU/VzdXh22+CwQOYJTmdfFho= Received: by 10.223.63.8 with SMTP id z8mr5144200fah.84.1317365237034; Thu, 29 Sep 2011 23:47:17 -0700 (PDT) Received: from localhost ([78.183.84.0]) by mx.google.com with ESMTPS id x22sm5423406faa.5.2011.09.29.23.47.14 (version=TLSv1/SSLv3 cipher=OTHER); Thu, 29 Sep 2011 23:47:15 -0700 (PDT) Date: Fri, 30 Sep 2011 09:47:12 +0300 From: Ali Polatel To: Austin Clements Subject: Re: Concerns regarding some library functions Message-ID: <20110930064712.GA30012@hayalet> Mail-Followup-To: Austin Clements , David Bremner , notmuch@notmuchmail.org References: <871uv2unfd.fsf@gmail.com> <87fwjhx6p5.fsf@convex-new.cs.unb.ca> <20110927224622.GR17905@mit.edu> <877h4tyug1.fsf@gmail.com> <20110929145129.GB17905@mit.edu> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="mYCpIKhGyMATD0i+" Content-Disposition: inline In-Reply-To: <20110929145129.GB17905@mit.edu> Organization: Pink Floyd User-Agent: Mutt/1.5.21 (2010-09-15) Cc: notmuch@notmuchmail.org 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: Fri, 30 Sep 2011 06:47:21 -0000 --mYCpIKhGyMATD0i+ Content-Type: text/plain; charset=utf-8; format=flowed Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Austin Clements yazm=C4=B1=C5=9F: >Quoth Ali Polatel on Sep 28 at 10:53 am: >> On Tue, 27 Sep 2011 18:46:22 -0400, Austin Clements w= rote: >> > Quoth David Bremner on Sep 27 at 1:59 pm: >> > > On Tue, 27 Sep 2011 16:25:58 +0300, Ali Polatel = wrote: >> > > >> > > > The problem with their design is NULL return may both mean an error >> > > > condition and "message not found". However, we already have a simi= lar >> > > > function which does not have such a flaw, namely notmuch_database_= add_message(). >> > > >> > > So, I take there is no way to distinguish those two outcomes? That d= oes >> > > sound bad. Looking at the code for notmuch-new, it looks like the re= turn >> > > value of notmuch_database_find_message_by_filename is used without >> > > checking it for NULL. Austin, can you comment on that at all? >> > >> > I'd be happy to distinguish these outcomes. I did >> > notmuch_database_find_message_by_filename the way I did only to be >> > consistent with notmuch_database_find_message. Since ndfmbf isn't >> > entrenched yet, now is a good time to change it. >> >> What about notmuch_database_find_message()? If we leave it as it is, >> this will lead to inconsistency and if we change it, we need to think >> about API breakage issues. > >Yes. We could just deal with that (there aren't *that* many API >consumers). For binary compatibility, I suppose we could even use >symbol versioning. > >> > The call in notmuch-new should check the return, though if it can't >> > find the message at that point, something has gone terribly wrong. >> > Segfaulting is never the answer, though. >> >> Indeed, just not to step on each other's feet, are you going to write a >> patch or shall I start writing one? > >Please feel free to write a patch. Below is a quick patch, which compiles and passes tests. Please comment. -alip -- >8 -- Subject: [PATCH] lib: make find_message{,by_filename) report errors 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 | 89 ++++++++++++++++++++++++++++++++++--------------= ---- lib/message.cc | 6 ++-- lib/notmuch.h | 61 +++++++++++++++++++++++++----------- notmuch-new.c | 4 ++- notmuch-restore.c | 11 ++++-- 5 files changed, 114 insertions(+), 57 deletions(-) diff --git a/lib/database.cc b/lib/database.cc index 9299c8d..6641aa5 100644 --- a/lib/database.cc +++ b/lib/database.cc @@ -360,13 +360,17 @@ _message_id_compressed (void *ctx, const char *messag= e_id) return compressed; } =20 -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) { notmuch_private_status_t status; unsigned int doc_id; =20 + if (message =3D=3D NULL) + return NOTMUCH_STATUS_NULL_POINTER; + if (strlen (message_id) > NOTMUCH_MESSAGE_ID_MAX) message_id =3D _message_id_compressed (notmuch, message_id); =20 @@ -375,14 +379,20 @@ notmuch_database_find_message (notmuch_database_t *no= tmuch, message_id, &doc_id); =20 if (status =3D=3D NOTMUCH_PRIVATE_STATUS_NO_DOCUMENT_FOUND) - return NULL; + *message =3D NULL; + else { + *message =3D _notmuch_message_create (notmuch, notmuch, doc_id, NULL); + if (*message =3D=3D NULL) + return NOTMUCH_STATUS_OUT_OF_MEMORY; + } =20 - 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 =3D TRUE; - return NULL; + *message =3D NULL; + return NOTMUCH_STATUS_XAPIAN_EXCEPTION; } } =20 @@ -1311,7 +1321,8 @@ _get_metadata_thread_id_key (void *ctx, const char *m= essage_id) =20 /* 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 +1330,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) { + notmuch_status_t status; notmuch_message_t *message; string thread_id_string; - const char *thread_id; char *metadata_key; Xapian::WritableDatabase *db; =20 - message =3D notmuch_database_find_message (notmuch, message_id); + status =3D notmuch_database_find_message (notmuch, message_id, &messag= e); + + if (status) + return status; =20 if (message) { - thread_id =3D talloc_steal (ctx, notmuch_message_get_thread_id (message)); + *thread_id =3D talloc_steal (ctx, notmuch_message_get_thread_id (message)= ); =20 notmuch_message_destroy (message); =20 - return thread_id; + return NOTMUCH_STATUS_SUCCESS; } =20 /* Message has not been seen yet. @@ -1351,15 +1366,15 @@ _resolve_message_id_to_thread_id (notmuch_database_= t *notmuch, thread_id_string =3D notmuch->xapian_db->get_metadata (metadata_key); =20 if (thread_id_string.empty()) { - thread_id =3D _notmuch_database_generate_thread_id (notmuch); - db->set_metadata (metadata_key, thread_id); + *thread_id =3D talloc_strdup(ctx, _notmuch_database_generate_thread_id (n= otmuch)); + db->set_metadata (metadata_key, *thread_id); } else { - thread_id =3D thread_id_string.c_str(); + *thread_id =3D talloc_strdup(ctx, thread_id_string.c_str()); } =20 talloc_free (metadata_key); =20 - return talloc_strdup (ctx, thread_id); + return NOTMUCH_STATUS_SUCCESS; } =20 static notmuch_status_t @@ -1446,9 +1461,12 @@ _notmuch_database_link_message_to_parents (notmuch_d= atabase_t *notmuch, _notmuch_message_add_term (message, "reference", parent_message_id); =20 - parent_thread_id =3D _resolve_message_id_to_thread_id (notmuch, - message, - parent_message_id); + ret =3D _resolve_message_id_to_thread_id (notmuch, + message, + parent_message_id, + &parent_thread_id); + if (ret) + goto DONE; =20 if (*thread_id =3D=3D NULL) { *thread_id =3D talloc_strdup (message, parent_thread_id); @@ -1759,11 +1777,12 @@ notmuch_status_t notmuch_database_remove_message (notmuch_database_t *notmuch, const char *filename) { - notmuch_message_t *message =3D - notmuch_database_find_message_by_filename (notmuch, filename); - notmuch_status_t status =3D NOTMUCH_STATUS_SUCCESS; + notmuch_status_t status; + notmuch_message_t *message; =20 - if (message) { + status =3D notmuch_database_find_message_by_filename (notmuch, filenam= e, &message); + + if (status =3D=3D NOTMUCH_STATUS_SUCCESS && message) { status =3D _notmuch_message_remove_filename (message, filename); if (status =3D=3D NOTMUCH_STATUS_SUCCESS) _notmuch_message_delete (message); @@ -1774,24 +1793,27 @@ notmuch_database_remove_message (notmuch_database_t= *notmuch, return status; } =20 -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) { void *local; const char *prefix =3D _find_prefix ("file-direntry"); char *direntry, *term; Xapian::PostingIterator i, end; - notmuch_message_t *message =3D NULL; notmuch_status_t status; =20 + if (message =3D=3D NULL) + return NOTMUCH_STATUS_NULL_POINTER; + local =3D talloc_new (notmuch); =20 try { status =3D _notmuch_database_filename_to_direntry (local, notmuch, filename, &direntry); if (status) - return NULL; + goto DONE; =20 term =3D talloc_asprintf (local, "%s%s", prefix, direntry); =20 @@ -1800,19 +1822,24 @@ notmuch_database_find_message_by_filename (notmuch_= database_t *notmuch, if (i !=3D end) { notmuch_private_status_t private_status; =20 - message =3D _notmuch_message_create (notmuch, notmuch, - *i, &private_status); + *message =3D _notmuch_message_create (notmuch, notmuch, + *i, &private_status); + if (*message =3D=3D NULL) + status =3D 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 =3D TRUE; - message =3D NULL; + status =3D NOTMUCH_STATUS_OUT_OF_MEMORY; } =20 + DONE: talloc_free (local); =20 - return message; + if (status) + *message =3D NULL; + return status; } =20 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_datab= ase_t *notmuch, unsigned int doc_id; char *term; =20 - *status_ret =3D NOTMUCH_PRIVATE_STATUS_SUCCESS; - - message =3D notmuch_database_find_message (notmuch, message_id); + *status_ret =3D (notmuch_private_status_t) notmuch_database_find_messa= ge (notmuch, message_id, &message); if (message) return talloc_steal (notmuch, message); + else if (*status_ret) + return NULL; =20 term =3D 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, =20 /* 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 + * (NOTMUCH_STATUS_SUCCESS) '*message' will be initialized to a message ob= ject. + * The user should call notmuch_message_destroy when done with the message. * - * This function returns NULL in the following situations: + * On any failure or when the message is not found, this function initiali= zes + * '*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! + * + * 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); =20 /* 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=20 - * 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 initializ= ed to + * a message object. The user should call notmuch_message_destroy when done + * with the message. * - * This function returns NULL in the following situations: + * On any failure or when the message is not found, this function initiali= zes + * '*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! + * + * 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); =20 /* 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 =3D notmuch_database_begin_atomic (notmuch); if (status) return status; - message =3D notmuch_database_find_message_by_filename (notmuch, path); + status =3D notmuch_database_find_message_by_filename (notmuch, path, &= message); + if (status || message =3D=3D NULL) + return status; status =3D notmuch_database_remove_message (notmuch, path); if (status =3D=3D NOTMUCH_STATUS_DUPLICATE_MESSAGE_ID) { add_files_state->renamed_messages++; diff --git a/notmuch-restore.c b/notmuch-restore.c index f095f64..3b0ae71 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 =3D xstrndup (line + match[2].rm_so, match[2].rm_eo - match[2].rm_so); =20 - message =3D notmuch_database_find_message (notmuch, message_id); - if (message =3D=3D NULL) { - fprintf (stderr, "Warning: Cannot apply tags to missing message: %s\n= ", - message_id); + status =3D notmuch_database_find_message (notmuch, message_id, &message); + if (status || message =3D=3D 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; } =20 --=20 1.7.6.1 --mYCpIKhGyMATD0i+ Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.18 (GNU/Linux) iEYEARECAAYFAk6FZfAACgkQQU4yORhF8iDwZACgroTylz80BdwAwFPMU27a7svv IqEAoIBDNgFG5/DltzS29qIKUeJ+QWbs =K7FZ -----END PGP SIGNATURE----- --mYCpIKhGyMATD0i+--