Hi,
I went briefly through your "patch". See my comments bellow.
On Sun, 09 Jan 2011, Thomas Schwinge wrote:
> I poked at notmuch-deliver's code two weeks ago already (Ali, beware,
> there'll be some few further patches coming), and in the last hours, I
> finally had a look at notmuch.h and some of the source code. Here is a
> diff containing some comments, or to-do items. Not all are fully fledged
> (I have neither been using talloc, nor Xapian before), but perhaps
> someone nevertheless feels like commenting on them. In general I can
> say, that the notmuch code makes a solid impression. :-)
>
[...]
> diff --git a/lib/database.cc b/lib/database.cc
> index 7a00917..b08c429 100644
> --- a/lib/database.cc
> +++ b/lib/database.cc
> @@ -1012,6 +1003,7 @@ _notmuch_database_get_directory_db_path (const char
> *path)
> * is an empty string, then both directory and basename will be
> * returned as NULL.
> */
> +/* XXX: isn't there a standard libc function that can be used? */
I do not think so. There may be something in glib.
> @@ -1735,11 +1737,15 @@ notmuch_database_remove_message (notmuch_database_t
> *notmuch,
> status = NOTMUCH_STATUS_DUPLICATE_MESSAGE_ID;
> }
> }
> +
> + /* XXX: talloc_free (term); */
This is where talloc helps us - it frees the memory semi-automatically
when talloc_free (local) is called. It seems there is some mistake,
though. It would be IMHO better to fix it by the patch bellow.
diff --git a/lib/database.cc b/lib/database.cc
index 7a00917..289e41c 100644
--- a/lib/database.cc
+++ b/lib/database.cc
@@ -1710,7 +1710,7 @@ notmuch_database_remove_message (notmuch_database_t
*notmuch,
if (status)
return status;
- term = talloc_asprintf (notmuch, "%s%s", prefix, direntry);
+ term = talloc_asprintf (local, "%s%s", prefix, direntry);
find_doc_ids_for_term (notmuch, term, &i, &end);
> } catch (const Xapian::Error &error) {
> fprintf (stderr, "Error: A Xapian exception occurred removing message:
> %s\n",
> error.get_msg().c_str());
> notmuch->exception_reported = TRUE;
> status = NOTMUCH_STATUS_XAPIAN_EXCEPTION;
> +
> + /* XXX: (conditionally) talloc_free (term); */
> }
>
> talloc_free (local);
Also fixed by the above patch.
> diff --git a/lib/directory.cc b/lib/directory.cc
> index 946be4f..925b1da 100644
> --- a/lib/directory.cc
> +++ b/lib/directory.cc
> @@ -140,6 +140,7 @@ _notmuch_directory_create (notmuch_database_t *notmuch,
> char *term = talloc_asprintf (local, "%s%s",
> _find_prefix ("directory"), db_path);
> directory->doc.add_term (term, 0);
> + /* XXX?: talloc_free (term); */
Handled by talloc_free (local);
> diff --git a/lib/message.cc b/lib/message.cc
> index adcd07d..cf4e36a 100644
> --- a/lib/message.cc
> +++ b/lib/message.cc
> @@ -449,8 +450,10 @@ _notmuch_message_add_filename (notmuch_message_t
> *message,
> status = _notmuch_database_filename_to_direntry (local,
> message->notmuch,
> filename, &direntry);
> - if (status)
> + if (status) {
> + /* XXX: talloc_free (local); */
Yes, this is missing here.
> return status;
> + }
>
> _notmuch_message_add_term (message, "file-direntry", direntry);
>
> @@ -730,9 +734,12 @@ _notmuch_message_add_term (notmuch_message_t *message,
>
> term = talloc_asprintf (message, "%s%s",
> _find_prefix (prefix_name), value);
> + /* XXX: term != NULL? */
I think that on Linux, malloc et al never fail, but the check should be
here to comply with the C standard.
> - if (strlen (term) > NOTMUCH_TERM_MAX)
> + if (strlen (term) > NOTMUCH_TERM_MAX) {
> + /* XXX: talloc_free (term); */
It might be here, but if not, term will be freed with the message.
> return NOTMUCH_PRIVATE_STATUS_TERM_TOO_LONG;
> + }
>
> message->doc.add_term (term, 0);
>
> @@ -820,6 +827,7 @@ notmuch_message_add_tag (notmuch_message_t *message,
> const char *tag)
> if (strlen (tag) > NOTMUCH_TAG_MAX)
> return NOTMUCH_STATUS_TAG_TOO_LONG;
>
> + /* XXX: what if tag is already present -- added again? */
I think that it is no problem and the function bellow does nothing.
> private_status = _notmuch_message_add_term (message, "tag", tag);
> if (private_status) {
> INTERNAL_ERROR ("_notmuch_message_add_term return unexpected value:
> %d\n",
> diff --git a/lib/notmuch.h b/lib/notmuch.h
> index e508309..ffc7f8f 100644
> --- a/lib/notmuch.h
> +++ b/lib/notmuch.h
> @@ -810,6 +810,7 @@ notmuch_message_set_flag (notmuch_message_t *message,
> * For the original textual representation of the Date header from the
> * message call notmuch_message_get_header() with a header value of
> * "date". */
> +/* XXX: what if Date: was missing? */
It seems that zero is returned in this case - see _notmuch_message_set_date().
Could you please check that your proposed fixes pass the test suite and
if so send them as individual patches in order to be easily applicable
by Carl?
Thanks,
Michal
