[PATCH 2/3] new: Add all initial tags at once

[Carl Worth]

On Thu, 27 Jan 2011 15:03:49 +1000, Carl Worth  wrote:
> Just a few hours ago I attended an interesting talk by Rusty Russell in
> which he talks about a CCAN module he has written called failtest which
> provides an implementation of this kind of testing.

I meant to include some links with the above. CCAN is here:

http://ccan.ozlabs.org/

And the failtest module is here:

http://ccan.ozlabs.org/info/failtest.html

I talked to Rusty about adding copyright attribution and a one-line
pointer to the LICENSE file to this. Once that's in place, we could
incorporate failtest.c into notmuch if it would be useful.

-Carl

-- 
carl.d.worth at intel.com
-- next part --
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
URL: 

[PATCH 2/3] new: Add all initial tags at once

[Carl Worth]

On Wed, 26 Jan 2011 17:52:53 +0100, Thomas Schwinge  
wrote:
> I do support the patch's idea (which was recently committed; and what
> follows in this message is not at all directed towards Michal, who wrote
> this patch) -- but what about return values checking?  This is one aspect
> of the notmuch C code (which I generally consider to be nice to read and
> of high quality, as I said before already), that I consider totally
> lacking -- there are literally hundreds of C functions calls where the
> return values are just discarded.  This is bad.  For example (simulating
> a full disk):
> 
> $ notmuch dump > /dev/full
> $ echo $?
> 0

All very well pointed out. This is clearly something we need to fix.

> Other languages have the concept of exceptions; C doesn't, so we're
> supposed to put some ``ABORT_IF_NOT_NOTMUCH_STATUS_SUCCESS(ret)''
> statements after each and every non-void (etc.) C function call.  Or make
> the functions abort themselves (which is not a too good idea, as we
> surely agree).  Or use a different programming language -- now, at the
> present state, it wouldn't be too painful to switch, in my opinion.  (I
> won't suggest any specific language, though.)

I wouldn't have any problem with anyone re-implementing notmuch in some
other language than C. But that's not something I would be likely to
work on myself, I don't think. 

>If staying with C (which I
> don't object, either), then this needs a whole code audit, and a lot of
> discipline in the future.

Even a code audit and developer discipline won't be enough here. We'll
still miss things.

What we need is exhaustive testing. A great approach is to take calls
like malloc, open, read, write, etc. and at each site, fork() and fail
the call along one path, (which should then exit with a failure), and
then let the other path continue.

Just a few hours ago I attended an interesting talk by Rusty Russell in
which he talks about a CCAN module he has written called failtest which
provides an implementation of this kind of testing.

I'd love to see something like that integrated with notmuch.

> Comments?  (And I hope this doesn't sound too harsh :-) -- but it is a
> serious programming issue.)

Please don't apologize! It would be a shame if people didn't share
problems they notice in the code. Being able to hear those kinds of
things is one of the great benefits I get from publishing this code as
free software.

So, please, keep the suggestions coming!

-Carl

-- 
carl.d.worth at intel.com
-- next part --
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
URL: 

[PATCH 2/3] new: Add all initial tags at once

[Michal Sojka]

On Thu, 27 Jan 2011, Carl Worth wrote:
> On Thu, 27 Jan 2011 15:03:49 +1000, Carl Worth  wrote:
> > Just a few hours ago I attended an interesting talk by Rusty Russell in
> > which he talks about a CCAN module he has written called failtest which
> > provides an implementation of this kind of testing.
> 
> I meant to include some links with the above. CCAN is here:
> 
>   http://ccan.ozlabs.org/
> 
> And the failtest module is here:
> 
>   http://ccan.ozlabs.org/info/failtest.html

Yes, this looks interesting. We may also want to use GCC function
attribute "__attribute__ ((warn_unused_result))" to get warnings when
the returned value is not checked.

-Michal

Re: [PATCH 2/3] new: Add all initial tags at once

[Michal Sojka]

On Thu, 27 Jan 2011, Carl Worth wrote:
 On Thu, 27 Jan 2011 15:03:49 +1000, Carl Worth cwo...@cworth.org wrote:
  Just a few hours ago I attended an interesting talk by Rusty Russell in
  which he talks about a CCAN module he has written called failtest which
  provides an implementation of this kind of testing.
 
 I meant to include some links with the above. CCAN is here:
 
   http://ccan.ozlabs.org/
 
 And the failtest module is here:
 
   http://ccan.ozlabs.org/info/failtest.html

Yes, this looks interesting. We may also want to use GCC function
attribute __attribute__ ((warn_unused_result)) to get warnings when
the returned value is not checked.

-Michal
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

[PATCH 2/3] new: Add all initial tags at once

[Thomas Schwinge]

Hallo!

On Fri, 21 Jan 2011 10:59:36 +0100, Michal Sojka  wrote:
> --- a/notmuch-new.c
> +++ b/notmuch-new.c
> @@ -418,6 +418,7 @@ add_files_recursive (notmuch_database_t *notmuch,
>   /* success */
>   case NOTMUCH_STATUS_SUCCESS:
>   state->added_messages++;
> + notmuch_message_freeze (message);
>   for (tag=state->new_tags; *tag != NULL; tag++)
>   notmuch_message_add_tag (message, *tag);
>   if (state->synchronize_flags == TRUE) {
> @@ -433,6 +434,7 @@ add_files_recursive (notmuch_database_t *notmuch,
>   notmuch_message_maildir_flags_to_tags (message);
>   }
>   }
> + notmuch_message_thaw (message);
>   break;
>   /* Non-fatal issues (go on to next file) */
>   case NOTMUCH_STATUS_DUPLICATE_MESSAGE_ID:

I do support the patch's idea (which was recently committed; and what
follows in this message is not at all directed towards Michal, who wrote
this patch) -- but what about return values checking?  This is one aspect
of the notmuch C code (which I generally consider to be nice to read and
of high quality, as I said before already), that I consider totally
lacking -- there are literally hundreds of C functions calls where the
return values are just discarded.  This is bad.  For example (simulating
a full disk):

$ notmuch dump > /dev/full
$ echo $?
0

The command returned ``success'' -- but no data has been saved.  This
could, in some extreme (but still likely) case mean: total tag data loss.
(This is likely due to printf / close or fprintf / fclose (yes, really,
(f)close! -- think of buffering) not getting their return values
checked.)  Here is what is expected:

$ echo foo > /dev/full
bash: echo: write error: No space left on device
$ echo $?
1

Then, in the few (!) lines quoted above, there are (exactly / only) calls
to notmuch_message_freeze, notmuch_message_add_tag,
notmuch_message_maildir_flags_to_tags, notmuch_message_thaw -- each of
which can fail, will return this via the notmuch_status_t return value
(whose possible values are documented, too), but none has their return
value checked.

Other languages have the concept of exceptions; C doesn't, so we're
supposed to put some ``ABORT_IF_NOT_NOTMUCH_STATUS_SUCCESS(ret)''
statements after each and every non-void (etc.) C function call.  Or make
the functions abort themselves (which is not a too good idea, as we
surely agree).  Or use a different programming language -- now, at the
present state, it wouldn't be too painful to switch, in my opinion.  (I
won't suggest any specific language, though.)  If staying with C (which I
don't object, either), then this needs a whole code audit, and a lot of
discipline in the future.

Comments?  (And I hope this doesn't sound too harsh :-) -- but it is a
serious programming issue.)


Gr??e,
 Thomas
-- next part --
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: not available
URL: 

Re: [PATCH 2/3] new: Add all initial tags at once

[Carl Worth]

On Thu, 27 Jan 2011 15:03:49 +1000, Carl Worth cwo...@cworth.org wrote:
 Just a few hours ago I attended an interesting talk by Rusty Russell in
 which he talks about a CCAN module he has written called failtest which
 provides an implementation of this kind of testing.

I meant to include some links with the above. CCAN is here:

http://ccan.ozlabs.org/

And the failtest module is here:

http://ccan.ozlabs.org/info/failtest.html

I talked to Rusty about adding copyright attribution and a one-line
pointer to the LICENSE file to this. Once that's in place, we could
incorporate failtest.c into notmuch if it would be useful.

-Carl

-- 
carl.d.wo...@intel.com


pgpXALChob42r.pgp
Description: PGP signature
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

[PATCH 2/3] new: Add all initial tags at once

[Michal Sojka]

If there are several tags applied to the new messages, it is beneficial
to store them to the database at one, because it saves some time,
especially when the notmuch new is run for the first time.

This patch decreased the time for initial import from 1h 35m to 1h 14m.
---
 notmuch-new.c |2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/notmuch-new.c b/notmuch-new.c
index a2af045..d71e497 100644
--- a/notmuch-new.c
+++ b/notmuch-new.c
@@ -418,6 +418,7 @@ add_files_recursive (notmuch_database_t *notmuch,
/* success */
case NOTMUCH_STATUS_SUCCESS:
state->added_messages++;
+   notmuch_message_freeze (message);
for (tag=state->new_tags; *tag != NULL; tag++)
notmuch_message_add_tag (message, *tag);
if (state->synchronize_flags == TRUE) {
@@ -433,6 +434,7 @@ add_files_recursive (notmuch_database_t *notmuch,
notmuch_message_maildir_flags_to_tags (message);
}
}
+   notmuch_message_thaw (message);
break;
/* Non-fatal issues (go on to next file) */
case NOTMUCH_STATUS_DUPLICATE_MESSAGE_ID:
-- 
1.7.2.3

[PATCH 2/3] new: Add all initial tags at once

[Michal Sojka]

If there are several tags applied to the new messages, it is beneficial
to store them to the database at one, because it saves some time,
especially when the notmuch new is run for the first time.

This patch decreased the time for initial import from 1h 35m to 1h 14m.
---
 notmuch-new.c |2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/notmuch-new.c b/notmuch-new.c
index a2af045..d71e497 100644
--- a/notmuch-new.c
+++ b/notmuch-new.c
@@ -418,6 +418,7 @@ add_files_recursive (notmuch_database_t *notmuch,
/* success */
case NOTMUCH_STATUS_SUCCESS:
state-added_messages++;
+   notmuch_message_freeze (message);
for (tag=state-new_tags; *tag != NULL; tag++)
notmuch_message_add_tag (message, *tag);
if (state-synchronize_flags == TRUE) {
@@ -433,6 +434,7 @@ add_files_recursive (notmuch_database_t *notmuch,
notmuch_message_maildir_flags_to_tags (message);
}
}
+   notmuch_message_thaw (message);
break;
/* Non-fatal issues (go on to next file) */
case NOTMUCH_STATUS_DUPLICATE_MESSAGE_ID:
-- 
1.7.2.3

___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch