[PATCH 0/3] API: add notes on lifetimes

[PATCH 0/3] API: add notes on lifetimes

[rhn]

Hi,

this patch series addresses API shortcomings that were found while working on the Rust bindings [0].

The first two patches address the problem that the docs never clearly state when messages obtained as replies are destroyed, while the last patch fixes abroken API example.

Thanks for Dirk Van Haerenborgh for working out how long notmuch objects live.

Cheers,
rhn

[0] https://github.com/vhdirk/notmuch-rs

rhn (3):
  lib: Explicitly state when replies will be destroyed
  test: Check for replies obeying lifetime guarantees
  docs: Use correct call to notmuch_query_search_threads in usage
    example

 lib/notmuch.h             | 10 ++++-
 test/T720-lib-lifetime.sh | 83 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 92 insertions(+), 1 deletion(-)
 create mode 100755 test/T720-lib-lifetime.sh

-- 
2.17.2

[PATCH 1/3] lib: Explicitly state when replies will be destroyed

[rhn]

Without an explicit guarantee, it's not clear how to use the reference.
---
 lib/notmuch.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/lib/notmuch.h b/lib/notmuch.h
index 247f6ad7..aa032e09 100644
--- a/lib/notmuch.h
+++ b/lib/notmuch.h
@@ -1400,6 +1400,8 @@ notmuch_message_get_thread_id (notmuch_message_t *message);
  * If there are no replies to 'message', this function will return
  * NULL. (Note that notmuch_messages_valid will accept that NULL
  * value as legitimate, and simply return FALSE for it.)
+ *
+ * The returned list will be destroyed when the thread is destroyed.
  */
 notmuch_messages_t *
 notmuch_message_get_replies (notmuch_message_t *message);
-- 
2.17.2

[PATCH 2/3] test: Check for replies obeying lifetime guarantees

[rhn]

The test attempts to check that a message coming from a thread outlives its messages list and gets destroyed together with the thread.
---
 test/T720-lib-lifetime.sh | 83 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 83 insertions(+)
 create mode 100755 test/T720-lib-lifetime.sh

diff --git a/test/T720-lib-lifetime.sh b/test/T720-lib-lifetime.sh
new file mode 100755
index 00000000..3d94d4df
--- /dev/null
+++ b/test/T720-lib-lifetime.sh
@@ -0,0 +1,83 @@
+#!/usr/bin/env bash
+#
+# Copyright (c) 2018 rhn
+#
+
+
+test_description="Lifetime constraints for library"
+
+. $(dirname "$0")/test-lib.sh || exit 1
+
+add_email_corpus threading
+
+test_begin_subtest "building database"
+test_expect_success "NOTMUCH_NEW"
+
+test_begin_subtest "Message outlives parent Messages from replies"
+
+test_C ${MAIL_DIR} <<'EOF'
+#include <stdio.h>
+#include <stdlib.h>
+#include <notmuch.h>
+int main (int argc, char** argv)
+{
+   notmuch_database_t *db;
+   notmuch_status_t stat;
+   stat = notmuch_database_open (argv[1], NOTMUCH_DATABASE_MODE_READ_ONLY, &db);
+   if (stat != NOTMUCH_STATUS_SUCCESS) {
+     fprintf (stderr, "error opening database: %d\n", stat);
+     exit (1);
+   }
+
+   notmuch_query_t *query = notmuch_query_create (db, "id:B00-root@example.org");
+   notmuch_threads_t *threads;
+
+   stat = notmuch_query_search_threads (query, &threads);
+   if (stat != NOTMUCH_STATUS_SUCCESS) {
+     fprintf (stderr, "error querying threads: %d\n", stat);
+     exit (1);
+   }
+
+   if (!notmuch_threads_valid (threads)) {
+     fprintf (stderr, "invalid threads");
+     exit (1);
+   }
+
+   notmuch_thread_t *thread = notmuch_threads_get (threads);
+   notmuch_messages_t *messages = notmuch_thread_get_messages (thread);
+
+   if (!notmuch_messages_valid (messages)) {
+     fprintf (stderr, "invalid messages");
+     exit (1);
+   }
+
+   notmuch_message_t *message = notmuch_messages_get (messages);
+   notmuch_messages_t *replies = notmuch_message_get_replies (message);
+   if (!notmuch_messages_valid (replies)) {
+     fprintf (stderr, "invalid replies");
+     exit (1);
+   }
+
+   notmuch_message_t *reply = notmuch_messages_get (replies);
+
+   notmuch_messages_destroy (replies); // the reply should not get destroyed here
+   notmuch_message_destroy (message);
+   notmuch_messages_destroy (messages); // nor here
+
+   const char *mid = notmuch_message_get_message_id (reply); // should not crash when accessing
+   fprintf (stdout, "Reply id: %s\n", mid);
+   notmuch_message_destroy (reply);
+   notmuch_thread_destroy (thread); // this destroys the reply
+   notmuch_threads_destroy (threads);
+   notmuch_query_destroy (query);
+   notmuch_database_destroy (db);
+}
+EOF
+cat <<'EOF' >EXPECTED
+== stdout ==
+Reply id: B01-child@example.org
+== stderr ==
+EOF
+test_expect_equal_file EXPECTED OUTPUT
+
+test_done
-- 
2.17.2

[PATCH 3/3] docs: Use correct call to notmuch_query_search_threads in usage example

[rhn]

---
 lib/notmuch.h | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/lib/notmuch.h b/lib/notmuch.h
index aa032e09..00f69846 100644
--- a/lib/notmuch.h
+++ b/lib/notmuch.h
@@ -892,7 +892,12 @@ notmuch_query_add_tag_exclude (notmuch_query_t *query, const char *tag);
  *
  *     query = notmuch_query_create (database, query_string);
  *
- *     for (threads = notmuch_query_search_threads (query);
+ *     notmuch_status_t stat = notmuch_query_search_threads (query, &threads);
+ *     if (stat != NOTMUCH_STATUS_SUCCESS) {
+ *         goto exit;
+ *     }
+ *
+ *     for (;
  *          notmuch_threads_valid (threads);
  *          notmuch_threads_move_to_next (threads))
  *     {
@@ -901,6 +906,7 @@ notmuch_query_add_tag_exclude (notmuch_query_t *query, const char *tag);
  *         notmuch_thread_destroy (thread);
  *     }
  *
+ *  exit:
  *     notmuch_query_destroy (query);
  *
  * Note: If you are finished with a thread before its containing
-- 
2.17.2

Re: [PATCH 0/3] API: add notes on lifetimes

[Tomi Ollila]

On Mon, Dec 17 2018, rhn wrote:

> Hi,
>
> this patch series addresses API shortcomings that were found while working on the Rust bindings [0].
>
> The first two patches address the problem that the docs never clearly state when messages obtained as replies are destroyed, while the last patch fixes abroken API example.
>
> Thanks for Dirk Van Haerenborgh for working out how long notmuch objects live.

Looks good. Have to trust that change in example is correct (why would it
not be?) I hope tests pass :D

Tomi

>
> Cheers,
> rhn
>
> [0] https://github.com/vhdirk/notmuch-rs
>
> rhn (3):
>   lib: Explicitly state when replies will be destroyed
>   test: Check for replies obeying lifetime guarantees
>   docs: Use correct call to notmuch_query_search_threads in usage
>     example
>
>  lib/notmuch.h             | 10 ++++-
>  test/T720-lib-lifetime.sh | 83 +++++++++++++++++++++++++++++++++++++++
>  2 files changed, 92 insertions(+), 1 deletion(-)
>  create mode 100755 test/T720-lib-lifetime.sh
>
> -- 
> 2.17.2
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> https://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH 0/3] API: add notes on lifetimes

[rhn]

On Mon, 17 Dec 2018 22:05:45 +0200
Tomi Ollila <tomi.ollila@iki.fi> wrote:

> On Mon, Dec 17 2018, rhn wrote:
> 
> > Hi,
> >
> > this patch series addresses API shortcomings that were found while working on the Rust bindings [0].
> >
> > The first two patches address the problem that the docs never clearly state when messages obtained as replies are destroyed, while the last patch fixes abroken API example.
> >
> > Thanks for Dirk Van Haerenborgh for working out how long notmuch objects live.  
> 
> Looks good. Have to trust that change in example is correct (why would it
> not be?) I hope tests pass :D
> 
> Tomi

The tests obviously pass, you can check them yourself. They are unfortunately prone to false negatives (e.g. passes when it shouldn't), because freed memory can still be sometimes accessed.

They could be detected more reliably if valgrind's memcheck was part of the test suite, but I think this patch still adds value.

I'm eagerly hoping for a merge!

Cheers,
rhn

> 
> >
> > Cheers,
> > rhn
> >
> > [0] https://github.com/vhdirk/notmuch-rs
> >
> > rhn (3):
> >   lib: Explicitly state when replies will be destroyed
> >   test: Check for replies obeying lifetime guarantees
> >   docs: Use correct call to notmuch_query_search_threads in usage
> >     example
> >
> >  lib/notmuch.h             | 10 ++++-
> >  test/T720-lib-lifetime.sh | 83 +++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 92 insertions(+), 1 deletion(-)
> >  create mode 100755 test/T720-lib-lifetime.sh
> >
> > -- 
> > 2.17.2
> >
> > _______________________________________________
> > notmuch mailing list
> > notmuch@notmuchmail.org
> > https://notmuchmail.org/mailman/listinfo/notmuch  

Re: [PATCH 3/3] docs: Use correct call to notmuch_query_search_threads in usage example

[David Bremner]

rhn <gihu.rhn@porcupinefactory.org> writes:

> ---
>  lib/notmuch.h | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>

I'm pushed the series, with some alleged improvement to the example in
the last patch.  Sorry it took so long, I felt like I needed to double
check the internals documented by the first patch. And I also didn't
find that idea really fun ;).

d