From ddbfa03acd5c9766f2fb99290856c8bb5de93649 Mon Sep 17 00:00:00 2001 From: James Youngman Date: Sun, 4 Apr 2010 17:14:31 +0100 Subject: [PATCH 2/4] Don't issue an error message twice for the same target file. To: address@hidden * find/defs.h (struct state): New member, already_issued_stat_error_msg, used to de-duplicate error messages. If it is true, we already issued an error message for the current target file. Declare fatal_target_file_error, fatal_nontarget_file_error, nonfatal_target_file_error, nonfatal_nontarget_file_error. Between them, they replace fatal_file_error and nonfatal_file_error. The *target_file_error versions are de-duplicated and the nontarget_file_error_versions are not. * find/util.c (nonfatal_nontarget_file_error): Implement. (fatal_nontarget_file_error): Implement. (nonfatal_target_file_error): Implement. (fatal_target_file_error): Implement. (fatal_file_error): Remove. (nonfatal_file_error): Remove. (error_severity): Define error_severity (moved from ftsfind.c) (get_statinfo): Call nonfatal_target_file_error in order to avoid a duplicate message. ALso call error_severity after a different call to error to preserve the constraint that we exit with a nonzero status if we issue a diagnostic. (cleanup): Call nonfatal_nontarget_file_error instead of nonfatal_file_error. * find/ftsfind.c (error_severity): Move to util.c. * find/tree.c (build_expression_tree): Initialise state.already_issued_stat_error_msg. * find/find.c (main): Initialise state.already_issued_stat_error_msg. (wd_sanity_check): Call fatal_target_file_error instead of fatal_file_error. (chdir_back): Call fatal_nontarget_file_error instead of fatal_file_error. (process_path): Initialise state.already_issued_stat_error_msg. * find/ftsfind.c (consider_visiting): Call nonfatal_target_file_error instead of calling error directly. (find): Call error_severity to ensure exit status is nonzero after a call to error. (main): Initialise state.already_issued_stat_error_msg. * find/parser.c (collect_arg_stat_info): Call fatal_target_file_error instead of fatal_file_error. (parse_newerXY): Likewise. (parse_samefile): Likewise. (parse_samefile): Likewise. (open_output_file): Call fatal_nontarget_file_error instead of fatal_file_error. * find/pred.c (checked_fprintf): Likewise. (checked_print_quoted): Likewise. (checked_fwrite): Likewise. (checked_fflush): Likewise. * find/sharefile.c (entry_free): Likewise. Signed-off-by: James Youngman --- ChangeLog | 60 +++++++++++++++++++++++++++++++++++++++++ find/defs.h | 12 +++++++- find/find.c | 8 +++-- find/ftsfind.c | 42 +++++++--------------------- find/parser.c | 10 +++--- find/pred.c | 8 +++--- find/sharefile.c | 2 +- find/tree.c | 1 + find/util.c | 78 ++++++++++++++++++++++++++++++++++++++++++------------ 9 files changed, 158 insertions(+), 63 deletions(-) diff --git a/ChangeLog b/ChangeLog index d044174..29bde02 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,5 +1,65 @@ 2010-04-05 James Youngman + Don't issue an error message twice for the same target file. + * find/defs.h (struct state): New member, + already_issued_stat_error_msg, used to de-duplicate error + messages. If it is true, we already issued an error message for + the current target file. + Declare fatal_target_file_error, fatal_nontarget_file_error, + nonfatal_target_file_error, nonfatal_nontarget_file_error. + Between them, they replace fatal_file_error and + nonfatal_file_error. The *target_file_error versions are + de-duplicated and the nontarget_file_error_versions are not. + * find/util.c (nonfatal_nontarget_file_error): Implement. + (fatal_nontarget_file_error): Implement. + (nonfatal_target_file_error): Implement. + (fatal_target_file_error): Implement. + (fatal_file_error): Remove. + (nonfatal_file_error): Remove. + (error_severity): Define error_severity (moved from ftsfind.c) + (get_statinfo): Call nonfatal_target_file_error in order to avoid + a duplicate message. ALso call error_severity after a different + call to error to preserve the constraint that we exit with a + nonzero status if we issue a diagnostic. + (cleanup): Call nonfatal_nontarget_file_error instead of + nonfatal_file_error. + * find/ftsfind.c (error_severity): Move to util.c. + * find/tree.c (build_expression_tree): Initialise + state.already_issued_stat_error_msg. + * find/find.c (main): Initialise state.already_issued_stat_error_msg. + (wd_sanity_check): Call fatal_target_file_error instead of + fatal_file_error. + (chdir_back): Call fatal_nontarget_file_error instead of + fatal_file_error. + (process_path): Initialise state.already_issued_stat_error_msg. + * find/ftsfind.c (consider_visiting): Call + nonfatal_target_file_error instead of calling error directly. + (find): Call error_severity to ensure exit status is nonzero after + a call to error. + (main): Initialise state.already_issued_stat_error_msg. + * find/parser.c (collect_arg_stat_info): Call + fatal_target_file_error instead of fatal_file_error. + (parse_newerXY): Likewise. + (parse_samefile): Likewise. + (parse_samefile): Likewise. + (open_output_file): Call fatal_nontarget_file_error instead of + fatal_file_error. + * find/pred.c (checked_fprintf): Likewise. + (checked_print_quoted): Likewise. + (checked_fwrite): Likewise. + (checked_fflush): Likewise. + * find/sharefile.c (entry_free): Likewise. + + Fix Savannah bug #29435: fd_is_cloexec does not work on Fedora + buildhosts. + Fix open_cloexec on hosts which ignore O_CLOEXEC (i.e. old kernels). + * lib/fdleak.c (o_cloexec_works): New function, detects whether + the open flag O_CLOEXEC has any effect. + (open_cloexec): Call o_cloexec_works, just once, to find out + whether O_CLOEXEC is effective. If not, set the close-on-exec + flag on fds by calling set_cloexec_flag. + * NEWS: Mention this bugfix. + Use bool instead of the previous typedef boolean. * find/defs.h: Don't create typedef "boolean"; use the standard type bool. Update other declarations accordingly. diff --git a/find/defs.h b/find/defs.h index f0750da..e7892f3 100644 --- a/find/defs.h +++ b/find/defs.h @@ -502,11 +502,16 @@ extern bool check_nofollow(void); void complete_pending_execs(struct predicate *p); void complete_pending_execdirs(int dir_fd); /* Passing dir_fd is an unpleasant CodeSmell. */ const char *safely_quote_err_filename (int n, char const *arg); -void fatal_file_error(const char *name) ATTRIBUTE_NORETURN; -void nonfatal_file_error(const char *name); + +void fatal_target_file_error (int errno_value, const char *name) ATTRIBUTE_NORETURN; +void fatal_nontarget_file_error (int errno_value, const char *name) ATTRIBUTE_NORETURN; +void nonfatal_target_file_error (int erron_value, const char *name); +void nonfatal_nontarget_file_error (int erron_value, const char *name); + int process_leading_options PARAMS((int argc, char *argv[])); void set_option_defaults PARAMS((struct options *p)); +void error_severity PARAMS((int level)); #if 0 #define apply_predicate(pathname, stat_buf_ptr, node) \ @@ -664,6 +669,9 @@ struct state /* Shared files, opened via the interface in sharefile.h. */ sharefile_handle shared_files; + + /* Avoid multiple error messages for the same file. */ + boolean already_issued_stat_error_msg; }; /* finddata.c */ diff --git a/find/find.c b/find/find.c index f5be91d..04e2968 100644 --- a/find/find.c +++ b/find/find.c @@ -136,6 +136,7 @@ main (int argc, char **argv) remember_non_cloexec_fds (); } + state.already_issued_stat_error_msg = false; state.shared_files = sharefile_init ("w"); if (NULL == state.shared_files) { @@ -473,7 +474,7 @@ wd_sanity_check (const char *thing_to_stat, set_stat_placeholders (newinfo); if ((*options.xstat) (current_dir, newinfo) != 0) - fatal_file_error (thing_to_stat); + fatal_target_file_error (errno, thing_to_stat); if (old_dev != newinfo->st_dev) { @@ -944,7 +945,7 @@ chdir_back (void) #endif if (chdir (starting_dir) != 0) - fatal_file_error (starting_dir); + fatal_nontarget_file_error (errno, starting_dir); wd_sanity_check (starting_dir, program_name, @@ -963,7 +964,7 @@ chdir_back (void) if (fchdir (starting_desc) != 0) { - fatal_file_error (starting_dir); + fatal_nontarget_file_error (errno, starting_dir); } } } @@ -1184,6 +1185,7 @@ process_path (char *pathname, char *name, bool leaf, char *parent, state.type = 0; state.have_stat = false; state.have_type = false; + state.already_issued_stat_error_msg = false; if (!digest_mode (&mode, pathname, name, &stat_buf, leaf)) return 0; diff --git a/find/ftsfind.c b/find/ftsfind.c index 4b9dca2..aa086a4 100644 --- a/find/ftsfind.c +++ b/find/ftsfind.c @@ -173,18 +173,6 @@ inside_dir (int dir_fd) static void init_mounted_dev_list (void); #endif -/* We have encountered an error which should affect the exit status. - * This is normally used to change the exit status from 0 to 1. - * However, if the exit status is already 2 for example, we don't want to - * reduce it to 1. - */ -static void -error_severity (int level) -{ - if (state.exit_status < level) - state.exit_status = level; -} - #define STRINGIFY(X) #X #define HANDLECASE(N) case N: return #N; @@ -371,9 +359,6 @@ show_outstanding_execdirs (FILE *fp) /* No debug output is wanted. */ } } - - - static void consider_visiting (FTS *p, FTSENT *ent) @@ -410,15 +395,13 @@ consider_visiting (FTS *p, FTSENT *ent) if (ent->fts_info == FTS_ERR || ent->fts_info == FTS_DNR) { - error (0, ent->fts_errno, "%s", - safely_quote_err_filename (0, ent->fts_path)); - error_severity (1); + nonfatal_target_file_error (ent->fts_errno, ent->fts_path); return; } else if (ent->fts_info == FTS_DC) { issue_loop_warning (ent); - error_severity (1); + error_severity (EXIT_FAILURE); return; } else if (ent->fts_info == FTS_SLNONE) @@ -432,8 +415,7 @@ consider_visiting (FTS *p, FTSENT *ent) */ if (symlink_loop (ent->fts_accpath)) { - error (0, ELOOP, "%s", safely_quote_err_filename (0, ent->fts_path)); - error_severity (1); + nonfatal_target_file_error (ELOOP, ent->fts_path); return; } } @@ -442,9 +424,7 @@ consider_visiting (FTS *p, FTSENT *ent) if (ent->fts_level == 0) { /* e.g., nonexistent starting point */ - error (0, ent->fts_errno, "%s", - safely_quote_err_filename (0, ent->fts_path)); - error_severity (1); /* remember problem */ + nonfatal_target_file_error (ent->fts_errno, ent->fts_path); return; } else @@ -454,16 +434,12 @@ consider_visiting (FTS *p, FTSENT *ent) */ if (symlink_loop (ent->fts_accpath)) { - error (0, ELOOP, "%s", - safely_quote_err_filename (0, ent->fts_path)); - error_severity (1); + nonfatal_target_file_error (ELOOP, ent->fts_path); return; } else { - error(0, ent->fts_errno, "%s", - safely_quote_err_filename(0, ent->fts_path)); - error_severity(1); + nonfatal_target_file_error (ent->fts_errno, ent->fts_path); /* Continue despite the error, as file name without stat info * might be better than not even processing the file name. This * can lead to repeated error messages later on, though, if a @@ -475,7 +451,7 @@ consider_visiting (FTS *p, FTSENT *ent) * such. */ } - + } } @@ -632,11 +608,13 @@ find (char *arg) { error (0, errno, _("cannot search %s"), safely_quote_err_filename (0, arg)); + error_severity (EXIT_FAILURE); } else { while ( (ent=fts_read (p)) != NULL ) { + state.already_issued_stat_error_msg = false; state.have_stat = false; state.have_type = !!ent->fts_statp->st_mode; state.type = state.have_type ? ent->fts_statp->st_mode : 0; @@ -651,6 +629,7 @@ find (char *arg) error (0, errno, _("failed to restore working directory after searching %s"), arg); + error_severity (EXIT_FAILURE); return false; } p = NULL; @@ -700,6 +679,7 @@ main (int argc, char **argv) else set_program_name ("find"); + state.already_issued_stat_error_msg = false; state.exit_status = 0; state.execdirs_outstanding = false; state.cwd_dir_fd = AT_FDCWD; diff --git a/find/parser.c b/find/parser.c index bb09ed0..44dff55 100644 --- a/find/parser.c +++ b/find/parser.c @@ -742,7 +742,7 @@ collect_arg_stat_info (char **argv, int *arg_ptr, struct stat *p, } else { - fatal_file_error (filename); + fatal_target_file_error (errno, filename); } } else @@ -1683,7 +1683,7 @@ parse_newerXY (const struct parser_table* entry, char **argv, int *arg_ptr) /* Stat the named file. */ set_stat_placeholders (&stat_newer); if ((*options.xstat) (argv[*arg_ptr], &stat_newer)) - fatal_file_error (argv[*arg_ptr]); + fatal_target_file_error (errno, argv[*arg_ptr]); if (!get_stat_Ytime (&stat_newer, y, &our_pred->args.reftime.ts)) { @@ -2423,7 +2423,7 @@ parse_samefile (const struct parser_table* entry, char **argv, int *arg_ptr) */ if (0 != fstat (fd, &fst)) { - fatal_file_error (argv[*arg_ptr]); + fatal_target_file_error (errno, argv[*arg_ptr]); } else { @@ -2433,7 +2433,7 @@ parse_samefile (const struct parser_table* entry, char **argv, int *arg_ptr) * destination of the link, not the link itself. */ if ((*options.xstat) (argv[*arg_ptr], &st)) - fatal_file_error (argv[*arg_ptr]); + fatal_target_file_error (errno, argv[*arg_ptr]); if ((options.symlink_handling == SYMLINK_NEVER_DEREF) && (!options.open_nofollow_available)) @@ -3787,7 +3787,7 @@ open_output_file (const char *path, struct format_val *p) if (p->stream == NULL) { - fatal_file_error (path); + fatal_nontarget_file_error (errno, path); } } diff --git a/find/pred.c b/find/pred.c index 6850372..918db2c 100644 --- a/find/pred.c +++ b/find/pred.c @@ -696,7 +696,7 @@ checked_fprintf (struct format_val *dest, const char *fmt, ...) va_start (ap, fmt); rv = vfprintf (dest->stream, fmt, ap); if (rv < 0) - nonfatal_file_error (dest->filename); + nonfatal_nontarget_file_error (errno, dest->filename); } @@ -707,7 +707,7 @@ checked_print_quoted (struct format_val *dest, int rv = print_quoted (dest->stream, dest->quote_opts, dest->dest_is_tty, format, s); if (rv < 0) - nonfatal_file_error (dest->filename); + nonfatal_nontarget_file_error (errno, dest->filename); } @@ -716,7 +716,7 @@ checked_fwrite (void *p, size_t siz, size_t nmemb, struct format_val *dest) { int items_written = fwrite (p, siz, nmemb, dest->stream); if (items_written < nmemb) - nonfatal_file_error (dest->filename); + nonfatal_nontarget_file_error (errno, dest->filename); } static void @@ -724,7 +724,7 @@ checked_fflush (struct format_val *dest) { if (0 != fflush (dest->stream)) { - nonfatal_file_error (dest->filename); + nonfatal_nontarget_file_error (errno, dest->filename); } } diff --git a/find/sharefile.c b/find/sharefile.c index 8e0f4cc..1442d7b 100644 --- a/find/sharefile.c +++ b/find/sharefile.c @@ -76,7 +76,7 @@ entry_free (void *pv) if (p->fp) { if (0 != fclose (p->fp)) - fatal_file_error (p->name); + fatal_nontarget_file_error (errno, p->name); } free (p->name); free (p); diff --git a/find/tree.c b/find/tree.c index 15e3129..a9cf3d7 100644 --- a/find/tree.c +++ b/find/tree.c @@ -1284,6 +1284,7 @@ build_expression_tree (int argc, char *argv[], int end_of_leading_options) /* Build the input order list. */ while (i < argc ) { + state.already_issued_stat_error_msg = false; if (!looks_like_expression (argv[i], false)) { error (0, 0, _("paths must precede expression: %s"), argv[i]); diff --git a/find/util.c b/find/util.c index 9693e57..62d80c8 100644 --- a/find/util.c +++ b/find/util.c @@ -210,18 +210,14 @@ get_statinfo (const char *pathname, const char *name, struct stat *p) /* Savannah bug #16378. */ error (0, 0, _("WARNING: file %s appears to have mode 0000"), quotearg_n_style (0, options.err_quoting_style, name)); + error_severity (1); } } else { if (!options.ignore_readdir_race || (errno != ENOENT) ) { - /* FIXME: this error message might repeat the one from - * the FTS_NS case in consider_visiting. How to avoid this? - */ - error (0, errno, "%s", - safely_quote_err_filename (0, pathname)); - state.exit_status = 1; + nonfatal_target_file_error (errno, pathname); } return -1; } @@ -489,7 +485,7 @@ cleanup (void) } if (fflush (stdout) == EOF) - nonfatal_file_error ("standard output"); + nonfatal_nontarget_file_error (errno, "standard output"); } /* Savannah bug #16378 manifests as an assertion failure in pred_type() @@ -1077,35 +1073,83 @@ safely_quote_err_filename (int n, char const *arg) return quotearg_n_style (n, options.err_quoting_style, arg); } +/* We have encountered an error which should affect the exit status. + * This is normally used to change the exit status from 0 to 1. + * However, if the exit status is already 2 for example, we don't want to + * reduce it to 1. + */ +void +error_severity (int level) +{ + if (state.exit_status < level) + state.exit_status = level; +} + /* report_file_err */ static void -report_file_err(int exitval, int errno_value, const char *name) +report_file_err(int exitval, int errno_value, + boolean is_target_file, const char *name) { /* It is important that the errno value is passed in as a function * argument before we call safely_quote_err_filename(), because otherwise * we might find that safely_quote_err_filename() changes errno. */ - if (state.exit_status < 1) - state.exit_status = 1; - - error (exitval, errno_value, "%s", safely_quote_err_filename (0, name)); + if (!is_target_file || !state.already_issued_stat_error_msg) + { + error (exitval, errno_value, "%s", safely_quote_err_filename (0, name)); + error_severity (1); + } + if (is_target_file) + { + state.already_issued_stat_error_msg = true; + } } -/* fatal_file_error +/* nonfatal_target_file_error * */ void -fatal_file_error(const char *name) +nonfatal_target_file_error (int errno_value, const char *name) { - report_file_err (1, errno, name); + report_file_err (0, errno_value, true, name); +} + +/* fatal_target_file_error + * + * Report an error on a target file (i.e. a file we are searching). + * Such errors are only reported once per searched file. + * + */ +void +fatal_target_file_error(int errno_value, const char *name) +{ + report_file_err (1, errno_value, true, name); /*NOTREACHED*/ abort (); } +/* nonfatal_nontarget_file_error + * + */ void -nonfatal_file_error (const char *name) +nonfatal_nontarget_file_error (int errno_value, const char *name) { - report_file_err (0, errno, name); + report_file_err (0, errno_value, false, name); } +/* fatal_nontarget_file_error + * + */ +void +fatal_nontarget_file_error(int errno_value, const char *name) +{ + /* We're going to exit fatally, so make sure we always isssue the error + * message, even if it will be duplicate. Motivation: otherwise it may + * not be clear what went wrong. + */ + state.already_issued_stat_error_msg = false; + report_file_err (1, errno_value, false, name); + /*NOTREACHED*/ + abort (); +} -- 1.7.0