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

classic Classic list List threaded Threaded
6 messages Options
rhn rhn
Reply | Threaded
Open this post in threaded view
|

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

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

_______________________________________________
notmuch mailing list
[hidden email]
https://notmuchmail.org/mailman/listinfo/notmuch
rhn rhn
Reply | Threaded
Open this post in threaded view
|

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

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

_______________________________________________
notmuch mailing list
[hidden email]
https://notmuchmail.org/mailman/listinfo/notmuch
rhn rhn
Reply | Threaded
Open this post in threaded view
|

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

In reply to this post by 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:[hidden email]");
+   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: [hidden email]
+== stderr ==
+EOF
+test_expect_equal_file EXPECTED OUTPUT
+
+test_done
--
2.17.2

_______________________________________________
notmuch mailing list
[hidden email]
https://notmuchmail.org/mailman/listinfo/notmuch
rhn rhn
Reply | Threaded
Open this post in threaded view
|

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

In reply to this post by 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

_______________________________________________
notmuch mailing list
[hidden email]
https://notmuchmail.org/mailman/listinfo/notmuch
Tomi Ollila-2 Tomi Ollila-2
Reply | Threaded
Open this post in threaded view
|

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

In reply to this post by rhn
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
> [hidden email]
> https://notmuchmail.org/mailman/listinfo/notmuch
_______________________________________________
notmuch mailing list
[hidden email]
https://notmuchmail.org/mailman/listinfo/notmuch
rhn-2 rhn-2
Reply | Threaded
Open this post in threaded view
|

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

On Mon, 17 Dec 2018 22:05:45 +0200
Tomi Ollila <[hidden email]> 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
> > [hidden email]
> > https://notmuchmail.org/mailman/listinfo/notmuch 

_______________________________________________
notmuch mailing list
[hidden email]
https://notmuchmail.org/mailman/listinfo/notmuch