* [PATCH] lib: add talloc reference from string map iterator to map
@ 2016-09-23 9:33 David Bremner
2016-09-24 9:54 ` Tomi Ollila
2016-09-24 13:18 ` David Bremner
0 siblings, 2 replies; 5+ messages in thread
From: David Bremner @ 2016-09-23 9:33 UTC (permalink / raw)
To: notmuch
This is needed so that when the map is modified during traversal, and
thus unlinked by the database code, the map is not disposed of until the
iterator is done with it.
---
According to my obviously fallible memory, this was always intended to
work something like this. For me, this change fixes the test failures
in T610 on Debian stable. The same bad behaviour is visible by running
T610/test6 under valgrind even in places where the test passes.
lib/string-map.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/lib/string-map.c b/lib/string-map.c
index 591ff6d..0bb77e9 100644
--- a/lib/string-map.c
+++ b/lib/string-map.c
@@ -170,6 +170,9 @@ _notmuch_string_map_iterator_create (notmuch_string_map_t *map, const char *key,
if (unlikely (iter == NULL))
return NULL;
+ if (unlikely (talloc_reference (iter, map) == NULL))
+ return NULL;
+
iter->key = talloc_strdup (iter, key);
iter->exact = exact;
iter->current = bsearch_first (map->pairs, map->length, key, exact);
--
2.1.4
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH] lib: add talloc reference from string map iterator to map
2016-09-23 9:33 [PATCH] lib: add talloc reference from string map iterator to map David Bremner
@ 2016-09-24 9:54 ` Tomi Ollila
2016-09-24 11:16 ` David Bremner
2016-09-24 13:18 ` David Bremner
1 sibling, 1 reply; 5+ messages in thread
From: Tomi Ollila @ 2016-09-24 9:54 UTC (permalink / raw)
To: David Bremner, notmuch
On Fri, Sep 23 2016, David Bremner <david@tethera.net> wrote:
> This is needed so that when the map is modified during traversal, and
> thus unlinked by the database code, the map is not disposed of until the
> iterator is done with it.
> ---
>
> According to my obviously fallible memory, this was always intended to
> work something like this. For me, this change fixes the test failures
> in T610 on Debian stable. The same bad behaviour is visible by running
> T610/test6 under valgrind even in places where the test passes.
For me the test pass on Scientific Linux 6.2, Ubuntu 16.04 and Fedora 24.
I tried to run this under valgrind ( ./T610-message-property.sh --valgrind )
but got so noisy output that I could not resolve anything definite from it.
Also /T600-named-queries.sh --val printed noisy output -- but perhaps
the noise difference (less there) can inform something.
Anyway, this seems to fix a bug that might affect someone, and at least
the situation is better with this so this should be pushed soon...
Btw: does 'Debian stable' refer to Debian 8.6(+) -- just for the record
anyone reading this message years from now :D
> lib/string-map.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/lib/string-map.c b/lib/string-map.c
> index 591ff6d..0bb77e9 100644
> --- a/lib/string-map.c
> +++ b/lib/string-map.c
> @@ -170,6 +170,9 @@ _notmuch_string_map_iterator_create (notmuch_string_map_t *map, const char *key,
> if (unlikely (iter == NULL))
> return NULL;
>
> + if (unlikely (talloc_reference (iter, map) == NULL))
> + return NULL;
> +
> iter->key = talloc_strdup (iter, key);
> iter->exact = exact;
> iter->current = bsearch_first (map->pairs, map->length, key, exact);
> --
> 2.1.4
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] lib: add talloc reference from string map iterator to map
2016-09-24 9:54 ` Tomi Ollila
@ 2016-09-24 11:16 ` David Bremner
2016-09-24 11:43 ` Tomi Ollila
0 siblings, 1 reply; 5+ messages in thread
From: David Bremner @ 2016-09-24 11:16 UTC (permalink / raw)
To: Tomi Ollila, notmuch
Tomi Ollila <tomi.ollila@iki.fi> writes:
>
> I tried to run this under valgrind ( ./T610-message-property.sh --valgrind )
> but got so noisy output that I could not resolve anything definite
> from it.
Hmm. On a related topic, --valgrind seems broken here:
╭─ zancas:software/upstream/notmuch/test
╰─ (git)-[master]-% ./T610-message-property.sh --val
./test-lib.sh:[:18: unknown condition: -lt
./test-lib.sh:27: command not found: shopt
test-lib: Testing message property API
cc1: error: unrecognised debug output level ' -O0'
All the tests fail because none of the binaries get built.
Same results if I run from bash instead of my normal zsh.
> Also /T600-named-queries.sh --val printed noisy output -- but perhaps
> the noise difference (less there) can inform something.
I ran
% ./T610-message-property --debug
% cd tmp.T610-message-property
% LD_LIBRARY_PATH=../../lib valgrind ./test6 `pwd`/mail
>
> Anyway, this seems to fix a bug that might affect someone, and at least
> the situation is better with this so this should be pushed soon...
>
> Btw: does 'Debian stable' refer to Debian 8.6(+) -- just for the record
> anyone reading this message years from now :D
Yes, thanks for reminding me to upgrade ;). I tested and the failure is
there with 8.6
d
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] lib: add talloc reference from string map iterator to map
2016-09-24 11:16 ` David Bremner
@ 2016-09-24 11:43 ` Tomi Ollila
0 siblings, 0 replies; 5+ messages in thread
From: Tomi Ollila @ 2016-09-24 11:43 UTC (permalink / raw)
To: David Bremner, notmuch
On Sat, Sep 24 2016, David Bremner <david@tethera.net> wrote:
> Tomi Ollila <tomi.ollila@iki.fi> writes:
>>
>> I tried to run this under valgrind ( ./T610-message-property.sh --valgrind )
>> but got so noisy output that I could not resolve anything definite
>> from it.
>
> Hmm. On a related topic, --valgrind seems broken here:
>
> ╭─ zancas:software/upstream/notmuch/test
> ╰─ (git)-[master]-% ./T610-message-property.sh --val
> ./test-lib.sh:[:18: unknown condition: -lt
> ./test-lib.sh:27: command not found: shopt
>
> test-lib: Testing message property API
> cc1: error: unrecognised debug output level ' -O0'
>
> All the tests fail because none of the binaries get built.
>
> Same results if I run from bash instead of my normal zsh.
>
>> Also /T600-named-queries.sh --val printed noisy output -- but perhaps
>> the noise difference (less there) can inform something.
>
> I ran
> % ./T610-message-property --debug
> % cd tmp.T610-message-property
> % LD_LIBRARY_PATH=../../lib valgrind ./test6 `pwd`/mail
>
Ok, I run the same before and after patching:
40 14:32 0:00 ./T610-message-property --debug
41 14:32 0:02 ./T610-message-property.sh --debug
42 14:32 0:00 cd tmp.T610-message-property/
43 14:32 0:00 ls
44 14:33 0:00 LD_LIBRARY_PATH=../../lib valgrind ./test6 $PWD/mail
45 14:33 0:25 sudo dnf install valgrind
46 14:33 0:02 LD_LIBRARY_PATH=../../lib valgrind ./test6 $PWD/mail
49 14:34 0:00 cd ..
50 14:34 0:00 cd ..
54 14:35 0:00 sc gu:tmp-m* .
57 14:35 0:00 git am tmp-mbox
58 14:35 0:07 ./configure
59 14:35 0:28 make
60 14:36 0:00 cd test/
61 14:37 0:02 ./T610-message-property.sh --debug
62 14:37 0:00 cd tmp.T610-message-property/
63 14:37 0:01 LD_LIBRARY_PATH=../../lib valgrind ./test6 $PWD/mail
and can verify that valgring output was "dirty" before and clean after.
Tomi
>>
>> Anyway, this seems to fix a bug that might affect someone, and at least
>> the situation is better with this so this should be pushed soon...
>>
>> Btw: does 'Debian stable' refer to Debian 8.6(+) -- just for the record
>> anyone reading this message years from now :D
>
> Yes, thanks for reminding me to upgrade ;). I tested and the failure is
> there with 8.6
>
> d
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] lib: add talloc reference from string map iterator to map
2016-09-23 9:33 [PATCH] lib: add talloc reference from string map iterator to map David Bremner
2016-09-24 9:54 ` Tomi Ollila
@ 2016-09-24 13:18 ` David Bremner
1 sibling, 0 replies; 5+ messages in thread
From: David Bremner @ 2016-09-24 13:18 UTC (permalink / raw)
To: notmuch
David Bremner <david@tethera.net> writes:
> This is needed so that when the map is modified during traversal, and
> thus unlinked by the database code, the map is not disposed of until the
> iterator is done with it.
> ---
pushed,
d
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2016-09-24 13:18 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-09-23 9:33 [PATCH] lib: add talloc reference from string map iterator to map David Bremner
2016-09-24 9:54 ` Tomi Ollila
2016-09-24 11:16 ` David Bremner
2016-09-24 11:43 ` Tomi Ollila
2016-09-24 13:18 ` David Bremner
Code repositories for project(s) associated with this public inbox
https://yhetil.org/notmuch.git/
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).