Re: Concerns regarding some library functions

[Ali Polatel <polatel@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 15556 bytes --]

Austin Clements yazmış:
>Quoth Ali Polatel on Sep 28 at 10:53 am:
>> On Tue, 27 Sep 2011 18:46:22 -0400, Austin Clements <amdragon@MIT.EDU> wrote:
>> > Quoth David Bremner on Sep 27 at  1:59 pm:
>> > > On Tue, 27 Sep 2011 16:25:58 +0300, Ali Polatel <polatel@gmail.com> wrote:
>> > >
>> > > > The problem with their design is NULL return may both mean an error
>> > > > condition and "message not found". However, we already have a similar
>> > > > 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 does
>> > > sound bad. Looking at the code for notmuch-new, it looks like the return
>> > > 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 *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)
  {
      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,20 @@ 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 +1321,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 +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;
  
-    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 +1366,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());
      }
  
      talloc_free (metadata_key);
  
-    return talloc_strdup (ctx, thread_id);
+    return NOTMUCH_STATUS_SUCCESS;
  }
  
  static notmuch_status_t
@@ -1446,9 +1461,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 +1777,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);
@@ -1774,24 +1793,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)
  {
      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 +1822,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);
      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
+ * (NOTMUCH_STATUS_SUCCESS) '*message' will be initialized 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 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!
+ *
+ * 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.
   *
- * 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!
+ *
+ * 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..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 = 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;
  	}
  
-- 
1.7.6.1

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]