* [PATCH 0/3] Fix some error handling in notmuch new
@ 2012-02-27 15:49 Austin Clements
2012-02-27 15:49 ` [PATCH 1/3] new: Consistently treat fatal errors as fatal Austin Clements
` (4 more replies)
0 siblings, 5 replies; 22+ messages in thread
From: Austin Clements @ 2012-02-27 15:49 UTC (permalink / raw)
To: notmuch
This series cleans up some bad error handling in notmuch new. In many
cases, fatal errors were being ignored, sometimes blatantly and
sometimes because the caller did not treat them as fatal even if the
callee did.
^ permalink raw reply [flat|nested] 22+ messages in thread* [PATCH 1/3] new: Consistently treat fatal errors as fatal 2012-02-27 15:49 [PATCH 0/3] Fix some error handling in notmuch new Austin Clements @ 2012-02-27 15:49 ` Austin Clements 2012-04-16 15:53 ` Mark Walters 2012-02-27 15:49 ` [PATCH 2/3] new: Handle fatal errors in remove_filename and _remove_directory Austin Clements ` (3 subsequent siblings) 4 siblings, 1 reply; 22+ messages in thread From: Austin Clements @ 2012-02-27 15:49 UTC (permalink / raw) To: notmuch Previously, fatal errors in add_files_recursive were not treated as fatal by its callers (including itself!) and add_files_recursive sometimes returned errors on non-fatal conditions. This makes add_files_recursive errors consistently fatal and updates all callers to treat them as fatal. --- notmuch-new.c | 13 ++++++++----- 1 files changed, 8 insertions(+), 5 deletions(-) diff --git a/notmuch-new.c b/notmuch-new.c index 4f13535..bd9786a 100644 --- a/notmuch-new.c +++ b/notmuch-new.c @@ -308,7 +308,6 @@ add_files_recursive (notmuch_database_t *notmuch, if (num_fs_entries == -1) { fprintf (stderr, "Error opening directory %s: %s\n", path, strerror (errno)); - ret = NOTMUCH_STATUS_FILE_ERROR; goto DONE; } @@ -351,8 +350,10 @@ add_files_recursive (notmuch_database_t *notmuch, next = talloc_asprintf (notmuch, "%s/%s", path, entry->d_name); status = add_files_recursive (notmuch, next, state); - if (status && ret == NOTMUCH_STATUS_SUCCESS) + if (status) { ret = status; + goto DONE; + } talloc_free (next); next = NULL; } @@ -933,6 +934,8 @@ notmuch_new_command (void *ctx, int argc, char *argv[]) } ret = add_files (notmuch, db_path, &add_files_state); + if (ret) + goto DONE; gettimeofday (&tv_start, NULL); for (f = add_files_state.removed_files->head; f && !interrupted; f = f->next) { @@ -965,6 +968,7 @@ notmuch_new_command (void *ctx, int argc, char *argv[]) } } + DONE: talloc_free (add_files_state.removed_files); talloc_free (add_files_state.removed_directories); talloc_free (add_files_state.directory_mtimes); @@ -1012,10 +1016,9 @@ notmuch_new_command (void *ctx, int argc, char *argv[]) printf ("\n"); - if (ret) { - printf ("\nNote: At least one error was encountered: %s\n", + if (ret) + printf ("\nNote: A fatal error was encountered: %s\n", notmuch_status_to_string (ret)); - } notmuch_database_close (notmuch); -- 1.7.7.3 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH 1/3] new: Consistently treat fatal errors as fatal 2012-02-27 15:49 ` [PATCH 1/3] new: Consistently treat fatal errors as fatal Austin Clements @ 2012-04-16 15:53 ` Mark Walters 2012-04-22 4:11 ` Austin Clements 0 siblings, 1 reply; 22+ messages in thread From: Mark Walters @ 2012-04-16 15:53 UTC (permalink / raw) To: Austin Clements, notmuch On Mon, 27 Feb 2012, Austin Clements <amdragon@MIT.EDU> wrote: > Previously, fatal errors in add_files_recursive were not treated as > fatal by its callers (including itself!) and add_files_recursive > sometimes returned errors on non-fatal conditions. This makes > add_files_recursive errors consistently fatal and updates all callers > to treat them as fatal. Hi I have attempted to review this but am feeling a little out of my depth. This patch seems fine except for one thing I am unsure about: > --- > notmuch-new.c | 13 ++++++++----- > 1 files changed, 8 insertions(+), 5 deletions(-) > > diff --git a/notmuch-new.c b/notmuch-new.c > index 4f13535..bd9786a 100644 > --- a/notmuch-new.c > +++ b/notmuch-new.c > @@ -308,7 +308,6 @@ add_files_recursive (notmuch_database_t *notmuch, > if (num_fs_entries == -1) { > fprintf (stderr, "Error opening directory %s: %s\n", > path, strerror (errno)); > - ret = NOTMUCH_STATUS_FILE_ERROR; > goto DONE; > } > If I understand this, then this change makes a failure to open a directory non-fatal. In the comment before the function it says * Also, we don't immediately act on file/directory removal since we * must ensure that in the case of a rename that the new filename is * added before the old filename is removed, (so that no information * is lost from the database). I am worried that we could fail to find some files because of the file error above, and then we delete them from the database. Maybe this could only happen if those emails have just been moved to this file-error directory? Best wishes Mark > @@ -351,8 +350,10 @@ add_files_recursive (notmuch_database_t *notmuch, > > next = talloc_asprintf (notmuch, "%s/%s", path, entry->d_name); > status = add_files_recursive (notmuch, next, state); > - if (status && ret == NOTMUCH_STATUS_SUCCESS) > + if (status) { > ret = status; > + goto DONE; > + } > talloc_free (next); > next = NULL; > } > @@ -933,6 +934,8 @@ notmuch_new_command (void *ctx, int argc, char *argv[]) > } > > ret = add_files (notmuch, db_path, &add_files_state); > + if (ret) > + goto DONE; > > gettimeofday (&tv_start, NULL); > for (f = add_files_state.removed_files->head; f && !interrupted; f = f->next) { > @@ -965,6 +968,7 @@ notmuch_new_command (void *ctx, int argc, char *argv[]) > } > } > > + DONE: > talloc_free (add_files_state.removed_files); > talloc_free (add_files_state.removed_directories); > talloc_free (add_files_state.directory_mtimes); > @@ -1012,10 +1016,9 @@ notmuch_new_command (void *ctx, int argc, char *argv[]) > > printf ("\n"); > > - if (ret) { > - printf ("\nNote: At least one error was encountered: %s\n", > + if (ret) > + printf ("\nNote: A fatal error was encountered: %s\n", > notmuch_status_to_string (ret)); > - } > > notmuch_database_close (notmuch); > > -- > 1.7.7.3 > > _______________________________________________ > notmuch mailing list > notmuch@notmuchmail.org > http://notmuchmail.org/mailman/listinfo/notmuch ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1/3] new: Consistently treat fatal errors as fatal 2012-04-16 15:53 ` Mark Walters @ 2012-04-22 4:11 ` Austin Clements 0 siblings, 0 replies; 22+ messages in thread From: Austin Clements @ 2012-04-22 4:11 UTC (permalink / raw) To: Mark Walters; +Cc: notmuch Quoth Mark Walters on Apr 16 at 4:53 pm: > > On Mon, 27 Feb 2012, Austin Clements <amdragon@MIT.EDU> wrote: > > Previously, fatal errors in add_files_recursive were not treated as > > fatal by its callers (including itself!) and add_files_recursive > > sometimes returned errors on non-fatal conditions. This makes > > add_files_recursive errors consistently fatal and updates all callers > > to treat them as fatal. > > Hi I have attempted to review this but am feeling a little out of my > depth. This patch seems fine except for one thing I am unsure about: > > > --- > > notmuch-new.c | 13 ++++++++----- > > 1 files changed, 8 insertions(+), 5 deletions(-) > > > > diff --git a/notmuch-new.c b/notmuch-new.c > > index 4f13535..bd9786a 100644 > > --- a/notmuch-new.c > > +++ b/notmuch-new.c > > @@ -308,7 +308,6 @@ add_files_recursive (notmuch_database_t *notmuch, > > if (num_fs_entries == -1) { > > fprintf (stderr, "Error opening directory %s: %s\n", > > path, strerror (errno)); > > - ret = NOTMUCH_STATUS_FILE_ERROR; > > goto DONE; > > } > > > > If I understand this, then this change makes a failure to open a > directory non-fatal. In the comment before the function it says > > * Also, we don't immediately act on file/directory removal since we > * must ensure that in the case of a rename that the new filename is > * added before the old filename is removed, (so that no information > * is lost from the database). > > I am worried that we could fail to find some files because of the > file error above, and then we delete them from the database. Maybe this > could only happen if those emails have just been moved to this > file-error directory? Hmm. This won't *actively* remove files, since that only happens if we successfully scan a directory and find a message that's in the database but not in that directory (*not* scanning the directory won't add anything to the remove list). However, you are right that if a message is moved from some other directory into a directory that we fail to open, that message will be deleted "by omission". I've added the error back, along with a comment explaining it. > Best wishes > > Mark > > > @@ -351,8 +350,10 @@ add_files_recursive (notmuch_database_t *notmuch, > > > > next = talloc_asprintf (notmuch, "%s/%s", path, entry->d_name); > > status = add_files_recursive (notmuch, next, state); > > - if (status && ret == NOTMUCH_STATUS_SUCCESS) > > + if (status) { > > ret = status; > > + goto DONE; > > + } > > talloc_free (next); > > next = NULL; > > } > > @@ -933,6 +934,8 @@ notmuch_new_command (void *ctx, int argc, char *argv[]) > > } > > > > ret = add_files (notmuch, db_path, &add_files_state); > > + if (ret) > > + goto DONE; > > > > gettimeofday (&tv_start, NULL); > > for (f = add_files_state.removed_files->head; f && !interrupted; f = f->next) { > > @@ -965,6 +968,7 @@ notmuch_new_command (void *ctx, int argc, char *argv[]) > > } > > } > > > > + DONE: > > talloc_free (add_files_state.removed_files); > > talloc_free (add_files_state.removed_directories); > > talloc_free (add_files_state.directory_mtimes); > > @@ -1012,10 +1016,9 @@ notmuch_new_command (void *ctx, int argc, char *argv[]) > > > > printf ("\n"); > > > > - if (ret) { > > - printf ("\nNote: At least one error was encountered: %s\n", > > + if (ret) > > + printf ("\nNote: A fatal error was encountered: %s\n", > > notmuch_status_to_string (ret)); > > - } > > > > notmuch_database_close (notmuch); ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 2/3] new: Handle fatal errors in remove_filename and _remove_directory 2012-02-27 15:49 [PATCH 0/3] Fix some error handling in notmuch new Austin Clements 2012-02-27 15:49 ` [PATCH 1/3] new: Consistently treat fatal errors as fatal Austin Clements @ 2012-02-27 15:49 ` Austin Clements 2012-04-16 16:02 ` Mark Walters 2012-02-27 15:49 ` [PATCH 3/3] new: Print final fatal error message to stderr Austin Clements ` (2 subsequent siblings) 4 siblings, 1 reply; 22+ messages in thread From: Austin Clements @ 2012-02-27 15:49 UTC (permalink / raw) To: notmuch Previously such errors were simply ignored. Now they cause an immediate cleanup and abort. --- notmuch-new.c | 24 ++++++++++++++++++------ 1 files changed, 18 insertions(+), 6 deletions(-) diff --git a/notmuch-new.c b/notmuch-new.c index bd9786a..0cbd479 100644 --- a/notmuch-new.c +++ b/notmuch-new.c @@ -780,8 +780,10 @@ remove_filename (notmuch_database_t *notmuch, add_files_state->renamed_messages++; if (add_files_state->synchronize_flags == TRUE) notmuch_message_maildir_flags_to_tags (message); - } else + status = NOTMUCH_STATUS_SUCCESS; + } else if (status == NOTMUCH_STATUS_SUCCESS) { add_files_state->removed_messages++; + } notmuch_message_destroy (message); notmuch_database_end_atomic (notmuch); return status; @@ -789,12 +791,13 @@ remove_filename (notmuch_database_t *notmuch, /* Recursively remove all filenames from the database referring to * 'path' (or to any of its children). */ -static void +static notmuch_status_t _remove_directory (void *ctx, notmuch_database_t *notmuch, const char *path, add_files_state_t *add_files_state) { + notmuch_status_t status; notmuch_directory_t *directory; notmuch_filenames_t *files, *subdirs; char *absolute; @@ -807,8 +810,10 @@ _remove_directory (void *ctx, { absolute = talloc_asprintf (ctx, "%s/%s", path, notmuch_filenames_get (files)); - remove_filename (notmuch, absolute, add_files_state); + status = remove_filename (notmuch, absolute, add_files_state); talloc_free (absolute); + if (status) + return status; } for (subdirs = notmuch_directory_get_child_directories (directory); @@ -817,11 +822,14 @@ _remove_directory (void *ctx, { absolute = talloc_asprintf (ctx, "%s/%s", path, notmuch_filenames_get (subdirs)); - _remove_directory (ctx, notmuch, absolute, add_files_state); + status = _remove_directory (ctx, notmuch, absolute, add_files_state); talloc_free (absolute); + if (status) + return status; } notmuch_directory_destroy (directory); + return NOTMUCH_STATUS_SUCCESS; } int @@ -939,7 +947,9 @@ notmuch_new_command (void *ctx, int argc, char *argv[]) gettimeofday (&tv_start, NULL); for (f = add_files_state.removed_files->head; f && !interrupted; f = f->next) { - remove_filename (notmuch, f->filename, &add_files_state); + ret = remove_filename (notmuch, f->filename, &add_files_state); + if (ret) + goto DONE; if (do_print_progress) { do_print_progress = 0; generic_print_progress ("Cleaned up", "messages", @@ -950,7 +960,9 @@ notmuch_new_command (void *ctx, int argc, char *argv[]) gettimeofday (&tv_start, NULL); for (f = add_files_state.removed_directories->head, i = 0; f && !interrupted; f = f->next, i++) { - _remove_directory (ctx, notmuch, f->filename, &add_files_state); + ret = _remove_directory (ctx, notmuch, f->filename, &add_files_state); + if (ret) + goto DONE; if (do_print_progress) { do_print_progress = 0; generic_print_progress ("Cleaned up", "directories", -- 1.7.7.3 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH 2/3] new: Handle fatal errors in remove_filename and _remove_directory 2012-02-27 15:49 ` [PATCH 2/3] new: Handle fatal errors in remove_filename and _remove_directory Austin Clements @ 2012-04-16 16:02 ` Mark Walters 2012-04-22 4:21 ` Austin Clements 0 siblings, 1 reply; 22+ messages in thread From: Mark Walters @ 2012-04-16 16:02 UTC (permalink / raw) To: Austin Clements, notmuch On Mon, 27 Feb 2012, Austin Clements <amdragon@MIT.EDU> wrote: > Previously such errors were simply ignored. Now they cause an > immediate cleanup and abort. This one looks fine except for a minor query. > --- > notmuch-new.c | 24 ++++++++++++++++++------ > 1 files changed, 18 insertions(+), 6 deletions(-) > > diff --git a/notmuch-new.c b/notmuch-new.c > index bd9786a..0cbd479 100644 > --- a/notmuch-new.c > +++ b/notmuch-new.c > @@ -780,8 +780,10 @@ remove_filename (notmuch_database_t *notmuch, > add_files_state->renamed_messages++; > if (add_files_state->synchronize_flags == TRUE) > notmuch_message_maildir_flags_to_tags (message); > - } else > + status = NOTMUCH_STATUS_SUCCESS; > + } else if (status == NOTMUCH_STATUS_SUCCESS) { > add_files_state->removed_messages++; > + } > notmuch_message_destroy (message); > notmuch_database_end_atomic (notmuch); > return status; > @@ -789,12 +791,13 @@ remove_filename (notmuch_database_t *notmuch, > > /* Recursively remove all filenames from the database referring to > * 'path' (or to any of its children). */ > -static void > +static notmuch_status_t > _remove_directory (void *ctx, > notmuch_database_t *notmuch, > const char *path, > add_files_state_t *add_files_state) > { > + notmuch_status_t status; > notmuch_directory_t *directory; > notmuch_filenames_t *files, *subdirs; > char *absolute; > @@ -807,8 +810,10 @@ _remove_directory (void *ctx, > { > absolute = talloc_asprintf (ctx, "%s/%s", path, > notmuch_filenames_get (files)); > - remove_filename (notmuch, absolute, add_files_state); > + status = remove_filename (notmuch, absolute, add_files_state); > talloc_free (absolute); > + if (status) > + return status; > } > > for (subdirs = notmuch_directory_get_child_directories (directory); > @@ -817,11 +822,14 @@ _remove_directory (void *ctx, > { > absolute = talloc_asprintf (ctx, "%s/%s", path, > notmuch_filenames_get (subdirs)); > - _remove_directory (ctx, notmuch, absolute, add_files_state); > + status = _remove_directory (ctx, notmuch, absolute, add_files_state); > talloc_free (absolute); > + if (status) > + return status; > } > > notmuch_directory_destroy (directory); > + return NOTMUCH_STATUS_SUCCESS; > } In the two "return status" lines above seem to mean we don't call notmuch_directory_destroy. Does that matter? The other query is not actually about this patch: just something that came up when reading it. Should notmuch_database_begin_atomic and notmuch_database_end_atomic always be paired? One of the (existing) error cases in remove_filename seems to return without calling end. Best wishes Mark > int > @@ -939,7 +947,9 @@ notmuch_new_command (void *ctx, int argc, char *argv[]) > > gettimeofday (&tv_start, NULL); > for (f = add_files_state.removed_files->head; f && !interrupted; f = f->next) { > - remove_filename (notmuch, f->filename, &add_files_state); > + ret = remove_filename (notmuch, f->filename, &add_files_state); > + if (ret) > + goto DONE; > if (do_print_progress) { > do_print_progress = 0; > generic_print_progress ("Cleaned up", "messages", > @@ -950,7 +960,9 @@ notmuch_new_command (void *ctx, int argc, char *argv[]) > > gettimeofday (&tv_start, NULL); > for (f = add_files_state.removed_directories->head, i = 0; f && !interrupted; f = f->next, i++) { > - _remove_directory (ctx, notmuch, f->filename, &add_files_state); > + ret = _remove_directory (ctx, notmuch, f->filename, &add_files_state); > + if (ret) > + goto DONE; > if (do_print_progress) { > do_print_progress = 0; > generic_print_progress ("Cleaned up", "directories", > -- > 1.7.7.3 > > _______________________________________________ > notmuch mailing list > notmuch@notmuchmail.org > http://notmuchmail.org/mailman/listinfo/notmuch ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/3] new: Handle fatal errors in remove_filename and _remove_directory 2012-04-16 16:02 ` Mark Walters @ 2012-04-22 4:21 ` Austin Clements 0 siblings, 0 replies; 22+ messages in thread From: Austin Clements @ 2012-04-22 4:21 UTC (permalink / raw) To: Mark Walters; +Cc: notmuch Quoth Mark Walters on Apr 16 at 5:02 pm: > On Mon, 27 Feb 2012, Austin Clements <amdragon@MIT.EDU> wrote: > > Previously such errors were simply ignored. Now they cause an > > immediate cleanup and abort. > > This one looks fine except for a minor query. > > > --- > > notmuch-new.c | 24 ++++++++++++++++++------ > > 1 files changed, 18 insertions(+), 6 deletions(-) > > > > diff --git a/notmuch-new.c b/notmuch-new.c > > index bd9786a..0cbd479 100644 > > --- a/notmuch-new.c > > +++ b/notmuch-new.c > > @@ -780,8 +780,10 @@ remove_filename (notmuch_database_t *notmuch, > > add_files_state->renamed_messages++; > > if (add_files_state->synchronize_flags == TRUE) > > notmuch_message_maildir_flags_to_tags (message); > > - } else > > + status = NOTMUCH_STATUS_SUCCESS; > > + } else if (status == NOTMUCH_STATUS_SUCCESS) { > > add_files_state->removed_messages++; > > + } > > notmuch_message_destroy (message); > > notmuch_database_end_atomic (notmuch); > > return status; > > @@ -789,12 +791,13 @@ remove_filename (notmuch_database_t *notmuch, > > > > /* Recursively remove all filenames from the database referring to > > * 'path' (or to any of its children). */ > > -static void > > +static notmuch_status_t > > _remove_directory (void *ctx, > > notmuch_database_t *notmuch, > > const char *path, > > add_files_state_t *add_files_state) > > { > > + notmuch_status_t status; > > notmuch_directory_t *directory; > > notmuch_filenames_t *files, *subdirs; > > char *absolute; > > @@ -807,8 +810,10 @@ _remove_directory (void *ctx, > > { > > absolute = talloc_asprintf (ctx, "%s/%s", path, > > notmuch_filenames_get (files)); > > - remove_filename (notmuch, absolute, add_files_state); > > + status = remove_filename (notmuch, absolute, add_files_state); > > talloc_free (absolute); > > + if (status) > > + return status; > > } > > > > for (subdirs = notmuch_directory_get_child_directories (directory); > > @@ -817,11 +822,14 @@ _remove_directory (void *ctx, > > { > > absolute = talloc_asprintf (ctx, "%s/%s", path, > > notmuch_filenames_get (subdirs)); > > - _remove_directory (ctx, notmuch, absolute, add_files_state); > > + status = _remove_directory (ctx, notmuch, absolute, add_files_state); > > talloc_free (absolute); > > + if (status) > > + return status; > > } > > > > notmuch_directory_destroy (directory); > > + return NOTMUCH_STATUS_SUCCESS; > > } > > In the two "return status" lines above seem to mean we don't call > notmuch_directory_destroy. Does that matter? Good point. I've fixed this to use the usual goto DONE cleanup idiom. > The other query is not actually about this patch: just something that > came up when reading it. Should notmuch_database_begin_atomic and > notmuch_database_end_atomic always be paired? One of the (existing) > error cases in remove_filename seems to return without calling end. Yes, they should be. I've added a patch to fix that. > Best wishes > > Mark > > > int > > @@ -939,7 +947,9 @@ notmuch_new_command (void *ctx, int argc, char *argv[]) > > > > gettimeofday (&tv_start, NULL); > > for (f = add_files_state.removed_files->head; f && !interrupted; f = f->next) { > > - remove_filename (notmuch, f->filename, &add_files_state); > > + ret = remove_filename (notmuch, f->filename, &add_files_state); > > + if (ret) > > + goto DONE; > > if (do_print_progress) { > > do_print_progress = 0; > > generic_print_progress ("Cleaned up", "messages", > > @@ -950,7 +960,9 @@ notmuch_new_command (void *ctx, int argc, char *argv[]) > > > > gettimeofday (&tv_start, NULL); > > for (f = add_files_state.removed_directories->head, i = 0; f && !interrupted; f = f->next, i++) { > > - _remove_directory (ctx, notmuch, f->filename, &add_files_state); > > + ret = _remove_directory (ctx, notmuch, f->filename, &add_files_state); > > + if (ret) > > + goto DONE; > > if (do_print_progress) { > > do_print_progress = 0; > > generic_print_progress ("Cleaned up", "directories", ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 3/3] new: Print final fatal error message to stderr 2012-02-27 15:49 [PATCH 0/3] Fix some error handling in notmuch new Austin Clements 2012-02-27 15:49 ` [PATCH 1/3] new: Consistently treat fatal errors as fatal Austin Clements 2012-02-27 15:49 ` [PATCH 2/3] new: Handle fatal errors in remove_filename and _remove_directory Austin Clements @ 2012-02-27 15:49 ` Austin Clements 2012-04-22 4:27 ` [PATCH v2 0/4] Fix some error handling in notmuch new Austin Clements 2012-04-22 15:50 ` [PATCH v3 0/4] Fix some error handling in notmuch new Austin Clements 4 siblings, 0 replies; 22+ messages in thread From: Austin Clements @ 2012-02-27 15:49 UTC (permalink / raw) To: notmuch This was going to stdout. I removed the newline at the beginning of printing the fatal error message because it wouldn't make sense if you were only looking at the stderr stream (e.g., you had redirected stdout to /dev/null). --- notmuch-new.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-) diff --git a/notmuch-new.c b/notmuch-new.c index 0cbd479..9ad790d 100644 --- a/notmuch-new.c +++ b/notmuch-new.c @@ -1029,8 +1029,8 @@ notmuch_new_command (void *ctx, int argc, char *argv[]) printf ("\n"); if (ret) - printf ("\nNote: A fatal error was encountered: %s\n", - notmuch_status_to_string (ret)); + fprintf (stderr, "Note: A fatal error was encountered: %s\n", + notmuch_status_to_string (ret)); notmuch_database_close (notmuch); -- 1.7.7.3 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH v2 0/4] Fix some error handling in notmuch new 2012-02-27 15:49 [PATCH 0/3] Fix some error handling in notmuch new Austin Clements ` (2 preceding siblings ...) 2012-02-27 15:49 ` [PATCH 3/3] new: Print final fatal error message to stderr Austin Clements @ 2012-04-22 4:27 ` Austin Clements 2012-04-22 4:27 ` [PATCH v2 1/4] new: Consistently treat fatal errors as fatal Austin Clements ` (3 more replies) 2012-04-22 15:50 ` [PATCH v3 0/4] Fix some error handling in notmuch new Austin Clements 4 siblings, 4 replies; 22+ messages in thread From: Austin Clements @ 2012-04-22 4:27 UTC (permalink / raw) To: notmuch Version 2 should address Mark's comments. It also adds a patch to fix an additional error handling error he pointed out in remove_filename. ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v2 1/4] new: Consistently treat fatal errors as fatal 2012-04-22 4:27 ` [PATCH v2 0/4] Fix some error handling in notmuch new Austin Clements @ 2012-04-22 4:27 ` Austin Clements 2012-04-22 4:27 ` [PATCH v2 2/4] new: Handle fatal errors in remove_filename and _remove_directory Austin Clements ` (2 subsequent siblings) 3 siblings, 0 replies; 22+ messages in thread From: Austin Clements @ 2012-04-22 4:27 UTC (permalink / raw) To: notmuch Previously, fatal errors in add_files_recursive were not treated as fatal by its callers (including itself!). This makes add_files_recursive errors consistently fatal and updates all callers to treat them as fatal. --- notmuch-new.c | 16 ++++++++++++---- 1 files changed, 12 insertions(+), 4 deletions(-) diff --git a/notmuch-new.c b/notmuch-new.c index 4f13535..15c0b36 100644 --- a/notmuch-new.c +++ b/notmuch-new.c @@ -308,6 +308,10 @@ add_files_recursive (notmuch_database_t *notmuch, if (num_fs_entries == -1) { fprintf (stderr, "Error opening directory %s: %s\n", path, strerror (errno)); + /* We consider this a fatal error because, if a user moved a + * message from another directory that we were able to scan + * into this directory, skipping this directory will cause + * that message to be lost. */ ret = NOTMUCH_STATUS_FILE_ERROR; goto DONE; } @@ -351,8 +355,10 @@ add_files_recursive (notmuch_database_t *notmuch, next = talloc_asprintf (notmuch, "%s/%s", path, entry->d_name); status = add_files_recursive (notmuch, next, state); - if (status && ret == NOTMUCH_STATUS_SUCCESS) + if (status) { ret = status; + goto DONE; + } talloc_free (next); next = NULL; } @@ -933,6 +939,8 @@ notmuch_new_command (void *ctx, int argc, char *argv[]) } ret = add_files (notmuch, db_path, &add_files_state); + if (ret) + goto DONE; gettimeofday (&tv_start, NULL); for (f = add_files_state.removed_files->head; f && !interrupted; f = f->next) { @@ -965,6 +973,7 @@ notmuch_new_command (void *ctx, int argc, char *argv[]) } } + DONE: talloc_free (add_files_state.removed_files); talloc_free (add_files_state.removed_directories); talloc_free (add_files_state.directory_mtimes); @@ -1012,10 +1021,9 @@ notmuch_new_command (void *ctx, int argc, char *argv[]) printf ("\n"); - if (ret) { - printf ("\nNote: At least one error was encountered: %s\n", + if (ret) + printf ("\nNote: A fatal error was encountered: %s\n", notmuch_status_to_string (ret)); - } notmuch_database_close (notmuch); -- 1.7.9.1 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH v2 2/4] new: Handle fatal errors in remove_filename and _remove_directory 2012-04-22 4:27 ` [PATCH v2 0/4] Fix some error handling in notmuch new Austin Clements 2012-04-22 4:27 ` [PATCH v2 1/4] new: Consistently treat fatal errors as fatal Austin Clements @ 2012-04-22 4:27 ` Austin Clements 2012-04-22 7:34 ` Mark Walters 2012-04-22 4:27 ` [PATCH v2 3/4] new: Print final fatal error message to stderr Austin Clements 2012-04-22 4:27 ` [PATCH v2 4/4] new: Fix missing end_atomic in remove_filename on error Austin Clements 3 siblings, 1 reply; 22+ messages in thread From: Austin Clements @ 2012-04-22 4:27 UTC (permalink / raw) To: notmuch Previously such errors were simply ignored. Now they cause an immediate cleanup and abort. --- notmuch-new.c | 25 +++++++++++++++++++------ 1 files changed, 19 insertions(+), 6 deletions(-) diff --git a/notmuch-new.c b/notmuch-new.c index 15c0b36..92e0489 100644 --- a/notmuch-new.c +++ b/notmuch-new.c @@ -785,8 +785,10 @@ remove_filename (notmuch_database_t *notmuch, add_files_state->renamed_messages++; if (add_files_state->synchronize_flags == TRUE) notmuch_message_maildir_flags_to_tags (message); - } else + status = NOTMUCH_STATUS_SUCCESS; + } else if (status == NOTMUCH_STATUS_SUCCESS) { add_files_state->removed_messages++; + } notmuch_message_destroy (message); notmuch_database_end_atomic (notmuch); return status; @@ -794,12 +796,13 @@ remove_filename (notmuch_database_t *notmuch, /* Recursively remove all filenames from the database referring to * 'path' (or to any of its children). */ -static void +static notmuch_status_t _remove_directory (void *ctx, notmuch_database_t *notmuch, const char *path, add_files_state_t *add_files_state) { + notmuch_status_t status = NOTMUCH_STATUS_SUCCESS; notmuch_directory_t *directory; notmuch_filenames_t *files, *subdirs; char *absolute; @@ -812,8 +815,10 @@ _remove_directory (void *ctx, { absolute = talloc_asprintf (ctx, "%s/%s", path, notmuch_filenames_get (files)); - remove_filename (notmuch, absolute, add_files_state); + status = remove_filename (notmuch, absolute, add_files_state); talloc_free (absolute); + if (status) + goto DONE; } for (subdirs = notmuch_directory_get_child_directories (directory); @@ -822,11 +827,15 @@ _remove_directory (void *ctx, { absolute = talloc_asprintf (ctx, "%s/%s", path, notmuch_filenames_get (subdirs)); - _remove_directory (ctx, notmuch, absolute, add_files_state); + status = _remove_directory (ctx, notmuch, absolute, add_files_state); talloc_free (absolute); + if (status) + goto DONE; } + DONE: notmuch_directory_destroy (directory); + return NOTMUCH_STATUS_SUCCESS; } int @@ -944,7 +953,9 @@ notmuch_new_command (void *ctx, int argc, char *argv[]) gettimeofday (&tv_start, NULL); for (f = add_files_state.removed_files->head; f && !interrupted; f = f->next) { - remove_filename (notmuch, f->filename, &add_files_state); + ret = remove_filename (notmuch, f->filename, &add_files_state); + if (ret) + goto DONE; if (do_print_progress) { do_print_progress = 0; generic_print_progress ("Cleaned up", "messages", @@ -955,7 +966,9 @@ notmuch_new_command (void *ctx, int argc, char *argv[]) gettimeofday (&tv_start, NULL); for (f = add_files_state.removed_directories->head, i = 0; f && !interrupted; f = f->next, i++) { - _remove_directory (ctx, notmuch, f->filename, &add_files_state); + ret = _remove_directory (ctx, notmuch, f->filename, &add_files_state); + if (ret) + goto DONE; if (do_print_progress) { do_print_progress = 0; generic_print_progress ("Cleaned up", "directories", -- 1.7.9.1 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH v2 2/4] new: Handle fatal errors in remove_filename and _remove_directory 2012-04-22 4:27 ` [PATCH v2 2/4] new: Handle fatal errors in remove_filename and _remove_directory Austin Clements @ 2012-04-22 7:34 ` Mark Walters 2012-04-22 15:36 ` Austin Clements 0 siblings, 1 reply; 22+ messages in thread From: Mark Walters @ 2012-04-22 7:34 UTC (permalink / raw) To: Austin Clements, notmuch On Sun, 22 Apr 2012, Austin Clements <amdragon@MIT.EDU> wrote: > Previously such errors were simply ignored. Now they cause an > immediate cleanup and abort. > --- > notmuch-new.c | 25 +++++++++++++++++++------ > 1 files changed, 19 insertions(+), 6 deletions(-) > > diff --git a/notmuch-new.c b/notmuch-new.c > index 15c0b36..92e0489 100644 > --- a/notmuch-new.c > +++ b/notmuch-new.c > @@ -785,8 +785,10 @@ remove_filename (notmuch_database_t *notmuch, > add_files_state->renamed_messages++; > if (add_files_state->synchronize_flags == TRUE) > notmuch_message_maildir_flags_to_tags (message); > - } else > + status = NOTMUCH_STATUS_SUCCESS; > + } else if (status == NOTMUCH_STATUS_SUCCESS) { > add_files_state->removed_messages++; > + } > notmuch_message_destroy (message); > notmuch_database_end_atomic (notmuch); > return status; > @@ -794,12 +796,13 @@ remove_filename (notmuch_database_t *notmuch, > > /* Recursively remove all filenames from the database referring to > * 'path' (or to any of its children). */ > -static void > +static notmuch_status_t > _remove_directory (void *ctx, > notmuch_database_t *notmuch, > const char *path, > add_files_state_t *add_files_state) > { > + notmuch_status_t status = NOTMUCH_STATUS_SUCCESS; > notmuch_directory_t *directory; > notmuch_filenames_t *files, *subdirs; > char *absolute; > @@ -812,8 +815,10 @@ _remove_directory (void *ctx, > { > absolute = talloc_asprintf (ctx, "%s/%s", path, > notmuch_filenames_get (files)); > - remove_filename (notmuch, absolute, add_files_state); > + status = remove_filename (notmuch, absolute, add_files_state); > talloc_free (absolute); > + if (status) > + goto DONE; > } > > for (subdirs = notmuch_directory_get_child_directories (directory); > @@ -822,11 +827,15 @@ _remove_directory (void *ctx, > { > absolute = talloc_asprintf (ctx, "%s/%s", path, > notmuch_filenames_get (subdirs)); > - _remove_directory (ctx, notmuch, absolute, add_files_state); > + status = _remove_directory (ctx, notmuch, absolute, add_files_state); > talloc_free (absolute); > + if (status) > + goto DONE; > } > > + DONE: > notmuch_directory_destroy (directory); > + return NOTMUCH_STATUS_SUCCESS; Doesn't this need to be return status or something? Best wishes Mark > } > > int > @@ -944,7 +953,9 @@ notmuch_new_command (void *ctx, int argc, char *argv[]) > > gettimeofday (&tv_start, NULL); > for (f = add_files_state.removed_files->head; f && !interrupted; f = f->next) { > - remove_filename (notmuch, f->filename, &add_files_state); > + ret = remove_filename (notmuch, f->filename, &add_files_state); > + if (ret) > + goto DONE; > if (do_print_progress) { > do_print_progress = 0; > generic_print_progress ("Cleaned up", "messages", > @@ -955,7 +966,9 @@ notmuch_new_command (void *ctx, int argc, char *argv[]) > > gettimeofday (&tv_start, NULL); > for (f = add_files_state.removed_directories->head, i = 0; f && !interrupted; f = f->next, i++) { > - _remove_directory (ctx, notmuch, f->filename, &add_files_state); > + ret = _remove_directory (ctx, notmuch, f->filename, &add_files_state); > + if (ret) > + goto DONE; > if (do_print_progress) { > do_print_progress = 0; > generic_print_progress ("Cleaned up", "directories", > -- > 1.7.9.1 > > _______________________________________________ > notmuch mailing list > notmuch@notmuchmail.org > http://notmuchmail.org/mailman/listinfo/notmuch ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 2/4] new: Handle fatal errors in remove_filename and _remove_directory 2012-04-22 7:34 ` Mark Walters @ 2012-04-22 15:36 ` Austin Clements 0 siblings, 0 replies; 22+ messages in thread From: Austin Clements @ 2012-04-22 15:36 UTC (permalink / raw) To: Mark Walters; +Cc: notmuch Quoth Mark Walters on Apr 22 at 8:34 am: > On Sun, 22 Apr 2012, Austin Clements <amdragon@MIT.EDU> wrote: > > Previously such errors were simply ignored. Now they cause an > > immediate cleanup and abort. > > --- > > notmuch-new.c | 25 +++++++++++++++++++------ > > 1 files changed, 19 insertions(+), 6 deletions(-) > > > > diff --git a/notmuch-new.c b/notmuch-new.c > > index 15c0b36..92e0489 100644 > > --- a/notmuch-new.c > > +++ b/notmuch-new.c > > @@ -785,8 +785,10 @@ remove_filename (notmuch_database_t *notmuch, > > add_files_state->renamed_messages++; > > if (add_files_state->synchronize_flags == TRUE) > > notmuch_message_maildir_flags_to_tags (message); > > - } else > > + status = NOTMUCH_STATUS_SUCCESS; > > + } else if (status == NOTMUCH_STATUS_SUCCESS) { > > add_files_state->removed_messages++; > > + } > > notmuch_message_destroy (message); > > notmuch_database_end_atomic (notmuch); > > return status; > > @@ -794,12 +796,13 @@ remove_filename (notmuch_database_t *notmuch, > > > > /* Recursively remove all filenames from the database referring to > > * 'path' (or to any of its children). */ > > -static void > > +static notmuch_status_t > > _remove_directory (void *ctx, > > notmuch_database_t *notmuch, > > const char *path, > > add_files_state_t *add_files_state) > > { > > + notmuch_status_t status = NOTMUCH_STATUS_SUCCESS; > > notmuch_directory_t *directory; > > notmuch_filenames_t *files, *subdirs; > > char *absolute; > > @@ -812,8 +815,10 @@ _remove_directory (void *ctx, > > { > > absolute = talloc_asprintf (ctx, "%s/%s", path, > > notmuch_filenames_get (files)); > > - remove_filename (notmuch, absolute, add_files_state); > > + status = remove_filename (notmuch, absolute, add_files_state); > > talloc_free (absolute); > > + if (status) > > + goto DONE; > > } > > > > for (subdirs = notmuch_directory_get_child_directories (directory); > > @@ -822,11 +827,15 @@ _remove_directory (void *ctx, > > { > > absolute = talloc_asprintf (ctx, "%s/%s", path, > > notmuch_filenames_get (subdirs)); > > - _remove_directory (ctx, notmuch, absolute, add_files_state); > > + status = _remove_directory (ctx, notmuch, absolute, add_files_state); > > talloc_free (absolute); > > + if (status) > > + goto DONE; > > } > > > > + DONE: > > notmuch_directory_destroy (directory); > > + return NOTMUCH_STATUS_SUCCESS; > > Doesn't this need to be return status or something? Arrg. Yes, of course. I wish I knew a good way to test this stuff. ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v2 3/4] new: Print final fatal error message to stderr 2012-04-22 4:27 ` [PATCH v2 0/4] Fix some error handling in notmuch new Austin Clements 2012-04-22 4:27 ` [PATCH v2 1/4] new: Consistently treat fatal errors as fatal Austin Clements 2012-04-22 4:27 ` [PATCH v2 2/4] new: Handle fatal errors in remove_filename and _remove_directory Austin Clements @ 2012-04-22 4:27 ` Austin Clements 2012-04-22 4:27 ` [PATCH v2 4/4] new: Fix missing end_atomic in remove_filename on error Austin Clements 3 siblings, 0 replies; 22+ messages in thread From: Austin Clements @ 2012-04-22 4:27 UTC (permalink / raw) To: notmuch This was going to stdout. I removed the newline at the beginning of printing the fatal error message because it wouldn't make sense if you were only looking at the stderr stream (e.g., you had redirected stdout to /dev/null). --- notmuch-new.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-) diff --git a/notmuch-new.c b/notmuch-new.c index 92e0489..2103b18 100644 --- a/notmuch-new.c +++ b/notmuch-new.c @@ -1035,8 +1035,8 @@ notmuch_new_command (void *ctx, int argc, char *argv[]) printf ("\n"); if (ret) - printf ("\nNote: A fatal error was encountered: %s\n", - notmuch_status_to_string (ret)); + fprintf (stderr, "Note: A fatal error was encountered: %s\n", + notmuch_status_to_string (ret)); notmuch_database_close (notmuch); -- 1.7.9.1 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH v2 4/4] new: Fix missing end_atomic in remove_filename on error 2012-04-22 4:27 ` [PATCH v2 0/4] Fix some error handling in notmuch new Austin Clements ` (2 preceding siblings ...) 2012-04-22 4:27 ` [PATCH v2 3/4] new: Print final fatal error message to stderr Austin Clements @ 2012-04-22 4:27 ` Austin Clements 2012-04-22 7:42 ` Mark Walters 3 siblings, 1 reply; 22+ messages in thread From: Austin Clements @ 2012-04-22 4:27 UTC (permalink / raw) To: notmuch Previously, if we failed to find the message by filename in remove_filename, we would return immediately from the function without ending its atomic block. Now this code follows the usual goto DONE idiom to perform cleanup. --- notmuch-new.c | 5 ++++- 1 files changed, 4 insertions(+), 1 deletions(-) diff --git a/notmuch-new.c b/notmuch-new.c index 2103b18..9eebea4 100644 --- a/notmuch-new.c +++ b/notmuch-new.c @@ -779,7 +779,8 @@ remove_filename (notmuch_database_t *notmuch, return status; status = notmuch_database_find_message_by_filename (notmuch, path, &message); if (status || message == NULL) - return status; + goto DONE; + status = notmuch_database_remove_message (notmuch, path); if (status == NOTMUCH_STATUS_DUPLICATE_MESSAGE_ID) { add_files_state->renamed_messages++; @@ -790,6 +791,8 @@ remove_filename (notmuch_database_t *notmuch, add_files_state->removed_messages++; } notmuch_message_destroy (message); + + DONE: notmuch_database_end_atomic (notmuch); return status; } -- 1.7.9.1 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH v2 4/4] new: Fix missing end_atomic in remove_filename on error 2012-04-22 4:27 ` [PATCH v2 4/4] new: Fix missing end_atomic in remove_filename on error Austin Clements @ 2012-04-22 7:42 ` Mark Walters 0 siblings, 0 replies; 22+ messages in thread From: Mark Walters @ 2012-04-22 7:42 UTC (permalink / raw) To: Austin Clements, notmuch On Sun, 22 Apr 2012, Austin Clements <amdragon@MIT.EDU> wrote: > Previously, if we failed to find the message by filename in > remove_filename, we would return immediately from the function without > ending its atomic block. Now this code follows the usual goto DONE > idiom to perform cleanup. The whole series looks good to me modulo the return value in Patch 2/4. Best wishes Mark > --- > notmuch-new.c | 5 ++++- > 1 files changed, 4 insertions(+), 1 deletions(-) > > diff --git a/notmuch-new.c b/notmuch-new.c > index 2103b18..9eebea4 100644 > --- a/notmuch-new.c > +++ b/notmuch-new.c > @@ -779,7 +779,8 @@ remove_filename (notmuch_database_t *notmuch, > return status; > status = notmuch_database_find_message_by_filename (notmuch, path, &message); > if (status || message == NULL) > - return status; > + goto DONE; > + > status = notmuch_database_remove_message (notmuch, path); > if (status == NOTMUCH_STATUS_DUPLICATE_MESSAGE_ID) { > add_files_state->renamed_messages++; > @@ -790,6 +791,8 @@ remove_filename (notmuch_database_t *notmuch, > add_files_state->removed_messages++; > } > notmuch_message_destroy (message); > + > + DONE: > notmuch_database_end_atomic (notmuch); > return status; > } > -- > 1.7.9.1 > > _______________________________________________ > notmuch mailing list > notmuch@notmuchmail.org > http://notmuchmail.org/mailman/listinfo/notmuch ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v3 0/4] Fix some error handling in notmuch new 2012-02-27 15:49 [PATCH 0/3] Fix some error handling in notmuch new Austin Clements ` (3 preceding siblings ...) 2012-04-22 4:27 ` [PATCH v2 0/4] Fix some error handling in notmuch new Austin Clements @ 2012-04-22 15:50 ` Austin Clements 2012-04-22 15:50 ` [PATCH v3 1/4] new: Consistently treat fatal errors as fatal Austin Clements ` (3 more replies) 4 siblings, 4 replies; 22+ messages in thread From: Austin Clements @ 2012-04-22 15:50 UTC (permalink / raw) To: notmuch This version fixes a silly mistake Mark pointed out. No further review is necessary, per id:"877gx8kyad.fsf@qmul.ac.uk". ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v3 1/4] new: Consistently treat fatal errors as fatal 2012-04-22 15:50 ` [PATCH v3 0/4] Fix some error handling in notmuch new Austin Clements @ 2012-04-22 15:50 ` Austin Clements 2012-04-25 2:28 ` David Bremner 2012-04-22 15:50 ` [PATCH v3 2/4] new: Handle fatal errors in remove_filename and _remove_directory Austin Clements ` (2 subsequent siblings) 3 siblings, 1 reply; 22+ messages in thread From: Austin Clements @ 2012-04-22 15:50 UTC (permalink / raw) To: notmuch Previously, fatal errors in add_files_recursive were not treated as fatal by its callers (including itself!). This makes add_files_recursive errors consistently fatal and updates all callers to treat them as fatal. --- notmuch-new.c | 16 ++++++++++++---- 1 files changed, 12 insertions(+), 4 deletions(-) diff --git a/notmuch-new.c b/notmuch-new.c index 4f13535..15c0b36 100644 --- a/notmuch-new.c +++ b/notmuch-new.c @@ -308,6 +308,10 @@ add_files_recursive (notmuch_database_t *notmuch, if (num_fs_entries == -1) { fprintf (stderr, "Error opening directory %s: %s\n", path, strerror (errno)); + /* We consider this a fatal error because, if a user moved a + * message from another directory that we were able to scan + * into this directory, skipping this directory will cause + * that message to be lost. */ ret = NOTMUCH_STATUS_FILE_ERROR; goto DONE; } @@ -351,8 +355,10 @@ add_files_recursive (notmuch_database_t *notmuch, next = talloc_asprintf (notmuch, "%s/%s", path, entry->d_name); status = add_files_recursive (notmuch, next, state); - if (status && ret == NOTMUCH_STATUS_SUCCESS) + if (status) { ret = status; + goto DONE; + } talloc_free (next); next = NULL; } @@ -933,6 +939,8 @@ notmuch_new_command (void *ctx, int argc, char *argv[]) } ret = add_files (notmuch, db_path, &add_files_state); + if (ret) + goto DONE; gettimeofday (&tv_start, NULL); for (f = add_files_state.removed_files->head; f && !interrupted; f = f->next) { @@ -965,6 +973,7 @@ notmuch_new_command (void *ctx, int argc, char *argv[]) } } + DONE: talloc_free (add_files_state.removed_files); talloc_free (add_files_state.removed_directories); talloc_free (add_files_state.directory_mtimes); @@ -1012,10 +1021,9 @@ notmuch_new_command (void *ctx, int argc, char *argv[]) printf ("\n"); - if (ret) { - printf ("\nNote: At least one error was encountered: %s\n", + if (ret) + printf ("\nNote: A fatal error was encountered: %s\n", notmuch_status_to_string (ret)); - } notmuch_database_close (notmuch); -- 1.7.9.1 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH v3 1/4] new: Consistently treat fatal errors as fatal 2012-04-22 15:50 ` [PATCH v3 1/4] new: Consistently treat fatal errors as fatal Austin Clements @ 2012-04-25 2:28 ` David Bremner 0 siblings, 0 replies; 22+ messages in thread From: David Bremner @ 2012-04-25 2:28 UTC (permalink / raw) To: Austin Clements, notmuch Austin Clements <amdragon@MIT.EDU> writes: > Previously, fatal errors in add_files_recursive were not treated as > fatal by its callers (including itself!). This makes > add_files_recursive errors consistently fatal and updates all callers > to treat them as fatal. series pushed. d ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v3 2/4] new: Handle fatal errors in remove_filename and _remove_directory 2012-04-22 15:50 ` [PATCH v3 0/4] Fix some error handling in notmuch new Austin Clements 2012-04-22 15:50 ` [PATCH v3 1/4] new: Consistently treat fatal errors as fatal Austin Clements @ 2012-04-22 15:50 ` Austin Clements 2012-04-22 15:50 ` [PATCH v3 3/4] new: Print final fatal error message to stderr Austin Clements 2012-04-22 15:50 ` [PATCH v3 4/4] new: Fix missing end_atomic in remove_filename on error Austin Clements 3 siblings, 0 replies; 22+ messages in thread From: Austin Clements @ 2012-04-22 15:50 UTC (permalink / raw) To: notmuch Previously such errors were simply ignored. Now they cause an immediate cleanup and abort. --- notmuch-new.c | 25 +++++++++++++++++++------ 1 files changed, 19 insertions(+), 6 deletions(-) diff --git a/notmuch-new.c b/notmuch-new.c index 15c0b36..2faf2f8 100644 --- a/notmuch-new.c +++ b/notmuch-new.c @@ -785,8 +785,10 @@ remove_filename (notmuch_database_t *notmuch, add_files_state->renamed_messages++; if (add_files_state->synchronize_flags == TRUE) notmuch_message_maildir_flags_to_tags (message); - } else + status = NOTMUCH_STATUS_SUCCESS; + } else if (status == NOTMUCH_STATUS_SUCCESS) { add_files_state->removed_messages++; + } notmuch_message_destroy (message); notmuch_database_end_atomic (notmuch); return status; @@ -794,12 +796,13 @@ remove_filename (notmuch_database_t *notmuch, /* Recursively remove all filenames from the database referring to * 'path' (or to any of its children). */ -static void +static notmuch_status_t _remove_directory (void *ctx, notmuch_database_t *notmuch, const char *path, add_files_state_t *add_files_state) { + notmuch_status_t status = NOTMUCH_STATUS_SUCCESS; notmuch_directory_t *directory; notmuch_filenames_t *files, *subdirs; char *absolute; @@ -812,8 +815,10 @@ _remove_directory (void *ctx, { absolute = talloc_asprintf (ctx, "%s/%s", path, notmuch_filenames_get (files)); - remove_filename (notmuch, absolute, add_files_state); + status = remove_filename (notmuch, absolute, add_files_state); talloc_free (absolute); + if (status) + goto DONE; } for (subdirs = notmuch_directory_get_child_directories (directory); @@ -822,11 +827,15 @@ _remove_directory (void *ctx, { absolute = talloc_asprintf (ctx, "%s/%s", path, notmuch_filenames_get (subdirs)); - _remove_directory (ctx, notmuch, absolute, add_files_state); + status = _remove_directory (ctx, notmuch, absolute, add_files_state); talloc_free (absolute); + if (status) + goto DONE; } + DONE: notmuch_directory_destroy (directory); + return status; } int @@ -944,7 +953,9 @@ notmuch_new_command (void *ctx, int argc, char *argv[]) gettimeofday (&tv_start, NULL); for (f = add_files_state.removed_files->head; f && !interrupted; f = f->next) { - remove_filename (notmuch, f->filename, &add_files_state); + ret = remove_filename (notmuch, f->filename, &add_files_state); + if (ret) + goto DONE; if (do_print_progress) { do_print_progress = 0; generic_print_progress ("Cleaned up", "messages", @@ -955,7 +966,9 @@ notmuch_new_command (void *ctx, int argc, char *argv[]) gettimeofday (&tv_start, NULL); for (f = add_files_state.removed_directories->head, i = 0; f && !interrupted; f = f->next, i++) { - _remove_directory (ctx, notmuch, f->filename, &add_files_state); + ret = _remove_directory (ctx, notmuch, f->filename, &add_files_state); + if (ret) + goto DONE; if (do_print_progress) { do_print_progress = 0; generic_print_progress ("Cleaned up", "directories", -- 1.7.9.1 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH v3 3/4] new: Print final fatal error message to stderr 2012-04-22 15:50 ` [PATCH v3 0/4] Fix some error handling in notmuch new Austin Clements 2012-04-22 15:50 ` [PATCH v3 1/4] new: Consistently treat fatal errors as fatal Austin Clements 2012-04-22 15:50 ` [PATCH v3 2/4] new: Handle fatal errors in remove_filename and _remove_directory Austin Clements @ 2012-04-22 15:50 ` Austin Clements 2012-04-22 15:50 ` [PATCH v3 4/4] new: Fix missing end_atomic in remove_filename on error Austin Clements 3 siblings, 0 replies; 22+ messages in thread From: Austin Clements @ 2012-04-22 15:50 UTC (permalink / raw) To: notmuch This was going to stdout. I removed the newline at the beginning of printing the fatal error message because it wouldn't make sense if you were only looking at the stderr stream (e.g., you had redirected stdout to /dev/null). --- notmuch-new.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-) diff --git a/notmuch-new.c b/notmuch-new.c index 2faf2f8..bf9b120 100644 --- a/notmuch-new.c +++ b/notmuch-new.c @@ -1035,8 +1035,8 @@ notmuch_new_command (void *ctx, int argc, char *argv[]) printf ("\n"); if (ret) - printf ("\nNote: A fatal error was encountered: %s\n", - notmuch_status_to_string (ret)); + fprintf (stderr, "Note: A fatal error was encountered: %s\n", + notmuch_status_to_string (ret)); notmuch_database_close (notmuch); -- 1.7.9.1 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH v3 4/4] new: Fix missing end_atomic in remove_filename on error 2012-04-22 15:50 ` [PATCH v3 0/4] Fix some error handling in notmuch new Austin Clements ` (2 preceding siblings ...) 2012-04-22 15:50 ` [PATCH v3 3/4] new: Print final fatal error message to stderr Austin Clements @ 2012-04-22 15:50 ` Austin Clements 3 siblings, 0 replies; 22+ messages in thread From: Austin Clements @ 2012-04-22 15:50 UTC (permalink / raw) To: notmuch Previously, if we failed to find the message by filename in remove_filename, we would return immediately from the function without ending its atomic block. Now this code follows the usual goto DONE idiom to perform cleanup. --- notmuch-new.c | 5 ++++- 1 files changed, 4 insertions(+), 1 deletions(-) diff --git a/notmuch-new.c b/notmuch-new.c index bf9b120..473201e 100644 --- a/notmuch-new.c +++ b/notmuch-new.c @@ -779,7 +779,8 @@ remove_filename (notmuch_database_t *notmuch, return status; status = notmuch_database_find_message_by_filename (notmuch, path, &message); if (status || message == NULL) - return status; + goto DONE; + status = notmuch_database_remove_message (notmuch, path); if (status == NOTMUCH_STATUS_DUPLICATE_MESSAGE_ID) { add_files_state->renamed_messages++; @@ -790,6 +791,8 @@ remove_filename (notmuch_database_t *notmuch, add_files_state->removed_messages++; } notmuch_message_destroy (message); + + DONE: notmuch_database_end_atomic (notmuch); return status; } -- 1.7.9.1 ^ permalink raw reply related [flat|nested] 22+ messages in thread
end of thread, other threads:[~2012-04-25 2:28 UTC | newest] Thread overview: 22+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-02-27 15:49 [PATCH 0/3] Fix some error handling in notmuch new Austin Clements 2012-02-27 15:49 ` [PATCH 1/3] new: Consistently treat fatal errors as fatal Austin Clements 2012-04-16 15:53 ` Mark Walters 2012-04-22 4:11 ` Austin Clements 2012-02-27 15:49 ` [PATCH 2/3] new: Handle fatal errors in remove_filename and _remove_directory Austin Clements 2012-04-16 16:02 ` Mark Walters 2012-04-22 4:21 ` Austin Clements 2012-02-27 15:49 ` [PATCH 3/3] new: Print final fatal error message to stderr Austin Clements 2012-04-22 4:27 ` [PATCH v2 0/4] Fix some error handling in notmuch new Austin Clements 2012-04-22 4:27 ` [PATCH v2 1/4] new: Consistently treat fatal errors as fatal Austin Clements 2012-04-22 4:27 ` [PATCH v2 2/4] new: Handle fatal errors in remove_filename and _remove_directory Austin Clements 2012-04-22 7:34 ` Mark Walters 2012-04-22 15:36 ` Austin Clements 2012-04-22 4:27 ` [PATCH v2 3/4] new: Print final fatal error message to stderr Austin Clements 2012-04-22 4:27 ` [PATCH v2 4/4] new: Fix missing end_atomic in remove_filename on error Austin Clements 2012-04-22 7:42 ` Mark Walters 2012-04-22 15:50 ` [PATCH v3 0/4] Fix some error handling in notmuch new Austin Clements 2012-04-22 15:50 ` [PATCH v3 1/4] new: Consistently treat fatal errors as fatal Austin Clements 2012-04-25 2:28 ` David Bremner 2012-04-22 15:50 ` [PATCH v3 2/4] new: Handle fatal errors in remove_filename and _remove_directory Austin Clements 2012-04-22 15:50 ` [PATCH v3 3/4] new: Print final fatal error message to stderr Austin Clements 2012-04-22 15:50 ` [PATCH v3 4/4] new: Fix missing end_atomic in remove_filename on error Austin Clements
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).