Austin Clements yazmış:
Quoth Ali Polatel on Sep 28 at 10:53 am:On Tue, 27 Sep 2011 18:46:22 -0400, Austin Clements <amdra...@mit.edu> wrote: > Quoth David Bremner on Sep 27 at 1:59 pm: > > On Tue, 27 Sep 2011 16:25:58 +0300, Ali Polatel <pola...@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....@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
pgp4ZFZhHoO4Y.pgp
Description: PGP signature
_______________________________________________ notmuch mailing list notmuch@notmuchmail.org http://notmuchmail.org/mailman/listinfo/notmuch
