Hallo!
On Fri, 21 Jan 2011 10:59:36 +0100, Michal Sojka <sojkam1 at fel.cvut.cz> 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:
<http://notmuchmail.org/pipermail/notmuch/attachments/20110126/5e44cba0/attachment.pgp>
