[PATCH 1/3] new: Consistently treat fatal errors as fatal
Quoth Mark Walters on Apr 16 at 4:53 pm:
>
> On Mon, 27 Feb 2012, Austin Clements 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);
Re: [PATCH 1/3] new: Consistently treat fatal errors as fatal
Quoth Mark Walters on Apr 16 at 4:53 pm:
>
> On Mon, 27 Feb 2012, Austin Clements 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);
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch
[PATCH 1/3] new: Consistently treat fatal errors as fatal
On Mon, 27 Feb 2012, Austin Clements 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 at notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch
Re: [PATCH 1/3] new: Consistently treat fatal errors as fatal
On Mon, 27 Feb 2012, Austin Clements 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
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch
[PATCH 1/3] new: Consistently treat fatal errors as fatal
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
[PATCH 1/3] new: Consistently treat fatal errors as fatal
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
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch
