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